Various CMake fixes #5

uartie · 2017-12-01T18:55:45Z

Fixes for #1, #2 and #3

oliver-sang · 2017-12-04T07:56:36Z

cmrtlib/linux/CMakeLists.txt

 if(NOT "${LIBVA_INSTALL_PATH}" STREQUAL "")
    include_directories(BEFORE ${LIBVA_INSTALL_PATH})
+else()
+    include(FindPkgConfig)


what's the prerequisite to make this FindPkgConfig work properly?
I just build libva below way in Ubuntu 16.04 -
$ ./autogen.sh --prefix=/usr --libdir=/usr/lib/x86_64-linux-gnu
$ make
$ sudo make install
Then below LIBVA_INCLUDE_DIRS and LIBVA_X11_INCLUDE_DIRS are empty

Another strange thing is if I change the judgement as libva>=2.0.0 (even 3.0.0), LIBVA_FOUND will still be true. Same for LIBVA_X11.

I checked by pkg-config -
$ pkg-config --modversion libva
1.0.0
And /usr/lib/x86_64-linux-gnu/pkgconfig/libva.pc also told me -

Name: libva
Description: Userspace Video Acceleration (VA) core interface
Version: 1.0.0
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<

What I missed? Thanks

what's the prerequisite to make this FindPkgConfig work properly?

The prerequisite is that the libva.pc file must be in the pkg-config search path. In your case, it is (i.e. /usr/lib/x86_64-linux-gnu/pkgconfig). If the .pc file is not in the default search path, then user can set PKG_CONFIG_PATH environment variable to give pkg-config additional paths to search.

Then below LIBVA_INCLUDE_DIRS and LIBVA_X11_INCLUDE_DIRS are empty

LIBVA_INCLUDE_DIRS and LIBVA_X11_INCLUDE_DIRS will be empty if its directories are already included in the default cmake include paths.

Another strange thing is if I change the judgement as libva>=2.0.0 (even 3.0.0), LIBVA_FOUND will still be true. Same for LIBVA_X11.

I was able to reproduce your findings for libva>=2.0.0 but not for libva-x11>=2.0.0. Turns out that the previous call to pkg_check_modules(LIBVA REQUIRED libva>=1.0.0) in media_driver/cmake/linux/media_include_paths_linux.cmake overshadows the second call... I will fix this.

oliver-sang · 2017-12-04T08:11:33Z

CMakeLists.txt

+
+    configure_file(${CMAKE_CURRENT_SOURCE_DIR}/media_driver/cmake/linux/intel-media.sh.in ${CMAKE_CURRENT_SOURCE_DIR}/media_driver/cmake/linux/intel-media.sh)
+
+    install (FILES ${CMAKE_CURRENT_BINARY_DIR}/media_driver/iHD_drv_video.so DESTINATION ${CMAKE_INSTALL_LIBDIR}/dri/)


on Ubuntu 64-bit, this CMAKE_INSTALL_LIBDIR will be 'lib', and the final install path will be '/usr/lib/dri/', which is not we wanted. how to remain original path (/usr/lib/x86_64-linux-gnu/dri/)?

If you define -DCMAKE_INSTALL_PREFIX=/usr during cmake, then CMAKE_INSTALL_LIBDIR will be set to /usr/lib/x86_64-linux-gnu on Ubuntu.

Also, user can override with -DCMAKE_INSTALL_LIBDIR=<somedir>

oliver-sang · 2017-12-04T08:16:49Z

CMakeLists.txt

+    configure_file(${CMAKE_CURRENT_SOURCE_DIR}/media_driver/cmake/linux/intel-media.sh.in ${CMAKE_CURRENT_SOURCE_DIR}/media_driver/cmake/linux/intel-media.sh)
+
+    install (FILES ${CMAKE_CURRENT_BINARY_DIR}/media_driver/iHD_drv_video.so DESTINATION ${CMAKE_INSTALL_LIBDIR}/dri/)
+    install (FILES ${CMAKE_CURRENT_SOURCE_DIR}/media_driver/cmake/linux/intel-media.sh  DESTINATION ${CMAKE_INSTALL_SYSCONFDIR}/profile.d/)


on Ubuntu, this CMAKE_INSTALL_SYSCONFDIR is 'etc', and the final package will put intel-media.sh under /usr/etc/profile.d/ , which is not usable to set the LIBVA search path correctly. Why the prefix '/usr' is added?

If you define -DCMAKE_INSTALL_PREFIX=/usr during cmake, then CMAKE_INSTALL_SYSCONFDIR will be set to /etc on Ubuntu. (See https://cmake.org/cmake/help/v3.5/module/GNUInstallDirs.html#special-cases for more details)

oliver-sang · 2017-12-04T08:18:56Z

CMakeLists.txt


    if (IS_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/../libva-install/usr/bin)
-        install (DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/../libva-install/usr/bin DESTINATION /usr/
+        install (DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/../libva-install/usr/bin DESTINATION ${CMAKE_INSTALL_PREFIX}


This CMAKE_INSTALL_PREFIX is /usr/local on Ubuntu, How to make it just '/usr'?

use -DCMAKE_INSTALL_PREFIX=/usr when configuring with cmake command

oliver-sang · 2017-12-04T08:20:38Z

media_driver/cmake/linux/intel-media.sh.in

@@ -1,3 +1,3 @@
 # add libva driver path/name exporting for intel media solution
-export LIBVA_DRIVERS_PATH=/usr/lib/x86_64-linux-gnu/dri/
+export LIBVA_DRIVERS_PATH=${CMAKE_INSTALL_LIBDIR}/dri/


again, since the CMAKE_INSTALL_LIBDIR will be 'lib', the intel-media.sh has the incorrect contents -

export LIBVA_DRIVERS_PATH=lib/dri/

Ah yes, you are right... this should be CMAKE_INSTALL_FULL_LIBDIR. I will fix it.

oliver-sang · 2017-12-04T08:38:17Z

media_driver/cmake/linux/media_include_paths_linux.cmake

@@ -27,6 +27,9 @@ else()
    pkg_check_modules(LIBVA REQUIRED libva>=1.0.0)
    if(LIBVA_FOUND)
        include_directories(BEFORE ${LIBVA_INCLUDE_DIRS})
+        if("${LIBVA_DRIVERS_PATH}" STREQUAL "")
+            pkg_get_variable(LIBVA_DRIVERS_PATH libva driverdir)


I saw this pkg_get_variable() get the correct LIBVA_DRIVERS_PATH as "/usr/lib/x86_64-linux-gnu/dri" but it cannot pass along to parent cmake
(root CMakefiles.txt uses add_subdirectory(media_driver) so the variables have another scope, though here LIBVA_DRIVERS_PATH get correct value, the counterpart one in up-level is still empty)

Thanks, I will fix this.

uartie · 2017-12-04T19:17:51Z

@oliver-sang, the patches have been updated. Let me know if you find any more issues. Thanks.

uartie · 2017-12-04T19:59:44Z

@oliver-sang FYI I also added 2 additional patches.

oliver-sang · 2017-12-05T14:23:03Z

@uartie thanks. I ran out of time today and will try your new patches tomorrow.

uartie · 2017-12-06T16:43:02Z

@oliver-sang looking forward to your final review

oliver-sang · 2017-12-07T01:31:53Z

@uartie sorry, I added comments about configure_file() command. I suggest to set the target (intel-media.sh) as in CMAKE_CURRENT_BINARY_DIR (you just use CMAKE_CURRENT_SOURCE_DIR). Do I add that comment in improper way then it was missed? If so, sorry, and could you check and update the patch a little?

I like your patches that make us extend to none-ubuntu system. Thanks a lot!

uartie · 2017-12-07T01:38:28Z

@uartie sorry, I added comments about configure_file() command. I suggest to set the target (intel-media.sh) as in CMAKE_CURRENT_BINARY_DIR (you just use CMAKE_CURRENT_SOURCE_DIR). Do I add that comment in improper way then it was missed? If so, sorry, and could you check and update the patch a little?

Yes, I must have missed that comment. I used CMAKE_CURRENT_SOURCE_DIR since that is what was used originally (before my patch). How does CMAKE_CURRENT_BINARY_DIR make it different?

uartie · 2017-12-07T01:40:10Z

Oh nevermind, I understand what you're saying now... the "destination" path for configure_file should be CMAKE_CURRENT_BINARY_DIR. I will fix that.

If user does not set -DLIBVA_INSTALL_PATH, then attempt to find it using pkgconfig. Fixes intel#3 Signed-off-by: U. Artie Eoff <[email protected]>

oliver-sang · 2017-12-07T01:42:29Z

@uartie yes, that's what I mean :) Thanks

Use GNUInstallDirs to determine where to install artifacts. Fixes intel#1 Signed-off-by: U. Artie Eoff <[email protected]>

1. If -DLIBVA_DRIVERS_PATH not defined and -DLIBVA_INSTALL_PATH defined, then set LIBVA_DRIVERS_PATH to a default value. 2. If -DLIBVA_DRIVERS_PATH not defined and -DLIBVA_INSTALL_PATH not defined, then set LIBVA_DRIVERS_PATH to libva.pc (pkgconfig) driverdir value if LIBVA_FOUND, otherwise set to a default value. 3. If -DLIBVA_DRIVERS_PATH defined, then use it. Fixes intel#2 Signed-off-by: U. Artie Eoff <[email protected]>

cmake configuration now supports custom install prefix. Update README and script to define "-DCMAKE_INSTALL_PREFIX=/usr" for previous equivalent functionality. Signed-off-by: U. Artie Eoff <[email protected]>

On some setups it does not make sense to require intel-media.sh to be installed. For example, most VAAPI power-users don't want the VAAPI driver predefined since they may be using multiple drivers for different purposes/setups. Add -DINSTALL_DRIVER_SYSCONF=[ON|OFF] option so users can enable/disable it as necessary. Default value is "ON" to maintain previous default functionality. Signed-off-by: U. Artie Eoff <[email protected]>

uartie · 2017-12-07T01:49:38Z

Fixed.

oliver-sang · 2017-12-06T06:45:52Z

CMakeLists.txt

-    install (FILES ${CMAKE_CURRENT_SOURCE_DIR}/media_driver/cmake/linux/intel-media.sh  DESTINATION /etc/profile.d/)
-    install (FILES ${CMAKE_CURRENT_BINARY_DIR}/cmrtlib/linux/igfxcmrt64.so DESTINATION /usr/lib/x86_64-linux-gnu/)
+
+    configure_file(${CMAKE_CURRENT_SOURCE_DIR}/media_driver/cmake/linux/intel-media.sh.in ${CMAKE_CURRENT_SOURCE_DIR}/media_driver/cmake/linux/intel-media.sh)


could you change the configure target to BINARY_DIR? otherwise, it will leave a change in git repo

hmm... this is an outdated change. latest changes use BINARY instead of SOURCE. Perhaps your comments got cached before I made the change and only got posted after you approved the latest.

don't know what happened. but when merging, it should use the latest version. Thanks!

oliver-sang · 2017-12-06T06:55:18Z

CMakeLists.txt

+    if (INSTALL_DRIVER_SYSCONF)
+        configure_file (
+            ${CMAKE_CURRENT_SOURCE_DIR}/media_driver/cmake/linux/intel-media.sh.in
+            ${CMAKE_CURRENT_SOURCE_DIR}/media_driver/cmake/linux/intel-media.sh)


suggest to config to BINARY_DIR

hmm... this is an outdated change. latest changes use BINARY instead of SOURCE

don't know what happened. but when merging, it should use the latest version. Thanks!

oliver-sang · 2017-12-08T07:04:41Z

@uartie I plan to merge this PR. are you ok that I squash the 5 commits into one then merge? Or do you prefer merge as it is? Thanks

uartie · 2017-12-08T07:29:16Z

@oliver-sang it's really up to you if you want to squash them. However, I recommend merge as-is to retain commit messages and separation of changes.

uartie · 2017-12-08T07:30:18Z

In general, smaller changes per commit are easier to bisect if ever needed.

uartie · 2017-12-08T07:35:27Z

Also, I recommend a "Rebase and Merge" if you plan to use the GitHub UI to avoid an extra "merge commit"

oliver-sang · 2017-12-08T07:48:26Z

ok. Thanks

oliver-sang reviewed Dec 4, 2017

View reviewed changes

uartie force-pushed the cmake-fixes branch 2 times, most recently from d0b9c0b to 79abc8c Compare December 4, 2017 19:03

uartie force-pushed the cmake-fixes branch from e743e7b to 3baec0b Compare December 5, 2017 18:46

cmake: use pkgconfig to find libva

1be7caf

If user does not set -DLIBVA_INSTALL_PATH, then attempt to find it using pkgconfig. Fixes intel#3 Signed-off-by: U. Artie Eoff <[email protected]>

uartie added 4 commits December 6, 2017 17:44

cmake: do not use hard-coded install paths

77cd59f

Use GNUInstallDirs to determine where to install artifacts. Fixes intel#1 Signed-off-by: U. Artie Eoff <[email protected]>

update README and unified_cmake.sh for CMAKE_INSTALL_PREFIX

d6653ee

cmake configuration now supports custom install prefix. Update README and script to define "-DCMAKE_INSTALL_PREFIX=/usr" for previous equivalent functionality. Signed-off-by: U. Artie Eoff <[email protected]>

uartie force-pushed the cmake-fixes branch from 3baec0b to 8709c06 Compare December 7, 2017 01:49

oliver-sang approved these changes Dec 7, 2017

View reviewed changes

oliver-sang merged commit 81796c8 into intel:master Dec 8, 2017

xhaihao mentioned this pull request Apr 18, 2018

Segmentation fault when playing back h264 video on Ubuntu #148

Closed

dbabokin mentioned this pull request Jun 22, 2018

Problem running on CFL #195

Open

fulinjie mentioned this pull request Jan 14, 2020

[ffmpeg vaapi][DdiDecodeHEVCG11::AllocSliceControlBuffer] realloc() invalid next size #822

Closed

feiwan1 mentioned this pull request Apr 13, 2020

[EHL][FFmpeg][Enc] jpeg output size is 0. #902

Closed

fredvb mentioned this pull request Jul 12, 2020

Segmenatation fault seen when running sample_decode with Intel Media Driver Q4'19 Release #818

Closed

XinfengZhang mentioned this pull request May 17, 2021

H264 encoding crashes in vaEndPicture, since 21.x.x #1189

Closed

HeJunyan mentioned this pull request Jul 19, 2021

gst va decoder random crash when play some small resolution VP9 stream. #1227

Closed

nirik mentioned this pull request Oct 30, 2021

Crash on Xe graphics with seccomp #1283

Open

SiewHoon mentioned this pull request Nov 29, 2021

media-driver: hitting double free running with gst-vaapi decode pipeline with multiple decode #1300

Closed

xhaihao mentioned this pull request Mar 20, 2023

[Bug]: av1_qsv - Segmentation fault (core dumped) with "free kernel" build (-DENABLE_KERNELS=ON -DENABLE_NONFREE_KERNELS=OFF) #1617

Closed

zhangyichix mentioned this pull request Jun 8, 2023

[VP9] [Encode] Fill padding to tiled format buffer directly #1664

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various CMake fixes #5

Various CMake fixes #5

uartie commented Dec 1, 2017

oliver-sang Dec 4, 2017

uartie Dec 4, 2017

oliver-sang Dec 4, 2017

uartie Dec 4, 2017

uartie Dec 4, 2017

oliver-sang Dec 4, 2017

uartie Dec 4, 2017

oliver-sang Dec 4, 2017

uartie Dec 4, 2017

oliver-sang Dec 4, 2017 •

edited

Loading

uartie Dec 4, 2017

oliver-sang Dec 4, 2017

uartie Dec 4, 2017

uartie commented Dec 4, 2017

uartie commented Dec 4, 2017

oliver-sang commented Dec 5, 2017

uartie commented Dec 6, 2017

oliver-sang commented Dec 7, 2017

uartie commented Dec 7, 2017

uartie commented Dec 7, 2017

oliver-sang commented Dec 7, 2017

uartie commented Dec 7, 2017

oliver-sang Dec 6, 2017

uartie Dec 7, 2017

oliver-sang Dec 7, 2017

oliver-sang Dec 6, 2017

uartie Dec 7, 2017

oliver-sang Dec 7, 2017

oliver-sang commented Dec 8, 2017

uartie commented Dec 8, 2017

uartie commented Dec 8, 2017

uartie commented Dec 8, 2017

oliver-sang commented Dec 8, 2017


		configure_file(${CMAKE_CURRENT_SOURCE_DIR}/media_driver/cmake/linux/intel-media.sh.in ${CMAKE_CURRENT_SOURCE_DIR}/media_driver/cmake/linux/intel-media.sh)

		install (FILES ${CMAKE_CURRENT_BINARY_DIR}/media_driver/iHD_drv_video.so DESTINATION ${CMAKE_INSTALL_LIBDIR}/dri/)

Various CMake fixes #5

Various CMake fixes #5

Conversation

uartie commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliver-sang Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uartie commented Dec 4, 2017

uartie commented Dec 4, 2017

oliver-sang commented Dec 5, 2017

uartie commented Dec 6, 2017

oliver-sang commented Dec 7, 2017

uartie commented Dec 7, 2017

uartie commented Dec 7, 2017

oliver-sang commented Dec 7, 2017

uartie commented Dec 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliver-sang commented Dec 8, 2017

uartie commented Dec 8, 2017

uartie commented Dec 8, 2017

uartie commented Dec 8, 2017

oliver-sang commented Dec 8, 2017

oliver-sang Dec 4, 2017 •

edited

Loading