From 182f39ae5e26e988408d48fe1db13022d3a2c035 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Sat, 5 Sep 2026 16:05:21 +1000 Subject: [PATCH] Resolve incompatible AV1 color settings before allocating conversion storage --- HEIF_IMPLEMENTATION_PLAN.md | 36 +++++++++++++++++++ .../Heif/Av1/Pipeline/Av1FrameEncoder.cs | 6 ++-- .../Formats/Heif/HeifEncoderCore.Sequence.cs | 7 ++-- .../Formats/Heif/Av1/Av1EncoderFrameTests.cs | 22 ++++++++++++ .../Formats/Heif/HeifEncoderTests.cs | 29 +++++++++++---- 5 files changed, 90 insertions(+), 10 deletions(-) diff --git a/HEIF_IMPLEMENTATION_PLAN.md b/HEIF_IMPLEMENTATION_PLAN.md index ca562c6f27..50b6152433 100644 --- a/HEIF_IMPLEMENTATION_PLAN.md +++ b/HEIF_IMPLEMENTATION_PLAN.md @@ -191,6 +191,42 @@ Coefficient optimization and evaluation-stage investigation: `av1/encoder/speed_features.c:2709-2776` show usage-specific CPU settings and feature initialization. ImageSharp's 0-10 effort scale has not yet been reconciled with these policies. No new effort mapping is assumed. +Color-conversion boundary correction after checkpoint `f7bd907d6`, verified on 2026-09-05: + +- `HeifEncoderCore.Sequence.cs:74-194` resolved output sampling and preserved reversible YCgCo matrix metadata, + including when default 4:2:0 or requested 4:2:2 was incompatible. The shared converter's established contract + rejects that combination at `HeifColorConversionParameters.cs:265-282`. The public save regression failed + on default sampling with that exact exception (`matrix-fallback-before.trx`). +- PNG and TIFF resolve incompatible options through conversion at `PngEncoderCore.cs:1640-1654` and + `TiffEncoderCore.cs:378-444`. HEIF now extends its existing identity-matrix fallback to incompatible + YCgCo-Re/Ro sampling, converts with BT.601, and writes the matching matrix metadata. Explicit 4:4:4 remains + eligible for the existing reversible operator. This changes encoder option resolution, not decoder acceptance. +- Seven public cases retain the original profile object and values, inspect decoded matrix/range metadata, + and require byte-identical output to the same packed pixels explicitly encoded with the fallback matrix. + The cases include the original identity fallback and YCgCo-Re/Ro with default, 4:2:0, and 4:2:2 sampling. +- A separate lifetime defect existed at `Av1FrameEncoder.cs:1404-1434`: conversion parameters were resolved + after renting row storage. The internal-factory regression confirmed that rejected conversion had already + made one allocator request (`conversion-allocation-before.trx`). Resolution now precedes storage allocation; + the regression requires both allocation and return logs to remain empty. No new guard or owner was added. +- Final Release .NET 11 build: zero reported warnings and errors on the incremental build. Roslynk reports + zero compiler errors. Serialized Visual Studio VSTest with stop-on-failure passes 265/265 affected cases + (`conversion-final.trx` in the local takeover report directory). + The regenerated color and partition streams again match optimized native decoding on all 23,396 samples, + maximum error 0 and zero exceeding one. No benchmark or separate-encoder parity comparison was run. + +Color/output source coverage and remaining limits: + +- `Av1YuvConverter.cs:24-135,175-274` dispatches byte/high-bit-depth, complete/cropped/scaled output, and alpha + through shared HEIF adapters. `HeifPlanarColorConverter.cs:39-320,329-870` was read through both traversals: + conversion owns reusable row scratch, interpolates chroma before matrix conversion, and uses existing + `PixelOperations` packing/unpacking or Rgb48/Rgba64 conversion. +- `HeifColorConverter.Operator.cs:164-431` uses closed semantic operators and descending SIMD widths. + These architecture observations are not proof that every H.273 operator or pixel format is numerically correct. + Independent color-conversion and complete SIMD coverage remain open. +- The reference codec interface exposes native planes and strides (`aom/aom_image.h:284-292`). + The temporary comparison adapter supplies I420 using the managed RGB conversion. Its output cannot independently + validate that RGB conversion, even when both codec decoders agree on native planes. + ### Required completion gates Motion-controller investigation continued after correction checkpoint `578ec34d9`: diff --git a/src/ImageSharp/Formats/Heif/Av1/Pipeline/Av1FrameEncoder.cs b/src/ImageSharp/Formats/Heif/Av1/Pipeline/Av1FrameEncoder.cs index 19cc1f2611..e2db3b5c24 100644 --- a/src/ImageSharp/Formats/Heif/Av1/Pipeline/Av1FrameEncoder.cs +++ b/src/ImageSharp/Formats/Heif/Av1/Pipeline/Av1FrameEncoder.cs @@ -1408,6 +1408,10 @@ internal static class Av1FrameEncoder bool encodeAlpha, bool usesHighBitDepth) { + // Resolve conversion before renting storage: a rejected color description must not strand an owner + // in a constructor that never returns to the sequence encoder's disposal boundary. + this.parameters = Av1YuvConverter.GetConversionParameters(colorConfig, out HeifColorConversionMode mode); + this.colorConverter = HeifColorConverterBase.Create(mode, in this.parameters, colorConfig.IsMonochrome); int subsamplingY = colorConfig.SubSamplingY ? 1 : 0; int componentLength = encodeAlpha ? HeifPlanarAlphaEncoder.GetRowStorageLength(width) @@ -1427,8 +1431,6 @@ internal static class Av1FrameEncoder Memory storage = this.storageOwner.Memory; this.componentMemory = storage[..componentLength]; this.packedMemory = storage.Slice(componentLength, packedFloatLength); - this.parameters = Av1YuvConverter.GetConversionParameters(colorConfig, out HeifColorConversionMode mode); - this.colorConverter = HeifColorConverterBase.Create(mode, in this.parameters, colorConfig.IsMonochrome); this.encodeAlpha = encodeAlpha; this.packedPixelCount = usesHighBitDepth && !encodeAlpha ? width : 0; } diff --git a/src/ImageSharp/Formats/Heif/HeifEncoderCore.Sequence.cs b/src/ImageSharp/Formats/Heif/HeifEncoderCore.Sequence.cs index 7af697ec86..060c2a586e 100644 --- a/src/ImageSharp/Formats/Heif/HeifEncoderCore.Sequence.cs +++ b/src/ImageSharp/Formats/Heif/HeifEncoderCore.Sequence.cs @@ -130,10 +130,13 @@ internal sealed partial class HeifEncoderCore && sourceColorProfile.ColorPrimaries == CicpColorPrimaries.ItuRBt709_6 && sourceColorProfile.TransferCharacteristics == CicpTransferCharacteristics.Iec61966_2_1; + bool reversibleMatrix = sourceColorProfile.MatrixCoefficients is CicpMatrixCoefficients.YCgCoRe or CicpMatrixCoefficients.YCgCoRo; if (sourceColorProfile.MatrixCoefficients == CicpMatrixCoefficients.Unspecified - || (identityMatrix && !legalIdentityMatrix)) + || (identityMatrix && !legalIdentityMatrix) + || (reversibleMatrix && !isMonochrome && chromaSubsampling != HeifChromaSubsampling.Yuv444)) { - // The converter uses BT.601 for unspecified or incompatible identity signaling, so record that actual matrix. + // Packed source pixels can be converted to the requested sampling even when their metadata + // describes a matrix that requires 4:4:4. Use and signal BT.601 without changing source metadata. colorProfile = new CicpProfile( (byte)sourceColorProfile.ColorPrimaries, (byte)sourceColorProfile.TransferCharacteristics, diff --git a/tests/ImageSharp.Tests/Formats/Heif/Av1/Av1EncoderFrameTests.cs b/tests/ImageSharp.Tests/Formats/Heif/Av1/Av1EncoderFrameTests.cs index a645ef9de0..092922eef3 100644 --- a/tests/ImageSharp.Tests/Formats/Heif/Av1/Av1EncoderFrameTests.cs +++ b/tests/ImageSharp.Tests/Formats/Heif/Av1/Av1EncoderFrameTests.cs @@ -621,6 +621,28 @@ public class Av1EncoderFrameTests Assert.Equal(second.Size, decodedSecond.Size); } + [Fact] + public void SequenceEncoderRejectsInvalidConversionBeforeAllocatingStorage() + { + ObuColorConfig colorConfig = CreateColorConfig(Av1BitDepth.TenBit, Av1ColorFormat.Yuv420); + colorConfig.MatrixCoefficients = ObuMatrixCoefficients.YCgCoRe; + Configuration configuration = Configuration.Default.Clone(); + TestMemoryAllocator allocator = new(); + allocator.EnableNonThreadSafeLogging(); + configuration.MemoryAllocator = allocator; + + // This internal factory receives resolved AV1 settings. The shared converter already rejects a + // reversible matrix with subsampling; that rejection must occur before any owner can be stranded. + Assert.Throws(() => + { + using Av1FrameEncoder.SequenceEncoder encoder = Av1FrameEncoder.CreateColorSequenceEncoder( + configuration, 32, 32, colorConfig, 17, 9); + }); + + Assert.Empty(allocator.AllocationLog); + Assert.Empty(allocator.ReturnLog); + } + [Theory] [InlineData(false, EightBit)] [InlineData(false, TenBit)] diff --git a/tests/ImageSharp.Tests/Formats/Heif/HeifEncoderTests.cs b/tests/ImageSharp.Tests/Formats/Heif/HeifEncoderTests.cs index 0bb6e69177..28b7187ebd 100644 --- a/tests/ImageSharp.Tests/Formats/Heif/HeifEncoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Heif/HeifEncoderTests.cs @@ -990,23 +990,32 @@ public class HeifEncoderTests Assert.Equal(Av1BitDepth.TenBit, sequenceHeader.ColorConfig.BitDepth); } - [Fact] - public void Av1SanitizesIncompatibleIdentityMatrixWithoutMutatingSourceMetadata() + [Theory] + [InlineData(CicpMatrixCoefficients.Identity, HeifChromaSubsampling.Yuv420)] + [InlineData(CicpMatrixCoefficients.YCgCoRe, null)] + [InlineData(CicpMatrixCoefficients.YCgCoRe, HeifChromaSubsampling.Yuv420)] + [InlineData(CicpMatrixCoefficients.YCgCoRe, HeifChromaSubsampling.Yuv422)] + [InlineData(CicpMatrixCoefficients.YCgCoRo, null)] + [InlineData(CicpMatrixCoefficients.YCgCoRo, HeifChromaSubsampling.Yuv420)] + [InlineData(CicpMatrixCoefficients.YCgCoRo, HeifChromaSubsampling.Yuv422)] + public void Av1SanitizesIncompatibleMatrixWithoutMutatingSourceMetadata( + CicpMatrixCoefficients matrix, + HeifChromaSubsampling? subsampling) { - using Image image = new(8, 8); - CicpProfile sourceProfile = new(1, 13, 0, false); + using Image image = new(8, 8, new Rgb24(32, 96, 192)); + CicpProfile sourceProfile = new(1, 13, (byte)matrix, false); image.Metadata.CicpProfile = sourceProfile; using MemoryStream stream = new(); HeifEncoder encoder = new() { CompressionMethod = HeifCompressionMethod.Av1, - ChromaSubsampling = HeifChromaSubsampling.Yuv420, + ChromaSubsampling = subsampling, Effort = 0 }; image.Save(stream, encoder); Assert.Same(sourceProfile, image.Metadata.CicpProfile); - Assert.Equal(CicpMatrixCoefficients.Identity, sourceProfile.MatrixCoefficients); + Assert.Equal(matrix, sourceProfile.MatrixCoefficients); Assert.False(sourceProfile.FullRange); stream.Position = 0; @@ -1014,6 +1023,14 @@ public class HeifEncoderTests CicpProfile decodedProfile = Assert.IsType(decoded.Metadata.CicpProfile); Assert.Equal(CicpMatrixCoefficients.ItuRBt601_7_525, decodedProfile.MatrixCoefficients); Assert.False(decodedProfile.FullRange); + + // A fallback must change the actual encoded conversion as well as its metadata. Compare with the + // same packed pixels explicitly encoded using that fallback matrix and the requested sampling. + using Image explicitConversion = image.Clone(); + explicitConversion.Metadata.CicpProfile = new CicpProfile(1, 13, (byte)CicpMatrixCoefficients.ItuRBt601_7_525, false); + using MemoryStream expected = new(); + explicitConversion.Save(expected, encoder); + Assert.Equal(expected.ToArray(), stream.ToArray()); } [Fact]