fix(502): correct the deco-scoping claim, extract + test the routing guard
Build ErsatzTV Image / CI image pin matches docker/ci (pull_request) Successful in 13s
Build ErsatzTV Image / Docs update reminder (pull_request) Successful in 13s
Build ErsatzTV Image / decisions.md append-only (pull_request) Successful in 14s
Build CI Toolchain Image / Build & push CI image (push) Successful in 1m39s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 17s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 16s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 7m56s
Build ErsatzTV Image / Functional E2E (curl contracts) (pull_request) Successful in 15m2s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 17m48s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Build ErsatzTV Image / CI image pin matches docker/ci (pull_request) Successful in 13s
Build ErsatzTV Image / Docs update reminder (pull_request) Successful in 13s
Build ErsatzTV Image / decisions.md append-only (pull_request) Successful in 14s
Build CI Toolchain Image / Build & push CI image (push) Successful in 1m39s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 17s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 16s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 7m56s
Build ErsatzTV Image / Functional E2E (curl contracts) (pull_request) Successful in 15m2s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 17m48s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Adversarial review found a documentation defect, not a code one: both docs/decisions.md and the WatermarkSelector comment asserted the deco path was unaffected by this change. That is true of the *resolution* half and false of the *routing* half. SelectWatermarks puts deco-derived options into the same list the routing guard filters, so a deco watermark whose resolved path is a URL is rerouted to the graphics engine too — including the generated-initials localhost URL, which only the deco path still emits and which plausibly rendered through ffmpeg before. That reroute is intended (routing by what the path is beats routing by provenance, which would drift), so the fix is to say so accurately rather than to narrow the guard. Also records the accepted per-frame cost asymmetry the entry previously argued on correctness grounds alone. The guard is extracted as CanUseFFmpegNativeWatermark so it can be tested directly — review's highest-value gap was that the half of the fix which decides whether pixels appear had no automated coverage, only the one-off live E2E. Nine cases pin it, including the localhost-fallback reroute. Both deferrals now point at real issues instead of an unverifiable "tracked separately": #510 (deco vs precedence-level missing-logo policy) and #511 (remote-fetch hardening — timeout, size cap, redirects, pooling, caching, SSRF). Also pins scheme-case insensitivity in the selector.
This commit is contained in:
@@ -0,0 +1,77 @@
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.FFmpeg;
|
||||
using NUnit.Framework;
|
||||
using Shouldly;
|
||||
|
||||
namespace ErsatzTV.Core.Tests.FFmpeg;
|
||||
|
||||
/// <summary>
|
||||
/// Pins which watermarks ffmpeg may carry natively and which must go to the graphics engine.
|
||||
/// The remote-URL rule is the second half of the #502 fix: resolving the URL is useless if the
|
||||
/// resolved path is then handed to ffmpeg as a bare <c>-i</c> argument.
|
||||
/// </summary>
|
||||
[TestFixture]
|
||||
public class FFmpegNativeWatermarkRoutingTests
|
||||
{
|
||||
private const string LocalPath = "/cache/logos/ab/abc123.png";
|
||||
|
||||
private static WatermarkOptions Options(
|
||||
string imagePath,
|
||||
ChannelWatermarkMode mode = ChannelWatermarkMode.Permanent) =>
|
||||
new(new ChannelWatermark { Id = 1, Name = "wm", Mode = mode }, imagePath, Option<int>.None);
|
||||
|
||||
[Test]
|
||||
public void Local_Path_Single_Permanent_Watermark_Uses_FFmpeg()
|
||||
{
|
||||
FFmpegLibraryProcessService.CanUseFFmpegNativeWatermark(0, [Options(LocalPath)])
|
||||
.ShouldBeTrue();
|
||||
}
|
||||
|
||||
[TestCase("https://cdn.example.com/logos/channel.png")]
|
||||
[TestCase("http://cdn.example.com/logos/channel.png")]
|
||||
public void Remote_Url_Watermark_Goes_To_Graphics_Engine(string url)
|
||||
{
|
||||
FFmpegLibraryProcessService.CanUseFFmpegNativeWatermark(0, [Options(url)])
|
||||
.ShouldBeFalse();
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The generated-initials fallback is a localhost URL. Only the deco path still emits it, and it is
|
||||
/// routed by its resolved path like any other URL — see the #502 entry in docs/decisions.md and #510.
|
||||
/// </summary>
|
||||
[Test]
|
||||
public void Generated_Localhost_Logo_Url_Goes_To_Graphics_Engine()
|
||||
{
|
||||
FFmpegLibraryProcessService
|
||||
.CanUseFFmpegNativeWatermark(0, [Options("http://localhost:8409/iptv/logos/gen?text=Test")])
|
||||
.ShouldBeFalse();
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void Graphics_Elements_Present_Goes_To_Graphics_Engine()
|
||||
{
|
||||
FFmpegLibraryProcessService.CanUseFFmpegNativeWatermark(1, [Options(LocalPath)])
|
||||
.ShouldBeFalse();
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void Multiple_Watermarks_Go_To_Graphics_Engine()
|
||||
{
|
||||
FFmpegLibraryProcessService.CanUseFFmpegNativeWatermark(0, [Options(LocalPath), Options(LocalPath)])
|
||||
.ShouldBeFalse();
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void No_Watermarks_Does_Not_Use_FFmpeg()
|
||||
{
|
||||
FFmpegLibraryProcessService.CanUseFFmpegNativeWatermark(0, []).ShouldBeFalse();
|
||||
}
|
||||
|
||||
[TestCase(ChannelWatermarkMode.Intermittent)]
|
||||
[TestCase(ChannelWatermarkMode.None)]
|
||||
public void Non_Permanent_Watermark_Goes_To_Graphics_Engine(ChannelWatermarkMode mode)
|
||||
{
|
||||
FFmpegLibraryProcessService.CanUseFFmpegNativeWatermark(0, [Options(LocalPath, mode)])
|
||||
.ShouldBeFalse();
|
||||
}
|
||||
}
|
||||
@@ -125,6 +125,27 @@ public class WatermarkSelectorChannelLogoTests
|
||||
options.Watermark.ShouldBe(watermark);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Scheme comparison goes through <see cref="Uri" />, which lower-cases it. Pinned because the fix
|
||||
/// turns on <c>Artwork.IsExternalUrl</c>, and a case-sensitive check would silently fall back to the
|
||||
/// existence-gated branch and re-introduce the defect for an oddly-cased URL.
|
||||
/// </summary>
|
||||
[Test]
|
||||
public void ChannelWatermark_Should_Use_External_Url_Channel_Logo_Regardless_Of_Scheme_Case()
|
||||
{
|
||||
const string UpperCaseUrl = "HTTPS://cdn.example.com/logos/channel.png";
|
||||
ChannelWatermark watermark = ChannelLogoWatermark(2, "Channel");
|
||||
Channel channel = ChannelWithLogo(UpperCaseUrl, watermark);
|
||||
|
||||
Option<WatermarkOptions> result = _selector.GetWatermarkOptions(
|
||||
channel,
|
||||
Option<ChannelWatermark>.None,
|
||||
Option<ChannelWatermark>.None);
|
||||
|
||||
result.IsSome.ShouldBeTrue();
|
||||
result.IfNone(() => throw new InvalidOperationException()).ImagePath.ShouldBe(UpperCaseUrl);
|
||||
}
|
||||
|
||||
// ---- regressions: local-file behavior must not change ---------------------------------------
|
||||
|
||||
[Test]
|
||||
|
||||
@@ -68,6 +68,29 @@ public class FFmpegLibraryProcessService : IFFmpegProcessService
|
||||
_logger = logger;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Whether ffmpeg can carry the watermark itself (a single permanent watermark and no other graphics),
|
||||
/// rather than handing it to the graphics engine.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// A watermark whose resolved path is a remote URL is always refused here (#502). ffmpeg would receive it
|
||||
/// as a bare <c>-i</c> argument — and <c>ffprobe</c> the same string for animation detection — putting an
|
||||
/// unbounded network fetch inside stream startup, with no timeout, redirect or auth handling of ours, and
|
||||
/// with the image's real dimensions never probed. The graphics engine fetches remote images deliberately
|
||||
/// (<c>ImageElementBase.LoadImage</c>) and decodes them for their true size.
|
||||
/// <para>
|
||||
/// This is decided by the resolved <see cref="WatermarkOptions.ImagePath" /> alone, so it applies
|
||||
/// uniformly however the watermark was selected — channel, global, playout item <em>or</em> deco. Only
|
||||
/// `ChannelLogo` watermarks can produce a URL: `Custom` resolves through the image cache and
|
||||
/// `Resource` through the resources folder, so neither is ever rerouted.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
internal static bool CanUseFFmpegNativeWatermark(int graphicsElementCount, List<WatermarkOptions> watermarks) =>
|
||||
graphicsElementCount == 0
|
||||
&& watermarks.Count == 1
|
||||
&& watermarks.All(wm => wm.Watermark.Mode is ChannelWatermarkMode.Permanent
|
||||
&& !Artwork.IsExternalUrl(wm.ImagePath));
|
||||
|
||||
public async Task<PlayoutItemResult> ForPlayoutItem(
|
||||
string ffmpegPath,
|
||||
string ffprobePath,
|
||||
@@ -379,13 +402,7 @@ public class FFmpegLibraryProcessService : IFFmpegProcessService
|
||||
Option<GraphicsEngineContext> graphicsEngineContext = Option<GraphicsEngineContext>.None;
|
||||
List<GraphicsElementContext> graphicsElementContexts = [];
|
||||
|
||||
// use ffmpeg for single permanent watermark, graphics engine for all others.
|
||||
// a remote-URL watermark always takes the graphics engine (#502): that path fetches the image with
|
||||
// HttpClient and reads its real dimensions, whereas this one would hand the URL to ffprobe and
|
||||
// ffmpeg as a bare -i argument, putting an unbounded network fetch inside stream startup.
|
||||
if (graphicsElements.Count == 0 && watermarks.Count == 1
|
||||
&& watermarks.All(wm => wm.Watermark.Mode is ChannelWatermarkMode.Permanent)
|
||||
&& watermarks.All(wm => !Artwork.IsExternalUrl(wm.ImagePath)))
|
||||
if (CanUseFFmpegNativeWatermark(graphicsElements.Count, watermarks))
|
||||
{
|
||||
foreach (var wm in watermarks)
|
||||
{
|
||||
|
||||
@@ -367,10 +367,14 @@ public class WatermarkSelector(
|
||||
customPath,
|
||||
None);
|
||||
case ChannelWatermarkImageSource.ChannelLogo:
|
||||
// deliberately NOT ChannelLogoWatermarkOptions: the deco path has always passed its
|
||||
// resolved path through unchecked, so it already honored external-URL logos and #502 does
|
||||
// not apply here. Aligning its missing-file / no-artwork policy with the three precedence
|
||||
// levels above is a behavior change beyond this fix — tracked separately.
|
||||
// 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<Artwork> maybeLogoArtwork =
|
||||
Optional(channel.Artwork.Find(a => a.ArtworkKind == ArtworkKind.Logo));
|
||||
|
||||
+17
-3
@@ -2499,9 +2499,23 @@ additionally requires a non-URL path.
|
||||
deferred by an earlier entry in this file, so it stays ignored — now behind an explicit comment and a
|
||||
scope-guard test rather than as an accident of the existence check.
|
||||
- The **deco path** (`OptionsForWatermarks` → the private `GetWatermarkOptions`) has always returned its
|
||||
resolved path unchecked, so it already honored external URLs and #502 never applied to it. It keeps its
|
||||
own policy; aligning its missing-file and no-artwork behavior with the three precedence levels is a
|
||||
behavior change in its own right.
|
||||
resolved path unchecked, so #502's `File.Exists` defect never reached it and its *resolution* is unchanged.
|
||||
Aligning its missing-file and no-artwork behavior with the three precedence levels is a behavior change in
|
||||
its own right, tracked as **#510**.
|
||||
|
||||
**The routing change is NOT scoped that way, deliberately.** `CanUseFFmpegNativeWatermark` keys off the
|
||||
resolved `WatermarkOptions.ImagePath` alone, and `SelectWatermarks` puts deco-derived options into the same
|
||||
list — so a *deco* watermark resolving to a URL is rerouted to the graphics engine too, including the
|
||||
generated-initials `http://localhost:…/iptv/logos/gen` URL that the deco path does still pass through.
|
||||
Routing by provenance instead of by what the path actually is would mean deciding the same thing twice and
|
||||
letting the two drift; the URL-aware path is the right one for any URL. Worth knowing when picking up #510:
|
||||
that fallback plausibly rendered through ffmpeg before and now composites through ImageSharp, which this
|
||||
change's live-E2E did not cover.
|
||||
|
||||
**Accepted cost asymmetry.** Graphics-engine compositing is per-frame ImageSharp work rather than ffmpeg's
|
||||
`overlay` filter, so two channels with visually identical bugs now transcode at different cost based only on
|
||||
whether the logo is a URL. The rejected fetch-once-into-the-image-cache alternative would have avoided that;
|
||||
if **#511** (remote-fetch hardening: timeout, size cap, pooling, caching) adds such a cache, this goes with it.
|
||||
|
||||
The three gated levels now share one `ChannelLogoWatermarkOptions` helper so they cannot drift apart
|
||||
again — the duplication is what let the defect exist in triplicate. Covered by
|
||||
|
||||
Reference in New Issue
Block a user