From 77bb28711613023cea454820b8f0598aae8644ed Mon Sep 17 00:00:00 2001 From: Scott Williams Date: Sat, 9 Apr 2022 20:20:36 +0100 Subject: [PATCH 01/14] xunit helper to track undisposed memory --- .../Diagnostics/MemoryDiagnostics.cs | 110 +++++++++++++----- .../Formats/Png/PngDecoderTests.cs | 4 + .../Image/ImageTests.LoadPixelData.cs | 1 + .../Image/LargeImageIntegrationTests.cs | 2 + .../MemoryAllocatorValidator.cs | 43 +++++++ .../ImageProviders/FileProvider.cs | 8 +- ...idateDisposedMemoryAllocationsAttribute.cs | 36 ++++++ 7 files changed, 177 insertions(+), 27 deletions(-) create mode 100644 tests/ImageSharp.Tests/MemoryAllocatorValidator.cs create mode 100644 tests/ImageSharp.Tests/ValidateDisposedMemoryAllocationsAttribute.cs diff --git a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs index d83e737f12..6af2544138 100644 --- a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs +++ b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs @@ -15,10 +15,8 @@ namespace SixLabors.ImageSharp.Diagnostics /// public static class MemoryDiagnostics { - private static int totalUndisposedAllocationCount; - - private static UndisposedAllocationDelegate undisposedAllocation; - private static int undisposedAllocationSubscriptionCounter; + internal static readonly InteralMemoryDiagnostics Default = new(); + private static AsyncLocal localInstance = null; private static readonly object SyncRoot = new(); /// @@ -28,56 +26,116 @@ public static class MemoryDiagnostics /// public static event UndisposedAllocationDelegate UndisposedAllocation { - add + add => Current.UndisposedAllocation += value; + remove => Current.UndisposedAllocation -= value; + } + + internal static InteralMemoryDiagnostics Current + { + get { - lock (SyncRoot) + if (localInstance != null && localInstance.Value != null) { - undisposedAllocationSubscriptionCounter++; - undisposedAllocation += value; + return localInstance.Value; } + + return Default; } - remove + set { - lock (SyncRoot) + if (localInstance == null) { - undisposedAllocation -= value; - undisposedAllocationSubscriptionCounter--; + lock (SyncRoot) + { + localInstance ??= new AsyncLocal(); + } } + + localInstance.Value = value; } } /// /// Gets a value indicating the total number of memory resource objects leaked to the finalizer. /// - public static int TotalUndisposedAllocationCount => totalUndisposedAllocationCount; + public static int TotalUndisposedAllocationCount => Current.TotalUndisposedAllocationCount; - internal static bool UndisposedAllocationSubscribed => Volatile.Read(ref undisposedAllocationSubscriptionCounter) > 0; + internal static bool UndisposedAllocationSubscribed => Current.UndisposedAllocationSubscribed; - internal static void IncrementTotalUndisposedAllocationCount() => - Interlocked.Increment(ref totalUndisposedAllocationCount); + internal static void IncrementTotalUndisposedAllocationCount() => Current.IncrementTotalUndisposedAllocationCount(); - internal static void DecrementTotalUndisposedAllocationCount() => - Interlocked.Decrement(ref totalUndisposedAllocationCount); + internal static void DecrementTotalUndisposedAllocationCount() => Current.DecrementTotalUndisposedAllocationCount(); internal static void RaiseUndisposedMemoryResource(string allocationStackTrace) + => Current.RaiseUndisposedMemoryResource(allocationStackTrace); + + internal class InteralMemoryDiagnostics { - if (undisposedAllocation is null) + private int totalUndisposedAllocationCount; + + private UndisposedAllocationDelegate undisposedAllocation; + private int undisposedAllocationSubscriptionCounter; + private readonly object syncRoot = new(); + + /// + /// Fires when an ImageSharp object's undisposed memory resource leaks to the finalizer. + /// The event brings significant overhead, and is intended to be used for troubleshooting only. + /// For production diagnostics, use . + /// + public event UndisposedAllocationDelegate UndisposedAllocation { - return; + add + { + lock (this.syncRoot) + { + this.undisposedAllocationSubscriptionCounter++; + this.undisposedAllocation += value; + } + } + + remove + { + lock (this.syncRoot) + { + this.undisposedAllocation -= value; + this.undisposedAllocationSubscriptionCounter--; + } + } } - // Schedule on the ThreadPool, to avoid user callback messing up the finalizer thread. + /// + /// Gets a value indicating the total number of memory resource objects leaked to the finalizer. + /// + public int TotalUndisposedAllocationCount => this.totalUndisposedAllocationCount; + + internal bool UndisposedAllocationSubscribed => Volatile.Read(ref this.undisposedAllocationSubscriptionCounter) > 0; + + internal void IncrementTotalUndisposedAllocationCount() => + Interlocked.Increment(ref this.totalUndisposedAllocationCount); + + internal void DecrementTotalUndisposedAllocationCount() => + Interlocked.Decrement(ref this.totalUndisposedAllocationCount); + + internal void RaiseUndisposedMemoryResource(string allocationStackTrace) + { + if (this.undisposedAllocation is null) + { + return; + } + + // Schedule on the ThreadPool, to avoid user callback messing up the finalizer thread. #if NETSTANDARD2_1 || NETCOREAPP2_1_OR_GREATER - ThreadPool.QueueUserWorkItem( - stackTrace => undisposedAllocation?.Invoke(stackTrace), - allocationStackTrace, - preferLocal: false); + ThreadPool.QueueUserWorkItem( + stackTrace => this.undisposedAllocation?.Invoke(stackTrace), + allocationStackTrace, + preferLocal: false); #else ThreadPool.QueueUserWorkItem( - stackTrace => undisposedAllocation?.Invoke((string)stackTrace), + stackTrace => this.undisposedAllocation?.Invoke((string)stackTrace), allocationStackTrace); #endif + } } } } diff --git a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs index 752036126f..0a796b5143 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs @@ -103,10 +103,14 @@ public async Task DecodeAsync_NonGeneric_CreatesCorrectImageType(string path, Ty [Theory] [WithFileCollection(nameof(CommonTestImages), PixelTypes.Rgba32)] + [ValidateDisposedMemoryAllocations] public void Decode(TestImageProvider provider) where TPixel : unmanaged, IPixel { using Image image = provider.GetImage(PngDecoder); + //var testFile = TestFile.Create(provider.SourceFileOrDescription); + //using Image image = Image.Load(provider.Configuration, testFile.Bytes, PngDecoder); + image.DebugSave(provider); image.CompareToOriginal(provider, ImageComparer.Exact); } diff --git a/tests/ImageSharp.Tests/Image/ImageTests.LoadPixelData.cs b/tests/ImageSharp.Tests/Image/ImageTests.LoadPixelData.cs index a4044b906c..7683ee6889 100644 --- a/tests/ImageSharp.Tests/Image/ImageTests.LoadPixelData.cs +++ b/tests/ImageSharp.Tests/Image/ImageTests.LoadPixelData.cs @@ -14,6 +14,7 @@ public class LoadPixelData [Theory] [InlineData(false)] [InlineData(true)] + [ValidateDisposedMemoryAllocations] public void FromPixels(bool useSpan) { Rgba32[] data = { Color.Black, Color.White, Color.White, Color.Black, }; diff --git a/tests/ImageSharp.Tests/Image/LargeImageIntegrationTests.cs b/tests/ImageSharp.Tests/Image/LargeImageIntegrationTests.cs index 357d02be4b..ce1f902e59 100644 --- a/tests/ImageSharp.Tests/Image/LargeImageIntegrationTests.cs +++ b/tests/ImageSharp.Tests/Image/LargeImageIntegrationTests.cs @@ -55,6 +55,8 @@ public void PreferContiguousImageBuffers_LoadImage_BufferIsContiguous(string for static void RunTest(string formatInner) { + using IDisposable mem = MemoryAllocatorValidator.MonitorAllocations(); + Configuration configuration = Configuration.Default.Clone(); configuration.PreferContiguousImageBuffers = true; IImageEncoder encoder = configuration.ImageFormatsManager.FindEncoder( diff --git a/tests/ImageSharp.Tests/MemoryAllocatorValidator.cs b/tests/ImageSharp.Tests/MemoryAllocatorValidator.cs new file mode 100644 index 0000000000..2c3d98eb13 --- /dev/null +++ b/tests/ImageSharp.Tests/MemoryAllocatorValidator.cs @@ -0,0 +1,43 @@ +// Copyright (c) Six Labors. +// Licensed under the Apache License, Version 2.0. + +using System; +using System.Diagnostics; +using SixLabors.ImageSharp.Diagnostics; +using Xunit; + +namespace SixLabors.ImageSharp.Tests +{ + public static class MemoryAllocatorValidator + { + public static IDisposable MonitorAllocations(int max = 0) + { + MemoryDiagnostics.Current = new(); + return new TestMemoryAllocatorDisposable(max); + } + + public static void ValidateAllocation(int max = 0) + { + var count = MemoryDiagnostics.TotalUndisposedAllocationCount; + var pass = count <= max; + Assert.True(pass, $"Expected a max of {max} undisposed buffers but found {count}"); + + if (count > 0) + { + Debug.WriteLine("We should have Zero undisposed memory allocations."); + } + + MemoryDiagnostics.Current = null; + } + + public struct TestMemoryAllocatorDisposable : IDisposable + { + private readonly int max; + + public TestMemoryAllocatorDisposable(int max) => this.max = max; + + public void Dispose() + => ValidateAllocation(this.max); + } + } +} diff --git a/tests/ImageSharp.Tests/TestUtilities/ImageProviders/FileProvider.cs b/tests/ImageSharp.Tests/TestUtilities/ImageProviders/FileProvider.cs index e2e7d73bc6..3675356c38 100644 --- a/tests/ImageSharp.Tests/TestUtilities/ImageProviders/FileProvider.cs +++ b/tests/ImageSharp.Tests/TestUtilities/ImageProviders/FileProvider.cs @@ -7,6 +7,7 @@ using System.IO; using System.Reflection; using System.Threading.Tasks; +using SixLabors.ImageSharp.Diagnostics; using SixLabors.ImageSharp.Formats; using SixLabors.ImageSharp.Memory; using SixLabors.ImageSharp.PixelFormats; @@ -158,8 +159,13 @@ public override Image GetImage(IImageDecoder decoder) return this.LoadImage(decoder); } - var key = new Key(this.PixelType, this.FilePath, decoder); + // do cache so we can track allocation correctly when validating memory + if (MemoryDiagnostics.Current != MemoryDiagnostics.Default) + { + return this.LoadImage(decoder); + } + var key = new Key(this.PixelType, this.FilePath, decoder); Image cachedImage = Cache.GetOrAdd(key, _ => this.LoadImage(decoder)); return cachedImage.Clone(this.Configuration); diff --git a/tests/ImageSharp.Tests/ValidateDisposedMemoryAllocationsAttribute.cs b/tests/ImageSharp.Tests/ValidateDisposedMemoryAllocationsAttribute.cs new file mode 100644 index 0000000000..95d13b5954 --- /dev/null +++ b/tests/ImageSharp.Tests/ValidateDisposedMemoryAllocationsAttribute.cs @@ -0,0 +1,36 @@ +// Copyright (c) Six Labors. +// Licensed under the Apache License, Version 2.0. + +using System; +using System.Diagnostics; +using System.Reflection; +using Xunit.Sdk; + +namespace SixLabors.ImageSharp.Tests +{ + [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)] + public class ValidateDisposedMemoryAllocationsAttribute : BeforeAfterTestAttribute + { + private readonly int max = 0; + + public ValidateDisposedMemoryAllocationsAttribute() + : this(0) + { + } + + public ValidateDisposedMemoryAllocationsAttribute(int max) + { + this.max = max; + if (max > 0) + { + Debug.WriteLine("Needs fixing, we shoudl have Zero undisposed memory allocations."); + } + } + + public override void Before(MethodInfo methodUnderTest) + => MemoryAllocatorValidator.MonitorAllocations(this.max); // the disposable isn't important cause the validate below does the same thing + + public override void After(MethodInfo methodUnderTest) + => MemoryAllocatorValidator.ValidateAllocation(this.max); + } +} From 3872c6000c74f29cb6069e40e2a3fe09fce3d3a0 Mon Sep 17 00:00:00 2001 From: Scott Williams Date: Sat, 9 Apr 2022 20:21:37 +0100 Subject: [PATCH 02/14] revert loader change --- tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs index 0a796b5143..eadbf138a6 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs @@ -108,9 +108,6 @@ public void Decode(TestImageProvider provider) where TPixel : unmanaged, IPixel { using Image image = provider.GetImage(PngDecoder); - //var testFile = TestFile.Create(provider.SourceFileOrDescription); - //using Image image = Image.Load(provider.Configuration, testFile.Bytes, PngDecoder); - image.DebugSave(provider); image.CompareToOriginal(provider, ImageComparer.Exact); } From af7d84a376379afd9f7214ff5f34f12f5785ff2f Mon Sep 17 00:00:00 2001 From: Scott Williams Date: Sat, 9 Apr 2022 21:06:37 +0100 Subject: [PATCH 03/14] check disposed buffers for decoders --- src/ImageSharp/Formats/Bmp/BmpDecoderCore.cs | 9 ++- src/ImageSharp/Formats/Gif/GifDecoderCore.cs | 5 ++ .../Decompressors/JpegTiffCompression.cs | 6 +- .../Formats/Tiff/TiffDecoderCore.cs | 6 +- .../Formats/Webp/WebpDecoderCore.cs | 62 +++++++++++-------- .../Formats/Bmp/BmpDecoderTests.cs | 1 + .../Formats/Gif/GifDecoderTests.cs | 1 + .../Formats/Jpg/JpegDecoderTests.Metadata.cs | 15 +++-- .../Formats/Jpg/JpegDecoderTests.cs | 1 + .../Formats/Pbm/PbmDecoderTests.cs | 1 + .../Formats/Png/PngDecoderTests.cs | 2 +- .../Formats/Tga/TgaDecoderTests.cs | 1 + .../Formats/Tiff/TiffDecoderTests.cs | 1 + .../Formats/WebP/WebpDecoderTests.cs | 1 + .../ImageComparison/ImageComparingUtils.cs | 2 +- 15 files changed, 74 insertions(+), 40 deletions(-) diff --git a/src/ImageSharp/Formats/Bmp/BmpDecoderCore.cs b/src/ImageSharp/Formats/Bmp/BmpDecoderCore.cs index 962c9f0d67..ee0a312803 100644 --- a/src/ImageSharp/Formats/Bmp/BmpDecoderCore.cs +++ b/src/ImageSharp/Formats/Bmp/BmpDecoderCore.cs @@ -122,11 +122,12 @@ public BmpDecoderCore(Configuration configuration, IBmpDecoderOptions options) public Image Decode(BufferedReadStream stream, CancellationToken cancellationToken) where TPixel : unmanaged, IPixel { + Image image = null; try { int bytesPerColorMapEntry = this.ReadImageHeaders(stream, out bool inverted, out byte[] palette); - var image = new Image(this.Configuration, this.infoHeader.Width, this.infoHeader.Height, this.metadata); + image = new Image(this.Configuration, this.infoHeader.Width, this.infoHeader.Height, this.metadata); Buffer2D pixels = image.GetRootFramePixelBuffer(); @@ -193,8 +194,14 @@ public Image Decode(BufferedReadStream stream, CancellationToken } catch (IndexOutOfRangeException e) { + image?.Dispose(); throw new ImageFormatException("Bitmap does not have a valid format.", e); } + catch + { + image?.Dispose(); + throw; + } } /// diff --git a/src/ImageSharp/Formats/Gif/GifDecoderCore.cs b/src/ImageSharp/Formats/Gif/GifDecoderCore.cs index 16dca3324f..ab857a404a 100644 --- a/src/ImageSharp/Formats/Gif/GifDecoderCore.cs +++ b/src/ImageSharp/Formats/Gif/GifDecoderCore.cs @@ -151,6 +151,11 @@ public Image Decode(BufferedReadStream stream, CancellationToken } } } + //catch + //{ + // image?.Dispose(); + // throw; + //} finally { this.globalColorTable?.Dispose(); diff --git a/src/ImageSharp/Formats/Tiff/Compression/Decompressors/JpegTiffCompression.cs b/src/ImageSharp/Formats/Tiff/Compression/Decompressors/JpegTiffCompression.cs index cfbc32f4f6..4e788c76af 100644 --- a/src/ImageSharp/Formats/Tiff/Compression/Decompressors/JpegTiffCompression.cs +++ b/src/ImageSharp/Formats/Tiff/Compression/Decompressors/JpegTiffCompression.cs @@ -65,7 +65,8 @@ protected override void Decompress(BufferedReadStream stream, int byteCount, int jpegDecoder.ParseStream(stream, scanDecoderGray, CancellationToken.None); // TODO: Should we pass through the CancellationToken from the tiff decoder? - CopyImageBytesToBuffer(buffer, spectralConverterGray.GetPixelBuffer(CancellationToken.None)); + using var decompressedBuffer = spectralConverterGray.GetPixelBuffer(CancellationToken.None); + CopyImageBytesToBuffer(buffer, decompressedBuffer); break; } @@ -81,7 +82,8 @@ protected override void Decompress(BufferedReadStream stream, int byteCount, int jpegDecoder.ParseStream(stream, scanDecoder, CancellationToken.None); // TODO: Should we pass through the CancellationToken from the tiff decoder? - CopyImageBytesToBuffer(buffer, spectralConverter.GetPixelBuffer(CancellationToken.None)); + using var decompressedBuffer = spectralConverter.GetPixelBuffer(CancellationToken.None); + CopyImageBytesToBuffer(buffer, decompressedBuffer); break; } diff --git a/src/ImageSharp/Formats/Tiff/TiffDecoderCore.cs b/src/ImageSharp/Formats/Tiff/TiffDecoderCore.cs index 1198c519a2..5dd99fafc1 100644 --- a/src/ImageSharp/Formats/Tiff/TiffDecoderCore.cs +++ b/src/ImageSharp/Formats/Tiff/TiffDecoderCore.cs @@ -240,8 +240,8 @@ private ImageFrame DecodeFrame(ExifProfile tags, CancellationTok var stripOffsetsArray = (Array)tags.GetValueInternal(ExifTag.StripOffsets).GetValue(); var stripByteCountsArray = (Array)tags.GetValueInternal(ExifTag.StripByteCounts).GetValue(); - IMemoryOwner stripOffsetsMemory = this.ConvertNumbers(stripOffsetsArray, out Span stripOffsets); - IMemoryOwner stripByteCountsMemory = this.ConvertNumbers(stripByteCountsArray, out Span stripByteCounts); + using IMemoryOwner stripOffsetsMemory = this.ConvertNumbers(stripOffsetsArray, out Span stripOffsets); + using IMemoryOwner stripByteCountsMemory = this.ConvertNumbers(stripByteCountsArray, out Span stripByteCounts); if (this.PlanarConfiguration == TiffPlanarConfiguration.Planar) { @@ -262,8 +262,6 @@ private ImageFrame DecodeFrame(ExifProfile tags, CancellationTok cancellationToken); } - stripOffsetsMemory?.Dispose(); - stripByteCountsMemory?.Dispose(); return frame; } diff --git a/src/ImageSharp/Formats/Webp/WebpDecoderCore.cs b/src/ImageSharp/Formats/Webp/WebpDecoderCore.cs index 9d18e5d821..5646f71fac 100644 --- a/src/ImageSharp/Formats/Webp/WebpDecoderCore.cs +++ b/src/ImageSharp/Formats/Webp/WebpDecoderCore.cs @@ -82,38 +82,48 @@ public WebpDecoderCore(Configuration configuration, IWebpDecoderOptions options) public Image Decode(BufferedReadStream stream, CancellationToken cancellationToken) where TPixel : unmanaged, IPixel { - this.Metadata = new ImageMetadata(); - this.currentStream = stream; + Image image = null; + try + { - uint fileSize = this.ReadImageHeader(); + this.Metadata = new ImageMetadata(); + this.currentStream = stream; - using (this.webImageInfo = this.ReadVp8Info()) - { - if (this.webImageInfo.Features is { Animation: true }) - { - WebpThrowHelper.ThrowNotSupportedException("Animations are not supported"); - } + uint fileSize = this.ReadImageHeader(); - var image = new Image(this.Configuration, (int)this.webImageInfo.Width, (int)this.webImageInfo.Height, this.Metadata); - Buffer2D pixels = image.GetRootFramePixelBuffer(); - if (this.webImageInfo.IsLossless) + using (this.webImageInfo = this.ReadVp8Info()) { - var losslessDecoder = new WebpLosslessDecoder(this.webImageInfo.Vp8LBitReader, this.memoryAllocator, this.Configuration); - losslessDecoder.Decode(pixels, image.Width, image.Height); - } - else - { - var lossyDecoder = new WebpLossyDecoder(this.webImageInfo.Vp8BitReader, this.memoryAllocator, this.Configuration); - lossyDecoder.Decode(pixels, image.Width, image.Height, this.webImageInfo); - } + if (this.webImageInfo.Features is { Animation: true }) + { + WebpThrowHelper.ThrowNotSupportedException("Animations are not supported"); + } - // There can be optional chunks after the image data, like EXIF and XMP. - if (this.webImageInfo.Features != null) - { - this.ParseOptionalChunks(this.webImageInfo.Features); - } + image = new Image(this.Configuration, (int)this.webImageInfo.Width, (int)this.webImageInfo.Height, this.Metadata); + Buffer2D pixels = image.GetRootFramePixelBuffer(); + if (this.webImageInfo.IsLossless) + { + var losslessDecoder = new WebpLosslessDecoder(this.webImageInfo.Vp8LBitReader, this.memoryAllocator, this.Configuration); + losslessDecoder.Decode(pixels, image.Width, image.Height); + } + else + { + var lossyDecoder = new WebpLossyDecoder(this.webImageInfo.Vp8BitReader, this.memoryAllocator, this.Configuration); + lossyDecoder.Decode(pixels, image.Width, image.Height, this.webImageInfo); + } - return image; + // There can be optional chunks after the image data, like EXIF and XMP. + if (this.webImageInfo.Features != null) + { + this.ParseOptionalChunks(this.webImageInfo.Features); + } + + return image; + } + } + catch + { + image?.Dispose(); + throw; } } diff --git a/tests/ImageSharp.Tests/Formats/Bmp/BmpDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Bmp/BmpDecoderTests.cs index 71d0ab34a4..43ec45a34f 100644 --- a/tests/ImageSharp.Tests/Formats/Bmp/BmpDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Bmp/BmpDecoderTests.cs @@ -20,6 +20,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Bmp { [Trait("Format", "Bmp")] + [ValidateDisposedMemoryAllocations] public class BmpDecoderTests { public const PixelTypes CommonNonDefaultPixelTypes = PixelTypes.Rgba32 | PixelTypes.Bgra32 | PixelTypes.RgbaVector; diff --git a/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs index 1922b161f2..7a5241c5a8 100644 --- a/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs @@ -18,6 +18,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Gif { [Trait("Format", "Gif")] + [ValidateDisposedMemoryAllocations] public class GifDecoderTests { private const PixelTypes TestPixelTypes = PixelTypes.Rgba32 | PixelTypes.RgbaVector | PixelTypes.Argb32; diff --git a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Metadata.cs b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Metadata.cs index 9864d62b8f..d9915f17d6 100644 --- a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Metadata.cs +++ b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Metadata.cs @@ -179,11 +179,16 @@ private static void TestImageInfo(string imagePath, IImageDecoder decoder, bool var testFile = TestFile.Create(imagePath); using (var stream = new MemoryStream(testFile.Bytes, false)) { - IImageInfo imageInfo = useIdentify - ? ((IImageInfoDetector)decoder).Identify(Configuration.Default, stream, default) - : decoder.Decode(Configuration.Default, stream, default); - - test(imageInfo); + if (useIdentify) + { + IImageInfo imageInfo = ((IImageInfoDetector)decoder).Identify(Configuration.Default, stream, default); + test(imageInfo); + } + else + { + using var img = decoder.Decode(Configuration.Default, stream, default); + test(img); + } } } diff --git a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs index faf40ccf7d..1faa6f0f4c 100644 --- a/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs @@ -22,6 +22,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Jpg { // TODO: Scatter test cases into multiple test classes [Trait("Format", "Jpg")] + [ValidateDisposedMemoryAllocations] public partial class JpegDecoderTests { public const PixelTypes CommonNonDefaultPixelTypes = PixelTypes.Rgba32 | PixelTypes.Argb32 | PixelTypes.Bgr24 | PixelTypes.RgbaVector; diff --git a/tests/ImageSharp.Tests/Formats/Pbm/PbmDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Pbm/PbmDecoderTests.cs index 97237bca59..eb3bc8c9a5 100644 --- a/tests/ImageSharp.Tests/Formats/Pbm/PbmDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Pbm/PbmDecoderTests.cs @@ -11,6 +11,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Pbm { [Trait("Format", "Pbm")] + [ValidateDisposedMemoryAllocations] public class PbmDecoderTests { [Theory] diff --git a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs index eadbf138a6..a4fcf63baf 100644 --- a/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Png/PngDecoderTests.cs @@ -19,6 +19,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Png { [Trait("Format", "Png")] + [ValidateDisposedMemoryAllocations] public partial class PngDecoderTests { private const PixelTypes TestPixelTypes = PixelTypes.Rgba32 | PixelTypes.RgbaVector | PixelTypes.Argb32; @@ -103,7 +104,6 @@ public async Task DecodeAsync_NonGeneric_CreatesCorrectImageType(string path, Ty [Theory] [WithFileCollection(nameof(CommonTestImages), PixelTypes.Rgba32)] - [ValidateDisposedMemoryAllocations] public void Decode(TestImageProvider provider) where TPixel : unmanaged, IPixel { diff --git a/tests/ImageSharp.Tests/Formats/Tga/TgaDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Tga/TgaDecoderTests.cs index 55dc2ecdd8..e83c5a98cc 100644 --- a/tests/ImageSharp.Tests/Formats/Tga/TgaDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Tga/TgaDecoderTests.cs @@ -16,6 +16,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Tga { [Trait("Format", "Tga")] + [ValidateDisposedMemoryAllocations] public class TgaDecoderTests { private static TgaDecoder TgaDecoder => new(); diff --git a/tests/ImageSharp.Tests/Formats/Tiff/TiffDecoderTests.cs b/tests/ImageSharp.Tests/Formats/Tiff/TiffDecoderTests.cs index 39fa50c93c..9460f3a351 100644 --- a/tests/ImageSharp.Tests/Formats/Tiff/TiffDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/Tiff/TiffDecoderTests.cs @@ -14,6 +14,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Tiff { [Trait("Format", "Tiff")] + [ValidateDisposedMemoryAllocations] public class TiffDecoderTests : TiffDecoderBaseTester { public static readonly string[] MultiframeTestImages = Multiframes; diff --git a/tests/ImageSharp.Tests/Formats/WebP/WebpDecoderTests.cs b/tests/ImageSharp.Tests/Formats/WebP/WebpDecoderTests.cs index 1c92fdf335..f29fa5d793 100644 --- a/tests/ImageSharp.Tests/Formats/WebP/WebpDecoderTests.cs +++ b/tests/ImageSharp.Tests/Formats/WebP/WebpDecoderTests.cs @@ -13,6 +13,7 @@ namespace SixLabors.ImageSharp.Tests.Formats.Webp { [Trait("Format", "Webp")] + [ValidateDisposedMemoryAllocations] public class WebpDecoderTests { private static WebpDecoder WebpDecoder => new(); diff --git a/tests/ImageSharp.Tests/TestUtilities/ImageComparison/ImageComparingUtils.cs b/tests/ImageSharp.Tests/TestUtilities/ImageComparison/ImageComparingUtils.cs index 1801d6b590..fa25846748 100644 --- a/tests/ImageSharp.Tests/TestUtilities/ImageComparison/ImageComparingUtils.cs +++ b/tests/ImageSharp.Tests/TestUtilities/ImageComparison/ImageComparingUtils.cs @@ -26,7 +26,7 @@ public static void CompareWithReferenceDecoder( } var testFile = TestFile.Create(path); - Image magickImage = DecodeWithMagick(new FileInfo(testFile.FullPath)); + using Image magickImage = DecodeWithMagick(new FileInfo(testFile.FullPath)); if (useExactComparer) { ImageComparer.Exact.VerifySimilarity(magickImage, image); From 1557baea5932d87f050febe7a443c81a83db1376 Mon Sep 17 00:00:00 2001 From: Scott Williams Date: Sat, 9 Apr 2022 21:30:59 +0100 Subject: [PATCH 04/14] dispose the next chunk --- src/ImageSharp/Formats/Png/PngDecoderCore.cs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/ImageSharp/Formats/Png/PngDecoderCore.cs b/src/ImageSharp/Formats/Png/PngDecoderCore.cs index 497dc39674..12770bc521 100644 --- a/src/ImageSharp/Formats/Png/PngDecoderCore.cs +++ b/src/ImageSharp/Formats/Png/PngDecoderCore.cs @@ -227,10 +227,16 @@ public Image Decode(BufferedReadStream stream, CancellationToken return image; } + catch + { + image?.Dispose(); + throw; + } finally { this.scanline?.Dispose(); this.previousScanline?.Dispose(); + this.nextChunk?.Data?.Dispose(); } } @@ -472,6 +478,8 @@ private void InitializeImage(ImageMetadata metadata, out Image i this.bytesPerSample = this.header.BitDepth / 8; } + this.previousScanline?.Dispose(); + this.scanline?.Dispose(); this.previousScanline = this.memoryAllocator.Allocate(this.bytesPerScanline, AllocationOptions.Clean); this.scanline = this.Configuration.MemoryAllocator.Allocate(this.bytesPerScanline, AllocationOptions.Clean); } @@ -1359,6 +1367,7 @@ private int ReadNextDataChunk() { if (chunk.Type == PngChunkType.Data) { + chunk.Data?.Dispose(); return chunk.Length; } @@ -1453,6 +1462,9 @@ private void ValidateChunk(in PngChunk chunk) if (validCrc != inputCrc) { string chunkTypeName = Encoding.ASCII.GetString(chunkType); + + // ensure when throwing we dispose the data back to the memory allocator + chunk.Data?.Dispose(); PngThrowHelper.ThrowInvalidChunkCrc(chunkTypeName); } } From 2a1ae5cc4ccef9da590d44387ead974c78004845 Mon Sep 17 00:00:00 2001 From: Scott Williams Date: Sat, 9 Apr 2022 22:21:19 +0100 Subject: [PATCH 05/14] ensure we dispose beffers when decoders throw --- src/Directory.Build.props | 1 + .../Decoder/SpectralConverter{TPixel}.cs | 5 +- .../Formats/Jpeg/JpegDecoderCore.cs | 35 ++++++----- .../Formats/Tiff/TiffDecoderCore.cs | 58 +++++++++++-------- 4 files changed, 61 insertions(+), 38 deletions(-) diff --git a/src/Directory.Build.props b/src/Directory.Build.props index d211992a94..faa29865f2 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -27,6 +27,7 @@ + diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/SpectralConverter{TPixel}.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/SpectralConverter{TPixel}.cs index 430adeb21d..532892e060 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/SpectralConverter{TPixel}.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/SpectralConverter{TPixel}.cs @@ -95,7 +95,9 @@ public Buffer2D GetPixelBuffer(CancellationToken cancellationToken) } } - return this.pixelBuffer; + var buffer = this.pixelBuffer; + this.pixelBuffer = null; + return buffer; } /// @@ -210,6 +212,7 @@ public void Dispose() this.rgbBuffer?.Dispose(); this.paddedProxyPixelRow?.Dispose(); + this.pixelBuffer?.Dispose(); } } } diff --git a/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs b/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs index ef4e3ffac2..a4b42b994d 100644 --- a/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs +++ b/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs @@ -187,20 +187,27 @@ public Image Decode(BufferedReadStream stream, CancellationToken where TPixel : unmanaged, IPixel { using var spectralConverter = new SpectralConverter(this.Configuration); - - var scanDecoder = new HuffmanScanDecoder(stream, spectralConverter, cancellationToken); - - this.ParseStream(stream, scanDecoder, cancellationToken); - this.InitExifProfile(); - this.InitIccProfile(); - this.InitIptcProfile(); - this.InitXmpProfile(); - this.InitDerivedMetadataProperties(); - - return new Image( - this.Configuration, - spectralConverter.GetPixelBuffer(cancellationToken), - this.Metadata); + try + { + var scanDecoder = new HuffmanScanDecoder(stream, spectralConverter, cancellationToken); + + this.ParseStream(stream, scanDecoder, cancellationToken); + this.InitExifProfile(); + this.InitIccProfile(); + this.InitIptcProfile(); + this.InitXmpProfile(); + this.InitDerivedMetadataProperties(); + + return new Image( + this.Configuration, + spectralConverter.GetPixelBuffer(cancellationToken), + this.Metadata); + } + catch + { + this.Frame?.Dispose(); + throw; + } } /// diff --git a/src/ImageSharp/Formats/Tiff/TiffDecoderCore.cs b/src/ImageSharp/Formats/Tiff/TiffDecoderCore.cs index 5dd99fafc1..1cd3d2c0c1 100644 --- a/src/ImageSharp/Formats/Tiff/TiffDecoderCore.cs +++ b/src/ImageSharp/Formats/Tiff/TiffDecoderCore.cs @@ -157,40 +157,52 @@ public TiffDecoderCore(Configuration configuration, ITiffDecoderOptions options) public Image Decode(BufferedReadStream stream, CancellationToken cancellationToken) where TPixel : unmanaged, IPixel { - this.inputStream = stream; - var reader = new DirectoryReader(stream, this.Configuration.MemoryAllocator); - - IEnumerable directories = reader.Read(); - this.byteOrder = reader.ByteOrder; - this.isBigTiff = reader.IsBigTiff; - var frames = new List>(); - foreach (ExifProfile ifd in directories) + try { - cancellationToken.ThrowIfCancellationRequested(); - ImageFrame frame = this.DecodeFrame(ifd, cancellationToken); - frames.Add(frame); + this.inputStream = stream; + var reader = new DirectoryReader(stream, this.Configuration.MemoryAllocator); + + IEnumerable directories = reader.Read(); + this.byteOrder = reader.ByteOrder; + this.isBigTiff = reader.IsBigTiff; - if (this.decodingMode is FrameDecodingMode.First) + foreach (ExifProfile ifd in directories) { - break; + cancellationToken.ThrowIfCancellationRequested(); + ImageFrame frame = this.DecodeFrame(ifd, cancellationToken); + frames.Add(frame); + + if (this.decodingMode is FrameDecodingMode.First) + { + break; + } } - } - ImageMetadata metadata = TiffDecoderMetadataCreator.Create(frames, this.ignoreMetadata, reader.ByteOrder, reader.IsBigTiff); + ImageMetadata metadata = TiffDecoderMetadataCreator.Create(frames, this.ignoreMetadata, reader.ByteOrder, reader.IsBigTiff); - // TODO: Tiff frames can have different sizes. - ImageFrame root = frames[0]; - this.Dimensions = root.Size(); - foreach (ImageFrame frame in frames) - { - if (frame.Size() != root.Size()) + // TODO: Tiff frames can have different sizes. + ImageFrame root = frames[0]; + this.Dimensions = root.Size(); + foreach (ImageFrame frame in frames) { - TiffThrowHelper.ThrowNotSupported("Images with different sizes are not supported"); + if (frame.Size() != root.Size()) + { + TiffThrowHelper.ThrowNotSupported("Images with different sizes are not supported"); + } } + + return new Image(this.Configuration, metadata, frames); } + catch + { + foreach (ImageFrame f in frames) + { + f.Dispose(); + } - return new Image(this.Configuration, metadata, frames); + throw; + } } /// From a30c468f2757906c581e95adb7e6750fffdad0fe Mon Sep 17 00:00:00 2001 From: Scott Williams Date: Sat, 9 Apr 2022 22:35:21 +0100 Subject: [PATCH 06/14] drop commented code --- src/ImageSharp/Formats/Gif/GifDecoderCore.cs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/ImageSharp/Formats/Gif/GifDecoderCore.cs b/src/ImageSharp/Formats/Gif/GifDecoderCore.cs index ab857a404a..16dca3324f 100644 --- a/src/ImageSharp/Formats/Gif/GifDecoderCore.cs +++ b/src/ImageSharp/Formats/Gif/GifDecoderCore.cs @@ -151,11 +151,6 @@ public Image Decode(BufferedReadStream stream, CancellationToken } } } - //catch - //{ - // image?.Dispose(); - // throw; - //} finally { this.globalColorTable?.Dispose(); From 399d71ddf16c77ec660294c5a1142d5ecc736afd Mon Sep 17 00:00:00 2001 From: Scott Williams Date: Sat, 9 Apr 2022 22:39:42 +0100 Subject: [PATCH 07/14] stylecopped --- src/ImageSharp/Formats/Webp/WebpDecoderCore.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/ImageSharp/Formats/Webp/WebpDecoderCore.cs b/src/ImageSharp/Formats/Webp/WebpDecoderCore.cs index 5646f71fac..7052be4ea6 100644 --- a/src/ImageSharp/Formats/Webp/WebpDecoderCore.cs +++ b/src/ImageSharp/Formats/Webp/WebpDecoderCore.cs @@ -85,7 +85,6 @@ public Image Decode(BufferedReadStream stream, CancellationToken Image image = null; try { - this.Metadata = new ImageMetadata(); this.currentStream = stream; From d9af2395ee7fd6b3cc5b7032a13d0f68689eacc2 Mon Sep 17 00:00:00 2001 From: Scott Williams Date: Sat, 9 Apr 2022 22:48:48 +0100 Subject: [PATCH 08/14] indent for full framwork --- src/ImageSharp/Diagnostics/MemoryDiagnostics.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs index 6af2544138..f2a433ef1b 100644 --- a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs +++ b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs @@ -131,9 +131,9 @@ internal void RaiseUndisposedMemoryResource(string allocationStackTrace) allocationStackTrace, preferLocal: false); #else - ThreadPool.QueueUserWorkItem( - stackTrace => this.undisposedAllocation?.Invoke((string)stackTrace), - allocationStackTrace); + ThreadPool.QueueUserWorkItem( + stackTrace => this.undisposedAllocation?.Invoke((string)stackTrace), + allocationStackTrace); #endif } } From 99d12664a2cc558510cf97ecebe67f869dfb60b1 Mon Sep 17 00:00:00 2001 From: Scott Williams Date: Sun, 10 Apr 2022 09:32:50 +0100 Subject: [PATCH 09/14] Async local doesn't actually work for the finalizers, revert that piece. --- .../Diagnostics/MemoryDiagnostics.cs | 96 ++++++++----------- 1 file changed, 40 insertions(+), 56 deletions(-) diff --git a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs index f2a433ef1b..057242bae8 100644 --- a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs +++ b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs @@ -17,6 +17,10 @@ public static class MemoryDiagnostics { internal static readonly InteralMemoryDiagnostics Default = new(); private static AsyncLocal localInstance = null; + + // the async local end up out of scope during finalizers so putting into thte internal class is useless + private static UndisposedAllocationDelegate undisposedAllocation; + private static int undisposedAllocationSubscriptionCounter; private static readonly object SyncRoot = new(); /// @@ -26,8 +30,23 @@ public static class MemoryDiagnostics /// public static event UndisposedAllocationDelegate UndisposedAllocation { - add => Current.UndisposedAllocation += value; - remove => Current.UndisposedAllocation -= value; + add + { + lock (SyncRoot) + { + undisposedAllocationSubscriptionCounter++; + undisposedAllocation += value; + } + } + + remove + { + lock (SyncRoot) + { + undisposedAllocation -= value; + undisposedAllocationSubscriptionCounter--; + } + } } internal static InteralMemoryDiagnostics Current @@ -61,81 +80,46 @@ internal static InteralMemoryDiagnostics Current /// public static int TotalUndisposedAllocationCount => Current.TotalUndisposedAllocationCount; - internal static bool UndisposedAllocationSubscribed => Current.UndisposedAllocationSubscribed; + internal static bool UndisposedAllocationSubscribed => Volatile.Read(ref undisposedAllocationSubscriptionCounter) > 0; internal static void IncrementTotalUndisposedAllocationCount() => Current.IncrementTotalUndisposedAllocationCount(); internal static void DecrementTotalUndisposedAllocationCount() => Current.DecrementTotalUndisposedAllocationCount(); internal static void RaiseUndisposedMemoryResource(string allocationStackTrace) - => Current.RaiseUndisposedMemoryResource(allocationStackTrace); + { + if (undisposedAllocation is null) + { + return; + } + + // Schedule on the ThreadPool, to avoid user callback messing up the finalizer thread. +#if NETSTANDARD2_1 || NETCOREAPP2_1_OR_GREATER + ThreadPool.QueueUserWorkItem( + stackTrace => undisposedAllocation?.Invoke(stackTrace), + allocationStackTrace, + preferLocal: false); +#else + ThreadPool.QueueUserWorkItem( + stackTrace => undisposedAllocation?.Invoke((string)stackTrace), + allocationStackTrace); +#endif + } internal class InteralMemoryDiagnostics { private int totalUndisposedAllocationCount; - private UndisposedAllocationDelegate undisposedAllocation; - private int undisposedAllocationSubscriptionCounter; - private readonly object syncRoot = new(); - - /// - /// Fires when an ImageSharp object's undisposed memory resource leaks to the finalizer. - /// The event brings significant overhead, and is intended to be used for troubleshooting only. - /// For production diagnostics, use . - /// - public event UndisposedAllocationDelegate UndisposedAllocation - { - add - { - lock (this.syncRoot) - { - this.undisposedAllocationSubscriptionCounter++; - this.undisposedAllocation += value; - } - } - - remove - { - lock (this.syncRoot) - { - this.undisposedAllocation -= value; - this.undisposedAllocationSubscriptionCounter--; - } - } - } - /// /// Gets a value indicating the total number of memory resource objects leaked to the finalizer. /// public int TotalUndisposedAllocationCount => this.totalUndisposedAllocationCount; - internal bool UndisposedAllocationSubscribed => Volatile.Read(ref this.undisposedAllocationSubscriptionCounter) > 0; - internal void IncrementTotalUndisposedAllocationCount() => Interlocked.Increment(ref this.totalUndisposedAllocationCount); internal void DecrementTotalUndisposedAllocationCount() => Interlocked.Decrement(ref this.totalUndisposedAllocationCount); - - internal void RaiseUndisposedMemoryResource(string allocationStackTrace) - { - if (this.undisposedAllocation is null) - { - return; - } - - // Schedule on the ThreadPool, to avoid user callback messing up the finalizer thread. -#if NETSTANDARD2_1 || NETCOREAPP2_1_OR_GREATER - ThreadPool.QueueUserWorkItem( - stackTrace => this.undisposedAllocation?.Invoke(stackTrace), - allocationStackTrace, - preferLocal: false); -#else - ThreadPool.QueueUserWorkItem( - stackTrace => this.undisposedAllocation?.Invoke((string)stackTrace), - allocationStackTrace); -#endif - } } } } From 558ff99b08d9a316b8a3f6502b0dc465677a9b16 Mon Sep 17 00:00:00 2001 From: Scott Williams Date: Sun, 10 Apr 2022 09:42:48 +0100 Subject: [PATCH 10/14] revert Frame Dispose --- .../Formats/Jpeg/JpegDecoderCore.cs | 35 ++++++++----------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs b/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs index a4b42b994d..ef4e3ffac2 100644 --- a/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs +++ b/src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs @@ -187,27 +187,20 @@ public Image Decode(BufferedReadStream stream, CancellationToken where TPixel : unmanaged, IPixel { using var spectralConverter = new SpectralConverter(this.Configuration); - try - { - var scanDecoder = new HuffmanScanDecoder(stream, spectralConverter, cancellationToken); - - this.ParseStream(stream, scanDecoder, cancellationToken); - this.InitExifProfile(); - this.InitIccProfile(); - this.InitIptcProfile(); - this.InitXmpProfile(); - this.InitDerivedMetadataProperties(); - - return new Image( - this.Configuration, - spectralConverter.GetPixelBuffer(cancellationToken), - this.Metadata); - } - catch - { - this.Frame?.Dispose(); - throw; - } + + var scanDecoder = new HuffmanScanDecoder(stream, spectralConverter, cancellationToken); + + this.ParseStream(stream, scanDecoder, cancellationToken); + this.InitExifProfile(); + this.InitIccProfile(); + this.InitIptcProfile(); + this.InitXmpProfile(); + this.InitDerivedMetadataProperties(); + + return new Image( + this.Configuration, + spectralConverter.GetPixelBuffer(cancellationToken), + this.Metadata); } /// From cd7c91ccdf5a55e24fc62e63a7f4fca803341210 Mon Sep 17 00:00:00 2001 From: Scott Williams Date: Mon, 11 Apr 2022 16:20:30 +0100 Subject: [PATCH 11/14] move AsyncLocal out of core lib and into testing --- .../Diagnostics/MemoryDiagnostics.cs | 67 ++++++------------- .../MemoryAllocatorValidator.cs | 64 +++++++++++++----- .../ImageProviders/FileProvider.cs | 4 +- ...idateDisposedMemoryAllocationsAttribute.cs | 19 +++--- 4 files changed, 80 insertions(+), 74 deletions(-) diff --git a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs index 057242bae8..237f85b0f9 100644 --- a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs +++ b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs @@ -1,6 +1,7 @@ // Copyright (c) Six Labors. // Licensed under the Apache License, Version 2.0. +using System; using System.Threading; namespace SixLabors.ImageSharp.Diagnostics @@ -15,10 +16,8 @@ namespace SixLabors.ImageSharp.Diagnostics /// public static class MemoryDiagnostics { - internal static readonly InteralMemoryDiagnostics Default = new(); - private static AsyncLocal localInstance = null; + private static int totalUndisposedAllocationCount; - // the async local end up out of scope during finalizers so putting into thte internal class is useless private static UndisposedAllocationDelegate undisposedAllocation; private static int undisposedAllocationSubscriptionCounter; private static readonly object SyncRoot = new(); @@ -49,42 +48,34 @@ public static event UndisposedAllocationDelegate UndisposedAllocation } } - internal static InteralMemoryDiagnostics Current - { - get - { - if (localInstance != null && localInstance.Value != null) - { - return localInstance.Value; - } - - return Default; - } - - set - { - if (localInstance == null) - { - lock (SyncRoot) - { - localInstance ??= new AsyncLocal(); - } - } + /// + /// Fires when ImageSharp allocates memory from a MemoryAllocator + /// + internal static event Action MemoryAllocated; - localInstance.Value = value; - } - } + /// + /// Fires when ImageSharp releases allocated from a MemoryAllocator + /// + internal static event Action MemoryReleased; /// /// Gets a value indicating the total number of memory resource objects leaked to the finalizer. /// - public static int TotalUndisposedAllocationCount => Current.TotalUndisposedAllocationCount; + public static int TotalUndisposedAllocationCount => totalUndisposedAllocationCount; internal static bool UndisposedAllocationSubscribed => Volatile.Read(ref undisposedAllocationSubscriptionCounter) > 0; - internal static void IncrementTotalUndisposedAllocationCount() => Current.IncrementTotalUndisposedAllocationCount(); + internal static void IncrementTotalUndisposedAllocationCount() + { + Interlocked.Increment(ref totalUndisposedAllocationCount); + MemoryAllocated?.Invoke(); + } - internal static void DecrementTotalUndisposedAllocationCount() => Current.DecrementTotalUndisposedAllocationCount(); + internal static void DecrementTotalUndisposedAllocationCount() + { + Interlocked.Decrement(ref totalUndisposedAllocationCount); + MemoryReleased?.Invoke(); + } internal static void RaiseUndisposedMemoryResource(string allocationStackTrace) { @@ -105,21 +96,5 @@ internal static void RaiseUndisposedMemoryResource(string allocationStackTrace) allocationStackTrace); #endif } - - internal class InteralMemoryDiagnostics - { - private int totalUndisposedAllocationCount; - - /// - /// Gets a value indicating the total number of memory resource objects leaked to the finalizer. - /// - public int TotalUndisposedAllocationCount => this.totalUndisposedAllocationCount; - - internal void IncrementTotalUndisposedAllocationCount() => - Interlocked.Increment(ref this.totalUndisposedAllocationCount); - - internal void DecrementTotalUndisposedAllocationCount() => - Interlocked.Decrement(ref this.totalUndisposedAllocationCount); - } } } diff --git a/tests/ImageSharp.Tests/MemoryAllocatorValidator.cs b/tests/ImageSharp.Tests/MemoryAllocatorValidator.cs index 2c3d98eb13..13664ee9b2 100644 --- a/tests/ImageSharp.Tests/MemoryAllocatorValidator.cs +++ b/tests/ImageSharp.Tests/MemoryAllocatorValidator.cs @@ -2,7 +2,7 @@ // Licensed under the Apache License, Version 2.0. using System; -using System.Diagnostics; +using System.Threading; using SixLabors.ImageSharp.Diagnostics; using Xunit; @@ -10,34 +10,68 @@ namespace SixLabors.ImageSharp.Tests { public static class MemoryAllocatorValidator { - public static IDisposable MonitorAllocations(int max = 0) + private static readonly AsyncLocal LocalInstance = new(); + + public static bool MonitoringAllocations => LocalInstance.Value != null; + + static MemoryAllocatorValidator() { - MemoryDiagnostics.Current = new(); - return new TestMemoryAllocatorDisposable(max); + MemoryDiagnostics.MemoryAllocated += MemoryDiagnostics_MemoryAllocated; + MemoryDiagnostics.MemoryReleased += MemoryDiagnostics_MemoryReleased; } - public static void ValidateAllocation(int max = 0) + private static void MemoryDiagnostics_MemoryReleased() { - var count = MemoryDiagnostics.TotalUndisposedAllocationCount; - var pass = count <= max; - Assert.True(pass, $"Expected a max of {max} undisposed buffers but found {count}"); + TestMemoryDiagnostics backing = LocalInstance.Value; + if (backing != null) + { + backing.TotalRemainingAllocated--; + } + } - if (count > 0) + private static void MemoryDiagnostics_MemoryAllocated() + { + TestMemoryDiagnostics backing = LocalInstance.Value; + if (backing != null) { - Debug.WriteLine("We should have Zero undisposed memory allocations."); + backing.TotalAllocated++; + backing.TotalRemainingAllocated++; } + } - MemoryDiagnostics.Current = null; + public static TestMemoryDiagnostics MonitorAllocations() + { + var diag = new TestMemoryDiagnostics(); + LocalInstance.Value = diag; + return diag; } - public struct TestMemoryAllocatorDisposable : IDisposable + public static void StopMonitoringAllocations() => LocalInstance.Value = null; + + public static void ValidateAllocations(int expectedAllocationCount = 0) + => LocalInstance.Value?.Validate(expectedAllocationCount); + + public class TestMemoryDiagnostics : IDisposable { - private readonly int max; + public int TotalAllocated { get; set; } - public TestMemoryAllocatorDisposable(int max) => this.max = max; + public int TotalRemainingAllocated { get; set; } + + public void Validate(int expectedAllocationCount) + { + var count = this.TotalRemainingAllocated; + var pass = expectedAllocationCount == count; + Assert.True(pass, $"Expected a {expectedAllocationCount} undisposed buffers but found {count}"); + } public void Dispose() - => ValidateAllocation(this.max); + { + this.Validate(0); + if (LocalInstance.Value == this) + { + StopMonitoringAllocations(); + } + } } } } diff --git a/tests/ImageSharp.Tests/TestUtilities/ImageProviders/FileProvider.cs b/tests/ImageSharp.Tests/TestUtilities/ImageProviders/FileProvider.cs index 3675356c38..63c5ce31a2 100644 --- a/tests/ImageSharp.Tests/TestUtilities/ImageProviders/FileProvider.cs +++ b/tests/ImageSharp.Tests/TestUtilities/ImageProviders/FileProvider.cs @@ -159,8 +159,8 @@ public override Image GetImage(IImageDecoder decoder) return this.LoadImage(decoder); } - // do cache so we can track allocation correctly when validating memory - if (MemoryDiagnostics.Current != MemoryDiagnostics.Default) + // do not cache so we can track allocation correctly when validating memory + if (MemoryAllocatorValidator.MonitoringAllocations) { return this.LoadImage(decoder); } diff --git a/tests/ImageSharp.Tests/ValidateDisposedMemoryAllocationsAttribute.cs b/tests/ImageSharp.Tests/ValidateDisposedMemoryAllocationsAttribute.cs index 95d13b5954..65ed990dd7 100644 --- a/tests/ImageSharp.Tests/ValidateDisposedMemoryAllocationsAttribute.cs +++ b/tests/ImageSharp.Tests/ValidateDisposedMemoryAllocationsAttribute.cs @@ -11,26 +11,23 @@ namespace SixLabors.ImageSharp.Tests [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)] public class ValidateDisposedMemoryAllocationsAttribute : BeforeAfterTestAttribute { - private readonly int max = 0; + private readonly int expected = 0; public ValidateDisposedMemoryAllocationsAttribute() : this(0) { } - public ValidateDisposedMemoryAllocationsAttribute(int max) - { - this.max = max; - if (max > 0) - { - Debug.WriteLine("Needs fixing, we shoudl have Zero undisposed memory allocations."); - } - } + public ValidateDisposedMemoryAllocationsAttribute(int expected) + => this.expected = expected; public override void Before(MethodInfo methodUnderTest) - => MemoryAllocatorValidator.MonitorAllocations(this.max); // the disposable isn't important cause the validate below does the same thing + => MemoryAllocatorValidator.MonitorAllocations(); public override void After(MethodInfo methodUnderTest) - => MemoryAllocatorValidator.ValidateAllocation(this.max); + { + MemoryAllocatorValidator.ValidateAllocations(this.expected); + MemoryAllocatorValidator.StopMonitoringAllocations(); + } } } From f5975075006934c25afea64868b99ab9d598e9bc Mon Sep 17 00:00:00 2001 From: Scott Williams Date: Mon, 11 Apr 2022 16:23:59 +0100 Subject: [PATCH 12/14] fix comment --- src/ImageSharp/Diagnostics/MemoryDiagnostics.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs index 237f85b0f9..89f18cff61 100644 --- a/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs +++ b/src/ImageSharp/Diagnostics/MemoryDiagnostics.cs @@ -54,7 +54,7 @@ public static event UndisposedAllocationDelegate UndisposedAllocation internal static event Action MemoryAllocated; /// - /// Fires when ImageSharp releases allocated from a MemoryAllocator + /// Fires when ImageSharp releases memory allocated from a MemoryAllocator /// internal static event Action MemoryReleased; From f4f521cc5beccaa3b1ecf4f41dfacc6a0cb298a8 Mon Sep 17 00:00:00 2001 From: Scott Williams Date: Tue, 12 Apr 2022 17:56:08 +0100 Subject: [PATCH 13/14] revert to allow easier merge --- src/ImageSharp/Formats/Png/PngDecoderCore.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/ImageSharp/Formats/Png/PngDecoderCore.cs b/src/ImageSharp/Formats/Png/PngDecoderCore.cs index 12770bc521..05ce8d92e4 100644 --- a/src/ImageSharp/Formats/Png/PngDecoderCore.cs +++ b/src/ImageSharp/Formats/Png/PngDecoderCore.cs @@ -1367,7 +1367,6 @@ private int ReadNextDataChunk() { if (chunk.Type == PngChunkType.Data) { - chunk.Data?.Dispose(); return chunk.Length; } From 03d0b63c7c5ddf487f351238395f771976624544 Mon Sep 17 00:00:00 2001 From: miere43 Date: Sat, 9 Apr 2022 17:57:06 +0300 Subject: [PATCH 14/14] Fix chunk data memory leak when decoding PNG. Fix #2080 --- src/ImageSharp/Formats/Png/PngDecoderCore.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/ImageSharp/Formats/Png/PngDecoderCore.cs b/src/ImageSharp/Formats/Png/PngDecoderCore.cs index 05ce8d92e4..12770bc521 100644 --- a/src/ImageSharp/Formats/Png/PngDecoderCore.cs +++ b/src/ImageSharp/Formats/Png/PngDecoderCore.cs @@ -1367,6 +1367,7 @@ private int ReadNextDataChunk() { if (chunk.Type == PngChunkType.Data) { + chunk.Data?.Dispose(); return chunk.Length; }