Fix review

src/ImageSharp/Formats/Png/Chunks/AnimationControl.cs

@@ -5,21 +5,25 @@ using System.Buffers.Binary;
 
 namespace SixLabors.ImageSharp.Formats.Png.Chunks;
 
-internal record AnimationControl(
-    int NumberFrames,
-    int NumberPlays)
+internal readonly struct AnimationControl
 {
     public const int Size = 8;
 
+    public AnimationControl(int numberFrames, int numberPlays)
+    {
+        this.NumberFrames = numberFrames;
+        this.NumberPlays = numberPlays;
+    }
+
     /// <summary>
     /// Gets the number of frames
     /// </summary>
-    public int NumberFrames { get; } = NumberFrames;
+    public int NumberFrames { get; }
 
     /// <summary>
     /// Gets the number of times to loop this APNG.  0 indicates infinite looping.
     /// </summary>
-    public int NumberPlays { get; } = NumberPlays;
+    public int NumberPlays { get; }
 
     /// <summary>
     /// Writes the acTL to the given buffer.
@@ -38,6 +42,6 @@ internal record AnimationControl(
     /// <returns>The parsed acTL.</returns>
     public static AnimationControl Parse(ReadOnlySpan<byte> data)
         => new(
-            NumberFrames: BinaryPrimitives.ReadInt32BigEndian(data[..4]),
-            NumberPlays: BinaryPrimitives.ReadInt32BigEndian(data[4..8]));
+            numberFrames: BinaryPrimitives.ReadInt32BigEndian(data[..4]),
+            numberPlays: BinaryPrimitives.ReadInt32BigEndian(data[4..8]));
 }

src/ImageSharp/Formats/Png/Chunks/FrameControl.cs

@@ -86,32 +86,32 @@ internal readonly struct FrameControl
     {
         if (this.XOffset < 0)
         {
-            throw new NotSupportedException($"Invalid XOffset. Expected >= 0. Was '{this.XOffset}'.");
+            PngThrowHelper.ThrowInvalidParameter(this.XOffset, "Expected >= 0");
         }
 
         if (this.YOffset < 0)
         {
-            throw new NotSupportedException($"Invalid YOffset. Expected >= 0. Was '{this.YOffset}'.");
+            PngThrowHelper.ThrowInvalidParameter(this.YOffset, "Expected >= 0");
         }
 
         if (this.Width <= 0)
         {
-            throw new NotSupportedException($"Invalid Width. Expected > 0. Was '{this.Width}'.");
+            PngThrowHelper.ThrowInvalidParameter(this.Width, "Expected > 0");
         }
 
         if (this.Height <= 0)
         {
-            throw new NotSupportedException($"Invalid Height. Expected > 0. Was '{this.Height}'.");
+            PngThrowHelper.ThrowInvalidParameter(this.Height, "Expected > 0");
         }
 
         if (this.XOffset + this.Width > hdr.Width)
         {
-            throw new NotSupportedException($"Invalid XOffset or Width. The sum > PngHeader.Width. Was '{this.XOffset + this.Width}'.");
+            PngThrowHelper.ThrowInvalidParameter(this.XOffset, this.Width, $"The sum > {nameof(PngHeader)}.{nameof(PngHeader.Width)}");
         }
 
         if (this.YOffset + this.Height > hdr.Height)
         {
-            throw new NotSupportedException($"Invalid YOffset or Height. The sum > PngHeader.Height. Was '{this.YOffset + this.Height}'.");
+            PngThrowHelper.ThrowInvalidParameter(this.YOffset, this.Height, "The sum > PngHeader.Height");
         }
     }
 

src/ImageSharp/Formats/Png/PngDecoderCore.cs

@@ -34,12 +34,17 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
     private readonly Configuration configuration;
 
     /// <summary>
-    /// Gets or sets a value indicating whether the metadata should be ignored when the image is being decoded.
+    /// Whether the metadata should be ignored when the image is being decoded.
+    /// </summary>
+    private readonly uint maxFrames;
+
+    /// <summary>
+    /// Whether the metadata should be ignored when the image is being decoded.
     /// </summary>
     private readonly bool skipMetadata;
 
     /// <summary>
-    /// Gets or sets a value indicating whether to read the IHDR and tRNS chunks only.
+    /// Whether to read the IHDR and tRNS chunks only.
     /// </summary>
     private readonly bool colorMetadataOnly;
 
@@ -61,7 +66,7 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
     /// <summary>
     /// The png animation control.
     /// </summary>
-    private AnimationControl? animationControl;
+    private AnimationControl animationControl;
 
     /// <summary>
     /// The number of bytes per pixel.
@@ -116,6 +121,7 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
     {
         this.Options = options;
         this.configuration = options.Configuration;
+        this.maxFrames = options.MaxFrames;
         this.skipMetadata = options.SkipMetadata;
         this.memoryAllocator = this.configuration.MemoryAllocator;
     }
@@ -124,6 +130,7 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
     {
         this.Options = options;
         this.colorMetadataOnly = colorMetadataOnly;
+        this.maxFrames = options.MaxFrames;
         this.skipMetadata = true;
         this.configuration = options.Configuration;
         this.memoryAllocator = this.configuration.MemoryAllocator;
@@ -139,6 +146,7 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
     public Image<TPixel> Decode<TPixel>(BufferedReadStream stream, CancellationToken cancellationToken)
         where TPixel : unmanaged, IPixel<TPixel>
     {
+        uint frameCount = 0;
         ImageMetadata metadata = new();
         PngMetadata pngMetadata = metadata.GetPngMetadata();
         this.currentStream = stream;
@@ -174,10 +182,21 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
                             ReadGammaChunk(pngMetadata, chunk.Data.GetSpan());
                             break;
                         case PngChunkType.FrameControl:
+                            ++frameCount;
+                            if (frameCount == this.maxFrames)
+                            {
+                                break;
+                            }
+
                             currentFrame = null;
                             lastFrameControl = this.ReadFrameControlChunk(chunk.Data.GetSpan());
                             break;
                         case PngChunkType.FrameData:
+                            if (frameCount == this.maxFrames)
+                            {
+                                break;
+                            }
+
                             if (image is null)
                             {
                                 PngThrowHelper.ThrowMissingDefaultData();
@@ -290,6 +309,7 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
     /// <inheritdoc/>
     public ImageInfo Identify(BufferedReadStream stream, CancellationToken cancellationToken)
     {
+        uint frameCount = 0;
         ImageMetadata metadata = new();
         PngMetadata pngMetadata = metadata.GetPngMetadata();
         this.currentStream = stream;
@@ -331,9 +351,20 @@ internal sealed class PngDecoderCore : IImageDecoderInternals
                             ReadGammaChunk(pngMetadata, chunk.Data.GetSpan());
                             break;
                         case PngChunkType.FrameControl:
+                            ++frameCount;
+                            if (frameCount == this.maxFrames)
+                            {
+                                break;
+                            }
+
                             lastFrameControl = this.ReadFrameControlChunk(chunk.Data.GetSpan());
                             break;
                         case PngChunkType.FrameData:
+                            if (frameCount == this.maxFrames)
+                            {
+                                break;
+                            }
+
                             if (this.colorMetadataOnly)
                             {
                                 goto EOF;

src/ImageSharp/Formats/Png/PngEncoderCore.cs

@@ -1077,7 +1077,7 @@ internal sealed class PngEncoderCore : IImageEncoderInternals, IDisposable
     {
         int width = this.width;
         int height = this.height;
-        if (pixels.Metadata.TryGetAPngFrameMetadata(out PngFrameMetadata? pngMetadata))
+        if (pixels.Metadata.TryGetPngFrameMetadata(out PngFrameMetadata? pngMetadata))
         {
             width = pngMetadata.Width;
             height = pngMetadata.Height;

src/ImageSharp/Formats/Png/PngScanlineProcessor.cs

@@ -22,67 +22,16 @@ internal static class PngScanlineProcessor
         bool hasTrans,
         L16 luminance16Trans,
         L8 luminanceTrans)
-        where TPixel : unmanaged, IPixel<TPixel>
-    {
-        TPixel pixel = default;
-        ref byte scanlineSpanRef = ref MemoryMarshal.GetReference(scanlineSpan);
-        ref TPixel rowSpanRef = ref MemoryMarshal.GetReference(rowSpan);
-        int scaleFactor = 255 / (ColorNumerics.GetColorCountForBitDepth(header.BitDepth) - 1);
-
-        if (!hasTrans)
-        {
-            if (header.BitDepth == 16)
-            {
-                int o = 0;
-                for (nuint x = 0; x < (uint)header.Width; x++, o += 2)
-                {
-                    ushort luminance = BinaryPrimitives.ReadUInt16BigEndian(scanlineSpan.Slice(o, 2));
-                    pixel.FromL16(Unsafe.As<ushort, L16>(ref luminance));
-                    Unsafe.Add(ref rowSpanRef, x) = pixel;
-                }
-            }
-            else
-            {
-                for (nuint x = 0; x < (uint)header.Width; x++)
-                {
-                    byte luminance = (byte)(Unsafe.Add(ref scanlineSpanRef, x) * scaleFactor);
-                    pixel.FromL8(Unsafe.As<byte, L8>(ref luminance));
-                    Unsafe.Add(ref rowSpanRef, x) = pixel;
-                }
-            }
-
-            return;
-        }
-
-        if (header.BitDepth == 16)
-        {
-            La32 source = default;
-            int o = 0;
-            for (nuint x = 0; x < (uint)header.Width; x++, o += 2)
-            {
-                ushort luminance = BinaryPrimitives.ReadUInt16BigEndian(scanlineSpan.Slice(o, 2));
-                source.L = luminance;
-                source.A = luminance.Equals(luminance16Trans.PackedValue) ? ushort.MinValue : ushort.MaxValue;
-
-                pixel.FromLa32(source);
-                Unsafe.Add(ref rowSpanRef, x) = pixel;
-            }
-        }
-        else
-        {
-            La16 source = default;
-            byte scaledLuminanceTrans = (byte)(luminanceTrans.PackedValue * scaleFactor);
-            for (nuint x = 0; x < (uint)header.Width; x++)
-            {
-                byte luminance = (byte)(Unsafe.Add(ref scanlineSpanRef, x) * scaleFactor);
-                source.L = luminance;
-                source.A = luminance.Equals(scaledLuminanceTrans) ? byte.MinValue : byte.MaxValue;
-
-                pixel.FromLa16(source);
-                Unsafe.Add(ref rowSpanRef, x) = pixel;
-            }
-        }
-    }
+        where TPixel : unmanaged, IPixel<TPixel> =>
+        ProcessInterlacedGrayscaleScanline(
+            header,
+            scanlineSpan,
+            rowSpan,
+            0,
+            1,
+            hasTrans,
+            luminance16Trans,
+            luminanceTrans);
 
     public static void ProcessInterlacedGrayscaleScanline<TPixel>(
         in PngHeader header,
@@ -161,39 +110,15 @@ internal static class PngScanlineProcessor
         Span<TPixel> rowSpan,
         uint bytesPerPixel,
         uint bytesPerSample)
-        where TPixel : unmanaged, IPixel<TPixel>
-    {
-        TPixel pixel = default;
-        ref byte scanlineSpanRef = ref MemoryMarshal.GetReference(scanlineSpan);
-        ref TPixel rowSpanRef = ref MemoryMarshal.GetReference(rowSpan);
-
-        if (header.BitDepth == 16)
-        {
-            La32 source = default;
-            int o = 0;
-            for (nuint x = 0; x < (uint)header.Width; x++, o += 4)
-            {
-                source.L = BinaryPrimitives.ReadUInt16BigEndian(scanlineSpan.Slice(o, 2));
-                source.A = BinaryPrimitives.ReadUInt16BigEndian(scanlineSpan.Slice(o + 2, 2));
-
-                pixel.FromLa32(source);
-                Unsafe.Add(ref rowSpanRef, x) = pixel;
-            }
-        }
-        else
-        {
-            La16 source = default;
-            for (nuint x = 0; x < (uint)header.Width; x++)
-            {
-                nuint offset = x * bytesPerPixel;
-                source.L = Unsafe.Add(ref scanlineSpanRef, offset);
-                source.A = Unsafe.Add(ref scanlineSpanRef, offset + bytesPerSample);
-
-                pixel.FromLa16(source);
-                Unsafe.Add(ref rowSpanRef, x) = pixel;
-            }
-        }
-    }
+        where TPixel : unmanaged, IPixel<TPixel> =>
+        ProcessInterlacedGrayscaleWithAlphaScanline(
+            header,
+            scanlineSpan,
+            rowSpan,
+            0,
+            1,
+            bytesPerPixel,
+            bytesPerSample);
 
     public static void ProcessInterlacedGrayscaleWithAlphaScanline<TPixel>(
         in PngHeader header,
@@ -244,48 +169,14 @@ internal static class PngScanlineProcessor
         Span<TPixel> rowSpan,
         ReadOnlySpan<byte> palette,
         byte[] paletteAlpha)
-        where TPixel : unmanaged, IPixel<TPixel>
-    {
-        if (palette.IsEmpty)
-        {
-            PngThrowHelper.ThrowMissingPalette();
-        }
-
-        TPixel pixel = default;
-        ref byte scanlineSpanRef = ref MemoryMarshal.GetReference(scanlineSpan);
-        ref TPixel rowSpanRef = ref MemoryMarshal.GetReference(rowSpan);
-        ReadOnlySpan<Rgb24> palettePixels = MemoryMarshal.Cast<byte, Rgb24>(palette);
-        ref Rgb24 palettePixelsRef = ref MemoryMarshal.GetReference(palettePixels);
-
-        if (paletteAlpha?.Length > 0)
-        {
-            // If the alpha palette is not null and has one or more entries, this means, that the image contains an alpha
-            // channel and we should try to read it.
-            Rgba32 rgba = default;
-            ref byte paletteAlphaRef = ref MemoryMarshal.GetArrayDataReference(paletteAlpha);
-
-            for (nuint x = 0; x < (uint)header.Width; x++)
-            {
-                uint index = Unsafe.Add(ref scanlineSpanRef, x);
-                rgba.Rgb = Unsafe.Add(ref palettePixelsRef, index);
-                rgba.A = paletteAlpha.Length > index ? Unsafe.Add(ref paletteAlphaRef, index) : byte.MaxValue;
-
-                pixel.FromRgba32(rgba);
-                Unsafe.Add(ref rowSpanRef, x) = pixel;
-            }
-        }
-        else
-        {
-            for (nuint x = 0; x < (uint)header.Width; x++)
-            {
-                int index = Unsafe.Add(ref scanlineSpanRef, x);
-                Rgb24 rgb = Unsafe.Add(ref palettePixelsRef, index);
-
-                pixel.FromRgb24(rgb);
-                Unsafe.Add(ref rowSpanRef, x) = pixel;
-            }
-        }
-    }
+        where TPixel : unmanaged, IPixel<TPixel> =>
+        ProcessInterlacedPaletteScanline(header,
+            scanlineSpan,
+            rowSpan,
+            0,
+            1,
+            palette,
+            paletteAlpha);
 
     public static void ProcessInterlacedPaletteScanline<TPixel>(
         in PngHeader header,
@@ -297,6 +188,11 @@ internal static class PngScanlineProcessor
         byte[] paletteAlpha)
         where TPixel : unmanaged, IPixel<TPixel>
     {
+        if (palette.IsEmpty)
+        {
+            PngThrowHelper.ThrowMissingPalette();
+        }
+
         TPixel pixel = default;
         ref byte scanlineSpanRef = ref MemoryMarshal.GetReference(scanlineSpan);
         ref TPixel rowSpanRef = ref MemoryMarshal.GetReference(rowSpan);
@@ -347,9 +243,9 @@ internal static class PngScanlineProcessor
         TPixel pixel = default;
         ref TPixel rowSpanRef = ref MemoryMarshal.GetReference(rowSpan);
 
-        if (!hasTrans)
+        if (header.BitDepth == 16)
         {
-            if (header.BitDepth == 16)
+            if (!hasTrans)
             {
                 Rgb48 rgb48 = default;
                 int o = 0;
@@ -365,31 +261,27 @@ internal static class PngScanlineProcessor
             }
             else
             {
-                PixelOperations<TPixel>.Instance.FromRgb24Bytes(configuration, scanlineSpan, rowSpan, header.Width);
+                Rgb48 rgb48 = default;
+                Rgba64 rgba64 = default;
+                int o = 0;
+                for (nuint x = 0; x < (uint)header.Width; x++, o += bytesPerPixel)
+                {
+                    rgb48.R = BinaryPrimitives.ReadUInt16BigEndian(scanlineSpan.Slice(o, bytesPerSample));
+                    rgb48.G = BinaryPrimitives.ReadUInt16BigEndian(scanlineSpan.Slice(o + bytesPerSample, bytesPerSample));
+                    rgb48.B = BinaryPrimitives.ReadUInt16BigEndian(scanlineSpan.Slice(o + (2 * bytesPerSample), bytesPerSample));
+
+                    rgba64.Rgb = rgb48;
+                    rgba64.A = rgb48.Equals(rgb48Trans) ? ushort.MinValue : ushort.MaxValue;
+
+                    pixel.FromRgba64(rgba64);
+                    Unsafe.Add(ref rowSpanRef, x) = pixel;
+                }
             }
 
             return;
         }
 
-        if (header.BitDepth == 16)
-        {
-            Rgb48 rgb48 = default;
-            Rgba64 rgba64 = default;
-            int o = 0;
-            for (nuint x = 0; x < (uint)header.Width; x++, o += bytesPerPixel)
-            {
-                rgb48.R = BinaryPrimitives.ReadUInt16BigEndian(scanlineSpan.Slice(o, bytesPerSample));
-                rgb48.G = BinaryPrimitives.ReadUInt16BigEndian(scanlineSpan.Slice(o + bytesPerSample, bytesPerSample));
-                rgb48.B = BinaryPrimitives.ReadUInt16BigEndian(scanlineSpan.Slice(o + (2 * bytesPerSample), bytesPerSample));
-
-                rgba64.Rgb = rgb48;
-                rgba64.A = rgb48.Equals(rgb48Trans) ? ushort.MinValue : ushort.MaxValue;
-
-                pixel.FromRgba64(rgba64);
-                Unsafe.Add(ref rowSpanRef, x) = pixel;
-            }
-        }
-        else
+        if (hasTrans)
         {
             Rgba32 rgba32 = default;
             ReadOnlySpan<Rgb24> rgb24Span = MemoryMarshal.Cast<byte, Rgb24>(scanlineSpan);
@@ -404,6 +296,10 @@ internal static class PngScanlineProcessor
                 Unsafe.Add(ref rowSpanRef, x) = pixel;
             }
         }
+        else
+        {
+            PixelOperations<TPixel>.Instance.FromRgb24Bytes(configuration, scanlineSpan, rowSpan, header.Width);
+        }
     }
 
     public static void ProcessInterlacedRgbScanline<TPixel>(

src/ImageSharp/Formats/Png/PngThrowHelper.cs

@@ -2,6 +2,7 @@
 // Licensed under the Six Labors Split License.
 
 using System.Diagnostics.CodeAnalysis;
+using System.Runtime.CompilerServices;
 
 namespace SixLabors.ImageSharp.Formats.Png;
 
@@ -38,6 +39,14 @@ internal static class PngThrowHelper
     [DoesNotReturn]
     public static void ThrowInvalidChunkCrc(string chunkTypeName) => throw new InvalidImageContentException($"CRC Error. PNG {chunkTypeName} chunk is corrupt!");
 
+    [DoesNotReturn]
+    public static void ThrowInvalidParameter(object value, string message, [CallerArgumentExpression(nameof(value))] string name = "")
+        => throw new NotSupportedException($"Invalid {name}. {message}. Was '{value}'.");
+
+    [DoesNotReturn]
+    public static void ThrowInvalidParameter(object value1, object value2, string message, [CallerArgumentExpression(nameof(value1))] string name1 = "", [CallerArgumentExpression(nameof(value1))] string name2 = "")
+        => throw new NotSupportedException($"Invalid {name1} or {name2}. {message}. Was '{value1}' and '{value2}'.");
+
     [DoesNotReturn]
     public static void ThrowNotSupportedColor() => throw new NotSupportedException("Unsupported PNG color type.");