fix(502): render the on-screen bug for external-URL channel logos #513

2026-07-20T22:59:24+02:00

timothy commented

2026-07-20 22:59:24 +02:00

fixes #502

The defect

A channel whose logo is an external URL never rendered an on-screen bug, even with an ImageSource = ChannelLogo watermark attached. WatermarkSelector resolved the URL correctly and then existence-checked it on the filesystem — File.Exists("https://…") is always false — so all three precedence levels (playout item, channel, global) logged "Channel logo no longer exists" and returned None.

The channel editor advertises the URL as winning over an uploaded logo. That was true for the guide listing and silently false for the bug.

The fix

External artwork passes through; it is not downloaded into the image cache. That is already the convention everywhere else — ChannelPlaylist (M3U), the XMLTV template data and Channels/Mapper (SPA JSON) all emit the raw URL. No fetch→SaveArtworkToCache glue exists anywhere, and the render path doesn't need it: ImageElementBase.LoadImage already fetches an http(s) path with HttpClient and decodes it with ImageSharp for real pixel dimensions.

A remote-URL watermark is therefore forced onto the graphics engine, not the ffmpeg-native shortcut, which would hand the URL to ffprobe and ffmpeg as a bare -i argument — an unbounded network fetch inside stream startup, with dimensions left as a FrameSize(1, 1) placeholder.

The three gated levels now share one ChannelLogoWatermarkOptions helper; the triplicated block is what let the defect exist three times over.

Scope

Two adjacent defects deliberately left alone, both now behind explicit comments, tests and real issue refs:

The generated-initials fallback (localhost URL, #1) is killed by the same File.Exists. Reviving it is deferred in docs/decisions.md, so it stays ignored — now by intent rather than by accident.
The deco path never had the gate, so its resolution is untouched. Aligning its missing-file / no-artwork policy is a behavior change in its own right → #510.
Remote-fetch hardening (timeout, size cap, redirects, pooling, caching, SSRF) → #511.

Verification

Live-E2E on the render path, A/B against a real channel playout with an external-URL logo (magenta PNG served over http, 1920x1080 profile, Scaled/40%-width TopLeft watermark):

	magenta pixels in decoded frame	log
`origin/main`	0 / 129600 sampled	`Channel logo no longer exists at http://127.0.0.1:9911/logo.png and will be ignored`
this branch	36864 / 129600, top-left	renders

The base arm reproduces the issue's warning verbatim, so the test is non-vacuous end-to-end.

Unit: 16 new tests — WatermarkSelectorChannelLogoTests (external URL at all three levels, scheme-case, both preserved local-file regressions, generated-fallback scope guard) and FFmpegNativeWatermarkRoutingTests (9 cases pinning the routing guard, incl. the localhost-fallback reroute). Full suite 3955 passed / 0 failed. Format + BOM gates green locally.

Review

Cold-context adversarial review run pre-push; verdict was BLOCKED on a documentation defect — decisions.md and a code comment claimed the deco path was unaffected, but the routing guard keys off the resolved path alone and so reroutes deco-derived URL watermarks too. Corrected in the second commit rather than narrowing the guard (routing by what the path is beats routing by provenance), along with the review's other findings: the routing guard extracted so it is directly testable, the accepted per-frame cost asymmetry recorded, and both deferrals given real issue numbers.

Docs

docs/decisions.md — new entry + Index line. No API, route or SPA change, so no OpenAPI regen or parity update.

fixes #502 ## The defect A channel whose logo is an **external URL** never rendered an on-screen bug, even with an `ImageSource = ChannelLogo` watermark attached. `WatermarkSelector` resolved the URL correctly and then existence-checked it on the filesystem — `File.Exists("https://…")` is always false — so all three precedence levels (playout item, channel, global) logged *"Channel logo no longer exists"* and returned `None`. The channel editor advertises the URL as winning over an uploaded logo. That was true for the guide listing and silently false for the bug. ## The fix **External artwork passes through; it is not downloaded into the image cache.** That is already the convention everywhere else — `ChannelPlaylist` (M3U), the XMLTV template data and `Channels/Mapper` (SPA JSON) all emit the raw URL. No fetch→`SaveArtworkToCache` glue exists anywhere, and the render path doesn't need it: `ImageElementBase.LoadImage` already fetches an `http(s)` path with `HttpClient` and decodes it with ImageSharp for real pixel dimensions. **A remote-URL watermark is therefore forced onto the graphics engine, not the ffmpeg-native shortcut**, which would hand the URL to `ffprobe` and `ffmpeg` as a bare `-i` argument — an unbounded network fetch inside stream startup, with dimensions left as a `FrameSize(1, 1)` placeholder. The three gated levels now share one `ChannelLogoWatermarkOptions` helper; the triplicated block is what let the defect exist three times over. ## Scope Two adjacent defects deliberately left alone, both now behind explicit comments, tests and real issue refs: - The **generated-initials fallback** (`localhost` URL, #1) is killed by the same `File.Exists`. Reviving it is deferred in `docs/decisions.md`, so it stays ignored — now by intent rather than by accident. - The **deco path** never had the gate, so its *resolution* is untouched. Aligning its missing-file / no-artwork policy is a behavior change in its own right → **#510**. - Remote-fetch hardening (timeout, size cap, redirects, pooling, caching, SSRF) → **#511**. ## Verification **Live-E2E on the render path**, A/B against a real channel playout with an external-URL logo (magenta PNG served over http, 1920x1080 profile, Scaled/40%-width TopLeft watermark): | | magenta pixels in decoded frame | log | |---|---|---| | `origin/main` | **0** / 129600 sampled | `Channel logo no longer exists at http://127.0.0.1:9911/logo.png and will be ignored` | | this branch | **36864** / 129600, top-left | renders | The base arm reproduces the issue's warning verbatim, so the test is non-vacuous end-to-end. **Unit**: 16 new tests — `WatermarkSelectorChannelLogoTests` (external URL at all three levels, scheme-case, both preserved local-file regressions, generated-fallback scope guard) and `FFmpegNativeWatermarkRoutingTests` (9 cases pinning the routing guard, incl. the localhost-fallback reroute). Full suite **3955 passed / 0 failed**. Format + BOM gates green locally. ## Review Cold-context adversarial review run pre-push; verdict was BLOCKED on a **documentation** defect — `decisions.md` and a code comment claimed the deco path was unaffected, but the routing guard keys off the resolved path alone and so reroutes deco-derived URL watermarks too. Corrected in the second commit rather than narrowing the guard (routing by what the path *is* beats routing by provenance), along with the review's other findings: the routing guard extracted so it is directly testable, the accepted per-frame cost asymmetry recorded, and both deferrals given real issue numbers. ## Docs `docs/decisions.md` — new entry + Index line. No API, route or SPA change, so no OpenAPI regen or parity update.

timothy force-pushed fix/502-external-url-watermark from 8798369ea8 to 66448e1abf

2026-07-20 23:11:39 +02:00

Compare

timothy commented

2026-07-20 23:33:26 +02:00

Independent re-review of the fix commit + rebase — clean

Second cold-context adversarial pass, scoped to the code no reviewer had seen: the fix commit 66448e1a and the rebase onto the moved origin/main. (The first pass returned BLOCKED on the deco-scoping documentation defect; this one re-checks that correction rather than re-litigating the original diff.)

What it verified rather than took on trust:

The corrected docs are now true. Traced every claim in the new decisions.md paragraph and WatermarkSelector comment: OptionsForWatermarks really does AddRange deco options into the same list the routing guard filters; the deco branch really is the only surviving emitter of the generated-initials localhost URL; and "only ChannelLogo watermarks can produce a URL" holds because Custom and Resource both resolve through Path.Combine, which cannot yield an http(s) scheme even if a user stuffs a URL into watermark.Image.
The extraction is semantics-preserving. All(p) && All(q) ≡ All(p && q) for pure predicates; the empty-list case is guarded by Count == 1 short-circuiting, not by All — and No_Watermarks_Does_Not_Use_FFmpeg pins exactly that (relax Count == 1 and the assertion flips).
All 9 routing tests are non-vacuous — each targets one conjunct and fails if that conjunct is removed. ChannelWatermarkMode.None routing to the graphics engine pins pre-existing behavior (the old guard also required Permanent), not a new blessing.
Rebase is clean. Diff vs main is exactly the 5 intended files. Main's 264b1751 whitespace normalization survives; nothing from main reverted. The decisions.md resolution is strictly additive — two insertions, main's #508 entry intact and preceding ours — so the append-only job is satisfied. Index anchor matches the heading under slug rules.
Nothing newly broken: InternalsVisibleTo("ErsatzTV.Core.Tests") present, both <see cref>s resolve, and #510/#511 exist with titles matching what the comments attribute to them.

Non-blocking nits recorded and deliberately not fixed: backtick-vs-<c> inconsistency in one XML doc block, and ChannelWatermarkMode.OpacityExpression absent from the non-permanent TestCase set (same code path as Intermittent).

CI green on this head — Build & test (.NET), EF migration integrity, Formatting, decisions.md append-only, API docs in sync, Functional E2E all success; image push correctly skipped for a PR.

Review-verdict: MERGEABLE @ 66448e1a

## Independent re-review of the fix commit + rebase — clean Second cold-context adversarial pass, scoped to the code no reviewer had seen: the fix commit `66448e1a` and the rebase onto the moved `origin/main`. (The first pass returned BLOCKED on the deco-scoping documentation defect; this one re-checks that correction rather than re-litigating the original diff.) What it verified rather than took on trust: - **The corrected docs are now true.** Traced every claim in the new `decisions.md` paragraph and `WatermarkSelector` comment: `OptionsForWatermarks` really does `AddRange` deco options into the same list the routing guard filters; the deco branch really is the only surviving emitter of the generated-initials localhost URL; and "only `ChannelLogo` watermarks can produce a URL" holds because `Custom` and `Resource` both resolve through `Path.Combine`, which cannot yield an http(s) scheme even if a user stuffs a URL into `watermark.Image`. - **The extraction is semantics-preserving.** `All(p) && All(q) ≡ All(p && q)` for pure predicates; the empty-list case is guarded by `Count == 1` short-circuiting, not by `All` — and `No_Watermarks_Does_Not_Use_FFmpeg` pins exactly that (relax `Count == 1` and the assertion flips). - **All 9 routing tests are non-vacuous** — each targets one conjunct and fails if that conjunct is removed. `ChannelWatermarkMode.None` routing to the graphics engine pins pre-existing behavior (the old guard also required `Permanent`), not a new blessing. - **Rebase is clean.** Diff vs main is exactly the 5 intended files. Main's `264b1751` whitespace normalization survives; nothing from main reverted. The `decisions.md` resolution is strictly additive — two insertions, main's `#508` entry intact and preceding ours — so the append-only job is satisfied. Index anchor matches the heading under slug rules. - **Nothing newly broken**: `InternalsVisibleTo("ErsatzTV.Core.Tests")` present, both `<see cref>`s resolve, and #510/#511 exist with titles matching what the comments attribute to them. Non-blocking nits recorded and deliberately not fixed: backtick-vs-`<c>` inconsistency in one XML doc block, and `ChannelWatermarkMode.OpacityExpression` absent from the non-permanent `TestCase` set (same code path as `Intermittent`). CI green on this head — `Build & test (.NET)`, `EF migration integrity`, `Formatting`, `decisions.md append-only`, `API docs in sync`, `Functional E2E` all success; image push correctly skipped for a PR. Review-verdict: MERGEABLE @ 66448e1a

timothy merged commit 64de59376f into main

2026-07-20 23:33:50 +02:00

timothy deleted branch fix/502-external-url-watermark

2026-07-20 23:33:51 +02:00

timothy referenced this issue from a commit

2026-07-20 23:33:51 +02:00

Merge pull request 'fix(502): render the on-screen bug for external-URL channel logos' (#513) from fix/502-external-url-watermark into main

timothy referenced this pull request

2026-07-20 23:34:44 +02:00

External-URL channel logos never render an on-screen bug (File.Exists against a URL in WatermarkSelector) #502

timothy referenced this pull request

2026-07-20 23:36:02 +02:00