Repository navigation
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
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 |
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.
41bdf8b to
e8d3c37
Compare
Integration fix-up: the premultiplied DSK chain (Eyevinn#809) adds a converter that the videoconvert threading change does not know about, so it ran single-threaded for decoded clips. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit cf38144ac3957a638081fbeebd519936c9d1489c) (cherry picked from commit fa3cff270f91197e5c3f520e37ebd85e42670e29)
cefsrc paints premultiplied alpha and the vision mixer blends straight alpha, so HTML graphics composite too dark wherever they are partly transparent. Set dsk_N_alpha_mode = premultiplied on the mixer for every DSK input fed by a graphic assignment. The property comes from Eyevinn/strom#809; Strom without it stores and ignores the key. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
srperens
left a comment
There was a problem hiding this comment.
Verdict: Approve. This replaces my Approve of 2026-09-18 (review 5245632839, head 41bdf8b3), which the push dismissed. The verdict stands. The new head e8d3c37e is a rebase onto 8079d58 and nothing else: git range-diff pairs all seven commits, six of them = (identical), and commit 1 differs only in context lines that main moved underneath it. I re-read the one place where that context matters.
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| The rebase keeps the premultiplied chain intact next to main's newer DSK code | CONFIRMED |
backend/src/blocks/builtin/vision_mixer/builder/pipeline_cpu.rs:165 — if p.dsk_premultiplied(i) { still links queue → converter → stromunpremultiply → the DSK videoconvert. Main's backend/src/blocks/builtin/vision_mixer/builder/pipeline_cpu.rs:207 — // No output_format capsfilter here. A DSK is a keyer: forcing an now comes after that chain, so nothing forces an alpha-less format between the unpremultiply and compositor. The merge makes the chain safer, not less safe |
| The GL half still executes in this repo's CI at this head | CONFIRMED |
.github/workflows/ci.yml:159 — STROM_REQUIRE_GL: 1. Check (Linux) job 108424561489 (run 36249424199, head_sha e8d3c37) logs gpu_premultiplied_dsk_composites_correctly ... ok, gpu_blend_constant_follows_fade_to_black ... ok, both CPU cases ok, and wipe_between_letterboxed_sources_animates ... ok |
The alpha → blend-constant-color-alpha binding outlives the dropped handle |
EXTERNAL |
backend/src/blocks/builtin/vision_mixer/builder/pipeline_gpu.rs:603 — pad.bind_property("alpha", &pad, "blend-constant-color-alpha"). Unchanged from my earlier review. The assumption, as before: glib keeps a GBinding alive until one of its objects is finalized |
Tests & CI. Check (Linux), Build (Linux x86_64/ARM64), Check & Build (WASM), API Contract Check and Static JS Tests are green at e8d3c37. macOS and Windows show skipping. Nothing in the diff is platform-specific, so letting the post-merge main run cover them is reasonable.
One thing I noticed and ruled out: the job prints gst_mini_object_unref: assertion 'mini_object != NULL' failed about once per frame inside vision_mixer_premultiplied_alpha_test (36 times) and vision_mixer_fx_test (146). This PR does not introduce it:
- it was already there at the head I approved (job 105365686101: 35 and 147);
- the same signature appears in
output_refusal_test(125) andvision_mixer_keyed_alpha_test(24) on #904's main-basedCheck (Linux)job 109438704030, and neither test touches this diff.
It should probably be its own issue. It does not block this PR.
Overlaps
- #797 also adds
gstreamer1.0-glto CI, putting it in the cached package lists. This PRapt-get installs it in the Linux test job. The hunks do not collide. Whichever lands second carries a redundant install, which is harmless; clean it up in #797 if this merges first. - #822 (DSK edge on resized premultiplied inputs, Ask open) is blocked on this PR and builds on it. Merging does not settle its Option A vs B.
- #755: the stinger's
PremultipliedUnsupportedrefusal becomes replaceable once both land, as the PR body says.
Radius. SHARED, for the same reasons as in my earlier review. The product change is confined to the vision mixer builders plus one new element that nothing else creates. The CI change wraps every Linux backend test in xvfb-run. The job runs green with no skips, so that cost has been paid.
Confidence: HIGH
Integration fix-up: the premultiplied DSK chain (Eyevinn#809) adds a converter that the videoconvert threading change does not know about, so it ran single-threaded for decoded clips. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit cf38144ac3957a638081fbeebd519936c9d1489c) (cherry picked from commit fa3cff270f91197e5c3f520e37ebd85e42670e29) (cherry picked from commit ab4921c)
… flags Integration fix-up: upstream Eyevinn#789 adds two structured-logging flags to EventBroadcaster::new and AppState::new. Tests from Eyevinn#797, Eyevinn#803, Eyevinn#809, Eyevinn#810 and Eyevinn#844 still call the old signatures. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cefsrc paints premultiplied alpha and the vision mixer blends straight alpha, so HTML graphics composite too dark wherever they are partly transparent. Set dsk_N_alpha_mode = premultiplied on the mixer for every DSK input fed by a graphic assignment. The property comes from Eyevinn/strom#809; Strom without it stores and ignores the key. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Integration fix-up: the premultiplied DSK chain (Eyevinn#809) adds a converter that the videoconvert threading change does not know about, so it ran single-threaded for decoded clips. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit cf38144ac3957a638081fbeebd519936c9d1489c) (cherry picked from commit fa3cff270f91197e5c3f520e37ebd85e42670e29) (cherry picked from commit ab4921c)
… flags Integration fix-up: upstream Eyevinn#789 adds two structured-logging flags to EventBroadcaster::new and AppState::new. Tests from Eyevinn#797, Eyevinn#803, Eyevinn#809, Eyevinn#810 and Eyevinn#844 still call the old signatures. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cefsrc paints premultiplied alpha and the vision mixer blends straight alpha, so HTML graphics composite too dark wherever they are partly transparent. Set dsk_N_alpha_mode = premultiplied on the mixer for every DSK input fed by a graphic assignment. The property comes from Eyevinn/strom#809; Strom without it stores and ignores the key. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Integration fix-up: the premultiplied DSK chain (Eyevinn#809) adds a converter that the videoconvert threading change does not know about, so it ran single-threaded for decoded clips. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit cf38144ac3957a638081fbeebd519936c9d1489c) (cherry picked from commit fa3cff270f91197e5c3f520e37ebd85e42670e29) (cherry picked from commit ab4921c)
… flags Integration fix-up: upstream Eyevinn#789 adds two structured-logging flags to EventBroadcaster::new and AppState::new. Tests from Eyevinn#797, Eyevinn#803, Eyevinn#809, Eyevinn#810 and Eyevinn#844 still call the old signatures. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
e8d3c37 to
da5f82a
Compare
da5f82a to
a34bbeb
Compare
|
Claude (AI), on behalf of @wagenet. Rebased onto main. Non-mechanical parts:
Ran on macOS with |
srperens
left a comment
There was a problem hiding this comment.
Verdict: Request changes. This supersedes my review of 2026-09-29 (Approve at e8d3c37, auto-dismissed by the push), and the verdict does not stand. The product change has not moved: git range-diff shows commits 2-4 identical, and commit 1 differs only in context. What changed is main. #977 (afa7c4e) now renders GL on the Linux job through surfaceless EGL. That contradicts this PR's CI premise, and the branch no longer merges: a trial merge onto origin/main conflicts in .github/workflows/ci.yml, pipeline_gpu.rs and vision_mixer_fx_test.rs.
Requested changes
- Rebase onto main and drop the Xvfb step from 4b5ee22. Main's
Check (Linux)already setsSTROM_REQUIRE_GLwith no display server. - In
backend/tests/vision_mixer_premultiplied_alpha_test.rs, replace the localgl_available_or_required()withcommon::gl_available(&[...]). That helper callsinit_gl(), which picks EGL on a headless Linux host.
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| Surfaceless EGL fails on the runner, so GL needs GLX under Xvfb | CONTRADICTED |
Main push run 37933982230, Check (Linux) at dcccdd7: vision_mixer_fx_test 2 passed in 4.92 s and shader_validation_test 2 passed in 5.32 s, with no xvfb-run. See the main citations below |
| All five new tests executed at this head | CONFIRMED |
Job 110913177391: five ok, finished in 2.86s, including gpu_premultiplied_dsk_composites_correctly and gpu_blend_constant_follows_fade_to_black |
| The CPU chain and the GL binding are unchanged from my last review | CONFIRMED |
backend/src/blocks/builtin/vision_mixer/builder/pipeline_cpu.rs:165 — if p.dsk_premultiplied(i) {, backend/src/blocks/builtin/vision_mixer/builder/pipeline_gpu.rs:609 — pad.bind_property("alpha", &pad, "blend-constant-color-alpha") |
The CI change being replaced, on this branch: .github/workflows/ci.yml:152 — run: xvfb-run -a --server-args="-screen 0 1280x720x24" cargo test --package strom --features efp,nvidia, and backend/tests/vision_mixer_premultiplied_alpha_test.rs:474 — fn gl_available_or_required() -> bool {. On main, the helper is backend/tests/common/mod.rs:156 — pub fn gl_available(required: &[&str]) -> bool {, gated by .github/workflows/ci.yml:174 — STROM_REQUIRE_GL: 1.
Overlaps
- #797 changes the same output chain in
pipeline_gpu.rsand merges cleanly onto main. Land it first, then rebase this PR onto it. - #1056 asks whether page stingers on a CPU mixer should go through this PR's unpremultiply. That decision belongs here, and #1056 ships dark edges on CPU mixers if it lands first.
- #822 (Ask open) builds on this PR. Merging this one does not settle its options.
- The "Follow-up for #755" section is stale: #755 closed unmerged, and main's stinger (#1010) has no
PremultipliedUnsupported.
Radius: SHARED, as before. The product change stays inside the vision mixer builders plus one new element. After the rebase the CI change should go away entirely.
Tests & CI: one run at this head (2026-10-02), green. The run predates 69 main commits and the conflict, so it is not evidence for the merge. It needs a fresh run after the rebase. macOS and Windows show skipping, and the diff has no platform code. I ran nothing locally.
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>
Use common::gl_available, which renders through surfaceless EGL on a headless Linux host, and serialise the tests that render through llvmpipe. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
a34bbeb to
8cc98b3
Compare
|
Claude (AI), on behalf of @wagenet. Rebased onto main (dcccdd7) and pushed.
Ran locally on macOS with Not addressed: #1056 asks whether page stingers on a CPU mixer should go through this unpremultiply. That is a maintainer decision, and #797 landing order is yours to call. |
srperens
left a comment
There was a problem hiding this comment.
Verdict: Approve — supersedes my Request changes at a34bbeb, which I am dismissing. Both requested changes landed, and every test this PR adds ran green on Linux at this head.
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| The Xvfb CI change is gone | CONFIRMED |
git diff origin/main...pr809 --stat touches nothing under .github/. The GL gate is main's .github/workflows/ci.yml:174 — STROM_REQUIRE_GL: 1 |
| The premultiplied tests use the shared GL gate | CONFIRMED |
backend/tests/vision_mixer_premultiplied_alpha_test.rs:447 — if !common::gl_available(GL_ELEMENTS) { (and at line 464), calling backend/tests/common/mod.rs:156 — pub fn gl_available(required: &[&str]) -> bool { |
| All five new tests ran in CI, GPU ones included | CONFIRMED |
Job 113912327224: vision_mixer_premultiplied_alpha_test 5 passed in 2.64 s, including gpu_premultiplied_dsk_composites_correctly and gpu_blend_constant_follows_fade_to_black. No SKIP line anywhere in the job log |
| The product change is the one I approved on 2026-09-29 | CONFIRMED |
git range-diff: commits 2 and 4 are =. Commit 1 differs only in conflict context and chunks_exact_mut → as_chunks_mut (backend/src/gst/unpremultiply.rs:67 — for px in row.as_chunks_mut::<4>().0 {). Commit 3 differs only in context |
| The new closure holds the mixer weakly | CONFIRMED |
backend/src/blocks/builtin/vision_mixer/builder/pipeline_gpu.rs:32 — let dist_weak = dist_comp.downgrade();, upgraded inside the closure at line 695 |
Diagnosis: unchanged. The CPU path unpremultiplies at the input (backend/src/blocks/builtin/vision_mixer/builder/pipeline_cpu.rs:166 — if p.dsk_premultiplied(i) {). The GPU path corrects the blend on the pad (backend/src/blocks/builtin/vision_mixer/builder/pipeline_gpu.rs:703 — pad.bind_property("alpha", &pad, "blend-constant-color-alpha")). Coverage is ABSOLUTE for DSK inputs that the operator marks premultiplied. The default stays straight, and cpu_default_mode_leaves_the_source_alone guards that.
Radius: SHARED. It adds a property in strom-types, the frontend editor, both mixer builders, and one new element. Existing flows are untouched because the mode defaults to straight.
Overlaps: none that this merge settles.
- #797 changes the same GPU output chain. A local trial merge of the two heads is clean, in either order.
- #1056 asks whether page stingers on a CPU mixer should go through this unpremultiply. Merging this PR provides the mechanism but does not answer the question. The author leaves the call to the maintainer, on #1056.
- #822 (Ask open) builds on this PR, and merging does not choose between its options.
Tests & CI: run 37957695691 at 8cc98b3. Check (Linux), both Linux builds, WASM and API Contract are all green. vision_mixer_fx_test (2 passed) also ran under the shared gate. macOS and Windows are skipping, which is fine here because the diff has no platform code. I ran nothing locally.
Confidence: HIGH
Superseded by my review at 8cc98b3: both requested changes landed; verdict is now Approve.
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 tests in CI
The two GPU tests gate on
common::gl_available, so they render on the Linux job through surfaceless EGL (#977) and fail instead of skipping whenSTROM_REQUIRE_GLis set. They are#[serial(gl)]with the rest of the file: llvmpipe renders on the CPU and a sibling GL test can starve the pipeline under measurement. This PR no longer changes CI.Ran locally on macOS with
STROM_REQUIRE_GL=1 STROM_REQUIRE_GST_PLUGINS=1, after rebasing onto main at dcccdd7:cargo fmt --check,cargo clippy --workspace --all-targets -D warnings,vision_mixer_premultiplied_alpha_test(5/5, GL rendering),vision_mixer_fx_test(2/2),openapi_test, and lib tests matchingunpremultiplyandvision_mixer. Not run: the Linux job, and the revert checks again after the rebase.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.🤖 Generated with Claude Code