Repository navigation
fix(mixers): stop a late first input rewinding a live aggregator - #895
Conversation
srperens
left a comment
srperens
left a comment
There was a problem hiding this comment.
Verdict: Approve — right layer (the aggregator's own one-shot start-time selection), applied at every block-built force-live aggregator. Both new tests call the real block constructors, and both ran and passed in Check (Linux).
Non-blocking notes
disarm_start_time_selectionreturns silently when the downcast fails. That is correct for every element passed today. Adebug!on that branch would make a future non-aggregator caller visible instead of turning it into a no-op.builtin.compositor(both backends) and the vision mixer's GL backend get the same one-line call, but no test covers them. The GL path cannot run headless in CI. The softwarebuiltin.compositorpath could reuseoutput_pts_around_late_first_inputin a follow-up.
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| Applied wherever a block builds a live zero-start aggregator | CONFIRMED |
backend/src/blocks/builtin/mixer/elements.rs:47 — crate::gst::aggregator_start::disarm_start_time_selection(&mixer); (all builtin.mixer buses plus liveaudiorouter); backend/src/blocks/builtin/vision_mixer/elements.rs:127 — crate::gst::aggregator_start::disarm_start_time_selection(mixer); (PGM and multiview); two calls in builtin/compositor.rs. The only other audiomixer, in audiorouter.rs, is not force-live, so it never outputs on a timeout |
| Tests exercise the changed module and fail on revert | CONFIRMED |
backend/src/blocks/builtin/mixer/tests.rs:607 — let mixer = make_audiomixer("test_mixer_late_first_input", true, 30, 30).unwrap();, and make_dist_compositor in vision_mixer/tests.rs. Both assert that PTS are monotonic, so a revert that removes the call lets the rewind through. The failure on revert rests on the upstream behaviour below |
| The disarm survives Strom's start sequence | CONFIRMED |
It runs at build, in NULL. Flow start only moves upwards: backend/src/gst/pipeline/lifecycle.rs:112 — .set_state(gst::State::Ready), then PLAYING. The PAUSED→READY re-arm the body names never occurs on a running flow |
update_segment clears first_buffer, and an unset position reads as segment start |
EXTERNAL |
GstAggregator/Audio-/VideoAggregator behaviour, GStreamer ≥1.18; assumed as the doc comment states. The CI pass on 1.24.2 is consistent with it |
Diagnosis — The mechanism explains the whole symptom set: only the first join rewinds, because selection is one-shot. Only audio kills the viewer, because opusenc rejects a backwards buffer. Monitor tracks survive because their buses had already selected at startup. Coverage is BOUNDED to block-built aggregators, as the body states. A raw audiomixer or compositor placed in a flow is not covered. SPECULATIVE (not verified): a flushing seek into a live mixer may re-arm selection; I found no such seek on these elements.
Radius — SHARED: every builtin.mixer bus, liveaudiorouter, vision mixer PGM/MV, builtin.compositor. For non-force-live buses behaviour is unchanged: the first buffer used to set the position to MIN(0, unset) = 0, and an unset position now reads as 0 too. CPU and GL go through the same aggregator base, so they behave alike.
Overlaps — none on the design. #898 and #919 add functions right after make_audiomixer in the same file. That is a textual neighbour only and should rebase trivially in any order.
Tests & CI — All Linux, WASM and API checks green at 7308706. The job log shows test_make_audiomixer_late_first_input_does_not_rewind ... ok and dist_compositor_late_first_input_does_not_rewind ... ok. pipeline_lifecycle_test ran. Its gst_mini_object_unref CRITICAL lines are the known pre-existing noise also seen on main-based PRs. macOS/Windows skipped. Nothing here is platform-specific.
Confidence: HIGH
GstAggregator with start-time-selection=zero selects its start time when the first buffer reaches any sink pad, by setting the output position to MIN(0, position). A force-live mixer whose inputs start empty outputs on timeouts without ever reaching that point, so the first WHIP guest's first buffer, seconds into the flow, moves the output back to 0. The next buffer is stamped PTS 0; opusenc rejects it and webrtcsink ends every WHEP viewer session connected at that moment. Clear the one-shot selection at build time with gst_aggregator_update_segment and the aggregator's initial segment. Startup output still begins at 0. Applied to make_audiomixer (mixer and liveaudiorouter), the vision mixer compositors and builtin.compositor. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7308706 to
26eb997
Compare
srperens
left a comment
srperens
left a comment
There was a problem hiding this comment.
Supersedes my Approve at 7308706, which GitHub dismissed when the branch was pushed again. Re-reviewed at 26eb997, a rebase of the same single commit onto 34008fe. A line diff of the old and new patches differs only in where one blank line sits, so the PR's own change is unchanged and the verdict stands.
Verdict: Approve — the change is the same one I approved before, and both of its guards ran green again on the new base.
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| Applied wherever a block builds a live zero-start aggregator | CONFIRMED |
At this head, backend/src/blocks/builtin/mixer/elements.rs:47 — crate::gst::aggregator_start::disarm_start_time_selection(&mixer);, plus backend/src/blocks/builtin/vision_mixer/elements.rs:127 — crate::gst::aggregator_start::disarm_start_time_selection(mixer); and two calls in compositor.rs (:258, :480). No new zero-start force-live aggregator has landed on main since my review. The two liveadders in backend/src/blocks/builtin/whep.rs predate this PR. They set start-time-selection=first, which puts them outside the claimed scope. SPECULATIVE (not verified): whether first can rewind on a late first input. |
| Tests exercise the changed module and fail on revert | CONFIRMED |
backend/src/blocks/builtin/mixer/tests.rs:838 — let mixer = make_audiomixer("test_mixer_late_first_input", true, 30, 30).unwrap();. The test now sits after #909/#910's rewrite of this file and still calls the block constructor. The vision_mixer counterpart is unchanged. |
| Disarm survives the start sequence | CONFIRMED |
CONFIRMED in my review of 2026-09-30. lifecycle.rs was not changed by the rebase. |
update_segment clears first_buffer; an unset position reads as segment start |
EXTERNAL |
GstAggregator behaviour, GStreamer ≥1.18, as I assumed before. |
Radius — SHARED, with the argument unchanged from my last review: for non-force-live buses behaviour does not change.
Overlaps — none on the design. #919, #898 and #894 edit the same element files at neighbouring functions. Those are textual conflicts only. I asked for #920 to land before #919. This PR can go in any order.
Tests & CI — Run 36728644787 at 26eb997, Check (Linux) job 109932080544: test_make_audiomixer_late_first_input_does_not_rewind ... ok and dist_compositor_late_first_input_does_not_rewind ... ok. test_pipeline_cleanup_after_stop_and_drop ... ok. There is no FAILED line in the job. macOS and Windows skipped, but nothing here is platform-specific.
Confidence: HIGH
…988) The late-first-input regression tests for the vision mixer and the audio mixer pushed one buffer and then slept a fixed 300 ms before checking that the output had reached it. On a loaded macOS runner the streaming threads can be starved for longer than that (the same CI run also saw a 171 ms scheduling delay in a mediaplayer timing test), so the output stopped short of the pushed time and the test failed with "the late input never reached the output". Replace the fixed sleeps with waits on the output itself, with a 10 s deadline: before the push, until the aggregator has output on its own; after the push, until the output reaches the pushed time. A healthy run returns as soon as the condition holds. Also push between two output buffers instead of at an arbitrary moment. A push that lands while the aggregator is inside `aggregate` falls between the subclass reading the output position and writing it back, and that write hides the rewind. With #895's fix disabled, the old helper missed the rewind in 4 of 40 vision mixer runs; the aligned push caught it in 60 of 60. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
2 participants
Description
A WHEP viewer connected before the first WHIP guest joins loses its session when that guest's media arrives. The viewer's
opusencfails withbuffer going too far back in time, webrtcsink ends the session withCould not encode stream., and the player page reconnects about 30 s later. Only the first join after the flow starts does this. Viewers who connect later, and later joins, are unaffected.The cause is in
GstAggregator. Withstart-time-selection=zero, it picks its start time when the first buffer reaches any sink pad, by setting the output position toMIN(0, position). Aforce-livemixer whose inputs are empty never reaches that point at startup: it outputs silence (or background) on timeouts. So the guest's first buffer, seconds into the flow, moves the output back to 0. The next output buffer is stamped PTS 0, with a duration reaching forward to the correct end, and the stream then carries on. The program audio bus ofbuiltin.mixerand the vision mixer's PGM compositor both do this on the first join. Only audio kills the viewer:opusencrejects the buffer, while the one video frame stamped 0 passes through the encoder.The fix clears that one-shot selection when the element is built, with
gst_aggregator_update_segmentand the aggregator's own initial segment (start 0, position unset). Startup output still begins at 0; a late first input no longer rewinds it. It is applied wherever Strom builds a live aggregator with zero start time:make_audiomixer(sobuiltin.mixerandbuiltin.liveaudiorouter), the vision mixer's PGM and multiview compositors, andbuiltin.compositor.Evidence
Reproduced with a Chrome publisher and a Chrome program viewer against a fresh Strom per trial, using the Open Live flow shape (WHIP Input → Time Offset → Vision Mixer (GPU) → Video Encoder → WHEP Output; audio through
builtin.mixermain/monitor into the WHEP Outputs). The order decides it: viewer first, then publisher.b28be77bLeft out: trials where the viewer's session had already failed at startup for an unrelated reason (below), so there was no session to kill. Four trials on this branch later lost program video to the GPU mixer's
Need timestamped buffers!flow death, which draft #893 addresses; the join itself passed cleanly in each, and in five interleaved pairs that death hit one trial per arm.Where the rewind happens, from
GST_DEBUGtraces of failing trials:aggregator:5:<…:audiomixer> Selecting start time 0:00:00.0000000008.7 s into the flow, at the guest's first audio buffer. The multiview and solo buses had already selected theirs at 0.4 s, which is why the monitor track never breaks. The PGM compositor logs the same at the guest's first video frame.basesink:6: the WHEP producer appsinks for the main audio track receivestart 0, end 8.63after a continuous 10 ms stream; the monitor tracks do not. One program video producer receives one frame stamped 0 at the video join.appsrc:6,audioresample:6in the viewer's session:pop buffer … pts 0:00:00.000000000, dur 0:00:08.610000000, then the resampler'sdiscontinuity of 813501413271395 samples(a negative difference, printed unsigned) and theopusencerror.Not fixed here
About 1 in 5 trials on both builds, a viewer that connects in the first ~4 s of the flow fails at session start:
codec-parser-caps-video_0cannot transformvideo/x-h264, … colorimetry=(string)sRGB, andvideo_0stops withnot-negotiated. It has nothing to do with the WHIP join. The fix does not change it.Aggregators placed directly in a flow as raw elements, rather than built by a block, are not covered.
A PAUSED to READY change re-arms the aggregator's start-time selection. Flows build a new pipeline on every start, so Strom does not take that path.
How to review
backend/src/gst/aggregator_start.rsholds the explanation and the helper. The call sites are one line each. Each regression test builds the block's real element (make_audiomixer,make_dist_compositor), runs it live with an empty input for 500 ms, pushes one buffer stamped at the current running time, and asserts the output PTS never go backwards. With the helper'supdate_segmentcall removed, both fail with the production signature:output went back in time from 0:00:00.430000000 to 0:00:00.000000000.Adds
gstreamer-baseas a direct dependency forgst_base::Aggregator. It was already in the tree throughgstreamer-app.Type of Change
Checklist
cargo fmt --allcargo clippy --workspace --all-targets -- -D warningscargo test --workspaceTests run locally on macOS (GStreamer 1.28.6):
cargo test --workspace --no-fail-fast(1062 passed, 0 failed, 2 ignored),cargo clippy --workspace --all-targets -- -D warnings,cargo fmt --all --check. The two new tests were also run with the fix reverted and fail. The Chrome trials above ran on release builds ofb28be77band this branch. CI's Linux job (GStreamer 1.24.2) ran and passed both new tests; the fix-reverted failure was checked on macOS only.Written by Claude (AI) on behalf of @wagenet.
🤖 Generated with Claude Code