Do not depend on both avif and avif_internal #1819

vrabaud · 2023-11-29T12:11:23Z

This properly fixes #1816

In a follow-up PR, I can also move internal.h to ./src/ (or an include_private folder).

The header will still be accessed from targets if we target_include_directories them with $<TARGET_PROPERTY:avif,INCLUDE_DIRECTORIES> (which contains both public and private directories).

This will be less confusing for the user ("it's internal but is it still public?") and will enable us to split internal.h in several common-meaning headers.

wantehchang

Vincent: Thanks for writing a fix for #1816 quickly. But we should only export public functions.

wantehchang · 2023-11-30T02:05:08Z

include/avif/internal.h

@@ -89,9 +89,9 @@ typedef float (*avifTransferFunction)(float);
 // and can go beyond 1.0 for HDR transfer characteristics:
 // - For AVIF_TRANSFER_CHARACTERISTICS_SMPTE2084 (PQ), the linear range is [0.0, 10000/203]
 // - For AVIF_TRANSFER_CHARACTERISTICS_HLG, the linear range is [0.0, 1000/203]
-avifTransferFunction avifTransferCharacteristicsGetGammaToLinearFunction(avifTransferCharacteristics atc);
+AVIF_API avifTransferFunction avifTransferCharacteristicsGetGammaToLinearFunction(avifTransferCharacteristics atc);


We should not export the private functions declared in internal.h. Even though we don't provide the internal.h header to our clients, they can still call these functions if these functions are exported.

Note: In some Unix platforms (including Linux), we can give exported symbols a version (which is a string), which allows us to mark certain exported functions as private exports by using a version with "Private" in its name. But I don't think this kind of symbol versioning feature is available on Windows, and it may not even be available on all Unix platforms.

Agreed. I went with the OBJECT library and we can now hide symbols properly, while only compiling the library once.

vrabaud · 2023-12-04T13:58:12Z

@fdintino , it seems merge_static_libs does not work with generator expression. I guess there is no way around it right?

fdintino · 2023-12-04T14:22:21Z

Below this line in merge_static_libs.cmake:

libavif/cmake/Modules/merge_static_libs.cmake

Line 18 in 0c947b3

string(REGEX REPLACE "\\\$<BUILD_INTERFACE:([^>]*)>" "\\1" dep ${dep})

Add the line:

            string(REGEX REPLACE "\\\$<LINK_ONLY:([^>]*)>" "\\1" dep ${dep})

vrabaud · 2023-12-04T14:28:55Z

Thx fdintino ! BTW, any opinion on this CL?

fdintino · 2023-12-04T17:05:03Z

This all looks good to me.

wantehchang

Vincent: Thank you very much for figuring out a solution. Although I am not familiar with cmake's OBJECT libraries, your description of your solution sounds good to me. However, this does mean I don't understand some of the changes, so I also inspected the build log.

There are two problems with the current implementation. Please see my comments marked with "IMPORTANT".

include/avif/internal.h

wantehchang · 2023-12-04T23:46:01Z

tests/CMakeLists.txt

@@ -42,7 +42,7 @@ endforeach()

 if(AVIF_ENABLE_FUZZTEST OR AVIF_ENABLE_GTEST OR AVIF_BUILD_APPS)
    add_library(aviftest_helpers OBJECT gtest/aviftest_helpers.cc)
-    target_link_libraries(aviftest_helpers avif_apps avif_internal)
+    target_link_libraries(aviftest_helpers avif_apps_internal avif_internal)


Does this mean all tests are linked with avif_internal?

Ideally, are_images_equal and the tests that only use public libavif functions should be linked with avif. But it's more important to fix the dual avif/avif_internal linking bug first.

Right, but as aviftest_helpers uses avif_internal, it needs to be linked to avif_apps_internal. We could also create an aviftest_helpers_internal. Or split avif.c: internal.h is only used here for AVIF_CHECKERR, AVIF_CHECRES and avifImageCopySamples. We could envision a small utils library that contains non-avif specific stuff (like the AVIF_CHECK*) and an image library that would just deal with the basic functions for images (like avifImageCopySamples) which is half of avif.c.

I discussed the aviftest_helpers issue in #1836. The use of AVIF_CHECKERR and AVIF_CHECRES in aviftest_helpers.cc can be easily avoided. And we can make avifImageCopySamples a public function.

cmake/Modules/merge_static_libs.cmake

CMakeLists.txt

wantehchang

Vincent: I inspected the build log. This is much better

High-level comments:

The position-independent code changes seem unrelated to fixing the dual avif/avif_internal linking bug.
Please see the comment marked with "IMPORTANT".

CMakeLists.txt

wantehchang · 2023-12-06T01:40:48Z

CMakeLists.txt

    )
-    target_link_libraries(avif_apps avif PNG::PNG ZLIB::ZLIB JPEG::JPEG)
+    set_property(TARGET avif_apps_obj PROPERTY POSITION_INDEPENDENT_CODE ON)


This change does not seem necessary.

If this is necessary or desirable, then we should also compile avifdec.c and avifenc.c as position-independent code, right?

Like my comment at line 515 above, if this change can stand alone, it is better to make this change in a separate pull request.

CMakeLists.txt

Obsolete

vrabaud · 2023-12-07T09:49:09Z

I duplicated code in tests/gtest/aviftest_helpers.cc so that it does not depend on avif_internal like you requested. And only tests testing the internal API now depend on avif_internal

wantehchang

Vincent: I suggest some changes. Also please see my comment marked with "IMPORTANT" about the avif_apps and avif_apps_internal libraries.

CMakeLists.txt

tests/gtest/aviftest_helpers.cc

tests/CMakeLists.txt

CMakeLists.txt

wantehchang

LGTM. Thanks a lot!

CMakeLists.txt

wantehchang · 2023-12-09T15:55:01Z

CMakeLists.txt

@@ -127,21 +127,22 @@ else()
    set(XCRUN)
 endif()

+# This is also needed to get shared libraries (e.g. pixbufloader-avif) to compile against a static libavif.
+set(CMAKE_POSITION_INDEPENDENT_CODE ON)


Please mention this change in the final commit message (when you squash and merge this PR), so that it is not buried in this large PR.

I did not add it to the CHANGELOG as it seems too minor.

CMakeLists.txt

wantehchang · 2023-12-09T16:02:27Z

tests/gtest/avifincrtest_helpers.h

+    }                                 \
+  } while (0)
+//------------------------------------------------------------------------------
+


Delete this.

Among the files that include this header, only avifincrtest_helpers.cc uses the AVIF_CHECK* macros. But avifincrtest_helpers.cc also includes aviftest_helpers.h.

Note: Some files (e.g., avifgainmaptest.cc) include both this header and aviftest_helpers.h. I am surprised that compilers allow duplicate macro definitions. I guess if the definitions are exactly the same, then they are allowed.

Indeed, that's how C works: you are allowed to redefine the macro as long as it is the same:

tests/gtest/aviftest_helpers.cc

tests/gtest/aviftest_helpers.h

Now define avif_obj on which avif and avif_internal depend. There is only one compilation happening. Only symbol export is different between the two libs

vrabaud requested a review from wantehchang November 29, 2023 12:12

wantehchang previously requested changes Nov 30, 2023

View reviewed changes

vrabaud force-pushed the avif_internal branch 5 times, most recently from 4e4b931 to 6b62f12 Compare December 1, 2023 12:17

vrabaud requested a review from wantehchang December 1, 2023 13:59

vrabaud force-pushed the avif_internal branch from 6b62f12 to 2f1b082 Compare December 4, 2023 13:03

wantehchang reviewed Dec 5, 2023

View reviewed changes

vrabaud force-pushed the avif_internal branch 3 times, most recently from 1f4f6cf to 4a4d425 Compare December 5, 2023 09:35

y-guyon mentioned this pull request Dec 5, 2023

Rename avifImageCopySamples to avifImageCopyPlanes #1836

Closed

wantehchang reviewed Dec 6, 2023

View reviewed changes

vrabaud force-pushed the avif_internal branch from 4a4d425 to a5b75ba Compare December 7, 2023 09:47

vrabaud changed the title ~~Remove avif_internal.~~ Do not depend on both avif and avif_internal Dec 7, 2023

wantehchang reviewed Dec 8, 2023

View reviewed changes

vrabaud force-pushed the avif_internal branch 5 times, most recently from 7f572c9 to ba9b812 Compare December 8, 2023 23:10

wantehchang approved these changes Dec 9, 2023

View reviewed changes

vrabaud added 5 commits December 11, 2023 12:39

Redefine avif_internal.

60a30e0

Now define avif_obj on which avif and avif_internal depend. There is only one compilation happening. Only symbol export is different between the two libs

Fix comments

ee38c05

Take comments into account

ec4818d

Fix comments

4054077

Apply AVIF_DLL and format

bde3f0a

vrabaud force-pushed the avif_internal branch from ba9b812 to f777357 Compare December 11, 2023 11:40

Fix comments.

152292e

vrabaud force-pushed the avif_internal branch from f777357 to 152292e Compare December 11, 2023 11:43

vrabaud merged commit 7b9a072 into AOMediaCodec:main Dec 11, 2023

vrabaud deleted the avif_internal branch December 11, 2023 11:52

fdintino mentioned this pull request Jul 30, 2024

Add the avif_enable_warnings INTERFACE library #2327

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not depend on both avif and avif_internal #1819

Do not depend on both avif and avif_internal #1819

vrabaud commented Nov 29, 2023

wantehchang left a comment

wantehchang Nov 30, 2023

vrabaud Dec 1, 2023

vrabaud commented Dec 4, 2023 •

edited by wantehchang

Loading

fdintino commented Dec 4, 2023

vrabaud commented Dec 4, 2023

fdintino commented Dec 4, 2023

wantehchang left a comment

wantehchang Dec 4, 2023 •

edited

Loading

vrabaud Dec 5, 2023 •

edited

Loading

wantehchang Dec 6, 2023

wantehchang left a comment

wantehchang Dec 6, 2023

vrabaud commented Dec 7, 2023

wantehchang left a comment

wantehchang left a comment

wantehchang Dec 9, 2023

vrabaud Dec 11, 2023

wantehchang Dec 9, 2023

vrabaud Dec 11, 2023

Do not depend on both avif and avif_internal #1819

Do not depend on both avif and avif_internal #1819

Conversation

vrabaud commented Nov 29, 2023

wantehchang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrabaud commented Dec 4, 2023 • edited by wantehchang Loading

fdintino commented Dec 4, 2023

vrabaud commented Dec 4, 2023

fdintino commented Dec 4, 2023

wantehchang left a comment

Choose a reason for hiding this comment

wantehchang Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

vrabaud Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wantehchang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrabaud commented Dec 7, 2023

wantehchang left a comment

Choose a reason for hiding this comment

wantehchang left a 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

vrabaud commented Dec 4, 2023 •

edited by wantehchang

Loading

wantehchang Dec 4, 2023 •

edited

Loading

vrabaud Dec 5, 2023 •

edited

Loading