timothy/ersatztv
1
0
Fork 0
Code Issues 51 Pull Requests 5 Actions Packages Projects Releases 9 Wiki Activity

refactor(525): extract RemoteImageDecodeBudget from ImageElementBase

Browse Source
This commit is contained in:
timothy
2026-07-21 12:00:01 +02:00
parent e450d4983c
commit f795db0731
4 changed files with 117 additions and 138 deletions
Download Patch File Download Diff File Expand all files Collapse all files
ErsatzTV.Core.Tests/Images/RemoteImageDecodeBudgetTests.cs
+52
View File
@@ -0,0 +1,52 @@
using ErsatzTV.Core.Images;
using NUnit.Framework;
using Shouldly;
namespace ErsatzTV.Core.Tests.Images;
[TestFixture]
public class RemoteImageDecodeBudgetTests
{
private static readonly Uri Uri = new("https://example.com/logo.png");
// the product is the real bound: 2500x2500 x600 is affordable on each axis alone but not together
[Test]
public void Should_Reject_Dimensions_And_Frames_Affordable_Alone_But_Not_Together()
{
((long)2500 * 2500).ShouldBeLessThanOrEqualTo(RemoteImageDecodeBudget.MaxRemoteDecodedPixels);
600.ShouldBeLessThanOrEqualTo(RemoteImageDecodeBudget.MaxRemoteFrames);
InvalidOperationException ex = Should.Throw<InvalidOperationException>(
() => RemoteImageDecodeBudget.EnsureDecodeAffordable(2500, 2500, 600, Uri));
ex.Message.ShouldContain("pixel limit");
}
[Test]
public void Should_Reject_Too_Many_Frames_Even_When_Each_Is_Tiny() =>
Should.Throw<InvalidOperationException>(
() => RemoteImageDecodeBudget.EnsureDecodeAffordable(8, 8, RemoteImageDecodeBudget.MaxRemoteFrames + 1, Uri))
.Message.ShouldContain("frame limit");
[Test]
public void Should_Reject_A_Single_Oversized_Frame() =>
Should.Throw<InvalidOperationException>(
() => RemoteImageDecodeBudget.EnsureDimensionsAffordable(30000, 30000, Uri))
.Message.ShouldContain("pixel limit");
[Test]
public void Should_Allow_A_Single_Large_Still_Within_Budget() =>
Should.NotThrow(() => RemoteImageDecodeBudget.EnsureDecodeAffordable(7680, 4320, 1, Uri));
[Test]
public void Should_Charge_At_Least_One_Frame_When_Header_Reports_None() =>
Should.Throw<InvalidOperationException>(
() => RemoteImageDecodeBudget.EnsureDecodeAffordable(30000, 30000, 0, Uri));
[Test]
public void Should_Afford_Fewer_Frames_As_Frames_Get_Larger()
{
RemoteImageDecodeBudget.AffordableFrames(8, 8).ShouldBe(RemoteImageDecodeBudget.MaxRemoteFrames);
RemoteImageDecodeBudget.AffordableFrames(1000, 1000).ShouldBe(50);
RemoteImageDecodeBudget.AffordableFrames(7000, 7000).ShouldBe(1);
}
}
ErsatzTV.Core/Images/RemoteImageDecodeBudget.cs
+54
View File
@@ -0,0 +1,54 @@
namespace ErsatzTV.Core.Images;
/// <summary>
/// The decode-budget policy for a remote image, as pure arithmetic so it can be enforced both at
/// render time (graphics engine) and at save time (logo download) without materializing
/// multi-gigabyte images. Extracted from ImageElementBase for reuse. (ersatztv#525, from #511.)
/// </summary>
public static class RemoteImageDecodeBudget
{
/// <summary>
/// Ceiling on TOTAL decoded pixels — width x height x frames, as one product. Checking
/// dimensions and frame count independently does not bound the decode: a 60 KiB 2500x2500 x600
/// GIF passes both a 50 MP dimension check and a 600 frame check and costs ~14 GiB.
/// </summary>
public const long MaxRemoteDecodedPixels = 50_000_000;
/// <summary>Frame ceiling, a cheap legible guard against absurd counts of tiny frames.</summary>
public const int MaxRemoteFrames = 600;
public static void EnsureDimensionsAffordable(int width, int height, Uri uri)
{
long pixels = (long)width * height;
if (pixels > MaxRemoteDecodedPixels)
{
throw new InvalidOperationException(
$"Remote image {uri} is {width}x{height} ({pixels} pixels), over the "
+ $"{MaxRemoteDecodedPixels} pixel limit");
}
}
public static int AffordableFrames(int width, int height)
{
long perFrame = Math.Max((long)width * height, 1);
return (int)Math.Clamp(MaxRemoteDecodedPixels / perFrame, 1, MaxRemoteFrames);
}
public static void EnsureDecodeAffordable(int width, int height, int frameCount, Uri uri)
{
int frames = Math.Max(frameCount, 1);
if (frames > MaxRemoteFrames)
{
throw new InvalidOperationException(
$"Remote image {uri} has {frames} frames, over the {MaxRemoteFrames} frame limit");
}
long totalPixels = (long)width * height * frames;
if (totalPixels > MaxRemoteDecodedPixels)
{
throw new InvalidOperationException(
$"Remote image {uri} decodes to {width}x{height} x{frames} frames "
+ $"({totalPixels} pixels), over the {MaxRemoteDecodedPixels} pixel limit");
}
}
}
ErsatzTV.Infrastructure.Tests/Streaming/Graphics/RemoteImageDecodeLimitTests.cs
+5 -69
View File
@@ -1,4 +1,5 @@
using System.Buffers.Binary;
using ErsatzTV.Core.Images;
using ErsatzTV.Infrastructure.Streaming.Graphics;
using NUnit.Framework;
using Shouldly;
@@ -51,7 +52,7 @@ public class RemoteImageDecodeLimitTests
// 10000 x 5000 = 50,000,000 -- exactly the budget, so it must NOT be rejected. the decode
// then fails on the truncated body, which proves the check let it through. NOTE this test
// would also pass with the guard deleted entirely; deletion is covered by the bomb test
// above, and the boundary arithmetic by EnsureDecodeAffordable's own tests.
// above, and the boundary arithmetic by RemoteImageDecodeBudgetTests (ErsatzTV.Core.Tests).
await using MemoryStream stream = PngHeaderDeclaring(10000, 5000);
Exception ex = await Should.ThrowAsync<Exception>(
@@ -60,77 +61,12 @@ public class RemoteImageDecodeLimitTests
ex.Message.ShouldNotContain("pixel limit");
}
// --- the budget policy itself, tested as arithmetic so no multi-GB image is ever allocated ---
// THE bomb the first fix missed: 2500x2500 x600 frames is ~60 KiB on the wire, passes a
// dimensions-only check (6.25 MP) AND a frames-only check (exactly 600), and costs ~14 GiB to
// decode. Only the PRODUCT catches it. (Found by adversarial re-review; ersatztv#511.)
[Test]
public void Should_Reject_Dimensions_And_Frames_That_Are_Affordable_Alone_But_Not_Together()
{
const int Width = 2500;
const int Height = 2500;
const int Frames = 600;
// each guard, in isolation, says yes
((long)Width * Height).ShouldBeLessThanOrEqualTo(ImageElementBase.MaxRemoteDecodedPixels);
Frames.ShouldBeLessThanOrEqualTo(ImageElementBase.MaxRemoteFrames);
InvalidOperationException ex = Should.Throw<InvalidOperationException>(
() => ImageElementBase.EnsureDecodeAffordable(Width, Height, Frames, ImageUri));
ex.Message.ShouldContain("pixel limit");
}
// M1: the frame guard had no coverage at all in the first fix
[Test]
public void Should_Reject_Too_Many_Frames_Even_When_Each_Is_Tiny()
{
InvalidOperationException ex = Should.Throw<InvalidOperationException>(
() => ImageElementBase.EnsureDecodeAffordable(8, 8, ImageElementBase.MaxRemoteFrames + 1, ImageUri));
ex.Message.ShouldContain("frame limit");
}
[Test]
public void Should_Allow_A_Single_Large_Still_Within_Budget()
{
// 8K is ~33 MP -- must keep working
Should.NotThrow(() => ImageElementBase.EnsureDecodeAffordable(7680, 4320, 1, ImageUri));
}
[Test]
public void Should_Allow_A_Typical_Animated_Logo()
{
Should.NotThrow(() => ImageElementBase.EnsureDecodeAffordable(288, 288, 600, ImageUri));
}
[Test]
public void Should_Afford_Fewer_Frames_As_Frames_Get_Larger()
{
// tiny frames are capped by the frame guard, not the pixel budget
ImageElementBase.AffordableFrames(8, 8).ShouldBe(ImageElementBase.MaxRemoteFrames);
// 1000x1000 -> 50M / 1M = 50 frames
ImageElementBase.AffordableFrames(1000, 1000).ShouldBe(50);
// a frame so large only one fits
ImageElementBase.AffordableFrames(7000, 7000).ShouldBe(1);
}
[Test]
public void Should_Allow_A_Product_Exactly_At_The_Budget()
{
// 10000 x 5000 x 1 == MaxRemoteDecodedPixels exactly
Should.NotThrow(() => ImageElementBase.EnsureDecodeAffordable(10000, 5000, 1, ImageUri));
}
// the retained-frame budget is INDEPENDENT of the decode budget: this source is trivial to
// decode (6 MP total) but retains ~5 GB of SKBitmap once every frame is scaled to 1080p
[Test]
public void Should_Reject_Cheap_Frames_That_Are_Expensive_Once_Scaled()
{
Should.NotThrow(() => ImageElementBase.EnsureDecodeAffordable(100, 100, 600, ImageUri));
Should.NotThrow(() => RemoteImageDecodeBudget.EnsureDecodeAffordable(100, 100, 600, ImageUri));
InvalidOperationException ex = Should.Throw<InvalidOperationException>(
() => ImageElementBase.EnsureScaledFramesAffordable(600, 1920, 1080, ImageUri));
@@ -156,13 +92,13 @@ public class RemoteImageDecodeLimitTests
[Test]
public async Task Should_Reject_An_Animation_Whose_Header_Under_Reports_Its_Frames()
{
await using MemoryStream stream = Apng(64, 64, ImageElementBase.MaxRemoteFrames + 100);
await using MemoryStream stream = Apng(64, 64, RemoteImageDecodeBudget.MaxRemoteFrames + 100);
// the premise: the header really does under-report, so a header-derived budget waves it through
stream.Position = 0;
ImageInfo info = await Image.IdentifyAsync(stream);
info.FrameMetadataCollection.Count.ShouldBe(0, "the APNG header under-reports; that is the whole point");
Should.NotThrow(() => ImageElementBase.EnsureDecodeAffordable(64, 64, info.FrameMetadataCollection.Count, ImageUri));
Should.NotThrow(() => RemoteImageDecodeBudget.EnsureDecodeAffordable(64, 64, info.FrameMetadataCollection.Count, ImageUri));
stream.Position = 0;
InvalidOperationException ex = await Should.ThrowAsync<InvalidOperationException>(
ErsatzTV.Infrastructure/Streaming/Graphics/Image/ImageElementBase.cs
+6 -69
View File
@@ -1,5 +1,6 @@
using System.Runtime.InteropServices;
using ErsatzTV.Core.Domain;
using ErsatzTV.Core.Images;
using ErsatzTV.Core.Interfaces.Streaming;
using ErsatzTV.FFmpeg.State;
using SixLabors.ImageSharp;
@@ -17,24 +18,10 @@ namespace ErsatzTV.Infrastructure.Streaming.Graphics;
public abstract class ImageElementBase(IRemoteImageFetcher remoteImageFetcher) : GraphicsElement, IDisposable
{
/// <summary>
/// Ceiling on TOTAL decoded pixels for a remote image — width x height x frames, as one
/// product. Checking dimensions and frame count independently does not bound the decode:
/// 2500x2500 x 600 frames is 60 KiB on the wire, passes a 50 MP dimension check and a 600
/// frame check, and costs ~14 GiB to decode. Only the product catches that.
/// 8K is ~33 MP, so a single large still fits comfortably.
/// </summary>
internal const long MaxRemoteDecodedPixels = 50_000_000;
/// <summary>
/// Frame ceiling for a remote animation, kept alongside the product budget as a cheap,
/// legible guard against absurd frame counts of tiny frames.
/// </summary>
internal const int MaxRemoteFrames = 600;
/// <summary>
/// Ceiling on total pixels RETAINED after scaling — frames x scaled width x scaled height.
/// Independent of the source budget above: a 100x100 source is trivial to decode but, at 600
/// Independent of the source decode budget in <see cref="RemoteImageDecodeBudget" />: a
/// 100x100 source is trivial to decode but, at 600
/// frames scaled to 1920x1080, retains ~5 GB of <see cref="SKBitmap" />. At 4 bytes per
/// pixel this bounds retention at ~800 MB, which still allows ~96 full-frame 1080p frames
/// (~3s at 30fps) or 600 frames of a 577x577 logo.
@@ -170,9 +157,9 @@ public abstract class ImageElementBase(IRemoteImageFetcher remoteImageFetcher) :
// FrameMetadataCollection.Count == 0 while the decoder happily produces 600 frames, so a
// header-derived frame budget is enforced on a number the decoder does not honor — a
// 134 KiB file decodes to ~36 GiB. (Second adversarial re-review; ersatztv#511.)
EnsureDimensionsAffordable(info.Width, info.Height, uri);
RemoteImageDecodeBudget.EnsureDimensionsAffordable(info.Width, info.Height, uri);
int affordableFrames = AffordableFrames(info.Width, info.Height);
int affordableFrames = RemoteImageDecodeBudget.AffordableFrames(info.Width, info.Height);
stream.Position = 0;
@@ -190,7 +177,7 @@ public abstract class ImageElementBase(IRemoteImageFetcher remoteImageFetcher) :
{
// re-verify against REALITY rather than against the header. this is the check that
// actually holds; everything above it only avoids decoding when we can tell in advance.
EnsureDecodeAffordable(image.Width, image.Height, image.Frames.Count, uri);
RemoteImageDecodeBudget.EnsureDecodeAffordable(image.Width, image.Height, image.Frames.Count, uri);
return image;
}
catch
@@ -200,56 +187,6 @@ public abstract class ImageElementBase(IRemoteImageFetcher remoteImageFetcher) :
}
}
/// <summary>Rejects a single frame that cannot fit the decode budget on its own.</summary>
internal static void EnsureDimensionsAffordable(int width, int height, Uri uri)
{
long pixels = (long)width * height;
if (pixels > MaxRemoteDecodedPixels)
{
throw new InvalidOperationException(
$"Remote image {uri} is {width}x{height} ({pixels} pixels), over the "
+ $"{MaxRemoteDecodedPixels} pixel limit");
}
}
/// <summary>
/// How many frames of this size the decode budget affords. Used to cap the DECODER, so the
/// bound does not depend on the header's frame count being honest.
/// </summary>
internal static int AffordableFrames(int width, int height)
{
long perFrame = Math.Max((long)width * height, 1);
return (int)Math.Clamp(MaxRemoteDecodedPixels / perFrame, 1, MaxRemoteFrames);
}
/// <summary>
/// The decode-budget policy, kept free of I/O so the arithmetic can be tested at every
/// boundary without materializing multi-gigabyte images. Call this with the number of frames
/// the decoder ACTUALLY produced — never with a header-reported count, which can be zero for
/// an animation the decoder then expands to hundreds of frames.
/// </summary>
internal static void EnsureDecodeAffordable(int width, int height, int frameCount, Uri uri)
{
int frames = Math.Max(frameCount, 1);
if (frames > MaxRemoteFrames)
{
throw new InvalidOperationException(
$"Remote image {uri} has {frames} frames, over the {MaxRemoteFrames} frame limit");
}
// THE PRODUCT is the real bound. Checking dimensions and frames separately lets a 60 KiB
// 2500x2500 x600 GIF through at a ~14 GiB decode cost. (Found by adversarial re-review of
// the first fix for this, which checked them independently.)
long totalPixels = (long)width * height * frames;
if (totalPixels > MaxRemoteDecodedPixels)
{
throw new InvalidOperationException(
$"Remote image {uri} decodes to {width}x{height} x{frames} frames "
+ $"({totalPixels} pixels), over the {MaxRemoteDecodedPixels} pixel limit");
}
}
/// <summary>
/// Bounds what is RETAINED after scaling. Separate from the source budget because the two
/// are independent: a cheap-to-decode 100x100 source scaled to 1920x1080 across 600 frames