Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not include source files and/or name include files properly #32

Open
dvrogozh opened this issue Sep 13, 2018 · 11 comments
Open

Do not include source files and/or name include files properly #32

dvrogozh opened this issue Sep 13, 2018 · 11 comments

Comments

@dvrogozh
Copy link
Contributor

#include "../../../Utility/CpuSwizzleBlt/CpuSwizzleBlt.c"

Please, do not include source files and/or name include files properly. Other places may exist.

@drprajap
Copy link
Contributor

This source file is a special inclusion which works as .c and header file as well depending on purpose...it is included by design and is intentional.

@chivakker
Copy link

chivakker commented Sep 13, 2018

@drprajap can the relative directory #includes be fixed as well? If your cmake is already including -I 'pkg-config -cflags igdgmm', then all includes should relate on that absolute directory and includes should look like:

#include "Utility/CpuSwizzleBit/CPUswizzleBit.h"

While you're at it, maybe it is time to fix #5

It may seem like a lot of work, but a clean API including a public one will benefit everyone using this library. Please seriously consider taking the hit now and not later.

c files should not be included. If that's your design, please review your design.

@drprajap
Copy link
Contributor

Yes, relative directory path can be fixed and I was checking the builds with that change. when I use
#include "Utility/CpuSwizzleBit/CPUswizzleBit.h"
with igdgmm package present on the system, media-driver builds fine. but When the package is not present and it builds from gmmlib sources, the media-driver compilation fails with "file not found" error.

I am looking into the issue right now and will update on that.

Regarding other includes, we can fix all other hard coded paths with this change. But reorganizing headers requires bigger effort, which cannot be taken at this point, because team is working on other higher priority tasks. Once all the clients transition is successful with DLL, We will take the reorganization with priority along with API cleanup. That is next task in our plate.

Regarding c file inclusion, as per design, CpuSwizzleBlt is standalone feature and we wanted to keep it independent so that it can be used in other projects/tools without any dependency. If we divide in 2 headers, we add additional dependency for other projects and it easily gets polluted with other non-relevant stuff. This file rarely changes so we do not want to consider change to the design.

@uartie
Copy link
Contributor

uartie commented Sep 26, 2018

Please fix this asap... media-driver won't compile when using gmmlib from pkg-config settings.

<include-prefix>/igdgmm/inc/External/Common/GmmCommonExt.h:63:10: fatal error: ../../../Utility/CpuSwizzleBlt/CpuSwizzleBlt.c: No such file or directory
 #include "../../../Utility/CpuSwizzleBlt/CpuSwizzleBlt.c"
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Clearly, I see <include-prefix>/igdgmm/GmmLib/Utility/CpuSwizzleBlt/CpuSwizzleBlt.c is installed, but the relative include in GmmCommonExt.h is completely wrong.

gmmlib (master) tags/intel-gmmlib-18.3.pre2-0-g5b61c8a6b188 
media-driver (master) heads/master-0-g6d80b29459b8

@dvrogozh
Copy link
Contributor Author

@uartie: I am using gmmlib from pkg-config, but I don't see this issue. I am not CentOS 7.5. This issue looks to be compiler/OS specific. Maybe you can provide a PR fix for that since you can try it?

@uartie
Copy link
Contributor

uartie commented Sep 26, 2018

@dvrogozh please make sure you don't have a stale installation of gmmlib (i.e. remove your old <install-prefix>/igdgmm directory before reinstalling with latest code). Let me know if you still don't see this after that. IIRC, I didn't see this problem last week.

@uartie
Copy link
Contributor

uartie commented Sep 26, 2018

I wouldn't suspect install would be compiler/os specific other than the base install prefix

@uartie
Copy link
Contributor

uartie commented Sep 26, 2018

@dvrogozh please make sure you don't have a stale installation of gmmlib (i.e. remove your old /igdgmm directory before reinstalling with latest code). Let me know if you still don't see this after that. IIRC, I didn't see this problem last week.

My apologies, turns out I had a stale install of gmmlib, doh (foot in mouth)! Error is gone for me now.

drprajap added a commit to drprajap/gmmlib that referenced this issue Sep 28, 2018
@nehudesi
Copy link

nehudesi commented Oct 2, 2018

With 18.3 tag checked out , I am getting below error:

    VAPictureParameterBufferHEVC
../../../../../../../usr/include/va/va_dec_hevc.h:195:3: note: 'VAPictureParameterBufferHEVC' declared here
} VAPictureParameterBufferHEVC;
  ^
2 errors generated.
make[2]: *** [media_driver/CMakeFiles/iHD_drv_video_OBJ.dir/build.make:9735: media_driver/CMakeFiles/iHD_drv_video_OBJ.dir/linux/common/codec/ddi/media_libva_encoder.cpp.o] Error 1
In file included from ../../media-driver-intel-media-18.3.0/media_driver/linux/common/codec/ddi/media_ddi_decode_hevc.cpp:27:
In file included from ../../media-driver-intel-media-18.3.0/media_driver/linux/common/codec/ddi/media_libva_decoder.h:30:
../../media-driver-intel-media-18.3.0/media_driver/linux/common/ddi/media_libva.h:213:5: error: unknown type name 'VASliceParameterBufferHEVCExtension'; did you mean 'VASliceParameterBufferHEVC'?
    VASliceParameterBufferHEVCExtension          *pVASliceParaBufHEVCRext;
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    VASliceParameterBufferHEVC
../../../../../../../usr/include/va/va_dec_hevc.h:328:3: note: 'VASliceParameterBufferHEVC' declared here
} VASliceParameterBufferHEVC;
  ^
In file included from ../../media-driver-intel-media-18.3.0/media_driver/linux/common/codec/ddi/media_ddi_decode_hevc.cpp:27:
In file included from ../../media-driver-intel-media-18.3.0/media_driver/linux/common/codec/ddi/media_libva_decoder.h:30:
../../media-driver-intel-media-18.3.0/media_driver/linux/common/ddi/media_libva.h:219:5: error: unknown type name 'VAPictureParameterBufferHEVCExtension'; did you mean 'VAPictureParameterBufferHEVC'?
    VAPictureParameterBufferHEVCExtension        PicParamHEVCRext;
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    VAPictureParameterBufferHEVC
../../../../../../../usr/include/va/va_dec_hevc.h:195:3: note: 'VAPictureParameterBufferHEVC' declared here
} VAPictureParameterBufferHEVC;
  ^
../../media-driver-intel-media-18.3.0/media_driver/linux/common/codec/ddi/media_ddi_decode_hevc.cpp:434:14: error: use of undeclared identifier 'VASubsetsParameterBufferType'; did you mean 'VASliceParameterBufferType'?
        case VASubsetsParameterBufferType:
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
             VASliceParameterBufferType
../../../../../../../usr/include/va/va.h:1533:5: note: 'VASliceParameterBufferType' declared here
    VASliceParameterBufferType          = 4,
    ^
../../media-driver-intel-media-18.3.0/media_driver/linux/common/codec/ddi/media_ddi_decode_hevc.cpp:434:14: error: duplicate case value 'VASliceParameterBufferType'
        case VASubsetsParameterBufferType:
             ^
../../media-driver-intel-media-18.3.0/media_driver/linux/common/codec/ddi/media_ddi_decode_hevc.cpp:407:14: note: previous case defined here
        case VASliceParameterBufferType:
             ^
4 errors generated.
make[2]: *** [media_driver/CMakeFiles/iHD_drv_video_OBJ.dir/build.make:9783: media_driver/CMakeFiles/iHD_drv_video_OBJ.dir/linux/common/codec/ddi/media_ddi_decode_hevc.cpp.o] Error 1

Looks like it is related issue.

@dvrogozh
Copy link
Contributor Author

dvrogozh commented Oct 2, 2018

@nehudesi : you build error don't seem to be related with gmmlib at all. Instead this seem to be related with mismatch libva and media-driver. Please, make sure that you checked out the following tags, build and install dependencies in the following order:

@nehudesi
Copy link

nehudesi commented Oct 2, 2018

Appreciate your reply @dvrogozh ! I will work on that and let you know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants