From b305e6e896214f926ae4fec32b74f59b95eb1abb Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Sat, 5 Sep 2026 19:51:40 +1000 Subject: [PATCH] Release AV1 frame state when neighbor context construction fails --- HEIF_IMPLEMENTATION_PLAN.md | 23 +++++ .../Formats/Heif/Av1/Tiling/Av1TileReader.cs | 23 ++--- .../Formats/Heif/Av1/Av1TilingTests.cs | 89 +++++++++++++++++++ 3 files changed, 124 insertions(+), 11 deletions(-) diff --git a/HEIF_IMPLEMENTATION_PLAN.md b/HEIF_IMPLEMENTATION_PLAN.md index 54f6ddf140..4d83fd1e93 100644 --- a/HEIF_IMPLEMENTATION_PLAN.md +++ b/HEIF_IMPLEMENTATION_PLAN.md @@ -96,6 +96,29 @@ Reference-edge correction checkpoint `578ec34d9`, verified on 2026-09-05: output parity, complete encoder control flow, or decoder-wide conformance. No benchmark was run. Temporary launch scripts, native comparison output, logs, and reports remain local and are excluded from commits. +Tile-reader construction ownership correction, verified on 2026-09-05: + +- At checkpoint `330d4c4ea`, `Av1TileReader.cs:279-316` acquired frame syntax storage before constructing + above/left neighbor contexts. Failure in the above constructor bypassed cleanup; failure in the left + constructor returned only the above context. Both leaked the completed `FrameInfo` owner. This is a + demonstrated lifetime defect, independent of reconstruction quality and encoder search completeness. +- Reference `av1/common/alloccommon.c:370-408,411-452` keeps partial above-context allocations reachable + from common state; decoder destruction calls `av1_remove_common` (`av1/decoder/decoder.c:239`), which + frees those contexts (`alloccommon.c:501-506`). Native contexts are reused until dimensions outgrow them + (`av1/decoder/decodeframe.c:5125-5135`); the managed per-frame allocation lifetime remains a separate + architectural deviation. This correction only restores cleanup at the existing managed owning boundary. +- The new regression failed when the ninth allocator request was rejected: all eight successful frame-state + allocations had no matching return (`tile-ownership-red.trx`). Serialized VSTest stopped on that failure. + The existing first catch now also covers above-context construction, and the left-context catch returns + both preceding owners. No new buffer, owner, guard, or native dependency was introduced. +- Four standalone-reader cases cover monochrome/color and 64/128 superblocks, reject every allocator request + in turn, and require exactly one return per successful allocation. After the final edit, the focused tiling, + reference-store, reference-motion, and decoder conformance set passed **137/137** through serialized + Release .NET 11 Visual Studio VSTest in 1.8500 minutes (`tile-ownership-final.trx`). The final build had + zero warnings/errors; Roslynk reported zero compiler errors. Reports remain in the temporary directory + `D:\GitHub\ynse01\av1-takeover-20260905`, outside the repository. No benchmark was run; this does not + establish separate-encoder sample parity or complete decoder ownership/reference-lifetime equivalence. + Sequence-construction ownership correction, verified subsequently on 2026-09-05: - At checkpoint `578ec34d9`, `Av1FrameEncoder.cs:1492-1560,1668-1713,1778-1824` allocated common state and diff --git a/src/ImageSharp/Formats/Heif/Av1/Tiling/Av1TileReader.cs b/src/ImageSharp/Formats/Heif/Av1/Tiling/Av1TileReader.cs index 8fdb67afa5..f4052b7b53 100644 --- a/src/ImageSharp/Formats/Heif/Av1/Tiling/Av1TileReader.cs +++ b/src/ImageSharp/Formats/Heif/Av1/Tiling/Av1TileReader.cs @@ -272,35 +272,36 @@ internal sealed class Av1TileReader : IAv1TileReader, IDisposable } } + // Above contexts span the aligned frame width, while left contexts are reused for each superblock row. + int planesCount = sequenceHeader.ColorConfig.PlaneCount; + int modeInfoWideColumnCount = Av1Math.AlignPowerOf2( + frameHeader.ModeInfoColumnCount, + sequenceHeader.SuperblockSizeLog2 - Av1Constants.ModeInfoSizeLog2); + try { this.FrameInfo.InitializeSegmentIds(this.FrameHeader, this.primaryReferenceState); this.FrameInfo.InitializeLoopRestoration(this.SequenceHeader, this.FrameHeader); + this.aboveNeighborContext = new Av1ParseAboveNeighbor4x4Context(configuration, planesCount, modeInfoWideColumnCount); } catch { - // FrameInfo has already rented the active frame's syntax storage. Return every successful rent if - // a later segmentation or restoration allocation prevents this reader from being constructed. + // FrameInfo owns the active frame's syntax storage. Return it if segmentation, restoration, or the + // first neighbor context fails, because no constructed reader reaches the caller's using statement. this.FrameInfo.Dispose(); throw; } - // Above contexts span the aligned frame width, while left contexts are reused for each superblock row. - int planesCount = sequenceHeader.ColorConfig.PlaneCount; - int modeInfoWideColumnCount = Av1Math.AlignPowerOf2( - frameHeader.ModeInfoColumnCount, - sequenceHeader.SuperblockSizeLog2 - Av1Constants.ModeInfoSizeLog2); - - this.aboveNeighborContext = new Av1ParseAboveNeighbor4x4Context(configuration, planesCount, modeInfoWideColumnCount); try { this.leftNeighborContext = new Av1ParseLeftNeighbor4x4Context(configuration, planesCount, sequenceHeader.SuperblockModeInfoSize); } catch { - // The reader is not returned when its second context allocation fails, so release the first rent here - // rather than relying on an owner that the caller cannot reach. + // The second context can fail after both frame state and the above context have acquired owners. + // Neither survives failed construction, so unwind both completed owners here. this.aboveNeighborContext.Dispose(); + this.FrameInfo.Dispose(); throw; } diff --git a/tests/ImageSharp.Tests/Formats/Heif/Av1/Av1TilingTests.cs b/tests/ImageSharp.Tests/Formats/Heif/Av1/Av1TilingTests.cs index b3d1ac8540..61f9aa57e2 100644 --- a/tests/ImageSharp.Tests/Formats/Heif/Av1/Av1TilingTests.cs +++ b/tests/ImageSharp.Tests/Formats/Heif/Av1/Av1TilingTests.cs @@ -12,12 +12,80 @@ using SixLabors.ImageSharp.Formats.Heif.Av1.Tiling; using SixLabors.ImageSharp.Formats.Heif.Av1.Transform; using SixLabors.ImageSharp.Memory; using SixLabors.ImageSharp.PixelFormats; +using SixLabors.ImageSharp.Tests.Memory; namespace SixLabors.ImageSharp.Tests.Formats.Heif.Av1; [Trait("Format", "Avif")] public class Av1TilingTests { + [Theory] + [InlineData(false, false)] + [InlineData(false, true)] + [InlineData(true, false)] + [InlineData(true, true)] + public void ConstructionFailureReturnsEveryAllocation(bool use128x128Superblock, bool monochrome) + { + ObuSequenceHeader sequenceHeader = new() + { + MaxFrameWidth = 128, + MaxFrameHeight = 128, + Use128x128Superblock = use128x128Superblock, + ColorConfig = new ObuColorConfig + { + BitDepth = Av1BitDepth.EightBit, + IsMonochrome = monochrome, + SubSamplingX = true, + SubSamplingY = true + } + }; + ObuFrameHeader frameHeader = new() + { + FrameSize = new ObuFrameSize + { + FrameWidth = 128, + FrameHeight = 128, + SuperResolutionUpscaledWidth = 128, + RenderWidth = 128, + RenderHeight = 128 + }, + ModeInfoColumnCount = 32, + ModeInfoRowCount = 32, + ModeInfoStride = 32 + }; + + Configuration configuration = Configuration.Default.Clone(); + TestMemoryAllocator successfulAllocator = new(); + successfulAllocator.EnableNonThreadSafeLogging(); + configuration.MemoryAllocator = successfulAllocator; + using (Av1TileReader reader = new(configuration, sequenceHeader, frameHeader)) + { + Assert.NotEmpty(successfulAllocator.AllocationLog); + } + + Assert.Equal(successfulAllocator.AllocationLog.Count, successfulAllocator.ReturnLog.Count); + for (int failureIndex = 0; failureIndex < successfulAllocator.AllocationLog.Count; failureIndex++) + { + FailingTileAllocator allocator = new(failureIndex); + configuration.MemoryAllocator = allocator; + + // Fail each actual rent, including nested frame-state and neighbor-context constructors. No reader + // reaches the caller's using statement on failure, so construction must return every completed owner. + InvalidMemoryOperationException exception = Assert.Throws(() => + { + using Av1TileReader reader = new(configuration, sequenceHeader, frameHeader); + }); + + Assert.Equal("Tile allocation failure.", exception.Message); + Assert.Equal(failureIndex, allocator.AllocationLog.Count); + Assert.All( + allocator.AllocationLog, + allocation => Assert.Single(allocator.ReturnLog, returned => returned.AllocationId == allocation.AllocationId)); + + Assert.Equal(allocator.AllocationLog.Count, allocator.ReturnLog.Count); + } + } + /// /// Verifies that frame mode-information indices do not wrap at the unsigned 16-bit boundary. /// @@ -347,4 +415,25 @@ public class Av1TilingTests Assert.Equal(dataSize * 8, bitStreamReader.BitPosition); Assert.Equal(superblockCount, frameDecoder.SuperblockCount); } + + private sealed class FailingTileAllocator : TestMemoryAllocator + { + private readonly int failureIndex; + + public FailingTileAllocator(int failureIndex) + { + this.failureIndex = failureIndex; + this.EnableNonThreadSafeLogging(); + } + + protected override AllocationTrackedMemoryManager AllocateCore(int length, AllocationOptions options) + { + if (this.AllocationLog.Count == this.failureIndex) + { + throw new InvalidMemoryOperationException("Tile allocation failure."); + } + + return base.AllocateCore(length, options); + } + } }