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

fix: link against transitive libaom library dependencies #2304

Merged
merged 5 commits into from
Jul 25, 2024

Conversation

fdintino
Copy link
Contributor

fixes #2274

@fdintino fdintino marked this pull request as draft July 22, 2024 17:47
@fdintino fdintino marked this pull request as ready for review July 22, 2024 19:39
Copy link
Collaborator

@wantehchang wantehchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frankie: Thanks a lot for the fix. I have two questions.

cmake/Modules/LocalAom.cmake Outdated Show resolved Hide resolved
cmake/Modules/Findaom.cmake Outdated Show resolved Hide resolved
Copy link
Collaborator

@wantehchang wantehchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frankie: Thanks for the updated PR. It looks good. I just have a question. I will test your PR tomorrow.

cmake/Modules/LocalAom.cmake Outdated Show resolved Hide resolved
Copy link
Collaborator

@wantehchang wantehchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an entry to CHANGELOG.md. Thanks!

@vrabaud vrabaud added this to the v1.1.1 milestone Jul 24, 2024
Copy link
Collaborator

@wantehchang wantehchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you, Frankie!

@wantehchang wantehchang merged commit f086d98 into AOMediaCodec:main Jul 25, 2024
30 checks passed
@wantehchang
Copy link
Collaborator

Frankie: I merged your pull request before I tested it. I must have made a mistake last time -- in #2274 (comment) I said building libaom as a shared library (with -DBUILD_SHARED_LIBS=ON) was a workaround. However, today I can reproduce the bug whether I build libaom as a static library or a shared library. I asked you to restrict your fix to the libaom static library case. So it doesn't fix the libaom shared library case. Sorry!

Let me go back to an earlier version of your fix that also handles the libaom shared library case and see if it fixes that case.

# by prepending the build directory to PKG_CONFIG_PATH and then calling
# pkg_check_modules
if(WIN32)
set(ENV{PKG_CONFIG_PATH} "${AOM_EXT_SOURCE_DIR}/build.libavif;$ENV{PKG_CONFIG_PATH}")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fdintino Frankie: On Windows this fails with the following linker error:

[33/37] Linking C shared library avif.dll
FAILED: avif.dll avif.lib
C:\WINDOWS\system32\cmd.exe /C "cd . && "C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\IDE\CommonExtensions\Microsoft\CMake\CMake\bin\cmake.exe" -E vs_link_dll --intdir=CMakeFiles\avif.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\mt.exe --manifests  -- C:\PROGRA~1\MICROS~2\2022\COMMUN~1\VC\Tools\MSVC\1440~1.338\bin\Hostx64\x64\link.exe /nologo CMakeFiles\avif_obj.dir\src\alpha.c.obj CMakeFiles\avif_obj.dir\src\avif.c.obj CMakeFiles\avif_obj.dir\src\colr.c.obj CMakeFiles\avif_obj.dir\src\colrconvert.c.obj CMakeFiles\avif_obj.dir\src\diag.c.obj CMakeFiles\avif_obj.dir\src\exif.c.obj CMakeFiles\avif_obj.dir\src\io.c.obj CMakeFiles\avif_obj.dir\src\mem.c.obj CMakeFiles\avif_obj.dir\src\obu.c.obj CMakeFiles\avif_obj.dir\src\rawdata.c.obj CMakeFiles\avif_obj.dir\src\read.c.obj CMakeFiles\avif_obj.dir\src\reformat.c.obj CMakeFiles\avif_obj.dir\src\reformat_libsharpyuv.c.obj CMakeFiles\avif_obj.dir\src\reformat_libyuv.c.obj CMakeFiles\avif_obj.dir\src\scale.c.obj CMakeFiles\avif_obj.dir\src\stream.c.obj CMakeFiles\avif_obj.dir\src\utils.c.obj CMakeFiles\avif_obj.dir\src\write.c.obj CMakeFiles\avif_obj.dir\third_party\libyuv\source\scale.c.obj CMakeFiles\avif_obj.dir\third_party\libyuv\source\scale_common.c.obj CMakeFiles\avif_obj.dir\third_party\libyuv\source\scale_any.c.obj CMakeFiles\avif_obj.dir\third_party\libyuv\source\row_common.c.obj CMakeFiles\avif_obj.dir\third_party\libyuv\source\planar_functions.c.obj CMakeFiles\avif_obj.dir\src\codec_aom.c.obj  /out:avif.dll /implib:avif.lib /pdb:avif.pdb /dll /version:16.1 /machine:x64 /debug /INCREMENTAL  C:\src\libavif.2\libavif\ext\aom\build.libavif\aom.lib  _aom_dep_lib_m-NOTFOUND.lib  kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib  && cd ."
LINK Pass 1: command "C:\PROGRA~1\MICROS~2\2022\COMMUN~1\VC\Tools\MSVC\1440~1.338\bin\Hostx64\x64\link.exe /nologo CMakeFiles\avif_obj.dir\src\alpha.c.obj CMakeFiles\avif_obj.dir\src\avif.c.obj CMakeFiles\avif_obj.dir\src\colr.c.obj CMakeFiles\avif_obj.dir\src\colrconvert.c.obj CMakeFiles\avif_obj.dir\src\diag.c.obj CMakeFiles\avif_obj.dir\src\exif.c.obj CMakeFiles\avif_obj.dir\src\io.c.obj CMakeFiles\avif_obj.dir\src\mem.c.obj CMakeFiles\avif_obj.dir\src\obu.c.obj CMakeFiles\avif_obj.dir\src\rawdata.c.obj CMakeFiles\avif_obj.dir\src\read.c.obj CMakeFiles\avif_obj.dir\src\reformat.c.obj CMakeFiles\avif_obj.dir\src\reformat_libsharpyuv.c.obj CMakeFiles\avif_obj.dir\src\reformat_libyuv.c.obj CMakeFiles\avif_obj.dir\src\scale.c.obj CMakeFiles\avif_obj.dir\src\stream.c.obj CMakeFiles\avif_obj.dir\src\utils.c.obj CMakeFiles\avif_obj.dir\src\write.c.obj CMakeFiles\avif_obj.dir\third_party\libyuv\source\scale.c.obj CMakeFiles\avif_obj.dir\third_party\libyuv\source\scale_common.c.obj CMakeFiles\avif_obj.dir\third_party\libyuv\source\scale_any.c.obj CMakeFiles\avif_obj.dir\third_party\libyuv\source\row_common.c.obj CMakeFiles\avif_obj.dir\third_party\libyuv\source\planar_functions.c.obj CMakeFiles\avif_obj.dir\src\codec_aom.c.obj /out:avif.dll /implib:avif.lib /pdb:avif.pdb /dll /version:16.1 /machine:x64 /debug /INCREMENTAL C:\src\libavif.2\libavif\ext\aom\build.libavif\aom.lib _aom_dep_lib_m-NOTFOUND.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST /MANIFESTFILE:CMakeFiles\avif.dir/intermediate.manifest CMakeFiles\avif.dir/manifest.res" failed (exit code 1104) with the following output:
LINK : fatal error LNK1104: cannot open file '_aom_dep_lib_m-NOTFOUND.lib'

The contents of aom.pc are:

# libaom pkg-config.
prefix=C:/Program Files (x86)/AOM
exec_prefix=${prefix}
includedir=${prefix}/include
libdir=${exec_prefix}/lib

Name: aom
Description: Alliance for Open Media AV1 codec library v3.9.1.
Version: 3.9.1
Requires:
Conflicts:
Libs: -L${libdir} -laom
Libs.private: -lm
Cflags: -I${includedir}

-lm should not be added to aom.pc on Windows.

We can work around this aom.pc bug with this patch:

diff --git a/cmake/Modules/LocalAom.cmake b/cmake/Modules/LocalAom.cmake
index 7f315d59..06c6874b 100644
--- a/cmake/Modules/LocalAom.cmake
+++ b/cmake/Modules/LocalAom.cmake
@@ -38,6 +38,9 @@ if(EXISTS "${LIB_FILENAME}")
     set(_AOM_PC_LIBRARIES "${_AOM_STATIC_LIBRARIES}")
     # remove "aom" so we only have library dependencies
     list(REMOVE_ITEM _AOM_PC_LIBRARIES "aom")
+    if(WIN32)
+        list(REMOVE_ITEM _AOM_PC_LIBRARIES "m")
+    endif()

     # Add absolute paths to libraries
     foreach(_lib ${_AOM_PC_LIBRARIES})

I am wondering if you have a better fix. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems sensible to me. I presume it would be temporary, until the issue is fixed in libaom?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick reply. Yes, I'd like to fix this bug in libaom. Not sure when it is safe to remove this workaround though.

Another idea I have (but I don't know how to implement) is to handle the failure of the find_library(_aom_dep_lib_${_lib} ${_lib} HINTS ${_AOM_STATIC_LIBRARY_DIRS}) call at line 44.

Copy link
Contributor Author

@fdintino fdintino Jul 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not at my computer, but I think find_library sets a variable such that you can check if(NOT ${_aom_dep_lib_${_lib}_FOUND})

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I converted my patch to a pull request: #2333

I filed an issue on libaom: https://aomedia.g-issues.chromium.org/issues/356153293.

wantehchang added a commit to wantehchang/libavif that referenced this pull request Jul 29, 2024
Fix an issue introduced in
AOMediaCodec#2304 when libaom is built
locally in the ext/ directory. See
AOMediaCodec#2304 (review)
for more information.

Related to AOMediaCodec#2274.
wantehchang added a commit that referenced this pull request Jul 29, 2024
Fix an issue introduced in
#2304 when libaom is built
locally in the ext/ directory. See
#2304 (review)
for more information.

Related to #2274.
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 this pull request may close these issues.

v1.1.0 failed to build with aom -DCONFIG_TUNE_BUTTERAUGLI or -DCONFIG_TUNE_VMAF
3 participants