upgrade vendored libavif to 1.0.1 release (and dav1d to 1.2.1 release) about sdl_image

[kmilos]

Btw, this also requires changes to find_package(libavif ...) (for both SDL2 and main) - because of SameMajorVersion libavif CMake config file policy (non-vendored) 1.0.x will currently get rejected if 0.x.y was requested.

[sezero]

@kmilos: libavif cmake'ry outputs libavif.dll w/o any versioning in dll name (like libavif-16.dll), is that what you mean?

[kmilos]

No, I mean e.g. find_package(libavif 0.9.2) will FAIL if you have libavif 1.0.1 installed because of SameMajorVersion policy provided by the libavif 1.0.1 CMake config itself, on any platform.

CMake Warning at CMakeLists.txt:5 (find_package):
  Could not find a configuration file for package "libavif" that is
  compatible with requested version "0.9.2".

  The following configuration files were considered but not accepted:

    C:/msys64/ucrt64/lib/cmake/libavif/libavif-config.cmake, version: 1.0.1

So you either have to find_package(libavif) w/o version and then manually check libavif_VERSION if you want, or add an additional find_package(libavif 1.0.0) call if find_package(libavif 0.9.2) failed.

This has nothing to do w/ DLL names.

[sezero]

No, I mean e.g. find_package(libavif 0.9.2) will FAIL if you have libavif 1.0.1 installed because of SameMajorVersion policy provided by the libavif 1.0.1 CMake config itself, on any platform.

CMake Warning at CMakeLists.txt:5 (find_package):
  Could not find a configuration file for package "libavif" that is
  compatible with requested version "0.9.2".

  The following configuration files were considered but not accepted:

    C:/msys64/ucrt64/lib/cmake/libavif/libavif-config.cmake, version: 1.0.1

So you either have to find_package(libavif) w/o version and then manually check libavif_VERSION if you want, or add an additional find_package(libavif 1.0.0) call if find_package(libavif 0.9.2) failed.

Ah, you are right. I had tested on the SDL2 branch using autotools, but not with cmake:
it fails exactly as you described.

@slouken, @icculus: What should be our policy here? There are distros out there with libavif-0.10.x, libavif-0.11.x (maybe even libavif-0.9.x). Check 1.0.0 first, and if it failed then check 0.9.2??

This has nothing to do w/ DLL names.

Right. That one is another problem, and it actually is a problem but at libavif side.

[kmilos]

Yes, autotools go through pkg-conf files where the version check is a simple >= so it works.

Right. That one is another problem, and it actually is a problem but at libavif side.

More of a problem on CMake side, don't know why they turned that off (or rather never had it on) by default...

Anyway, just build libavif (and any other shared lib) w/ -DCMAKE_DLL_NAME_WITH_SOVERSION=ON as we now do for MSYS2 packages.

[sezero]

Anyway, just build libavif (and any other shared lib) w/ -DCMAKE_DLL_NAME_WITH_SOVERSION=ON as we now do for MSYS2 packages.

Thanks.

P.S.: Did you rebuild your SDL2_image against it?

[kmilos]

P.S.: Did you rebuild your SDL2_image against it?

Yes, we just temporarily changed the version check as in the PKGBUILD recipe using sed.

[sezero]

@slouken, @icculus: What should be our policy here? There are distros out there with libavif-0.10.x, libavif-0.11.x (maybe even libavif-0.9.x). Check 1.0.0 first, and if it failed then check 0.9.2??

Maybe something like the following? (applies to both SDL2 and SDL3. Kde does something similar)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 9d3d4fe..8a7490f 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -371 +371,7 @@ if(SDL2IMAGE_AVIF)
-        find_package(libavif ${LIBAVIF_MINIMUM_VERSION} REQUIRED)
+        find_package(libavif 1.0 QUIET)
+        if(NOT libavif_FOUND)
+            find_package(libavif ${LIBAVIF_MINIMUM_VERSION} QUIET)
+        endif()
+        if(NOT libavif_FOUND)
+            message(FATAL_ERROR "libavif NOT found")
+        endif()

CC: @madebr

[icculus]

Maybe something like the following?

I don't think it needs to be marked QUIET, it's probably okay if people see this can't find a specific version so it tried an older one...but I don't feel strongly about that if there's a good reason to keep that flag.

Otherwise, I think this is fine, but I always defer to @madebr's opinion on CMake issues.

[sezero]

I don't think it needs to be marked QUIET, it's probably okay if people see this can't find a specific version

Well, cmake output is too verbose in not-found cases of find_package:

CMake Warning at CMakeLists.txt:371 (find_package):
  Could not find a configuration file for package "libavif" that is
  compatible with requested version "0.9.1".

  The following configuration files were considered but not accepted:

    /home/ozzie/z1/lib/cmake/libavif/libavif-config.cmake, version: 1.0.0

Maybe something like this?

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 9d3d4fe..e13cb4b 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -371 +371,11 @@ if(SDL2IMAGE_AVIF)
-        find_package(libavif ${LIBAVIF_MINIMUM_VERSION} REQUIRED)
+        find_package(libavif 1.0 QUIET)
+        if(NOT libavif_FOUND)
+            message(STATUS "libavif-1.0 or compatible not found")
+            find_package(libavif ${LIBAVIF_MINIMUM_VERSION} QUIET)
+        endif()
+        if(libavif_FOUND)
+            message(STATUS "libavif-${libavif_VERSION} found")
+        else()
+            message(STATUS "libavif-${LIBAVIF_MINIMUM_VERSION} or compatible not found")
+            message(FATAL_ERROR "libavif NOT found")
+        endif()

[madebr]

Too bad version ranges don't work.

sezero's suggestion looks good.
It needs to be duplicated/ported in cmake/SDL3_imageConfig.cmake (with find_dependency) for users of the static SDL_image that don't dynamically load the external libraries.

[sezero]

sezero's suggestion looks good. It needs to be duplicated/ported in cmake/SDL3_imageConfig.cmake (with find_dependency) for users of the static SDL_image that don't dynamically load the external libraries.

May I leave this cmake'ry to you?

P.S.: Compared avif.h across versions:

• 0.9.1 (what require as minimum) -> 0.9.2: no abi changes, as far as I can see.
• 0.9.2 -> 0.9.3: abi did change (struct avifDecoder has new members inserted)
• 0.9.3 -> 0.10.[01]: abi did change
• 0.10.0 -> 0.11.[01]: abi did change
• 0.11.0 -> 1.0.0: abi did change.

To avoid any confusion, I wanted to bump our minimum required version from 0.9.1
to 0.9.3. However, ubuntu is a problem:
https://repology.org/project/libavif/versions

[madebr]

May I leave this cmake'ry to you?

No problem.

To avoid any confusion, I wanted to bump our minimum required version from 0.9.1 to 0.9.3. However, ubuntu is a problem: https://repology.org/project/libavif/versions

The oldest Ubuntu version in the list is 22.04 LTS with 0.9.3.
Older Ubuntu versions will become unsupported soon. 20.04 LTS is not even in the list.
So I think it would be ok to bump the version.
Am I missing something?

[sezero]

sezero's suggestion looks good. It needs to be duplicated/ported in cmake/SDL3_imageConfig.cmake (with find_dependency) for users of the static SDL_image that don't dynamically load the external libraries.

May I leave this cmake'ry to you?

P.S.: Compared avif.h across versions:

* 0.9.1 (what require as minimum) -> 0.9.2: no abi changes, as far as I can see.

* 0.9.2 -> 0.9.3: abi did change (`struct avifDecoder` has new members inserted)

* 0.9.3 -> 0.10.[01]: abi did change

* 0.10.0 -> 0.11.[01]: abi did change

* 0.11.0 -> 1.0.0: abi did change.

To avoid any confusion, I wanted to bump our minimum required version from 0.9.1 to 0.9.3. However, ubuntu is a problem: https://repology.org/project/libavif/versions

No, it looks like I misread that: ubuntu-22.04 does seem to have 0.9.3

Therefore, I suggest that we bump our minimum required libavif to 0.9.3
Proposed patch below (configury needs regenerating of course. And
@madebr can possibly do a better job with cmake side.)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 9d3d4fe..ef5062f 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -97 +96,0 @@
-set(LIBAVIF_MINIMUM_VERSION "0.9.1")
@@ -371 +370,19 @@
-        find_package(libavif ${LIBAVIF_MINIMUM_VERSION} REQUIRED)
+        find_package(libavif 1.0 QUIET)
+        if(NOT libavif_FOUND)
+            message(STATUS "libavif-1.0 or compatible not found")
+            find_package(libavif 0.11 QUIET)
+        endif()
+        if(NOT libavif_FOUND)
+            message(STATUS "libavif-0.11 or compatible not found")
+            find_package(libavif 0.10 QUIET)
+        endif()
+        if(NOT libavif_FOUND)
+            message(STATUS "libavif-0.10 or compatible not found")
+            find_package(libavif 0.9.3 QUIET)
+        endif()
+        if(libavif_FOUND)
+            message(STATUS "libavif-${libavif_VERSION} found")
+        else()
+            message(STATUS "libavif-${LIBAVIF_MINIMUM_VERSION} or compatible not found")
+            message(FATAL_ERROR "libavif NOT found")
+        endif()
diff --git a/configure.ac b/configure.ac
index 2080596..b0a7bd2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -306,17 +306,22 @@
 
 load_avif=0
 if test x$enable_avif = xyes; then
-    PKG_CHECK_MODULES([LIBAVIF], [libavif], [dnl
+    PKG_CHECK_MODULES([LIBAVIF], [libavif >= 0.9.3], [dnl
         have_avif_hdr=yes
         have_avif_lib=yes
         have_avif_pc=yes
       ], [dnl
-        AC_CHECK_HEADER([avif/avif.h], [
+        AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
+          #include <avif/avif.h>
+        ]],, [[
+          #if (AVIF_VERSION < 90300)
+          #error libavif too old.
+          #endif
+          return !!avifVersion();
+        ]])], [
             have_avif_hdr=yes
-            LIBAVIF_CFLAGS=""
-        ])
-        AC_CHECK_LIB([avif], [avifVersion], [
             have_avif_lib=yes
+            LIBAVIF_CFLAGS=""
             LIBAVIF_LIBS="-lavif"
         ])
       ])

[madebr]

Looks good, apart from we only need to check 0.9.3 and 1.0.0. The 0.9.3 is interpreted as a minimum required version.
If you add EXACT to find_package, then cmake becomes picky.

[sezero]

Looks good, apart from we only need to check 0.9.3 and 1.0.0. The 0.9.3 is interpreted as a minimum required version. If you add EXACT to find_package, then cmake becomes picky.

OK, pushed the following:
SDL2: 8065fd0
SDL3: 05448e2

Leaving any polishing + extra work to you.

(The ticket is still open because the actual title subject hasn't happened yet.)

[sezero]

@madebr, @slouken:

Off-topic to this ticket (at least some of it), but noticed some weirdness
in dynamic lib detection, at least in the CI logs:

• SDL2, cmake: Avif messages don't match the latest changes.
  E.g.: at the least, note the missing SDL2_image: Using system libavif
  message. dynamic libavif is also missing. Looks like the it is only
  reporting from find_package without the QUIET flag:

-- Checking for module 'libavif'
--   Found libavif, version 1.0.1

• SDL2, autotools: find_lib for libavif.dylib may be versioned, and
  especially in the libavif case the version is important. This line:
  avif_lib=[find_lib libavif.dylib]
  .. needs changing.
  (Actually, all other dylibs have the same issue I think, but libavif
  has the most abi breakages across versions recently.)

• SDL3 (cmake), Mac: Discovered dynamic libs seem to include full
  version number: Why? That's too limiting. E.g. libavif.16.0.1.dylib
  and libavif.16.0.0.dylib are equivalent abi-wise, and libavif.16.dylib
  (guessing the name here) should be enough. See, e.g.:

-- Dynamic libavif: libavif.16.0.1.dylib
-- SDL3_image: Using system libjxl
-- Found libjxl: /usr/local/lib/libjxl.dylib  
-- Dynamic libjxl: libjxl.0.8.2.dylib
-- SDL3_image: Using system libtiff
-- Found TIFF: /usr/local/lib/libtiff.dylib (found version "4.0.9") 
-- Dynamic libtiff: libtiff.6.dylib
-- SDL3_image: Using system libwebp
[...]
-- Dynamic libwebpdemux: libwebpdemux.2.0.13.dylib
-- Dynamic libwebp: libwebp.7.1.7.dylib

[icculus]

Well, cmake output is too verbose in not-found cases of find_package:

This is already resolved, but wow, you're right about that!

[madebr]

@sezero
I fixed the names of shared libraries on macos.
I wrongly assumed .dylibs on apple had the same file name format as linux .so file names.
Please check again current SDL3 master (SDL2 is in ci on my fork).

EDIT: pushed to SDL2

[madebr]

With CMake, libavif on SDL2 is only tested on mingw64: it needs an explicit -DSDL2IMAGE_AVIF=ON.
I'm not sure how the build tests succeed even though it does export SDL_IMAGE_TEST_REQUIRE_LOAD_AVIF=1.

[sezero]

@sezero I fixed the names of shared libraries on macos. I wrongly assumed .dylibs on apple had the same file name format as linux .so file names.

Thanks.

Other satellite libs (SDL_mixer especially) may need similar treatment.

Please check again current SDL3 master

CI looks fine so far.

EDIT: pushed to SDL2

Does SDL2_imageConfig.cmake.in not need a similar touch?

[madebr]

Other satellite libs (SDL_mixer especially) may need similar treatment.

I will do a sync this weekend.

EDIT: pushed to SDL2

Does SDL2_imageConfig.cmake.in not need a similar touch?

Yes, see 0c34e8c

[sezero]

Other satellite libs (SDL_mixer especially) may need similar treatment.

I will do a sync this weekend.

OK

EDIT: pushed to SDL2

Does SDL2_imageConfig.cmake.in not need a similar touch?

Yes, see 0c34e8c

Missed that, sorry.

P.S.: Autotools find_lib for mac, at least for libavif, still need adjusting. (see above)

P.S./2: CI logs in SDL2 cmake runs still seem to output weird. (see above)

[madebr]

P.S./2: CI logs in SDL2 cmake runs still seem to output weird. (see above)

-- Checking for module 'libavif'
--   Found libavif, version 1.0.1

This is output of pkg_check_modules
The build script of libjxl tools do this. Probably to build a tool that converts all kinds of images to jxl.

[madebr]

I see this with SDL2_image's autotools on msys2:

-- dynamic libjxl -> libjxl_threads.dll

Later, the tests fail:

ERROR: 09/09/23 01:38:18: Assert 'Initialization should succeed (Failed loading JxlDecoderCreate: The specified procedure could not be found.)': Failed
INFO:  09/09/23 01:38:18: Assert 'Building filename should succeed (Failed loading JxlDecoderCreate: The specified procedure could not be found.)': Passed
INFO:  09/09/23 01:38:18: Assert 'Building ref filename should succeed (Failed loading JxlDecoderCreate: The specified procedure could not be found.)': Passed
INFO:  09/09/23 01:38:18: Assert 'Loading reference should succeed ()': Passed
ERROR: 09/09/23 01:38:18: Assert 'Initialization should succeed (Failed loading JxlDecoderCreate: The specified procedure could not be found.)': Failed
INFO:  09/09/23 01:38:18: Assert 'Building filename should succeed (Failed loading JxlDecoderCreate: The specified procedure could not be found.)': Passed
INFO:  09/09/23 01:38:18: Assert 'Building ref filename should succeed (Failed loading JxlDecoderCreate: The specified procedure could not be found.)': Passed
INFO:  09/09/23 01:38:18: Assert 'Loading reference should succeed ()': Passed
ERROR: 09/09/23 01:38:18: Assert 'Initialization should succeed (Failed loading JxlDecoderCreate: The specified procedure could not be found.)': Failed
INFO:  09/09/23 01:38:18: Assert 'Building filename should succeed (Failed loading JxlDecoderCreate: The specified procedure could not be found.)': Passed
INFO:  09/09/23 01:38:18: Assert 'Building ref filename should succeed (Failed loading JxlDecoderCreate: The specified procedure could not be found.)': Passed

https://packages.msys2.org/package/mingw-w64-x86_64-libjxl
The package provides libjxl_threads.dll and libjxl.dll.

[sezero]

I see this with SDL2_image's autotools on msys2:

-- dynamic libjxl -> libjxl_threads.dll

Sigh... We do find_lib "libjxl*.dll" for mingw/cygwin, * there is the culprit. Should it have been -* instead, I wonder?

[sezero]

I see this with SDL2_image's autotools on msys2:

-- dynamic libjxl -> libjxl_threads.dll

Sigh... We do find_lib "libjxl*.dll" for mingw/cygwin, * there is the culprit. Should it have been -* instead, I wonder?

@slouken, @icculus, @madebr: Is the following correct?
If it is, I can change all others similarly and also in SDL_mixer.

diff --git a/configure.ac b/configure.ac
index 021e2a7..0cc7bcc 100644
--- a/configure.ac
+++ b/configure.ac
@@ -339,10 +339,16 @@ if test x$enable_avif = xyes; then
 
         case "$host" in
             *-*-darwin*)
-                avif_lib=[`find_lib libavif.dylib`]
+                avif_lib=[`find_lib "libavif.[0-9]*.dylib"`]
+                if test x$avif_lib = x; then
+                    avif_lib=[`find_lib libavif.dylib`]
+                fi
                 ;;
             *-*-cygwin* | *-*-mingw*)
-                avif_lib=[`find_lib "libavif*.dll"`]
+                avif_lib=[`find_lib "libavif-[0-9]*.dll"`]
+                if test x$avif_lib = x; then
+                    avif_lib=[`find_lib libavif.dll`]
+                fi
                 ;;
             *)
                 avif_lib=[`find_lib "libavif[0-9]*.so.*"`]

[madebr]

Is the vendored libavif compatible with the vendored libdav1d?

/home/maarten/projects/SDL_image/external/libavif/src/codec_dav1d.c: In function ‘dav1dCodecGetNextImage’:
/home/maarten/projects/SDL_image/external/libavif/src/codec_dav1d.c:66:22: error: ‘Dav1dSettings’ has no member named ‘n_frame_threads’
   66 |         dav1dSettings.n_frame_threads = 1;
      |                      ^
/home/maarten/projects/SDL_image/external/libavif/src/codec_dav1d.c:67:23: error: ‘Dav1dSettings’ has no member named ‘n_tile_threads’; did you mean ‘n_threads’?
   67 |         dav1dSettings.n_tile_threads = AVIF_CLAMP(decoder->maxThreads, 1, DAV1D_MAX_TILE_THREADS);
      |                       ^~~~~~~~~~~~~~
      |                       n_threads
In file included from /home/maarten/projects/SDL_image/external/libavif/src/codec_dav1d.c:4:
/home/maarten/projects/SDL_image/external/libavif/src/codec_dav1d.c:67:75: error: ‘DAV1D_MAX_TILE_THREADS’ undeclared (first use in this function); did you mean ‘DAV1D_MAX_TILE_ROWS’?
   67 |         dav1dSettings.n_tile_threads = AVIF_CLAMP(decoder->maxThreads, 1, DAV1D_MAX_TILE_THREADS);
      |                                                                           ^~~~~~~~~~~~~~~~~~~~~~
/home/maarten/projects/SDL_image/external/libavif/include/avif/internal.h:14:62: note: in definition of macro ‘AVIF_CLAMP’
   14 | #define AVIF_CLAMP(x, low, high) (((x) < (low)) ? (low) : (((high) < (x)) ? (high) : (x)))
      |                                                              ^~~~
/home/maarten/projects/SDL_image/external/libavif/src/codec_dav1d.c:67:75: note: each undeclared identifier is reported only once for each function it appears in
   67 |         dav1dSettings.n_tile_threads = AVIF_CLAMP(decoder->maxThreads, 1, DAV1D_MAX_TILE_THREADS);
      |                                                                           ^~~~~~~~~~~~~~~~~~~~~~
/home/maarten/projects/SDL_image/external/libavif/include/avif/internal.h:14:62: note: in definition of macro ‘AVIF_CLAMP’
   14 | #define AVIF_CLAMP(x, low, high) (((x) < (low)) ? (low) : (((high) < (x)) ? (high) : (x)))
      |                                                              ^~~~
ninja: build stopped: subcommand failed.

[sezero]

Is the vendored libavif compatible with the vendored libdav1d?

I recently built libavif-1.0.1 against dav1d-1.2.1 locally without any issues at all.
But I didn't do that through our 'vendored' mechanism.

It may be that the current vendored dav1d (1.0.0) is old and incompatible?
(unlikely, but still...)

[madebr]

Is the vendored libavif compatible with the vendored libdav1d?

I recently built libavif-1.0.1 against dav1d-1.2.1 locally without any issues at all. But I didn't do that through our 'vendored' mechanism.

It may be that the current vendored dav1d (1.0.0) is old and incompatible? (unlikely, but still...)

It was a bug in my CMakeLists.txt. I was mixing the dav1d version and its soversion.

[madebr]

I think this issue is fixed by #384

[sezero]

I think this issue is fixed by #384

(1) 5f4a94a and c88caa5 need porting to SDL3_mixer SDL2_mixer. (Hopefully not missed anything among the commits.)

(2) SDL2, autotools: c88caa5 couldn't be ported to SDL2 autotools and the libjxl.dll issue was band-aided. Therefore, I suggest the following. NOTE: Even though the issue is general, I only touched libavif and libjxl discoveries. Please review. (CC: @slouken).

diff --git a/configure.ac b/configure.ac
index 72b3eee..2f9b49c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -339,10 +339,16 @@ if test x$enable_avif = xyes; then
 
         case "$host" in
             *-*-darwin*)
-                avif_lib=[`find_lib libavif.dylib`]
+                avif_lib=[`find_lib "libavif.[0-9]*.dylib"`]
+                if test x$avif_lib = x; then
+                    avif_lib=[`find_lib "libavif*.dylib"`]
+                fi
                 ;;
             *-*-cygwin* | *-*-mingw*)
-                avif_lib=[`find_lib "libavif*.dll"`]
+                avif_lib=[`find_lib "libavif-[0-9]*.dll"`]
+                if test x$avif_lib = x; then
+                    avif_lib=[`find_lib "libavif*.dll"`]
+                fi
                 ;;
             *)
                 avif_lib=[`find_lib "libavif[0-9]*.so.*"`]
@@ -435,10 +441,16 @@ if test x$enable_jxl = xyes; then
 
         case "$host" in
             *-*-darwin*)
-                jxl_lib=[`find_lib libjxl.dylib`]
+                jxl_lib=[`find_lib libjxl.[0-9]*.dylib`]
+                if test x$jxl_lib = x; then
+                    jxl_lib=[`find_lib libjxl.dylib`]
+                fi
                 ;;
             *-*-cygwin* | *-*-mingw*)
-                jxl_lib=[`find_lib "libjxl.dll"`]
+                jxl_lib=[`find_lib "libjxl-[0-9]*.dll"`]
+                if test x$jxl_lib = x; then
+                    jxl_lib=[`find_lib "libjxl.dll"`]
+                fi
                 ;;
             *)
                 jxl_lib=[`find_lib "libjxl[0-9]*.so.*"`]