Conversation
|
Noted as a draft, so I am holding off on a full review until it is marked ready for review. If you want one before then, say so here or request me as a reviewer. |
srperens
left a comment
There was a problem hiding this comment.
Verdict: Request changes — the compositing work is right and the on-air table is convincing, but the GL half of the feature has no CI coverage: both GPU tests take an early return on this repo's runners and report ok, which is the pattern CLAUDE.md's Tests section exists to stop. Everything below that is a note, not an objection.
Requested changes
- Make the GL skip loud in CI, the way this repo already does it elsewhere.
.github/workflows/ci.yml:149—STROM_REQUIRE_GST_PLUGINS: 1— is the existing precedent: an env var that converts a silent skip into a failure. Add the equivalent (STROM_REQUIRE_GL, checked insidegl_environment_available) plus the packages a headless llvmpipe context needs, so either the two GPU tests execute or the job goes red. If a GL context genuinely cannot be had on these runners, say so in the PR body and drop the two tests to a documented manual check — a test that returns before asserting is worse than no test, because it reads as coverage on the next reviewer's screen.
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| The CPU tests really execute in CI | CONFIRMED |
Check (Linux) job 104488573594 of run 35000840310 (head_sha 21b484ff8657233f5e96f971aa02dd82f932bf39): test cpu_premultiplied_dsk_composites_correctly ... ok, test cpu_premultiplied_clip_through_media_player ... ok |
| The GPU tests do not | CONTRADICTED as coverage |
backend/tests/vision_mixer_premultiplied_alpha_test.rs:454 — eprintln!("SKIP: GL environment unavailable (no context or GL elements missing)"); — and in that job both finished inside 0.09 s of the file starting, against the ~5 s the body says a rendering run takes |
| The DSK-on ordering fix cannot disturb the CPU mixer | CONFIRMED |
backend/src/gst/pipeline/effects/mixer_ops.rs:248 — if enabled && pad.has_property("blend-constant-color-alpha") { — compositor pads do not carry that property, so the arm is inert there |
| The new element allocates nothing per frame | CONFIRMED |
backend/src/gst/unpremultiply.rs:53 — `static LUT: LazyLock<Box<[u8]>> = LazyLock::new( |
Diagnosis — Root cause is right and not a guess, and the two backends are fixed by different means for a stated reason: CPU converts the source, GL corrects the blend. Coverage is BOUNDED, and the bound is stated honestly — DSK inputs only, and a ~1 px dark edge when compositor resizes a premultiplied source.
Radius — SHARED: mixer_ops.rs is on the path of every DSK toggle on every vision mixer, CPU and GPU, and the new element registers process-wide. Both are gated — by has_property and by an opt-in property defaulting to straight — so existing flows are untouched, which the cpu default mode case checks.
Worth keeping — backend/tests/vision_mixer_premultiplied_alpha_test.rs:401 — // than skipping keeps the test from passing green without running. — the CPU tests deliberately panic rather than skip when a plugin is absent. That is exactly the right instinct, and it is what makes the GPU pair's early return stand out rather than blend in.
Note, not blocking — backend/src/blocks/builtin/vision_mixer/builder/pipeline_gpu.rs:596 — pad.bind_property("alpha", &pad, "blend-constant-color-alpha") — binds a pad to itself. That it survives the builder returning depends on GLib keeping the GBinding alive independently of the dropped Rust handle, which this repository cannot verify; EXTERNAL, and the fade-to-black test would catch a regression in it — once that test runs.
Confidence: HIGH
compositor and glvideomixerelement blend straight alpha, and caps cannot say that a stream is premultiplied. cefsrc paints premultiplied BGRA, so every HTML graphic on a keyed input composites too dark wherever alpha is partial: 50% white over grey 64 reads 96 on both backends instead of 160. Each DSK input gets a dsk_N_alpha_mode property, straight (default) or premultiplied, matching the alpha_mode values used for stinger sources. CPU: a premultiplied input runs through a converter and a new in-place element, stromunpremultiply (a VideoFilter over packed RGBA-family formats, lookup table with a 0/255 fast path), before the existing DSK chain. The converter lets decoded YUV-with-alpha clips reach it. GL: no element. The DSK pad blends source RGB with constant-alpha, and a property binding keeps blend-constant-color-alpha equal to the pad's alpha, so DSK toggles, takes, the property API and fade-to-black's control binding all stay correct. blend-function-src-rgb=one would be right only at full pad alpha. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cefsrc can now convert to straight alpha itself (centricular/gstcefsrc#113). Declaring such a source premultiplied as well converts twice and composites too bright, so the alpha mode description says to leave it straight. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A premultiplied DSK pad on the GL mixer blends with constant-alpha, and a property binding copies alpha into the blend constant after each write. DSK on sets alpha from a non-streaming thread, so a frame drawn between the two writes sees alpha 1 with the constant still 0 and puts the graphic's silhouette on air in black. Set the constant first. Disabling needs nothing: the mixer skips pads at alpha 0. No test: the window is two consecutive property writes, which a test cannot hit reliably. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Log an unrecognised or non-string dsk_N_alpha_mode before falling back to straight. Open Live writes the value from code, and a typo otherwise shows only as a graphic that is too dark. - The property description no longer names cefsrc's unpremultiply option, which the pinned build lacks, and says a premultiplied source on the CPU mixer should arrive at program resolution: compositor resizes after the conversion, which darkens edges by about a pixel. - The CPU chain comment no longer claims the converter handles GPU memory; plain videoconvert does not. - The build log line includes each DSK's alpha mode. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two GPU tests in vision_mixer_premultiplied_alpha_test returned before asserting on Linux CI and reported ok, which reads as coverage. The Linux runner can render: gstreamer1.0-gl supplies the GL elements, and GLX against an Xvfb display on llvmpipe gives a context (EGL cannot initialize — the runner has a card node but no render node). A bare glvideomixerelement blend there reads 160 for 50% white over grey 64, matching the hardware path. Install that stack on the Check (Linux) job, run the backend suite under xvfb-run, and set STROM_REQUIRE_GL so a GL skip fails the job. The GPU tests in vision_mixer_fx_test and shader_validation_test also execute there now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
llvmpipe renders on the CPU and costs whole cores, and these tests also scan every program frame they pull. Two of them at once on a 4-core runner starves the observer: the wipe test saw the picture before the transition and the one after it, and concluded the wipe never animated. Run alone, the same wipe arrives frame by frame at 30 fps. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
21b484f to
d756cbf
Compare
|
Fixed, and the GL tests now run in CI rather than skipping loudly. The premise turned out to be wrong in my favour: the Linux runner was missing So the Linux job installs that stack, runs the backend suite under That surfaced one real problem: llvmpipe renders on the CPU and costs whole cores, and two GL tests at once on a 4-core runner starve each other. Verified on a dispatched run of this branch, Check (Linux): On the self-binding note: Also rebased onto current main. 🤖 Generated with Claude Code |
The observer pulled from an appsink with max-buffers=1 drop=true, so it saw whichever frame was current when it asked and the sink discarded the rest. One scan of a 1280x720 frame costs more than the frame interval on a slow llvmpipe runner, and a 2 s wipe then reads as the picture before it followed by the picture after it — "no frame showed both sources", with the end state correct. That is how the inverted wipe failed on CI while the classic one, which had the settle wait's slack in front of it, passed. Record every program frame in a BUFFER probe instead, sampling one pixel in 64 so the scan cannot throttle the pipeline it measures, and ask the recorded series whether any frame after the take showed both sources. What the mixer rendered no longer depends on what the observer managed to grab. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
srperens
left a comment
There was a problem hiding this comment.
Verdict: Approve — supersedes my Request changes of 2026-09-17 (review-5236829941, head 21b484ff), which no longer stands. The one requested change is in, and it was answered better than I asked: the GL tests now execute on Linux, on every PR, rather than behind a label-gated macOS job.
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| The GL half now actually executes in this repo's CI | CONFIRMED |
.github/workflows/ci.yml:150 — run: sudo apt-get install -y gstreamer1.0-gl libgl1-mesa-dri libglx-mesa0 xvfb with .github/workflows/ci.yml:159 — STROM_REQUIRE_GL: 1. In Check (Linux) job 105365686101, vision_mixer_premultiplied_alpha_test.rs reports 5 passed ... finished in 2.31s with gpu_premultiplied_dsk_composites_correctly ... ok — against the 0.09 s green skip at the previous head |
| The change does not silently flip the whole suite onto GPU paths | CONFIRMED |
backend/src/gpu.rs:100 — !r.contains("llvmpipe") && !r.contains("softpipe") && !r.contains("swrast") — so auto-selection still picks the CPU compositor on this runner, exactly as the step's comment claims. Only tests that ask for the GPU backend change behaviour |
| No test anywhere in the job skips silently under the new environment | CONFIRMED |
zero SKIP: lines in the whole Check (Linux) log, and the job is green |
The alpha -> blend-constant-color-alpha binding outlives the dropped Rust handle |
EXTERNAL |
backend/src/blocks/builtin/vision_mixer/builder/pipeline_gpu.rs:596 — pad.bind_property("alpha", &pad, "blend-constant-color-alpha"). Assumption stated: glib keeps a GBinding alive until one of the two objects is finalized, so discarding the builder's return value does not unbind. Not settleable from this repository |
A side effect worth more than the feature — this fixes a coverage hole that predates the PR. vision_mixer_fx_test.rs ran in 6.21 s in this job; on main it has been finishing in ~0.04 s, which is the documented signature of both its tests skipping behind the GL guard. Two tests that have silently guarded nothing for weeks now render for real. That is not claimed in the PR body and should not be lost in the merge.
Ordering note for #755 — that PR's four alpha tests are gated on the same STROM_REQUIRE_GL, which today is set on the macOS job alone, so they skip (0.09 s) and I asked there for the ci:macos label. If this PR merges first, they execute on Linux for free and that ask disappears. Merging #809 before #755 is the cheaper order.
Repo rules — register_element_setup captures a WeakRef and upgrades inside the closure, so no strong reference to the mixer is held by a closure the element owns. The new unpremultiply element (backend/src/gst/unpremultiply.rs:31 — pub fn register() -> bool {) registers statically and idempotently, and holds no pipeline reference. Shared types went to strom-types (types/src/vision_mixer.rs). New workspace dependency gstreamer-base is pinned at 0.25/v1_22, matching every other GStreamer crate in Cargo.toml.
Radius — SHARED, and argued: the product change is confined to the vision mixer's CPU and GPU builders plus one new element that nothing else instantiates. What reaches further is the CI change, which now wraps every backend test on Linux in xvfb-run and sets three GL environment variables for all of them. The risk that carries is a previously-skipping test starting to fail; the job is green and skip-free, so that risk has been paid down rather than deferred.
Confidence: HIGH
Superseded by my review at head 41bdf8b: the requested CI change is in and the GL tests now execute on Linux, so this CHANGES_REQUESTED sits at a dead head and misreports the PR.
Written by Claude (AI), reviews by a human.
compositorandglvideomixerelementblend straight alpha, and caps cannot say that a stream is premultiplied.cefsrcpaints premultiplied BGRA, so every HTML graphic on a keyed input is too dark wherever alpha is partial: 50% white over grey 64 reads 96 on both backends; correct is 160.Each DSK input gets a
dsk_N_alpha_modeproperty,straight(default) orpremultiplied. The values matchalpha_modein #755. The property is not live; changing it needs a flow restart.CPU. A premultiplied input runs through a converter and a new in-place element,
stromunpremultiply, ahead of the existing DSK chain. It is aVideoFilterover BGRA/RGBA/ARGB/ABGR using a lookup table with a 0/255 fast path, registered on first use. The converter lets decoded YUV-with-alpha clips (A420) reach it and is passthrough forcefsrc. Cost at 1080p, optimized build: 0.5 ms per frame for a lower-third-shaped graphic, 1.4 ms when every pixel has partial alpha. Those figures assume the element can write the frame in place. When the frame is shared, as with the pinned gstcefsrc re-pushing its last paint or a tee in front, GStreamer copies it first: about 0.1 ms more per 1080p frame, measured on an M4 Max.GL. No element. The DSK pad blends source RGB with
constant-alpha, which is correct while the blend constant equals the pad's alpha. A GObject property binding (alpha→blend-constant-color-alpha) keeps them equal for every writer: DSK toggles, takes, the pad property API, and fade to black, whose control binding setsalphaper frame.blend-function-src-rgb=oneis right only at full pad alpha (176 instead of 112 at 0.5). DSK on writes the constant beforealpha: the binding copies it after the write, and a frame drawn in between would show the graphic's silhouette in black.Known limitation. On the CPU mixer,
compositorresizes the DSK to program size afterstromunpremultiply, so a premultiplied source at another resolution gets a ~1 px dark edge (20-60 levels on light graphics, measured with barecompositor). That covers Media Player clips at their native size andcefsrcwired straight into a 720p or 4K program. A Video Format block at program resolution in front of the DSK input avoids it; Open Live's generated flows already do this, and the property description says so. GL is unaffected: it resizes the premultiplied texture.On air
Headless backend with a real
cefsrcDSK graphic over grey 64, program tapped to PNG. Three runs, identical:Opaque regions are unchanged in every case.
Tests
vision_mixer_premultiplied_alpha_testbuilds the real block throughPipelineManager, with avideotestsrcpainting128,128,128,128ondsk_in_0(CI has nocefsrc):Each part of the fix was reverted in turn and the tests fail as expected: without the CPU chain both CPU premultiplied tests read 96 / 95; without the GL blend function both GL tests read 96; without the binding both GL tests read 32.
Ran locally on macOS (GStreamer 1.28.6): the full
cargo testbackend suite (681 passed, 2 ignored),strom-typeslib tests,openapi_test,cargo clippy --workspace --all-targets -D warnings.openapi.jsonis unchanged because block definitions are not part of the schema.GL now runs in CI
The two GPU tests used to return before asserting on Linux and report
ok. They execute there now. The runner was missinggstreamer1.0-gl— the GL elements were never installed, so the probe could not have succeeded whatever the context situation. With that package plus Mesa, GL works through GLX on an Xvfb display: EGL is not an option, the runner has a/dev/dricard node but no render node, and surfaceless EGL fails withEGL_NOT_INITIALIZED. A bareglvideomixerelementblend there reads 160 for 50 % white over grey 64 — llvmpipe's blend matches the hardware path — andgltestsrcprerolls in ~1.2 s.So the Linux job installs that stack, runs the backend suite under
xvfb-run, and setsSTROM_REQUIRE_GL=1(the existinggl_available_or_requiredgate, whichvision_mixer_fx_testalready had and this test file now shares) so a GL skip fails the job.vision_mixer_fx_testandshader_validation_testexecute there too.One consequence: llvmpipe renders on the CPU and costs whole cores, and two GL tests at once on a 4-core runner starve each other — the wipe test in
vision_mixer_fx_testsaw the picture before the transition and the one after it and concluded the wipe never animated. Run alone it arrives frame by frame at 30 fps. Both files' rendering tests are#[serial(gl)]now.Verified on a dispatched run of this branch (Check (Linux)):
gpu_premultiplied_dsk_composites_correctly ... ok,gpu_blend_constant_follows_fade_to_black ... ok, the three CPU cases ok,vision_mixer_fx_test2 passed in 5.91 s,shader_validation_test2 passed in 3.81 s. Vision mixer auto-selection is unaffected:has_hardware_gl()rejects llvmpipe, so only tests that ask for the GPU backend behave differently.The follow-up commits (DSK-on ordering, a warning for unrecognised
dsk_N_alpha_modevalues, the edge note in the property description, a build log field) were checked withcargo clippy --workspace --all-targets -D warnings,vision_mixer_premultiplied_alpha_testwith the GL tests rendering locally, and the backend lib tests matchingvision_mixer,unpremultiplyandmixer_ops. The DSK-on ordering has no test: it closes a window of microseconds between two property writes, which a test cannot hit reliably.Relation to centricular/gstcefsrc#113
That PR adds
unpremultiplytocefsrc, fixing HTML graphics at the source for every consumer, including the Compositor block and PiP overlays this PR does not cover. Strom can set it once the Dockerfile's gstcefsrc pin (b633408) is bumped. This PR is still needed for premultiplied clips.Do not use both on one input: that converts twice and composites too bright. The property description says so. In Open Live's flows, converting in
cefsrcalso moves the conversion ahead of the Video Format resize, which brings back the dark edge above whenever program is not 1080p.Follow-up for #755
Once both land, the stinger's
PremultipliedUnsupportedrefusal can be replaced by setting the keyed input'sdsk_N_alpha_modetopremultiplied(comment on #755).🤖 Generated with Claude Code