Repository navigation
Build fpcalc against the system FFmpeg, and fix building against FFmpeg 4.x - #166
Conversation
The test jobs pass BUILD_TOOLS=OFF, so the FFmpeg audio reader is never compiled there, and the packaging jobs build it against a pinned FFmpeg 8.0. Nothing covered the combination distributions actually use: tools enabled, system FFmpeg. That left the FFmpeg detection in CMakeLists.txt and FindFFmpeg.cmake untested for older releases. Add a job that builds with BUILD_TOOLS=ON against the FFmpeg that ubuntu-22.04 ships, runs the test suite, and checks fpcalc still produces the reference fingerprint. It prints the FFmpeg package versions first so a failure says which ones it was built against.
The audio reader uses AVChannelLayout, ch_layout and av_channel_layout_*, all of which arrived in libavutil 57.24.100 (FFmpeg 5.1). Building the tools against anything older fails outright, so despite #158 the 4.2 to 5.0 range that CMake accepts could not actually be compiled. Add ffmpeg_compat.h with the version check and a macro for the channel count, and give both call sites in the reader and both setters in the swresample processor a pre-5.1 path using the bitmask layout, av_get_default_channel_layout and the in_channel_layout/out_channel_layout options. The new test-linux-tools job is what found this, building against the FFmpeg that ubuntu-22.04 ships.
avformat_open_input() and av_find_best_stream() took non-const pointers before libavformat 59 (FFmpeg 5.0), so the const declarations the reader uses do not compile there. Qualify the two declarations through a macro that is empty below 59.
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2 participants
Reopened from #165 (GitHub will not reopen a merged PR, so this is a fresh one from the same branch — same three commits, unchanged).
Why the title changed
The single-commit squash of #165 read "Build fpcalc against the system FFmpeg in CI", which hid the more significant half: the branch also fixes building against FFmpeg 4.x, which had been broken outright. Worth keeping visible in the history rather than only in the diff.
The CI gap
test-linuxandtest-macosboth pass-DBUILD_TOOLS=OFF, so the FFmpeg audio reader is never compiled there — FFmpeg is only pulled in as an FFT provider for theavfftentry. The packaging jobs do buildfpcalc, butpackage/build.shpinsFFMPEG_VERSION=8.0.So the combination distributions use — tools enabled, system FFmpeg — was never built, which is exactly the configuration the FFmpeg detection in
CMakeLists.txtandcmake/modules/FindFFmpeg.cmakeexists for.test-linux-toolsbuilds with-DBUILD_TOOLS=ONagainst the FFmpegubuntu-22.04ships (4.4.2), runs the suite, and checksfpcalcstill reproducestests/data/test.mp3.fpcalc.out. It prints the FFmpeg package versions first so a failure reports what it built against.What that job found
Two independent reasons the source could not compile against 4.x, so the 4.2–5.0 range #158 taught CMake to accept was never actually buildable:
AVChannelLayout,ch_layoutandav_channel_layout_*arrived in libavutil 57.24.100 (FFmpeg 5.1) and were used unconditionally.avformat_open_input()andav_find_best_stream()took non-constAVInputFormat*/AVCodec**before libavformat 59 (FFmpeg 5.0).New
src/audio/ffmpeg_compat.hholds both version checks. The reader and the swresample processor get guarded pre-5.1 paths using the bitmask layout,av_get_default_channel_layout()and thein_channel_layout/out_channel_layoutoptions.Modern builds take the same code they always did — the packaging jobs against FFmpeg 8.0 confirm that, and
fpcalcproduces the reference fingerprint on both 4.4.2 and 8.0.Note
It took two rounds to get green, each revealing the next incompatibility, which says 4.x had drifted entirely out of coverage. The alternative is still to require FFmpeg >= 5.1, have CMake reject anything older and point the job at a newer runner — less code to carry. This takes the other route, and the new job is what keeps it honest.