Skip to content

feat(vision_mixer): composite premultiplied-alpha DSK inputs correctly - #809

Open
wagenet wants to merge 5 commits into
Eyevinn:mainfrom
wagenet:wagenet/keyed-premultiplied-alpha
Open

wagenet wants to merge 5 commits into
Eyevinn:mainfrom
wagenet:wagenet/keyed-premultiplied-alpha

Conversation

@wagenet

@wagenet wagenet commented Sep 12, 2026 •

Copy link
Copy Markdown
Contributor

Written by Claude (AI), reviews by a human.

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 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_mode property, straight (default) or premultiplied. The values match alpha_mode in #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 a VideoFilter over 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 for cefsrc. 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 sets alpha per frame. blend-function-src-rgb=one is right only at full pad alpha (176 instead of 112 at 0.5). DSK on writes the constant before alpha: 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, compositor resizes the DSK to program size after stromunpremultiply, so a premultiplied source at another resolution gets a ~1 px dark edge (20-60 levels on light graphics, measured with bare compositor). That covers Media Player clips at their native size and cefsrc wired 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 cefsrc DSK graphic over grey 64, program tapped to PNG. Three runs, identical:

50% white, pad α 1 50% red, pad α 1 50% white, pad α 0.5 50% red, pad α 0.5
CPU, default 96 96,32,32 80 80,48,48
CPU, premultiplied 160 160,32,32 112 112,48,48
GL, default 96 96,32,32 80 80,48,48
GL, premultiplied 160 160,32,32 112 112,48,48

Opaque regions are unchanged in every case.

Tests

vision_mixer_premultiplied_alpha_test builds the real block through PipelineManager, with a videotestsrc painting 128,128,128,128 on dsk_in_0 (CI has no cefsrc):

  • CPU, premultiplied: 160 at pad alpha 1, 112 at 0.5.
  • CPU, property unset: 96, so existing flows are untouched.
  • CPU, premultiplied A420 FFV1 clip through the Media Player block: 157.5 / 110.4 (videoconvert's A420 round trip turns 128 into 126).
  • GL, premultiplied: 160 / 112.
  • GL, fade to black: the blend constant reaches 0 with the animated pad alpha.

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 test backend suite (681 passed, 2 ignored), strom-types lib tests, openapi_test, cargo clippy --workspace --all-targets -D warnings. openapi.json is 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 when STROM_REQUIRE_GL is 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 matching unpremultiply and vision_mixer. Not run: the Linux job, and the revert checks again after the rebase.

Relation to centricular/gstcefsrc#113

That PR adds unpremultiply to cefsrc, 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 cefsrc also 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

@srperens

Copy link
Copy Markdown
Collaborator

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 srperens left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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 inside gl_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

@wagenet

wagenet commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

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 gstreamer1.0-gl entirely, so the GL elements were never installed and the probe could not have succeeded whatever the context situation. With that package plus Mesa, a context is available — through GLX on an Xvfb display, not EGL (the runner has a /dev/dri card node but no render node, and surfaceless EGL fails with EGL_NOT_INITIALIZED). A bare glvideomixerelement blend under llvmpipe reads 160 for 50 % white over grey 64, matching the hardware path, and gltestsrc prerolls in ~1.2 s.

So the Linux job installs that stack, runs the backend suite under xvfb-run, and sets STROM_REQUIRE_GL=1. This test file now uses the same gl_available_or_required gate vision_mixer_fx_test already had, so the skip fails the job instead of passing green. vision_mixer_fx_test and shader_validation_test execute on Linux now too.

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. wipe_between_letterboxed_sources_animates failed the first time — it 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, so 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, vision_mixer_fx_test 2 passed in 5.91 s, shader_validation_test 2 passed in 3.81 s, job green. Auto backend selection is untouched — has_hardware_gl() rejects llvmpipe, so only tests that explicitly ask for the GPU backend behave differently.

On the self-binding note: bind_property("alpha", &pad, "blend-constant-color-alpha") — agreed it rests on GLib keeping the GBinding alive after the Rust handle drops, and gpu_blend_constant_follows_fade_to_black is the test that would catch it going wrong. That test runs in CI now.

Also rebased onto current main.

🤖 Generated with Claude Code

srperens
srperens previously approved these changes Sep 18, 2026

@srperens srperens left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@srperens
srperens dismissed their stale review September 18, 2026 08:09

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.

@wagenet
wagenet force-pushed the wagenet/keyed-premultiplied-alpha branch from 41bdf8b to e8d3c37 Compare September 26, 2026 14:42
wagenet added a commit to wagenet/strom that referenced this pull request Sep 27, 2026
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)
wagenet added a commit to wagenet/open-live that referenced this pull request Sep 28, 2026
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
srperens previously approved these changes Sep 29, 2026

@srperens srperens left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) and vision_mixer_keyed_alpha_test (24) on #904's main-based Check (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-gl to CI, putting it in the cached package lists. This PR apt-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 PremultipliedUnsupported refusal 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

wagenet added a commit to wagenet/strom that referenced this pull request Sep 29, 2026
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)
wagenet added a commit to wagenet/strom that referenced this pull request Sep 29, 2026
… 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>
wagenet added a commit to wagenet/open-live that referenced this pull request Sep 29, 2026
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>
wagenet added a commit to wagenet/strom that referenced this pull request Sep 29, 2026
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)
wagenet added a commit to wagenet/strom that referenced this pull request Sep 29, 2026
… 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>
wagenet added a commit to wagenet/open-live that referenced this pull request Sep 30, 2026
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>
wagenet added a commit to wagenet/strom that referenced this pull request Oct 1, 2026
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)
wagenet added a commit to wagenet/strom that referenced this pull request Oct 1, 2026
… 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>
@wagenet

wagenet commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Claude (AI), on behalf of @wagenet.

Rebased onto main. Non-mechanical parts:

Ran on macOS with STROM_REQUIRE_GL=1 STROM_REQUIRE_GST_PLUGINS=1: fmt, cargo clippy --workspace --all-targets -D warnings, frontend wasm32 clippy, vision_mixer_premultiplied_alpha_test (5/5, GL rendering), vision_mixer_fx_test, vision_mixer_source_resize_test, vision_mixer_cpu_test, both CUDA vision mixer tests, shader_validation_test, failed_start_teardown_test, openapi_test, and lib tests matching vision_mixer/unpremultiply/properties across the workspace. All pass. The Linux Xvfb job was not run locally.

@srperens srperens left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Rebase onto main and drop the Xvfb step from 4b5ee22. Main's Check (Linux) already sets STROM_REQUIRE_GL with no display server.
  2. In backend/tests/vision_mixer_premultiplied_alpha_test.rs, replace the local gl_available_or_required() with common::gl_available(&[...]). That helper calls init_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.rs and 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

wagenet and others added 5 commits October 9, 2026 09:14
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>
@wagenet
wagenet force-pushed the wagenet/keyed-premultiplied-alpha branch from a34bbeb to 8cc98b3 Compare October 9, 2026 16:14
@wagenet

wagenet commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

Claude (AI), on behalf of @wagenet.

Rebased onto main (dcccdd7) and pushed.

  • Conflict in pipeline_gpu.rs: kept both the underlay-watch clone and this PR's dist_weak clone.
  • The Xvfb/xvfb-run CI change is gone; ci.yml is now identical to main. The wipe-test rewrite and the vision_mixer_fx_test serialisation went with it, since they were workarounds for GL under Xvfb.
  • vision_mixer_premultiplied_alpha_test now uses common::gl_available instead of its own probe.
  • Clippy on Rust 1.98 (chunks_exact_to_as_chunks) flagged stromunpremultiply; switched to as_chunks.
  • PR body: dropped the Xvfb section and the stale feat(vision-mixer): stinger transitions #755 follow-up.

Ran locally on macOS with STROM_REQUIRE_GL=1: fmt, clippy -D warnings, the premultiplied test (5/5), vision_mixer_fx_test, openapi_test, and lib tests for unpremultiply/vision_mixer. The Linux job has not run on this head.

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.

wagenet added a commit to wagenet/strom that referenced this pull request Oct 9, 2026

@srperens srperens left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@srperens
srperens dismissed their stale review October 12, 2026 08:07

Superseded by my review at 8cc98b3: both requested changes landed; verdict is now Approve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants