Repository navigation
Conversation
srperens
left a comment
There was a problem hiding this comment.
Verdict: Comment — the new job is the real thing: it defeats the skip guard, and the three tests are confirmed executing in this repository rather than only on the fork. The entrypoint refactor is behaviour-preserving as far as it can be read statically. What holds it short of Approve is that the Dockerfile half ships in an image no check on this pull request builds.
Requested changes
- Either extend the
cef-changespath trigger to builddocker/strom-full/Dockerfileon a pull request that touches it, or state in the body that the Dockerfile change is covered only by the local arm64 build..github/workflows/docker-publish-full.yml:8—workflow_run:— and aworkflow_dispatch, are its only triggers, so the first CI build of the new bind-mount happens after merge, during a publish.
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| All three tests executed here, not only on the fork | CONFIRMED |
job 103644297674 of run 34727553991, whose head_sha is 29364e81e48d403065044c4ff4ae909bc7b273c8: STROM_REQUIRE_CEF: 1 in the job env, then test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 6.37s |
The Chromium flags are unchanged by the move into cef-env.sh |
CONFIRMED |
docker/strom-full/cef-env.sh:13 — CEF_SOFTWARE_FLAGS="no-sandbox,disable-gpu,disable-gpu-compositing,use-gl=disabled,${CEF_COMMON_FLAGS}" expands byte-for-byte to the string the entrypoint set inline at all three branches; docker/strom-full/cef-env.sh:17 — CEF_GPU_FLAGS="no-sandbox,use-gl=angle,use-angle=vulkan,enable-gpu-rasterization,ignore-gpu-blocklist,enable-zero-copy,${CEF_COMMON_FLAGS}" likewise, and the five symlinks moved into install-gstcefsrc.sh resolve to the same paths |
| The image still builds | UNVERIFIED |
docker/strom-full/Dockerfile:22 — RUN --mount=type=bind,source=install-gstcefsrc.sh,target=/tmp/install-gstcefsrc.sh \ — is reached by no check on this PR. The bind source is correct against context: docker/strom-full, and docker/build-push-action@v7 supplies BuildKit, so I expect it to work — but expecting is not the same as a green build |
Diagnosis — Right layer. The gap was that cefsrc is absent everywhere in CI, so an element behind HTML sources and DSK graphics had no guard at all; installing the real plugin in a dedicated job is the only thing that closes it. Coverage is BOUNDED, and the bound is honestly stated: test 2 is a DSK smoke test because the pinned b633408 advertises framerate=[1/1, 60/1], so it cannot yet guard the 0/1 stall.
Radius — SHARED, and it needs arguing because the diff edits a shipped artifact. It changes the startup environment of every strom-full deployment, but no application code, no API surface and no event contract; the blast is confined to whether cefsrc renders in the image. The failure mode if cef-env.sh were wrong is loud — docker/strom-full/entrypoint.sh:32 — . /usr/local/lib/strom/cef-env.sh — sources it unconditionally, so a missing or broken file fails at container start rather than degrading quietly.
Tests & CI — All ten checks green at this head, including Test (Linux, cefsrc) (7m0s). This is the rare PR that satisfies the CLAUDE.md rule most often broken — a test needing a GStreamer element that can actually run in CI — because STROM_REQUIRE_CEF=1 converts the skip into a failure. Worth recording that the three tests still skip in Check (Linux), which is correct, and that .github/workflows/ci.yml:125 — && (github.event_name == 'push' — is what keeps them honest: a PR that breaks cefsrc without touching the trigger paths goes unnoticed until the merge to main runs the job.
Confidence: HIGH
29364e8 to
167dd1e
Compare
|
Posted by Claude (Claude Code) on behalf of @wagenet. Rebased on current main and addressed the requested change. The unbuilt Dockerfile. Taken the first branch: a new It ran green on this PR: A flake the rebase exposed. The first post-rebase run failed Failure rate before the change was 1 in 6 (one Eyevinn run, three fork runs at the same commit passing). After it, 5 of 5: this PR's run plus four fork dispatches. I could not reproduce it locally — the tests skip on macOS and the local Docker daemon is down — so the opaque-black-before-load explanation rests on that one failure message. |
srperens
left a comment
There was a problem hiding this comment.
Verdict: Approve — supersedes my review at 29364e81. Its one request is closed in the stronger form: rather than documenting the gap, this now builds the image on a pull request that changes it, and that job has run green at this head. Nothing else in the diff moved — the two new commits are that job and the alpha test's paint wait.
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| The image is now built by a check on this PR, and pushes nothing | CONFIRMED |
.github/workflows/ci.yml:228 — if: needs.cef-changes.outputs.image == 'true' — gates it; .github/workflows/ci.yml:243 — push: false — and .github/workflows/ci.yml:249 — cache-from: type=gha,scope=docker-full-amd64 — reads the publish cache with no cache-to, no registry login and no secret in the job |
That check exercises cef-env.sh, not just the build |
CONFIRMED |
docker/strom-full/Dockerfile:42 — ENTRYPOINT ["/entrypoint.sh"] — with docker/strom-full/entrypoint.sh:78 — exec "$@", so docker run strom-full:ci gst-inspect-1.0 cefsrc sources the env file before loading the plugin |
All three tests executed at e68a637, none skipped |
CONFIRMED |
job 105280587876 of run 35244345206: STROM_REQUIRE_CEF: 1 in the job env, then running 3 tests and test result: ok. 3 passed; 0 failed; 0 ignored |
| The paint wait fixes the flake by mechanism, not by luck | CONFIRMED |
backend/tests/cefsrc_test.rs:523 — ` let red_or_blue = g < 32 && ((r > 223 && b < 32) |
The pinned b633408 advertises framerate=[1/1, 60/1], so test 2 cannot guard the 0/1 stall |
EXTERNAL |
a prebuilt upstream plugin, not readable from this repository; the body states the bound and I am taking it at face value |
Diagnosis — Unchanged from my last review and still right: cefsrc was absent from every CI job, so the element behind HTML sources and DSK graphics had no guard at all. Coverage is BOUNDED and the bound is stated rather than glossed — test 2 is a DSK smoke test until the pin passes c89c2ce, and tests 1 and 3 are not reverse-tested because flipping cefsrc's alpha needs a different build of it. CLAUDE.md asks exactly that disclosure of a test that cannot be reverse-tested, and the body makes it.
Radius — SHARED, argued as before: the diff edits a shipped artifact's startup environment but no application code, no API surface and no event contract. What is new since that argument is that the failure mode is no longer only reasoned about — the image now builds and loads cefsrc in CI.
Overlaps — #797, #809 and #838 also edit .github/workflows/ci.yml, in other jobs; this adds its block above check-linux, so the collision is rebase cost rather than a contested decision. None of them owns a design question this merge would settle. The one real interaction is already handled in the body: the composited on-air alpha is deliberately not asserted because #809 changes it, leaving it to whichever lands second. #822 is the open report about premultiplied DSK inputs that test 3 gives a fixed reference point for.
Confidence: HIGH
… 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>
… 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>
… 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>
761b6d9 to
fe9a105
Compare
|
Claude (AI), on behalf of @wagenet. Rebased onto main.
Ran: fmt and |
Add a Test (Linux, cefsrc) job that installs the prebuilt gstcefsrc plugin in an ubuntu:questing container, the strom-full base, and runs backend/tests/cefsrc_test.rs with STROM_REQUIRE_CEF=1, so a missing cefsrc fails instead of skipping. It runs on main, on ci:cef PRs, on PRs touching the gstcefsrc pin or the strom-full image, and on dispatch. The tests cover flow restarts (finalization, threads, CEF subprocesses), a cefsrc on a vision mixer DSK input reaching the compositor, and cefsrc emitting premultiplied alpha. The image and CI share one gstcefsrc version (the Dockerfile ARG), one install script and one runtime script for the Chromium flags, Xvfb, cache and mallinfo shim. The image's filesystem gains only cef-env.sh, and the entrypoint's environment is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The publish workflow was the only build of docker/strom-full/Dockerfile, and it runs after merge. The new job builds the image for amd64 without pushing, then loads cefsrc through the entrypoint, which fails if cef-env.sh or the plugin install is broken. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The premultiplied-alpha test counted any frame with a non-zero centre alpha as painted. On Eyevinn CI it sampled an opaque black frame (BGRA 0,0,0,255) and failed. It now waits for the animated corner square to show red or blue, which only the page's script draws. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fe9a105 to
407a40e
Compare
srperens
left a comment
There was a problem hiding this comment.
Verdict: Comment. This supersedes my review of 2026-09-18 (Approve at e68a637, auto-dismissed by the push). The diff still earns that verdict, but I cannot approve on a red Check (Linux), and that failure is not this PR's. The new head 407a40e is a rebase onto 0216c1c. In git range-diff, commits 2 and 3 are identical. Commit 1 carries main's #979 cache handling into cef-env.sh and main's tarball checksum into install-gstcefsrc.sh.
Needs a maintainer
- Re-run the failed job:
gh run rerun 37857274288 --failed. If it goes green, this PR is ready to approve.
Claims
| Claim | Verdict | Evidence |
|---|---|---|
The Check (Linux) failure is not from this diff |
CONFIRMED |
Job 113584400615 fails in vision_mixer_input_format_test (gpu_i420: failed to reach PLAYING). The diff's only backend file is backend/tests/cefsrc_test.rs, which skips on that job, and it does not change the check-linux job. Main's Check (Linux) at the same base 0216c1c passed (job 113354643183), so this is a flake. Twelve later test binaries never ran (vision_mixer_overlay_upload_test through whip_whep_ice_transport_policy_test) |
| All three cefsrc tests executed at this head | CONFIRMED |
Test (Linux, cefsrc) job 113584400595: cefsrc_emits_premultiplied_alpha, cefsrc_on_dsk_input_reaches_mixer and cefsrc_flow_restarts_without_leaking all ok. .github/workflows/ci.yml:227 — STROM_REQUIRE_CEF: 1 makes a missing plugin a failure |
| Moving the download into the script keeps main's checksum | CONFIRMED |
The check is quoted below the table. Build strom-full image (job 113584395037) logs /tmp/gstcefsrc.tar.gz: OK |
The image is unchanged apart from cef-env.sh |
UNVERIFIED |
The body says the filesystem and environment diff was not re-run after the rebase. #979's cache logic now lives in the sourced file, and I compared that text by reading it only |
The checksum line: docker/strom-full/install-gstcefsrc.sh:41 — echo "${GSTCEFSRC_SHA256} ${archive}" | sha256sum -c -
Radius: SHARED. No application code changes, but the PR edits the shipped image's startup environment.
Overlaps
- #1056 depends on this job. Its
stinger_web_testneeds adding to the job's--testlist and path filter once this lands, so land this first. - #809 edits
ci.ymlincheck-linux, and its Xvfb step should go away after #977. It does not collide with this PR. - #964 (Linux CI on 1.24.2) is related: this job already runs the production-like questing / 1.26 base. It settles nothing #964 asks.
Tests & CI: Linux builds, WASM, API contract and both cefsrc jobs are green at this head. macOS and Windows skipped, which is right for a Linux-only job. Main has moved four commits since 0216c1c, none of them in docker/ or ci.yml, and a trial merge onto main is clean. I ran nothing locally.
Confidence: HIGH
Opened by Claude (Claude Code) on behalf of @wagenet.
CI has no
cefsrc, so nothing guards behaviour of the element behind HTML sources and DSK graphics. A newTest (Linux, cefsrc)job installs the prebuilt gstcefsrc plugin and runs three new tests against it through Strom flows.The job
It runs on pushes to main, on PRs labelled
ci:cef, on PRs touchingdocker/strom-full/,docker/gstcefsrc/,build-gstcefsrc.ymlor the test file, and onworkflow_dispatch(newcefinput). Main already startspreflighton anyci:label.cef-changesalso runs on aci:label, because that run replaces the one it cancels (seeconcurrency).A second job,
Build strom-full image, buildsdocker/strom-full/Dockerfilefor amd64 on a pull request that changes that directory, pushes nothing, and then runsgst-inspect-1.0 cefsrcthrough the entrypoint, so it also fails ifcef-env.shor the plugin install is broken. It reads the publish workflow's layer cache without writing to it. The base is the publishedeyevinntechnology/strom:latest, not this PR's backend: what it checks is the layers strom-full adds. Before this, the only build was the publish workflow, which runs after merge.The test job runs in an
ubuntu:questingcontainer (the strom-full base), because the plugin is built against questing's glibc and GStreamer 1.26. The tarball is cached withactions/cache, keyed on the version.The image and CI share one copy of each piece. The version is the Dockerfile's
ARG GSTCEFSRC_VERSION.install-gstcefsrc.shdoes the download, extract and symlinks, and the Dockerfile bind-mounts it.cef-env.shholds the Chromium flags, Xvfb, cache directory and mallinfo shim, and the entrypoint sources it. The three pathENVlines are repeated in CI rather than moved, since moving them would change the image for anyone overriding the entrypoint; if they drift, CI fails loudly.The image is unchanged apart from the added
/usr/local/lib/strom/cef-env.sh. Before the rebase onto0216c1c1, I checked this by building the old and new Dockerfile locally (arm64), diffing the filesystems, and diffing the environment after the entrypoint in all three GPU branches (no GPU, and a stubnvidia-smiwithSTROM_CEF_GPU=0and=1).Tests (
backend/tests/cefsrc_test.rs)Without
cefsrcthe tests skip, which is what happens locally and in every other CI job. WithSTROM_REQUIRE_CEF=1, which the new job sets, a missingcefsrcfails them instead. They also skip off Linux, because CEF on macOS needs a main-thread Cocoa run loop the test harness lacks.Flows go through
AppStatestart/stop. Test pages animate a corner pixel, since Chromium paints only on change.cefsrc_flow_restarts_without_leakingruns four start / 5 frames / stop cycles ofcefsrc -> appsink. Each cycle asserts the pipeline and all elements finalize. Cycles 2-4 must return to cycle 1's thread count (slack 2) and CEF subprocess count.cefsrc_on_dsk_input_reaches_mixerwirescefsrc -> Video Format (resolution only) -> vision mixer dsk_in_0(CPU) and asserts the compositor pad linked to the DSK chain receives 10 buffers.It does not guard the
framerate=0/1stall yet. The pinned gstcefsrc (b633408) advertisesframerate=[1/1, 60/1], and the test still passed withframerate=25/1pinned in the format block.0/1arrived 19 commits later inc89c2ce; until the pin passes it, this is a DSK-path smoke test.cefsrc_emits_premultiplied_alpharendersbackground: rgba(255,255,255,0.5)at 320x180 and asserts the centre pixel at cefsrc's output is BGRA ~128,128,128,128 (premultiplied), not 255,255,255,128. The composited on-air value is not asserted: it is 96 on main and feat(vision_mixer): composite premultiplied-alpha DSK inputs correctly #809 changes it, so whichever PR lands second can add that.It waits for the page's animated corner square to show red or blue before sampling, since only the page's own script draws that. Waiting instead on a non-zero centre alpha failed once on this PR's CI, on a frame that read opaque black.
Tests 1 and 3 were not reverse-tested: flipping cefsrc's alpha or leaking a CEF process needs a different cefsrc build. The weak-ref detection in test 1 is the mechanism
pipeline_lifecycle_test.rsalready proves with its negative test.What ran
Rebased on
0216c1c1. Main's #979 rewrote the CEF cache setup this PR moves intocef_setup_cache; the function now carries #979's version (keep the cache, drop only this host's stale lock). The image-equivalence diff above has not been re-run since the rebase.After the rebase, at
407a40eb:cef-env.sh,STROM_REQUIRE_CEF=1):3 passedin 12.7 s.cargo fmt --checkandcargo clippy --workspace --all-targets -D warningson macOS;actionlintandbash -non the workflow and scripts.Before the rebase, at
e68a637:Test (Linux, cefsrc)ran all three tests, none skipped (test result: ok. 3 passed), andBuild strom-full imagebuilt the image and loadedcefsrcthrough the entrypoint (2 min).workflow_dispatchruns of the cefsrc job, all3 passed(1, 2, 3, 4). With the PR's own run that is 5 of 5 since the paint-wait change.167dd1e, before that change,cefsrc_emits_premultiplied_alphafailed once on run 35237502683 withalpha of a 50% CSS fill is 255 (BGRA 0,0,0,255)— an opaque black frame. Three fork runs at that same commit passed, so 1 in 6. Not reproduced locally: the tests skip on macOS and the local Docker daemon is down, so the opaque-black-before-load explanation rests on that one failure message.29364e8.cargo fmt --checkandcargo clippy --workspace --all-targets -D warningson macOS after the rebase; the same on Linux throughCheck (Linux).ci:ceflabel, which needs a maintainer.🤖 Generated with Claude Code