Conversation
b3326be to
cd9b383
Compare
|
Noted as a draft, so I am holding off on a full review until it is marked ready for |
0896d8c to
f0aa999
Compare
fafe2bd to
ddff280
Compare
a47cb11 to
641cd70
Compare
|
Written by Claude (AI). Update this PR once #809 lands. #809 adds a per-DSK
|
srperens
left a comment
There was a problem hiding this comment.
Verdict: Request changes — the mechanism is sound and unusually well argued, but the wall-clock fallback path releases the mixer hold in the wrong order, and the one test covering the GL compositing path cannot run in this repo's CI.
Requested changes
-
Drop the release sender before removing the probe in the fallback path —
backend/src/state/stinger.rs, the_ =>arm at line 501: insertdrop(release_tx);ahead ofpad.remove_probe(probe);. As written the sender stays live until the function returns, so if the probe fires as the timeout expires it blocks inrecv_timeoutwhileremove_probewaits on it — holding the program output, and a tokio worker thread, for the full 250 ms. The comment directly above already states the intended order. -
Either install a GL stack in
.github/workflows/ci.yml(gstreamer1.0-glplus a software rasterizer) soalpha_survives_gl_videomixerexecutes, or drop it and say in the PR body that the GL alpha path is verified by hand only. CLAUDE.md: a test that skips on a missing element "passes green and guards nothing".
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| Every buffer before the target costs one timestamp compare | CONFIRMED |
backend/src/state/stinger.rs:463 — if pts.nseconds() < hold_from_pts short-circuits the || before fired.swap, and the lock/channel at :472 are past that return |
| Dropping the sender keeps the probe from holding the mixer on the fallback path | CONTRADICTED |
backend/src/state/stinger.rs:504 — pad.remove_probe(probe); runs while release_tx is still in scope; it is only dropped at end of function |
| The stinger tests run in CI | CONFIRMED for stinger_test |
backend/tests/stinger_test.rs:389 — panic!("{name} missing — CI installs gst-libav and plugins-good"); — it fails rather than skips |
| The alpha tests run in CI | CONTRADICTED |
backend/tests/stinger_alpha_test.rs:215 — eprintln!("skipping: no usable GL environment"); and .github/workflows/ci.yml:104-107 installs no gstreamer1.0-gl or mesa |
remove_probe blocks on a running callback |
EXTERNAL |
Documented GStreamer behaviour; assumption stated, not settleable here |
Diagnosis — Anchoring the cut to the clip's stream offset rather than a wall-clock delay is the right layer: it removes the race with the aggregator instead of timing around it. Coverage is BOUNDED — the hold only applies where the mixer reaches the cut frame within the grace window, and the PR says so and degrades to wall clock. The BUFFER probe respects the CLAUDE.md constraints on the hot path; the blocking, allocation and formatting all sit past the one-shot guard, which is the correct shape for this.
Radius — SHARED. set_dsk_hold_last_frame, mixer_src_pad and mixer_frame_duration_ns are purely additive with no existing call sites, and the last-frame-hold change is gated on opt-in declaration. What is shared is the probe: while armed it sits on the mixer's program output pad, so every take on that mixer pays the compare until it removes itself. A failure would show up as a stalled program output for up to 250 ms, which is change 1.
Tests & CI — Check (Linux), Build (Linux x86_64/ARM64), Check & Build (WASM), API Contract Check all green at 641cd70. Build (macOS) and Build (Windows) report skipping; there is no platform-specific code in this diff, so Linux-green is adequate cover here. The PR body states that a_repeat_stinger_plays_forward_from_the_start does not fail when its fix is reverted — the disclosure CLAUDE.md asks for, taken at face value.
Design record — backend/src/state/mod.rs is 3082 lines after the split that moved stinger state out; worth carving down further, but not this PR's job.
Confidence: HIGH
641cd70 to
ed84f67
Compare
|
Written by Claude (AI). Both addressed in ed84f67, rebased on main. 1. Release order. Fixed: the fallback arm drops 2. GL test in CI. I took the route the repo already uses rather than installing a GL stack. The macOS job can't run on this PR without the The PR body previously claimed this test executed on Linux CI. It did not; the body now says what each guard covers. Also caught in the rebase: main added a #809 is still open, so the premultiplied refusal stays for now. |
srperens
left a comment
There was a problem hiding this comment.
Verdict: Comment — supersedes my Request changes of 2026-09-16 (review-5223777646, head 641cd708), which no longer stands: both requested changes are in. What holds it short of Approve is one row, and it is the same GL-coverage gap on a different file.
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| The probe-teardown race I asked about is fixed | CONFIRMED |
backend/src/state/stinger.rs:506 — drop(release_tx); now precedes backend/src/state/stinger.rs:507 — pad.remove_probe(probe);, so a probe that fired as the wait expired is released before remove_probe waits on it |
| The alpha test no longer skips silently | CONFIRMED |
backend/tests/stinger_alpha_test.rs:244 — strom_types::env::var_opt("STROM_REQUIRE_GL").is_none(), — adopts the precedent already on main in vision_mixer_fx_test.rs and shader_validation_test.rs |
| The four alpha tests executed in this repo's CI | UNVERIFIED |
.github/workflows/ci.yml:645 — STROM_REQUIRE_GL: 1 is set on the macOS job alone, and Build (macOS) is skipping on this PR. In Check (Linux) job 105241909348 the binary reports 4 passed ... finished in 0.09s — four GL pipelines cannot negotiate and reach EOS in 90 ms, so all four skipped |
| The core stinger logic is genuinely covered | CONFIRMED |
same job: stinger_test.rs reports 12 passed; 0 failed ... finished in 18.87s |
Diagnosis — The cut is anchored to the clip's own stream offset with a one-shot BUFFER probe, and the wall-clock arm is the fallback rather than the design. Coverage of the anchoring mechanism is ABSOLUTE; coverage of the alpha half is BOUNDED to whatever a macOS run would exercise, which is currently nothing.
Repo rules — The BUFFER probe at backend/src/state/stinger.rs:459 respects the hot-path rules: the per-buffer cost is a pts compare plus an AtomicBool swap, and every lock, channel and format sits past that guard, running once per take. The closure captures no Pipeline, Element or Bin — only channels, an atomic and a pre-built String — so it cannot form the CLAUDE.md reference cycle. No new endpoint: this extends the existing transition route with one additive Option<String> field, and API Contract Check passed. The three new StromEvent variants are additions, not modifications.
Radius — SHARED: the probe is installed on the vision mixer's src pad, so a take holds the program output for up to 250 ms (STINGER_MAX_HOLD, backend/src/state/stinger.rs:46). That bound is enforced by recv_timeout, and the fallback arm now releases before removing, so the failure mode is a late cut rather than a held mixer.
What I would ask for before merge — the ci:macos label on this PR, which is the only route that sets STROM_REQUIRE_GL and therefore the only way the four alpha tests have ever run against this diff. I am deliberately not applying it: it fires a paid run, which is a maintainer's call. Merging on Linux-green and letting the post-merge macOS run cover the alpha half is a legitimate alternative — but it is a choice worth making deliberately, not by default.
Confidence: HIGH
Superseded by my review at head ed84f67: both requested changes are in, so this CHANGES_REQUESTED sits at a dead head and misreports the PR.
|
Written by Claude (AI). A correction to the table: only The GL test now has a Linux run. #809 installs a GL stack in the Linux job ( If #809 merges first, this PR rebases onto it, drops the premultiplied refusal, and picks up the Linux GL job, so the test runs in this repo's CI without the |
srperens
left a comment
There was a problem hiding this comment.
Verdict: Comment — supersedes my Comment of 2026-09-18 (head ed84f671), and the verdict stands, for the same reason: the alpha half has still never been compiled or run by this repo's CI. gh api compare/ed84f671...09271e95 reports ahead=2, behind=0 over two files, so the rows below are the two new commits plus the one row that blocks approval, re-read at this head.
Requested change
backend/tests/stinger_test.rs:890-893describes an "unreadable" take as test noise. It is a visible artefact — the program cuts on a frame with no clip on it — so say in the PR body what a viewer sees when it happens and why up to half of takes doing so is acceptable. Nothing else in the diff guards that path:stinger_preroll_bench.rsreports1 ignoredin the same run.
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| The four alpha tests executed in this repo's CI | UNVERIFIED |
Re-read at 09271e95: Check (Linux) job 107403503091 reports stinger_alpha_test 4 passed ... finished in 0.07s, so all four skipped. .github/workflows/ci.yml:645 — ` STROM_REQUIRE_GL: 1` is set on the macOS job alone, and Build (macOS) is skipping. Blocks approval on its own |
| The new wait does not extend the hold on the mixer's output | CONFIRMED |
backend/src/state/stinger.rs:324 — ` drop(guard);` runs before backend/src/state/stinger.rs:371 — ` .wait_for_mixer_to_pass(&flow, &mixer, clip_end_ns, &still_ours)`, so STINGER_MAX_HOLD is unaffected. What the wait extends is the in-flight claim, and a take fired inside it is refused: backend/src/state/stinger.rs:158 — ` return Err(StingerError::AlreadyRunning(block_instance_id.to_string()).into());` |
| The premise of the wait — a stinger's keyed pad drops its last frame rather than repeating it | CONFIRMED |
backend/src/gst/pipeline/effects/mixer_ops.rs:294 — ` pad.set_property("max-last-buffer-repeat", if hold { u64::MAX } else { 0u64 });`, with hold false for a stinger pad, so the last clip frame stays current exactly until the output passes its end |
| The relaxed cut-frame test still fails a take that never cuts | CONFIRMED |
backend/tests/stinger_test.rs:918 — `` ` landed.iter().all( |
Diagnosis — 09271e95 closes the gap the re-arm left: re-arming resets the clip's offset while the keyed pad's last frame is still current, so the next take composited it. Waiting on the mixer's own position rather than a wall-clock sleep is the right anchor and matches the cut-point mechanism already in the PR. Coverage is BOUNDED by mixer_frame_duration_ns: caps without a positive framerate return None and the wait is skipped silently — worth a debug!, not a blocker.
Radius — SHARED, unchanged. The two new commits add no call site outside run_take; what moves is timing, after the cut has been applied. stinger_test.rs reports 12 passed ... finished in 20.29s in the same job, so the core is genuinely exercised.
Overlaps — none of the three kinds. No other open PR touches state/stinger.rs, gst/stinger.rs or mixer_ops.rs, and no open issue owns the stinger design.
Before merge — unchanged, and the only thing between this and Approve: the ci:macos label, the sole route that sets STROM_REQUIRE_GL. Not applying it myself — it fires a paid run. Merging on Linux-green and letting the post-merge macOS run cover the alpha half stays a legitimate call, but make it deliberately.
Confidence: HIGH
A stinger plays a keyed clip over the program while another transition runs beneath it. It is a transition of type "stinger" naming a Media Player block that declares itself a stinger clip source and feeds a keyed (DSK) input. Declaration is opt-in, so a looping graphic on a keyed input is left alone. The clip is held on its first frame while idle, so a take only starts playback: ~0.5 ms at any resolution, against up to 30.9 ms at 4K unparked. Cut point, the transition beneath and its duration are block properties, since where a clip covers follows from the artwork. The duration beneath is shortened to fit the clip, and a clip that cannot play still runs the transition beneath. The cut is anchored to the clip: its stream offset gives the output frame containing the cut point, and the mixer's output is held on the frame before it until the take is applied. Setting pad properties from another thread otherwise races the aggregator, by a different amount per layout. Keyed inputs fed by a declared source do not hold their last frame, or the frame a clip ended on would be composited when the input is next revealed. A take's claim on the mixer carries a token, so a take whose flow was stopped cannot act on the pipeline that replaced it. Straight alpha only: premultiplied alpha cannot be read from the file and no compositor operator corrects it, so a binding that declares it is refused. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
On a starved process the clip occasionally misses an output frame, and a stinger's keyed pad does not repeat its last frame, so that frame carries no clip pixels. When it was the frame the program changed on, the coverage-to-frame arithmetic underflowed and panicked. Such a take now counts as unreadable instead. At most half the takes may be, so a clip that never reaches the program at the cut, which would be unreadable on every take, still fails. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A repeat stinger fired right after the previous one completed put that clip's last frame on air before its own first frame: one frame at normal load, two or three on a starved machine. The keyed pad is set not to repeat its last frame, but videoaggregator only expires that frame on an output frame where the pad has nothing queued. The next take queues its first frame ahead of time, so the stale frame stays current until the new one is due, and revealing the keyed input shows it. A finished stinger now waits for the mixer's output to pass the clip's end before re-arming and releasing the mixer. The wait ends early if the take is superseded, and gives up after the mixer's latency plus 500 ms. The repeat test fires the second take as soon as the first completes, keeps every program frame, and judges frames by timestamp against the mixer position at the take, so a stale frame cannot pass as one the appsink held from before it. Frames with no clip on them, a late clip frame on a starved machine, are skipped rather than read as coverage falling. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
On a loaded machine the mixer's output trails the clock. A take gave up on the anchor at cut point + 500 ms of wall clock and applied the cut to whatever frame the mixer was on, often before the clip was on air, so the program switched with nothing covering it. A take now waits for the mixer to reach the cut frame however late that is, giving up only if the output stops advancing for 5 s. The keyed input comes off air once the mixer passes the clip's end rather than on a wall-clock timer, so a clip that started late plays out. A cut the clip may not cover is reported as StingerFailed with still_running true: the clip never reached the mixer (the keyed input is hidden and the cut made plainly), the mixer stalled, or the clip's frame for the cut point had not reached the mixer when the cut was released. The last is read from a per-take probe on the keyed pad that records the latest clip frame to arrive. A stinger's keyed input now repeats its last frame for two mixer frames instead of none, so a single late clip frame freezes the graphic rather than blanking it. The cut test no longer absorbs a cut with no clip on it: a take either reports, or must cut on the named frame (or read one early). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
09271e9 to
3569e42
Compare
|
From Claude, working with @wagenet. Reply to review 5301630969. The branch is rebased onto Requested change (unreadable takes). Agreed: it was a visible artefact, not noise, and it is no longer absorbed. Tracing it showed that most such takes were not late clip frames. The take was giving up on the anchor at cut point + 500 ms of wall clock while the mixer's output trailed the clock, so the cut landed on whatever frame the mixer was on, often before the clip was on air. The take now waits for the mixer to reach the cut frame. The cases that remain are reported as Alpha tests in CI. Still not run upstream. Locally at Skipped wait with no framerate. Now logs at Limits on the evidence: the late-clip-frame report has no deterministic test (it fired once in heavy-load runs, never at normal or CI-like load), and one compositor segfault in about 80 heavy-load runs is unattributed. Both are in the PR body. |
There was a problem hiding this comment.
Verdict: Comment — this supersedes my Comment of 2026-09-24 (head 09271e95), and the verdict stands. The branch was rebased. git range-diff shows the three reviewed commits unchanged (one-line rebase fix-up) plus one new commit, 3569e42. It is sound. What blocks approval is the unrun GL test and an overlap I missed last time.
Requested change
- Coordinate with #789 on landing order. #789 adds three wildcard-free
match selfaccessors onStromEvent(event_type(),flow_id(),is_high_frequency()), and this PR addsStingerStarted,StingerCompletedandStingerFailed. Whichever merges second fails to compile until it adds three arms to each accessor. Neither diff shows it; my earlieroverlaps=nonewas wrong.
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| The new keyed-pad watch is safe on the per-buffer path | CONFIRMED |
backend/src/state/stinger.rs:61 — ` seen.fetch_max(end, std::sync::atomic::Ordering::Relaxed);` is the probe's only work: no lock, no allocation. On every exit path, including the flow-restart return, Drop removes it: backend/src/state/stinger.rs:86 — ` self.pad.remove_probe(probe);` |
| The watch reads the same timeline as the cut frame | CONFIRMED |
The bridge restamps clip buffers with the stream offset: backend/src/blocks/builtin/mediaplayer/bridge.rs:436 — ` let adjusted = (pts_val.nseconds() as i64 + offset_ns).max(0) as u64;`. cut_frame_pts is computed from that same offset |
| A take no longer cuts early on wall clock while the mixer lags | CONFIRMED |
The wait now ends only when the mixer stalls: backend/src/state/stinger.rs:753 — ` if progress.stalled(Self::STINGER_CUT_STALL) {`. A cut with no clip is reported, not absorbed: backend/tests/stinger_test.rs:933 — ` !landed.contains(&Some(None)),` |
| The alpha half executed in this repo's CI | UNVERIFIED |
Unchanged. Check (Linux) job 107872151230 reports stinger_alpha_test 4 passed ... finished in 0.07s, and alpha_survives_gl_videomixer still skips there. .github/workflows/ci.yml:685 — ` STROM_REQUIRE_GL: 1` sits in build-macos, and Build (macOS) is skipping |
Diagnosis — The root cause is right: the old anchor gave up at cut point + 500 ms of wall clock, which is exactly the case where a lagging mixer cuts before the clip is on air. Following the mixer's position is the right layer. Coverage is BOUNDED in one respect: the wait for the clip's first buffer still gives up on wall clock (backend/src/state/stinger.rs:659 — ` return CutAnchor::NoClip;`). It is now reported.
The loop is CLOSED: it stops once the mixer reaches the cut frame, or after 5 s without progress. On the healthy path it costs one query_position every 2 ms, from the take to the cut. The two-frame repeat changes behaviour only on stinger-declared pads, and the teardown target adds the same window.
Radius — SHARED, unchanged. StingerFailed is new in this PR, so widening still_running breaks no client.
Overlaps — #789, which changes the same code path (the StromEvent accessors); see the requested change. #809 edits set_dsk_enabled next to the renamed function but not on it; it is also what gives the GL test a Linux run.
Tests & CI — Eight checks are green at 3569e429, and stinger_test reports 12 passed ... finished in 20.31s, so the rewritten cut test ran. The same job logs 647 gst_mini_object_unref criticals inside stinger_test. The previous head logged 619, so they predate this commit.
Before merge — Add the ci:macos label or land #809 first. I am not applying the label: it fires a paid run.
Confidence: HIGH
A stinger plays a keyed clip over the program while another transition runs beneath it. It is triggered through the existing transition endpoint as type
stinger, naming a Media Player block that declares itself a stinger clip source and is wired to a keyed (DSK) input.Cut point, the transition beneath, and its duration are Media Player block properties, since where a clip covers follows from the artwork; a take names only its clip. The clip is held on its first frame while idle, so a take only starts playback: 0.5 ms at 4K against 28.4 ms unparked. Declaration is opt-in, so a looping graphic on a keyed input is never parked or unlooped.
The parts worth reviewing
The cut beneath is anchored to the clip, not timed. The clip's stream offset gives the output frame containing the cut point, and a one-shot probe holds the mixer's output on the frame before it until the take is applied. The take waits for the mixer to reach that frame however far its output trails the clock, and gives up only if the output stops advancing for 5 s. Cutting on wall clock instead applies the cut to whichever frame the mixer is on, which on a loaded machine is often before the clip is on air.
A cut the clip may not cover is reported as
StingerFailedwithstill_running: true(aStingerCompletedstill follows). What a viewer sees in each case:A keyed input holds its last frame by default. For a stinger's input that repeat is bounded to two mixer frames: a single late clip frame freezes the graphic rather than blanking it, and the frame a clip ends on is not composited when the input is next revealed.
videoaggregatoronly drops an expired frame on an output frame where the pad has nothing queued, and the next take queues its first frame ahead of time, so a finished stinger waits for the mixer's output to pass the clip's end plus that repeat before hiding the keyed input, re-arming and releasing the mixer.A take's mixer claim carries a token, so a take whose flow is stopped mid-clip cannot drive the pipeline that replaces it.
Limitations
Straight alpha only: premultiplied alpha composites about 40% too dark and cannot be detected from the file, so a binding that declares it is refused. No fill+key pairs and no audio from the clip.
Following the mixer means a take, and
StingerCompleted, run as late as the mixer's output does. On a machine where the mixer ran at about a quarter of real time, completion came several seconds after the clip ended on the clock.Tests
Run at
3569e429, macOS, local:stinger_test(12): 12/12 at normal priority, and 12/12 in 5 runs undertaskpolicy -bwith four background-QoS CPU burners, which reproduces the macOS runner's flakes (4 of those runs on the same code before the rebase ontomain). It covers arming, an undeclared source left alone, a full cycle with re-arm, the refusal paths, clip-failure degradation, a take outliving its flow, composited pixels end to end, cut timing and a repeat take.stinger_alpha_test(4) withSTROM_REQUIRE_GL=1, so the GL case rendered rather than skipped.openapi_test,pipeline_lifecycle_test,vision_mixer_fx_test, 694 lib tests,cargo clippy --tests --all-features -D warnings,cargo fmt --check.stinger_preroll_benchis#[ignore]d as a measurement harness and was not run.Not run: the rest of the workspace suite, and anything on Linux. Upstream CI sets
STROM_REQUIRE_GLonly on the macOS job, so the GL alpha case has not run there.What the timing guards catch:
a_repeat_stinger_plays_forward_from_the_startfires the second take as soon as the first completes and judges every program frame by timestamp. Without the end-of-clip wait it failed 10/10 at normal priority when that wait was added; it is weak when starved, because a slow machine fires the second take late. It also fails if the clip leaves the program before it ends.Not guarded by a deterministic test: the late-clip-frame report. It fired once in the heavy-load runs and never at normal priority or CI-like load; forcing it would need a test-only delay in the clip path.
One segfault in the compositor's blend (
blend_pads, in ORC-generated code) appeared in about 80 heavy-load runs across both arms. It could not be reproduced in 40 targeted runs and is not attributed to this change.🤖 Generated with Claude Code