build: bundle a static ffmpeg, libsrt and librist (closes #6) - #4
Open
datagutt wants to merge 12 commits into
Open
build: bundle a static ffmpeg, libsrt and librist (closes #6)#4datagutt wants to merge 12 commits into
datagutt wants to merge 12 commits into
Conversation
The plugin linked whatever ffmpeg the host obs shipped, which caused two problems. obs-deps pins libsrt 1.5.2 (2022) and librist 0.2.7, so every transport fix since then was missing. And obs bumped ffmpeg 7 (avcodec-61) to 8.1 (avcodec-62) between the 32.1 and 32.2 lines, so a binary linked against one would not load where the other was present, forcing a separate release artifact per obs line. deps/build-deps.sh now builds a pinned decode-only ffmpeg 8.1.2 with libsrt 1.5.6, librist 0.2.18 and mbedtls 3.6.4, linked statically. libobs is the only version sensitive link left, and it gates modules on (major, minor) <= host, so one artifact per platform covers 32.1 and every newer line. Symbol isolation is load-bearing: obs has already loaded its own ffmpeg exporting the same names, so an exported avcodec_open2 here would resolve to obs's copy and run against another library's structs. The module is built with hidden visibility, --exclude-libs,ALL and a version script that exports only obs_module_*. scripts/verify-plugin.sh asserts this after every build, since a successful compile does not prove it. Two silent failures worth recording. ffmpeg's configure ignores unknown names in an --enable-* list, so --enable-protocol=srt (the component is libsrt) produced a build with CONFIG_LIBSRT=yes and no srt protocol at all; build-deps.sh now asserts every required component against config.mak. And librist's meson found the host's shared libmbedcrypto instead of ours, which fix_mbedtls_pc corrects for both librist and libsrt.
WalkthroughThe plugin now builds a pinned static media dependency stack, links it through CMake with symbol isolation, verifies platform binaries, and publishes one artifact per platform. Build, release, and project documentation reflect the bundled stack and OBS compatibility model. ChangesBundled media stack
Project documentation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CI as Build workflow
participant Deps as deps/build-deps.sh
participant CMake as CMake
participant Verify as verify-plugin.sh
participant Release as Release workflow
CI->>Deps: Build bundled static dependencies
CI->>CMake: Configure and build plugin
CI->>Verify: Check dependencies and exports
Verify-->>CI: Return verification status
CI->>Release: Provide platform artifact
Release->>Release: Package platform archive
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Three findings from the first CI run. meson aborts on Windows with "Found GNU link.exe instead of MSVC link.exe": MSYS2 ships a coreutils /usr/bin/link.exe that shadows MSVC's linker on PATH. CMake is unaffected because it resolves the linker next to the compiler, but meson probes PATH. cl.exe and link.exe share a directory, so build-deps.sh now puts that directory first rather than modifying MSYS2. The macOS export check misparsed nm output. nm renders an indirect symbol with an empty address column, which shifts every field and made "(indirect" look like an exported symbol. Use nm -gUj and drop the column parsing; the build was correct, the assertion was not. The macOS artifact carried an absolute /opt/homebrew/opt/jansson/lib/libjansson.4.dylib load command, so the shipped plugin required Homebrew on the user's machine. The plugin never calls jansson; it is only reachable from the OBS headers, so the include path is needed and the library is not. verify-plugin.sh now also rejects any load command outside @rpath, /usr/lib and /System so this cannot regress.
meson names its static library librist.a even under MSVC, but FFmpeg's msvc toolchain rewrites the -lrist in librist.pc into rist.lib, so configure failed with "librist >= 0.2.7 not found using pkg-config" even though the library had just built and installed. Install the .lib name alongside it. Patching the .pc back to an absolute path would also work, but that is the link-ordering trap fix_mbedtls_pc exists to undo. FFmpeg's configure only reports "X not found using pkg-config" on failure, which does not distinguish a missing library from one whose name or link order the toolchain got wrong. Dump the tail of config.log so the next failure of this class is diagnosable from the CI log alone.
FFmpeg's configure failed to link against librist and mbedcrypto with
unresolved __imp_strdup, __imp_fopen and friends. Not a missing library: a
C runtime mismatch. cl.exe defaults to the static CRT when given no flag,
while CMake and meson both emit /MD, so FFmpeg silently built /MT against
/MD dependencies and the __imp_* import thunks had nothing to resolve to.
Mixing them is worse than a link error. /MD and /MT binaries get separate
heaps and separate stdio state, so a buffer allocated in one and freed in
the other corrupts the process. The plugin DLL is /MD (CMake's default, and
what libobs uses), so /MD is the target and all three build systems are now
pinned to it explicitly instead of left to their defaults. CMP0091 has to be
forced too, or a dependency declaring cmake_minimum_required below 3.15 gets
the old policy and ignores the setting.
Also two latent bugs in the same area. meson_common is seeded with the
options every platform needs so it is never empty, because macOS still ships
bash 3.2 where expanding an empty array under set -u aborts. And the system
library dedupe no longer uses the "${arr[@]:-}" idiom to assign, which
yields a single empty element on an empty array and would have written an
empty entry into the CMake link list.
libsrt builds srt_static.lib and meson builds librist.a, but FFmpeg's msvc toolchain rewrites the -lsrt and -lrist it reads from their .pc files into srt.lib and rist.lib. The mismatch surfaces as "not found using pkg-config" from a library that installed successfully seconds earlier, which is a misleading enough error to be worth handling once rather than per library. ensure_msvc_lib_name replaces the librist specific copy added earlier and now covers both, failing loudly with a directory listing if it cannot find any candidate rather than leaving configure to misreport it.
mbedTLS draws entropy from BCryptGenRandom on Windows but its generated .pc files do not declare bcrypt, so a static link through pkg-config fails on that single symbol. FFmpeg reports it as "ERROR: mbedTLS not found", which points at the wrong thing entirely. Also give configure an explicit include and library search path. Not every probe goes through pkg-config; when FFmpeg falls back to linking a bare -lmbedtls it has no search path and fails with "cannot open input file mbedtls.lib" for a library that is sitting right there.
FFmpeg's configure reached the zlib check for the first time and failed with "zlib requested but not found". Linux and macOS supply zlib as a system library; Windows has none. Building it is preferable to dropping --enable-zlib on Windows: a codec or container feature that silently differs per platform is a worse outcome than one more short build step. ZLIB joins the asserted component list so it cannot go missing quietly either. ensure_msvc_lib_name now takes extra candidate basenames, since zlib's CMake names its static output zlibstatic while FFmpeg asks for z.lib. The stdbit.h error just above the failure in the log is unrelated: that is FFmpeg probing for a C23 header MSVC does not have, and it falls back.
FFmpeg now configures on Windows (16 hwaccels, every asserted component present) and fails in its own compile instead: zconf.h(486): fatal error C1083: Cannot open include file: 'unistd.h' zconf.h.cmakein still carries an autoconf-era block that defines Z_HAVE_UNISTD_H from #ifdef HAVE_UNISTD_H. CMake probes for unistd.h, correctly does not find it under MSVC, and leaves Z_HAVE_UNISTD_H undefined earlier in the file, but that block then defines it anyway: FFmpeg's config.h contains "#define HAVE_UNISTD_H 0" and #ifdef is true for a value of zero. The header goes on to include a file MSVC does not have. zlib's own ./configure rewrites this line for exactly this reason; its CMake path does not, so patch it after install. The two remaining paths that could redefine Z_HAVE_UNISTD_H are gated on __WATCOMC__ and on _LARGEFILE64_SOURCE && !_WIN32, neither of which can fire here.
…line The deps build now completes on Windows, FFmpeg included, and the plugin link exposed two problems in emit_cmake. LNK1104: cannot open file 'ibpath:...lib.lib.lib'. The -l* arm is greedy enough to read the -libpath:C:/x that --extra-ldflags puts into FFmpeg's .pc files as a library named "ibpath:C:/x". Match the MSVC search-path spellings alongside -L, above that arm. zlib reached neither list. The Windows .pc chain never surfaces it as a -l flag, so it was dropped entirely and would have surfaced as unresolved inflate/deflate once the link got that far. It is ours only on Windows, so list it explicitly there, after its consumers. Static library candidates are now probed .lib first so a Windows link uses one naming convention rather than mixing librist.a with the rist.lib beside it. On Linux only the lib*.a form exists, so the order changes nothing.
The plugin link came down to a single unresolved external: avformat.lib(tls_mbedtls.o): unresolved external symbol gmtime_r referenced in function mbedtls_gen_x509_cert Upstream bug rather than anything about this build. tls_mbedtls.c calls gmtime_r without including libavutil/time_internal.h, which is where FFmpeg keeps the ff_gmtime_r fallback for platforms lacking the POSIX function. Every target except MSVC resolves the real gmtime_r, so the missing include never shows up there. Applied on all platforms rather than gated on Windows: the include is a no-op where HAVE_GMTIME_R is set, and one source state everywhere beats a platform-specific divergence. The patch verifies its own anchor and fails loudly if a future FFmpeg bump moves it.
A field crash on Windows resolved obs-irl-source.dll frames only to hex offsets because the plugin's PDB was never uploaded. Ship it alongside the DLL in the CI artifact (not the release zip). OBS's crash handler reads a PDB sitting next to the module, so the next crash log names the faulting function instead of a base+offset.
datagutt
changed the title
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/build.yml (2)
191-205: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEnforce the complete Windows isolation contract.
The dependency check misses
libav*.dll,avfilter, andavdevice. The export check only rejects selected bundled prefixes, so unrelated exports can pass. Theobs_module_loadcheck also uses a substring match.Parse the export names, fail when any name does not match
^obs_module_, and require an exactobs_module_loadexport. Match all host FFmpeg DLL variants.As per coding guidelines,
.github/workflows/*.ymlmust verify that the binary has nolibav*dependency and exports onlyobs_module_*.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build.yml around lines 191 - 205, Update the Windows validation checks in the PowerShell workflow block: expand the dependency regex to reject all host FFmpeg DLL variants, including libav*.dll, avfilter, and avdevice. Parse the export names and fail if any export does not match ^obs_module_, while requiring an exact obs_module_load export rather than a substring match; preserve the existing $fail and status reporting behavior.Source: Coding guidelines
9-18: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd least-privilege permissions to the build workflow.
Add a workflow-level
permissionsblock starting withcontents: read, then add only the specific permissions required by steps that need write access. This workflow executes checked-out code and build scripts without an explicit least-privilege scope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build.yml around lines 9 - 18, Add a workflow-level permissions block near the top of the build workflow with contents: read as the default, then grant only the additional write permissions explicitly required by the workflow’s individual steps. Keep all unspecified permissions denied and preserve the existing build configuration.Source: Linters/SAST tools
🧹 Nitpick comments (1)
.github/workflows/build.yml (1)
73-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the Meson and Ninja versions.
pip install meson ninjaresolves the latest releases on every run. Pin both versions and include them in the dependency cache key to keep builds reproducible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build.yml around lines 73 - 75, Update the “Install meson” workflow step to install explicitly pinned versions of both Meson and Ninja instead of unversioned packages, and include those pinned versions in the dependency cache key used by the workflow so cache entries are invalidated when either version changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build.yml:
- Around line 225-228: Move the explanatory PDB comments above the YAML literal
block introduced by path: | in the upload-artifact configuration, or remove
them; ensure only the actual obs-irl-source.pdb path remains inside the block so
comments are not treated as artifact paths.
---
Outside diff comments:
In @.github/workflows/build.yml:
- Around line 191-205: Update the Windows validation checks in the PowerShell
workflow block: expand the dependency regex to reject all host FFmpeg DLL
variants, including libav*.dll, avfilter, and avdevice. Parse the export names
and fail if any export does not match ^obs_module_, while requiring an exact
obs_module_load export rather than a substring match; preserve the existing
$fail and status reporting behavior.
- Around line 9-18: Add a workflow-level permissions block near the top of the
build workflow with contents: read as the default, then grant only the
additional write permissions explicitly required by the workflow’s individual
steps. Keep all unspecified permissions denied and preserve the existing build
configuration.
---
Nitpick comments:
In @.github/workflows/build.yml:
- Around line 73-75: Update the “Install meson” workflow step to install
explicitly pinned versions of both Meson and Ninja instead of unversioned
packages, and include those pinned versions in the dependency cache key used by
the workflow so cache entries are invalidated when either version changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bd9cdd78-fdc5-4b0e-81fb-c877568c7b64
📒 Files selected for processing (1)
.github/workflows/build.yml
Comment on lines
+225
to
+228
| # PDB ships in the CI artifact (not the release zip) so a crash | ||
| # log resolves obs-irl-source.dll frames to function names. | ||
| # OBS's crash handler reads it if it sits next to the DLL. | ||
| build/RelWithDebInfo/obs-irl-source.pdb |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow context =="
nl -ba .github/workflows/build.yml | sed -n '200,245p'
echo
echo "== YAML parse of path block lines =="
python3 - <<'PY'
from pathlib import Path
import yaml
p = Path('.github/workflows/build.yml')
data = yaml.safe_load(p.read_text())
jobs = data.get('jobs', {})
for job_name, job in jobs.items():
steps = job.get('steps', [])
for i, step in enumerate(steps):
if 'upload-artifact' in (step.get('uses') or '') or step.get('with', {}).get('path'):
print(f"{job_name}.{i}: uses={step.get('uses')}, with.path type={type(step.get('with',{}).get('path')).__name__}")
pth = step.get('with',{}).get('path')
if isinstance(pth, str):
for line_no,line in enumerate(pth.splitlines(),1):
print(f" path line {line_no!r}; starts_hash={line.lstrip().startswith('#')}")
elif isinstance(pth, list):
for idx,line in enumerate(pth,1):
print(f" path item {idx!r}; starts_hash={line.lstrip().startswith('#')}")
PY
echo
echo "== changed context if available =="
git diff -- .github/workflows/build.yml | sed -n '1,220p' || trueRepository: irlserver/obs-irl-source
Length of output: 225
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow context =="
awk '{printf "%7d %s\n", NR, $0}' .github/workflows/build.yml | sed -n '200,245p'
echo
echo "== YAML parser availability =="
python3 - <<'PY'
try:
import yaml
print("pyyaml available:", yaml.__version__)
except Exception as e:
print("pyyaml unavailable:", type(e).__name__, e)
PY
echo
echo "== YAWL/ruamel availability =="
python3 - <<'PY'
pkg = __import__('pkgutil')
for name in ('ruamel.yaml', 'ruamel'):
print(name, any(True for _, n, _ in pkg.iter_modules() if n.startswith(name)))
PY
echo
echo "== extract path scalars with Python =="
python3 - <<'PY'
from pathlib import Path
p = Path('.github/workflows/build.yml')
text = p.read_text()
lines = text.splitlines()
for i, line in enumerate(lines, 1):
if line.lstrip().startswith('upload-artifact') or 'upload-artifact' in line:
lo=max(1,i-8); hi=min(len(lines), i+20)
print(f"--- upload-artifact near line {i} ---")
for j in range(lo, hi+1):
print(f"{j:6d} {lines[j-1]}")
PY
echo
echo "== changed context if available =="
git diff -- .github/workflows/build.yml | sed -n '1,220p' || trueRepository: irlserver/obs-irl-source
Length of output: 5628
Move the PDB comments outside the path: | block.
Lines beginning with # inside the literal block scalar become additional artifacts paths for upload-artifact, so the upload receives the explanatory text as paths. Keep those comments above the block or remove them.
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 60-228: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build.yml around lines 225 - 228, Move the explanatory PDB
comments above the YAML literal block introduced by path: | in the
upload-artifact configuration, or remove them; ensure only the actual
obs-irl-source.pdb path remains inside the block so comments are not treated as
artifact paths.
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.
Links a pinned FFmpeg, libsrt, librist and mbedTLS into the plugin statically instead of using whatever the host OBS ships.
Why
Two problems, one fix.
Stale transport libraries. obs-deps pins libsrt 1.5.2 (September 2022) and librist 0.2.7. Every SRT and RIST fix since then was missing.
The FFmpeg major split the releases. OBS 32.1 bundles FFmpeg 7 (
avcodec-61), 32.2 bundles 8.1 (avcodec-62). A binary linked against one will not load where the other is present, so every release shipped a separate Windows and macOS artifact per OBS line.One artifact per platform
libobs was never the constraint.
obs_init_modulegates a plugin on(mod.ver() & 0xFFFF0000) <= LIBOBS_API_VER, so major and minor only, with no equality check, and it looks up nothing butobs_module_*. Building against the oldest supported line yields one binary that loads on that line and every newer one.OBS_VERSIONat the top ofbuild.ymlpins that oldest line.OBS_DEPS_VERSIONsurvives only because libobs itself needs obs-deps to build; the plugin no longer touches it.Symbol isolation
This is load-bearing, not cosmetic. OBS has already loaded its own FFmpeg, exporting the same symbol names as the archives linked here. An exported
avcodec_open2in the plugin would resolve through the global scope to OBS's copy, so the plugin's calls would run against a different library's structs.The module is built with hidden visibility,
--exclude-libs,ALLand a version script exporting only theobs_module_*entry points.scripts/verify-plugin.shasserts this after every build, with adumpbinequivalent in the Windows job, because a successful compile does not prove it.Verified against a harness using the identical link line, with a system FFmpeg loaded
RTLD_GLOBALfirst to simulate OBS:DT_NEEDEDreduces to libstdc++, libm, libz, libgcc_s, libc. Nolibav*, and nolibmbedcrypto, which confirms librist bound the static copy. Stripped size roughly 12 MB.Two silent failures found on the way
Both now guarded, and worth knowing about if anyone touches the component lists.
--enable-protocol=srtis a no-op: FFmpeg names the componentlibsrt. Configure ignores unknown names in an--enable-*list without a diagnostic, and produced a build withCONFIG_LIBSRT=yesnext to!CONFIG_LIBSRT_PROTOCOL=yes, meaning the library was linked in and SRT was completely dead.build-deps.shnow asserts every required component against the generatedffbuild/config.mak.librist's meson located mbedTLS with
cc.find_libraryand bound the host's sharedlibmbedcrypto.so, which would have reintroduced a runtime dependency and mismatched the copy libsrt uses.fix_mbedtls_pcrewrites bothlibrist.pcandsrt.pcto resolve out of the prefix.Component scope
The bundled FFmpeg is decode only (
--disable-everythingplus an explicit list) and carries exactly what the README advertises: H.264, HEVC, AV1, VP9, AAC, Opus, MP3, AC3/E-AC3 and PCM; MPEG-TS, FLV, MP4, Matroska, HLS and RTSP; SRT, RIST, RTMP(S), HTTP(S), TCP, UDP and RTP. Hardware decode is VAAPI and NVDEC on Linux, D3D11VA/DXVA2 and NVDEC on Windows, VideoToolbox on macOS.Trimming that list further is a user-visible feature removal, not a size optimisation.
deps/README.mdsays so.Licensing
FFmpeg is configured LGPLv3 (
--enable-version3, no--enable-gpl), compatible with the plugin's AGPL-3.0. Nothing here needs GPL components since the plugin decodes and never encodes. libsrt is MPL-2.0, librist BSD-2-Clause, mbedTLS Apache-2.0, all compatible with (A)GPLv3.Verification status
CI is green on all three platforms (run 30029171320): Linux x64, Windows x64, macOS ARM64. Each job builds the bundled stack, builds the plugin, and runs the isolation checks. Windows reports 16 hwaccels compiled in, no FFmpeg DLL imports, no leaked bundled symbols, and
obs_module_loadexported.Artifact sizes: Linux 16 MB (unstripped, with debug info), Windows 4 MB, macOS 3 MB.
The isolation guarantee was also validated locally against a harness using the identical link line, with a system FFmpeg loaded
RTLD_GLOBALfirst to simulate OBS:Not yet done: loading the artifacts in a real OBS install on each platform (the RELEASING.md smoke test). CI proves the binary builds, isolates, and exports correctly; it does not prove a stream plays. That is the manual pre-publish step.
Commit history
The first commit is the feature; the remaining ten are toolchain fixes that CI surfaced, almost all Windows (MSVC/MSYS2 CRT, static library naming, pkg-config quirks, one upstream FFmpeg
gmtime_rinclude bug). Happy to squash to a single commit on merge if you would rather not carry the fix-forward trail.Summary by CodeRabbit
New Features
Bug Fixes
Documentation