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

cmake: backport support for vendored libavif to SDL2_image #392

Merged
merged 1 commit into from
Oct 16, 2023

Conversation

sezero
Copy link
Contributor

@sezero sezero commented Oct 14, 2023

@madebr: Please review this.

There is a CI run failure with MacOS vendored build like:

   "_dav1d_warp_affine_8x8_8bpc_sse2", referenced from:
      _dav1d_mc_dsp_init_8bpc in mc_tmpl.c.o

... but I think that is a problem with your dav1d cmake support
and not with this backport: I think dav1d is built with either
incorrect nasm output format flags or without any flags at all.
In mac case, we want -fmacho, in windows case we want -fwin,
and in linux/elf case we want -felf, and all need appending a
32 or 64 depending on x86 or x64 target.

@sezero sezero added this to the 2.8.0 milestone Oct 14, 2023
@sezero sezero requested a review from madebr October 14, 2023 11:39
@sezero sezero changed the base branch from main to SDL2 October 14, 2023 11:40
@sezero
Copy link
Contributor Author

sezero commented Oct 14, 2023

OK, I cross-built dav1d for x86_64 mac, the same errors, and cmake
seems to use -f macho64 correctly. On the other hand build using
meson as a dylib with the same environment succeeds.

The problem seems to be with symbol decoration: macho and windows-
x86 symbols must be prefixed with and underscore. This probably has
something to do with meson defining PREFIX macro...

@sezero
Copy link
Contributor Author

sezero commented Oct 14, 2023

For reference, here are the configs generated by meson: meson_configs.zip

@sezero
Copy link
Contributor Author

sezero commented Oct 14, 2023

Manually adding PREFIX define in config.asm and config.h generated
by your dav1d cmake port: build succeeds.

However: There seem to be more missing stuff in dav1d cmake port,
one such thing is STACK_ALIGNMENT in config.asm. There may be others.

Is it not possible to run meson configurator from within cmake??

@sezero
Copy link
Contributor Author

sezero commented Oct 14, 2023

Is it not possible to run meson configurator from within cmake??

I could find https://discourse.cmake.org/t/3985 and
https://cmake.org/cmake/help/latest/module/ExternalProject.html,
but I'm not fluent enough with cmake to make use of them

@sezero
Copy link
Contributor Author

sezero commented Oct 14, 2023

The following seems to work for me and seems to match meson better.
Tested by cross-compiling dav1d as an x86_64 dylib. @madebr: Please
review.
[EDIT: patch applied to dav1d, see next post for discussion]

@sezero
Copy link
Contributor Author

sezero commented Oct 15, 2023

OK, I pushed libsdl-org/dav1d@66d3dd8 to our dav1d 1.2.1-SDL branch and bumped the dav1d revision here accordingly. The CI link failure seems to be cured. @madebr: Please confirm that things are in order. If you confirm, I will bump it in SDL3 branch too.

@slouken: Noticed a few things in your dav1d Android configuration:

  • PREFIX is defined in all configs: Is that accurate, or some copy/paste artifact?

  • There is no config.asm in none of the configs.

  • STACK_ALIGNMENT is defined as 32: is that deliberately picked? Because meson build systems defines it either as 4 or 16 unless user-specified.

  • STACK_ALIGNMENT is defined only for x86_64 config but not arm configs: Intentional??

  • EDIT: Same arguments for the dav1d configs in SDL_image/Xcode (except for PREFIX which is really needed when targeting APPLE platforms.)

@madebr
Copy link
Contributor

madebr commented Oct 15, 2023

Your dav1d changes look fine, I have these suggestions:

  • Make TRIM_DSP_FUNCTIONS a compile definitions, instead of adding it to the config file. CMAKE_BUILD_TYPE is not defined for multi-config generators, such as Visual Studio.
  • Make DAV1D_STACK_ALIGNMENT a configurable option, that can be overriden by users.
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -151,19 +151,15 @@ else()
   set(CONFIG_LOG 0)
 endif()
 
-list(APPEND config01_variables TRIM_DSP_FUNCTIONS)
-if(CMAKE_BUILD_TYPE STREQUAL "Release" OR
-   CMAKE_BUILD_TYPE STREQUAL "MinSizeRel")
-set(TRIM_DSP_FUNCTIONS 1)
-endif()
+# TRIM_DSP_FUNCTIONS=1 if Release or Debug mode
+add_definitions(-DTRIM_DSP_FUNCTIONS=$<BOOL:$<OR:$<CONFIG:Release>,$<CONFIG:MinSizeRel>>>)
 
-if(NOT DAV1D_STACK_ALIGNMENT)
-  if(DAV1D_CPU_X64 OR APPLE OR (CMAKE_SYSTEM_NAME MATCHES ".*Linux"))
-    set(DAV1D_STACK_ALIGNMENT 16)
-  else()
-    set(DAV1D_STACK_ALIGNMENT 4)
-  endif()
+if(DAV1D_CPU_X64 OR APPLE OR CMAKE_SYSTEM_NAME MATCHES ".*Linux")
+    set(DAV1D_STACK_ALIGNMENT_DEFAULT 16)
+else()
+    set(DAV1D_STACK_ALIGNMENT_DEFAULT 4)
 endif()
+set(DAV1D_STACK_ALIGNMENT "${DAV1D_STACK_ALIGNMENT_DEFAULT}" CACHE STRING "Dav1d stack alignment")
 list(APPEND config_variables_value STACK_ALIGNMENT)
 set(STACK_ALIGNMENT ${DAV1D_STACK_ALIGNMENT})

Is it not possible to run meson configurator from within cmake??

I could find https://discourse.cmake.org/t/3985 and https://cmake.org/cmake/help/latest/module/ExternalProject.html, but I'm not fluent enough with cmake to make use of them

I tried this approach and asked on the #cmake channel on cpplang.slack.com, but got no satisfying answer (apart from people telling me they got no interest in cmake, or that I should use a package manager).
While trying, I had to auto-generate a "cross build definition file" for meson which I can't do for all toolchains + cmake gnerators.

@sezero
Copy link
Contributor Author

sezero commented Oct 15, 2023

Your dav1d changes look fine, I have these suggestions:

* Make `TRIM_DSP_FUNCTIONS` a compile definitions, instead of adding it to the config file. `CMAKE_BUILD_TYPE` is not defined for multi-config generators, such as Visual Studio.

OK

* Make `DAV1D_STACK_ALIGNMENT` a configurable option, that can be overriden by users.
[...]
+if(DAV1D_CPU_X64 OR APPLE OR CMAKE_SYSTEM_NAME MATCHES ".*Linux")
+    set(DAV1D_STACK_ALIGNMENT_DEFAULT 16)
+else()
+    set(DAV1D_STACK_ALIGNMENT_DEFAULT 4)
 endif()
+set(DAV1D_STACK_ALIGNMENT "${DAV1D_STACK_ALIGNMENT_DEFAULT}" CACHE STRING "Dav1d stack alignment")

OK. It's guaranteed that cmdline arg like -DDAV1D_STACK_ALIGNMENT works here, yes?

Is it not possible to run meson configurator from within cmake??

I could find https://discourse.cmake.org/t/3985 and https://cmake.org/cmake/help/latest/module/ExternalProject.html, but I'm not fluent enough with cmake to make use of them

I tried this approach and asked on the #cmake channel on cpplang.slack.com, but got no satisfying answer (apart from people telling me they got no interest in cmake, or that I should use a package manager). While trying, I had to auto-generate a "cross build definition file" for meson which I can't do for all toolchains + cmake gnerators.

OK, then.

@sezero
Copy link
Contributor Author

sezero commented Oct 15, 2023

OK. It's guaranteed that cmdline arg like -DDAV1D_STACK_ALIGNMENT works here, yes?

OK, stopped beign lazy and confirmed that it works, pushed the suggested changes to dav1d, bumped the rev here.

Is this PR good?

@slouken: Comments for the notes I wrote about your Android and Xcode configs?

@sezero
Copy link
Contributor Author

sezero commented Oct 15, 2023

One thing I'm curious about is that how this is happening with MSVC:

-- Performing Test LINKER_SUPPORTS_NO_UNDEFINED - Success

@madebr
Copy link
Contributor

madebr commented Oct 15, 2023

One thing I'm curious about is that how this is happening with MSVC:

-- Performing Test LINKER_SUPPORTS_NO_UNDEFINED - Success

SDL3 has this too. The MSVC linker emits a warning, not an error, when it receives an unknown argument.
There is a cmake issue tracking this.

@sezero
Copy link
Contributor Author

sezero commented Oct 15, 2023

One thing I'm curious about is that how this is happening with MSVC:

-- Performing Test LINKER_SUPPORTS_NO_UNDEFINED - Success

SDL3 has this too. The MSVC linker emits a warning, not an error, when it receives an unknown argument. There is a cmake issue tracking this.

Well, we should restrict that to non-windows.. (And also to non-openbsd like SDL2/3..)

@sezero
Copy link
Contributor Author

sezero commented Oct 16, 2023

Anyways, merging this patch. The rest should be subject to new tickets.

@sezero sezero merged commit 09fab01 into libsdl-org:SDL2 Oct 16, 2023
8 checks passed
@sezero sezero deleted the sdl2avif branch October 16, 2023 17:00
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.

2 participants