Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed Issue#1628 #1629

Merged
merged 23 commits into from
Jun 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
7900b43
Image<TPixel>.Frames now properly throws ObjectDisposedException afte…
May 13, 2021
d48b152
Image<TPixel> private methods no longer check if object was disposed …
May 13, 2021
7029b2f
Image<TPixel> private property PixelSource no longer checks if object…
May 13, 2021
1c45c1a
Removed GC.SuppressFinalize(this) from Image.Dispose() due to it not …
May 13, 2021
095ce41
Added tests for issue#1628
May 13, 2021
acf9d85
Moved dispose control logic to base Image class
May 13, 2021
8ec1013
Removed redundant flag from Image.Dispose(bool) call
May 13, 2021
ff4b269
Removed invalid tests
May 13, 2021
cbca565
ImageFrameCollection now properly implements IDisposable interface & …
May 14, 2021
127e9dd
All ImageFrameCollection<TPixel> public properties & method now check…
May 14, 2021
3f8bd3d
Added internal accessor for root frame
May 14, 2021
009e935
Added tests for issues#1628
May 14, 2021
f0f0c08
Fixed couple of invalid tests for ImageFrameCollection<TPixel>
May 14, 2021
a71ce19
ImageFrameCollection.Contains first checks if it was disposed first
May 14, 2021
f561053
Fixed a couple of failing tests
May 14, 2021
34706fb
Merge branch 'master' into image-disposed-fix
JimBobSquarePants May 18, 2021
1c8dcef
Renamed private Image<TPixel>.PixelSourse to PixelSourceUnsafe
May 21, 2021
e787ffa
Implemented dispose method according to common convention.
May 21, 2021
d54ff0e
Fixed disposable resouce leak in unit test.
May 21, 2021
5704403
Implemented ThrowObjectDisposedException for the ThrowHelper, replace…
May 21, 2021
65808ae
Fix throwhelper
JimBobSquarePants Jun 2, 2021
5d2884e
Merge branch 'master' into pr/1629
JimBobSquarePants Jun 2, 2021
afee881
Make frames resonly
JimBobSquarePants Jun 2, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions src/ImageSharp/Image.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.IO;
using System.Runtime.CompilerServices;
using System.Threading;
using System.Threading.Tasks;
using SixLabors.ImageSharp.Advanced;
Expand All @@ -19,6 +20,8 @@ namespace SixLabors.ImageSharp
/// </summary>
public abstract partial class Image : IImage, IConfigurationProvider
{
private bool isDisposed;

private Size size;
private readonly Configuration configuration;

Expand Down Expand Up @@ -80,16 +83,23 @@ internal Image(
/// <inheritdoc />
public void Dispose()
{
if (this.isDisposed)
{
return;
}

this.Dispose(true);
GC.SuppressFinalize(this);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't be removing the finalizer suppression in the base class.

https://docs.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose#the-dispose-method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't know SuppressFinalize do nothing if type has no finalizer, thanks!


this.isDisposed = true;
}

/// <summary>
/// Saves the image to the given stream using the given image encoder.
/// </summary>
/// <param name="stream">The stream to save the image to.</param>
/// <param name="encoder">The encoder to save the image with.</param>
/// <exception cref="System.ArgumentNullException">Thrown if the stream or encoder is null.</exception>
/// <exception cref="ArgumentNullException">Thrown if the stream or encoder is null.</exception>
public void Save(Stream stream, IImageEncoder encoder)
{
Guard.NotNull(stream, nameof(stream));
Expand Down Expand Up @@ -148,7 +158,13 @@ public abstract Image<TPixel2> CloneAs<TPixel2>(Configuration configuration)
/// <summary>
/// Throws <see cref="ObjectDisposedException"/> if the image is disposed.
/// </summary>
internal abstract void EnsureNotDisposed();
internal void EnsureNotDisposed()
{
if (this.isDisposed)
{
ThrowObjectDisposedException(this.GetType());
}
}

/// <summary>
/// Accepts a <see cref="IImageVisitor"/>.
Expand All @@ -167,6 +183,9 @@ public abstract Image<TPixel2> CloneAs<TPixel2>(Configuration configuration)
/// <param name="cancellationToken">The token to monitor for cancellation requests.</param>
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;
Expand Down
108 changes: 98 additions & 10 deletions src/ImageSharp/ImageFrameCollection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,18 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.Runtime.CompilerServices;

namespace SixLabors.ImageSharp
{
/// <summary>
/// Encapsulates a pixel-agnostic collection of <see cref="ImageFrame"/> instances
/// that make up an <see cref="Image"/>.
/// </summary>
public abstract class ImageFrameCollection : IEnumerable<ImageFrame>
public abstract class ImageFrameCollection : IDisposable, IEnumerable<ImageFrame>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow. I hadn't realized this wasn't already implementing the type. Good catch!

{
private bool isDisposed;

/// <summary>
/// Gets the number of frames.
/// </summary>
Expand All @@ -21,7 +24,15 @@ public abstract class ImageFrameCollection : IEnumerable<ImageFrame>
/// <summary>
/// Gets the root frame.
/// </summary>
public ImageFrame RootFrame => this.NonGenericRootFrame;
public ImageFrame RootFrame
{
get
{
this.EnsureNotDisposed();

return this.NonGenericRootFrame;
}
}

/// <summary>
/// Gets the root frame. (Implements <see cref="RootFrame"/>.)
Expand All @@ -36,7 +47,15 @@ public abstract class ImageFrameCollection : IEnumerable<ImageFrame>
/// </value>
/// <param name="index">The index.</param>
/// <returns>The <see cref="ImageFrame"/> at the specified index.</returns>
public ImageFrame this[int index] => this.NonGenericGetFrame(index);
public ImageFrame this[int index]
{
get
{
this.EnsureNotDisposed();

return this.NonGenericGetFrame(index);
}
}

/// <summary>
/// Determines the index of a specific <paramref name="frame"/> in the <seealso cref="ImageFrameCollection"/>.
Expand All @@ -52,14 +71,24 @@ public abstract class ImageFrameCollection : IEnumerable<ImageFrame>
/// <param name="source">The <seealso cref="ImageFrame"/> to clone and insert into the <seealso cref="ImageFrameCollection"/>.</param>
/// <exception cref="ArgumentException">Frame must have the same dimensions as the image.</exception>
/// <returns>The cloned <see cref="ImageFrame"/>.</returns>
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);
}

/// <summary>
/// Clones the <paramref name="source"/> frame and appends the clone to the end of the collection.
/// </summary>
/// <param name="source">The raw pixel data to generate the <seealso cref="ImageFrame{TPixel}"/> from.</param>
/// <returns>The cloned <see cref="ImageFrame{TPixel}"/>.</returns>
public ImageFrame AddFrame(ImageFrame source) => this.NonGenericAddFrame(source);
public ImageFrame AddFrame(ImageFrame source)
{
this.EnsureNotDisposed();

return this.NonGenericAddFrame(source);
}

/// <summary>
/// Removes the frame at the specified index and frees all freeable resources associated with it.
Expand Down Expand Up @@ -91,23 +120,38 @@ public abstract class ImageFrameCollection : IEnumerable<ImageFrame>
/// <param name="index">The zero-based index of the frame to export.</param>
/// <exception cref="InvalidOperationException">Cannot remove last frame.</exception>
/// <returns>The new <see cref="Image{TPixel}"/> with the specified frame.</returns>
public Image ExportFrame(int index) => this.NonGenericExportFrame(index);
public Image ExportFrame(int index)
{
this.EnsureNotDisposed();

return this.NonGenericExportFrame(index);
}

/// <summary>
/// Creates an <see cref="Image{T}"/> with only the frame at the specified index
/// with the same metadata as the original image.
/// </summary>
/// <param name="index">The zero-based index of the frame to clone.</param>
/// <returns>The new <see cref="Image{TPixel}"/> with the specified frame.</returns>
public Image CloneFrame(int index) => this.NonGenericCloneFrame(index);
public Image CloneFrame(int index)
{
this.EnsureNotDisposed();

return this.NonGenericCloneFrame(index);
}

/// <summary>
/// Creates a new <seealso cref="ImageFrame{TPixel}" /> and appends it to the end of the collection.
/// </summary>
/// <returns>
/// The new <see cref="ImageFrame{TPixel}" />.
/// </returns>
public ImageFrame CreateFrame() => this.NonGenericCreateFrame();
public ImageFrame CreateFrame()
{
this.EnsureNotDisposed();

return this.NonGenericCreateFrame();
}

/// <summary>
/// Creates a new <seealso cref="ImageFrame{TPixel}" /> and appends it to the end of the collection.
Expand All @@ -116,14 +160,55 @@ public abstract class ImageFrameCollection : IEnumerable<ImageFrame>
/// <returns>
/// The new <see cref="ImageFrame{TPixel}" />.
/// </returns>
public ImageFrame CreateFrame(Color backgroundColor) => this.NonGenericCreateFrame(backgroundColor);
public ImageFrame CreateFrame(Color backgroundColor)
{
this.EnsureNotDisposed();

return this.NonGenericCreateFrame(backgroundColor);
}

/// <inheritdoc />
public IEnumerator<ImageFrame> GetEnumerator() => this.NonGenericGetEnumerator();
public void Dispose()
{
if (this.isDisposed)
{
return;
}

this.Dispose(true);
GC.SuppressFinalize(this);

this.isDisposed = true;
}

/// <inheritdoc />
public IEnumerator<ImageFrame> GetEnumerator()
{
this.EnsureNotDisposed();

return this.NonGenericGetEnumerator();
}

/// <inheritdoc/>
IEnumerator IEnumerable.GetEnumerator() => this.GetEnumerator();

/// <summary>
/// Throws <see cref="ObjectDisposedException"/> if the image frame is disposed.
/// </summary>
protected void EnsureNotDisposed()
{
if (this.isDisposed)
{
ThrowObjectDisposedException(this.GetType());
}
}

/// <summary>
/// Disposes the object and frees resources for the Garbage Collector.
/// </summary>
/// <param name="disposing">Whether to dispose of managed and unmanaged objects.</param>
protected abstract void Dispose(bool disposing);

/// <summary>
/// Implements <see cref="GetEnumerator"/>.
/// </summary>
Expand Down Expand Up @@ -178,5 +263,8 @@ public abstract class ImageFrameCollection : IEnumerable<ImageFrame>
/// <param name="backgroundColor">The background color.</param>
/// <returns>The new frame.</returns>
protected abstract ImageFrame NonGenericCreateFrame(Color backgroundColor);

[MethodImpl(InliningOptions.ColdPath)]
private static void ThrowObjectDisposedException(Type type) => throw new ObjectDisposedException(type.Name);
}
}
Loading