From 9cbe70e4866aa0f77fbbddf51aadbeb89992b0c4 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 20:54:00 +0200 Subject: [PATCH 1/5] fix(510): one watermark resolver for all four attachment points 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 --- .../WatermarkSelectorDecoResolutionTests.cs | 327 ++++++++++++++++++ ErsatzTV.Core/FFmpeg/WatermarkSelector.cs | 275 ++++++--------- docs/channels.md | 21 +- docs/decisions/README.md | 1 + .../ffmpeg/watermark-resolution-unified.md | 63 ++++ 5 files changed, 524 insertions(+), 163 deletions(-) create mode 100644 ErsatzTV.Core.Tests/FFmpeg/WatermarkSelectorDecoResolutionTests.cs create mode 100644 docs/decisions/records/ffmpeg/watermark-resolution-unified.md diff --git a/ErsatzTV.Core.Tests/FFmpeg/WatermarkSelectorDecoResolutionTests.cs b/ErsatzTV.Core.Tests/FFmpeg/WatermarkSelectorDecoResolutionTests.cs new file mode 100644 index 000000000..372a7c58e --- /dev/null +++ b/ErsatzTV.Core.Tests/FFmpeg/WatermarkSelectorDecoResolutionTests.cs @@ -0,0 +1,327 @@ +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Domain.Filler; +using ErsatzTV.Core.Domain.Scheduling; +using ErsatzTV.Core.FFmpeg; +using ErsatzTV.Core.Images; +using ErsatzTV.Core.Interfaces.FFmpeg; +using ErsatzTV.Core.Interfaces.Images; +using Microsoft.Extensions.Logging.Abstractions; +using NSubstitute; +using NUnit.Framework; +using Shouldly; +using Testably.Abstractions.Testing; + +namespace ErsatzTV.Core.Tests.FFmpeg; + +/// +/// Pins ersatztv#510: a watermark attached through a DECO resolves by exactly the same policy as the three +/// precedence levels (playout item, channel, global). +/// +/// +/// Before #510 the deco path had its own copy of the image-source switch that resolved paths *unchecked*, so +/// one channel could disagree with itself about whether a bug rendered purely by how the watermark was +/// attached. The divergence covered all three values, not just +/// ChannelLogo: +/// +/// a missing local file was handed downstream as a dead path (and a dead LOCAL path can reach +/// ffmpeg as a bare -i argument via CanUseFFmpegNativeWatermark, so it is worse than a +/// skipped overlay); +/// an un-migrated external-URL logo was handed down as a renderable URL, which +/// graphics.channel-logo-caching (#525) forbids the render path from fetching; +/// a channel with no logo artwork got the generated-initials localhost URL, which a live-E2E on a +/// real transcoded frame confirmed DID render — the deco path only. #510 resolved that split in favour +/// of "no on-screen bug" everywhere. +/// +/// The Deco_And_Channel_Level_Should_Resolve_Identically cases are the structural guard: they assert +/// the two callers agree, so re-introducing a per-caller policy fails here rather than silently in prod. +/// +[TestFixture] +public class WatermarkSelectorDecoResolutionTests +{ + private const string ExternalLogoUrl = "https://cdn.example.com/logos/channel.png"; + private const string LogoStoredPath = "abc123.png"; + private const string LogoCachePath = "/cache/logos/ab/abc123.png"; + private const string CustomStoredPath = "def456.png"; + private const string CustomCachePath = "/cache/watermarks/de/def456.png"; + private const string ResourceImage = "song-progress.png"; + + private static string ResourcePath => Path.Combine(FileSystemLayout.ResourcesCacheFolder, ResourceImage); + + /// Builds a selector whose mock filesystem contains exactly . + private static WatermarkSelector Selector(Deco playoutDeco, params string[] existingFiles) + { + var mockFileSystem = new MockFileSystem(); + foreach (string file in existingFiles) + { + mockFileSystem.Initialize().WithFile(file); + } + + var fakeImageCache = Substitute.For(); + fakeImageCache.GetPathForImage(Arg.Any(), Arg.Is(ArtworkKind.Logo), Arg.Any>()) + .Returns(_ => LogoCachePath); + fakeImageCache.GetPathForImage(Arg.Any(), Arg.Is(ArtworkKind.Watermark), Arg.Any>()) + .Returns(_ => CustomCachePath); + + var decoSelector = Substitute.For(); + decoSelector.GetDecoEntries(Arg.Any(), Arg.Any()) + .Returns(new DecoEntries(Option.None, Optional(playoutDeco))); + + return new WatermarkSelector( + mockFileSystem, + fakeImageCache, + decoSelector, + NullLogger.Instance); + } + + private static ChannelWatermark Watermark(ChannelWatermarkImageSource source, string image = "") => + new() + { + Id = 7, + Name = "Deco Bug", + ImageSource = source, + Image = image, + Mode = ChannelWatermarkMode.Permanent + }; + + private static Deco DecoWith(ChannelWatermark watermark) => + new() + { + Id = 1, + Name = "Test Deco", + WatermarkMode = DecoMode.Override, + UseWatermarkDuringFiller = true, + DecoWatermarks = [new DecoWatermark { WatermarkId = watermark.Id, Watermark = watermark }], + Watermarks = [] + }; + + private static Channel ChannelWith(string logoPath, ChannelWatermark channelWatermark = null) + { + var channel = new Channel(Guid.Empty) + { + Id = 1, + Number = "1", + Name = "Test", + StreamingMode = StreamingMode.TransportStream, + Artwork = [], + Watermark = channelWatermark, + WatermarkId = channelWatermark?.Id + }; + + if (logoPath is not null) + { + channel.Artwork.Add(new Artwork { ArtworkKind = ArtworkKind.Logo, Path = logoPath }); + } + + return channel; + } + + private static PlayoutItem PlayoutItem() => + new() + { + FillerKind = FillerKind.None, + DisableWatermarks = false, + Watermarks = [], + Playout = new Playout() + }; + + private static List SelectViaDeco( + ChannelWatermark watermark, + Channel channel, + params string[] existingFiles) + { + WatermarkSelector selector = Selector(DecoWith(watermark), existingFiles); + return selector.SelectWatermarks( + Option.None, + channel, + PlayoutItem(), + DateTimeOffset.Now); + } + + // ---- positive control: the arrangement CAN produce a watermark ------------------------------ + // + // Without this, every "resolves to nothing" assertion below could pass vacuously (a broken deco + // arrangement that never reaches the resolver at all looks identical to a correct refusal). + + [Test] + public void Deco_ChannelLogo_Should_Use_Cached_Path_When_Local_Logo_Exists() + { + ChannelWatermark watermark = Watermark(ChannelWatermarkImageSource.ChannelLogo); + Channel channel = ChannelWith(LogoStoredPath); + + List result = SelectViaDeco(watermark, channel, LogoCachePath); + + result.Count.ShouldBe(1); + result[0].ImagePath.ShouldBe(LogoCachePath); + } + + // ---- ChannelLogo: the three cases #510 was filed for ---------------------------------------- + + [Test] + public void Deco_ChannelLogo_Should_Be_Ignored_When_Logo_Is_An_External_Url() + { + ChannelWatermark watermark = Watermark(ChannelWatermarkImageSource.ChannelLogo); + Channel channel = ChannelWith(ExternalLogoUrl); + + List result = SelectViaDeco(watermark, channel); + + result.ShouldBeEmpty(); + } + + [Test] + public void Deco_ChannelLogo_Should_Be_Ignored_When_Local_Logo_File_Is_Missing() + { + ChannelWatermark watermark = Watermark(ChannelWatermarkImageSource.ChannelLogo); + Channel channel = ChannelWith(LogoStoredPath); + + // nothing on disk + List result = SelectViaDeco(watermark, channel); + + result.ShouldBeEmpty(); + } + + /// + /// The #510 policy decision: with no logo artwork the generated-initials fallback is NOT used. It + /// genuinely rendered here before (confirmed by live-E2E on a real frame), so this is a deliberate, + /// recorded behavior change — not a no-op cleanup. + /// + [Test] + public void Deco_ChannelLogo_Should_Be_Ignored_When_Channel_Has_No_Logo_Artwork() + { + ChannelWatermark watermark = Watermark(ChannelWatermarkImageSource.ChannelLogo); + Channel channel = ChannelWith(null); + + List result = SelectViaDeco(watermark, channel); + + result.ShouldBeEmpty(); + } + + /// + /// Explicitly pins that the generated-initials URL is never handed downstream as a renderable path, + /// rather than only that the result is empty — an assertion that survives a future refactor which + /// starts returning options again. + /// + [Test] + public void Deco_ChannelLogo_Should_Never_Return_The_Generated_Initials_Url() + { + ChannelWatermark watermark = Watermark(ChannelWatermarkImageSource.ChannelLogo); + Channel channel = ChannelWith(null); + string generatedUrl = ChannelLogoGenerator.GenerateChannelLogoUrl(channel); + + List result = SelectViaDeco(watermark, channel); + + result.Select(o => o.ImagePath).ShouldNotContain(generatedUrl); + } + + // ---- Custom and Resource: the two arms #510 did not mention but that diverged too ----------- + + [Test] + public void Deco_Custom_Should_Use_Cached_Path_When_File_Exists() + { + ChannelWatermark watermark = Watermark(ChannelWatermarkImageSource.Custom, CustomStoredPath); + Channel channel = ChannelWith(LogoStoredPath); + + List result = SelectViaDeco(watermark, channel, CustomCachePath); + + result.Count.ShouldBe(1); + result[0].ImagePath.ShouldBe(CustomCachePath); + } + + [Test] + public void Deco_Custom_Should_Be_Ignored_When_File_Is_Missing() + { + ChannelWatermark watermark = Watermark(ChannelWatermarkImageSource.Custom, CustomStoredPath); + Channel channel = ChannelWith(LogoStoredPath); + + List result = SelectViaDeco(watermark, channel); + + result.ShouldBeEmpty(); + } + + [Test] + public void Deco_Custom_Should_Be_Ignored_When_Image_Is_Blank() + { + ChannelWatermark watermark = Watermark(ChannelWatermarkImageSource.Custom, " "); + Channel channel = ChannelWith(LogoStoredPath); + + List result = SelectViaDeco(watermark, channel, CustomCachePath); + + result.ShouldBeEmpty(); + } + + [Test] + public void Deco_Resource_Should_Use_Resource_Path_When_File_Exists() + { + ChannelWatermark watermark = Watermark(ChannelWatermarkImageSource.Resource, ResourceImage); + Channel channel = ChannelWith(LogoStoredPath); + + List result = SelectViaDeco(watermark, channel, ResourcePath); + + result.Count.ShouldBe(1); + result[0].ImagePath.ShouldBe(ResourcePath); + } + + [Test] + public void Deco_Resource_Should_Be_Ignored_When_File_Is_Missing() + { + ChannelWatermark watermark = Watermark(ChannelWatermarkImageSource.Resource, ResourceImage); + Channel channel = ChannelWith(LogoStoredPath); + + List result = SelectViaDeco(watermark, channel); + + result.ShouldBeEmpty(); + } + + // ---- the structural guard: deco and channel-level must agree, case for case ------------------ + + private static IEnumerable ParityCases() + { + yield return new TestCaseData(ChannelWatermarkImageSource.ChannelLogo, LogoStoredPath, new[] { LogoCachePath }) + .SetName("ChannelLogo, local file present"); + yield return new TestCaseData(ChannelWatermarkImageSource.ChannelLogo, LogoStoredPath, Array.Empty()) + .SetName("ChannelLogo, local file missing"); + yield return new TestCaseData(ChannelWatermarkImageSource.ChannelLogo, ExternalLogoUrl, Array.Empty()) + .SetName("ChannelLogo, external URL"); + yield return new TestCaseData(ChannelWatermarkImageSource.ChannelLogo, null, Array.Empty()) + .SetName("ChannelLogo, no logo artwork"); + yield return new TestCaseData(ChannelWatermarkImageSource.Custom, LogoStoredPath, new[] { CustomCachePath }) + .SetName("Custom, file present"); + yield return new TestCaseData(ChannelWatermarkImageSource.Custom, LogoStoredPath, Array.Empty()) + .SetName("Custom, file missing"); + yield return new TestCaseData(ChannelWatermarkImageSource.Resource, LogoStoredPath, new[] { ResourcePath }) + .SetName("Resource, file present"); + yield return new TestCaseData(ChannelWatermarkImageSource.Resource, LogoStoredPath, Array.Empty()) + .SetName("Resource, file missing"); + } + + [TestCaseSource(nameof(ParityCases))] + public void Deco_And_Channel_Level_Should_Resolve_Identically( + ChannelWatermarkImageSource source, + string logoPath, + string[] existingFiles) + { + string image = source switch + { + ChannelWatermarkImageSource.Custom => CustomStoredPath, + ChannelWatermarkImageSource.Resource => ResourceImage, + _ => "" + }; + + // deco path + ChannelWatermark decoWatermark = Watermark(source, image); + List viaDeco = SelectViaDeco(decoWatermark, ChannelWith(logoPath), existingFiles); + + // channel precedence level, same watermark definition and same channel + ChannelWatermark channelWatermark = Watermark(source, image); + Channel channel = ChannelWith(logoPath, channelWatermark); + Option viaChannel = Selector(null, existingFiles) + .GetWatermarkOptions(channel, Option.None, Option.None); + + List decoPaths = viaDeco.Select(o => o.ImagePath).ToList(); + + // built explicitly rather than via Option.ToList(), which yields a LanguageExt Lst + var channelPaths = new List(); + viaChannel.IfSome(o => channelPaths.Add(o.ImagePath)); + + decoPaths.ShouldBe(channelPaths); + } +} diff --git a/ErsatzTV.Core/FFmpeg/WatermarkSelector.cs b/ErsatzTV.Core/FFmpeg/WatermarkSelector.cs index 9db8a1c02..5e6e070d4 100644 --- a/ErsatzTV.Core/FFmpeg/WatermarkSelector.cs +++ b/ErsatzTV.Core/FFmpeg/WatermarkSelector.cs @@ -171,125 +171,111 @@ public class WatermarkSelector( // check for playout item watermark foreach (ChannelWatermark watermark in playoutItemWatermark) { - switch (watermark.ImageSource) + // A custom watermark with no image at all is a bad-form-validation artifact, and it has always + // fallen THROUGH to the channel/global watermark rather than resolving to "no watermark". That + // stays true: unifying *resolution* (#510) must not change which watermark WINS. + if (watermark.ImageSource is ChannelWatermarkImageSource.Custom + && string.IsNullOrWhiteSpace(watermark.Image)) { - // used for song progress overlay - case ChannelWatermarkImageSource.Resource: - string resourcePath = fileSystem.Path.Combine( - FileSystemLayout.ResourcesCacheFolder, - watermark.Image); - if (fileSystem.File.Exists(resourcePath)) - { - return new WatermarkOptions(watermark, resourcePath, Option.None); - } - - logger.LogWarning( - "Watermark resource no longer exists at {Path} and will be ignored", - resourcePath); - return None; - case ChannelWatermarkImageSource.Custom: - // bad form validation makes this possible - if (string.IsNullOrWhiteSpace(watermark.Image)) - { - logger.LogWarning( - "Watermark {Name} has custom image configured with no image; ignoring", - watermark.Name); - break; - } - - logger.LogDebug("Watermark will come from playout item (custom)"); - - string customPath = imageCache.GetPathForImage( - watermark.Image, - ArtworkKind.Watermark, - Option.None); - - if (fileSystem.File.Exists(customPath)) - { - return new WatermarkOptions(watermark, customPath, None); - } - - logger.LogWarning( - "Custom watermark no longer exists at {Path} and will be ignored", - customPath); - return None; - case ChannelWatermarkImageSource.ChannelLogo: - logger.LogDebug("Watermark will come from playout item (channel logo)"); - - return ChannelLogoWatermarkOptions(channel, watermark); - default: - throw new NotSupportedException("Unsupported watermark image source"); + logger.LogWarning( + "Watermark {Name} has custom image configured with no image; ignoring", + watermark.Name); + break; } + + logger.LogDebug("Watermark will come from playout item ({ImageSource})", watermark.ImageSource); + return ResolveWatermark(channel, watermark); } // check for channel watermark if (channel.Watermark != null) { - switch (channel.Watermark.ImageSource) - { - case ChannelWatermarkImageSource.Custom: - logger.LogDebug("Watermark will come from channel (custom)"); - - string customPath = imageCache.GetPathForImage( - channel.Watermark.Image, - ArtworkKind.Watermark, - Option.None); - - if (fileSystem.File.Exists(customPath)) - { - return new WatermarkOptions(channel.Watermark, customPath, None); - } - - logger.LogWarning( - "Custom watermark no longer exists at {Path} and will be ignored", - customPath); - return None; - case ChannelWatermarkImageSource.ChannelLogo: - logger.LogDebug("Watermark will come from channel (channel logo)"); - - return ChannelLogoWatermarkOptions(channel, channel.Watermark); - default: - throw new NotSupportedException("Unsupported watermark image source"); - } + logger.LogDebug("Watermark will come from channel ({ImageSource})", channel.Watermark.ImageSource); + return ResolveWatermark(channel, channel.Watermark); } // check for global watermark foreach (ChannelWatermark watermark in globalWatermark) { - switch (watermark.ImageSource) - { - case ChannelWatermarkImageSource.Custom: - logger.LogDebug("Watermark will come from global (custom)"); - - string customPath = imageCache.GetPathForImage( - watermark.Image, - ArtworkKind.Watermark, - Option.None); - - if (fileSystem.File.Exists(customPath)) - { - return new WatermarkOptions(watermark, customPath, None); - } - - logger.LogWarning( - "Custom watermark no longer exists at {Path} and will be ignored", - customPath); - return None; - case ChannelWatermarkImageSource.ChannelLogo: - logger.LogDebug("Watermark will come from global (channel logo)"); - - return ChannelLogoWatermarkOptions(channel, watermark); - default: - throw new NotSupportedException("Unsupported watermark image source"); - } + logger.LogDebug("Watermark will come from global ({ImageSource})", watermark.ImageSource); + return ResolveWatermark(channel, watermark); } return Option.None; } /// - /// Resolves a watermark to a renderable path, - /// shared by the playout-item, channel and global precedence levels so all three agree. + /// The single place a becomes a renderable image path, shared by every + /// watermark source: the three precedence levels (playout item, channel, global) AND the deco path. + /// + /// + /// Before #510 the deco path had its own copy of this switch that resolved paths *unchecked* — it handed + /// down a nonexistent file, an un-migrated external URL, and the generated-initials localhost URL, while + /// the three precedence levels degraded all three to "no watermark". So the same channel could disagree + /// with itself about whether a bug rendered, purely by how the watermark was attached. Duplication is what + /// let that drift happen (it existed in triplicate before #502), so there is now exactly one resolver and + /// no per-caller policy. An unresolvable watermark is always "no on-screen bug", never a dead path passed + /// downstream: a dead LOCAL path could reach ffmpeg as a bare -i argument via + /// CanUseFFmpegNativeWatermark, which is materially worse than a skipped overlay. + /// + private Option ResolveWatermark(Channel channel, ChannelWatermark watermark) + { + switch (watermark.ImageSource) + { + // used for song progress overlay + case ChannelWatermarkImageSource.Resource: + string resourcePath = fileSystem.Path.Combine( + FileSystemLayout.ResourcesCacheFolder, + watermark.Image); + if (fileSystem.File.Exists(resourcePath)) + { + return new WatermarkOptions(watermark, resourcePath, Option.None); + } + + logger.LogWarning( + "Watermark resource no longer exists at {Path} and will be ignored", + resourcePath); + return None; + + case ChannelWatermarkImageSource.Custom: + // bad form validation makes this possible + if (string.IsNullOrWhiteSpace(watermark.Image)) + { + logger.LogWarning( + "Watermark {Name} has custom image configured with no image; ignoring", + watermark.Name); + return None; + } + + string customPath = imageCache.GetPathForImage( + watermark.Image, + ArtworkKind.Watermark, + Option.None); + + if (fileSystem.File.Exists(customPath)) + { + return new WatermarkOptions(watermark, customPath, None); + } + + logger.LogWarning( + "Custom watermark no longer exists at {Path} and will be ignored", + customPath); + return None; + + case ChannelWatermarkImageSource.ChannelLogo: + return ChannelLogoWatermarkOptions(channel, watermark); + + // deliberately loud: a newly-added image source must fail visibly rather than silently + // resolve to some neighbouring source's behavior + default: + throw new NotSupportedException("Unsupported watermark image source"); + } + } + + /// + /// Resolves a watermark to a renderable path. + /// Since #510 this is reached from , so all FOUR sources — the playout-item, + /// channel and global precedence levels AND the deco path — agree. /// /// /// As of #525 an external-URL logo is downloaded and cached at save time, so a URL path here can only @@ -324,79 +310,46 @@ public class WatermarkSelector( return None; } - // with no logo artwork the only candidate is the generated-initials image, whose URL hardcodes - // localhost (ChannelLogoGenerator.GenerateChannelLogoUrl, issue #1). It has never rendered here and - // reviving it is deliberately deferred in docs/decisions.md, so it stays ignored. + // With no logo artwork at all the only candidate is the generated-initials image, served over HTTP from + // ChannelLogoGenerator.GenerateChannelLogoUrl -- a URL that hardcodes localhost (issue #1, closed as a + // topology problem without removing the hardcode). + // + // Until #510 that URL WAS returned by the deco path, and it genuinely rendered: a live-E2E on a real + // transcoded frame confirmed the nameplate compositing through the graphics engine (the /iptv/logos/gen + // route sits on ArtworkController, which carries no auth filter, so the container-internal self-fetch + // succeeded). It never rendered at the three precedence levels. #510 resolved that split in favour of + // "no bug", because a render-time HTTP fetch inside stream startup is exactly what `graphics.channel-logo-caching` + // (#525) eliminated for logos -- so the fallback is now off everywhere rather than on for one caller. + // Reviving it properly means generating the image into the image cache so it resolves to a LOCAL path; + // that is deliberately out of scope here and tracked separately. logger.LogWarning( - "Channel logo no longer exists at {Path} and will be ignored", + "Channel {Channel} has no logo artwork; rendering without an on-screen bug. The generated-initials " + + "fallback ({Url}) is deliberately not used by the render path", + channel.Number, ChannelLogoGenerator.GenerateChannelLogoUrl(channel)); return None; } + /// + /// Resolves the watermarks attached to a deco. Since #510 this shares + /// with the three precedence levels rather than carrying its own unchecked copy of the same switch. + /// + /// + /// Resolution is now identical to the precedence levels; what stays deco-specific is only WHICH + /// watermarks apply and whether they merge with or override the rest (handled in + /// ). Routing is unaffected and remains keyed off the resolved path + /// alone — FFmpegLibraryProcessService.CanUseFFmpegNativeWatermark sends any URL to the + /// graphics engine regardless of provenance. + /// private List OptionsForWatermarks(Channel channel, IEnumerable watermarks) { var result = new List(); foreach (var watermark in watermarks) { - result.AddRange(GetWatermarkOptions(channel, watermark)); + result.AddRange(ResolveWatermark(channel, watermark)); } return result; } - - private Option GetWatermarkOptions(Channel channel, ChannelWatermark watermark) - { - switch (watermark.ImageSource) - { - // used for song progress overlay - case ChannelWatermarkImageSource.Resource: - return new WatermarkOptions( - watermark, - Path.Combine(FileSystemLayout.ResourcesCacheFolder, watermark.Image), - Option.None); - case ChannelWatermarkImageSource.Custom: - // bad form validation makes this possible - if (string.IsNullOrWhiteSpace(watermark.Image)) - { - logger.LogWarning( - "Watermark {Name} has custom image configured with no image; ignoring", - watermark.Name); - break; - } - - string customPath = imageCache.GetPathForImage( - watermark.Image, - ArtworkKind.Watermark, - Option.None); - return new WatermarkOptions( - watermark, - customPath, - None); - case ChannelWatermarkImageSource.ChannelLogo: - // deliberately NOT ChannelLogoWatermarkOptions: the deco path has always passed its resolved - // path through unchecked, so #502's File.Exists defect never reached it and its *resolution* - // is unchanged here. Aligning its missing-file / no-artwork policy with the three precedence - // levels above is a behavior change beyond this fix — tracked in #510. - // Note this only scopes resolution: the ffmpeg-native-vs-graphics-engine routing in - // FFmpegLibraryProcessService.CanUseFFmpegNativeWatermark keys off the resolved path alone, so a - // deco watermark resolving to a URL (an external logo, or the generated-initials URL below) is - // rerouted to the graphics engine like any other. That is intended: it is the URL-aware path. - string channelPath = ChannelLogoGenerator.GenerateChannelLogoUrl(channel); - Option maybeLogoArtwork = - Optional(channel.Artwork.Find(a => a.ArtworkKind == ArtworkKind.Logo)); - foreach (var logoArtwork in maybeLogoArtwork) - { - channelPath = Artwork.IsExternalUrl(logoArtwork.Path) - ? logoArtwork.Path - : imageCache.GetPathForImage(logoArtwork.Path, ArtworkKind.Logo, Option.None); - } - - return new WatermarkOptions(watermark, channelPath, None); - default: - throw new NotSupportedException("Unsupported watermark image source"); - } - - return Option.None; - } } diff --git a/docs/channels.md b/docs/channels.md index f5c813b81..137e92315 100644 --- a/docs/channels.md +++ b/docs/channels.md @@ -201,8 +201,25 @@ with no bug until you re-save it. Historical context (the old `File.Exists`-on-a bounded render-time fetch that preceded caching) is in `docs/decisions.md` under `graphics.channel-logo-caching`, #502 and #511. -Note: `ChannelLogoGenerator.GenerateChannelLogoUrl()` hardcodes `localhost` for watermark logo -fetching — see issue #1 for details. +**No usable logo means no on-screen bug — from every attachment point (#510).** A `ChannelLogo` +watermark resolves through one shared resolver (`WatermarkSelector.ResolveWatermark`) whether it is +attached via a playout item, the channel, the global setting, **or a deco**. All four agree: an +un-migrated external URL, a missing cached file, and a channel with no logo artwork each render +*without* a bug and log a warning. Nothing hands a dead path or a URL downstream — a dead local path +could otherwise reach ffmpeg as a bare `-i` argument and break the stream, which is worse than a +skipped overlay. + +Before #510 the deco path had its own unchecked copy of that resolution, so the same channel could +disagree with itself about whether a bug rendered based only on how the watermark was attached. The +divergence covered `Custom` and `Resource` image sources too, not just `ChannelLogo`. + +That change switched off one thing that *did* work: a channel with **no** logo artwork used to get a +generated-initials nameplate (`/iptv/logos/gen`, drawn by `ChannelLogoGenerator`) when — and only +when — the watermark came from a deco. It is now off everywhere, because serving it means an HTTP +fetch inside stream startup, exactly what `graphics.channel-logo-caching` (#525) removed for logos, +and because `ChannelLogoGenerator.GenerateChannelLogoUrl()` hardcodes `localhost` (issue #1, closed +as a topology problem without removing the hardcode). Reviving it properly means generating the image +into the image cache so it resolves to a local path; see `ffmpeg.watermark-resolution-unified`. ## On Now / Next overlay (#74) diff --git a/docs/decisions/README.md b/docs/decisions/README.md index e7cb319a6..e87cabc21 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -75,6 +75,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ffmpeg.qsv-decode-encode-split` | QSV decode is decoupled from QSV encode via a single `FFmpegProfile.QsvPreferNativeDecoder` bool (default ON, Linux-only), so a QSV encode profile can decode with the more tolerant native VA-API decoder instead of the QSV decoder, mirroring Jellyfin's hybrid decode/encode toggle instead of a general decode-family enum. | 2026-07-20 | [link](records/ffmpeg/qsv-decode-encode-split.md) | | `ffmpeg.qsv-extra-hw-frames-floor` | a QSV upload never emits `extra_hw_frames` below `FFmpegState.MinimumQsvExtraHardwareFrames` (64); a stored `0` or negative value is treated as "no pool configured" rather than honored literally, because with no headroom any unthrottled read exhausts the pool and the transcode writes nothing at all. | 2026-07-21 | [link](records/ffmpeg/qsv-extra-hw-frames-floor.md) | | `ffmpeg.remote-image-fetcher-bounded` | remote graphics-engine images are fetched through `IRemoteImageFetcher` with a pooled `HttpClientFactory` client, a body-covering deadline, a wire-transfer size cap, and a decoder-enforced `DecoderOptions.MaxFrames` bound re-verified post-decode — never cached, re-fetched per element init. | 2026-07-20 | [link](records/ffmpeg/remote-image-fetcher-bounded.md) | +| `ffmpeg.watermark-resolution-unified` | Every `ChannelWatermark` becomes a renderable path through one shared `WatermarkSelector.ResolveWatermark`, used by the playout-item, channel and global precedence levels AND the deco path, for all three `ChannelWatermarkImageSource` values. An unresolvable watermark (missing file, un-migrated external URL, or no logo artwork) always means no on-screen bug plus a warning — never a dead path or a URL handed downstream. The generated-initials fallback is therefore off everywhere, including the deco path where it demonstrably rendered. | 2026-07-26 | [link](records/ffmpeg/watermark-resolution-unified.md) | | `ffmpeg.work-ahead-slot-atomic` | `workAheadSegmenterLimit` is enforced by a single compare-exchange claim on a shared `WorkAheadSlots` pool taken by the *caller* of `Transcode`, which then passes ownership in and gets the release in `Transcode`'s `finally` — never a `Volatile.Read` compare in one place and an `Interlocked.Increment` in another. | 2026-07-21 | [link](records/ffmpeg/work-ahead-slot-atomic.md) | | `ffmpeg.work-ahead-slot-release-never-negative` | `Release()` reads the count and compare-exchanges `current - 1` only when `current > 0`; a release against an empty pool records an unbalanced release and returns `false` **without ever writing a negative value**. It never decrements first and clamps afterward. The single caller (`HlsSessionWorker.Transcode`'s `finally`) logs a warning on the `false` return. | 2026-07-21 | [link](records/ffmpeg/work-ahead-slot-release-never-negative.md) | | `graphics.channel-level-attachment` | A channel can attach `GraphicsElement`s directly via a new `ChannelGraphicsElement` join table (a base layer under deco/playout-item elements), and a built-in text element (`on-now-next.yml`) is seeded once per database so the On Now/Next overlay works out of the box. | 2026-07-22 | [link](records/graphics/channel-level-attachment.md) | diff --git a/docs/decisions/records/ffmpeg/watermark-resolution-unified.md b/docs/decisions/records/ffmpeg/watermark-resolution-unified.md new file mode 100644 index 000000000..4dc03a0cd --- /dev/null +++ b/docs/decisions/records/ffmpeg/watermark-resolution-unified.md @@ -0,0 +1,63 @@ +--- +key: ffmpeg.watermark-resolution-unified +title: 2026-07-26 — One watermark resolver for all four attachment points; no usable logo means no bug (#510) +status: active +since: '2026-07-26' +supersedes: none +superseded-by: none +rule: Every `ChannelWatermark` becomes a renderable path through one shared `WatermarkSelector.ResolveWatermark`, used by the playout-item, channel and global precedence levels AND the deco path, for all three `ChannelWatermarkImageSource` values. An unresolvable watermark (missing file, un-migrated external URL, or no logo artwork) always means no on-screen bug plus a warning — never a dead path or a URL handed downstream. The generated-initials fallback is therefore off everywhere, including the deco path where it demonstrably rendered. +signals: 'deco watermark, missing logo policy, generated initials, nameplate bug, watermark resolution divergence · paths: `ErsatzTV.Core/FFmpeg/WatermarkSelector.cs` (`ResolveWatermark`, `ChannelLogoWatermarkOptions`, `OptionsForWatermarks`), `ChannelLogoGenerator.GenerateChannelLogoUrl` · issues: #510, #502, #525, #1, #67' +mechanics: '`WatermarkSelectorDecoResolutionTests` (incl. the `Deco_And_Channel_Level_Should_Resolve_Identically` parity cases); `docs/channels.md` → Watermarks' +--- + +`WatermarkSelector` resolved a watermark in two places with two policies. The three precedence levels +existence-checked every 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 a bug rendered, based only on how the watermark was attached. + +#502 had noted the split and deferred it here. What #502's note got wrong is scope: the divergence was +never `ChannelLogo`-only. The deco path skipped the existence check for **`Custom` and `Resource` +too** — three arms, not one. Fixing the named arm and leaving two is how this defect reached +triplicate in the first place, so there is now exactly **one** resolver and no per-caller policy. + +**Severity is not merely cosmetic, which is what settled the direction.** A dead *local* path is not +harmlessly skipped: `CanUseFFmpegNativeWatermark` routes a single permanent watermark to ffmpeg as a +bare `-i` argument, and only *URLs* are excluded from that shortcut. So the deco path could hand +ffmpeg a nonexistent input file. A URL, by contrast, reaches the graphics engine, whose fetch failures +are caught into "overlay disabled". Unchecked resolution was the more dangerous of the two policies. + +**The generated-initials fallback was real, and is deliberately switched off.** With no logo artwork +the deco path returned `ChannelLogoGenerator.GenerateChannelLogoUrl` — a `localhost` URL. A live-E2E +on a real transcoded frame (deco in `Override` mode, `ChannelLogo` watermark, channel with no logo +artwork) confirmed the nameplate **did** composite: `/iptv/logos/gen` sits on `ArtworkController`, +which carries no auth filter (unlike `IptvController`), so the container-internal self-fetch +succeeded. This is recorded because the #502-era comment asserted the opposite ("it has never +rendered here") and a future reader would otherwise re-inherit that error. + +It is still removed, because keeping it means an HTTP fetch inside stream startup per element init — +precisely what `graphics.channel-logo-caching` (#525) eliminated for logos — and it depends on #1's +hardcoded `localhost`, which #1 closed as a topology problem without removing. Keeping it *only* on +the deco path would preserve the exact incoherence this record exists to remove. + +**Prod blast radius was measured, not assumed:** 0 `Deco` rows, 0 `DecoWatermark` rows, and all 43 +channels have logo artwork — so no rendered output changes. That measurement is what made "no bug" +affordable over the two alternatives (enable initials everywhere; or cache the initials into the image +cache and then enable). The second alternative is the right end-state and is **rejected only on +scope**, not on merit: generating the image into the image cache would make it a local path, honouring +#525 and sidestepping #1 entirely. Anyone reviving the nameplate should do it that way rather than by +reinstating a render-time URL. + +**Preserved deliberately:** a playout-item `Custom` watermark with a blank image still falls *through* +to the channel/global watermark instead of resolving to "no watermark". Unifying resolution must not +change which watermark *wins*. Likewise routing is untouched — `CanUseFFmpegNativeWatermark` still +keys off the resolved path alone, so any URL goes to the graphics engine regardless of provenance. + +**Newly reachable, as a strict improvement:** the channel and global arms previously threw +`NotSupportedException` on a `Resource` watermark (a hand-edited or imported row could kill the +stream); they now resolve it like the playout-item arm. The `default:` arm still throws, so a +*newly added* image source fails loudly rather than inheriting a neighbour's behavior. + +The parity test cases are the structural guard: they assert deco and channel-level resolution return +identical paths case for case, so re-introducing a per-caller policy fails in CI rather than in prod. +Against the pre-fix resolver 12 of the 18 new tests fail, which is what proves they are load-bearing +rather than vacuous. -- 2.47.3 From 1a7f15fb27b78485bea4fde06ad5c3986a853ef3 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 21:12:13 +0200 Subject: [PATCH 2/5] =?UTF-8?q?fix(510):=20address=20independent=20review?= =?UTF-8?q?=20=E2=80=94=20Resource=20null=20guard,=20honest=20routing=20cl?= =?UTF-8?q?aim?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../WatermarkSelectorDecoResolutionTests.cs | 243 ++++++++++++++++-- ErsatzTV.Core/FFmpeg/WatermarkSelector.cs | 49 +++- docs/channels.md | 10 +- docs/decisions/README.md | 2 +- .../ffmpeg/watermark-resolution-unified.md | 60 +++-- 5 files changed, 313 insertions(+), 51 deletions(-) diff --git a/ErsatzTV.Core.Tests/FFmpeg/WatermarkSelectorDecoResolutionTests.cs b/ErsatzTV.Core.Tests/FFmpeg/WatermarkSelectorDecoResolutionTests.cs index 372a7c58e..16661cd20 100644 --- a/ErsatzTV.Core.Tests/FFmpeg/WatermarkSelectorDecoResolutionTests.cs +++ b/ErsatzTV.Core.Tests/FFmpeg/WatermarkSelectorDecoResolutionTests.cs @@ -50,10 +50,17 @@ public class WatermarkSelectorDecoResolutionTests /// Builds a selector whose mock filesystem contains exactly . private static WatermarkSelector Selector(Deco playoutDeco, params string[] existingFiles) { + // one Initialize() call, chained -- calling it per file would leave "does a second Initialize() + // preserve the first file?" untested, and a silently under-seeded filesystem makes a + // "resolves to nothing" assertion pass for the wrong reason var mockFileSystem = new MockFileSystem(); - foreach (string file in existingFiles) + if (existingFiles.Length > 0) { - mockFileSystem.Initialize().WithFile(file); + var initialized = mockFileSystem.Initialize().WithFile(existingFiles[0]); + foreach (string file in existingFiles.Skip(1)) + { + initialized = initialized.WithFile(file); + } } var fakeImageCache = Substitute.For(); @@ -62,6 +69,16 @@ public class WatermarkSelectorDecoResolutionTests fakeImageCache.GetPathForImage(Arg.Any(), Arg.Is(ArtworkKind.Watermark), Arg.Any>()) .Returns(_ => CustomCachePath); + // Faithful to the real ImageCache.GetPathForImage, which does fileName[..2] and therefore THROWS on a + // blank/null name. Modelling that is what makes the blank-image guard tests mutation-sensitive: before + // #510 the channel and global arms had no guard and this threw out of stream startup. + fakeImageCache + .GetPathForImage( + Arg.Is(s => string.IsNullOrWhiteSpace(s)), + Arg.Any(), + Arg.Any>()) + .Returns(_ => throw new ArgumentOutOfRangeException(nameof(IImageCache.GetPathForImage))); + var decoSelector = Substitute.For(); decoSelector.GetDecoEntries(Arg.Any(), Arg.Any()) .Returns(new DecoEntries(Option.None, Optional(playoutDeco))); @@ -196,9 +213,11 @@ public class WatermarkSelectorDecoResolutionTests } /// - /// Explicitly pins that the generated-initials URL is never handed downstream as a renderable path, - /// rather than only that the result is empty — an assertion that survives a future refactor which - /// starts returning options again. + /// Redundant with the empty-result assertion above **by construction today** — an empty list trivially + /// contains no URL. Kept deliberately as a belt-and-braces pin on the specific value: if a future change + /// makes this arrangement return options again (e.g. reviving the nameplate per #652), the sibling test + /// fails with "expected empty" while this one fails naming the generated URL, which is the more + /// actionable message. It is not independent coverage and should not be counted as such. /// [Test] public void Deco_ChannelLogo_Should_Never_Return_The_Generated_Initials_Url() @@ -271,41 +290,225 @@ public class WatermarkSelectorDecoResolutionTests result.ShouldBeEmpty(); } + // ---- non-deco consequences of the SAME unification ------------------------------------------- + // + // These pin precedence-level behavior rather than deco behavior, but they exist because of the #510 + // unification: one is the single piece of per-caller policy deliberately kept, the others are arms that + // used to throw. Without them a future refactor can delete the survivor, or re-introduce the crash, with + // a fully green suite. + + /// + /// The one surviving per-caller policy: a playout-item `Custom` watermark with a blank image falls + /// THROUGH to the channel/global watermark rather than resolving to "no watermark". Unifying + /// resolution must not change which watermark WINS. + /// + [Test] + public void Blank_Custom_Playout_Item_Watermark_Should_Fall_Through_To_Channel_Watermark() + { + ChannelWatermark playoutItemWatermark = Watermark(ChannelWatermarkImageSource.Custom, " "); + ChannelWatermark channelWatermark = Watermark(ChannelWatermarkImageSource.Custom, CustomStoredPath); + channelWatermark.Id = 8; + Channel channel = ChannelWith(LogoStoredPath, channelWatermark); + + Option result = Selector(null, CustomCachePath) + .GetWatermarkOptions(channel, playoutItemWatermark, Option.None); + + // the CHANNEL watermark wins -- not None, and not the blank playout-item one + result.IsSome.ShouldBeTrue(); + WatermarkOptions options = result.IfNone(() => throw new InvalidOperationException()); + options.ImagePath.ShouldBe(CustomCachePath); + options.Watermark.Id.ShouldBe(8); + } + + /// + /// Before #510 the channel and global arms had no blank-image guard, so they reached + /// ImageCache.GetPathForImage whose fileName[..2] threw out of stream startup. Now a + /// warning plus no watermark. + /// + [TestCase(null)] + [TestCase("")] + [TestCase(" ")] + public void Channel_Level_Blank_Custom_Watermark_Should_Resolve_To_None_Not_Throw(string image) + { + ChannelWatermark channelWatermark = Watermark(ChannelWatermarkImageSource.Custom, image); + Channel channel = ChannelWith(LogoStoredPath, channelWatermark); + + Option result = Should.NotThrow( + () => Selector(null).GetWatermarkOptions( + channel, + Option.None, + Option.None)); + + result.IsNone.ShouldBeTrue(); + } + + /// + /// Same fall-through, but landing on the GLOBAL watermark — the channel-level variant above cannot + /// distinguish "fell through correctly" from "stopped at the channel by accident". + /// + [Test] + public void Blank_Custom_Playout_Item_Watermark_Should_Fall_Through_To_Global_Watermark() + { + ChannelWatermark playoutItemWatermark = Watermark(ChannelWatermarkImageSource.Custom, " "); + ChannelWatermark globalWatermark = Watermark(ChannelWatermarkImageSource.Custom, CustomStoredPath); + globalWatermark.Id = 9; + + // no channel-level watermark, so the only remaining candidate is the global one + Channel channel = ChannelWith(LogoStoredPath); + + Option result = Selector(null, CustomCachePath) + .GetWatermarkOptions(channel, playoutItemWatermark, globalWatermark); + + result.IsSome.ShouldBeTrue(); + WatermarkOptions options = result.IfNone(() => throw new InvalidOperationException()); + options.ImagePath.ShouldBe(CustomCachePath); + options.Watermark.Id.ShouldBe(9); + } + + /// + /// The complement of the fall-through cases: a NON-blank custom image whose file is merely missing must + /// NOT fall through — it resolves to "no watermark" and the channel watermark never gets a turn. + /// Without this, widening the blank-image guard to "any unresolvable custom" would pass unnoticed. + /// + [Test] + public void Missing_But_Named_Custom_Playout_Item_Watermark_Should_Not_Fall_Through() + { + ChannelWatermark playoutItemWatermark = Watermark(ChannelWatermarkImageSource.Custom, CustomStoredPath); + ChannelWatermark channelWatermark = Watermark(ChannelWatermarkImageSource.Custom, CustomStoredPath); + channelWatermark.Id = 8; + Channel channel = ChannelWith(LogoStoredPath, channelWatermark); + + // nothing on disk, so the playout-item watermark is unresolvable + Option result = Selector(null) + .GetWatermarkOptions(channel, playoutItemWatermark, Option.None); + + result.IsNone.ShouldBeTrue(); + } + + /// + /// `CreateWatermarkHandler`/`UpdateWatermarkHandler` write `Image = null` for every non-`Custom` + /// watermark, so an API-created `Resource` watermark hits `Path.Combine(folder, null)` — an + /// `ArgumentNullException` out of stream startup. Uses the persisted shape (null), not a hand-made + /// filename, which is what the rest of the fixture would otherwise assume. + /// + [TestCase(null)] + [TestCase("")] + public void Resource_Watermark_With_No_Image_Name_Should_Resolve_To_None_Not_Throw(string image) + { + ChannelWatermark channelWatermark = Watermark(ChannelWatermarkImageSource.Resource, image); + Channel channel = ChannelWith(LogoStoredPath, channelWatermark); + + Option result = Should.NotThrow( + () => Selector(null).GetWatermarkOptions( + channel, + Option.None, + Option.None)); + + result.IsNone.ShouldBeTrue(); + } + + /// + /// Dropping an unresolvable watermark shortens the list handed to + /// CanUseFFmpegNativeWatermark, whose predicate includes `Count == 1`. So this is also the pin on + /// the observable routing change: two attached permanent watermarks, one missing, now yield ONE option + /// (ffmpeg-native) where they previously yielded two (graphics engine). + /// + [Test] + public void Deco_With_One_Valid_And_One_Missing_Watermark_Should_Return_Only_The_Valid_One() + { + ChannelWatermark valid = Watermark(ChannelWatermarkImageSource.ChannelLogo); + ChannelWatermark missing = Watermark(ChannelWatermarkImageSource.Custom, CustomStoredPath); + missing.Id = 8; + + var deco = new Deco + { + Id = 1, + Name = "Test Deco", + WatermarkMode = DecoMode.Override, + UseWatermarkDuringFiller = true, + DecoWatermarks = + [ + new DecoWatermark { WatermarkId = valid.Id, Watermark = valid }, + new DecoWatermark { WatermarkId = missing.Id, Watermark = missing } + ], + Watermarks = [] + }; + + // only the channel logo's cached file exists; the custom watermark's does not + List result = Selector(deco, LogoCachePath).SelectWatermarks( + Option.None, + ChannelWith(LogoStoredPath), + PlayoutItem(), + DateTimeOffset.Now); + + result.Count.ShouldBe(1); + result[0].ImagePath.ShouldBe(LogoCachePath); + } + + /// + /// Before #510 the global arm had no Resource case and hit default: throw. + /// + [Test] + public void Global_Level_Resource_Watermark_Should_Resolve_Instead_Of_Throwing() + { + ChannelWatermark globalWatermark = Watermark(ChannelWatermarkImageSource.Resource, ResourceImage); + Channel channel = ChannelWith(LogoStoredPath); + + Option result = Should.NotThrow( + () => Selector(null, ResourcePath).GetWatermarkOptions( + channel, + Option.None, + globalWatermark)); + + result.IsSome.ShouldBeTrue(); + result.IfNone(() => throw new InvalidOperationException()).ImagePath.ShouldBe(ResourcePath); + } + // ---- the structural guard: deco and channel-level must agree, case for case ------------------ private static IEnumerable ParityCases() { - yield return new TestCaseData(ChannelWatermarkImageSource.ChannelLogo, LogoStoredPath, new[] { LogoCachePath }) + // (image source, watermark.Image, channel logo path, files that exist) + yield return new TestCaseData( + ChannelWatermarkImageSource.ChannelLogo, "", LogoStoredPath, new[] { LogoCachePath }) .SetName("ChannelLogo, local file present"); - yield return new TestCaseData(ChannelWatermarkImageSource.ChannelLogo, LogoStoredPath, Array.Empty()) + yield return new TestCaseData( + ChannelWatermarkImageSource.ChannelLogo, "", LogoStoredPath, Array.Empty()) .SetName("ChannelLogo, local file missing"); - yield return new TestCaseData(ChannelWatermarkImageSource.ChannelLogo, ExternalLogoUrl, Array.Empty()) + yield return new TestCaseData( + ChannelWatermarkImageSource.ChannelLogo, "", ExternalLogoUrl, Array.Empty()) .SetName("ChannelLogo, external URL"); - yield return new TestCaseData(ChannelWatermarkImageSource.ChannelLogo, null, Array.Empty()) + yield return new TestCaseData( + ChannelWatermarkImageSource.ChannelLogo, "", null, Array.Empty()) .SetName("ChannelLogo, no logo artwork"); - yield return new TestCaseData(ChannelWatermarkImageSource.Custom, LogoStoredPath, new[] { CustomCachePath }) + yield return new TestCaseData( + ChannelWatermarkImageSource.Custom, CustomStoredPath, LogoStoredPath, new[] { CustomCachePath }) .SetName("Custom, file present"); - yield return new TestCaseData(ChannelWatermarkImageSource.Custom, LogoStoredPath, Array.Empty()) + yield return new TestCaseData( + ChannelWatermarkImageSource.Custom, CustomStoredPath, LogoStoredPath, Array.Empty()) .SetName("Custom, file missing"); - yield return new TestCaseData(ChannelWatermarkImageSource.Resource, LogoStoredPath, new[] { ResourcePath }) + // Both sides agree here by construction (each returns nothing), which is the point: it documents that + // the blank-image fall-through asymmetry lives ONLY at the playout-item level -- covered by + // Blank_Custom_Playout_Item_Watermark_Should_Fall_Through_To_Channel_Watermark -- rather than leaving + // the omission looking like an evasion. + yield return new TestCaseData( + ChannelWatermarkImageSource.Custom, " ", LogoStoredPath, Array.Empty()) + .SetName("Custom, blank image"); + yield return new TestCaseData( + ChannelWatermarkImageSource.Resource, ResourceImage, LogoStoredPath, new[] { ResourcePath }) .SetName("Resource, file present"); - yield return new TestCaseData(ChannelWatermarkImageSource.Resource, LogoStoredPath, Array.Empty()) + yield return new TestCaseData( + ChannelWatermarkImageSource.Resource, ResourceImage, LogoStoredPath, Array.Empty()) .SetName("Resource, file missing"); } [TestCaseSource(nameof(ParityCases))] public void Deco_And_Channel_Level_Should_Resolve_Identically( ChannelWatermarkImageSource source, + string image, string logoPath, string[] existingFiles) { - string image = source switch - { - ChannelWatermarkImageSource.Custom => CustomStoredPath, - ChannelWatermarkImageSource.Resource => ResourceImage, - _ => "" - }; - // deco path ChannelWatermark decoWatermark = Watermark(source, image); List viaDeco = SelectViaDeco(decoWatermark, ChannelWith(logoPath), existingFiles); diff --git a/ErsatzTV.Core/FFmpeg/WatermarkSelector.cs b/ErsatzTV.Core/FFmpeg/WatermarkSelector.cs index 5e6e070d4..3d7f689f8 100644 --- a/ErsatzTV.Core/FFmpeg/WatermarkSelector.cs +++ b/ErsatzTV.Core/FFmpeg/WatermarkSelector.cs @@ -210,20 +210,41 @@ public class WatermarkSelector( /// /// /// Before #510 the deco path had its own copy of this switch that resolved paths *unchecked* — it handed - /// down a nonexistent file, an un-migrated external URL, and the generated-initials localhost URL, while - /// the three precedence levels degraded all three to "no watermark". So the same channel could disagree - /// with itself about whether a bug rendered, purely by how the watermark was attached. Duplication is what - /// let that drift happen (it existed in triplicate before #502), so there is now exactly one resolver and - /// no per-caller policy. An unresolvable watermark is always "no on-screen bug", never a dead path passed - /// downstream: a dead LOCAL path could reach ffmpeg as a bare -i argument via - /// CanUseFFmpegNativeWatermark, which is materially worse than a skipped overlay. + /// down a nonexistent file, an un-migrated external URL, and the generated-initials localhost URL. The + /// playout-item level checked all three sources; the channel and global levels checked + /// Custom/ChannelLogo and *threw* for Resource (no arm, so `default:`). So the same + /// channel could disagree with itself about whether a bug rendered, purely by how the watermark was + /// attached. Duplication is what let that drift happen (it existed in triplicate before #502), so there is + /// now one resolver and no per-caller policy. An unresolvable watermark resolves to "no on-screen bug", + /// never a dead path passed downstream: a dead LOCAL path could reach ffmpeg as a bare -i argument + /// via CanUseFFmpegNativeWatermark, which is materially worse than a skipped overlay. + /// + /// Watermarks built OUTSIDE this selector are not covered — the song-progress overlay is constructed as a + /// WatermarkOptions directly by the streaming and troubleshooting handlers and is still unchecked + /// (#653). + /// /// private Option ResolveWatermark(Channel channel, ChannelWatermark watermark) { switch (watermark.ImageSource) { - // used for song progress overlay + // Reachable only from a hand-edited or imported row: nothing in the app writes a Resource + // watermark to the database. The real song-progress overlay does NOT come through here -- it is + // built directly as a WatermarkOptions by the streaming/troubleshooting handlers, which bypass + // this resolver and are still unchecked (#653). case ChannelWatermarkImageSource.Resource: + // Image is NULL for every non-Custom watermark the API writes (CreateWatermarkHandler / + // UpdateWatermarkHandler both set `Image = null` unless ImageSource is Custom), so this guard is + // load-bearing, not defensive: Path.Combine(folder, null) throws ArgumentNullException, which + // would surface as a failed stream start rather than a missing overlay. + if (string.IsNullOrWhiteSpace(watermark.Image)) + { + logger.LogWarning( + "Watermark {Name} uses a resource image but has no image name; ignoring", + watermark.Name); + return None; + } + string resourcePath = fileSystem.Path.Combine( FileSystemLayout.ResourcesCacheFolder, watermark.Image); @@ -337,9 +358,15 @@ public class WatermarkSelector( /// /// Resolution is now identical to the precedence levels; what stays deco-specific is only WHICH /// watermarks apply and whether they merge with or override the rest (handled in - /// ). Routing is unaffected and remains keyed off the resolved path - /// alone — FFmpegLibraryProcessService.CanUseFFmpegNativeWatermark sends any URL to the - /// graphics engine regardless of provenance. + /// ). + /// + /// The routing PREDICATE is unchanged — CanUseFFmpegNativeWatermark still keys off the resolved + /// path alone and sends any URL to the graphics engine regardless of provenance. Its INPUT can change, + /// though: dropping an unresolvable watermark shortens this list, so a deco carrying one valid and one + /// missing permanent watermark now yields count 1 (ffmpeg-native) where it previously yielded count 2 + /// (graphics engine). That is intended — the surviving watermark is a single valid permanent local image, + /// exactly what the native path is for — but it IS an observable routing change, not a no-op. + /// /// private List OptionsForWatermarks(Channel channel, IEnumerable watermarks) { diff --git a/docs/channels.md b/docs/channels.md index 137e92315..cb69dba08 100644 --- a/docs/channels.md +++ b/docs/channels.md @@ -205,9 +205,10 @@ bounded render-time fetch that preceded caching) is in `docs/decisions.md` under watermark resolves through one shared resolver (`WatermarkSelector.ResolveWatermark`) whether it is attached via a playout item, the channel, the global setting, **or a deco**. All four agree: an un-migrated external URL, a missing cached file, and a channel with no logo artwork each render -*without* a bug and log a warning. Nothing hands a dead path or a URL downstream — a dead local path -could otherwise reach ffmpeg as a bare `-i` argument and break the stream, which is worse than a -skipped overlay. +*without* a bug and log a warning. The selector never hands a dead path or a URL downstream — a dead +local path could otherwise reach ffmpeg as a bare `-i` argument and break the stream, which is worse +than a skipped overlay. (One watermark is built *outside* the selector and is still unchecked: the +song-progress overlay — see #653.) Before #510 the deco path had its own unchecked copy of that resolution, so the same channel could disagree with itself about whether a bug rendered based only on how the watermark was attached. The @@ -219,7 +220,8 @@ when — the watermark came from a deco. It is now off everywhere, because servi fetch inside stream startup, exactly what `graphics.channel-logo-caching` (#525) removed for logos, and because `ChannelLogoGenerator.GenerateChannelLogoUrl()` hardcodes `localhost` (issue #1, closed as a topology problem without removing the hardcode). Reviving it properly means generating the image -into the image cache so it resolves to a local path; see `ffmpeg.watermark-resolution-unified`. +into the image cache so it resolves to a local path — tracked as **#652**; the rationale is in +`ffmpeg.watermark-resolution-unified`. ## On Now / Next overlay (#74) diff --git a/docs/decisions/README.md b/docs/decisions/README.md index e87cabc21..686aaca7d 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -75,7 +75,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ffmpeg.qsv-decode-encode-split` | QSV decode is decoupled from QSV encode via a single `FFmpegProfile.QsvPreferNativeDecoder` bool (default ON, Linux-only), so a QSV encode profile can decode with the more tolerant native VA-API decoder instead of the QSV decoder, mirroring Jellyfin's hybrid decode/encode toggle instead of a general decode-family enum. | 2026-07-20 | [link](records/ffmpeg/qsv-decode-encode-split.md) | | `ffmpeg.qsv-extra-hw-frames-floor` | a QSV upload never emits `extra_hw_frames` below `FFmpegState.MinimumQsvExtraHardwareFrames` (64); a stored `0` or negative value is treated as "no pool configured" rather than honored literally, because with no headroom any unthrottled read exhausts the pool and the transcode writes nothing at all. | 2026-07-21 | [link](records/ffmpeg/qsv-extra-hw-frames-floor.md) | | `ffmpeg.remote-image-fetcher-bounded` | remote graphics-engine images are fetched through `IRemoteImageFetcher` with a pooled `HttpClientFactory` client, a body-covering deadline, a wire-transfer size cap, and a decoder-enforced `DecoderOptions.MaxFrames` bound re-verified post-decode — never cached, re-fetched per element init. | 2026-07-20 | [link](records/ffmpeg/remote-image-fetcher-bounded.md) | -| `ffmpeg.watermark-resolution-unified` | Every `ChannelWatermark` becomes a renderable path through one shared `WatermarkSelector.ResolveWatermark`, used by the playout-item, channel and global precedence levels AND the deco path, for all three `ChannelWatermarkImageSource` values. An unresolvable watermark (missing file, un-migrated external URL, or no logo artwork) always means no on-screen bug plus a warning — never a dead path or a URL handed downstream. The generated-initials fallback is therefore off everywhere, including the deco path where it demonstrably rendered. | 2026-07-26 | [link](records/ffmpeg/watermark-resolution-unified.md) | +| `ffmpeg.watermark-resolution-unified` | Every watermark `WatermarkSelector` resolves goes through one shared `ResolveWatermark` — the playout-item, channel and global precedence levels AND the deco path, for all three `ChannelWatermarkImageSource` values. An unresolvable watermark (missing file, un-migrated external URL, or no logo artwork) resolves to no on-screen bug plus a warning, never a dead path or a URL handed downstream; the one deliberate exception is a playout-item `Custom` with a blank image, which still falls THROUGH to channel/global. The generated-initials fallback is therefore off everywhere, including the deco path where it demonstrably rendered. Watermarks built OUTSIDE the selector (the song-progress overlay, #653) are not covered and remain unchecked. | 2026-07-26 | [link](records/ffmpeg/watermark-resolution-unified.md) | | `ffmpeg.work-ahead-slot-atomic` | `workAheadSegmenterLimit` is enforced by a single compare-exchange claim on a shared `WorkAheadSlots` pool taken by the *caller* of `Transcode`, which then passes ownership in and gets the release in `Transcode`'s `finally` — never a `Volatile.Read` compare in one place and an `Interlocked.Increment` in another. | 2026-07-21 | [link](records/ffmpeg/work-ahead-slot-atomic.md) | | `ffmpeg.work-ahead-slot-release-never-negative` | `Release()` reads the count and compare-exchanges `current - 1` only when `current > 0`; a release against an empty pool records an unbalanced release and returns `false` **without ever writing a negative value**. It never decrements first and clamps afterward. The single caller (`HlsSessionWorker.Transcode`'s `finally`) logs a warning on the `false` return. | 2026-07-21 | [link](records/ffmpeg/work-ahead-slot-release-never-negative.md) | | `graphics.channel-level-attachment` | A channel can attach `GraphicsElement`s directly via a new `ChannelGraphicsElement` join table (a base layer under deco/playout-item elements), and a built-in text element (`on-now-next.yml`) is seeded once per database so the On Now/Next overlay works out of the box. | 2026-07-22 | [link](records/graphics/channel-level-attachment.md) | diff --git a/docs/decisions/records/ffmpeg/watermark-resolution-unified.md b/docs/decisions/records/ffmpeg/watermark-resolution-unified.md index 4dc03a0cd..e74781063 100644 --- a/docs/decisions/records/ffmpeg/watermark-resolution-unified.md +++ b/docs/decisions/records/ffmpeg/watermark-resolution-unified.md @@ -5,20 +5,31 @@ status: active since: '2026-07-26' supersedes: none superseded-by: none -rule: Every `ChannelWatermark` becomes a renderable path through one shared `WatermarkSelector.ResolveWatermark`, used by the playout-item, channel and global precedence levels AND the deco path, for all three `ChannelWatermarkImageSource` values. An unresolvable watermark (missing file, un-migrated external URL, or no logo artwork) always means no on-screen bug plus a warning — never a dead path or a URL handed downstream. The generated-initials fallback is therefore off everywhere, including the deco path where it demonstrably rendered. -signals: 'deco watermark, missing logo policy, generated initials, nameplate bug, watermark resolution divergence · paths: `ErsatzTV.Core/FFmpeg/WatermarkSelector.cs` (`ResolveWatermark`, `ChannelLogoWatermarkOptions`, `OptionsForWatermarks`), `ChannelLogoGenerator.GenerateChannelLogoUrl` · issues: #510, #502, #525, #1, #67' +rule: Every watermark `WatermarkSelector` resolves goes through one shared `ResolveWatermark` — the playout-item, channel and global precedence levels AND the deco path, for all three `ChannelWatermarkImageSource` values. An unresolvable watermark (missing file, un-migrated external URL, or no logo artwork) resolves to no on-screen bug plus a warning, never a dead path or a URL handed downstream; the one deliberate exception is a playout-item `Custom` with a blank image, which still falls THROUGH to channel/global. The generated-initials fallback is therefore off everywhere, including the deco path where it demonstrably rendered. Watermarks built OUTSIDE the selector (the song-progress overlay, #653) are not covered and remain unchecked. +signals: 'deco watermark, missing logo policy, generated initials, nameplate bug, watermark resolution divergence, song-progress overlay bypass · paths: `ErsatzTV.Core/FFmpeg/WatermarkSelector.cs` (`ResolveWatermark`, `ChannelLogoWatermarkOptions`, `OptionsForWatermarks`), `ChannelLogoGenerator.GenerateChannelLogoUrl` · issues: #510, #502, #525, #1, #67, #652, #653' mechanics: '`WatermarkSelectorDecoResolutionTests` (incl. the `Deco_And_Channel_Level_Should_Resolve_Identically` parity cases); `docs/channels.md` → Watermarks' --- -`WatermarkSelector` resolved a watermark in two places with two policies. The three precedence levels -existence-checked every 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 a bug rendered, based only on how the watermark was attached. +`WatermarkSelector` resolved a watermark 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 the +channel and global levels checked `Custom`/`ChannelLogo` and *threw* for `Resource` (they had no arm +for it, so it fell to `default:`). So one channel could disagree with itself about whether a bug +rendered, based only on how the watermark was attached. #502 had noted the split and deferred it here. What #502's note got wrong is scope: the divergence was never `ChannelLogo`-only. The deco path skipped the existence check for **`Custom` and `Resource` too** — three arms, not one. Fixing the named arm and leaving two is how this defect reached -triplicate in the first place, so there is now exactly **one** resolver and no per-caller policy. +triplicate in the first place, so the selector now has exactly **one** resolver and no per-caller +policy. + +**Scope boundary, found by review of this very change:** "one resolver" is true *of the selector*, not +of the application. `GetPlayoutItemProcessByChannelNumberHandler` and +`PrepareTroubleshootingPlaybackHandler` build the song-progress overlay's `WatermarkOptions` by hand +with an unchecked `Path.Combine(ResourcesCacheFolder, …)` and then `watermarks.Clear()`, so they +bypass `ResolveWatermark` and can still hand ffmpeg a nonexistent `-i` (the exact hazard described +below). Pre-existing and left alone here; tracked as **#653**. An earlier draft of this record claimed +the hazard was closed outright — it is closed only for watermarks the selector resolves. **Severity is not merely cosmetic, which is what settled the direction.** A dead *local* path is not harmlessly skipped: `CanUseFFmpegNativeWatermark` routes a single permanent watermark to ffmpeg as a @@ -44,18 +55,37 @@ channels have logo artwork — so no rendered output changes. That measurement i affordable over the two alternatives (enable initials everywhere; or cache the initials into the image cache and then enable). The second alternative is the right end-state and is **rejected only on scope**, not on merit: generating the image into the image cache would make it a local path, honouring -#525 and sidestepping #1 entirely. Anyone reviving the nameplate should do it that way rather than by -reinstating a render-time URL. +#525 and sidestepping #1 entirely. That work is **#652** — anyone reviving the nameplate should do it +that way rather than by reinstating a render-time URL. **Preserved deliberately:** a playout-item `Custom` watermark with a blank image still falls *through* to the channel/global watermark instead of resolving to "no watermark". Unifying resolution must not -change which watermark *wins*. Likewise routing is untouched — `CanUseFFmpegNativeWatermark` still -keys off the resolved path alone, so any URL goes to the graphics engine regardless of provenance. +change which watermark *wins*. -**Newly reachable, as a strict improvement:** the channel and global arms previously threw -`NotSupportedException` on a `Resource` watermark (a hand-edited or imported row could kill the -stream); they now resolve it like the playout-item arm. The `default:` arm still throws, so a -*newly added* image source fails loudly rather than inheriting a neighbour's behavior. +**Routing: the predicate is unchanged, its input is not.** `CanUseFFmpegNativeWatermark` still keys off +the resolved path alone, so any URL still goes to the graphics engine regardless of provenance. But it +also tests `watermarks.Count == 1`, and dropping an unresolvable watermark shortens that list — so a +deco carrying one valid and one missing permanent watermark now routes ffmpeg-native (count 1) where it +previously routed to the graphics engine (count 2). Intended, since what survives is a single valid +permanent local image, but it is an observable routing change and an earlier draft of this record wrongly +called routing untouched. Pinned by `Deco_With_One_Valid_And_One_Missing_Watermark_Should_Return_Only_The_Valid_One`. + +**Three strict improvements, all previously crash-shaped.** The channel and global arms threw +`NotSupportedException` on a `Resource` watermark; they now resolve it like the playout-item arm — and +the `Resource` arm gained a blank/null guard it never had, because `CreateWatermarkHandler` and +`UpdateWatermarkHandler` both write `Image = null` for every non-`Custom` watermark, so an +API-created `Resource` watermark reached `Path.Combine(folder, null)` and threw `ArgumentNullException` +at *any* level, including the playout-item one. Less +obviously, those two arms also had **no blank-image guard** on `Custom` — only the playout-item arm +did — so they called `ImageCache.GetPathForImage`, whose `fileName[..2]` throws +`ArgumentOutOfRangeException` on `""` and `NullReferenceException` on `null`. That state is reachable +through the current API, not just legacy rows: `CreateWatermarkHandler`/`UpdateWatermarkHandler` +assign `Image = update.Image?.Path` with nothing requiring an image. So a channel-level `Custom` +watermark saved with its image cleared threw *out of the selector during stream startup* — killing +playback, not just the overlay — and now degrades to a warning plus no watermark. No existing test +could see this, because every fixture stubs `IImageCache` and never executes `fileName[..2]`. +The `default:` arm still throws, so a *newly added* image source fails loudly rather than inheriting +a neighbour's behavior. The parity test cases are the structural guard: they assert deco and channel-level resolution return identical paths case for case, so re-introducing a per-caller policy fails in CI rather than in prod. -- 2.47.3 From edf8be4b5e19274b14fac691b694305e2f05c212 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 21:29:11 +0200 Subject: [PATCH 3/5] =?UTF-8?q?fix(510):=20re-review=20round=20=E2=80=94?= =?UTF-8?q?=20make=20two=20review-added=20tests=20actually=20falsifiable?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../WatermarkSelectorDecoResolutionTests.cs | 61 ++++++++++++------- ErsatzTV.Core/FFmpeg/WatermarkSelector.cs | 14 +++-- .../ffmpeg/watermark-resolution-unified.md | 17 ++++-- 3 files changed, 62 insertions(+), 30 deletions(-) diff --git a/ErsatzTV.Core.Tests/FFmpeg/WatermarkSelectorDecoResolutionTests.cs b/ErsatzTV.Core.Tests/FFmpeg/WatermarkSelectorDecoResolutionTests.cs index 16661cd20..e686f14e7 100644 --- a/ErsatzTV.Core.Tests/FFmpeg/WatermarkSelectorDecoResolutionTests.cs +++ b/ErsatzTV.Core.Tests/FFmpeg/WatermarkSelectorDecoResolutionTests.cs @@ -210,25 +210,12 @@ public class WatermarkSelectorDecoResolutionTests List result = SelectViaDeco(watermark, channel); result.ShouldBeEmpty(); - } - /// - /// Redundant with the empty-result assertion above **by construction today** — an empty list trivially - /// contains no URL. Kept deliberately as a belt-and-braces pin on the specific value: if a future change - /// makes this arrangement return options again (e.g. reviving the nameplate per #652), the sibling test - /// fails with "expected empty" while this one fails naming the generated URL, which is the more - /// actionable message. It is not independent coverage and should not be counted as such. - /// - [Test] - public void Deco_ChannelLogo_Should_Never_Return_The_Generated_Initials_Url() - { - ChannelWatermark watermark = Watermark(ChannelWatermarkImageSource.ChannelLogo); - Channel channel = ChannelWith(null); - string generatedUrl = ChannelLogoGenerator.GenerateChannelLogoUrl(channel); - - List result = SelectViaDeco(watermark, channel); - - result.Select(o => o.ImagePath).ShouldNotContain(generatedUrl); + // Folded in from a separate test that asserted only this. On its own it was vacuous — an empty list + // trivially contains no URL — so it is a second assertion here rather than a test implying independent + // coverage. It earns its place by naming the value if this ever starts returning options again (#652). + result.Select(o => o.ImagePath) + .ShouldNotContain(ChannelLogoGenerator.GenerateChannelLogoUrl(channel)); } // ---- Custom and Resource: the two arms #510 did not mention but that diverged too ----------- @@ -370,21 +357,48 @@ public class WatermarkSelectorDecoResolutionTests /// NOT fall through — it resolves to "no watermark" and the channel watermark never gets a turn. /// Without this, widening the blank-image guard to "any unresolvable custom" would pass unnoticed. /// + /// + /// The channel-level fallback is deliberately an INDEPENDENTLY RESOLVABLE `ChannelLogo` watermark whose + /// cached file exists. An earlier version of this test gave the fallback the same missing custom path as + /// the playout-item watermark, which made it unfalsifiable: a wrongly-widened guard would have fallen + /// through to a fallback that also resolved to None, so the assertion held either way. + /// [Test] public void Missing_But_Named_Custom_Playout_Item_Watermark_Should_Not_Fall_Through() { ChannelWatermark playoutItemWatermark = Watermark(ChannelWatermarkImageSource.Custom, CustomStoredPath); - ChannelWatermark channelWatermark = Watermark(ChannelWatermarkImageSource.Custom, CustomStoredPath); + ChannelWatermark channelWatermark = Watermark(ChannelWatermarkImageSource.ChannelLogo); channelWatermark.Id = 8; Channel channel = ChannelWith(LogoStoredPath, channelWatermark); - // nothing on disk, so the playout-item watermark is unresolvable - Option result = Selector(null) + // the channel logo's cached file EXISTS, so a fall-through would return it and fail this test; + // the custom watermark's file does not, so the playout-item watermark is unresolvable + Option result = Selector(null, LogoCachePath) .GetWatermarkOptions(channel, playoutItemWatermark, Option.None); result.IsNone.ShouldBeTrue(); } + /// + /// Positive control for the test above: the same arrangement, but with the playout-item watermark BLANK + /// rather than missing, must fall through and return the resolvable channel logo. Together the pair + /// shows the guard distinguishes blank from unresolvable, rather than both landing on None. + /// + [Test] + public void Blank_Custom_Playout_Item_Watermark_Should_Fall_Through_To_A_Resolvable_Channel_Logo() + { + ChannelWatermark playoutItemWatermark = Watermark(ChannelWatermarkImageSource.Custom, " "); + ChannelWatermark channelWatermark = Watermark(ChannelWatermarkImageSource.ChannelLogo); + channelWatermark.Id = 8; + Channel channel = ChannelWith(LogoStoredPath, channelWatermark); + + Option result = Selector(null, LogoCachePath) + .GetWatermarkOptions(channel, playoutItemWatermark, Option.None); + + result.IsSome.ShouldBeTrue(); + result.IfNone(() => throw new InvalidOperationException()).ImagePath.ShouldBe(LogoCachePath); + } + /// /// `CreateWatermarkHandler`/`UpdateWatermarkHandler` write `Image = null` for every non-`Custom` /// watermark, so an API-created `Resource` watermark hits `Path.Combine(folder, null)` — an @@ -443,6 +457,11 @@ public class WatermarkSelectorDecoResolutionTests result.Count.ShouldBe(1); result[0].ImagePath.ShouldBe(LogoCachePath); + + // The routing claim itself, not just the filtering: call the real predicate. Asserting Count == 1 alone + // would leave the decision record's "now routes ffmpeg-native" statement unpinned, since the decision + // lives in FFmpegLibraryProcessService rather than in the selector. + FFmpegLibraryProcessService.CanUseFFmpegNativeWatermark(0, result).ShouldBeTrue(); } /// diff --git a/ErsatzTV.Core/FFmpeg/WatermarkSelector.cs b/ErsatzTV.Core/FFmpeg/WatermarkSelector.cs index 3d7f689f8..d547e742b 100644 --- a/ErsatzTV.Core/FFmpeg/WatermarkSelector.cs +++ b/ErsatzTV.Core/FFmpeg/WatermarkSelector.cs @@ -215,7 +215,9 @@ public class WatermarkSelector( /// Custom/ChannelLogo and *threw* for Resource (no arm, so `default:`). So the same /// channel could disagree with itself about whether a bug rendered, purely by how the watermark was /// attached. Duplication is what let that drift happen (it existed in triplicate before #502), so there is - /// now one resolver and no per-caller policy. An unresolvable watermark resolves to "no on-screen bug", + /// now one resolver. Exactly one piece of per-caller policy survives, and it lives in the CALLER rather + /// than here: a playout-item Custom watermark with a blank image falls through to channel/global + /// (see ). An unresolvable watermark resolves to "no on-screen bug", /// never a dead path passed downstream: a dead LOCAL path could reach ffmpeg as a bare -i argument /// via CanUseFFmpegNativeWatermark, which is materially worse than a skipped overlay. /// @@ -228,10 +230,12 @@ public class WatermarkSelector( { switch (watermark.ImageSource) { - // Reachable only from a hand-edited or imported row: nothing in the app writes a Resource - // watermark to the database. The real song-progress overlay does NOT come through here -- it is - // built directly as a WatermarkOptions by the streaming/troubleshooting handlers, which bypass - // this resolver and are still unchecked (#653). + // NOT dead code and NOT only hand-edited rows: CreateWatermarkHandler/UpdateWatermarkHandler + // persist whatever ImageSource the request names, so a Resource watermark is creatable through + // the API -- always with Image = null, which is why the guard below is essential. + // Separately, the real song-progress overlay does NOT come through here: it is built directly as a + // WatermarkOptions by the streaming/troubleshooting handlers, which bypass this resolver and are + // still unchecked (#653). case ChannelWatermarkImageSource.Resource: // Image is NULL for every non-Custom watermark the API writes (CreateWatermarkHandler / // UpdateWatermarkHandler both set `Image = null` unless ImageSource is Custom), so this guard is diff --git a/docs/decisions/records/ffmpeg/watermark-resolution-unified.md b/docs/decisions/records/ffmpeg/watermark-resolution-unified.md index e74781063..bdc929402 100644 --- a/docs/decisions/records/ffmpeg/watermark-resolution-unified.md +++ b/docs/decisions/records/ffmpeg/watermark-resolution-unified.md @@ -20,8 +20,9 @@ rendered, based only on how the watermark was attached. #502 had noted the split and deferred it here. What #502's note got wrong is scope: the divergence was never `ChannelLogo`-only. The deco path skipped the existence check for **`Custom` and `Resource` too** — three arms, not one. Fixing the named arm and leaving two is how this defect reached -triplicate in the first place, so the selector now has exactly **one** resolver and no per-caller -policy. +triplicate in the first place, so the selector now has exactly **one** resolver. One piece of +per-caller policy survives by design, and it lives in the *caller* rather than the resolver: the +playout-item blank-`Custom` fall-through described below. **Scope boundary, found by review of this very change:** "one resolver" is true *of the selector*, not of the application. `GetPlayoutItemProcessByChannelNumberHandler` and @@ -89,5 +90,13 @@ a neighbour's behavior. The parity test cases are the structural guard: they assert deco and channel-level resolution return identical paths case for case, so re-introducing a per-caller policy fails in CI rather than in prod. -Against the pre-fix resolver 12 of the 18 new tests fail, which is what proves they are load-bearing -rather than vacuous. + +**Measured mutation sensitivity: 19 of the 29 cases fail when the pre-fix resolver is restored under the +final fixture.** The other 10 pass both ways by design — they pin behavior this change deliberately +*preserves* (the blank-`Custom` fall-through, file-present resolution) plus the positive control, so +passing before and after is the correct outcome for them, not a gap. This figure is worth re-measuring +rather than trusting if the fixture changes: the review of this very change found two tests that had +been added to close review findings yet could not fail — one whose fall-through fallback was itself +unresolvable, and one that asserted a filtered list length while the routing claim it was cited for +lived in a function it never called. Both are fixed; the lesson is that a test added under review +pressure needs the same negative control as the original. -- 2.47.3 From d189d17157ba4a26ae19dcdec43bf68097ed204c Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 21:38:09 +0200 Subject: [PATCH 4/5] =?UTF-8?q?fix(510):=20quote=20the=20record's=20rule?= =?UTF-8?q?=20scalar=20=E2=80=94=20unquoted=20"=20#653"=20was=20a=20YAML?= =?UTF-8?q?=20comment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI's script-tests job (test_frontmatter_reader_matches_pyyaml_on_every_real_record) caught that the dependency-free hand parser and PyYAML disagreed on this record's `rule:` field. Cause: `rule:` was an unquoted plain YAML scalar containing " #653)", and in a plain scalar " #" starts a comment. PyYAML therefore truncated the value at "...(the song-progress overlay," and silently dropped the rest of the clause -- including the scope carve-out naming #653, which is the whole point of that sentence. The generated catalog row was built from the truncated value, so the active-decisions view was quietly missing it. Same trap as fix(631) (a record TITLE whose unquoted " #" was a comment). The `signals:` and `mechanics:` fields were already single-quoted, which is why only `rule:` diverged; `title:` is safe because its "(#510)" has no space before the "#". Single-quoted `rule:` and regenerated the catalog, which now carries the full sentence. Worth recording: I ran scripts/decisions_validate.py locally but NOT scripts/tests, so the gate that catches exactly this class was the one local check I skipped. Touching the decisions corpus means running the pytest suite, not just the validator -- the validator reads through the hand parser and so cannot see a hand-parser-vs-PyYAML divergence by construction. Local: 225/225 script tests pass, decisions-validate OK. refs #510 --- docs/decisions/records/ffmpeg/watermark-resolution-unified.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/decisions/records/ffmpeg/watermark-resolution-unified.md b/docs/decisions/records/ffmpeg/watermark-resolution-unified.md index bdc929402..42742ae4e 100644 --- a/docs/decisions/records/ffmpeg/watermark-resolution-unified.md +++ b/docs/decisions/records/ffmpeg/watermark-resolution-unified.md @@ -5,7 +5,7 @@ status: active since: '2026-07-26' supersedes: none superseded-by: none -rule: Every watermark `WatermarkSelector` resolves goes through one shared `ResolveWatermark` — the playout-item, channel and global precedence levels AND the deco path, for all three `ChannelWatermarkImageSource` values. An unresolvable watermark (missing file, un-migrated external URL, or no logo artwork) resolves to no on-screen bug plus a warning, never a dead path or a URL handed downstream; the one deliberate exception is a playout-item `Custom` with a blank image, which still falls THROUGH to channel/global. The generated-initials fallback is therefore off everywhere, including the deco path where it demonstrably rendered. Watermarks built OUTSIDE the selector (the song-progress overlay, #653) are not covered and remain unchecked. +rule: 'Every watermark `WatermarkSelector` resolves goes through one shared `ResolveWatermark` — the playout-item, channel and global precedence levels AND the deco path, for all three `ChannelWatermarkImageSource` values. An unresolvable watermark (missing file, un-migrated external URL, or no logo artwork) resolves to no on-screen bug plus a warning, never a dead path or a URL handed downstream; the one deliberate exception is a playout-item `Custom` with a blank image, which still falls THROUGH to channel/global. The generated-initials fallback is therefore off everywhere, including the deco path where it demonstrably rendered. Watermarks built OUTSIDE the selector (the song-progress overlay, #653) are not covered and remain unchecked.' signals: 'deco watermark, missing logo policy, generated initials, nameplate bug, watermark resolution divergence, song-progress overlay bypass · paths: `ErsatzTV.Core/FFmpeg/WatermarkSelector.cs` (`ResolveWatermark`, `ChannelLogoWatermarkOptions`, `OptionsForWatermarks`), `ChannelLogoGenerator.GenerateChannelLogoUrl` · issues: #510, #502, #525, #1, #67, #652, #653' mechanics: '`WatermarkSelectorDecoResolutionTests` (incl. the `Deco_And_Channel_Level_Should_Resolve_Identically` parity cases); `docs/channels.md` → Watermarks' --- -- 2.47.3 From bc1a37ff01473c528839e04e59dea3317445e7f4 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 21:53:01 +0200 Subject: [PATCH 5/5] fix(510): pin the blank-guard's is-Custom discriminator, verified by mutation 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 #510 --- .../WatermarkSelectorDecoResolutionTests.cs | 46 +++++++++++++++++-- .../ffmpeg/watermark-resolution-unified.md | 32 +++++++++---- 2 files changed, 66 insertions(+), 12 deletions(-) diff --git a/ErsatzTV.Core.Tests/FFmpeg/WatermarkSelectorDecoResolutionTests.cs b/ErsatzTV.Core.Tests/FFmpeg/WatermarkSelectorDecoResolutionTests.cs index e686f14e7..8ae3e001d 100644 --- a/ErsatzTV.Core.Tests/FFmpeg/WatermarkSelectorDecoResolutionTests.cs +++ b/ErsatzTV.Core.Tests/FFmpeg/WatermarkSelectorDecoResolutionTests.cs @@ -384,10 +384,17 @@ public class WatermarkSelectorDecoResolutionTests /// rather than missing, must fall through and return the resolvable channel logo. Together the pair /// shows the guard distinguishes blank from unresolvable, rather than both landing on None. /// - [Test] - public void Blank_Custom_Playout_Item_Watermark_Should_Fall_Through_To_A_Resolvable_Channel_Logo() + /// + /// Parameterized over all three blank forms because the guard is IsNullOrWhiteSpace: testing only + /// " " would let a mutation to image == " " pass while silently breaking fall-through + /// for null and "" — and null is the form the API actually persists. + /// + [TestCase(null)] + [TestCase("")] + [TestCase(" ")] + public void Blank_Custom_Playout_Item_Watermark_Should_Fall_Through_To_A_Resolvable_Channel_Logo(string image) { - ChannelWatermark playoutItemWatermark = Watermark(ChannelWatermarkImageSource.Custom, " "); + ChannelWatermark playoutItemWatermark = Watermark(ChannelWatermarkImageSource.Custom, image); ChannelWatermark channelWatermark = Watermark(ChannelWatermarkImageSource.ChannelLogo); channelWatermark.Id = 8; Channel channel = ChannelWith(LogoStoredPath, channelWatermark); @@ -399,6 +406,39 @@ public class WatermarkSelectorDecoResolutionTests result.IfNone(() => throw new InvalidOperationException()).ImagePath.ShouldBe(LogoCachePath); } + /// + /// Pins the ImageSource is Custom half of the blank-image guard, which nothing else covers. + /// + /// + /// A ChannelLogo watermark's Image is NORMALLY blank — the API persists `Image = null` for + /// every non-`Custom` source — so if the guard's `is Custom` discriminator were dropped, leaving only + /// `IsNullOrWhiteSpace(Image)`, every playout-item `ChannelLogo` watermark would fall through to + /// channel/global instead of resolving the channel's own logo. This test fails on that mutation: the + /// playout-item watermark carries a distinguishing Id, so falling through is observable even though both + /// levels would resolve to the same cached path. + /// + [Test] + public void Blank_Image_ChannelLogo_Playout_Item_Watermark_Should_Win_And_Not_Fall_Through() + { + // Image is left blank, exactly as the API stores a ChannelLogo watermark + ChannelWatermark playoutItemWatermark = Watermark(ChannelWatermarkImageSource.ChannelLogo); + playoutItemWatermark.Id = 42; + + ChannelWatermark channelWatermark = Watermark(ChannelWatermarkImageSource.ChannelLogo); + channelWatermark.Id = 8; + Channel channel = ChannelWith(LogoStoredPath, channelWatermark); + + Option result = Selector(null, LogoCachePath) + .GetWatermarkOptions(channel, playoutItemWatermark, Option.None); + + result.IsSome.ShouldBeTrue(); + WatermarkOptions options = result.IfNone(() => throw new InvalidOperationException()); + options.ImagePath.ShouldBe(LogoCachePath); + + // the PLAYOUT-ITEM watermark won; a fall-through would have returned the channel's (Id 8) + options.Watermark.Id.ShouldBe(42); + } + /// /// `CreateWatermarkHandler`/`UpdateWatermarkHandler` write `Image = null` for every non-`Custom` /// watermark, so an API-created `Resource` watermark hits `Path.Combine(folder, null)` — an diff --git a/docs/decisions/records/ffmpeg/watermark-resolution-unified.md b/docs/decisions/records/ffmpeg/watermark-resolution-unified.md index 42742ae4e..8732c32e6 100644 --- a/docs/decisions/records/ffmpeg/watermark-resolution-unified.md +++ b/docs/decisions/records/ffmpeg/watermark-resolution-unified.md @@ -91,12 +91,26 @@ a neighbour's behavior. The parity test cases are the structural guard: they assert deco and channel-level resolution return identical paths case for case, so re-introducing a per-caller policy fails in CI rather than in prod. -**Measured mutation sensitivity: 19 of the 29 cases fail when the pre-fix resolver is restored under the -final fixture.** The other 10 pass both ways by design — they pin behavior this change deliberately -*preserves* (the blank-`Custom` fall-through, file-present resolution) plus the positive control, so -passing before and after is the correct outcome for them, not a gap. This figure is worth re-measuring -rather than trusting if the fixture changes: the review of this very change found two tests that had -been added to close review findings yet could not fail — one whose fall-through fallback was itself -unresolvable, and one that asserted a filtered list length while the routing claim it was cited for -lived in a function it never called. Both are fixed; the lesson is that a test added under review -pressure needs the same negative control as the original. +**Measured mutation sensitivity: 19 of the 32 cases fail when the pre-fix resolver is restored under the +final fixture.** The other 13 pass both ways by design — they pin behavior this change deliberately +*preserves* (the blank-`Custom` fall-through, file-present resolution, the `is Custom` discriminator) +plus the positive control, so passing before and after is the correct outcome for them, not a gap. + +Two guard mutations were run individually, because a whole-file revert cannot show that a test aimed at +a specific clause actually hits it: + +| Mutation | Caught by | +|---|---| +| drop `ImageSource is Custom` from the blank guard | `Blank_Image_ChannelLogo_Playout_Item_Watermark_Should_Win_And_Not_Fall_Through` (1 failure, and the only test that catches it) | +| narrow `IsNullOrWhiteSpace` to `== " "` | the `null` and `""` cases of `Blank_Custom_Playout_Item_Watermark_Should_Fall_Through_To_A_Resolvable_Channel_Logo` | + +That first mutation matters more than it looks: a `ChannelLogo` watermark's `Image` is *normally* blank, +so dropping the discriminator would send every playout-item `ChannelLogo` watermark down the +fall-through path instead of resolving the channel's own logo. + +Re-measure rather than trust these figures if the fixture changes. Review of this very change found +**two tests that had been added to close earlier review findings yet could not fail** — one whose +fall-through fallback was itself unresolvable (so a wrongly-widened guard still yielded `None`), and one +that asserted a filtered list length while the routing claim it was cited for lived in a function it +never called. The lesson is that a test added under review pressure needs the same negative control as +the original, and that a test's *name* is not evidence it pins what it claims. -- 2.47.3