From 1a6b46504db9d0ea11e333e02ce7a7e65a3c5b44 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Tue, 22 Aug 2023 09:59:41 +1000 Subject: [PATCH 1/7] Cleanup based on feedback --- src/ImageSharp/Formats/Gif/GifEncoderCore.cs | 32 +++++++++++--------- src/ImageSharp/Formats/Gif/GifMetadata.cs | 4 +-- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/src/ImageSharp/Formats/Gif/GifEncoderCore.cs b/src/ImageSharp/Formats/Gif/GifEncoderCore.cs index be08c0da9..ccf8feacc 100644 --- a/src/ImageSharp/Formats/Gif/GifEncoderCore.cs +++ b/src/ImageSharp/Formats/Gif/GifEncoderCore.cs @@ -135,7 +135,7 @@ internal sealed class GifEncoderCore : IImageEncoderInternals byte backgroundIndex = unchecked((byte)transparencyIndex); if (transparencyIndex == -1) { - backgroundIndex = gifMetadata.BackgroundColor; + backgroundIndex = gifMetadata.BackgroundColorIndex; } // Get the number of bits. @@ -236,18 +236,19 @@ internal sealed class GifEncoderCore : IImageEncoderInternals { this.WriteGraphicalControlExtension(metadata, transparencyIndex, stream); - Buffer2DRegion region = ((IPixelSource)quantized).PixelBuffer.GetRegion(); + Buffer2D indices = ((IPixelSource)quantized).PixelBuffer; + Rectangle interest = indices.FullRectangle(); bool useLocal = this.colorTableMode == GifColorTableMode.Local || (metadata?.ColorTableMode == GifColorTableMode.Local); int bitDepth = ColorNumerics.GetBitsNeededForColorDepth(quantized.Palette.Length); - this.WriteImageDescriptor(region.Rectangle, useLocal, bitDepth, stream); + this.WriteImageDescriptor(interest, useLocal, bitDepth, stream); if (useLocal) { this.WriteColorTable(quantized, bitDepth, stream); } - this.WriteImageData(region, stream, quantized.Palette.Length, transparencyIndex); + this.WriteImageData(indices, interest, stream, quantized.Palette.Length, transparencyIndex); } private void EncodeAdditionalFrame( @@ -322,20 +323,20 @@ internal sealed class GifEncoderCore : IImageEncoderInternals transparencyIndex = GetTransparentIndex(quantized, metadata); // Trim down the buffer to the minimum size required. - // Buffer2DRegion region = ((IPixelSource)quantized).PixelBuffer.GetRegion(); - Buffer2DRegion region = TrimTransparentPixels(((IPixelSource)quantized).PixelBuffer, transparencyIndex); + Buffer2D indices = ((IPixelSource)quantized).PixelBuffer; + Rectangle interest = TrimTransparentPixels(indices, transparencyIndex); this.WriteGraphicalControlExtension(metadata, transparencyIndex, stream); int bitDepth = ColorNumerics.GetBitsNeededForColorDepth(quantized.Palette.Length); - this.WriteImageDescriptor(region.Rectangle, useLocal, bitDepth, stream); + this.WriteImageDescriptor(interest, useLocal, bitDepth, stream); if (useLocal) { this.WriteColorTable(quantized, bitDepth, stream); } - this.WriteImageData(region, stream, quantized.Palette.Length, transparencyIndex); + this.WriteImageData(indices, interest, stream, quantized.Palette.Length, transparencyIndex); } private void DeDuplicatePixels( @@ -399,11 +400,11 @@ internal sealed class GifEncoderCore : IImageEncoderInternals } } - private static Buffer2DRegion TrimTransparentPixels(Buffer2D buffer, int transparencyIndex) + private static Rectangle TrimTransparentPixels(Buffer2D buffer, int transparencyIndex) { if (transparencyIndex < 0) { - return buffer.GetRegion(); + return buffer.FullRectangle(); } byte trimmableIndex = unchecked((byte)transparencyIndex); @@ -596,7 +597,7 @@ internal sealed class GifEncoderCore : IImageEncoderInternals if (top == bottom || left == right) { // The entire image is transparent. - return buffer.GetRegion(); + return buffer.FullRectangle(); } if (!isTransparentRow) @@ -605,7 +606,7 @@ internal sealed class GifEncoderCore : IImageEncoderInternals bottom = buffer.Height; } - return buffer.GetRegion(Rectangle.FromLTRB(left, top, Math.Min(right + 1, buffer.Width), Math.Min(bottom + 1, buffer.Height))); + return Rectangle.FromLTRB(left, top, Math.Min(right + 1, buffer.Width), Math.Min(bottom + 1, buffer.Height)); } /// @@ -923,11 +924,14 @@ internal sealed class GifEncoderCore : IImageEncoderInternals /// Writes the image pixel data to the stream. /// /// The containing indexed pixels. + /// The region of interest. /// The stream to write to. /// The length of the frame color palette. /// The index of the color used to represent transparency. - private void WriteImageData(Buffer2DRegion indices, Stream stream, int paletteLength, int transparencyIndex) + private void WriteImageData(Buffer2D indices, Rectangle interest, Stream stream, int paletteLength, int transparencyIndex) { + Buffer2DRegion region = indices.GetRegion(interest); + // Pad the bit depth when required for encoding the image data. // This is a common trick which allows to use out of range indexes for transparency and avoid allocating a larger color palette // as decoders skip indexes that are out of range. @@ -936,6 +940,6 @@ internal sealed class GifEncoderCore : IImageEncoderInternals : 0; using LzwEncoder encoder = new(this.memoryAllocator, ColorNumerics.GetBitsNeededForColorDepth(paletteLength + padding)); - encoder.Encode(indices, stream); + encoder.Encode(region, stream); } } diff --git a/src/ImageSharp/Formats/Gif/GifMetadata.cs b/src/ImageSharp/Formats/Gif/GifMetadata.cs index 184ccd644..0172344ff 100644 --- a/src/ImageSharp/Formats/Gif/GifMetadata.cs +++ b/src/ImageSharp/Formats/Gif/GifMetadata.cs @@ -23,7 +23,7 @@ public class GifMetadata : IDeepCloneable { this.RepeatCount = other.RepeatCount; this.ColorTableMode = other.ColorTableMode; - this.BackgroundColor = other.BackgroundColor; + this.BackgroundColorIndex = other.BackgroundColorIndex; if (other.GlobalColorTable?.Length > 0) { @@ -58,7 +58,7 @@ public class GifMetadata : IDeepCloneable /// Gets or sets the index at the for the background color. /// The background color is the color used for those pixels on the screen that are not covered by an image. /// - public byte BackgroundColor { get; set; } + public byte BackgroundColorIndex { get; set; } /// /// Gets or sets the collection of comments about the graphics, credits, descriptions or any From ed3860cfda28f3087f962e4871bec50ce103b7d4 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Wed, 30 Aug 2023 12:01:07 +1000 Subject: [PATCH 2/7] Handle EOF in Jpeg bit reader when data is bad to prevent DOS attack. (#2516) * Handle EOF in bit reader when data is bad. * Allow parallel processing of multi-megapixel image * Stream seek can exceed the length of a stream * Try triggering on release branches * Update JpegBitReader.cs * Skin on Win .NET 6 * All Win OS is an issue * Address feedback * add validation to CanIterateWithoutIntOverflow --------- Co-authored-by: antonfirsov --- .github/workflows/build-and-test.yml | 1 + .../Advanced/ParallelRowIterator.cs | 10 ++--- .../Jpeg/Components/Decoder/JpegBitReader.cs | 7 +++- .../Formats/Jpg/JpegDecoderTests.cs | 17 ++++++++ .../Helpers/ParallelRowIteratorTests.cs | 39 +++++++++++++++++++ tests/ImageSharp.Tests/TestImages.cs | 1 + .../Images/Input/Jpg/issues/Hang_C438A851.jpg | 3 ++ 7 files changed, 72 insertions(+), 6 deletions(-) create mode 100644 tests/Images/Input/Jpg/issues/Hang_C438A851.jpg diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index b5cc5daca..853cad738 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -9,6 +9,7 @@ on: pull_request: branches: - main + - release/* types: [ labeled, opened, synchronize, reopened ] jobs: Build: diff --git a/src/ImageSharp/Advanced/ParallelRowIterator.cs b/src/ImageSharp/Advanced/ParallelRowIterator.cs index 0eb5952a6..657654a84 100644 --- a/src/ImageSharp/Advanced/ParallelRowIterator.cs +++ b/src/ImageSharp/Advanced/ParallelRowIterator.cs @@ -50,7 +50,7 @@ public static partial class ParallelRowIterator int width = rectangle.Width; int height = rectangle.Height; - int maxSteps = DivideCeil(width * height, parallelSettings.MinimumPixelsProcessedPerTask); + int maxSteps = DivideCeil(width * (long)height, parallelSettings.MinimumPixelsProcessedPerTask); int numOfSteps = Math.Min(parallelSettings.MaxDegreeOfParallelism, maxSteps); // Avoid TPL overhead in this trivial case: @@ -115,7 +115,7 @@ public static partial class ParallelRowIterator int width = rectangle.Width; int height = rectangle.Height; - int maxSteps = DivideCeil(width * height, parallelSettings.MinimumPixelsProcessedPerTask); + int maxSteps = DivideCeil(width * (long)height, parallelSettings.MinimumPixelsProcessedPerTask); int numOfSteps = Math.Min(parallelSettings.MaxDegreeOfParallelism, maxSteps); MemoryAllocator allocator = parallelSettings.MemoryAllocator; int bufferLength = Unsafe.AsRef(operation).GetRequiredBufferLength(rectangle); @@ -180,7 +180,7 @@ public static partial class ParallelRowIterator int width = rectangle.Width; int height = rectangle.Height; - int maxSteps = DivideCeil(width * height, parallelSettings.MinimumPixelsProcessedPerTask); + int maxSteps = DivideCeil(width * (long)height, parallelSettings.MinimumPixelsProcessedPerTask); int numOfSteps = Math.Min(parallelSettings.MaxDegreeOfParallelism, maxSteps); // Avoid TPL overhead in this trivial case: @@ -242,7 +242,7 @@ public static partial class ParallelRowIterator int width = rectangle.Width; int height = rectangle.Height; - int maxSteps = DivideCeil(width * height, parallelSettings.MinimumPixelsProcessedPerTask); + int maxSteps = DivideCeil(width * (long)height, parallelSettings.MinimumPixelsProcessedPerTask); int numOfSteps = Math.Min(parallelSettings.MaxDegreeOfParallelism, maxSteps); MemoryAllocator allocator = parallelSettings.MemoryAllocator; int bufferLength = Unsafe.AsRef(operation).GetRequiredBufferLength(rectangle); @@ -270,7 +270,7 @@ public static partial class ParallelRowIterator } [MethodImpl(InliningOptions.ShortMethod)] - private static int DivideCeil(int dividend, int divisor) => 1 + ((dividend - 1) / divisor); + private static int DivideCeil(long dividend, int divisor) => (int)Math.Min(1 + ((dividend - 1) / divisor), int.MaxValue); private static void ValidateRectangle(Rectangle rectangle) { diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs index d80679db6..0877dbc92 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/JpegBitReader.cs @@ -212,7 +212,12 @@ internal struct JpegBitReader private int ReadStream() { int value = this.badData ? 0 : this.stream.ReadByte(); - if (value == -1) + + // We've encountered the end of the file stream which means there's no EOI marker or the marker has been read + // during decoding of the SOS marker. + // When reading individual bits 'badData' simply means we have hit a marker, When data is '0' and the stream is exhausted + // 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 // in the image or the SOS marker has the wrong dimensions set. diff --git a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs index 594e7ebe7..eaa9f82cb 100644 --- a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs @@ -325,4 +325,21 @@ public partial class JpegDecoderTests image.DebugSave(provider); image.CompareToOriginal(provider); } + + [Theory] + [WithFile(TestImages.Jpeg.Issues.HangBadScan, PixelTypes.L8)] + public void DecodeHang(TestImageProvider provider) + where TPixel : unmanaged, IPixel + { + if (TestEnvironment.IsWindows && + TestEnvironment.RunsOnCI) + { + // Windows CI runs consistently fail with OOM. + return; + } + + using Image image = provider.GetImage(JpegDecoder.Instance); + Assert.Equal(65503, image.Width); + Assert.Equal(65503, image.Height); + } } diff --git a/tests/ImageSharp.Tests/Helpers/ParallelRowIteratorTests.cs b/tests/ImageSharp.Tests/Helpers/ParallelRowIteratorTests.cs index 1700b4a73..d393850d6 100644 --- a/tests/ImageSharp.Tests/Helpers/ParallelRowIteratorTests.cs +++ b/tests/ImageSharp.Tests/Helpers/ParallelRowIteratorTests.cs @@ -2,6 +2,8 @@ // Licensed under the Six Labors Split License. using System.Numerics; +using System.Runtime.CompilerServices; +using Castle.Core.Configuration; using SixLabors.ImageSharp.Advanced; using SixLabors.ImageSharp.Memory; using SixLabors.ImageSharp.PixelFormats; @@ -406,6 +408,43 @@ public class ParallelRowIteratorTests Assert.Contains(width <= 0 ? "Width" : "Height", ex.Message); } + [Fact] + public void CanIterateWithoutIntOverflow() + { + ParallelExecutionSettings parallelSettings = ParallelExecutionSettings.FromConfiguration(Configuration.Default); + const int max = 100_000; + + Rectangle rect = new(0, 0, max, max); + int intervalMaxY = 0; + void RowAction(RowInterval rows, Span memory) => intervalMaxY = Math.Max(rows.Max, intervalMaxY); + + TestRowOperation operation = new(); + TestRowIntervalOperation intervalOperation = new(RowAction); + + ParallelRowIterator.IterateRows(Configuration.Default, rect, in operation); + Assert.Equal(max - 1, operation.MaxY.Value); + + ParallelRowIterator.IterateRowIntervals, Rgba32>(rect, in parallelSettings, in intervalOperation); + Assert.Equal(max, intervalMaxY); + } + + private readonly struct TestRowOperation : IRowOperation + { + public TestRowOperation() + { + } + + public StrongBox MaxY { get; } = new StrongBox(); + + public void Invoke(int y) + { + lock (this.MaxY) + { + this.MaxY.Value = Math.Max(y, this.MaxY.Value); + } + } + } + private readonly struct TestRowIntervalOperation : IRowIntervalOperation { private readonly Action action; diff --git a/tests/ImageSharp.Tests/TestImages.cs b/tests/ImageSharp.Tests/TestImages.cs index a25424b6d..afde66ffa 100644 --- a/tests/ImageSharp.Tests/TestImages.cs +++ b/tests/ImageSharp.Tests/TestImages.cs @@ -291,6 +291,7 @@ public static class TestImages public const string Issue2334_NotEnoughBytesA = "Jpg/issues/issue-2334-a.jpg"; public const string Issue2334_NotEnoughBytesB = "Jpg/issues/issue-2334-b.jpg"; public const string Issue2478_JFXX = "Jpg/issues/issue-2478-jfxx.jpg"; + public const string HangBadScan = "Jpg/issues/Hang_C438A851.jpg"; public static class Fuzz { diff --git a/tests/Images/Input/Jpg/issues/Hang_C438A851.jpg b/tests/Images/Input/Jpg/issues/Hang_C438A851.jpg new file mode 100644 index 000000000..97ab9ad0f --- /dev/null +++ b/tests/Images/Input/Jpg/issues/Hang_C438A851.jpg @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:580760756f2e7e3ed0752a4ec53d6b6786a4f005606f3a50878f732b3b2a1bcb +size 413 From b48dbc5087de9bd98dc8ed0f478550e22d4e1ff9 Mon Sep 17 00:00:00 2001 From: antonfirsov Date: Sat, 2 Sep 2023 00:00:47 +0200 Subject: [PATCH 3/7] if transparencyIndex is outside the palette, ignore it --- .../Quantization/EuclideanPixelMap{TPixel}.cs | 31 +++++-------------- 1 file changed, 8 insertions(+), 23 deletions(-) diff --git a/src/ImageSharp/Processing/Processors/Quantization/EuclideanPixelMap{TPixel}.cs b/src/ImageSharp/Processing/Processors/Quantization/EuclideanPixelMap{TPixel}.cs index e767ac4f7..f75664903 100644 --- a/src/ImageSharp/Processing/Processors/Quantization/EuclideanPixelMap{TPixel}.cs +++ b/src/ImageSharp/Processing/Processors/Quantization/EuclideanPixelMap{TPixel}.cs @@ -53,21 +53,16 @@ internal sealed class EuclideanPixelMap : IDisposable this.rgbaPalette = new Rgba32[palette.Length]; this.cache = new ColorDistanceCache(configuration.MemoryAllocator); PixelOperations.Instance.ToRgba32(configuration, this.Palette.Span, this.rgbaPalette); - this.transparentIndex = transparentIndex; + + // If the provided transparentIndex is outside of the palette, silently ignore it. + this.transparentIndex = transparentIndex < this.Palette.Length ? transparentIndex : -1; } /// /// Gets the color palette of this . /// The palette memory is owned by the palette source that created it. /// - public ReadOnlyMemory Palette - { - [MethodImpl(InliningOptions.ShortMethod)] - get; - - [MethodImpl(InliningOptions.ShortMethod)] - private set; - } + public ReadOnlyMemory Palette { get; private set; } /// /// Returns the closest color in the palette and the index of that pixel. @@ -106,10 +101,10 @@ internal sealed class EuclideanPixelMap : IDisposable } /// - /// Allows setting the transparent index after construction. + /// Allows setting the transparent index after construction. If the provided transparentIndex is outside of the palette, silently ignore it. /// /// An explicit index at which to match transparent pixels. - public void SetTransparentIndex(int index) => this.transparentIndex = index; + public void SetTransparentIndex(int index) => this.transparentIndex = index < this.Palette.Length ? index : -1; [MethodImpl(InliningOptions.ShortMethod)] private int GetClosestColorSlow(Rgba32 rgba, ref TPixel paletteRef, out TPixel match) @@ -122,19 +117,9 @@ internal sealed class EuclideanPixelMap : IDisposable { // We have explicit instructions. No need to search. index = this.transparentIndex; + DebugGuard.MustBeLessThan(index, this.Palette.Length, nameof(index)); this.cache.Add(rgba, (byte)index); - - if (index >= 0 && index < this.Palette.Length) - { - match = Unsafe.Add(ref paletteRef, (uint)index); - } - else - { - Unsafe.SkipInit(out TPixel pixel); - pixel.FromScaledVector4(Vector4.Zero); - match = pixel; - } - + match = Unsafe.Add(ref paletteRef, (uint)index); return index; } From 54b7e04f7a3c2921af3c769bd6c27fd3d5156f04 Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Sat, 2 Sep 2023 13:04:03 +0200 Subject: [PATCH 4/7] Fix #2518 (#2519) * OilPaint benchmark * fix #2518 * Update OilPaintingProcessor{TPixel}.cs * clamp the vector to 0..1 and undo buffer overallocation * throw ImageProcessingException instead of clamping --------- Co-authored-by: James Jackson-South --- .../Effects/OilPaintingProcessor{TPixel}.cs | 47 +++++++++++-------- .../Processing/OilPaint.cs | 19 ++++++++ .../Processors/Effects/OilPaintTest.cs | 13 +++-- 3 files changed, 55 insertions(+), 24 deletions(-) create mode 100644 tests/ImageSharp.Benchmarks/Processing/OilPaint.cs diff --git a/src/ImageSharp/Processing/Processors/Effects/OilPaintingProcessor{TPixel}.cs b/src/ImageSharp/Processing/Processors/Effects/OilPaintingProcessor{TPixel}.cs index 6352230de..1491fe073 100644 --- a/src/ImageSharp/Processing/Processors/Effects/OilPaintingProcessor{TPixel}.cs +++ b/src/ImageSharp/Processing/Processors/Effects/OilPaintingProcessor{TPixel}.cs @@ -4,6 +4,7 @@ using System.Buffers; using System.Numerics; using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; using SixLabors.ImageSharp.Advanced; using SixLabors.ImageSharp.Memory; using SixLabors.ImageSharp.PixelFormats; @@ -34,17 +35,25 @@ internal class OilPaintingProcessor : ImageProcessor /// protected override void OnFrameApply(ImageFrame source) { + int levels = Math.Clamp(this.definition.Levels, 1, 255); int brushSize = Math.Clamp(this.definition.BrushSize, 1, Math.Min(source.Width, source.Height)); using Buffer2D targetPixels = this.Configuration.MemoryAllocator.Allocate2D(source.Size()); source.CopyTo(targetPixels); - RowIntervalOperation operation = new(this.SourceRectangle, targetPixels, source.PixelBuffer, this.Configuration, brushSize >> 1, this.definition.Levels); - ParallelRowIterator.IterateRowIntervals( + RowIntervalOperation operation = new(this.SourceRectangle, targetPixels, source.PixelBuffer, this.Configuration, brushSize >> 1, levels); + try + { + ParallelRowIterator.IterateRowIntervals( this.Configuration, this.SourceRectangle, in operation); + } + catch (Exception ex) + { + throw new ImageProcessingException("The OilPaintProcessor failed. The most likely reason is that a pixel component was outside of its' allowed range.", ex); + } Buffer2D.SwapOrCopyContent(source.PixelBuffer, targetPixels); } @@ -105,18 +114,18 @@ internal class OilPaintingProcessor : ImageProcessor Span targetRowVector4Span = targetRowBuffer.Memory.Span; Span targetRowAreaVector4Span = targetRowVector4Span.Slice(this.bounds.X, this.bounds.Width); - ref float binsRef = ref bins.GetReference(); - ref int intensityBinRef = ref Unsafe.As(ref binsRef); - ref float redBinRef = ref Unsafe.Add(ref binsRef, (uint)this.levels); - ref float blueBinRef = ref Unsafe.Add(ref redBinRef, (uint)this.levels); - ref float greenBinRef = ref Unsafe.Add(ref blueBinRef, (uint)this.levels); + Span binsSpan = bins.GetSpan(); + Span intensityBinsSpan = MemoryMarshal.Cast(binsSpan); + Span redBinSpan = binsSpan[this.levels..]; + Span blueBinSpan = redBinSpan[this.levels..]; + Span greenBinSpan = blueBinSpan[this.levels..]; for (int y = rows.Min; y < rows.Max; y++) { Span sourceRowPixelSpan = this.source.DangerousGetRowSpan(y); Span sourceRowAreaPixelSpan = sourceRowPixelSpan.Slice(this.bounds.X, this.bounds.Width); - PixelOperations.Instance.ToVector4(this.configuration, sourceRowAreaPixelSpan, sourceRowAreaVector4Span); + PixelOperations.Instance.ToVector4(this.configuration, sourceRowAreaPixelSpan, sourceRowAreaVector4Span, PixelConversionModifiers.Scale); for (int x = this.bounds.X; x < this.bounds.Right; x++) { @@ -140,7 +149,7 @@ internal class OilPaintingProcessor : ImageProcessor int offsetX = x + fxr; offsetX = Numerics.Clamp(offsetX, 0, maxX); - Vector4 vector = sourceOffsetRow[offsetX].ToVector4(); + Vector4 vector = sourceOffsetRow[offsetX].ToScaledVector4(); float sourceRed = vector.X; float sourceBlue = vector.Z; @@ -148,21 +157,21 @@ internal class OilPaintingProcessor : ImageProcessor int currentIntensity = (int)MathF.Round((sourceBlue + sourceGreen + sourceRed) / 3F * (this.levels - 1)); - Unsafe.Add(ref intensityBinRef, (uint)currentIntensity)++; - Unsafe.Add(ref redBinRef, (uint)currentIntensity) += sourceRed; - Unsafe.Add(ref blueBinRef, (uint)currentIntensity) += sourceBlue; - Unsafe.Add(ref greenBinRef, (uint)currentIntensity) += sourceGreen; + intensityBinsSpan[currentIntensity]++; + redBinSpan[currentIntensity] += sourceRed; + blueBinSpan[currentIntensity] += sourceBlue; + greenBinSpan[currentIntensity] += sourceGreen; - if (Unsafe.Add(ref intensityBinRef, (uint)currentIntensity) > maxIntensity) + if (intensityBinsSpan[currentIntensity] > maxIntensity) { - maxIntensity = Unsafe.Add(ref intensityBinRef, (uint)currentIntensity); + maxIntensity = intensityBinsSpan[currentIntensity]; maxIndex = currentIntensity; } } - float red = MathF.Abs(Unsafe.Add(ref redBinRef, (uint)maxIndex) / maxIntensity); - float blue = MathF.Abs(Unsafe.Add(ref blueBinRef, (uint)maxIndex) / maxIntensity); - float green = MathF.Abs(Unsafe.Add(ref greenBinRef, (uint)maxIndex) / maxIntensity); + float red = redBinSpan[maxIndex] / maxIntensity; + float blue = blueBinSpan[maxIndex] / maxIntensity; + float green = greenBinSpan[maxIndex] / maxIntensity; float alpha = sourceRowVector4Span[x].W; targetRowVector4Span[x] = new Vector4(red, green, blue, alpha); @@ -171,7 +180,7 @@ internal class OilPaintingProcessor : ImageProcessor Span targetRowAreaPixelSpan = this.targetPixels.DangerousGetRowSpan(y).Slice(this.bounds.X, this.bounds.Width); - PixelOperations.Instance.FromVector4Destructive(this.configuration, targetRowAreaVector4Span, targetRowAreaPixelSpan); + PixelOperations.Instance.FromVector4Destructive(this.configuration, targetRowAreaVector4Span, targetRowAreaPixelSpan, PixelConversionModifiers.Scale); } } } diff --git a/tests/ImageSharp.Benchmarks/Processing/OilPaint.cs b/tests/ImageSharp.Benchmarks/Processing/OilPaint.cs new file mode 100644 index 000000000..239d5a93b --- /dev/null +++ b/tests/ImageSharp.Benchmarks/Processing/OilPaint.cs @@ -0,0 +1,19 @@ +// Copyright (c) Six Labors. +// Licensed under the Six Labors Split License. + +using BenchmarkDotNet.Attributes; +using SixLabors.ImageSharp.PixelFormats; +using SixLabors.ImageSharp.Processing; + +namespace SixLabors.ImageSharp.Benchmarks.Processing; + +[Config(typeof(Config.MultiFramework))] +public class OilPaint +{ + [Benchmark] + public void DoOilPaint() + { + using Image image = new Image(1920, 1200, new(127, 191, 255)); + image.Mutate(ctx => ctx.OilPaint()); + } +} diff --git a/tests/ImageSharp.Tests/Processing/Processors/Effects/OilPaintTest.cs b/tests/ImageSharp.Tests/Processing/Processors/Effects/OilPaintTest.cs index 990a97bed..10811a559 100644 --- a/tests/ImageSharp.Tests/Processing/Processors/Effects/OilPaintTest.cs +++ b/tests/ImageSharp.Tests/Processing/Processors/Effects/OilPaintTest.cs @@ -27,8 +27,7 @@ public class OilPaintTest [WithFileCollection(nameof(InputImages), nameof(OilPaintValues), PixelTypes.Rgba32)] public void FullImage(TestImageProvider provider, int levels, int brushSize) where TPixel : unmanaged, IPixel - { - provider.RunValidatingProcessorTest( + => provider.RunValidatingProcessorTest( x => { x.OilPaint(levels, brushSize); @@ -36,17 +35,21 @@ public class OilPaintTest }, ImageComparer.TolerantPercentage(0.01F), appendPixelTypeToFileName: false); - } [Theory] [WithFileCollection(nameof(InputImages), nameof(OilPaintValues), PixelTypes.Rgba32)] [WithTestPatternImages(nameof(OilPaintValues), 100, 100, PixelTypes.Rgba32)] public void InBox(TestImageProvider provider, int levels, int brushSize) where TPixel : unmanaged, IPixel - { - provider.RunRectangleConstrainedValidatingProcessorTest( + => provider.RunRectangleConstrainedValidatingProcessorTest( (x, rect) => x.OilPaint(levels, brushSize, rect), $"{levels}-{brushSize}", ImageComparer.TolerantPercentage(0.01F)); + + [Fact] + public void Issue2518_PixelComponentOutsideOfRange_ThrowsImageProcessingException() + { + using Image image = new(10, 10, new RgbaVector(1, 1, 100)); + Assert.Throws(() => image.Mutate(ctx => ctx.OilPaint())); } } From b56633e2f07a81f92d17c79c42990519035d6743 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Mon, 11 Sep 2023 20:51:39 +1000 Subject: [PATCH 5/7] Review fixes --- src/ImageSharp/Formats/Gif/GifDecoderCore.cs | 11 ++++++----- src/ImageSharp/Formats/Gif/GifFrameMetadata.cs | 3 +++ src/ImageSharp/Formats/Gif/GifMetadata.cs | 3 +++ 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/ImageSharp/Formats/Gif/GifDecoderCore.cs b/src/ImageSharp/Formats/Gif/GifDecoderCore.cs index 776cb0e3c..bc41c89dc 100644 --- a/src/ImageSharp/Formats/Gif/GifDecoderCore.cs +++ b/src/ImageSharp/Formats/Gif/GifDecoderCore.cs @@ -710,10 +710,10 @@ internal sealed class GifDecoderCore : IImageDecoderInternals gifMeta.ColorTableMode = GifColorTableMode.Local; Color[] colorTable = new Color[this.imageDescriptor.LocalColorTableSize]; - ref Rgb24 localBase = ref MemoryMarshal.GetReference(MemoryMarshal.Cast(this.currentLocalColorTable!.GetSpan()[..this.currentLocalColorTableSize])); + ReadOnlySpan rgbTable = MemoryMarshal.Cast(this.currentLocalColorTable!.GetSpan()[..this.currentLocalColorTableSize]); for (int i = 0; i < colorTable.Length; i++) { - colorTable[i] = new Color(Unsafe.Add(ref localBase, (uint)i)); + colorTable[i] = new Color(rgbTable[i]); } gifMeta.LocalColorTable = colorTable; @@ -784,13 +784,14 @@ internal sealed class GifDecoderCore : IImageDecoderInternals this.globalColorTable = this.memoryAllocator.Allocate(globalColorTableLength, AllocationOptions.Clean); // Read the global color table data from the stream and preserve it in the gif metadata - stream.Read(this.globalColorTable.GetSpan()); + Span globalColorTableSpan = this.globalColorTable.GetSpan(); + stream.Read(globalColorTableSpan); Color[] colorTable = new Color[this.logicalScreenDescriptor.GlobalColorTableSize]; - ref Rgb24 globalBase = ref MemoryMarshal.GetReference(MemoryMarshal.Cast(this.globalColorTable.GetSpan())); + ReadOnlySpan rgbTable = MemoryMarshal.Cast(globalColorTableSpan); for (int i = 0; i < colorTable.Length; i++) { - colorTable[i] = new Color(Unsafe.Add(ref globalBase, (uint)i)); + colorTable[i] = new Color(rgbTable[i]); } this.gifMetadata.GlobalColorTable = colorTable; diff --git a/src/ImageSharp/Formats/Gif/GifFrameMetadata.cs b/src/ImageSharp/Formats/Gif/GifFrameMetadata.cs index f7959ac12..faabf7dfa 100644 --- a/src/ImageSharp/Formats/Gif/GifFrameMetadata.cs +++ b/src/ImageSharp/Formats/Gif/GifFrameMetadata.cs @@ -1,6 +1,8 @@ // Copyright (c) Six Labors. // Licensed under the Six Labors Split License. +using SixLabors.ImageSharp.PixelFormats; + namespace SixLabors.ImageSharp.Formats.Gif; /// @@ -41,6 +43,7 @@ public class GifFrameMetadata : IDeepCloneable /// /// Gets or sets the local color table, if any. + /// The underlying pixel format is represented by . /// public ReadOnlyMemory? LocalColorTable { get; set; } diff --git a/src/ImageSharp/Formats/Gif/GifMetadata.cs b/src/ImageSharp/Formats/Gif/GifMetadata.cs index 0172344ff..d25e2a5cc 100644 --- a/src/ImageSharp/Formats/Gif/GifMetadata.cs +++ b/src/ImageSharp/Formats/Gif/GifMetadata.cs @@ -1,6 +1,8 @@ // Copyright (c) Six Labors. // Licensed under the Six Labors Split License. +using SixLabors.ImageSharp.PixelFormats; + namespace SixLabors.ImageSharp.Formats.Gif; /// @@ -51,6 +53,7 @@ public class GifMetadata : IDeepCloneable /// /// Gets or sets the global color table, if any. + /// The underlying pixel format is represented by . /// public ReadOnlyMemory? GlobalColorTable { get; set; } From 1e53adf85ea63e197e7c4392daef71755e3a0d06 Mon Sep 17 00:00:00 2001 From: Jeffrey Parker Date: Wed, 13 Sep 2023 02:26:55 -0500 Subject: [PATCH 6/7] Fix for issue 2504 - ensuring image and individual frame metadata are written out correctly --- .../Formats/Tiff/TiffEncoderCore.cs | 5 + .../Tiff/TiffEncoderEntriesCollector.cs | 114 +++++++++++++++--- .../Formats/Tiff/TiffMetadataTests.cs | 91 ++++++++++++++ 3 files changed, 195 insertions(+), 15 deletions(-) diff --git a/src/ImageSharp/Formats/Tiff/TiffEncoderCore.cs b/src/ImageSharp/Formats/Tiff/TiffEncoderCore.cs index d7243c696..b7338ac20 100644 --- a/src/ImageSharp/Formats/Tiff/TiffEncoderCore.cs +++ b/src/ImageSharp/Formats/Tiff/TiffEncoderCore.cs @@ -157,6 +157,7 @@ internal sealed class TiffEncoderCore : IImageEncoderInternals long ifdMarker = WriteHeader(writer, buffer); Image metadataImage = image; + foreach (ImageFrame frame in image.Frames) { cancellationToken.ThrowIfCancellationRequested(); @@ -235,9 +236,13 @@ internal sealed class TiffEncoderCore : IImageEncoderInternals if (image != null) { + // Write the metadata for the root image entriesCollector.ProcessMetadata(image, this.skipMetadata); } + // Write the metadata for the frame + entriesCollector.ProcessMetadata(frame, this.skipMetadata); + entriesCollector.ProcessFrameInfo(frame, imageMetadata); entriesCollector.ProcessImageFormat(this); diff --git a/src/ImageSharp/Formats/Tiff/TiffEncoderEntriesCollector.cs b/src/ImageSharp/Formats/Tiff/TiffEncoderEntriesCollector.cs index cf9b4ae21..e7ee0c65b 100644 --- a/src/ImageSharp/Formats/Tiff/TiffEncoderEntriesCollector.cs +++ b/src/ImageSharp/Formats/Tiff/TiffEncoderEntriesCollector.cs @@ -7,6 +7,7 @@ using SixLabors.ImageSharp.Formats.Tiff.Constants; using SixLabors.ImageSharp.Metadata; using SixLabors.ImageSharp.Metadata.Profiles.Exif; using SixLabors.ImageSharp.Metadata.Profiles.Xmp; +using SixLabors.ImageSharp.PixelFormats; namespace SixLabors.ImageSharp.Formats.Tiff; @@ -19,6 +20,9 @@ internal class TiffEncoderEntriesCollector public void ProcessMetadata(Image image, bool skipMetadata) => new MetadataProcessor(this).Process(image, skipMetadata); + public void ProcessMetadata(ImageFrame frame, bool skipMetadata) + => new MetadataProcessor(this).Process(frame, skipMetadata); + public void ProcessFrameInfo(ImageFrame frame, ImageMetadata imageMetadata) => new FrameInfoProcessor(this).Process(frame, imageMetadata); @@ -56,15 +60,30 @@ internal class TiffEncoderEntriesCollector public void Process(Image image, bool skipMetadata) { - ImageFrame rootFrame = image.Frames.RootFrame; - ExifProfile rootFrameExifProfile = rootFrame.Metadata.ExifProfile; - XmpProfile rootFrameXmpProfile = rootFrame.Metadata.XmpProfile; + this.ProcessProfiles(image.Metadata, skipMetadata); - this.ProcessProfiles(image.Metadata, skipMetadata, rootFrameExifProfile, rootFrameXmpProfile); + if (!skipMetadata) + { + this.ProcessMetadata(image.Metadata.ExifProfile ?? new ExifProfile()); + } + + if (!this.Collector.Entries.Exists(t => t.Tag == ExifTag.Software)) + { + this.Collector.Add(new ExifString(ExifTagValue.Software) + { + Value = SoftwareValue + }); + } + } + + + public void Process(ImageFrame frame, bool skipMetadata) + { + this.ProcessProfiles(frame.Metadata, skipMetadata); if (!skipMetadata) { - this.ProcessMetadata(rootFrameExifProfile ?? new ExifProfile()); + this.ProcessMetadata(frame.Metadata.ExifProfile ?? new ExifProfile()); } if (!this.Collector.Entries.Exists(t => t.Tag == ExifTag.Software)) @@ -150,16 +169,16 @@ internal class TiffEncoderEntriesCollector } } - private void ProcessProfiles(ImageMetadata imageMetadata, bool skipMetadata, ExifProfile exifProfile, XmpProfile xmpProfile) + private void ProcessProfiles(ImageMetadata imageMetadata, bool skipMetadata) { - if (!skipMetadata && (exifProfile != null && exifProfile.Parts != ExifParts.None)) + if (!skipMetadata && (imageMetadata.ExifProfile != null && imageMetadata.ExifProfile.Parts != ExifParts.None)) { - foreach (IExifValue entry in exifProfile.Values) + foreach (IExifValue entry in imageMetadata.ExifProfile.Values) { if (!this.Collector.Entries.Exists(t => t.Tag == entry.Tag) && entry.GetValue() != null) { ExifParts entryPart = ExifTags.GetPart(entry.Tag); - if (entryPart != ExifParts.None && exifProfile.Parts.HasFlag(entryPart)) + if (entryPart != ExifParts.None && imageMetadata.ExifProfile.Parts.HasFlag(entryPart)) { this.Collector.AddOrReplace(entry.DeepClone()); } @@ -168,7 +187,7 @@ internal class TiffEncoderEntriesCollector } else { - exifProfile?.RemoveValue(ExifTag.SubIFDOffset); + imageMetadata.ExifProfile?.RemoveValue(ExifTag.SubIFDOffset); } if (!skipMetadata && imageMetadata.IptcProfile != null) @@ -183,7 +202,7 @@ internal class TiffEncoderEntriesCollector } else { - exifProfile?.RemoveValue(ExifTag.IPTC); + imageMetadata.ExifProfile?.RemoveValue(ExifTag.IPTC); } if (imageMetadata.IccProfile != null) @@ -197,21 +216,86 @@ internal class TiffEncoderEntriesCollector } else { - exifProfile?.RemoveValue(ExifTag.IccProfile); + imageMetadata.ExifProfile?.RemoveValue(ExifTag.IccProfile); + } + + if (!skipMetadata && imageMetadata.XmpProfile != null) + { + ExifByteArray xmp = new(ExifTagValue.XMP, ExifDataType.Byte) + { + Value = imageMetadata.XmpProfile.Data + }; + + this.Collector.AddOrReplace(xmp); + } + else + { + imageMetadata.ExifProfile?.RemoveValue(ExifTag.XMP); + } + } + + private void ProcessProfiles(ImageFrameMetadata frameMetadata, bool skipMetadata) + { + if (!skipMetadata && (frameMetadata.ExifProfile != null && frameMetadata.ExifProfile.Parts != ExifParts.None)) + { + foreach (IExifValue entry in frameMetadata.ExifProfile.Values) + { + if (!this.Collector.Entries.Exists(t => t.Tag == entry.Tag) && entry.GetValue() != null) + { + ExifParts entryPart = ExifTags.GetPart(entry.Tag); + if (entryPart != ExifParts.None && frameMetadata.ExifProfile.Parts.HasFlag(entryPart)) + { + this.Collector.AddOrReplace(entry.DeepClone()); + } + } + } + } + else + { + frameMetadata.ExifProfile?.RemoveValue(ExifTag.SubIFDOffset); + } + + if (!skipMetadata && frameMetadata.IptcProfile != null) + { + frameMetadata.IptcProfile.UpdateData(); + ExifByteArray iptc = new(ExifTagValue.IPTC, ExifDataType.Byte) + { + Value = frameMetadata.IptcProfile.Data + }; + + this.Collector.AddOrReplace(iptc); + } + else + { + frameMetadata.ExifProfile?.RemoveValue(ExifTag.IPTC); + } + + if (frameMetadata.IccProfile != null) + { + ExifByteArray icc = new(ExifTagValue.IccProfile, ExifDataType.Undefined) + { + Value = frameMetadata.IccProfile.ToByteArray() + }; + + this.Collector.AddOrReplace(icc); + } + else + { + frameMetadata.ExifProfile?.RemoveValue(ExifTag.IccProfile); } - if (!skipMetadata && xmpProfile != null) + if (!skipMetadata && frameMetadata.XmpProfile != null) { ExifByteArray xmp = new(ExifTagValue.XMP, ExifDataType.Byte) { - Value = xmpProfile.Data + Value = frameMetadata.XmpProfile.Data }; this.Collector.AddOrReplace(xmp); } else { - exifProfile?.RemoveValue(ExifTag.XMP); + frameMetadata.ExifProfile?.RemoveValue(ExifTag.XMP); } } } diff --git a/tests/ImageSharp.Tests/Formats/Tiff/TiffMetadataTests.cs b/tests/ImageSharp.Tests/Formats/Tiff/TiffMetadataTests.cs index b671addf9..1225bbc8c 100644 --- a/tests/ImageSharp.Tests/Formats/Tiff/TiffMetadataTests.cs +++ b/tests/ImageSharp.Tests/Formats/Tiff/TiffMetadataTests.cs @@ -7,6 +7,7 @@ using SixLabors.ImageSharp.Formats.Tiff; using SixLabors.ImageSharp.Formats.Tiff.Constants; using SixLabors.ImageSharp.Metadata; using SixLabors.ImageSharp.Metadata.Profiles.Exif; +using SixLabors.ImageSharp.Metadata.Profiles.Icc; using SixLabors.ImageSharp.Metadata.Profiles.Iptc; using SixLabors.ImageSharp.Metadata.Profiles.Xmp; using SixLabors.ImageSharp.PixelFormats; @@ -318,4 +319,94 @@ public class TiffMetadataTests Assert.Equal((ushort)TiffPlanarConfiguration.Chunky, encodedImageExifProfile.GetValue(ExifTag.PlanarConfiguration)?.Value); Assert.Equal(exifProfileInput.Values.Count, encodedImageExifProfile.Values.Count); } + + [Theory] + [WithFile(SampleMetadata, PixelTypes.Rgba32)] + public void Encode_PreservesMetadata_IptcAndIcc(TestImageProvider provider) + where TPixel : unmanaged, IPixel + { + // Load Tiff image + DecoderOptions options = new() { SkipMetadata = false }; + using Image image = provider.GetImage(TiffDecoder.Instance, options); + + ImageMetadata inputMetaData = image.Metadata; + ImageFrame rootFrameInput = image.Frames.RootFrame; + + IptcProfile iptcProfile = new(); + iptcProfile.SetValue(IptcTag.Name, "Test name"); + rootFrameInput.Metadata.IptcProfile = iptcProfile; + + IccProfileHeader iccProfileHeader = new IccProfileHeader(); + iccProfileHeader.Class = IccProfileClass.ColorSpace; + IccProfile iccProfile = new(); + rootFrameInput.Metadata.IccProfile = iccProfile; + + TiffFrameMetadata frameMetaInput = rootFrameInput.Metadata.GetTiffMetadata(); + XmpProfile xmpProfileInput = rootFrameInput.Metadata.XmpProfile; + ExifProfile exifProfileInput = rootFrameInput.Metadata.ExifProfile; + IptcProfile iptcProfileInput = rootFrameInput.Metadata.IptcProfile; + IccProfile iccProfileInput = rootFrameInput.Metadata.IccProfile; + + Assert.Equal(TiffCompression.Lzw, frameMetaInput.Compression); + Assert.Equal(TiffBitsPerPixel.Bit4, frameMetaInput.BitsPerPixel); + + // Save to Tiff + TiffEncoder tiffEncoder = new() { PhotometricInterpretation = TiffPhotometricInterpretation.Rgb }; + using MemoryStream ms = new(); + image.Save(ms, tiffEncoder); + + // Assert + ms.Position = 0; + using Image encodedImage = Image.Load(ms); + + ImageMetadata encodedImageMetaData = encodedImage.Metadata; + ImageFrame rootFrameEncodedImage = encodedImage.Frames.RootFrame; + TiffFrameMetadata tiffMetaDataEncodedRootFrame = rootFrameEncodedImage.Metadata.GetTiffMetadata(); + ExifProfile encodedImageExifProfile = rootFrameEncodedImage.Metadata.ExifProfile; + XmpProfile encodedImageXmpProfile = rootFrameEncodedImage.Metadata.XmpProfile; + IptcProfile encodedImageIptcProfile = rootFrameEncodedImage.Metadata.IptcProfile; + IccProfile encodedImageIccProfile = rootFrameEncodedImage.Metadata.IccProfile; + + Assert.Equal(TiffBitsPerPixel.Bit4, tiffMetaDataEncodedRootFrame.BitsPerPixel); + Assert.Equal(TiffCompression.Lzw, tiffMetaDataEncodedRootFrame.Compression); + + Assert.Equal(inputMetaData.HorizontalResolution, encodedImageMetaData.HorizontalResolution); + Assert.Equal(inputMetaData.VerticalResolution, encodedImageMetaData.VerticalResolution); + Assert.Equal(inputMetaData.ResolutionUnits, encodedImageMetaData.ResolutionUnits); + + Assert.Equal(rootFrameInput.Width, rootFrameEncodedImage.Width); + Assert.Equal(rootFrameInput.Height, rootFrameEncodedImage.Height); + + PixelResolutionUnit resolutionUnitInput = UnitConverter.ExifProfileToResolutionUnit(exifProfileInput); + PixelResolutionUnit resolutionUnitEncoded = UnitConverter.ExifProfileToResolutionUnit(encodedImageExifProfile); + Assert.Equal(resolutionUnitInput, resolutionUnitEncoded); + Assert.Equal(exifProfileInput.GetValue(ExifTag.XResolution).Value.ToDouble(), encodedImageExifProfile.GetValue(ExifTag.XResolution).Value.ToDouble()); + Assert.Equal(exifProfileInput.GetValue(ExifTag.YResolution).Value.ToDouble(), encodedImageExifProfile.GetValue(ExifTag.YResolution).Value.ToDouble()); + + Assert.NotNull(xmpProfileInput); + Assert.NotNull(encodedImageXmpProfile); + Assert.Equal(xmpProfileInput.Data, encodedImageXmpProfile.Data); + + Assert.NotNull(iptcProfileInput); + Assert.NotNull(encodedImageIptcProfile); + Assert.Equal(iptcProfileInput.Data, encodedImageIptcProfile.Data); + Assert.Equal(iptcProfileInput.GetValues(IptcTag.Name)[0].Value, encodedImageIptcProfile.GetValues(IptcTag.Name)[0].Value); + + Assert.NotNull(iccProfileInput); + Assert.NotNull(encodedImageIccProfile); + Assert.Equal(iccProfileInput.Entries.Length, encodedImageIccProfile.Entries.Length); + Assert.Equal(iccProfileInput.Header.Class, encodedImageIccProfile.Header.Class); + + Assert.Equal(exifProfileInput.GetValue(ExifTag.Software).Value, encodedImageExifProfile.GetValue(ExifTag.Software).Value); + Assert.Equal(exifProfileInput.GetValue(ExifTag.ImageDescription).Value, encodedImageExifProfile.GetValue(ExifTag.ImageDescription).Value); + Assert.Equal(exifProfileInput.GetValue(ExifTag.Make).Value, encodedImageExifProfile.GetValue(ExifTag.Make).Value); + Assert.Equal(exifProfileInput.GetValue(ExifTag.Copyright).Value, encodedImageExifProfile.GetValue(ExifTag.Copyright).Value); + Assert.Equal(exifProfileInput.GetValue(ExifTag.Artist).Value, encodedImageExifProfile.GetValue(ExifTag.Artist).Value); + Assert.Equal(exifProfileInput.GetValue(ExifTag.Orientation).Value, encodedImageExifProfile.GetValue(ExifTag.Orientation).Value); + Assert.Equal(exifProfileInput.GetValue(ExifTag.Model).Value, encodedImageExifProfile.GetValue(ExifTag.Model).Value); + + Assert.Equal((ushort)TiffPlanarConfiguration.Chunky, encodedImageExifProfile.GetValue(ExifTag.PlanarConfiguration)?.Value); + // Adding the IPTC and ICC profiles dynamically increments the number of values in the original EXIF profile by 2 + Assert.Equal(exifProfileInput.Values.Count + 2, encodedImageExifProfile.Values.Count); + } } From bf261f0d5efcb732da1a28a1f767d60344040f6c Mon Sep 17 00:00:00 2001 From: Jeffrey Parker Date: Wed, 13 Sep 2023 23:24:14 -0500 Subject: [PATCH 7/7] Stylecop extra blank line issue fixed. Refactored code to extract common logic. --- .../Tiff/TiffEncoderEntriesCollector.cs | 114 ++++++------------ .../Formats/Tiff/TiffMetadataTests.cs | 4 +- 2 files changed, 39 insertions(+), 79 deletions(-) diff --git a/src/ImageSharp/Formats/Tiff/TiffEncoderEntriesCollector.cs b/src/ImageSharp/Formats/Tiff/TiffEncoderEntriesCollector.cs index e7ee0c65b..c8e28111e 100644 --- a/src/ImageSharp/Formats/Tiff/TiffEncoderEntriesCollector.cs +++ b/src/ImageSharp/Formats/Tiff/TiffEncoderEntriesCollector.cs @@ -6,8 +6,9 @@ using SixLabors.ImageSharp.Common.Helpers; using SixLabors.ImageSharp.Formats.Tiff.Constants; using SixLabors.ImageSharp.Metadata; using SixLabors.ImageSharp.Metadata.Profiles.Exif; +using SixLabors.ImageSharp.Metadata.Profiles.Icc; +using SixLabors.ImageSharp.Metadata.Profiles.Iptc; using SixLabors.ImageSharp.Metadata.Profiles.Xmp; -using SixLabors.ImageSharp.PixelFormats; namespace SixLabors.ImageSharp.Formats.Tiff; @@ -76,7 +77,6 @@ internal class TiffEncoderEntriesCollector } } - public void Process(ImageFrame frame, bool skipMetadata) { this.ProcessProfiles(frame.Metadata, skipMetadata); @@ -171,79 +171,30 @@ internal class TiffEncoderEntriesCollector private void ProcessProfiles(ImageMetadata imageMetadata, bool skipMetadata) { - if (!skipMetadata && (imageMetadata.ExifProfile != null && imageMetadata.ExifProfile.Parts != ExifParts.None)) - { - foreach (IExifValue entry in imageMetadata.ExifProfile.Values) - { - if (!this.Collector.Entries.Exists(t => t.Tag == entry.Tag) && entry.GetValue() != null) - { - ExifParts entryPart = ExifTags.GetPart(entry.Tag); - if (entryPart != ExifParts.None && imageMetadata.ExifProfile.Parts.HasFlag(entryPart)) - { - this.Collector.AddOrReplace(entry.DeepClone()); - } - } - } - } - else - { - imageMetadata.ExifProfile?.RemoveValue(ExifTag.SubIFDOffset); - } - - if (!skipMetadata && imageMetadata.IptcProfile != null) - { - imageMetadata.IptcProfile.UpdateData(); - ExifByteArray iptc = new(ExifTagValue.IPTC, ExifDataType.Byte) - { - Value = imageMetadata.IptcProfile.Data - }; - - this.Collector.AddOrReplace(iptc); - } - else - { - imageMetadata.ExifProfile?.RemoveValue(ExifTag.IPTC); - } - - if (imageMetadata.IccProfile != null) - { - ExifByteArray icc = new(ExifTagValue.IccProfile, ExifDataType.Undefined) - { - Value = imageMetadata.IccProfile.ToByteArray() - }; - - this.Collector.AddOrReplace(icc); - } - else - { - imageMetadata.ExifProfile?.RemoveValue(ExifTag.IccProfile); - } - - if (!skipMetadata && imageMetadata.XmpProfile != null) - { - ExifByteArray xmp = new(ExifTagValue.XMP, ExifDataType.Byte) - { - Value = imageMetadata.XmpProfile.Data - }; - - this.Collector.AddOrReplace(xmp); - } - else - { - imageMetadata.ExifProfile?.RemoveValue(ExifTag.XMP); - } + this.ProcessExifProfile(skipMetadata, imageMetadata.ExifProfile); + this.ProcessIptcProfile(skipMetadata, imageMetadata.IptcProfile, imageMetadata.ExifProfile); + this.ProcessIccProfile(imageMetadata.IccProfile, imageMetadata.ExifProfile); + this.ProcessXmpProfile(skipMetadata, imageMetadata.XmpProfile, imageMetadata.ExifProfile); } private void ProcessProfiles(ImageFrameMetadata frameMetadata, bool skipMetadata) { - if (!skipMetadata && (frameMetadata.ExifProfile != null && frameMetadata.ExifProfile.Parts != ExifParts.None)) + this.ProcessExifProfile(skipMetadata, frameMetadata.ExifProfile); + this.ProcessIptcProfile(skipMetadata, frameMetadata.IptcProfile, frameMetadata.ExifProfile); + this.ProcessIccProfile(frameMetadata.IccProfile, frameMetadata.ExifProfile); + this.ProcessXmpProfile(skipMetadata, frameMetadata.XmpProfile, frameMetadata.ExifProfile); + } + + private void ProcessExifProfile(bool skipMetadata, ExifProfile exifProfile) + { + if (!skipMetadata && (exifProfile != null && exifProfile.Parts != ExifParts.None)) { - foreach (IExifValue entry in frameMetadata.ExifProfile.Values) + foreach (IExifValue entry in exifProfile.Values) { if (!this.Collector.Entries.Exists(t => t.Tag == entry.Tag) && entry.GetValue() != null) { ExifParts entryPart = ExifTags.GetPart(entry.Tag); - if (entryPart != ExifParts.None && frameMetadata.ExifProfile.Parts.HasFlag(entryPart)) + if (entryPart != ExifParts.None && exifProfile.Parts.HasFlag(entryPart)) { this.Collector.AddOrReplace(entry.DeepClone()); } @@ -252,50 +203,59 @@ internal class TiffEncoderEntriesCollector } else { - frameMetadata.ExifProfile?.RemoveValue(ExifTag.SubIFDOffset); + exifProfile?.RemoveValue(ExifTag.SubIFDOffset); } + } - if (!skipMetadata && frameMetadata.IptcProfile != null) + private void ProcessIptcProfile(bool skipMetadata, IptcProfile iptcProfile, ExifProfile exifProfile) + { + if (!skipMetadata && iptcProfile != null) { - frameMetadata.IptcProfile.UpdateData(); + iptcProfile.UpdateData(); ExifByteArray iptc = new(ExifTagValue.IPTC, ExifDataType.Byte) { - Value = frameMetadata.IptcProfile.Data + Value = iptcProfile.Data }; this.Collector.AddOrReplace(iptc); } else { - frameMetadata.ExifProfile?.RemoveValue(ExifTag.IPTC); + exifProfile?.RemoveValue(ExifTag.IPTC); } + } - if (frameMetadata.IccProfile != null) + private void ProcessIccProfile(IccProfile iccProfile, ExifProfile exifProfile) + { + if (iccProfile != null) { ExifByteArray icc = new(ExifTagValue.IccProfile, ExifDataType.Undefined) { - Value = frameMetadata.IccProfile.ToByteArray() + Value = iccProfile.ToByteArray() }; this.Collector.AddOrReplace(icc); } else { - frameMetadata.ExifProfile?.RemoveValue(ExifTag.IccProfile); + exifProfile?.RemoveValue(ExifTag.IccProfile); } + } - if (!skipMetadata && frameMetadata.XmpProfile != null) + private void ProcessXmpProfile(bool skipMetadata, XmpProfile xmpProfile, ExifProfile exifProfile) + { + if (!skipMetadata && xmpProfile != null) { ExifByteArray xmp = new(ExifTagValue.XMP, ExifDataType.Byte) { - Value = frameMetadata.XmpProfile.Data + Value = xmpProfile.Data }; this.Collector.AddOrReplace(xmp); } else { - frameMetadata.ExifProfile?.RemoveValue(ExifTag.XMP); + exifProfile?.RemoveValue(ExifTag.XMP); } } } diff --git a/tests/ImageSharp.Tests/Formats/Tiff/TiffMetadataTests.cs b/tests/ImageSharp.Tests/Formats/Tiff/TiffMetadataTests.cs index 1225bbc8c..e31487cd2 100644 --- a/tests/ImageSharp.Tests/Formats/Tiff/TiffMetadataTests.cs +++ b/tests/ImageSharp.Tests/Formats/Tiff/TiffMetadataTests.cs @@ -336,8 +336,7 @@ public class TiffMetadataTests iptcProfile.SetValue(IptcTag.Name, "Test name"); rootFrameInput.Metadata.IptcProfile = iptcProfile; - IccProfileHeader iccProfileHeader = new IccProfileHeader(); - iccProfileHeader.Class = IccProfileClass.ColorSpace; + IccProfileHeader iccProfileHeader = new() { Class = IccProfileClass.ColorSpace }; IccProfile iccProfile = new(); rootFrameInput.Metadata.IccProfile = iccProfile; @@ -406,6 +405,7 @@ public class TiffMetadataTests Assert.Equal(exifProfileInput.GetValue(ExifTag.Model).Value, encodedImageExifProfile.GetValue(ExifTag.Model).Value); Assert.Equal((ushort)TiffPlanarConfiguration.Chunky, encodedImageExifProfile.GetValue(ExifTag.PlanarConfiguration)?.Value); + // Adding the IPTC and ICC profiles dynamically increments the number of values in the original EXIF profile by 2 Assert.Equal(exifProfileInput.Values.Count + 2, encodedImageExifProfile.Values.Count); }