fix(510): one watermark resolver for all four attachment points #655
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: timothy/ersatztv#655
Reference in New Issue
Block a user
fixes #510
What
WatermarkSelectorresolved watermarks in two places with two policies. The deco path had its own copy of the image-source switch that returned whatever path it computed, unchecked. The precedence levels checked — though not uniformly: the playout-item level checked all three sources, while channel and global checkedCustom/ChannelLogoand threw forResource(no arm, sodefault:). So one channel could disagree with itself about whether an on-screen bug rendered, based only on how the watermark was attached.Extracted one
ResolveWatermarkused by all four attachment points.Policy chosen (owner's call): no usable logo ⇒ no on-screen bug, everywhere.
Two things #510's own text got wrong
ChannelLogo-only — the deco path skipped the existence check forCustomandResourcetoo. Three arms, not one. Fixing the named arm and leaving two is exactly how this defect reached triplicate before #502.CanUseFFmpegNativeWatermarkhands a single permanent watermark to ffmpeg as a bare-iand excludes only URLs. So the deco path could hand ffmpeg a nonexistent input file. URLs, by contrast, reach the graphics engine where load failures are caught into "overlay disabled".The nameplate: it was real, and it is deliberately removed
A channel with no logo artwork used to get a generated-initials nameplate — only via the deco path. A live-E2E on a real transcoded frame confirmed it composited (
/iptv/logos/genis onArtworkController, which carries no auth filter unlikeIptvController, so the container-internal self-fetch succeeded). The #502-era comment claiming "it has never rendered here" was wrong; the new record says so explicitly.It is still removed: serving it means an HTTP fetch inside stream startup — what
graphics.channel-logo-caching(#525) eliminated for logos — and it depends on #1's hardcodedlocalhost. Reviving it by caching the image is #652.Blast radius measured, not assumed: prod has 0
Decorows, 0DecoWatermarkrows, and all 43 channels have logo artwork. No rendered output changes.Three crash-shaped fixes found along the way
NotSupportedExceptionon aResourcewatermark; they now resolve it.Resourcearm gained a null guard it never had.CreateWatermarkHandler/UpdateWatermarkHandlerwriteImage = nullfor every non-Customwatermark, so an API-createdResourcewatermark reachedPath.Combine(folder, null)→ArgumentNullExceptionat any level.Customhad no blank-image guard, so a cleared image hitImageCache'sfileName[..2]and threw out of stream startup — killing playback, not just the overlay.Deliberately preserved / honestly declared
Customwith a blank image still falls through to channel/global. Unifying resolution must not change which watermark wins. This is the one surviving per-caller policy and it lives in the caller.CanUseFFmpegNativeWatermarkalso testsCount == 1, and dropping an unresolvable watermark shortens the list — so a deco with one valid and one missing permanent watermark now routes ffmpeg-native where it previously routed to the graphics engine. Intended, but observable; pinned by a test that calls the real predicate.WatermarkOptionsdirectly by the streaming and troubleshooting handlers, still unchecked, and can still hit the bare--ihazard. Pre-existing, left alone, tracked as #653.Review
Three rounds, two model families (Codex cross-family + cold Opus), every finding verified against source before acting. Rounds 1 and 2 both returned BLOCKED.
The two findings worth highlighting were confidently-wrong justifications in my own documentation, not broken code: the "exactly one resolver" over-claim and "routing is unaffected". Round 3 then caught that two tests I had added to close round-1 findings could not fail — one whose fall-through fallback was itself unresolvable, one that asserted a list length while the routing claim lived in a function it never called. Both fixed.
Verification
dotnet formatexit 0, no BOMs,decisions-validate: OK.Docs
ffmpeg.watermark-resolution-unified+ regenerated catalog.docs/channels.md→ Watermarks.docs.corpus-size-signala long record that is all distinct findings is a legitimate decline, and the length is the review findings above. Flagging it rather than cutting substance the reviewers asked for.ffmpeg/external-logo-graphics-engine.md(#502) is left unedited even though its forward-looking prose is now outdated, perdocs.decision-lifecycle(records are point-in-time, superseded by newer ones, not continuously edited). The new record carries the correction. Push back if you'd rather I amend it with aDecisions-Edit:trailer.🤖 Generated with Claude Code
Correction to
d189d171's commit messageThat commit fixes a real problem but mis-states its impact, and since a wrong explanation outlives the fix it is worth correcting here rather than leaving it in the history unqualified.
The commit message says:
That is wrong.
scripts/build_decisions_catalog.pyimportsscripts.decisions_liband reads frontmatter through the repo's dependency-free hand parser, not PyYAML — deliberately, perread-path-runs-where-deps-dont-exist. The hand parser does not treat#in a plain scalar as a comment, so it read the full value all along. Verified after the fact:The catalog row was complete both before and after. Regenerating it produced no diff, which is why that commit touches exactly one file.
What the defect actually was: the unquoted
rule:scalar meant the two parsers disagreed — PyYAML truncated at#653), the hand parser did not. Nothing in the repo reads that field with PyYAML today except the test that caught it, so the impact was latent, not active: a trap for any future consumer using a real YAML parser, and a genuine violation of the invarianttest_frontmatter_reader_matches_pyyaml_on_every_real_recordexists to protect. Quoting it is still the right fix; the blast radius was zero rather than "the catalog was wrong".The rest of that commit message stands, including the part I do want on the record: I ran
decisions_validate.pylocally but notscripts/tests, and the validator structurally cannot catch this class — it reads through the hand parser, so a hand-parser-vs-PyYAML divergence is invisible to it by construction. Touching the decisions corpus means running the pytest suite too.Round-4 review found the fall-through tests, while now falsifiable, still did not pin the whole guard. Two gaps, both closed and both verified by running the mutation rather than by asserting the test would catch it: 1. All three fall-through tests used only " ", so narrowing IsNullOrWhiteSpace to `image == " "` would have passed while breaking fall-through for null and "" -- and null is the form the API actually persists. Parameterized over null, "" and " ". 2. Nothing pinned the guard's `ImageSource is Custom` clause. This is the sharper of the two: a ChannelLogo watermark's Image is NORMALLY blank (the API writes Image = null for every non-Custom source), so dropping the discriminator would send every playout-item ChannelLogo watermark down the fall-through path instead of resolving the channel's own logo -- with a fully green suite. Added Blank_Image_ChannelLogo_Playout_Item_Watermark_Should_Win_And_Not_Fall_Through, which distinguishes the two levels by watermark Id so a fall-through is observable even though both resolve to the same cached path. Mutation results (each mutation applied on its own, then reverted): drop `is Custom` from the guard -> 1 failure, and the new test is the ONLY test that catches it IsNullOrWhiteSpace -> == " " -> the null and "" parameterized cases fail Negative control re-measured on the final 32-case fixture: 19 fail against the origin/main resolver. The 13 that pass both ways pin deliberately preserved behavior plus the positive control, which the record now states explicitly along with the mutation table. This round's lesson, recorded in the record: a test's NAME is not evidence it pins what it claims, and a whole-file revert cannot show that a test aimed at a specific clause actually reaches that clause -- only mutating the clause can. Gates: 2661 tests green across 4 projects, 225/225 script tests (the gate I skipped before the last push), decisions-validate OK, format exit 0, no BOMs. refs #510Review-verdict: MERGEABLE @
bc1a37fFive review rounds across two model families (Codex cross-family, cold Opus). Rounds 1, 2, 3 and 4 returned BLOCKED; every finding verified against source before acting. Round 5 clean with no findings at any severity. Two guard mutations applied individually and measured, not merely asserted. 2661 tests green, 225/225 script tests, live-E2E re-run on the shipping binary.