fix(rtp): disable header extension aggregation on every depayloader

[wagenet]

Problem

A single corrupt RTP stream can abort the whole process, killing every unrelated flow on the server:

gstrtpbasedepayload.c:942:gst_rtp_base_depayload_handle_buffer:
'gst_buffer_list_length (priv->hdrext_buffers) == 0' should be TRUE

Since 1.24 GstRTPBaseDepayload caches each packet's RTP header while assembling an output buffer, clearing it only when the subclass pushes or flushes. rtph264depay breaks that invariant: an interrupted fragmentation unit calls delayed() then finish_fragmentation_unit(), which in access-unit mode can absorb the truncated NAL without pushing anything. The cache stays populated and the assert fires. g_assert_true is not defusable.

Upstream: gstreamer#5057, unfixed.

Fix

Disable header-extension aggregation on every depayloader. That restores pre-1.24 behaviour — extensions are read from the current packet instead of accumulated — so the cache never fills.

Nothing is lost: Strom reads no header-extension metadata, and the extensions that matter for transport (transport-cc, abs-send-time, mid) are consumed by webrtcbin well upstream of any depayloader.

install() sweeps depayloaders already present in a pipeline and connects deep-element-added for ones autoplugged later. The closure captures nothing, so it creates no reference cycle.

Because the abort takes down the process rather than one flow, it does not matter which pipeline hosts the offending depayloader. Strom runs three kinds that can host one, and all three now call install() while still in NULL:

Pipeline	How a depayloader gets there
Main (PipelineManager::start())	decodebin autoplugs one, or a user places one directly in a flow
Media Player internal (decode + passthrough)	uridecodebin / urisourcebin autoplug one from an rtsp:// URI
WHIP Input session	whipserversrc autoplugs them inside its own bin

Why resolve the symbol at runtime

There is no GObject property — the C setter is the only switch, and it is Since: 1.24. Binding it normally would raise the workspace build floor from 1.22, breaking Debian 12 and the ARM64 cross-compile target (Raspberry Pi OS 12), which both ship 1.22. Resolving at runtime keeps one binary working on both: below 1.24 the lookup fails and we do nothing, which is correct, because the bug doesn't exist there.

dlsym(RTLD_DEFAULT, …) on Unix; on Windows GetModuleHandleEx(FROM_ADDRESS) anchored on an already-linked RTP symbol, to avoid hardcoding a DLL name that varies between builds.

Tests

Six unit tests plus one integration test, all verified to fail when the fix is reverted:

Revert	What fails
install() neutered	the two install_disables_aggregation_* unit tests
install() call removed from start()	rtp_hdrext_aggregation_test
install() calls removed from the Media Player bridge	decode_pipeline_disables_hdrext_aggregation, passthrough_pipeline_disables_hdrext_aggregation

The rtp_hdrext unit tests can't see their call sites, hence the separate tests per site. The two Media Player tests call the real create_decode_pipeline / create_passthrough_pipeline, then add a depayloader to a nested bin — the same path deep-element-added sees for an autoplugged one — because CI cannot serve an RTSP stream.

rtph264depay_enables_aggregation_by_default confirms the unpatched default really is on, so the getter reads live state rather than always returning false. symbol_resolves_on_modern_gstreamer fails loudly if the lookup ever breaks on >= 1.24 — otherwise the workaround would silently no-op.

rtph264depay is in gstreamer1.0-plugins-good, already in CI, so these run rather than skip.

Gap: the WHIP Input call site has no test. build_whipserversrc binds a TCP port and needs a full endpoint config, so there is no cheap way to construct one in a unit test. It is covered by inspection only.

What I ran

On macOS, GStreamer >= 1.24: full cargo test (543 lib + all integration, 0 failures), cargo clippy --all-targets -D warnings (zero diagnostics), cargo fmt --check, pipeline_lifecycle_test 3/3 including the circular-reference detector, and every revert scenario in the table above.

The macOS and Windows builds are off for push/PR, so this PR's own check list skips them and the platform-specific FFI would otherwise merge never having compiled. Dispatched manually on the fork, where the branch lives, since workflow_dispatch on this repo needs admin: run 33179848797 on 55e91ef — all green, including Build (macOS) and Build (Windows). Both ran Clippy and the test suite on their own OS, so the Windows GetModuleHandleEx/GetProcAddress path has now compiled and linked for the first time, and the tests above executed on macOS and Windows as well as Linux.

Still not exercised anywhere: the < 1.24 no-op path — no CI image ships 1.22, and every test early-returns when is_supported() is false. It is the trivial branch (lookup fails, install() returns), but it is untested.

[srperens]

Verdict: Comment — sound mechanism and test discipline for the pipeline it touches, but the fix is narrower than its own framing ("disable ... on every depayloader"), and its platform-specific code is unverified by CI.

Requested changes

1. Also call rtp_hdrext::install() on the Media Player block's internal pipelines — backend/src/blocks/builtin/mediaplayer/bridge.rs:28 and :138 each build a separate gst::Pipeline running uridecodebin/urisourcebin, which will autoplug an RTP depayloader for an rtsp:// source URI exactly like decodebin does in the main pipeline. That pipeline is never passed to rtp_hdrext::install(), so an interrupted RTSP H264 stream through a Media Player block still takes the whole process down with the bug this PR exists to fix.

Claims

Claim	Verdict	Evidence
install() runs before any pipeline state change	CONFIRMED	backend/src/gst/pipeline/lifecycle.rs:17 rtp_hdrext::install(&self.pipeline); runs before set_state(Ready) at lifecycle.rs:65-67 and set_state(Playing) later
deep-element-added closure captures nothing, no ref cycle	CONFIRMED	backend/src/gst/rtp_hdrext.rs:177 bin.connect("deep-element-added", false, move |args| { reads only args, no external variable moved in
Covers "every depayloader" the process runs	BOUNDED/CONTRADICTED as stated	Only reaches self.pipeline's bin tree (rtp_hdrext.rs:168 iterate_recurse() + the signal above). Verified two other independent gst::Pipeline instances exist and are never passed to install(): WHIP Input's session_pipeline (backend/src/blocks/builtin/whip.rs:570) and Media Player's internal pipelines (bridge.rs:28, :138). The WHIP one is fine — issue #685 itself traced the crash to the main-pipeline decodebin, not an element inside whipserversrc — but Media Player's uridecodebin/urisourcebin is a real, unguarded second path
libgstrtp-1.0 is already loaded so dlsym(RTLD_DEFAULT, ...) finds the symbol	EXTERNAL	Dynamic-linker behavior, not settleable from this repo; the new gstreamer-rtp workspace dependency (Cargo.toml:33, backend/Cargo.toml:58) does cause the lib to link at load time on ELF/Mach-O, which supports the assumption
Windows GetModuleHandleExA/GetProcAddress path and the Unix dlsym path both work as written	UNVERIFIED	Build (macOS) and Build (Windows) both show skipping on this PR's checks (head c7db33a8); per CLAUDE.md/CI, neither builds on push/PR, so this is the only new platform-specific code in the diff and it has never compiled

Diagnosis — Root cause and mechanism match issue #685's own confirmed investigation (interrupted FU-A with contiguous sequence numbers on a depayloader that has the X-bit-triggered aggregation cache populated); the "why runtime dlsym, not a feature bump" tradeoff is well-argued and consistent with the workspace's v1_22 floor (Cargo.toml:29). The gap isn't the mechanism, it's coverage: BOUNDED to pipelines the code actually calls install() on, and today that's one of at least three that host GStreamer elements capable of autoplugging an RTP depayloader.

Tests & CI — Linux x86_64/ARM64, Check, WASM, API Contract all pass on c7db33a8. The new unit tests and rtp_hdrext_aggregation_test.rs genuinely exercise PipelineManager::start() (not a reimplementation) and read as revert-sensitive per the PR's own table. macOS/Windows: skipped, so the platform-specific FFI is unverified — dispatch with gh workflow run ci.yml --ref wagenet/685-hdrext-aggregation -f platforms=macos (and =windows) before merge.

Confidence: HIGH

[wagenet]

Pushed 55e91ef and rewrote the PR description. Both points from the review are addressed.

Coverage. The review was right that "every depayloader" overstated the diff — install() only ever ran on the main pipeline. It now also runs on the Media Player block's two internal pipelines (bridge.rs, decode and passthrough), where an rtsp:// URI makes uridecodebin/urisourcebin autoplug a depayloader.

I went one further and covered WHIP Input's session_pipeline too, which the review judged out of scope. The reasoning there was that #685 traced the crash to the main-pipeline decodebin, not to anything inside whipserversrc — true, but whipserversrc does autoplug depayloaders in its own bin, and g_assert_true aborts the process rather than the flow. Which pipeline hosts the offending depayloader doesn't change the outcome, so leaving it out would keep the title overstating the fix. It is one line, and install() is a no-op below 1.24.

Platform verification. Build (macOS) and Build (Windows) skip on push/PR by design (if: github.event_name == 'workflow_dispatch'), so the new FFI would have merged without ever compiling. workflow_dispatch on this repo needs admin, so I dispatched it on the fork where the branch lives: run 33179848797 on 55e91ef, all green. Both jobs ran Clippy and the test suite on their own OS, so the Windows GetModuleHandleEx/GetProcAddress path has now compiled and linked, and the rtp_hdrext tests executed on macOS and Windows as well as Linux. That run lives on the fork and won't appear in this PR's check list.

Tests. Two new unit tests call the real Media Player constructors, then add a depayloader to a nested bin — the same path deep-element-added sees for an autoplugged one, since CI can't serve RTSP. Both fail with the install() calls removed; I verified that by commenting them out rather than assuming it. Locally: 543 lib tests, all integration tests, clippy --all-targets -D warnings, fmt --check, all clean.

Two gaps left, both now stated in the description: the WHIP call site has no test (build_whipserversrc binds a port and needs a full endpoint config), and the < 1.24 no-op path is untested anywhere, since no CI image ships 1.22 and every test early-returns when is_supported() is false.

_{Written by Claude Code on behalf of @wagenet.}

[srperens]

Supersedes my 2026-08-28 v3 review (id 5049165959, COMMENTED) at head c7db33a8. That review requested one change: also call rtp_hdrext::install() on the Media Player's internal pipelines. This head does that, plus adds it to WHIP Input for completeness. Verdict changes to Comment→Comment (same), gap closed.

Verdict: Comment — the requested-change gap is closed with genuine coverage; only the pre-existing macOS/Windows CI gap remains, which blocks approval per protocol regardless of the fork run cited in the PR body.

Claims

Claim	Verdict	Evidence
Media Player's two internal pipelines now call install() before any state change	CONFIRMED	backend/src/blocks/builtin/mediaplayer/bridge.rs:129 and :326, both immediately before Ok(pipeline); the pipeline is only later moved to Playing from connect_main_pipeline_handler, invoked asynchronously from builder.rs:236-247 after construction returns — install always precedes any state transition
WHIP Input's session_pipeline now calls install() before Playing	CONFIRMED	backend/src/blocks/builtin/whip.rs:1040 rtp_hdrext::install(&session_pipeline); sits between the .add(&whipserversrc) call and .set_state(gst::State::Playing) at line ~1046
New Media Player tests exercise the real constructors, not a reimplementation	CONFIRMED	bridge.rs:689 and :700 call create_decode_pipeline(...) / create_passthrough_pipeline(...) directly, then add a depayloader to a nested gst::Bin — the same path deep-element-added observes for an autoplugged element — and assert rtp_hdrext::is_enabled(&depay) == Some(false)
gstreamer_rtp::RTPBaseDepayload and rtp_hdrext::is_enabled/is_supported used by the new tests exist and match call signatures	CONFIRMED	backend/Cargo.toml:58 gstreamer-rtp.workspace = true; backend/src/gst/rtp_hdrext.rs:142,150,159 define is_enabled, is_supported, install with matching signatures
macOS/Windows FFI path now verified by this repo's CI	CONTRADICTED as stated	gh pr checks 721 at head 55e91efa still shows Build (Windows) and Build (macOS) as skipping, unchanged from the prior review. The green run cited in the PR body (github.com/wagenet/strom/actions/runs/33179848797) is on the author's fork, not Eyevinn/strom — it is not this repo's CI

Diagnosis — unchanged from my prior review; mechanism and root cause still check out.

Radius — GLOBAL, as before (pipeline lifecycle, three independent gst::Pipeline hosts), now fully covered rather than bounded to one.

Tests & CI — Build (Linux x86_64/ARM64), Check (Linux), Check & Build (WASM), API Contract Check all pass on 55e91efa. Build (macOS)/Build (Windows) still skip on push/PR per repo config — this remains the only new platform-specific code (dlsym/GetModuleHandleEx) never compiled by this repo's own CI. Dispatch before merge: gh workflow run ci.yml --ref wagenet/685-hdrext-aggregation -f platforms=macos (and =windows).

Confidence: HIGH

[wagenet]

(from Claude, on behalf of @wagenet)

This PR is unapproved on the Build (macOS) coverage gap alone — no code change is requested. That blocker is shared with #721, #722, #726 and #735, and the documented remedy (gh workflow run ci.yml --ref <branch> -f platforms=macos) appears not to work for fork branches. Written up once in #735 (comment) rather than repeated on each PR.