From 7900b43d1d75c53441e989d44809f5d5f2bca00d Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Thu, 13 May 2021 22:19:36 +0300 Subject: [PATCH 01/21] Image.Frames now properly throws ObjectDisposedException after being disposed --- src/ImageSharp/Image{TPixel}.cs | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/ImageSharp/Image{TPixel}.cs b/src/ImageSharp/Image{TPixel}.cs index 83ecc37530..0a0b0d0d60 100644 --- a/src/ImageSharp/Image{TPixel}.cs +++ b/src/ImageSharp/Image{TPixel}.cs @@ -25,6 +25,8 @@ public sealed class Image : Image { private bool isDisposed; + private ImageFrameCollection frames; + /// /// Initializes a new instance of the class /// with the height and the width of the image. @@ -84,7 +86,7 @@ public Image(int width, int height) internal Image(Configuration configuration, int width, int height, ImageMetadata metadata) : base(configuration, PixelTypeInfo.Create(), metadata, width, height) { - this.Frames = new ImageFrameCollection(this, width, height, default(TPixel)); + this.frames = new ImageFrameCollection(this, width, height, default(TPixel)); } /// @@ -104,7 +106,7 @@ internal Image( ImageMetadata metadata) : base(configuration, PixelTypeInfo.Create(), metadata, width, height) { - this.Frames = new ImageFrameCollection(this, width, height, memoryGroup); + this.frames = new ImageFrameCollection(this, width, height, memoryGroup); } /// @@ -124,7 +126,7 @@ internal Image( ImageMetadata metadata) : base(configuration, PixelTypeInfo.Create(), metadata, width, height) { - this.Frames = new ImageFrameCollection(this, width, height, backgroundColor); + this.frames = new ImageFrameCollection(this, width, height, backgroundColor); } /// @@ -137,7 +139,7 @@ internal Image( internal Image(Configuration configuration, ImageMetadata metadata, IEnumerable> frames) : base(configuration, PixelTypeInfo.Create(), metadata, ValidateFramesAndGetSize(frames)) { - this.Frames = new ImageFrameCollection(this, frames); + this.frames = new ImageFrameCollection(this, frames); } /// @@ -146,7 +148,14 @@ internal Image(Configuration configuration, ImageMetadata metadata, IEnumerable< /// /// Gets the collection of image frames. /// - public new ImageFrameCollection Frames { get; } + public new ImageFrameCollection Frames + { + get + { + this.EnsureNotDisposed(); + return this.frames; + } + } /// /// Gets the root frame. From d48b15227da821e9b49c78c6dc88586e892d7145 Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Thu, 13 May 2021 22:25:05 +0300 Subject: [PATCH 02/21] Image private methods no longer check if object was disposed - it is done at public method calls --- src/ImageSharp/Image{TPixel}.cs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/ImageSharp/Image{TPixel}.cs b/src/ImageSharp/Image{TPixel}.cs index 0a0b0d0d60..4db3badb01 100644 --- a/src/ImageSharp/Image{TPixel}.cs +++ b/src/ImageSharp/Image{TPixel}.cs @@ -235,10 +235,10 @@ public Image Clone(Configuration configuration) { this.EnsureNotDisposed(); - var clonedFrames = new ImageFrame[this.Frames.Count]; + var clonedFrames = new ImageFrame[this.frames.Count]; for (int i = 0; i < clonedFrames.Length; i++) { - clonedFrames[i] = this.Frames[i].Clone(configuration); + clonedFrames[i] = this.frames[i].Clone(configuration); } return new Image(configuration, this.Metadata.DeepClone(), clonedFrames); @@ -254,10 +254,10 @@ public override Image CloneAs(Configuration configuration) { this.EnsureNotDisposed(); - var clonedFrames = new ImageFrame[this.Frames.Count]; + var clonedFrames = new ImageFrame[this.frames.Count]; for (int i = 0; i < clonedFrames.Length; i++) { - clonedFrames[i] = this.Frames[i].CloneAs(configuration); + clonedFrames[i] = this.frames[i].CloneAs(configuration); } return new Image(configuration, this.Metadata.DeepClone(), clonedFrames); @@ -273,7 +273,7 @@ protected override void Dispose(bool disposing) if (disposing) { - this.Frames.Dispose(); + this.frames.Dispose(); } this.isDisposed = true; @@ -315,9 +315,12 @@ internal void SwapOrCopyPixelsBuffersFrom(Image pixelSource) { Guard.NotNull(pixelSource, nameof(pixelSource)); - for (int i = 0; i < this.Frames.Count; i++) + this.EnsureNotDisposed(); + + ImageFrameCollection sourceFrames = pixelSource.Frames; + for (int i = 0; i < this.frames.Count; i++) { - this.Frames[i].SwapOrCopyPixelsBufferFrom(pixelSource.Frames[i]); + this.frames[i].SwapOrCopyPixelsBufferFrom(sourceFrames[i]); } this.UpdateSize(pixelSource.Size()); From 7029b2ffb16fa6257b9ce2716fb02de540949c7a Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Thu, 13 May 2021 22:39:15 +0300 Subject: [PATCH 03/21] Image private property PixelSource no longer checks if object was disposed, check is delegated to public methods using that property --- src/ImageSharp/Image{TPixel}.cs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/ImageSharp/Image{TPixel}.cs b/src/ImageSharp/Image{TPixel}.cs index 4db3badb01..9c32a2c314 100644 --- a/src/ImageSharp/Image{TPixel}.cs +++ b/src/ImageSharp/Image{TPixel}.cs @@ -160,7 +160,7 @@ internal Image(Configuration configuration, ImageMetadata metadata, IEnumerable< /// /// Gets the root frame. /// - private IPixelSource PixelSource => this.Frames?.RootFrame ?? throw new ObjectDisposedException(nameof(Image)); + private IPixelSource PixelSource => this.frames.RootFrame; /// /// Gets or sets the pixel at the specified position. @@ -174,6 +174,8 @@ internal Image(Configuration configuration, ImageMetadata metadata, IEnumerable< [MethodImpl(InliningOptions.ShortMethod)] get { + this.EnsureNotDisposed(); + this.VerifyCoords(x, y); return this.PixelSource.PixelBuffer.GetElementUnsafe(x, y); } @@ -181,6 +183,8 @@ internal Image(Configuration configuration, ImageMetadata metadata, IEnumerable< [MethodImpl(InliningOptions.ShortMethod)] set { + this.EnsureNotDisposed(); + this.VerifyCoords(x, y); this.PixelSource.PixelBuffer.GetElementUnsafe(x, y) = value; } @@ -198,6 +202,8 @@ public Span GetPixelRowSpan(int rowIndex) Guard.MustBeGreaterThanOrEqualTo(rowIndex, 0, nameof(rowIndex)); Guard.MustBeLessThan(rowIndex, this.Height, nameof(rowIndex)); + this.EnsureNotDisposed(); + return this.PixelSource.PixelBuffer.GetRowSpan(rowIndex); } From 1c45c1a7055a90af8bcb288140422eaa7db405ba Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Thu, 13 May 2021 22:51:55 +0300 Subject: [PATCH 04/21] Removed GC.SuppressFinalize(this) from Image.Dispose() due to it not having a Finalization method --- src/ImageSharp/Image.cs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/ImageSharp/Image.cs b/src/ImageSharp/Image.cs index fbb3ec2069..c07c7fe83f 100644 --- a/src/ImageSharp/Image.cs +++ b/src/ImageSharp/Image.cs @@ -78,11 +78,7 @@ internal Image( Configuration IConfigurationProvider.Configuration => this.configuration; /// - public void Dispose() - { - this.Dispose(true); - GC.SuppressFinalize(this); - } + public void Dispose() => this.Dispose(true); /// /// Saves the image to the given stream using the given image encoder. From 095ce416260625bff00b5e4877954bcf446ab7b5 Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Fri, 14 May 2021 00:29:41 +0300 Subject: [PATCH 05/21] Added tests for issue#1628 --- tests/ImageSharp.Tests/Image/ImageTests.cs | 71 ++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/tests/ImageSharp.Tests/Image/ImageTests.cs b/tests/ImageSharp.Tests/Image/ImageTests.cs index b7c6b3835a..1c942ac055 100644 --- a/tests/ImageSharp.Tests/Image/ImageTests.cs +++ b/tests/ImageSharp.Tests/Image/ImageTests.cs @@ -2,7 +2,9 @@ // Licensed under the Apache License, Version 2.0. using System; +using System.IO; using SixLabors.ImageSharp.Advanced; +using SixLabors.ImageSharp.Formats.Jpeg; using SixLabors.ImageSharp.Memory; using SixLabors.ImageSharp.Metadata; using SixLabors.ImageSharp.PixelFormats; @@ -169,5 +171,74 @@ public void Set_OutOfRangeY(bool enforceDisco, int y) Assert.Equal("y", ex.ParamName); } } + + public class Dispose + { + private readonly Configuration configuration = Configuration.CreateDefaultInstance(); + + public void MultipleDisposeCalls() + { + var image = new Image(this.configuration, 10, 10); + image.Dispose(); + image.Dispose(); + } + + [Fact] + public void NonPrivateProperties_ObjectDisposedException() + { + var image = new Image(this.configuration, 10, 10); + var genericImage = (Image)image; + + image.Dispose(); + + // Image + Assert.Throws(() => { var prop = image.Frames; }); + Assert.Throws(() => { var prop = image.Metadata; }); + Assert.Throws(() => { var prop = image.PixelType; }); + + // Image + Assert.Throws(() => { var prop = genericImage.Frames; }); + } + + [Fact] + public void Save_ObjectDisposedException() + { + var image = new Image(this.configuration, 10, 10); + var stream = new MemoryStream(); + var encoder = new JpegEncoder(); + + image.Dispose(); + + // Image + Assert.Throws(() => image.Save(stream, encoder)); + } + + [Fact] + public void AcceptVisitor_ObjectDisposedException() + { + // This test technically should exist but it's impossible to write proper test case without reflection: + // All visitor types are private and can't be created without context of some save/processing operation + // Save_ObjectDisposedException test checks this method with AcceptVisitor(EncodeVisitor) anyway + return; + } + + [Fact] + public void NonPrivateMethods_ObjectDisposedException() + { + var image = new Image(this.configuration, 10, 10); + var genericImage = (Image)image; + + image.Dispose(); + + // Image + Assert.Throws(() => { var res = image.Clone(this.configuration); }); + Assert.Throws(() => { var res = image.CloneAs(this.configuration); }); + Assert.Throws(() => { var res = image.GetPixelRowSpan(default); }); + Assert.Throws(() => { var res = image.TryGetSinglePixelSpan(out var _); }); + + // Image + Assert.Throws(() => { var res = genericImage.CloneAs(this.configuration); }); + } + } } } From acf9d85e8ca8a5ebd4894e85e1d6fe82d2e097b2 Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Fri, 14 May 2021 00:36:40 +0300 Subject: [PATCH 06/21] Moved dispose control logic to base Image class internal call EnsureNotDisposed is no longer virtual -> micro speedup gain in pixel index accessor Image[x, y]. --- src/ImageSharp/Image.cs | 22 ++++++++++++++++++++-- src/ImageSharp/Image{TPixel}.cs | 18 ------------------ 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/ImageSharp/Image.cs b/src/ImageSharp/Image.cs index c07c7fe83f..3aa30063d8 100644 --- a/src/ImageSharp/Image.cs +++ b/src/ImageSharp/Image.cs @@ -19,6 +19,8 @@ namespace SixLabors.ImageSharp /// public abstract partial class Image : IImage, IConfigurationProvider { + private bool isDisposed; + private Size size; private readonly Configuration configuration; @@ -78,7 +80,17 @@ internal Image( Configuration IConfigurationProvider.Configuration => this.configuration; /// - public void Dispose() => this.Dispose(true); + public void Dispose() + { + if (this.isDisposed) + { + return; + } + + this.Dispose(true); + + this.isDisposed = true; + } /// /// Saves the image to the given stream using the given image encoder. @@ -144,7 +156,13 @@ public abstract Image CloneAs(Configuration configuration) /// /// Throws if the image is disposed. /// - internal abstract void EnsureNotDisposed(); + internal void EnsureNotDisposed() + { + if (this.isDisposed) + { + throw new ObjectDisposedException("Trying to execute an operation on a disposed image."); + } + } /// /// Accepts a . diff --git a/src/ImageSharp/Image{TPixel}.cs b/src/ImageSharp/Image{TPixel}.cs index 9c32a2c314..c436430522 100644 --- a/src/ImageSharp/Image{TPixel}.cs +++ b/src/ImageSharp/Image{TPixel}.cs @@ -23,8 +23,6 @@ namespace SixLabors.ImageSharp public sealed class Image : Image where TPixel : unmanaged, IPixel { - private bool isDisposed; - private ImageFrameCollection frames; /// @@ -272,26 +270,10 @@ public override Image CloneAs(Configuration configuration) /// protected override void Dispose(bool disposing) { - if (this.isDisposed) - { - return; - } - if (disposing) { this.frames.Dispose(); } - - this.isDisposed = true; - } - - /// - internal override void EnsureNotDisposed() - { - if (this.isDisposed) - { - throw new ObjectDisposedException("Trying to execute an operation on a disposed image."); - } } /// From 8ec1013ce8ff41e933b21590333431ac45b4b536 Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Fri, 14 May 2021 00:55:27 +0300 Subject: [PATCH 07/21] Removed redundant flag from Image.Dispose(bool) call As Image does not have unmanaged resources and does not implement finalizer method, there's no need for disposable pattern with a pair of Dispose() & Dispose(bool). Due Dispose(bool) was changed to DisposeManaged(). --- src/ImageSharp/Image.cs | 7 +++---- src/ImageSharp/Image{TPixel}.cs | 8 +------- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/src/ImageSharp/Image.cs b/src/ImageSharp/Image.cs index 3aa30063d8..a3b425233b 100644 --- a/src/ImageSharp/Image.cs +++ b/src/ImageSharp/Image.cs @@ -87,7 +87,7 @@ public void Dispose() return; } - this.Dispose(true); + this.DisposeManaged(); this.isDisposed = true; } @@ -148,10 +148,9 @@ public abstract Image CloneAs(Configuration configuration) protected void UpdateSize(Size size) => this.size = size; /// - /// Disposes the object and frees resources for the Garbage Collector. + /// Internal routine for freeing managed resources called from /// - /// Whether to dispose of managed and unmanaged objects. - protected abstract void Dispose(bool disposing); + protected abstract void DisposeManaged(); /// /// Throws if the image is disposed. diff --git a/src/ImageSharp/Image{TPixel}.cs b/src/ImageSharp/Image{TPixel}.cs index c436430522..1fc77dc1f0 100644 --- a/src/ImageSharp/Image{TPixel}.cs +++ b/src/ImageSharp/Image{TPixel}.cs @@ -268,13 +268,7 @@ public override Image CloneAs(Configuration configuration) } /// - protected override void Dispose(bool disposing) - { - if (disposing) - { - this.frames.Dispose(); - } - } + protected override void DisposeManaged() => this.frames.Dispose(); /// public override string ToString() => $"Image<{typeof(TPixel).Name}>: {this.Width}x{this.Height}"; From ff4b269d590fbbad7d277a4f200ee0c7ac080e41 Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Fri, 14 May 2021 01:03:50 +0300 Subject: [PATCH 08/21] Removed invalid tests Subject to discuss. Image public properties Height, Width, Metadata and PixelType can't corrupt anything if backing image was disposed so I don't see any point altering that behaviour, it wasn't throwing before this branch and shouldn't throw after. --- tests/ImageSharp.Tests/Image/ImageTests.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/ImageSharp.Tests/Image/ImageTests.cs b/tests/ImageSharp.Tests/Image/ImageTests.cs index 1c942ac055..b6d78a356d 100644 --- a/tests/ImageSharp.Tests/Image/ImageTests.cs +++ b/tests/ImageSharp.Tests/Image/ImageTests.cs @@ -193,8 +193,6 @@ public void NonPrivateProperties_ObjectDisposedException() // Image Assert.Throws(() => { var prop = image.Frames; }); - Assert.Throws(() => { var prop = image.Metadata; }); - Assert.Throws(() => { var prop = image.PixelType; }); // Image Assert.Throws(() => { var prop = genericImage.Frames; }); From cbca5657889fb9a370f5b049d60dbae108104cfd Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Fri, 14 May 2021 03:00:57 +0300 Subject: [PATCH 09/21] ImageFrameCollection now properly implements IDisposable interface & ensures all operations are called on valid object --- src/ImageSharp/ImageFrameCollection.cs | 95 +++++++++++++++++-- .../ImageFrameCollection{TPixel}.cs | 3 +- 2 files changed, 88 insertions(+), 10 deletions(-) diff --git a/src/ImageSharp/ImageFrameCollection.cs b/src/ImageSharp/ImageFrameCollection.cs index 62ecc71f55..53ab2bf7c1 100644 --- a/src/ImageSharp/ImageFrameCollection.cs +++ b/src/ImageSharp/ImageFrameCollection.cs @@ -11,8 +11,10 @@ namespace SixLabors.ImageSharp /// Encapsulates a pixel-agnostic collection of instances /// that make up an . /// - public abstract class ImageFrameCollection : IEnumerable + public abstract class ImageFrameCollection : IDisposable, IEnumerable { + private bool isDisposed; + /// /// Gets the number of frames. /// @@ -21,7 +23,15 @@ public abstract class ImageFrameCollection : IEnumerable /// /// Gets the root frame. /// - public ImageFrame RootFrame => this.NonGenericRootFrame; + public ImageFrame RootFrame + { + get + { + this.EnsureNotDisposed(); + + return this.NonGenericRootFrame; + } + } /// /// Gets the root frame. (Implements .) @@ -36,7 +46,15 @@ public abstract class ImageFrameCollection : IEnumerable /// /// The index. /// The at the specified index. - public ImageFrame this[int index] => this.NonGenericGetFrame(index); + public ImageFrame this[int index] + { + get + { + this.EnsureNotDisposed(); + + return this.NonGenericGetFrame(index); + } + } /// /// Determines the index of a specific in the . @@ -59,7 +77,12 @@ public abstract class ImageFrameCollection : IEnumerable /// /// The raw pixel data to generate the from. /// The cloned . - public ImageFrame AddFrame(ImageFrame source) => this.NonGenericAddFrame(source); + public ImageFrame AddFrame(ImageFrame source) + { + this.EnsureNotDisposed(); + + return this.NonGenericAddFrame(source); + } /// /// Removes the frame at the specified index and frees all freeable resources associated with it. @@ -91,7 +114,12 @@ public abstract class ImageFrameCollection : IEnumerable /// The zero-based index of the frame to export. /// Cannot remove last frame. /// The new with the specified frame. - public Image ExportFrame(int index) => this.NonGenericExportFrame(index); + public Image ExportFrame(int index) + { + this.EnsureNotDisposed(); + + return this.NonGenericExportFrame(index); + } /// /// Creates an with only the frame at the specified index @@ -99,7 +127,12 @@ public abstract class ImageFrameCollection : IEnumerable /// /// The zero-based index of the frame to clone. /// The new with the specified frame. - public Image CloneFrame(int index) => this.NonGenericCloneFrame(index); + public Image CloneFrame(int index) + { + this.EnsureNotDisposed(); + + return this.NonGenericCloneFrame(index); + } /// /// Creates a new and appends it to the end of the collection. @@ -107,7 +140,12 @@ public abstract class ImageFrameCollection : IEnumerable /// /// The new . /// - public ImageFrame CreateFrame() => this.NonGenericCreateFrame(); + public ImageFrame CreateFrame() + { + this.EnsureNotDisposed(); + + return this.NonGenericCreateFrame(); + } /// /// Creates a new and appends it to the end of the collection. @@ -116,14 +154,53 @@ public abstract class ImageFrameCollection : IEnumerable /// /// The new . /// - public ImageFrame CreateFrame(Color backgroundColor) => this.NonGenericCreateFrame(backgroundColor); + public ImageFrame CreateFrame(Color backgroundColor) + { + this.EnsureNotDisposed(); + + return this.NonGenericCreateFrame(backgroundColor); + } /// - public IEnumerator GetEnumerator() => this.NonGenericGetEnumerator(); + public void Dispose() + { + if (this.isDisposed) + { + return; + } + + this.DisposeManaged(); + + this.isDisposed = true; + } + + /// + public IEnumerator GetEnumerator() + { + this.EnsureNotDisposed(); + + return this.NonGenericGetEnumerator(); + } /// IEnumerator IEnumerable.GetEnumerator() => this.GetEnumerator(); + /// + /// Throws if the image frame is disposed. + /// + protected void EnsureNotDisposed() + { + if(this.isDisposed) + { + throw new ObjectDisposedException("Trying to execute an operation on a disposed image frame."); + } + } + + /// + /// Internal routine for freeing managed resources called from + /// + protected abstract void DisposeManaged(); + /// /// Implements . /// diff --git a/src/ImageSharp/ImageFrameCollection{TPixel}.cs b/src/ImageSharp/ImageFrameCollection{TPixel}.cs index 36c3ee481f..b51e4dae53 100644 --- a/src/ImageSharp/ImageFrameCollection{TPixel}.cs +++ b/src/ImageSharp/ImageFrameCollection{TPixel}.cs @@ -335,7 +335,8 @@ private void ValidateFrame(ImageFrame frame) } } - internal void Dispose() + /// + protected override void DisposeManaged() { foreach (ImageFrame f in this.frames) { From 127e9ddcdd37e7030febb17efbb323526c05bd01 Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Fri, 14 May 2021 03:13:16 +0300 Subject: [PATCH 10/21] All ImageFrameCollection public properties & method now check if object was disposed --- .../ImageFrameCollection{TPixel}.cs | 58 ++++++++++++++++--- 1 file changed, 50 insertions(+), 8 deletions(-) diff --git a/src/ImageSharp/ImageFrameCollection{TPixel}.cs b/src/ImageSharp/ImageFrameCollection{TPixel}.cs index b51e4dae53..c1eae02806 100644 --- a/src/ImageSharp/ImageFrameCollection{TPixel}.cs +++ b/src/ImageSharp/ImageFrameCollection{TPixel}.cs @@ -67,7 +67,17 @@ internal ImageFrameCollection(Image parent, IEnumerable /// Gets the root frame. /// - public new ImageFrame RootFrame => this.frames.Count > 0 ? this.frames[0] : null; + public new ImageFrame RootFrame + { + get + { + this.EnsureNotDisposed(); + + // frame collection would always contain at least 1 frame + // the only exception is when collection is disposed what is checked via EnsureNotDisposed() call + return this.frames[0]; + } + } /// protected override ImageFrame NonGenericRootFrame => this.RootFrame; @@ -80,20 +90,30 @@ internal ImageFrameCollection(Image parent, IEnumerable /// The index. /// The at the specified index. - public new ImageFrame this[int index] => this.frames[index]; - - /// - public override int IndexOf(ImageFrame frame) + public new ImageFrame this[int index] { - return frame is ImageFrame specific ? this.IndexOf(specific) : -1; + get + { + this.EnsureNotDisposed(); + + return this.frames[index]; + } } + /// + public override int IndexOf(ImageFrame frame) => frame is ImageFrame specific ? this.IndexOf(specific) : -1; + /// /// Determines the index of a specific in the . /// /// The to locate in the . /// The index of item if found in the list; otherwise, -1. - public int IndexOf(ImageFrame frame) => this.frames.IndexOf(frame); + public int IndexOf(ImageFrame frame) + { + this.EnsureNotDisposed(); + + return this.frames.IndexOf(frame); + } /// /// Clones and inserts the into the at the specified . @@ -104,6 +124,8 @@ public override int IndexOf(ImageFrame frame) /// The cloned . public ImageFrame InsertFrame(int index, ImageFrame source) { + this.EnsureNotDisposed(); + this.ValidateFrame(source); ImageFrame clonedFrame = source.Clone(this.parent.GetConfiguration()); this.frames.Insert(index, clonedFrame); @@ -117,6 +139,8 @@ public ImageFrame InsertFrame(int index, ImageFrame source) /// The cloned . public ImageFrame AddFrame(ImageFrame source) { + this.EnsureNotDisposed(); + this.ValidateFrame(source); ImageFrame clonedFrame = source.Clone(this.parent.GetConfiguration()); this.frames.Add(clonedFrame); @@ -131,6 +155,8 @@ public ImageFrame AddFrame(ImageFrame source) /// The new . public ImageFrame AddFrame(ReadOnlySpan source) { + this.EnsureNotDisposed(); + var frame = ImageFrame.LoadPixelData( this.parent.GetConfiguration(), source, @@ -149,6 +175,7 @@ public ImageFrame AddFrame(ReadOnlySpan source) public ImageFrame AddFrame(TPixel[] source) { Guard.NotNull(source, nameof(source)); + return this.AddFrame(source.AsSpan()); } @@ -159,6 +186,8 @@ public ImageFrame AddFrame(TPixel[] source) /// Cannot remove last frame. public override void RemoveFrame(int index) { + this.EnsureNotDisposed(); + if (index == 0 && this.Count == 1) { throw new InvalidOperationException("Cannot remove last frame."); @@ -180,7 +209,12 @@ public override bool Contains(ImageFrame frame) => /// /// true if the contains the specified frame; otherwise, false. /// - public bool Contains(ImageFrame frame) => this.frames.Contains(frame); + public bool Contains(ImageFrame frame) + { + this.EnsureNotDisposed(); + + return this.frames.Contains(frame); + } /// /// Moves an from to . @@ -189,6 +223,8 @@ public override bool Contains(ImageFrame frame) => /// The index to move the frame to. public override void MoveFrame(int sourceIndex, int destinationIndex) { + this.EnsureNotDisposed(); + if (sourceIndex == destinationIndex) { return; @@ -208,6 +244,8 @@ public override void MoveFrame(int sourceIndex, int destinationIndex) /// The new with the specified frame. public new Image ExportFrame(int index) { + this.EnsureNotDisposed(); + ImageFrame frame = this[index]; if (this.Count == 1 && this.frames.Contains(frame)) @@ -228,6 +266,8 @@ public override void MoveFrame(int sourceIndex, int destinationIndex) /// The new with the specified frame. public new Image CloneFrame(int index) { + this.EnsureNotDisposed(); + ImageFrame frame = this[index]; ImageFrame clonedFrame = frame.Clone(); return new Image(this.parent.GetConfiguration(), this.parent.Metadata.DeepClone(), new[] { clonedFrame }); @@ -241,6 +281,8 @@ public override void MoveFrame(int sourceIndex, int destinationIndex) /// public new ImageFrame CreateFrame() { + this.EnsureNotDisposed(); + var frame = new ImageFrame( this.parent.GetConfiguration(), this.RootFrame.Width, From 3f8bd3d2e67913d2aa3500928dca44d6ed7fd35b Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Fri, 14 May 2021 03:20:03 +0300 Subject: [PATCH 11/21] Added internal accessor for root frame --- src/ImageSharp/ImageFrameCollection{TPixel}.cs | 9 +++++++++ src/ImageSharp/Image{TPixel}.cs | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/ImageSharp/ImageFrameCollection{TPixel}.cs b/src/ImageSharp/ImageFrameCollection{TPixel}.cs index c1eae02806..023da0d347 100644 --- a/src/ImageSharp/ImageFrameCollection{TPixel}.cs +++ b/src/ImageSharp/ImageFrameCollection{TPixel}.cs @@ -79,6 +79,15 @@ internal ImageFrameCollection(Image parent, IEnumerable + /// Gets root frame accessor in unsafe manner without any checks. + /// + /// + /// This property is most likely to be called from for indexing pixels. + /// already checks if it was disposed before querying for root frame. + /// + internal ImageFrame RootFrameUnsafe => this.frames[0]; + /// protected override ImageFrame NonGenericRootFrame => this.RootFrame; diff --git a/src/ImageSharp/Image{TPixel}.cs b/src/ImageSharp/Image{TPixel}.cs index 1fc77dc1f0..3805446fe5 100644 --- a/src/ImageSharp/Image{TPixel}.cs +++ b/src/ImageSharp/Image{TPixel}.cs @@ -158,7 +158,7 @@ internal Image(Configuration configuration, ImageMetadata metadata, IEnumerable< /// /// Gets the root frame. /// - private IPixelSource PixelSource => this.frames.RootFrame; + private IPixelSource PixelSource => this.frames.RootFrameUnsafe; /// /// Gets or sets the pixel at the specified position. From 009e9357bd13776b8cf2f8f90dfa9332f87e8d84 Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Fri, 14 May 2021 03:59:58 +0300 Subject: [PATCH 12/21] Added tests for issues#1628 --- .../ImageFrameCollectionTests.Generic.cs | 42 +++++++++++++++++++ .../ImageFrameCollectionTests.NonGeneric.cs | 35 ++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/tests/ImageSharp.Tests/Image/ImageFrameCollectionTests.Generic.cs b/tests/ImageSharp.Tests/Image/ImageFrameCollectionTests.Generic.cs index ecbc331b28..c889fa76b2 100644 --- a/tests/ImageSharp.Tests/Image/ImageFrameCollectionTests.Generic.cs +++ b/tests/ImageSharp.Tests/Image/ImageFrameCollectionTests.Generic.cs @@ -323,6 +323,48 @@ public void Contains_FalseIfNonMember() var frame = new ImageFrame(Configuration.Default, 10, 10); Assert.False(this.Image.Frames.Contains(frame)); } + + [Fact] + public void DisposeCall_NoThrowIfCalledMultiple() + { + var image = new Image(Configuration.Default, 10, 10); + var frameCollection = image.Frames as ImageFrameCollection; + + image.Dispose(); // this should invalidate underlying collection as well + frameCollection.Dispose(); + } + + [Fact] + public void PublicProperties_ThrowIfDisposed() + { + var image = new Image(Configuration.Default, 10, 10); + var frameCollection = image.Frames as ImageFrameCollection; + + image.Dispose(); // this should invalidate underlying collection as well + + Assert.Throws(() => { var prop = frameCollection.RootFrame; }); + } + + [Fact] + public void PublicMethods_ThrowIfDisposed() + { + var image = new Image(Configuration.Default, 10, 10); + var frameCollection = image.Frames as ImageFrameCollection; + + image.Dispose(); // this should invalidate underlying collection as well + + Assert.Throws(() => { var res = frameCollection.AddFrame(default); }); + Assert.Throws(() => { var res = frameCollection.CloneFrame(default); }); + Assert.Throws(() => { var res = frameCollection.Contains(default); }); + Assert.Throws(() => { var res = frameCollection.CreateFrame(); }); + Assert.Throws(() => { var res = frameCollection.CreateFrame(default); }); + Assert.Throws(() => { var res = frameCollection.ExportFrame(default); }); + Assert.Throws(() => { var res = frameCollection.GetEnumerator(); }); + Assert.Throws(() => { var prop = frameCollection.IndexOf(default); }); + Assert.Throws(() => { var prop = frameCollection.InsertFrame(default, default); }); + Assert.Throws(() => { frameCollection.RemoveFrame(default); }); + Assert.Throws(() => { frameCollection.MoveFrame(default, default); }); + } } } } diff --git a/tests/ImageSharp.Tests/Image/ImageFrameCollectionTests.NonGeneric.cs b/tests/ImageSharp.Tests/Image/ImageFrameCollectionTests.NonGeneric.cs index 92109ed479..7ff6b9b085 100644 --- a/tests/ImageSharp.Tests/Image/ImageFrameCollectionTests.NonGeneric.cs +++ b/tests/ImageSharp.Tests/Image/ImageFrameCollectionTests.NonGeneric.cs @@ -263,6 +263,41 @@ public void Contains_FalseIfNonMember() Assert.False(this.Image.Frames.Contains(frame)); } + [Fact] + public void PublicProperties_ThrowIfDisposed() + { + var image = new Image(Configuration.Default, 10, 10); + var frameCollection = image.Frames; + + image.Dispose(); // this should invalidate underlying collection as well + + Assert.Throws(() => { var prop = frameCollection.RootFrame; }); + } + + [Fact] + public void PublicMethods_ThrowIfDisposed() + { + var image = new Image(Configuration.Default, 10, 10); + var frameCollection = image.Frames; + + image.Dispose(); // this should invalidate underlying collection as well + + Assert.Throws(() => { var res = frameCollection.AddFrame((ImageFrame)null); }); + Assert.Throws(() => { var res = frameCollection.AddFrame((Rgba32[])null); }); + Assert.Throws(() => { var res = frameCollection.AddFrame((ImageFrame)null); }); + Assert.Throws(() => { var res = frameCollection.AddFrame(stackalloc Rgba32[0]); }); + Assert.Throws(() => { var res = frameCollection.CloneFrame(default); }); + Assert.Throws(() => { var res = frameCollection.Contains(default); }); + Assert.Throws(() => { var res = frameCollection.CreateFrame(); }); + Assert.Throws(() => { var res = frameCollection.CreateFrame(default); }); + Assert.Throws(() => { var res = frameCollection.ExportFrame(default); }); + Assert.Throws(() => { var res = frameCollection.GetEnumerator(); }); + Assert.Throws(() => { var prop = frameCollection.IndexOf(default); }); + Assert.Throws(() => { var prop = frameCollection.InsertFrame(default, default); }); + Assert.Throws(() => { frameCollection.RemoveFrame(default); }); + Assert.Throws(() => { frameCollection.MoveFrame(default, default); }); + } + /// /// Integration test for end-to end API validation. /// From f0f0c088abdf974f4c2e0a09425b5481ae2791d1 Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Fri, 14 May 2021 05:27:25 +0300 Subject: [PATCH 13/21] Fixed couple of invalid tests for ImageFrameCollection --- .../Image/ImageFrameCollectionTests.NonGeneric.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/ImageSharp.Tests/Image/ImageFrameCollectionTests.NonGeneric.cs b/tests/ImageSharp.Tests/Image/ImageFrameCollectionTests.NonGeneric.cs index 7ff6b9b085..15838f6902 100644 --- a/tests/ImageSharp.Tests/Image/ImageFrameCollectionTests.NonGeneric.cs +++ b/tests/ImageSharp.Tests/Image/ImageFrameCollectionTests.NonGeneric.cs @@ -279,13 +279,14 @@ public void PublicMethods_ThrowIfDisposed() { var image = new Image(Configuration.Default, 10, 10); var frameCollection = image.Frames; + var rgba32Array = new Rgba32[0]; image.Dispose(); // this should invalidate underlying collection as well Assert.Throws(() => { var res = frameCollection.AddFrame((ImageFrame)null); }); - Assert.Throws(() => { var res = frameCollection.AddFrame((Rgba32[])null); }); + Assert.Throws(() => { var res = frameCollection.AddFrame(rgba32Array); }); Assert.Throws(() => { var res = frameCollection.AddFrame((ImageFrame)null); }); - Assert.Throws(() => { var res = frameCollection.AddFrame(stackalloc Rgba32[0]); }); + Assert.Throws(() => { var res = frameCollection.AddFrame(rgba32Array.AsSpan()); }); Assert.Throws(() => { var res = frameCollection.CloneFrame(default); }); Assert.Throws(() => { var res = frameCollection.Contains(default); }); Assert.Throws(() => { var res = frameCollection.CreateFrame(); }); From a71ce1913f6066b3cf9769f37cbea063c57c3e23 Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Fri, 14 May 2021 05:28:28 +0300 Subject: [PATCH 14/21] ImageFrameCollection.Contains first checks if it was disposed first --- src/ImageSharp/ImageFrameCollection{TPixel}.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/ImageSharp/ImageFrameCollection{TPixel}.cs b/src/ImageSharp/ImageFrameCollection{TPixel}.cs index 023da0d347..92722494b1 100644 --- a/src/ImageSharp/ImageFrameCollection{TPixel}.cs +++ b/src/ImageSharp/ImageFrameCollection{TPixel}.cs @@ -208,8 +208,12 @@ public override void RemoveFrame(int index) } /// - public override bool Contains(ImageFrame frame) => - frame is ImageFrame specific && this.Contains(specific); + public override bool Contains(ImageFrame frame) + { + this.EnsureNotDisposed(); + + return frame is ImageFrame specific && this.frames.Contains(specific); + } /// /// Determines whether the contains the . From f56105393b2e9300dfba5cf4979578f7a99c176b Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Fri, 14 May 2021 19:14:53 +0300 Subject: [PATCH 15/21] Fixed a couple of failing tests --- src/ImageSharp/ImageFrameCollection.cs | 7 ++++++- src/ImageSharp/ImageFrameCollection{TPixel}.cs | 7 ++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/ImageSharp/ImageFrameCollection.cs b/src/ImageSharp/ImageFrameCollection.cs index 53ab2bf7c1..ed36c16539 100644 --- a/src/ImageSharp/ImageFrameCollection.cs +++ b/src/ImageSharp/ImageFrameCollection.cs @@ -70,7 +70,12 @@ public ImageFrame this[int index] /// The to clone and insert into the . /// Frame must have the same dimensions as the image. /// The cloned . - public ImageFrame InsertFrame(int index, ImageFrame source) => this.NonGenericInsertFrame(index, source); + public ImageFrame InsertFrame(int index, ImageFrame source) + { + this.EnsureNotDisposed(); + + return this.NonGenericInsertFrame(index, source); + } /// /// Clones the frame and appends the clone to the end of the collection. diff --git a/src/ImageSharp/ImageFrameCollection{TPixel}.cs b/src/ImageSharp/ImageFrameCollection{TPixel}.cs index 92722494b1..13ae092c42 100644 --- a/src/ImageSharp/ImageFrameCollection{TPixel}.cs +++ b/src/ImageSharp/ImageFrameCollection{TPixel}.cs @@ -110,7 +110,12 @@ internal ImageFrameCollection(Image parent, IEnumerable - public override int IndexOf(ImageFrame frame) => frame is ImageFrame specific ? this.IndexOf(specific) : -1; + public override int IndexOf(ImageFrame frame) + { + this.EnsureNotDisposed(); + + return frame is ImageFrame specific ? this.frames.IndexOf(specific) : -1; + } /// /// Determines the index of a specific in the . From 1c8dcefd6dda4fd1677e0f2d8a64a2edc0086d46 Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Fri, 21 May 2021 20:16:21 +0300 Subject: [PATCH 16/21] Renamed private Image.PixelSourse to PixelSourceUnsafe --- src/ImageSharp/Image{TPixel}.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/ImageSharp/Image{TPixel}.cs b/src/ImageSharp/Image{TPixel}.cs index 3805446fe5..9e3ad2636f 100644 --- a/src/ImageSharp/Image{TPixel}.cs +++ b/src/ImageSharp/Image{TPixel}.cs @@ -158,7 +158,7 @@ internal Image(Configuration configuration, ImageMetadata metadata, IEnumerable< /// /// Gets the root frame. /// - private IPixelSource PixelSource => this.frames.RootFrameUnsafe; + private IPixelSource PixelSourceUnsafe => this.frames.RootFrameUnsafe; /// /// Gets or sets the pixel at the specified position. @@ -175,7 +175,7 @@ internal Image(Configuration configuration, ImageMetadata metadata, IEnumerable< this.EnsureNotDisposed(); this.VerifyCoords(x, y); - return this.PixelSource.PixelBuffer.GetElementUnsafe(x, y); + return this.PixelSourceUnsafe.PixelBuffer.GetElementUnsafe(x, y); } [MethodImpl(InliningOptions.ShortMethod)] @@ -184,7 +184,7 @@ internal Image(Configuration configuration, ImageMetadata metadata, IEnumerable< this.EnsureNotDisposed(); this.VerifyCoords(x, y); - this.PixelSource.PixelBuffer.GetElementUnsafe(x, y) = value; + this.PixelSourceUnsafe.PixelBuffer.GetElementUnsafe(x, y) = value; } } @@ -202,7 +202,7 @@ public Span GetPixelRowSpan(int rowIndex) this.EnsureNotDisposed(); - return this.PixelSource.PixelBuffer.GetRowSpan(rowIndex); + return this.PixelSourceUnsafe.PixelBuffer.GetRowSpan(rowIndex); } /// From e787ffa518b2e20406bbe41a9a0e611fa28f87b2 Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Fri, 21 May 2021 20:23:10 +0300 Subject: [PATCH 17/21] Implemented dispose method according to common convention. --- src/ImageSharp/Image.cs | 6 ++++-- src/ImageSharp/ImageFrameCollection.cs | 6 ++++-- src/ImageSharp/ImageFrameCollection{TPixel}.cs | 13 ++++++++----- src/ImageSharp/Image{TPixel}.cs | 8 +++++++- 4 files changed, 23 insertions(+), 10 deletions(-) diff --git a/src/ImageSharp/Image.cs b/src/ImageSharp/Image.cs index a3b425233b..724fa4f96b 100644 --- a/src/ImageSharp/Image.cs +++ b/src/ImageSharp/Image.cs @@ -87,7 +87,8 @@ public void Dispose() return; } - this.DisposeManaged(); + this.Dispose(true); + GC.SuppressFinalize(this); this.isDisposed = true; } @@ -150,7 +151,8 @@ public abstract Image CloneAs(Configuration configuration) /// /// Internal routine for freeing managed resources called from /// - protected abstract void DisposeManaged(); + /// /// Whether to dispose of managed objects. + protected abstract void Dispose(bool disposing); /// /// Throws if the image is disposed. diff --git a/src/ImageSharp/ImageFrameCollection.cs b/src/ImageSharp/ImageFrameCollection.cs index ed36c16539..16d4285782 100644 --- a/src/ImageSharp/ImageFrameCollection.cs +++ b/src/ImageSharp/ImageFrameCollection.cs @@ -174,7 +174,8 @@ public void Dispose() return; } - this.DisposeManaged(); + this.Dispose(true); + GC.SuppressFinalize(this); this.isDisposed = true; } @@ -204,7 +205,8 @@ protected void EnsureNotDisposed() /// /// Internal routine for freeing managed resources called from /// - protected abstract void DisposeManaged(); + /// /// /// Whether to dispose of managed objects. + protected abstract void Dispose(bool disposing); /// /// Implements . diff --git a/src/ImageSharp/ImageFrameCollection{TPixel}.cs b/src/ImageSharp/ImageFrameCollection{TPixel}.cs index 13ae092c42..da024c9176 100644 --- a/src/ImageSharp/ImageFrameCollection{TPixel}.cs +++ b/src/ImageSharp/ImageFrameCollection{TPixel}.cs @@ -396,14 +396,17 @@ private void ValidateFrame(ImageFrame frame) } /// - protected override void DisposeManaged() + protected override void Dispose(bool disposing) { - foreach (ImageFrame f in this.frames) + if (disposing) { - f.Dispose(); - } + foreach (ImageFrame f in this.frames) + { + f.Dispose(); + } - this.frames.Clear(); + this.frames.Clear(); + } } private ImageFrame CopyNonCompatibleFrame(ImageFrame source) diff --git a/src/ImageSharp/Image{TPixel}.cs b/src/ImageSharp/Image{TPixel}.cs index 9e3ad2636f..e42022729f 100644 --- a/src/ImageSharp/Image{TPixel}.cs +++ b/src/ImageSharp/Image{TPixel}.cs @@ -268,7 +268,13 @@ public override Image CloneAs(Configuration configuration) } /// - protected override void DisposeManaged() => this.frames.Dispose(); + protected override void Dispose(bool disposing) + { + if (disposing) + { + this.frames.Dispose(); + } + } /// public override string ToString() => $"Image<{typeof(TPixel).Name}>: {this.Width}x{this.Height}"; From d54ff0e084aa823464f4f64c0ef32f30471b5c79 Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Fri, 21 May 2021 20:25:31 +0300 Subject: [PATCH 18/21] Fixed disposable resouce leak in unit test. --- tests/ImageSharp.Tests/Image/ImageTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ImageSharp.Tests/Image/ImageTests.cs b/tests/ImageSharp.Tests/Image/ImageTests.cs index b6d78a356d..1296f26c47 100644 --- a/tests/ImageSharp.Tests/Image/ImageTests.cs +++ b/tests/ImageSharp.Tests/Image/ImageTests.cs @@ -201,8 +201,8 @@ public void NonPrivateProperties_ObjectDisposedException() [Fact] public void Save_ObjectDisposedException() { + using var stream = new MemoryStream(); var image = new Image(this.configuration, 10, 10); - var stream = new MemoryStream(); var encoder = new JpegEncoder(); image.Dispose(); From 5704403030f8781da651c261fc4a4b50360c675c Mon Sep 17 00:00:00 2001 From: Dmitry Pentin Date: Fri, 21 May 2021 20:52:38 +0300 Subject: [PATCH 19/21] Implemented ThrowObjectDisposedException for the ThrowHelper, replaced raw throws with this in Image/ImageFrameCollection --- src/ImageSharp/Image.cs | 2 +- src/ImageSharp/ImageFrameCollection.cs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/ImageSharp/Image.cs b/src/ImageSharp/Image.cs index 724fa4f96b..fe72ec5c00 100644 --- a/src/ImageSharp/Image.cs +++ b/src/ImageSharp/Image.cs @@ -161,7 +161,7 @@ internal void EnsureNotDisposed() { if (this.isDisposed) { - throw new ObjectDisposedException("Trying to execute an operation on a disposed image."); + ThrowHelper.ThrowObjectDisposedException(this.GetType()); } } diff --git a/src/ImageSharp/ImageFrameCollection.cs b/src/ImageSharp/ImageFrameCollection.cs index 16d4285782..8c8edcd7a5 100644 --- a/src/ImageSharp/ImageFrameCollection.cs +++ b/src/ImageSharp/ImageFrameCollection.cs @@ -196,9 +196,9 @@ public IEnumerator GetEnumerator() /// protected void EnsureNotDisposed() { - if(this.isDisposed) + if (this.isDisposed) { - throw new ObjectDisposedException("Trying to execute an operation on a disposed image frame."); + ThrowHelper.ThrowObjectDisposedException(this.GetType()); } } From 65808ae55f1bc069434bacbf1964895720b9b1d0 Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Wed, 2 Jun 2021 15:34:39 +0100 Subject: [PATCH 20/21] Fix throwhelper --- src/ImageSharp/Image.cs | 10 +++++++--- src/ImageSharp/ImageFrameCollection.cs | 10 +++++++--- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/src/ImageSharp/Image.cs b/src/ImageSharp/Image.cs index fe72ec5c00..22b9ce8e5c 100644 --- a/src/ImageSharp/Image.cs +++ b/src/ImageSharp/Image.cs @@ -3,6 +3,7 @@ using System; using System.IO; +using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; using SixLabors.ImageSharp.Advanced; @@ -149,9 +150,9 @@ public abstract Image CloneAs(Configuration configuration) protected void UpdateSize(Size size) => this.size = size; /// - /// Internal routine for freeing managed resources called from + /// Disposes the object and frees resources for the Garbage Collector. /// - /// /// Whether to dispose of managed objects. + /// Whether to dispose of managed and unmanaged objects. protected abstract void Dispose(bool disposing); /// @@ -161,7 +162,7 @@ internal void EnsureNotDisposed() { if (this.isDisposed) { - ThrowHelper.ThrowObjectDisposedException(this.GetType()); + ThrowObjectDisposedException(this.GetType()); } } @@ -182,6 +183,9 @@ internal void EnsureNotDisposed() /// The token to monitor for cancellation requests. internal abstract Task AcceptAsync(IImageVisitorAsync visitor, CancellationToken cancellationToken); + [MethodImpl(InliningOptions.ColdPath)] + private static void ThrowObjectDisposedException(Type type) => throw new ObjectDisposedException(type.Name); + private class EncodeVisitor : IImageVisitor, IImageVisitorAsync { private readonly IImageEncoder encoder; diff --git a/src/ImageSharp/ImageFrameCollection.cs b/src/ImageSharp/ImageFrameCollection.cs index 8c8edcd7a5..07ba8c87f3 100644 --- a/src/ImageSharp/ImageFrameCollection.cs +++ b/src/ImageSharp/ImageFrameCollection.cs @@ -4,6 +4,7 @@ using System; using System.Collections; using System.Collections.Generic; +using System.Runtime.CompilerServices; namespace SixLabors.ImageSharp { @@ -198,14 +199,14 @@ protected void EnsureNotDisposed() { if (this.isDisposed) { - ThrowHelper.ThrowObjectDisposedException(this.GetType()); + ThrowObjectDisposedException(this.GetType()); } } /// - /// Internal routine for freeing managed resources called from + /// Disposes the object and frees resources for the Garbage Collector. /// - /// /// /// Whether to dispose of managed objects. + /// Whether to dispose of managed and unmanaged objects. protected abstract void Dispose(bool disposing); /// @@ -262,5 +263,8 @@ protected void EnsureNotDisposed() /// The background color. /// The new frame. protected abstract ImageFrame NonGenericCreateFrame(Color backgroundColor); + + [MethodImpl(InliningOptions.ColdPath)] + private static void ThrowObjectDisposedException(Type type) => throw new ObjectDisposedException(type.Name); } } From afee88123c36c11b506673d709d11db366c0e18c Mon Sep 17 00:00:00 2001 From: James Jackson-South Date: Wed, 2 Jun 2021 17:17:19 +0100 Subject: [PATCH 21/21] Make frames resonly --- src/ImageSharp/Image.cs | 2 +- src/ImageSharp/Image{TPixel}.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ImageSharp/Image.cs b/src/ImageSharp/Image.cs index 22b9ce8e5c..ce6aa69b58 100644 --- a/src/ImageSharp/Image.cs +++ b/src/ImageSharp/Image.cs @@ -99,7 +99,7 @@ public void Dispose() /// /// The stream to save the image to. /// The encoder to save the image with. - /// Thrown if the stream or encoder is null. + /// Thrown if the stream or encoder is null. public void Save(Stream stream, IImageEncoder encoder) { Guard.NotNull(stream, nameof(stream)); diff --git a/src/ImageSharp/Image{TPixel}.cs b/src/ImageSharp/Image{TPixel}.cs index e42022729f..b43ff0422b 100644 --- a/src/ImageSharp/Image{TPixel}.cs +++ b/src/ImageSharp/Image{TPixel}.cs @@ -23,7 +23,7 @@ namespace SixLabors.ImageSharp public sealed class Image : Image where TPixel : unmanaged, IPixel { - private ImageFrameCollection frames; + private readonly ImageFrameCollection frames; /// /// Initializes a new instance of the class