The prior commit (a4700185b) made SongMetadata.Artists/AlbumArtists
coalesce null to [] via backing-field getters, reasoning that EF
Core's PreferField access mode never observes the getter. Adversarial
review disproved this on real TvContext/SQLite: a single read of
.Artists on a TRACKED entity mutates the backing field through the
getter, flips the entity to Modified, and the next SaveChanges writes
[] over what was a NULL column -- silent data loss waiting on the
first tracked reader (today all readers happen to be AsNoTracking).
This also reversed docs/decisions/records/api/selection-projection-include-chain.md
(#671) without the doc update CLAUDE.md requires; #691 is that
record's own "sweep by FIELD" follow-up, so it should follow the
record, not contradict it.
Revert SongMetadata.cs to plain auto-properties (byte-identical to
origin/main, BOM still stripped per the #311 gate). Guard the read
sites instead, per the #671 convention (Optional(...).Flatten(),
matching Playouts/Mapper.cs and MediaItems/Mapper.cs):
- SongVideoGenerator.cs: hoist `artists`/`albumArtists` locals once
near the top of the metadata loop instead of repeating the guard at
each of the six former call sites.
- MediaCollectionRepository.cs (GroupIntoFakeCollections): guard the
two AlbumArtists reads at lines ~1147/~1160 that #691 never named --
dropping the entity-level fix without these would trade one bug for
two.
Verified RED per guard by removing only the Optional(...).Flatten()
clause (not the whole file): the artists local throws
ArgumentNullException at SongVideoGenerator.cs:88, the albumArtists
local at :89 (List.ToList() on a null IList<string> source -- same
loaded-gun shape the review demonstrated, precise exception type is
ArgumentNullException rather than NullReferenceException since the
throw site is Enumerable.ToList's null-source check). Restored both;
existing SongVideoGeneratorTests still pass. Full ErsatzTV.Core.Tests:
685 passed (1 pre-existing skip), ErsatzTV.Tests: 1996 passed (4
pre-existing skips), 0 failures in each. No EF model drift
(`dotnet ef migrations has-pending-model-changes` reports none).
`dotnet format --verify-no-changes` on the three touched files exits
0.
Refs #691
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Re-review of the previous fix commit found that two tests added to close
round-1 findings could not fail. Both verified before fixing:
1. Missing_But_Named_Custom_Playout_Item_Watermark_Should_Not_Fall_Through
gave the channel-level fallback the SAME missing custom path as the
playout-item watermark, so a wrongly-widened guard would have fallen
through to a fallback that also resolved to None -- the assertion held
either way. The fallback is now an independently resolvable ChannelLogo
whose cached file exists, so a fall-through returns it and fails the test.
Added the matching positive control (blank -> falls through and DOES
return that logo), so the pair shows the guard distinguishes blank from
unresolvable instead of both landing on None.
2. Deco_With_One_Valid_And_One_Missing_Watermark... asserted a filtered list
length while the routing claim the decision record cited it for lives in
FFmpegLibraryProcessService.CanUseFFmpegNativeWatermark, which the test
never called. It now calls the real predicate.
Also, three wrong claims of my own:
3. The Resource arm comment said "nothing in the app writes a Resource
watermark to the database". False -- CreateWatermarkHandler and
UpdateWatermarkHandler persist whatever ImageSource the request names, so
a Resource watermark IS creatable through the API, always with
Image = null. That is precisely why the new null guard is load-bearing,
so the comment was arguing for its own removal.
4. "One resolver and no per-caller policy" contradicted the surviving
playout-item blank-Custom fall-through documented a few lines later.
Reworded in both the record and the XML docs: one resolver, and exactly
one piece of per-caller policy which lives in the CALLER.
5. The record's "12 of 18 new tests fail pre-fix" was stale. Re-measured
against the final fixture: 19 of 29. The other 10 pass both ways by
design because they pin preserved behavior, which the record now says
explicitly rather than leaving the gap to be read as weakness.
Removed the vacuous generated-URL test rather than keeping it with an honest
comment -- an empty list trivially contains no URL, so it implied coverage it
never had. Its assertion is folded into the sibling test that has a real
arrangement.
Gates: 2772 tests green across 5 projects, dotnet format exit 0, no BOMs,
decisions-validate OK, live-E2E re-run against this binary (0 changed pixels,
nameplate absent, warning emitted).
refs #510
Two independent reviews (cross-family Codex + cold Opus) both returned
BLOCKED. Findings, all verified against source before acting:
1. Resource arm could throw ArgumentNullException (Codex, Medium). Making the
channel/global Resource arm reachable exposed that CreateWatermarkHandler
and UpdateWatermarkHandler write `Image = null` for EVERY non-Custom
watermark, so an API-created Resource watermark reached
Path.Combine(folder, null). Added the blank/null guard the arm never had.
This was live at the playout-item level too, not just newly-reachable code.
2. "Routing is unaffected" was false (Codex, Low but sharp). The predicate is
unchanged, but CanUseFFmpegNativeWatermark also tests Count == 1, and
dropping an unresolvable watermark shortens the list. 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 -- so it
is documented and pinned by a test rather than claimed away.
3. "Exactly one resolver" over-claimed (Opus, High). True of the selector, not
the application: the song-progress overlay is built as a WatermarkOptions
directly by the streaming and troubleshooting handlers, unchecked, and can
still hand ffmpeg a nonexistent -i. Pre-existing; scoped the claim in the
record and channels.md and filed #653.
4. Undeclared crash->degrade change (Opus, Medium). Channel/global Custom had
no blank-image guard, so a cleared image hit ImageCache's fileName[..2] and
threw out of stream startup. Now declared in the record and tested.
5. Contradictory rule text (Opus, Medium) -- the catalog one-liner said
"always no bug" while the body documents the playout-item fall-through
exception. Qualified; catalog regenerated.
6. History was wrong in both the record and the XML docs: the three precedence
levels did NOT all check every source -- channel/global had no Resource arm
and threw. Corrected.
Tests: 30 in the fixture now (was 18). New coverage for the preserved
blank-Custom fall-through (to channel AND to global), the complement case
(missing-but-named must NOT fall through), null/blank Resource, and the
valid+missing routing case. 17 of 24 failed against the pre-fix resolver
before this round; the fixture stays mutation-sensitive.
Also: hoisted the mock-filesystem Initialize() out of its loop so a
multi-file case cannot silently seed only the last file, and marked the
generated-URL test honestly as redundant-by-construction rather than
claiming independent coverage.
The decision record is now 81 prose lines, over the 60-line ceiling. Declared
as a legitimate decline per docs.corpus-size-signal: the length is the review
findings above, each a distinct fact, not redundancy.
refs #510#652#653
WatermarkSelector resolved watermarks in two places with two policies. The
three precedence levels (playout item, channel, global) existence-checked
every image source and degraded to None; the deco path had its own copy of
the same switch that returned whatever path it computed, unchecked. So one
channel could disagree with itself about whether an on-screen bug rendered,
based only on how the watermark was attached.
#502 deferred this here but scoped it to ChannelLogo. It was never
ChannelLogo-only: the deco path skipped the existence check for Custom and
Resource too. Extract one ResolveWatermark used by all four sites.
Severity is not cosmetic. A dead LOCAL path is not harmlessly skipped --
CanUseFFmpegNativeWatermark hands a single permanent watermark to ffmpeg as
a bare -i argument and excludes only URLs, so the deco path could hand
ffmpeg a nonexistent input file.
The generated-initials nameplate was real: a live-E2E on a real transcoded
frame confirmed it composited via the deco path (/iptv/logos/gen is on
ArtworkController, which has no auth filter, so the container-internal
self-fetch succeeded). The #502-era comment claiming "it has never rendered
here" was wrong, and the new record says so. It is still removed: serving it
means an HTTP fetch inside stream startup, which graphics.channel-logo-caching
(#525) eliminated for logos, and it depends on #1's hardcoded localhost.
Reviving it by caching the image instead is #652.
Measured blast radius on prod: 0 Deco rows, 0 DecoWatermark rows, all 43
channels have logo artwork -- no rendered output changes.
Preserved deliberately: a playout-item Custom watermark with a blank image
still falls THROUGH to the channel/global watermark; unifying resolution must
not change which watermark wins. Routing is untouched.
Strict improvement: the channel and global arms previously threw
NotSupportedException on a Resource watermark; they now resolve it. The
default arm still throws so a new image source fails loudly.
Tests: 18 new cases including a positive control and 8 deco-vs-channel parity
cases. 12 of the 18 fail against the pre-fix resolver, which is what proves
they are load-bearing rather than vacuous.
fixes#510
Adversarial review found a documentation defect, not a code one: both
docs/decisions.md and the WatermarkSelector comment asserted the deco path
was unaffected by this change. That is true of the *resolution* half and
false of the *routing* half. SelectWatermarks puts deco-derived options
into the same list the routing guard filters, so a deco watermark whose
resolved path is a URL is rerouted to the graphics engine too — including
the generated-initials localhost URL, which only the deco path still emits
and which plausibly rendered through ffmpeg before.
That reroute is intended (routing by what the path is beats routing by
provenance, which would drift), so the fix is to say so accurately rather
than to narrow the guard. Also records the accepted per-frame cost
asymmetry the entry previously argued on correctness grounds alone.
The guard is extracted as CanUseFFmpegNativeWatermark so it can be tested
directly — review's highest-value gap was that the half of the fix which
decides whether pixels appear had no automated coverage, only the one-off
live E2E. Nine cases pin it, including the localhost-fallback reroute.
Both deferrals now point at real issues instead of an unverifiable
"tracked separately": #510 (deco vs precedence-level missing-logo policy)
and #511 (remote-fetch hardening — timeout, size cap, redirects, pooling,
caching, SSRF).
Also pins scheme-case insensitivity in the selector.
A channel whose logo is an external URL never rendered a watermark, 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, which was true for the guide
listing and silently false for the bug.
External artwork passes through rather than being downloaded into the
image cache: that is already the convention everywhere else (M3U, XMLTV,
SPA JSON all emit the raw URL), no fetch->SaveArtworkToCache glue exists,
and the render path does not need it — ImageElementBase.LoadImage already
fetches an http(s) path with HttpClient and decodes it for real pixel
dimensions.
A remote-URL watermark is therefore forced onto the graphics engine
instead of the ffmpeg-native shortcut, which would otherwise hand the URL
to ffprobe and ffmpeg as a bare -i argument, putting an unbounded network
fetch inside stream startup.
The three gated precedence levels now share one ChannelLogoWatermarkOptions
helper — the triplicated block is what let the defect exist three times
over. Scope held narrow: the generated-initials localhost fallback (#1)
stays disabled behind an explicit comment and a scope-guard test, and the
deco path keeps its own long-standing unchecked policy.
Verified by live-E2E against a real channel playout with an external-URL
logo: origin/main renders 0 logo pixels and logs the "no longer exists"
warning verbatim; this branch renders the logo in the expected region.
Whitespace-only reformatting in FFmpegLibraryProcessService.cs is the
fix-as-you-touch format gate on pre-existing violations, plus a BOM strip.
fixes#502
Whitespace-only (git diff -w is empty); the #311 format job checks whole
touched files, and these two legacy files carried pre-existing violations
never caught before (no PR had touched them since the gate landed).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Third-round review caught that
Stale_Playlist_Guard_Should_Take_Precedence_Over_An_Otherwise_Valid_Three_Way
describes an impossible case: ThreeWay requires
processLaunched <= playlistExists, which is exactly the negation of the
guard condition, so the guard can never preempt a ThreeWay. What the
test really pins is precedence over the progress branches
(TwoWayLateProgress) — still the ordering that matters.
That is the same "rationale misstates the mechanism" defect the previous
commit existed to fix, landed inside the fix itself. Renaming rather
than leaving a test whose name teaches the next reader something false.
Also broadens the escape-hatch caveat: a stale playlist that slips past
the guard lands as TwoWay more often than ThreeWay, since FFmpeg has
usually not reported progress that early.
Test name and comments only; no logic change.
Re-review of the fix commit returned MERGEABLE with one Medium: the
comment justifying the stale-playlist guard misstated the mechanism, in
three places. It claimed Run "warns about a non-empty transcode folder
but does not delete it" — but StartFFmpegSessionHandler.FolderMustBeEmpty
calls EmptyFolder BEFORE the worker spawns, and Run's finally empties it
again. Verified directly rather than taken on the reviewer's word.
The real residual path is EmptyFolder FAILING: it swallows every
exception into a LogWarning and continues. Say that instead.
On a PR whose entire value is that the numbers mean what they say, a
rationale comment that misstates the mechanism is the same class of
defect the PR exists to prevent, so it does not get to ship as a nit.
Also documents that the guard is best-effort rather than a proof (if the
wipe failed, before-or-after-launch is a scheduling race, so an unlucky
sample can still slip through as an implausibly fast ThreeWay), and adds
the test the reviewer noted was missing: guard precedence over an
otherwise-valid ThreeWay.
Comments and one test only; no logic change.
From the cold adversarial review of the initial diff. No blockers were
found; these address what the numbers MEAN, which is the whole point of
an instrumentation change.
- The buckets span the worker's Run entry, not the startup stopwatch, so
prep overlaps the tail of `setup`. Rather than let the log imply an
invariant it does not satisfy, say "spans runEntry" in the line, spell
it out in the doc comment, and rename the test that had codified the
false `sum == startup` claim.
- Guard `processLaunched > playlistExists` -> Unavailable: a stale
live.m3u8 from a previous session (Run warns about a non-empty
transcode folder but does not delete it) would otherwise yield a
plausible-looking sample whose prep exceeds the measured phase.
- Split the two-way fallback into TwoWay vs TwoWayLateProgress. They are
different stories about the pipeline and discriminating stories is
what this issue is for.
- Document the 100ms playlist-poll quantization (it lands entirely in
firstGop, the smallest bucket) and the first-process-failed case where
ffmpegInit spans a retry.
- Short-circuit the per-line timestamp call; static readonly Unavailable.
- Tests for the new guard, progress-before-launch, and boundary equality
(so tightening >= to > later cannot pass silently).
#350's measurement showed `startup` is 81% of tune-in latency and carries
100% of its variance, while remaining one opaque bucket spanning FFmpeg
spawn -> input open/probe -> encoder init -> first GOP. Two hypotheses
survive that measurement (NFS input open vs VAAPI init under contention)
and they need opposite fixes, so split before optimizing.
Adds `prep` (ErsatzTV-side work before FFmpeg exists) + `ffmpegInit`
(launch -> first `-progress` output) + `firstGop` (-> live.m3u8 exists)
to the existing Information-level cold-start line.
The pipeline runs `-loglevel error -nostats -hide_banner`, so a healthy
FFmpeg writes nothing to stderr; the `-progress` stream is the only
zero-cost milestone available and `ffmpegInit` therefore still lumps
input-open with encoder-init. That limit is documented rather than
papered over, and the split degrades to the two-way form #472 accepts
when no progress arrives before the playlist.
Log-only: no transcode behavior change, no new endpoint or config knob.
fixes#472
Adds one Information-level structured log per HLS tune-in cold-start so the
real driver breakdown can be measured on prod before optimizing the transcode
pipeline (measure-before-optimize). Log-only; no transcode behavior change.
- WaitForPlaylistSegments returns a PlaylistSegmentsResult: Phase A (process
startup -> playlist exists) vs Phase B (segment fill), segments reached,
deadline-expired.
- StartFFmpegSessionHandler emits one summary: total = setup + startup + fill,
plus cleanly-detectable feature flags (subtitle burn-in, hwaccel family).
- ColdStartFeatures: pure, unit-tested args->features helper (14 cases).
Watermark / HDR->SDR / image-subtitle burn-in are deliberately not flagged
(all reduce to overlay= in the args, indistinguishable); the full ffmpeg
arguments remain available at Debug.
Refs #350 (instrumentation slice; optimization deferred pending real data).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* add new fields to database
* update editor
* audio and video normalization settings appear to work
* implement optional color normalization
* fix transcoding tests
* update changelog
* refactor classic and block schedules to use same alternate schedule selector
* handle start year and end year
* add migrations for classic and block schedules
* allow editing block template start and end year
* add tests that include years
* add date range editing to classic (alternate) schedules
* fix running tests locally
* restore media files load; needed for local folder scanners
* update changelog
* feedback
