MSVC portability: export macro, math constants, logging macros, and a Visual Studio CI job - #171
Merged
Merged
Conversation
added 13 commits
August 4, 2026 23:01
vidstab_api.h decided between __declspec(dllexport) and dllimport on
defined(_WIN32) && defined(_MSC_VER), which left two cases wrong:
- MinGW and clang-cl builds got no decoration at all, although both
understand __declspec and need it to produce a usable DLL,
- a static build (BUILD_SHARED_LIBS=OFF) told every consumer that the
symbols were dllimport, so linking against the .lib failed.
Replace it with vidstab_export.h, which keys the export case off the
vidstab_EXPORTS macro that CMake already defines for a shared build, the
static case off VIDSTAB_STATIC_DEFINE, and tests the platform rather than
the compiler. Names and semantics are those of generate_export_header(),
so the generated header can be dropped in unchanged should the three
CMake projects here ever be merged into one.
VS_API is also dropped from the 18 struct declarations that carried it.
An export attribute on an aggregate exports nothing in C -- there is no
code attached to a type -- and once VS_API expands to a GCC visibility
attribute rather than to nothing, "typedef struct VS_API foo" would draw
a warning on every include.
The test suite and the transcode plugins compile the library sources
straight into their targets, so both now define VIDSTAB_STATIC_DEFINE.
compat.h existed to define _USE_MATH_DEFINES before <math.h> and to spell out M_PI as a fallback. A single translation unit included it, while transformtype.c carried its own inline copy of the same block, so whether M_PI was available depended on which file you were in. _USE_MATH_DEFINES is a property of the whole compilation, so define it once in the build for MSVC and let every source just include <math.h>. compat.h and the duplicated block are gone; localmotion2transform.c includes <math.h> directly, which is what it wanted all along.
The GNU ", ## args" extension and MSVC's __VA_ARGS__ needed two separate definitions of every logging macro, the MSVC side going through a helper macro to get there. Both existed only to cope with a call that stops after the format string, leaving the variadic part empty. Passing the message type ahead of __VA_ARGS__ removes the problem instead of working around it twice: tag and format are then part of the variadic argument list, which is never empty, so plain C99 suffices. Checked with -std=c99 -pedantic against call sites with and without varargs.
The MSVC branch asked for /arch:AVX2, which is not a statement about SSE2 availability at all: it lets the compiler emit AVX2 into all generated code, so the resulting binaries fault on any CPU older than Haswell -- for a library whose own SIMD is plain SSE2 and which is meant to run wherever ffmpeg does. SSE2 needs no /arch on x64, where it is baseline, nor on 32 bit x86, where MSVC has defaulted to it since 2012, so the flag goes away with nothing replacing it. While here, stop claiming SSE3/SSSE3/SSE4.1 are present: none of them is baseline on any Windows target, and no caller in this project asks about them.
Three things kept the tests from compiling with MSVC, none of them about
the code under test:
- timeOfDayinMS() used gettimeofday() from <sys/time.h>, which Windows
does not have. It now reads a monotonic clock on both branches --
GetTickCount64() on Windows, clock_gettime(CLOCK_MONOTONIC)
elsewhere. Only differences between two calls were ever used, and a
monotonic source additionally cannot produce a negative interval
when the wall clock is adjusted mid-measurement.
- the GCC warning and -std= flags, and -msse2/-ffast-math, were passed
to every compiler.
- libm and libgomp were linked unconditionally; MSVC has the math
functions in its CRT and drives OpenMP from /openmp, so neither
library exists there.
The Linux build is unaffected: 35/35 unit tests still pass.
The Windows port had no CI at all, so it could only break silently -- and unlike the Linux builds it has no stream of users to report it. The job builds the library both shared and static, because src/vidstab_export.h resolves VS_API differently for each and only the shared build exercises the dllexport side, then builds and runs the test suite, including the forced scalar and SSE2 dispatch runs the x86_64 job does.
cl.exe is only on PATH inside a Visual Studio developer prompt, so the step failed before any building happened. CMake prints the compiler it selected during configure, which is the same information.
__const is a glibc-internal alias from <sys/cdefs.h>, deprecated even there, and MSVC has never had it -- it turned every file that includes testframework.h into a wall of syntax errors. The parameter names go with it: identifiers with two leading underscores are reserved for the implementation, which is precisely why they collided here.
___FUNCTION fell back to ((__const char *) 0) whenever __STDC_VERSION__ was not >= 199901L, which is MSVC's default in C mode -- so the fallback was exactly the branch MSVC took, and __const does not exist there. Add an MSVC branch using __FUNCTION__, spell the last-resort fallback with plain const, and provide __STRING (another <sys/cdefs.h> export, and only a stringification) where the platform lacks it.
Both were declared with a bound that is a variable holding a macro constant, which makes them variable length arrays. MSVC implements no part of that optional C99 feature. The bound is the same either way.
The fixed point interpolators were defined as plain "inline", which in C99 declares an inline definition, not an external one. GCC still emitted an external definition because the header declares them without inline, so the Linux link worked by that rule; MSVC does not, and transform.c takes the address of every one of them to fill in the function pointer in VSTransformData, so all four came out unresolved: transform.obj : error LNK2019: unresolved external symbol interpolateZero Nothing is lost by dropping the keyword: they are reached through a function pointer, so no call site could ever have been inlined, and the compiler remains free to inline the direct calls within the file. "inline static bicub_kernel" is untouched -- static inline has an external definition in neither language and is only used in this file.
test_store_restore wrote and read the .trf through handles opened in text mode. On Windows that translates \n to \r\n on the way out and back on the way in, and stops reading at the first 0x1A -- so the binary serialization mode lost most of its payload and the test failed with Cannot parse the given number of localmotions (got 37 of 191)! which is precisely the diagnosis src/serialize.c prints for this. On POSIX the mode letter has no effect, so this is a no-op there. Found by the new MSVC job, on its first green build.
The golden checksum is 4339987323, which is larger than 2^32-1, and both the constant and the accumulator were unsigned long. That is 64 bit on LP64 Linux and 32 bit on Windows, so on MSVC the constant was truncated and the running sum wrapped, and the test failed with a value that could never have matched: tests/test_draw.c:245: Test Failed: sum == 4339987323UL unsigned long long is 64 bit everywhere C99 is. The checksum is unchanged and still passes on Linux.
georgmartius
marked this pull request as ready for review
August 4, 2026 21:52
added 2 commits
August 5, 2026 22:54
The rewritten header was also renamed to vidstab_export.h, to match what
CMake's generate_export_header() emits. That is worth very little --
the macro names already match, so switching to the generated header is
a matter of repointing the includes -- and it is not free: vidstab_api.h
shipped in v1.1.2 and is installed into ${includedir}/vid.stab, so the
rename churns the file list of every package for no gain to anybody.
Contents and semantics are unchanged; only the file name and its include
guard go back to what v1.1.2 installed.
The installed headers decorate the public API with __declspec(dllimport) unless VIDSTAB_STATIC_DEFINE says the library is static (src/vidstab_api.h). Anyone linking the CMake target gets that define from the target's PUBLIC compile definitions; anyone going through pkg-config got nothing, so on Windows a static libvidstab could not be linked against at all. Cflags now carries the define for a static build and is unchanged for a shared one. Verified in both modes by installing to a prefix and building a consumer with `pkg-config --cflags --libs vidstab`.
This was referenced Aug 5, 2026
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.
Replaces the parts of the MSVC port that were hand-rolled or wrong, and puts a Visual Studio runner in CI so the Windows build stops breaking silently.
build: replace the hand-rolled export macro header—vidstab_api.hkeyed the dllexport/dllimport choice off_WIN32 && _MSC_VER, which left MinGW and clang-cl with no decoration and told static-build consumers that the symbols weredllimport. Replaced byvidstab_export.h, using thevidstab_EXPORTS/VIDSTAB_STATIC_DEFINEcontract of CMake'sgenerate_export_header().VS_APIalso dropped from 18typedef structdeclarations, where an export attribute does nothing in C.build: ask MSVC for the math constants from the build—compat.hand a duplicated copy of it insidetransformtype.care gone;_USE_MATH_DEFINESis set once for the whole compilation.vidstabdefines: one portable form for the vs_log_* macros— passing the message type ahead of__VA_ARGS__removes the empty-variadic problem instead of working around it once per compiler. Checked with-std=c99 -pedantic.FindSSE: do not force /arch:AVX2 on every MSVC build— that flag lets the compiler emit AVX2 into all generated code, so every Windows binary faulted on pre-Haswell CPUs. SSE2 needs no/archon x64. The per-kernel/arch:AVX2inVidstabSimd.cmakeis the correct mechanism and is untouched.tests: build and run the test suite under MSVC— portable monotonic timer instead ofgettimeofday(), and the GCC-only flags and libm/libgomp links are no longer passed to MSVC.ci: build and test with MSVC on a Visual Studio runner— shared and static library builds plus the test suite, with the forced scalar and SSE2 dispatch runs.Draft until the Windows job has actually gone green; everything else is verified on Linux (35/35 unit tests, no new warnings).
🤖 Generated with Claude Code