From 51cd84eb2f2354544eacdc8a2db3748daebe2fa2 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 18 Dec 2023 09:05:53 +0000 Subject: [PATCH 01/19] Bump actions/upload-artifact from 3 to 4 Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 3 to 4. - [Release notes](https://github.com/actions/upload-artifact/releases) - [Commits](https://github.com/actions/upload-artifact/compare/v3...v4) --- updated-dependencies: - dependency-name: actions/upload-artifact dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] --- .github/workflows/build-and-test.yml | 2 +- .github/workflows/code-coverage.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index 75bcb8a256..51765be6ea 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -132,7 +132,7 @@ jobs: XUNIT_PATH: .\tests\ImageSharp.Tests # Required for xunit - name: Export Failed Output - uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4 if: failure() with: name: actual_output_${{ runner.os }}_${{ matrix.options.framework }}${{ matrix.options.runtime }}.zip diff --git a/.github/workflows/code-coverage.yml b/.github/workflows/code-coverage.yml index 62b6477ee6..53eef45042 100644 --- a/.github/workflows/code-coverage.yml +++ b/.github/workflows/code-coverage.yml @@ -74,7 +74,7 @@ jobs: XUNIT_PATH: .\tests\ImageSharp.Tests # Required for xunit - name: Export Failed Output - uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4 if: failure() with: name: actual_output_${{ runner.os }}_${{ matrix.options.framework }}${{ matrix.options.runtime }}.zip From 382ee7fb2351979a194684707491a88a22a9070b Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Tue, 19 Mar 2024 00:30:20 +1000 Subject: [PATCH 02/19] Only exit after multiple EOF hits --- .../Jpeg/Components/Decoder/JpegBitReader.cs | 20 +++++++++++++++---- .../Formats/Jpg/JpegDecoderTests.cs | 11 ++++++++++ tests/ImageSharp.Tests/TestImages.cs | 1 + tests/Images/Input/Jpg/issues/Issue2638.jpg | 3 +++ 4 files changed, 31 insertions(+), 4 deletions(-) create mode 100644 tests/Images/Input/Jpg/issues/Issue2638.jpg diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs index 0877dbc922..c2b0cb6e07 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs @@ -22,6 +22,9 @@ internal struct JpegBitReader // Whether there is no more good data to pull from the stream for the current mcu. private bool badData; + // How many times have we hit the eof. + private int eofHitCount; + public JpegBitReader(BufferedReadStream stream) { this.stream = stream; @@ -31,6 +34,7 @@ internal struct JpegBitReader this.MarkerPosition = 0; this.badData = false; this.NoData = false; + this.eofHitCount = 0; } /// @@ -80,6 +84,9 @@ internal struct JpegBitReader [MethodImpl(InliningOptions.ShortMethod)] public bool HasBadMarker() => this.Marker != JpegConstants.Markers.XFF && !this.HasRestartMarker(); + [MethodImpl(InliningOptions.ShortMethod)] + public bool HasEndMarker() => this.Marker == JpegConstants.Markers.EOI; + [MethodImpl(InliningOptions.AlwaysInline)] public void FillBuffer() { @@ -219,11 +226,16 @@ internal struct JpegBitReader // we know we have hit the EOI and completed decoding the scan buffer. if (value == -1 || (this.badData && this.data == 0 && this.stream.Position >= this.stream.Length)) { - // We've encountered the end of the file stream which means there's no EOI marker + // We've passed the end of the file stream which means there's no EOI marker // in the image or the SOS marker has the wrong dimensions set. - this.badData = true; - this.NoData = true; - value = 0; + if (this.eofHitCount > JpegConstants.Huffman.FetchLoop) + { + this.badData = true; + this.NoData = true; + value = 0; + } + + this.eofHitCount++; } return value; diff --git a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs index c8d93f6e9e..6a94a98ac6 100644 --- a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs @@ -364,4 +364,15 @@ public partial class JpegDecoderTests image.DebugSave(provider); image.CompareToOriginal(provider); } + + // https://github.com/SixLabors/ImageSharp/issues/2638 + [Theory] + [WithFile(TestImages.Jpeg.Issues.Issue2638, PixelTypes.Rgba32)] + public void Issue2638_DecodeWorks(TestImageProvider provider) + where TPixel : unmanaged, IPixel + { + using Image image = provider.GetImage(JpegDecoder.Instance); + image.DebugSave(provider); + image.CompareToOriginal(provider); + } } diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index 6be8ff6a68..5da581e52f 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -316,6 +316,7 @@ public static class TestImages public const string HangBadScan = "Jpg/issues/Hang_C438A851.jpg"; public const string Issue2517 = "Jpg/issues/issue2517-bad-d7.jpg"; public const string Issue2067_CommentMarker = "Jpg/issues/issue-2067-comment.jpg"; + public const string Issue2638 = "Jpg/issues/Issue2638.jpg"; public static class Fuzz { diff --git a/tests/Images/Input/Jpg/issues/Issue2638.jpg b/tests/Images/Input/Jpg/issues/Issue2638.jpg new file mode 100644 index 0000000000..f42d67b0e8 --- /dev/null +++ b/tests/Images/Input/Jpg/issues/Issue2638.jpg @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:208d5b0b727bbef120a7e090e020a48f99c9e264c2d3939ba749f8620853c1fe +size 70876 From 45bc54ac4486ada9c0fe3b9ec35f33f23c174ce5 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Tue, 19 Mar 2024 00:31:57 +1000 Subject: [PATCH 03/19] Remove unused code. --- .../Formats/Jpeg/Components/Decoder/JpegBitReader.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs index c2b0cb6e07..babd2ff4df 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs @@ -84,9 +84,6 @@ internal struct JpegBitReader [MethodImpl(InliningOptions.ShortMethod)] public bool HasBadMarker() => this.Marker != JpegConstants.Markers.XFF && !this.HasRestartMarker(); - [MethodImpl(InliningOptions.ShortMethod)] - public bool HasEndMarker() => this.Marker == JpegConstants.Markers.EOI; - [MethodImpl(InliningOptions.AlwaysInline)] public void FillBuffer() { From 8acecdaf3d1d7796c606667f856f79111f2506bb Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Tue, 19 Mar 2024 00:35:23 +1000 Subject: [PATCH 04/19] Update comment --- src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs index babd2ff4df..e71d86a1d9 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs @@ -223,7 +223,7 @@ internal struct JpegBitReader // we know we have hit the EOI and completed decoding the scan buffer. if (value == -1 || (this.badData && this.data == 0 && this.stream.Position >= this.stream.Length)) { - // We've passed the end of the file stream which means there's no EOI marker + // We've hit the end of the file stream more times than allowed which means there's no EOI marker // in the image or the SOS marker has the wrong dimensions set. if (this.eofHitCount > JpegConstants.Huffman.FetchLoop) { From 2067026c0933ced15027db458b9d7d641ed7a65c Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Fri, 15 Mar 2024 20:02:00 +1000 Subject: [PATCH 05/19] Ensure VP8X alpha flag is updated correctly. --- .../Formats/Webp/BitWriter/BitWriterBase.cs | 26 +++++++++++++----- .../Formats/Webp/Chunks/WebpVp8X.cs | 23 +++++++++++++++- .../Formats/Webp/Lossless/Vp8LEncoder.cs | 20 +++++++++----- .../Formats/Webp/Lossy/Vp8Encoder.cs | 24 +++++++++++------ src/ImageSharp/Formats/Webp/RiffHelper.cs | 17 +++++++++++- .../Formats/Webp/WebpChunkParsingUtils.cs | 1 - .../Formats/Webp/WebpDecoderCore.cs | 2 +- .../Formats/Webp/WebpEncoderCore.cs | 27 ++++++++++++------- 8 files changed, 105 insertions(+), 35 deletions(-) diff --git a/src/ImageSharp/Formats/Webp/BitWriter/BitWriterBase.cs b/src/ImageSharp/Formats/Webp/BitWriter/BitWriterBase.cs index 9279926869..39c4beb618 100644 --- a/src/ImageSharp/Formats/Webp/BitWriter/BitWriterBase.cs +++ b/src/ImageSharp/Formats/Webp/BitWriter/BitWriterBase.cs @@ -88,7 +88,8 @@ internal abstract class BitWriterBase /// The color profile. /// Flag indicating, if a alpha channel is present. /// Flag indicating, if an animation parameter is present. - public static void WriteTrunksBeforeData( + /// A or a default instance. + public static WebpVp8X WriteTrunksBeforeData( Stream stream, uint width, uint height, @@ -102,16 +103,19 @@ internal abstract class BitWriterBase RiffHelper.BeginWriteRiffFile(stream, WebpConstants.WebpFourCc); // Write VP8X, header if necessary. + WebpVp8X vp8x = default; bool isVp8X = exifProfile != null || xmpProfile != null || iccProfile != null || hasAlpha || hasAnimation; if (isVp8X) { - WriteVp8XHeader(stream, exifProfile, xmpProfile, iccProfile, width, height, hasAlpha, hasAnimation); + vp8x = WriteVp8XHeader(stream, exifProfile, xmpProfile, iccProfile, width, height, hasAlpha, hasAnimation); if (iccProfile != null) { RiffHelper.WriteChunk(stream, (uint)WebpChunkType.Iccp, iccProfile.ToByteArray()); } } + + return vp8x; } /// @@ -124,10 +128,16 @@ internal abstract class BitWriterBase /// Write the trunks after data trunk. /// /// The stream to write to. - /// The exif profile. + /// The VP8X chunk. + /// Whether to update the chunk. + /// The initial position of the stream before encoding. + /// The EXIF profile. /// The XMP profile. public static void WriteTrunksAfterData( Stream stream, + in WebpVp8X vp8x, + bool updateVp8x, + long initialPosition, ExifProfile? exifProfile, XmpProfile? xmpProfile) { @@ -141,7 +151,7 @@ internal abstract class BitWriterBase RiffHelper.WriteChunk(stream, (uint)WebpChunkType.Xmp, xmpProfile.Data); } - RiffHelper.EndWriteRiffFile(stream, 4); + RiffHelper.EndWriteRiffFile(stream, in vp8x, updateVp8x, initialPosition); } /// @@ -186,19 +196,21 @@ internal abstract class BitWriterBase /// Writes a VP8X header to the stream. /// /// The stream to write to. - /// A exif profile or null, if it does not exist. - /// A XMP profile or null, if it does not exist. + /// An EXIF profile or null, if it does not exist. + /// An XMP profile or null, if it does not exist. /// The color profile. /// The width of the image. /// The height of the image. /// Flag indicating, if a alpha channel is present. /// Flag indicating, if an animation parameter is present. - protected static void WriteVp8XHeader(Stream stream, ExifProfile? exifProfile, XmpProfile? xmpProfile, IccProfile? iccProfile, uint width, uint height, bool hasAlpha, bool hasAnimation) + protected static WebpVp8X WriteVp8XHeader(Stream stream, ExifProfile? exifProfile, XmpProfile? xmpProfile, IccProfile? iccProfile, uint width, uint height, bool hasAlpha, bool hasAnimation) { WebpVp8X chunk = new(hasAnimation, xmpProfile != null, exifProfile != null, hasAlpha, iccProfile != null, width, height); chunk.Validate(MaxDimension, MaxCanvasPixels); chunk.WriteTo(stream); + + return chunk; } } diff --git a/src/ImageSharp/Formats/Webp/Chunks/WebpVp8X.cs b/src/ImageSharp/Formats/Webp/Chunks/WebpVp8X.cs index f781d6114d..f60d0c8374 100644 --- a/src/ImageSharp/Formats/Webp/Chunks/WebpVp8X.cs +++ b/src/ImageSharp/Formats/Webp/Chunks/WebpVp8X.cs @@ -3,7 +3,7 @@ namespace SixLabors.ImageSharp.Formats.Webp.Chunks; -internal readonly struct WebpVp8X +internal readonly struct WebpVp8X : IEquatable { public WebpVp8X(bool hasAnimation, bool hasXmp, bool hasExif, bool hasAlpha, bool hasIcc, uint width, uint height) { @@ -51,6 +51,24 @@ internal readonly struct WebpVp8X /// public uint Height { get; } + public static bool operator ==(WebpVp8X left, WebpVp8X right) => left.Equals(right); + + public static bool operator !=(WebpVp8X left, WebpVp8X right) => !(left == right); + + public override bool Equals(object? obj) => obj is WebpVp8X x && this.Equals(x); + + public bool Equals(WebpVp8X other) + => this.HasAnimation == other.HasAnimation + && this.HasXmp == other.HasXmp + && this.HasExif == other.HasExif + && this.HasAlpha == other.HasAlpha + && this.HasIcc == other.HasIcc + && this.Width == other.Width + && this.Height == other.Height; + + public override int GetHashCode() + => HashCode.Combine(this.HasAnimation, this.HasXmp, this.HasExif, this.HasAlpha, this.HasIcc, this.Width, this.Height); + public void Validate(uint maxDimension, ulong maxCanvasPixels) { if (this.Width > maxDimension || this.Height > maxDimension) @@ -65,6 +83,9 @@ internal readonly struct WebpVp8X } } + public WebpVp8X WithAlpha(bool hasAlpha) + => new(this.HasAnimation, this.HasXmp, this.HasExif, hasAlpha, this.HasIcc, this.Width, this.Height); + public void WriteTo(Stream stream) { byte flags = 0; diff --git a/src/ImageSharp/Formats/Webp/Lossless/Vp8LEncoder.cs b/src/ImageSharp/Formats/Webp/Lossless/Vp8LEncoder.cs index f15cb3eb58..f658e40f6f 100644 --- a/src/ImageSharp/Formats/Webp/Lossless/Vp8LEncoder.cs +++ b/src/ImageSharp/Formats/Webp/Lossless/Vp8LEncoder.cs @@ -236,7 +236,7 @@ internal class Vp8LEncoder : IDisposable /// public Vp8LHashChain HashChain { get; } - public void EncodeHeader(Image image, Stream stream, bool hasAnimation) + public WebpVp8X EncodeHeader(Image image, Stream stream, bool hasAnimation) where TPixel : unmanaged, IPixel { // Write bytes from the bit-writer buffer to the stream. @@ -246,7 +246,8 @@ internal class Vp8LEncoder : IDisposable ExifProfile exifProfile = this.skipMetadata ? null : metadata.ExifProfile; XmpProfile xmpProfile = this.skipMetadata ? null : metadata.XmpProfile; - BitWriterBase.WriteTrunksBeforeData( + // The alpha flag is updated following encoding. + WebpVp8X vp8x = BitWriterBase.WriteTrunksBeforeData( stream, (uint)image.Width, (uint)image.Height, @@ -261,9 +262,11 @@ internal class Vp8LEncoder : IDisposable WebpMetadata webpMetadata = WebpCommonUtils.GetWebpMetadata(image); BitWriterBase.WriteAnimationParameter(stream, webpMetadata.BackgroundColor, webpMetadata.RepeatCount); } + + return vp8x; } - public void EncodeFooter(Image image, Stream stream) + public void EncodeFooter(Image image, in WebpVp8X vp8x, bool hasAlpha, Stream stream, long initialPosition) where TPixel : unmanaged, IPixel { // Write bytes from the bit-writer buffer to the stream. @@ -272,7 +275,9 @@ internal class Vp8LEncoder : IDisposable ExifProfile exifProfile = this.skipMetadata ? null : metadata.ExifProfile; XmpProfile xmpProfile = this.skipMetadata ? null : metadata.XmpProfile; - BitWriterBase.WriteTrunksAfterData(stream, exifProfile, xmpProfile); + bool updateVp8x = hasAlpha && vp8x != default; + WebpVp8X updated = updateVp8x ? vp8x.WithAlpha(true) : vp8x; + BitWriterBase.WriteTrunksAfterData(stream, in updated, updateVp8x, initialPosition, exifProfile, xmpProfile); } /// @@ -284,7 +289,8 @@ internal class Vp8LEncoder : IDisposable /// The frame metadata. /// The to encode the image data to. /// Flag indicating, if an animation parameter is present. - public void Encode(ImageFrame frame, Rectangle bounds, WebpFrameMetadata frameMetadata, Stream stream, bool hasAnimation) + /// A indicating whether the frame contains an alpha channel. + public bool Encode(ImageFrame frame, Rectangle bounds, WebpFrameMetadata frameMetadata, Stream stream, bool hasAnimation) where TPixel : unmanaged, IPixel { // Convert image pixels to bgra array. @@ -323,6 +329,8 @@ internal class Vp8LEncoder : IDisposable { RiffHelper.EndWriteChunk(stream, prevPosition); } + + return hasAlpha; } /// @@ -501,7 +509,7 @@ internal class Vp8LEncoder : IDisposable /// The type of the pixels. /// The frame pixel buffer to convert. /// true, if the image is non opaque. - private bool ConvertPixelsToBgra(Buffer2DRegion pixels) + public bool ConvertPixelsToBgra(Buffer2DRegion pixels) where TPixel : unmanaged, IPixel { bool nonOpaque = false; diff --git a/src/ImageSharp/Formats/Webp/Lossy/Vp8Encoder.cs b/src/ImageSharp/Formats/Webp/Lossy/Vp8Encoder.cs index 6e9e4f9cd0..40d91ecf1a 100644 --- a/src/ImageSharp/Formats/Webp/Lossy/Vp8Encoder.cs +++ b/src/ImageSharp/Formats/Webp/Lossy/Vp8Encoder.cs @@ -310,7 +310,7 @@ internal class Vp8Encoder : IDisposable /// private int MbHeaderLimit { get; } - public void EncodeHeader(Image image, Stream stream, bool hasAlpha, bool hasAnimation) + public WebpVp8X EncodeHeader(Image image, Stream stream, bool hasAlpha, bool hasAnimation) where TPixel : unmanaged, IPixel { // Write bytes from the bitwriter buffer to the stream. @@ -320,7 +320,7 @@ internal class Vp8Encoder : IDisposable ExifProfile exifProfile = this.skipMetadata ? null : metadata.ExifProfile; XmpProfile xmpProfile = this.skipMetadata ? null : metadata.XmpProfile; - BitWriterBase.WriteTrunksBeforeData( + WebpVp8X vp8x = BitWriterBase.WriteTrunksBeforeData( stream, (uint)image.Width, (uint)image.Height, @@ -335,9 +335,11 @@ internal class Vp8Encoder : IDisposable WebpMetadata webpMetadata = WebpCommonUtils.GetWebpMetadata(image); BitWriterBase.WriteAnimationParameter(stream, webpMetadata.BackgroundColor, webpMetadata.RepeatCount); } + + return vp8x; } - public void EncodeFooter(Image image, Stream stream) + public void EncodeFooter(Image image, in WebpVp8X vp8x, bool hasAlpha, Stream stream, long initialPosition) where TPixel : unmanaged, IPixel { // Write bytes from the bitwriter buffer to the stream. @@ -346,7 +348,9 @@ internal class Vp8Encoder : IDisposable ExifProfile exifProfile = this.skipMetadata ? null : metadata.ExifProfile; XmpProfile xmpProfile = this.skipMetadata ? null : metadata.XmpProfile; - BitWriterBase.WriteTrunksAfterData(stream, exifProfile, xmpProfile); + bool updateVp8x = hasAlpha && vp8x != default; + WebpVp8X updated = updateVp8x ? vp8x.WithAlpha(true) : vp8x; + BitWriterBase.WriteTrunksAfterData(stream, in updated, updateVp8x, initialPosition, exifProfile, xmpProfile); } /// @@ -357,9 +361,10 @@ internal class Vp8Encoder : IDisposable /// The stream to encode the image data to. /// The region of interest within the frame to encode. /// The frame metadata. - public void EncodeAnimation(ImageFrame frame, Stream stream, Rectangle bounds, WebpFrameMetadata frameMetadata) - where TPixel : unmanaged, IPixel => - this.Encode(stream, frame, bounds, frameMetadata, true, null); + /// A indicating whether the frame contains an alpha channel. + public bool EncodeAnimation(ImageFrame frame, Stream stream, Rectangle bounds, WebpFrameMetadata frameMetadata) + where TPixel : unmanaged, IPixel + => this.Encode(stream, frame, bounds, frameMetadata, true, null); /// /// Encodes the static image frame to the specified stream. @@ -384,7 +389,8 @@ internal class Vp8Encoder : IDisposable /// The frame metadata. /// Flag indicating, if an animation parameter is present. /// The image to encode from. - private void Encode(Stream stream, ImageFrame frame, Rectangle bounds, WebpFrameMetadata frameMetadata, bool hasAnimation, Image image) + /// A indicating whether the frame contains an alpha channel. + private bool Encode(Stream stream, ImageFrame frame, Rectangle bounds, WebpFrameMetadata frameMetadata, bool hasAnimation, Image image) where TPixel : unmanaged, IPixel { int width = bounds.Width; @@ -514,6 +520,8 @@ internal class Vp8Encoder : IDisposable { encodedAlphaData?.Dispose(); } + + return hasAlpha; } /// diff --git a/src/ImageSharp/Formats/Webp/RiffHelper.cs b/src/ImageSharp/Formats/Webp/RiffHelper.cs index d3862ea8b7..b6318c7486 100644 --- a/src/ImageSharp/Formats/Webp/RiffHelper.cs +++ b/src/ImageSharp/Formats/Webp/RiffHelper.cs @@ -3,6 +3,7 @@ using System.Buffers.Binary; using System.Text; +using SixLabors.ImageSharp.Formats.Webp.Chunks; namespace SixLabors.ImageSharp.Formats.Webp; @@ -107,6 +108,7 @@ internal static class RiffHelper position++; } + // Add the size of the encoded file to the Riff header. BinaryPrimitives.WriteUInt32LittleEndian(buffer, dataSize); stream.Position = sizePosition; stream.Write(buffer); @@ -120,5 +122,18 @@ internal static class RiffHelper return sizePosition; } - public static void EndWriteRiffFile(Stream stream, long sizePosition) => EndWriteChunk(stream, sizePosition); + public static void EndWriteRiffFile(Stream stream, in WebpVp8X vp8x, bool updateVp8x, long sizePosition) + { + EndWriteChunk(stream, sizePosition + 4); + + // Write the VP8X chunk if necessary. + if (updateVp8x) + { + long position = stream.Position; + + stream.Position = sizePosition + 12; + vp8x.WriteTo(stream); + stream.Position = position; + } + } } diff --git a/src/ImageSharp/Formats/Webp/WebpChunkParsingUtils.cs b/src/ImageSharp/Formats/Webp/WebpChunkParsingUtils.cs index 80ffe8a996..07f09d45ea 100644 --- a/src/ImageSharp/Formats/Webp/WebpChunkParsingUtils.cs +++ b/src/ImageSharp/Formats/Webp/WebpChunkParsingUtils.cs @@ -2,7 +2,6 @@ // Licensed under the Six Labors Split License. using System.Buffers.Binary; -using System.Drawing; using SixLabors.ImageSharp.Formats.Webp.BitReader; using SixLabors.ImageSharp.Formats.Webp.Lossy; using SixLabors.ImageSharp.IO; diff --git a/src/ImageSharp/Formats/Webp/WebpDecoderCore.cs b/src/ImageSharp/Formats/Webp/WebpDecoderCore.cs index 2991f355ff..21f0f4946b 100644 --- a/src/ImageSharp/Formats/Webp/WebpDecoderCore.cs +++ b/src/ImageSharp/Formats/Webp/WebpDecoderCore.cs @@ -54,7 +54,7 @@ internal sealed class WebpDecoderCore : IImageDecoderInternals, IDisposable /// /// The flag to decide how to handle the background color in the Animation Chunk. /// - private BackgroundColorHandling backgroundColorHandling; + private readonly BackgroundColorHandling backgroundColorHandling; /// /// Initializes a new instance of the class. diff --git a/src/ImageSharp/Formats/Webp/WebpEncoderCore.cs b/src/ImageSharp/Formats/Webp/WebpEncoderCore.cs index e37c1d1796..d29759f9a1 100644 --- a/src/ImageSharp/Formats/Webp/WebpEncoderCore.cs +++ b/src/ImageSharp/Formats/Webp/WebpEncoderCore.cs @@ -1,6 +1,7 @@ // Copyright (c) Six Labors. // Licensed under the Six Labors Split License. +using SixLabors.ImageSharp.Formats.Webp.Chunks; using SixLabors.ImageSharp.Formats.Webp.Lossless; using SixLabors.ImageSharp.Formats.Webp.Lossy; using SixLabors.ImageSharp.Memory; @@ -143,12 +144,14 @@ internal sealed class WebpEncoderCore : IImageEncoderInternals this.nearLossless, this.nearLosslessQuality); - encoder.EncodeHeader(image, stream, hasAnimation); + long initialPosition = stream.Position; + bool hasAlpha = false; + WebpVp8X vp8x = encoder.EncodeHeader(image, stream, hasAnimation); // Encode the first frame. ImageFrame previousFrame = image.Frames.RootFrame; WebpFrameMetadata frameMetadata = WebpCommonUtils.GetWebpFrameMetadata(previousFrame); - encoder.Encode(previousFrame, previousFrame.Bounds(), frameMetadata, stream, hasAnimation); + hasAlpha |= encoder.Encode(previousFrame, previousFrame.Bounds(), frameMetadata, stream, hasAnimation); if (hasAnimation) { @@ -190,14 +193,14 @@ internal sealed class WebpEncoderCore : IImageEncoderInternals this.nearLossless, this.nearLosslessQuality); - animatedEncoder.Encode(encodingFrame, bounds, frameMetadata, stream, hasAnimation); + hasAlpha |= animatedEncoder.Encode(encodingFrame, bounds, frameMetadata, stream, hasAnimation); previousFrame = currentFrame; previousDisposal = frameMetadata.DisposalMethod; } } - encoder.EncodeFooter(image, stream); + encoder.EncodeFooter(image, in vp8x, hasAlpha, stream, initialPosition); } else { @@ -214,17 +217,20 @@ internal sealed class WebpEncoderCore : IImageEncoderInternals this.spatialNoiseShaping, this.alphaCompression); + long initialPosition = stream.Position; + bool hasAlpha = false; + WebpVp8X vp8x = default; if (image.Frames.Count > 1) { - // TODO: What about alpha here? - encoder.EncodeHeader(image, stream, false, true); + // The alpha flag is updated following encoding. + vp8x = encoder.EncodeHeader(image, stream, false, true); // Encode the first frame. ImageFrame previousFrame = image.Frames.RootFrame; WebpFrameMetadata frameMetadata = WebpCommonUtils.GetWebpFrameMetadata(previousFrame); WebpDisposalMethod previousDisposal = frameMetadata.DisposalMethod; - encoder.EncodeAnimation(previousFrame, stream, previousFrame.Bounds(), frameMetadata); + hasAlpha |= encoder.EncodeAnimation(previousFrame, stream, previousFrame.Bounds(), frameMetadata); // Encode additional frames // This frame is reused to store de-duplicated pixel buffers. @@ -263,18 +269,19 @@ internal sealed class WebpEncoderCore : IImageEncoderInternals this.spatialNoiseShaping, this.alphaCompression); - animatedEncoder.EncodeAnimation(encodingFrame, stream, bounds, frameMetadata); + hasAlpha |= animatedEncoder.EncodeAnimation(encodingFrame, stream, bounds, frameMetadata); previousFrame = currentFrame; previousDisposal = frameMetadata.DisposalMethod; } + + encoder.EncodeFooter(image, in vp8x, hasAlpha, stream, initialPosition); } else { encoder.EncodeStatic(stream, image); + encoder.EncodeFooter(image, in vp8x, hasAlpha, stream, initialPosition); } - - encoder.EncodeFooter(image, stream); } } } From 6733e8d8231f17ceb906f7d971bb7abaa7c01daf Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Sun, 17 Mar 2024 11:05:56 +1000 Subject: [PATCH 06/19] Update WebpVp8X.cs --- src/ImageSharp/Formats/Webp/Chunks/WebpVp8X.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ImageSharp/Formats/Webp/Chunks/WebpVp8X.cs b/src/ImageSharp/Formats/Webp/Chunks/WebpVp8X.cs index f60d0c8374..491f716500 100644 --- a/src/ImageSharp/Formats/Webp/Chunks/WebpVp8X.cs +++ b/src/ImageSharp/Formats/Webp/Chunks/WebpVp8X.cs @@ -66,7 +66,7 @@ internal readonly struct WebpVp8X : IEquatable && this.Width == other.Width && this.Height == other.Height; - public override int GetHashCode() + public override int GetHashCode() => HashCode.Combine(this.HasAnimation, this.HasXmp, this.HasExif, this.HasAlpha, this.HasIcc, this.Width, this.Height); public void Validate(uint maxDimension, ulong maxCanvasPixels) From 5d9e3051cb50c46fca7a3832188c44285e9d8d24 Mon Sep 17 00:00:00 2001 From: SpaceCheetah Date: Thu, 28 Mar 2024 12:33:23 -1000 Subject: [PATCH 07/19] Add test --- tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs | 3 ++- tests/ImageSharp.Tests/TestImages.cs | 1 + .../Decode_VerifyAllFrames_Rgba32_frame-offset.png/00.png | 3 +++ .../Decode_VerifyAllFrames_Rgba32_frame-offset.png/01.png | 3 +++ .../Decode_VerifyAllFrames_Rgba32_frame-offset.png/02.png | 3 +++ .../Decode_VerifyAllFrames_Rgba32_frame-offset.png/03.png | 3 +++ .../Decode_VerifyAllFrames_Rgba32_frame-offset.png/04.png | 3 +++ tests/Images/Input/Png/animated/frame-offset.png | 3 +++ 8 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/00.png create mode 100644 tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/01.png create mode 100644 tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/02.png create mode 100644 tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/03.png create mode 100644 tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/04.png create mode 100644 tests/Images/Input/Png/animated/frame-offset.png diff --git a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs index de99432bce..b6e798c303 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs @@ -87,7 +87,8 @@ public partial class PngDecoderTests TestImages.Png.DisposeBackgroundRegion, TestImages.Png.DisposePreviousFirst, TestImages.Png.DisposeBackgroundBeforeRegion, - TestImages.Png.BlendOverMultiple + TestImages.Png.BlendOverMultiple, + TestImages.Png.FrameOffset }; [Theory] diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index 5da581e52f..1a1a3cd9e6 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -73,6 +73,7 @@ public static class TestImages public const string DisposeBackgroundRegion = "Png/animated/15-dispose-background-region.png"; public const string DisposePreviousFirst = "Png/animated/12-dispose-prev-first.png"; public const string BlendOverMultiple = "Png/animated/21-blend-over-multiple.png"; + public const string FrameOffset = "Png/animated/frame-offset.png"; public const string Issue2666 = "Png/issues/Issue_2666.png"; // Filtered test images from http://www.schaik.com/pngsuite/pngsuite_fil_png.html diff --git a/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/00.png b/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/00.png new file mode 100644 index 0000000000..b9fa24c930 --- /dev/null +++ b/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/00.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:84e2353264e3488122f4d488d7c4b198ff5192ad0c662c7fb0a369c957ecc7ea +size 353 diff --git a/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/01.png b/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/01.png new file mode 100644 index 0000000000..6f3a27187c --- /dev/null +++ b/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/01.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:2916711d3f4d72eb66a5cfc2b40a3318eb4cce5b367658cfc7e3b573fd39cc33 +size 693 diff --git a/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/02.png b/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/02.png new file mode 100644 index 0000000000..50911cce57 --- /dev/null +++ b/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/02.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:7f9d5503414ccefa6b66661b1e93c2c3f6e4491f14af006a71153cecf43b52f5 +size 806 diff --git a/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/03.png b/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/03.png new file mode 100644 index 0000000000..89d2f95706 --- /dev/null +++ b/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/03.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:fe42b7dc6524d5589ad680650f4bcd181319b40b258b31e0932d6e936818e980 +size 570 diff --git a/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/04.png b/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/04.png new file mode 100644 index 0000000000..c3f2b99b8b --- /dev/null +++ b/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/04.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:8002ff5b3451b348f285eec15dd7a093c62d11d8b77c3ead9ac89ca6eb29977d +size 669 diff --git a/tests/Images/Input/Png/animated/frame-offset.png b/tests/Images/Input/Png/animated/frame-offset.png new file mode 100644 index 0000000000..4eebb44a3d --- /dev/null +++ b/tests/Images/Input/Png/animated/frame-offset.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:3c019073841b48b02cb07c779fed8654c6052aee700e7620d07f5d775d97088f +size 2156 From a58ef4bdb70e9421823b3bb4c6d86b28195073c0 Mon Sep 17 00:00:00 2001 From: SpaceCheetah Date: Thu, 28 Mar 2024 13:50:40 -1000 Subject: [PATCH 08/19] Fix ProcessInterlacedPaletteScanline not obeying frameControl.XOffset --- src/ImageSharp/Formats/Png/PngScanlineProcessor.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ImageSharp/Formats/Png/PngScanlineProcessor.cs b/src/ImageSharp/Formats/Png/PngScanlineProcessor.cs index aa937a8e2a..0f530b478e 100644 --- a/src/ImageSharp/Formats/Png/PngScanlineProcessor.cs +++ b/src/ImageSharp/Formats/Png/PngScanlineProcessor.cs @@ -180,8 +180,9 @@ internal static class PngScanlineProcessor ref byte scanlineSpanRef = ref MemoryMarshal.GetReference(scanlineSpan); ref TPixel rowSpanRef = ref MemoryMarshal.GetReference(rowSpan); ref Color paletteBase = ref MemoryMarshal.GetReference(palette.Value.Span); + uint offset = pixelOffset + frameControl.XOffset; - for (nuint x = pixelOffset, o = 0; x < frameControl.XMax; x += increment, o++) + for (nuint x = offset, o = 0; x < frameControl.XMax; x += increment, o++) { uint index = Unsafe.Add(ref scanlineSpanRef, o); Unsafe.Add(ref rowSpanRef, x) = TPixel.FromRgba32(Unsafe.Add(ref paletteBase, index).ToPixel()); From ce069bce2501bccf171bd585e8c854a58ac53687 Mon Sep 17 00:00:00 2001 From: SpaceCheetah Date: Thu, 28 Mar 2024 17:49:33 -1000 Subject: [PATCH 09/19] Fix frame dispose operation handling --- src/ImageSharp/Formats/Png/PngDecoderCore.cs | 25 +++++++++++-------- .../00.png | 4 +-- .../01.png | 4 +-- .../02.png | 4 +-- .../03.png | 4 +-- .../04.png | 4 +-- 6 files changed, 25 insertions(+), 20 deletions(-) diff --git a/src/ImageSharp/Formats/Png/PngDecoderCore.cs b/src/ImageSharp/Formats/Png/PngDecoderCore.cs index 6a321a3ba0..23e3033dcc 100644 --- a/src/ImageSharp/Formats/Png/PngDecoderCore.cs +++ b/src/ImageSharp/Formats/Png/PngDecoderCore.cs @@ -246,8 +246,13 @@ internal sealed class PngDecoderCore : IImageDecoderInternals currentFrameControl.Value, cancellationToken); - previousFrame = currentFrame; - previousFrameControl = currentFrameControl; + // if current frame dispose is restore to previous, then from future frame's perspective, it never happened + if (currentFrameControl.Value.DisposeOperation != PngDisposalMethod.RestoreToPrevious) + { + previousFrame = currentFrame; + previousFrameControl = currentFrameControl; + } + break; case PngChunkType.Data: @@ -645,18 +650,18 @@ internal sealed class PngDecoderCore : IImageDecoderInternals out ImageFrame frame) where TPixel : unmanaged, IPixel { - // We create a clone of the previous frame and add it. - // We will overpaint the difference of pixels on the current frame to create a complete image. - // This ensures that we have enough pixel data to process without distortion. #2450 frame = image.Frames.AddFrame(previousFrame ?? image.Frames.RootFrame); - // If the first `fcTL` chunk uses a `dispose_op` of APNG_DISPOSE_OP_PREVIOUS it should be treated as APNG_DISPOSE_OP_BACKGROUND. - if (previousFrameControl.DisposeOperation == PngDisposalMethod.RestoreToBackground - || (previousFrame is null && previousFrameControl.DisposeOperation == PngDisposalMethod.RestoreToPrevious)) + // if restoring to before first frame, restore to background + if (previousFrame is null && previousFrameControl.DisposeOperation == PngDisposalMethod.RestoreToPrevious) + { + Buffer2DRegion pixelRegion = frame.PixelBuffer.GetRegion(); + pixelRegion.Clear(); + } + else if (previousFrameControl.DisposeOperation == PngDisposalMethod.RestoreToBackground) { Rectangle restoreArea = previousFrameControl.Bounds; - Rectangle interest = Rectangle.Intersect(frame.Bounds(), restoreArea); - Buffer2DRegion pixelRegion = frame.PixelBuffer.GetRegion(interest); + Buffer2DRegion pixelRegion = frame.PixelBuffer.GetRegion(restoreArea); pixelRegion.Clear(); } diff --git a/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/00.png b/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/00.png index b9fa24c930..870ed61a44 100644 --- a/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/00.png +++ b/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/00.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:84e2353264e3488122f4d488d7c4b198ff5192ad0c662c7fb0a369c957ecc7ea -size 353 +oid sha256:b85aaf7153e0ca538856a58d7b069bcc13fadc468ea603c85f8782cc691f86c3 +size 387 diff --git a/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/01.png b/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/01.png index 6f3a27187c..cab85d9466 100644 --- a/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/01.png +++ b/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/01.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:2916711d3f4d72eb66a5cfc2b40a3318eb4cce5b367658cfc7e3b573fd39cc33 -size 693 +oid sha256:fcb83d6893dcfd869b764ff9846c259eaa0caf26cec3f0fc2cbae2c26f2eeaa5 +size 660 diff --git a/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/02.png b/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/02.png index 50911cce57..1a2c5adcf0 100644 --- a/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/02.png +++ b/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/02.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:7f9d5503414ccefa6b66661b1e93c2c3f6e4491f14af006a71153cecf43b52f5 -size 806 +oid sha256:562ec382f6d2af68e66092bf6949f66147d5f608d3c618eea5a7c1ea400737ff +size 768 diff --git a/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/03.png b/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/03.png index 89d2f95706..d850459ee8 100644 --- a/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/03.png +++ b/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/03.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:fe42b7dc6524d5589ad680650f4bcd181319b40b258b31e0932d6e936818e980 -size 570 +oid sha256:d12a7791b960072e32b78bd9aaf456dc99341eea1c66ea05050433d8c082c6ac +size 579 diff --git a/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/04.png b/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/04.png index c3f2b99b8b..000b0567de 100644 --- a/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/04.png +++ b/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_frame-offset.png/04.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:8002ff5b3451b348f285eec15dd7a093c62d11d8b77c3ead9ac89ca6eb29977d -size 669 +oid sha256:2db38d7ffcc95c23a5c94a06f10c6cc67406ae581a955c99ede4af97b1a044f8 +size 628 From b29962abca4758eabed06b526520d82ad31d514c Mon Sep 17 00:00:00 2001 From: SpaceCheetah Date: Thu, 28 Mar 2024 18:14:23 -1000 Subject: [PATCH 10/19] Add test for default image not animated --- tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs | 3 ++- tests/ImageSharp.Tests/TestImages.cs | 1 + .../00.png | 3 +++ .../01.png | 3 +++ tests/Images/Input/Png/animated/default-not-animated.png | 3 +++ 5 files changed, 12 insertions(+), 1 deletion(-) create mode 100644 tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_default-not-animated.png/00.png create mode 100644 tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_default-not-animated.png/01.png create mode 100644 tests/Images/Input/Png/animated/default-not-animated.png diff --git a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs index b6e798c303..152598ac81 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs @@ -88,7 +88,8 @@ public partial class PngDecoderTests TestImages.Png.DisposePreviousFirst, TestImages.Png.DisposeBackgroundBeforeRegion, TestImages.Png.BlendOverMultiple, - TestImages.Png.FrameOffset + TestImages.Png.FrameOffset, + TestImages.Png.DefaultNotAnimated }; [Theory] diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index 1a1a3cd9e6..5c80422dad 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -74,6 +74,7 @@ public static class TestImages public const string DisposePreviousFirst = "Png/animated/12-dispose-prev-first.png"; public const string BlendOverMultiple = "Png/animated/21-blend-over-multiple.png"; public const string FrameOffset = "Png/animated/frame-offset.png"; + public const string DefaultNotAnimated = "Png/animated/default-not-animated.png"; public const string Issue2666 = "Png/issues/Issue_2666.png"; // Filtered test images from http://www.schaik.com/pngsuite/pngsuite_fil_png.html diff --git a/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_default-not-animated.png/00.png b/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_default-not-animated.png/00.png new file mode 100644 index 0000000000..4c5ea8169a --- /dev/null +++ b/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_default-not-animated.png/00.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:8d4716e18655be53630d6d50daebe8c38e0eedb2432c7a73840b55d1473d5944 +size 1050 diff --git a/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_default-not-animated.png/01.png b/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_default-not-animated.png/01.png new file mode 100644 index 0000000000..790fe45e4c --- /dev/null +++ b/tests/Images/External/ReferenceOutput/PngDecoderTests/Decode_VerifyAllFrames_Rgba32_default-not-animated.png/01.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:9b5a6d3cf1a777f6b719c2a1cf79bffe2251355d75e6c0f7ce7a973b3d033419 +size 1177 diff --git a/tests/Images/Input/Png/animated/default-not-animated.png b/tests/Images/Input/Png/animated/default-not-animated.png new file mode 100644 index 0000000000..1ed72698d5 --- /dev/null +++ b/tests/Images/Input/Png/animated/default-not-animated.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:647d484c8f320b55824b9219270524df3edc434a4793e1627e0ee14af8d6e4f8 +size 1689 From 5cd98723dc5944f7f3fc98e4996832683b8ce88a Mon Sep 17 00:00:00 2001 From: SpaceCheetah Date: Thu, 28 Mar 2024 18:28:03 -1000 Subject: [PATCH 11/19] Fix handling of case where default image isn't animated --- src/ImageSharp/Formats/Png/PngDecoderCore.cs | 22 +++++++++++--------- src/ImageSharp/Formats/Png/PngMetadata.cs | 5 +++++ 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/src/ImageSharp/Formats/Png/PngDecoderCore.cs b/src/ImageSharp/Formats/Png/PngDecoderCore.cs index 23e3033dcc..222fe8ed34 100644 --- a/src/ImageSharp/Formats/Png/PngDecoderCore.cs +++ b/src/ImageSharp/Formats/Png/PngDecoderCore.cs @@ -234,8 +234,7 @@ internal sealed class PngDecoderCore : IImageDecoderInternals PngThrowHelper.ThrowMissingFrameControl(); } - previousFrameControl ??= new((uint)this.header.Width, (uint)this.header.Height); - this.InitializeFrame(previousFrameControl.Value, currentFrameControl.Value, image, previousFrame, out currentFrame); + this.InitializeFrame(previousFrameControl, currentFrameControl.Value, image, previousFrame, out currentFrame); this.currentStream.Position += 4; this.ReadScanlines( @@ -255,7 +254,7 @@ internal sealed class PngDecoderCore : IImageDecoderInternals break; case PngChunkType.Data: - + pngMetadata.DefaultImageAnimated = currentFrameControl != null; currentFrameControl ??= new((uint)this.header.Width, (uint)this.header.Height); if (image is null) { @@ -272,9 +271,12 @@ internal sealed class PngDecoderCore : IImageDecoderInternals this.ReadNextDataChunk, currentFrameControl.Value, cancellationToken); + if (pngMetadata.DefaultImageAnimated) + { + previousFrame = currentFrame; + previousFrameControl = currentFrameControl; + } - previousFrame = currentFrame; - previousFrameControl = currentFrameControl; break; case PngChunkType.Palette: this.palette = chunk.Data.GetSpan().ToArray(); @@ -643,7 +645,7 @@ internal sealed class PngDecoderCore : IImageDecoderInternals /// The previous frame. /// The created frame private void InitializeFrame( - FrameControl previousFrameControl, + FrameControl? previousFrameControl, FrameControl currentFrameControl, Image image, ImageFrame? previousFrame, @@ -652,15 +654,15 @@ internal sealed class PngDecoderCore : IImageDecoderInternals { frame = image.Frames.AddFrame(previousFrame ?? image.Frames.RootFrame); - // if restoring to before first frame, restore to background - if (previousFrame is null && previousFrameControl.DisposeOperation == PngDisposalMethod.RestoreToPrevious) + // If restoring to before first frame, restore to background. Same if first frame (previousFrameControl null). + if (previousFrameControl == null || (previousFrame is null && previousFrameControl.Value.DisposeOperation == PngDisposalMethod.RestoreToPrevious)) { Buffer2DRegion pixelRegion = frame.PixelBuffer.GetRegion(); pixelRegion.Clear(); } - else if (previousFrameControl.DisposeOperation == PngDisposalMethod.RestoreToBackground) + else if (previousFrameControl.Value.DisposeOperation == PngDisposalMethod.RestoreToBackground) { - Rectangle restoreArea = previousFrameControl.Bounds; + Rectangle restoreArea = previousFrameControl.Value.Bounds; Buffer2DRegion pixelRegion = frame.PixelBuffer.GetRegion(restoreArea); pixelRegion.Clear(); } diff --git a/src/ImageSharp/Formats/Png/PngMetadata.cs b/src/ImageSharp/Formats/Png/PngMetadata.cs index 93ddcf2636..c4ff3bbe24 100644 --- a/src/ImageSharp/Formats/Png/PngMetadata.cs +++ b/src/ImageSharp/Formats/Png/PngMetadata.cs @@ -83,6 +83,11 @@ public class PngMetadata : IDeepCloneable /// public uint RepeatCount { get; set; } = 1; + /// + /// Gets or sets a value indicating whether the default image is shown as part of the animated sequence + /// + public bool DefaultImageAnimated { get; set; } + /// public IDeepCloneable DeepClone() => new PngMetadata(this); From c23283508205a37e15baad4b55bd83ea5e790138 Mon Sep 17 00:00:00 2001 From: SpaceCheetah Date: Thu, 28 Mar 2024 19:23:35 -1000 Subject: [PATCH 12/19] Fix PngMetadata copy --- src/ImageSharp/Formats/Png/PngMetadata.cs | 3 ++- tests/ImageSharp.Tests/Formats/Png/PngMetadataTests.cs | 4 +++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/ImageSharp/Formats/Png/PngMetadata.cs b/src/ImageSharp/Formats/Png/PngMetadata.cs index c4ff3bbe24..766377f7c9 100644 --- a/src/ImageSharp/Formats/Png/PngMetadata.cs +++ b/src/ImageSharp/Formats/Png/PngMetadata.cs @@ -29,6 +29,7 @@ public class PngMetadata : IDeepCloneable this.InterlaceMethod = other.InterlaceMethod; this.TransparentColor = other.TransparentColor; this.RepeatCount = other.RepeatCount; + this.DefaultImageAnimated = other.DefaultImageAnimated; if (other.ColorTable?.Length > 0) { @@ -86,7 +87,7 @@ public class PngMetadata : IDeepCloneable /// /// Gets or sets a value indicating whether the default image is shown as part of the animated sequence /// - public bool DefaultImageAnimated { get; set; } + public bool DefaultImageAnimated { get; set; } = true; /// public IDeepCloneable DeepClone() => new PngMetadata(this); diff --git a/tests/ImageSharp.Tests/Formats/Png/PngMetadataTests.cs b/tests/ImageSharp.Tests/Formats/Png/PngMetadataTests.cs index b3c122a7a8..4f9ba9abee 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngMetadataTests.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngMetadataTests.cs @@ -32,7 +32,8 @@ public class PngMetadataTests InterlaceMethod = PngInterlaceMode.Adam7, Gamma = 2, TextData = new List { new PngTextData("name", "value", "foo", "bar") }, - RepeatCount = 123 + RepeatCount = 123, + DefaultImageAnimated = false }; PngMetadata clone = (PngMetadata)meta.DeepClone(); @@ -44,6 +45,7 @@ public class PngMetadataTests Assert.False(meta.TextData.Equals(clone.TextData)); Assert.True(meta.TextData.SequenceEqual(clone.TextData)); Assert.True(meta.RepeatCount == clone.RepeatCount); + Assert.True(meta.DefaultImageAnimated == clone.DefaultImageAnimated); clone.BitDepth = PngBitDepth.Bit2; clone.ColorType = PngColorType.Palette; From 5cd2d290526942a9f4a60dcc12fc8db03f7aa194 Mon Sep 17 00:00:00 2001 From: SpaceCheetah Date: Thu, 28 Mar 2024 20:11:43 -1000 Subject: [PATCH 13/19] Make PngEncoder respect DefaultImageAnimated --- src/ImageSharp/Formats/Png/PngEncoderCore.cs | 43 +++++++++++++------ .../Formats/Png/PngEncoderTests.cs | 23 ++++++++++ 2 files changed, 53 insertions(+), 13 deletions(-) diff --git a/src/ImageSharp/Formats/Png/PngEncoderCore.cs b/src/ImageSharp/Formats/Png/PngEncoderCore.cs index 113fef5957..078935306f 100644 --- a/src/ImageSharp/Formats/Png/PngEncoderCore.cs +++ b/src/ImageSharp/Formats/Png/PngEncoderCore.cs @@ -167,6 +167,7 @@ internal sealed class PngEncoderCore : IImageEncoderInternals, IDisposable ImageFrame? clonedFrame = null; ImageFrame currentFrame = image.Frames.RootFrame; + int currentFrameIndex = 0; bool clearTransparency = this.encoder.TransparentColorMode is PngTransparentColorMode.Clear; if (clearTransparency) @@ -196,28 +197,49 @@ internal sealed class PngEncoderCore : IImageEncoderInternals, IDisposable if (image.Frames.Count > 1) { this.WriteAnimationControlChunk(stream, (uint)image.Frames.Count, pngMetadata.RepeatCount); + } + + // If the first frame isn't animated, write it as usual and skip it when writing animated frames + if (!pngMetadata.DefaultImageAnimated || image.Frames.Count == 1) + { + FrameControl frameControl = new((uint)this.width, (uint)this.height); + this.WriteDataChunks(frameControl, currentFrame.PixelBuffer.GetRegion(), quantized, stream, false); + currentFrameIndex++; + } - // Write the first frame. + if (image.Frames.Count > 1) + { + // Write the first animated frame. + currentFrame = image.Frames[currentFrameIndex]; PngFrameMetadata frameMetadata = GetPngFrameMetadata(currentFrame); PngDisposalMethod previousDisposal = frameMetadata.DisposalMethod; FrameControl frameControl = this.WriteFrameControlChunk(stream, frameMetadata, currentFrame.Bounds(), 0); - this.WriteDataChunks(frameControl, currentFrame.PixelBuffer.GetRegion(), quantized, stream, false); + uint sequenceNumber = 1; + if (pngMetadata.DefaultImageAnimated) + { + this.WriteDataChunks(frameControl, currentFrame.PixelBuffer.GetRegion(), quantized, stream, false); + } + else + { + sequenceNumber += this.WriteDataChunks(frameControl, currentFrame.PixelBuffer.GetRegion(), quantized, stream, true); + } + + currentFrameIndex++; // Capture the global palette for reuse on subsequent frames. ReadOnlyMemory? previousPalette = quantized?.Palette.ToArray(); // Write following frames. - uint increment = 0; ImageFrame previousFrame = image.Frames.RootFrame; // This frame is reused to store de-duplicated pixel buffers. using ImageFrame encodingFrame = new(image.Configuration, previousFrame.Size()); - for (int i = 1; i < image.Frames.Count; i++) + for (; currentFrameIndex < image.Frames.Count; currentFrameIndex++) { ImageFrame? prev = previousDisposal == PngDisposalMethod.RestoreToBackground ? null : previousFrame; - currentFrame = image.Frames[i]; - ImageFrame? nextFrame = i < image.Frames.Count - 1 ? image.Frames[i + 1] : null; + currentFrame = image.Frames[currentFrameIndex]; + ImageFrame? nextFrame = currentFrameIndex < image.Frames.Count - 1 ? image.Frames[currentFrameIndex + 1] : null; frameMetadata = GetPngFrameMetadata(currentFrame); bool blend = frameMetadata.BlendMethod == PngBlendMethod.Over; @@ -238,22 +260,17 @@ internal sealed class PngEncoderCore : IImageEncoderInternals, IDisposable } // Each frame control sequence number must be incremented by the number of frame data chunks that follow. - frameControl = this.WriteFrameControlChunk(stream, frameMetadata, bounds, (uint)i + increment); + frameControl = this.WriteFrameControlChunk(stream, frameMetadata, bounds, sequenceNumber); // Dispose of previous quantized frame and reassign. quantized?.Dispose(); quantized = this.CreateQuantizedImageAndUpdateBitDepth(pngMetadata, encodingFrame, bounds, previousPalette); - increment += this.WriteDataChunks(frameControl, encodingFrame.PixelBuffer.GetRegion(bounds), quantized, stream, true); + sequenceNumber += this.WriteDataChunks(frameControl, encodingFrame.PixelBuffer.GetRegion(bounds), quantized, stream, true) + 1; previousFrame = currentFrame; previousDisposal = frameMetadata.DisposalMethod; } } - else - { - FrameControl frameControl = new((uint)this.width, (uint)this.height); - this.WriteDataChunks(frameControl, currentFrame.PixelBuffer.GetRegion(), quantized, stream, false); - } this.WriteEndChunk(stream); diff --git a/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs b/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs index a70fb86df1..e7884dd580 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs @@ -587,6 +587,29 @@ public partial class PngEncoderTests } } + [Theory] + [WithFile(TestImages.Png.DefaultNotAnimated, PixelTypes.Rgba32)] + public void Encode_DefaultNotAnimated(TestImageProvider provider) + where TPixel : unmanaged, IPixel + { + using Image image = provider.GetImage(PngDecoder.Instance); + using MemoryStream memStream = new(); + image.Save(memStream, PngEncoder); + memStream.Position = 0; + + image.DebugSave(provider: provider, encoder: PngEncoder, null, false); + + using Image output = Image.Load(memStream); + ImageComparer.Exact.VerifySimilarity(output, image); + + Assert.Equal(2, image.Frames.Count); + Assert.Equal(image.Frames.Count, output.Frames.Count); + + PngMetadata originalMetadata = image.Metadata.GetPngMetadata(); + PngMetadata outputMetadata = output.Metadata.GetPngMetadata(); + Assert.Equal(originalMetadata.DefaultImageAnimated, outputMetadata.DefaultImageAnimated); + } + [Theory] [MemberData(nameof(PngTrnsFiles))] public void Encode_PreserveTrns(string imagePath, PngBitDepth pngBitDepth, PngColorType pngColorType) From c5ebbfe76e84f904750199e0fa3482ce67111eca Mon Sep 17 00:00:00 2001 From: SpaceCheetah Date: Fri, 29 Mar 2024 21:34:48 -1000 Subject: [PATCH 14/19] Add more tests --- .../Formats/Png/PngEncoderTests.Chunks.cs | 33 +++++++++++++++++++ .../Formats/Png/PngMetadataTests.cs | 20 +++++++++++ 2 files changed, 53 insertions(+) diff --git a/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.Chunks.cs b/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.Chunks.cs index 044da21938..2e3390298b 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.Chunks.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.Chunks.cs @@ -3,6 +3,7 @@ using System.Buffers.Binary; using SixLabors.ImageSharp.Formats.Png; +using SixLabors.ImageSharp.Formats.Png.Chunks; using SixLabors.ImageSharp.PixelFormats; // ReSharper disable InconsistentNaming @@ -59,6 +60,38 @@ public partial class PngEncoderTests } } + [Theory] + [WithFile(TestImages.Png.DefaultNotAnimated, PixelTypes.Rgba32)] + [WithFile(TestImages.Png.APng, PixelTypes.Rgba32)] + public void AcTL_CorrectlyWritten(TestImageProvider provider) + where TPixel : unmanaged, IPixel + { + using Image image = provider.GetImage(PngDecoder.Instance); + PngMetadata metadata = image.Metadata.GetPngMetadata(); + int correctFrameCount = image.Frames.Count - (metadata.DefaultImageAnimated ? 0 : 1); + using MemoryStream memStream = new(); + image.Save(memStream, PngEncoder); + memStream.Position = 0; + Span bytesSpan = memStream.ToArray().AsSpan(8); // Skip header. + bool foundAcTl = false; + while (bytesSpan.Length > 0 && !foundAcTl) + { + int length = BinaryPrimitives.ReadInt32BigEndian(bytesSpan[..4]); + PngChunkType type = (PngChunkType)BinaryPrimitives.ReadInt32BigEndian(bytesSpan.Slice(4, 4)); + if (type == PngChunkType.AnimationControl) + { + AnimationControl control = AnimationControl.Parse(bytesSpan[8..]); + foundAcTl = true; + Assert.True(control.NumberFrames == correctFrameCount); + Assert.True(control.NumberPlays == metadata.RepeatCount); + } + + bytesSpan = bytesSpan[(4 + 4 + length + 4)..]; + } + + Assert.True(foundAcTl); + } + [Theory] [InlineData(PngChunkType.Gamma)] [InlineData(PngChunkType.Chroma)] diff --git a/tests/ImageSharp.Tests/Formats/Png/PngMetadataTests.cs b/tests/ImageSharp.Tests/Formats/Png/PngMetadataTests.cs index 4f9ba9abee..8308935d04 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngMetadataTests.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngMetadataTests.cs @@ -146,6 +146,26 @@ public class PngMetadataTests VerifyExifDataIsPresent(exif); } + [Theory] + [WithFile(TestImages.Png.DefaultNotAnimated, PixelTypes.Rgba32)] + public void Decode_IdentifiesDefaultFrameNotAnimated(TestImageProvider provider) + where TPixel : unmanaged, IPixel + { + using Image image = provider.GetImage(PngDecoder.Instance); + PngMetadata meta = image.Metadata.GetFormatMetadata(PngFormat.Instance); + Assert.False(meta.DefaultImageAnimated); + } + + [Theory] + [WithFile(TestImages.Png.APng, PixelTypes.Rgba32)] + public void Decode_IdentifiesDefaultFrameAnimated(TestImageProvider provider) + where TPixel : unmanaged, IPixel + { + using Image image = provider.GetImage(PngDecoder.Instance); + PngMetadata meta = image.Metadata.GetFormatMetadata(PngFormat.Instance); + Assert.True(meta.DefaultImageAnimated); + } + [Theory] [WithFile(TestImages.Png.PngWithMetadata, PixelTypes.Rgba32)] public void Decode_IgnoresExifData_WhenIgnoreMetadataIsTrue(TestImageProvider provider) From 61b5b0c3a943816ae1146b1a0a6ded4e7a429075 Mon Sep 17 00:00:00 2001 From: SpaceCheetah Date: Fri, 29 Mar 2024 21:35:53 -1000 Subject: [PATCH 15/19] Fix incorrect acTL frame count --- src/ImageSharp/Formats/Png/PngEncoderCore.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ImageSharp/Formats/Png/PngEncoderCore.cs b/src/ImageSharp/Formats/Png/PngEncoderCore.cs index 078935306f..99f721fcf4 100644 --- a/src/ImageSharp/Formats/Png/PngEncoderCore.cs +++ b/src/ImageSharp/Formats/Png/PngEncoderCore.cs @@ -196,7 +196,7 @@ internal sealed class PngEncoderCore : IImageEncoderInternals, IDisposable if (image.Frames.Count > 1) { - this.WriteAnimationControlChunk(stream, (uint)image.Frames.Count, pngMetadata.RepeatCount); + this.WriteAnimationControlChunk(stream, (uint)(image.Frames.Count - (pngMetadata.DefaultImageAnimated ? 0 : 1)), pngMetadata.RepeatCount); } // If the first frame isn't animated, write it as usual and skip it when writing animated frames From 5cd96d3bf007a7816691f6b9c2c843118d95a884 Mon Sep 17 00:00:00 2001 From: SpaceCheetah Date: Mon, 1 Apr 2024 17:03:25 -0700 Subject: [PATCH 16/19] re-add comment --- src/ImageSharp/Formats/Png/PngDecoderCore.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ImageSharp/Formats/Png/PngDecoderCore.cs b/src/ImageSharp/Formats/Png/PngDecoderCore.cs index 222fe8ed34..77edf2d9ad 100644 --- a/src/ImageSharp/Formats/Png/PngDecoderCore.cs +++ b/src/ImageSharp/Formats/Png/PngDecoderCore.cs @@ -654,7 +654,8 @@ internal sealed class PngDecoderCore : IImageDecoderInternals { frame = image.Frames.AddFrame(previousFrame ?? image.Frames.RootFrame); - // If restoring to before first frame, restore to background. Same if first frame (previousFrameControl null). + // If the first `fcTL` chunk uses a `dispose_op` of APNG_DISPOSE_OP_PREVIOUS it should be treated as APNG_DISPOSE_OP_BACKGROUND. + // So, if restoring to before first frame, clear entire area. Same if first frame (previousFrameControl null). if (previousFrameControl == null || (previousFrame is null && previousFrameControl.Value.DisposeOperation == PngDisposalMethod.RestoreToPrevious)) { Buffer2DRegion pixelRegion = frame.PixelBuffer.GetRegion(); From 3dec79c9797ad8d97740924a26980d5ae359ab40 Mon Sep 17 00:00:00 2001 From: SpaceCheetah Date: Mon, 1 Apr 2024 17:28:27 -0700 Subject: [PATCH 17/19] Add test for case with frame offsets --- .../Formats/Png/PngEncoderTests.cs | 31 ++++--------------- 1 file changed, 6 insertions(+), 25 deletions(-) diff --git a/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs b/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs index e7884dd580..595adbadce 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs @@ -448,6 +448,8 @@ public partial class PngEncoderTests [Theory] [WithFile(TestImages.Png.APng, PixelTypes.Rgba32)] + [WithFile(TestImages.Png.DefaultNotAnimated, PixelTypes.Rgba32)] + [WithFile(TestImages.Png.FrameOffset, PixelTypes.Rgba32)] public void Encode_APng(TestImageProvider provider) where TPixel : unmanaged, IPixel { @@ -459,15 +461,17 @@ public partial class PngEncoderTests image.DebugSave(provider: provider, encoder: PngEncoder, null, false); using Image output = Image.Load(memStream); - ImageComparer.Exact.VerifySimilarity(output, image); - Assert.Equal(5, image.Frames.Count); + // some loss from original, due to compositing + ImageComparer.TolerantPercentage(0.01f).VerifySimilarity(output, image); + Assert.Equal(image.Frames.Count, output.Frames.Count); PngMetadata originalMetadata = image.Metadata.GetPngMetadata(); PngMetadata outputMetadata = output.Metadata.GetPngMetadata(); Assert.Equal(originalMetadata.RepeatCount, outputMetadata.RepeatCount); + Assert.Equal(originalMetadata.DefaultImageAnimated, outputMetadata.DefaultImageAnimated); for (int i = 0; i < image.Frames.Count; i++) { @@ -587,29 +591,6 @@ public partial class PngEncoderTests } } - [Theory] - [WithFile(TestImages.Png.DefaultNotAnimated, PixelTypes.Rgba32)] - public void Encode_DefaultNotAnimated(TestImageProvider provider) - where TPixel : unmanaged, IPixel - { - using Image image = provider.GetImage(PngDecoder.Instance); - using MemoryStream memStream = new(); - image.Save(memStream, PngEncoder); - memStream.Position = 0; - - image.DebugSave(provider: provider, encoder: PngEncoder, null, false); - - using Image output = Image.Load(memStream); - ImageComparer.Exact.VerifySimilarity(output, image); - - Assert.Equal(2, image.Frames.Count); - Assert.Equal(image.Frames.Count, output.Frames.Count); - - PngMetadata originalMetadata = image.Metadata.GetPngMetadata(); - PngMetadata outputMetadata = output.Metadata.GetPngMetadata(); - Assert.Equal(originalMetadata.DefaultImageAnimated, outputMetadata.DefaultImageAnimated); - } - [Theory] [MemberData(nameof(PngTrnsFiles))] public void Encode_PreserveTrns(string imagePath, PngBitDepth pngBitDepth, PngColorType pngColorType) From c31db731ad0b1f070aeb28a0640bba5519faf7b4 Mon Sep 17 00:00:00 2001 From: SpaceCheetah Date: Mon, 1 Apr 2024 17:56:21 -0700 Subject: [PATCH 18/19] re-add rest of comment --- src/ImageSharp/Formats/Png/PngDecoderCore.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/ImageSharp/Formats/Png/PngDecoderCore.cs b/src/ImageSharp/Formats/Png/PngDecoderCore.cs index 77edf2d9ad..61074a26ed 100644 --- a/src/ImageSharp/Formats/Png/PngDecoderCore.cs +++ b/src/ImageSharp/Formats/Png/PngDecoderCore.cs @@ -652,6 +652,9 @@ internal sealed class PngDecoderCore : IImageDecoderInternals out ImageFrame frame) where TPixel : unmanaged, IPixel { + // We create a clone of the previous frame and add it. + // We will overpaint the difference of pixels on the current frame to create a complete image. + // This ensures that we have enough pixel data to process without distortion. #2450 frame = image.Frames.AddFrame(previousFrame ?? image.Frames.RootFrame); // If the first `fcTL` chunk uses a `dispose_op` of APNG_DISPOSE_OP_PREVIOUS it should be treated as APNG_DISPOSE_OP_BACKGROUND. From 94d7f3c479ed2ecf7e831f19da1bdac67b6d7398 Mon Sep 17 00:00:00 2001 From: SpaceCheetah Date: Tue, 2 Apr 2024 23:00:15 -0700 Subject: [PATCH 19/19] Rename DefaultImageAnimated to AnimateRootFrame --- src/ImageSharp/Formats/Png/PngDecoderCore.cs | 4 ++-- src/ImageSharp/Formats/Png/PngEncoderCore.cs | 6 +++--- src/ImageSharp/Formats/Png/PngMetadata.cs | 6 +++--- .../Formats/Png/PngEncoderTests.Chunks.cs | 2 +- tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs | 2 +- tests/ImageSharp.Tests/Formats/Png/PngMetadataTests.cs | 8 ++++---- 6 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/ImageSharp/Formats/Png/PngDecoderCore.cs b/src/ImageSharp/Formats/Png/PngDecoderCore.cs index 61074a26ed..9cf88f729d 100644 --- a/src/ImageSharp/Formats/Png/PngDecoderCore.cs +++ b/src/ImageSharp/Formats/Png/PngDecoderCore.cs @@ -254,7 +254,7 @@ internal sealed class PngDecoderCore : IImageDecoderInternals break; case PngChunkType.Data: - pngMetadata.DefaultImageAnimated = currentFrameControl != null; + pngMetadata.AnimateRootFrame = currentFrameControl != null; currentFrameControl ??= new((uint)this.header.Width, (uint)this.header.Height); if (image is null) { @@ -271,7 +271,7 @@ internal sealed class PngDecoderCore : IImageDecoderInternals this.ReadNextDataChunk, currentFrameControl.Value, cancellationToken); - if (pngMetadata.DefaultImageAnimated) + if (pngMetadata.AnimateRootFrame) { previousFrame = currentFrame; previousFrameControl = currentFrameControl; diff --git a/src/ImageSharp/Formats/Png/PngEncoderCore.cs b/src/ImageSharp/Formats/Png/PngEncoderCore.cs index 99f721fcf4..6e8224f01e 100644 --- a/src/ImageSharp/Formats/Png/PngEncoderCore.cs +++ b/src/ImageSharp/Formats/Png/PngEncoderCore.cs @@ -196,11 +196,11 @@ internal sealed class PngEncoderCore : IImageEncoderInternals, IDisposable if (image.Frames.Count > 1) { - this.WriteAnimationControlChunk(stream, (uint)(image.Frames.Count - (pngMetadata.DefaultImageAnimated ? 0 : 1)), pngMetadata.RepeatCount); + this.WriteAnimationControlChunk(stream, (uint)(image.Frames.Count - (pngMetadata.AnimateRootFrame ? 0 : 1)), pngMetadata.RepeatCount); } // If the first frame isn't animated, write it as usual and skip it when writing animated frames - if (!pngMetadata.DefaultImageAnimated || image.Frames.Count == 1) + if (!pngMetadata.AnimateRootFrame || image.Frames.Count == 1) { FrameControl frameControl = new((uint)this.width, (uint)this.height); this.WriteDataChunks(frameControl, currentFrame.PixelBuffer.GetRegion(), quantized, stream, false); @@ -215,7 +215,7 @@ internal sealed class PngEncoderCore : IImageEncoderInternals, IDisposable PngDisposalMethod previousDisposal = frameMetadata.DisposalMethod; FrameControl frameControl = this.WriteFrameControlChunk(stream, frameMetadata, currentFrame.Bounds(), 0); uint sequenceNumber = 1; - if (pngMetadata.DefaultImageAnimated) + if (pngMetadata.AnimateRootFrame) { this.WriteDataChunks(frameControl, currentFrame.PixelBuffer.GetRegion(), quantized, stream, false); } diff --git a/src/ImageSharp/Formats/Png/PngMetadata.cs b/src/ImageSharp/Formats/Png/PngMetadata.cs index 766377f7c9..d9028dd807 100644 --- a/src/ImageSharp/Formats/Png/PngMetadata.cs +++ b/src/ImageSharp/Formats/Png/PngMetadata.cs @@ -29,7 +29,7 @@ public class PngMetadata : IDeepCloneable this.InterlaceMethod = other.InterlaceMethod; this.TransparentColor = other.TransparentColor; this.RepeatCount = other.RepeatCount; - this.DefaultImageAnimated = other.DefaultImageAnimated; + this.AnimateRootFrame = other.AnimateRootFrame; if (other.ColorTable?.Length > 0) { @@ -85,9 +85,9 @@ public class PngMetadata : IDeepCloneable public uint RepeatCount { get; set; } = 1; /// - /// Gets or sets a value indicating whether the default image is shown as part of the animated sequence + /// Gets or sets a value indicating whether the root frame is shown as part of the animated sequence /// - public bool DefaultImageAnimated { get; set; } = true; + public bool AnimateRootFrame { get; set; } = true; /// public IDeepCloneable DeepClone() => new PngMetadata(this); diff --git a/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.Chunks.cs b/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.Chunks.cs index 2e3390298b..76fd260dd5 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.Chunks.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.Chunks.cs @@ -68,7 +68,7 @@ public partial class PngEncoderTests { using Image image = provider.GetImage(PngDecoder.Instance); PngMetadata metadata = image.Metadata.GetPngMetadata(); - int correctFrameCount = image.Frames.Count - (metadata.DefaultImageAnimated ? 0 : 1); + int correctFrameCount = image.Frames.Count - (metadata.AnimateRootFrame ? 0 : 1); using MemoryStream memStream = new(); image.Save(memStream, PngEncoder); memStream.Position = 0; diff --git a/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs b/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs index 595adbadce..35c446c704 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs @@ -471,7 +471,7 @@ public partial class PngEncoderTests PngMetadata outputMetadata = output.Metadata.GetPngMetadata(); Assert.Equal(originalMetadata.RepeatCount, outputMetadata.RepeatCount); - Assert.Equal(originalMetadata.DefaultImageAnimated, outputMetadata.DefaultImageAnimated); + Assert.Equal(originalMetadata.AnimateRootFrame, outputMetadata.AnimateRootFrame); for (int i = 0; i < image.Frames.Count; i++) { diff --git a/tests/ImageSharp.Tests/Formats/Png/PngMetadataTests.cs b/tests/ImageSharp.Tests/Formats/Png/PngMetadataTests.cs index 8308935d04..225e4deef2 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngMetadataTests.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngMetadataTests.cs @@ -33,7 +33,7 @@ public class PngMetadataTests Gamma = 2, TextData = new List { new PngTextData("name", "value", "foo", "bar") }, RepeatCount = 123, - DefaultImageAnimated = false + AnimateRootFrame = false }; PngMetadata clone = (PngMetadata)meta.DeepClone(); @@ -45,7 +45,7 @@ public class PngMetadataTests Assert.False(meta.TextData.Equals(clone.TextData)); Assert.True(meta.TextData.SequenceEqual(clone.TextData)); Assert.True(meta.RepeatCount == clone.RepeatCount); - Assert.True(meta.DefaultImageAnimated == clone.DefaultImageAnimated); + Assert.True(meta.AnimateRootFrame == clone.AnimateRootFrame); clone.BitDepth = PngBitDepth.Bit2; clone.ColorType = PngColorType.Palette; @@ -153,7 +153,7 @@ public class PngMetadataTests { using Image image = provider.GetImage(PngDecoder.Instance); PngMetadata meta = image.Metadata.GetFormatMetadata(PngFormat.Instance); - Assert.False(meta.DefaultImageAnimated); + Assert.False(meta.AnimateRootFrame); } [Theory] @@ -163,7 +163,7 @@ public class PngMetadataTests { using Image image = provider.GetImage(PngDecoder.Instance); PngMetadata meta = image.Metadata.GetFormatMetadata(PngFormat.Instance); - Assert.True(meta.DefaultImageAnimated); + Assert.True(meta.AnimateRootFrame); } [Theory]