fix: Windows build for sam3_image/sam3_video + close CI gap#7
Merged
Conversation
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 <SDL_opengl.h> 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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #5. The four bugs @mimi3421 reported are all real, and there is a fifth finding: Windows is in CI, but the job silently skipped
sam3_image/sam3_videobecause SDL2 was never installed on the runner, sofind_package(SDL2 QUIET)returned empty and CMake dropped both GUI targets. The library and headless examples happened to be portable, so CI stayed green while the Windows GUI pipeline was broken.Code fixes
sam3.cpp— theffmpeg/ffprobepopencalls were not Windows-safe:2>/dev/nulldoes not exist incmd.exe→ now usesNULon Windows via a_WIN32shim.popen(cmd, "r")opens the pipe in text mode on Windows, and CRLF translation silently mangles the raw RGB byte stream from ffmpeg → now opens with"rb"on Windows.examples/main_image.cpp+examples/main_video.cpp:#define SDL_MAIN_HANDLEDso SDL does not rewritemain()→SDL_main()(we don't linkSDL2main).GL/gl.h/OpenGL/gl3.h#ifdefblock with a single#include <SDL_opengl.h>, which abstracts thewindows.hprerequisite and exposes modern GL prototypes uniformly.examples/third-party/CMakeLists.txt— removedIMGUI_IMPL_OPENGL_LOADER_CUSTOM. ImGui v1.91.8 uses its bundledimgl3wloader (imgui_impl_opengl3_loader.h) when no loader mode is defined.LOADER_CUSTOMwas telling ImGui "the user provides GL function loading" when nothing did — Linux built by accident becauselibGL.soexports GL 3+ symbols at link time; MSVC'sopengl32.libdoes not.CI fix (
.github/workflows/ci.yml)cmake_argsnow passes-DCMAKE_TOOLCHAIN_FILE=C:/vcpkg/scripts/buildsystems/vcpkg.cmake -DVCPKG_TARGET_TRIPLET=x64-windows.sam3_image.exeandsam3_video.exeexist on Windows, so the silent-skip regression cannot return.Out of scope (tracked separately)
The issue also lists three side suggestions: mask-export filename collision on repeated clicks, GUI threading, and a Vulkan backend. These are unrelated to "does it build on Windows" and deserve separate issues.
Test plan
sam3_imageandsam3_videorebuild cleanly after theSDL_opengl.h+ loader changes (verified).LOADER_NONE-equivalent loader change is the risky bit for Linux).sam3_image.exeandsam3_video.exeand the verify step finds them — this is the load-bearing check, because if both EXEs link, every bug in the issue is fixed at compile/link time.sam3_video.exerun decodes frames without CRLF corruption (can't validate from macOS).🤖 Generated with Claude Code