libwebp simd code not properly built with Cmake build system

[sezero]

As the title says, e.g. with i686 + sse2 / sse4.1: SIMD build flags aren't properly passed to individual SIMD-specific sources in cmake E.g. for SSE2: the src checks two different macros, WEBP_USE_SSE2 and WEBP_HAVE_SSE2. The generated config.h defines WEBP_HAVE_SSE2 but since -msse2 isn't passed to sse2-specific sources and since the compiler doesn't doesn't default to SSE2 by default (at least that is the case with my toolchain) src/dsp/cpu.h does not define WEBP_USE_SSE2, and certain sources doesn't include SIMD paths as a result (sharpyuv_sse2.c.o is an empty object for me, for example.) Autotools does it just right, see individual Makefile.am files for details.

For the time being, I will be using autotools when building windows dlls (and when building sharpyuv for libavif.) For that, I created the attached patch while I was there, which I'll be pushing shortly: 0001-build-fixes-updates.patch.txt

Any improvements to cmake side requires @madebr I guess. Or, if we don't want to bother, we can close this.

[sezero]

I may be wrong about the whole tree: sharpyuv is certainly affected though.

EDIT: Yes, looks like only sharpyuv is affected..

[sezero]

@madebr: Is the following correct?

diff --git a/cmake/cpu.cmake b/cmake/cpu.cmake
index 7513ca8..20ae612 100644
--- a/cmake/cpu.cmake
+++ b/cmake/cpu.cmake
@@ -107,6 +107,7 @@ foreach(I_SIMD RANGE ${WEBP_SIMD_FLAGS_RANGE})
   # Check which files we should include or not.
   list(GET WEBP_SIMD_FILE_EXTENSIONS ${I_SIMD} WEBP_SIMD_FILE_EXTENSION)
   file(GLOB SIMD_FILES "${CMAKE_CURRENT_LIST_DIR}/../"
+       "sharpyuv/*${WEBP_SIMD_FILE_EXTENSION}"
        "src/dsp/*${WEBP_SIMD_FILE_EXTENSION}")
   if(WEBP_HAVE_${WEBP_SIMD_FLAG})
     # Memorize the file and flags.

[madebr]

Adding sources won't do anything, as the linker can resolve all missing links right now. Does the following patch fix your intel issue?

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -69,6 +69,20 @@ if(WIN32)
   option(WEBP_UNICODE "Build Unicode executables." ON)
 endif()

+set(WEBP_HAVE_SSE2 0)
+set(WEBP_HAVE_SSE41 0)
+include(CheckCCompilerFlag)
+check_c_compiler_flag("-msse2" COMPILER_SUPPORTS_-msse2)
+if(COMPILER_SUPPORTS_-msse2)
+  set(WEBP_HAVE_SSE2 1)
+  add_compile_options(-msse2)
+  check_c_compiler_flag("-msse4.1" COMPILER_SUPPORTS_-msse4.1)
+  if(COMPILER_SUPPORTS_-msse4.1)
+    set(WEBP_HAVE_SSE41 1)
+    add_compile_options(-msse4.1)
+  endif()
+endif()
+
 if(WEBP_BUILD_WEBP_JS)
   set(WEBP_BUILD_ANIM_UTILS OFF)

[sezero]

No, that's not the issue. The part of the cmake'ry I referenced above does check for simd sources and adds the necessary simd compiler flags if available, but it does so only for the ones under src/dsp/. Adding sharpyuv/ glob there does fix the issue; however I am not sure whether or not it is correct in terms of cmake, i.e. I am not sure what the file(GLOB ....) does with "${CMAKE_CURRENT_LIST_DIR}/../": Should it instead be like the following??

diff --git a/cmake/cpu.cmake b/cmake/cpu.cmake
index 7513ca8..c2fae13 100644
--- a/cmake/cpu.cmake
+++ b/cmake/cpu.cmake
@@ -106,8 +106,9 @@ foreach(I_SIMD RANGE ${WEBP_SIMD_FLAGS_RANGE})
   endif()
   # Check which files we should include or not.
   list(GET WEBP_SIMD_FILE_EXTENSIONS ${I_SIMD} WEBP_SIMD_FILE_EXTENSION)
-  file(GLOB SIMD_FILES "${CMAKE_CURRENT_LIST_DIR}/../"
-       "src/dsp/*${WEBP_SIMD_FILE_EXTENSION}")
+  file(GLOB SIMD_FILES
+   "${CMAKE_CURRENT_LIST_DIR}/../sharpyuv/*${WEBP_SIMD_FILE_EXTENSION}"
+   "${CMAKE_CURRENT_LIST_DIR}/../src/dsp/*${WEBP_SIMD_FILE_EXTENSION}")
   if(WEBP_HAVE_${WEBP_SIMD_FLAG})
     # Memorize the file and flags.
     foreach(FILE ${SIMD_FILES})

[madebr]

Yes, your change is fine. When I compile with -DCMAKE_C_FLAGS="-m32 -march=i486", and add

#else
#error no sse2
#endif

to sharpyuv/sharpyuv_sse2.c, it fails to build. With your changes, it builds.

[sezero]

With your changes, it builds.

Thanks. But, which patch is the correct one? Is it the one from https://github.com/libsdl-org/SDL_image/issues/423#issuecomment-1935094588 or the one from https://github.com/libsdl-org/SDL_image/issues/423#issuecomment-1935415492 ?

[madebr]

I tested both, and they both work. The cpu.cmake script sets variables, that are used by the main CMakeLists.txt. What matters is that the paths are relative to this main script.

That said, the 2nd approach is safer. Alternatively, you can also do:

+  file(GLOB SIMD_FILES
+   "${PROJECT_SOURE_DIR}/sharpyuv/*${WEBP_SIMD_FILE_EXTENSION}"
+   "${PROJECT_SOURE_DIR}/src/dsp/*${WEBP_SIMD_FILE_EXTENSION}")

PROJECT_SOURCE_DIR is the folder of the most recent call to project.

[sezero]

OK, applied my second patch for now. Please feel free to revise / improve further.

[madebr]

I'd even say the patch isfit for upstreaming. The steps don't look too hard: https://github.com/webmproject/libwebp/blob/main/CONTRIBUTING.md#sending-patches

[sezero]

I'd even say the patch isfit for upstreaming. The steps don't look too hard: https://github.com/webmproject/libwebp/blob/main/CONTRIBUTING.md#sending-patches

Done. (Pulled my hair a lot when trying to upload but figured it out eventually.)