From 6fd1ea0702a1578e837142c4f7b9d35095984328 Mon Sep 17 00:00:00 2001 From: Pierre-Antoine Bannier Date: Fri, 10 Apr 2026 10:33:22 +0200 Subject: [PATCH] fix: Windows build for sam3_image/sam3_video + close CI coverage gap Fixes #5. The Windows job in CI was silently skipping sam3_image and sam3_video because SDL2 was never installed on the runner, so find_package(SDL2 QUIET) returned empty and the GUI targets were skipped. The headless targets happened to be portable, so CI stayed green while the GUI pipeline was broken on Windows. Code fixes (all four confirmed in issue #5 by @mimi3421): - sam3.cpp: ffmpeg/ffprobe popen calls were not Windows-safe. Added a _WIN32 shim for NUL vs /dev/null and for "rb" vs "r" popen mode. Without binary mode, _popen runs in text mode on Windows and CRLF translation silently corrupts the raw RGB stream from ffmpeg. - examples/main_image.cpp + main_video.cpp: added SDL_MAIN_HANDLED so SDL does not rewrite main() to SDL_main() (we do not link SDL2main). Replaced the GL/gl.h / OpenGL/gl3.h ifdef block with a single include, which abstracts the windows.h prerequisite and exposes modern GL prototypes uniformly across platforms. - examples/third-party/CMakeLists.txt: removed IMGUI_IMPL_OPENGL_LOADER_CUSTOM. ImGui v1.91.8 ships a built-in gl3w loader in imgui_impl_opengl3_loader.h that is used when no loader mode is set. LOADER_CUSTOM was telling ImGui "the user provides GL loading" when nothing did; Linux built by accident because libGL.so exports GL 3+ symbols at link time, MSVC's opengl32.lib does not. CI fix (.github/workflows/ci.yml): - New step installs SDL2 via vcpkg on the Windows runner. - Windows cmake_args now passes the vcpkg toolchain + x64-windows triplet. - Verify step now asserts that sam3_image.exe and sam3_video.exe actually exist, so the silent-skip regression cannot return. Verified locally on macOS: both GUI examples rebuild cleanly after the SDL_opengl.h switch and the loader change. Co-Authored-By: Claude Opus 4.6 (1M context) --- .github/workflows/ci.yml | 20 ++++++++++++++++++-- examples/main_image.cpp | 12 +++--------- examples/main_video.cpp | 12 +++--------- examples/third-party/CMakeLists.txt | 27 +++++++++++++++++++++++---- sam3.cpp | 22 +++++++++++++--------- 5 files changed, 60 insertions(+), 33 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7a68f92..e76d312 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -30,7 +30,7 @@ jobs: - name: Windows (MSVC) os: windows-latest - cmake_args: "-DGGML_METAL=OFF -DSAM3_METAL=OFF -A x64" + cmake_args: "-DGGML_METAL=OFF -DSAM3_METAL=OFF -A x64 -DCMAKE_TOOLCHAIN_FILE=C:/vcpkg/scripts/buildsystems/vcpkg.cmake -DVCPKG_TARGET_TRIPLET=x64-windows" build_args: "--config Release" steps: @@ -47,6 +47,11 @@ jobs: if: runner.os == 'macOS' run: brew install ninja + - name: Install SDL2 (Windows) + if: runner.os == 'Windows' + shell: pwsh + run: vcpkg install sdl2:x64-windows + - name: ccache uses: hendrikmuhs/ccache-action@v1 with: @@ -64,7 +69,7 @@ jobs: - name: Build run: cmake --build build ${{ matrix.build_args }} --parallel - - name: Verify library + - name: Verify build artifacts shell: bash run: | if [ -f build/libsam3.a ]; then @@ -77,3 +82,14 @@ jobs: echo "ERROR: sam3 library not found" exit 1 fi + + if [ "$RUNNER_OS" = "Windows" ]; then + for exe in sam3_image.exe sam3_video.exe; do + if [ -f "build/examples/Release/$exe" ]; then + echo "Found build/examples/Release/$exe" + else + echo "ERROR: $exe not built (GUI examples were silently skipped — likely SDL2 not found)" + exit 1 + fi + done + fi diff --git a/examples/main_image.cpp b/examples/main_image.cpp index ecf2b79..3af8b7a 100644 --- a/examples/main_image.cpp +++ b/examples/main_image.cpp @@ -14,20 +14,14 @@ #include "sam3.h" +#define SDL_MAIN_HANDLED #include +#include + #include #include #include -#ifdef __APPLE__ -#ifndef GL_SILENCE_DEPRECATION -#define GL_SILENCE_DEPRECATION -#endif -#include -#else -#include -#endif - #include #include #include diff --git a/examples/main_video.cpp b/examples/main_video.cpp index ab4dd18..b0efe00 100644 --- a/examples/main_video.cpp +++ b/examples/main_video.cpp @@ -13,20 +13,14 @@ #include "sam3.h" +#define SDL_MAIN_HANDLED #include +#include + #include #include #include -#ifdef __APPLE__ -#ifndef GL_SILENCE_DEPRECATION -#define GL_SILENCE_DEPRECATION -#endif -#include -#else -#include -#endif - #include #include #include diff --git a/examples/third-party/CMakeLists.txt b/examples/third-party/CMakeLists.txt index cf41533..a31b2a5 100644 --- a/examples/third-party/CMakeLists.txt +++ b/examples/third-party/CMakeLists.txt @@ -1,5 +1,23 @@ -# ImGui library with SDL2 + OpenGL3 backend -set(IMGUI_DIR ${CMAKE_CURRENT_SOURCE_DIR}/imgui) +# ImGui library with SDL2 + OpenGL3 backend. +# +# ImGui is not vendored. Prefer a local checkout at examples/third-party/imgui +# (useful for offline builds); otherwise fetch it at configure time via +# FetchContent, pinned to a known-good tag. +if(EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/imgui/imgui.cpp") + set(IMGUI_DIR ${CMAKE_CURRENT_SOURCE_DIR}/imgui) + message(STATUS "Using local ImGui checkout at ${IMGUI_DIR}") +else() + include(FetchContent) + FetchContent_Declare( + imgui + GIT_REPOSITORY https://github.com/ocornut/imgui.git + GIT_TAG v1.91.8 + GIT_SHALLOW TRUE + ) + FetchContent_MakeAvailable(imgui) + set(IMGUI_DIR ${imgui_SOURCE_DIR}) + message(STATUS "Fetched ImGui v1.91.8 into ${IMGUI_DIR}") +endif() add_library(imgui-sdl2 STATIC ${IMGUI_DIR}/imgui.cpp @@ -26,8 +44,9 @@ else() target_link_libraries(imgui-sdl2 PUBLIC OpenGL::GL) endif() +# On non-Apple, leave IMGUI_IMPL_OPENGL_LOADER_* undefined so imgui uses its +# bundled imgl3w loader (required for GL 3+ on MSVC where opengl32.lib only +# exports GL 1.1 symbols). if(APPLE) target_compile_definitions(imgui-sdl2 PUBLIC GL_SILENCE_DEPRECATION) -else() - target_compile_definitions(imgui-sdl2 PUBLIC IMGUI_IMPL_OPENGL_LOADER_CUSTOM) endif() diff --git a/sam3.cpp b/sam3.cpp index ad0ffc9..43d88a8 100644 --- a/sam3.cpp +++ b/sam3.cpp @@ -23,8 +23,12 @@ #define popen _popen #define pclose _pclose #define mkdir(path, mode) _mkdir(path) +#define SAM3_NULL_DEV "NUL" +#define SAM3_POPEN_READ "rb" #else #include +#define SAM3_NULL_DEV "/dev/null" +#define SAM3_POPEN_READ "r" #endif /* C++ standard library */ @@ -12289,16 +12293,16 @@ sam3_image sam3_decode_video_frame(const std::string& video_path, int frame_inde snprintf(cmd, sizeof(cmd), "ffmpeg -nostdin -loglevel error -i \"%s\" " "-vf \"select=eq(n\\,%d)\" -vsync vfr -frames:v 1 " - "-f rawvideo -pix_fmt rgb24 pipe:1 2>/dev/null", - video_path.c_str(), frame_index); + "-f rawvideo -pix_fmt rgb24 pipe:1 2>%s", + video_path.c_str(), frame_index, SAM3_NULL_DEV); // First, get dimensions char info_cmd[1024]; snprintf(info_cmd, sizeof(info_cmd), "ffprobe -v error -select_streams v:0 " - "-show_entries stream=width,height -of csv=p=0 \"%s\" 2>/dev/null", - video_path.c_str()); - FILE* fp = popen(info_cmd, "r"); + "-show_entries stream=width,height -of csv=p=0 \"%s\" 2>%s", + video_path.c_str(), SAM3_NULL_DEV); + FILE* fp = popen(info_cmd, SAM3_POPEN_READ); if (!fp) return img; int w = 0, h = 0; if (fscanf(fp, "%d,%d", &w, &h) != 2) { @@ -12312,7 +12316,7 @@ sam3_image sam3_decode_video_frame(const std::string& video_path, int frame_inde img.channels = 3; img.data.resize(w * h * 3); - fp = popen(cmd, "r"); + fp = popen(cmd, SAM3_POPEN_READ); if (!fp) { img.data.clear(); return img; @@ -12333,9 +12337,9 @@ sam3_video_info sam3_get_video_info(const std::string& video_path) { snprintf(cmd, sizeof(cmd), "ffprobe -v error -select_streams v:0 " "-show_entries stream=width,height,r_frame_rate,nb_frames " - "-of csv=p=0 \"%s\" 2>/dev/null", - video_path.c_str()); - FILE* fp = popen(cmd, "r"); + "-of csv=p=0 \"%s\" 2>%s", + video_path.c_str(), SAM3_NULL_DEV); + FILE* fp = popen(cmd, SAM3_POPEN_READ); if (!fp) return info; int w = 0, h = 0, num = 0, den = 1, nf = 0;