Image & underlying ImageFrameCollection aren't behaving like they were disposed after calling Dispose()

[br3aker]

Prerequisites

• I have written a descriptive issue title
• I have verified that I am running the latest version of ImageSharp
• I have verified if the problem exist in both DEBUG and RELEASE mode
• I have searched open and closed issues to ensure it has not already been reported

Description

Image& Image underlying frames aren't disposed correctly, can be accessed after parent image disposal and can be mutated like it's a valid instance.

Frames access properties do not ensure that they are not disposed:

ImageSharp/src/ImageSharp/Image{TPixel}.cs

Line 144 in 993f96e

protected override ImageFrameCollection NonGenericFrameCollection => this.Frames;

ImageSharp/src/ImageSharp/Image{TPixel}.cs

Line 149 in 993f96e

public new ImageFrameCollection<TPixel> Frames { get; }

Dispose method does not nullify Frames property:

ImageSharp/src/ImageSharp/Image{TPixel}.cs

Lines 258 to 271 in 993f96e

	protected override void Dispose(bool disposing)
	{
	if (this.isDisposed)
	{
	return;
	}
	if (disposing)
	{
	this.Frames.Dispose();
	}
	this.isDisposed = true;
	}

Nearly all ImageFrameCollection methods affected leading to odd situations:

var image = Image.Load<ImageSharp.PixelFormats.Rgba32>(...);
var someOtherFrame = image.Frames.RootFrame.Clone();
image.Dispose();

// accepting root frame
var rootFromImage = image.GetRootFramePixelBuffer(); // throws ObjectDisposed as expected
var rootFromFrames = image.Frames.RootFrame; // returns null

// mutation still works
image.Frames.AddFrame(someOtherFrame);

// and this mutation can lead to image returning spans like it was never disposed
var span = image.GetPixelRowSpan(0);

Last line works kinda like expected because everything else is broken

*All above also works for base Image class

Steps to Reproduce

Use this code snippet for any image:

var image = Image.Load<ImageSharp.PixelFormats.Rgba32>("path");
var someOtherFrame = image.Frames.RootFrame.Clone();
image.Dispose();

var rootFromFrames = image.Frames.RootFrame;

image.Frames.AddFrame(someOtherFrame);
var span = image.GetPixelRowSpan(0);

[JimBobSquarePants]

Seems like a simple case of adding checks + throwhelpher method. Would you be able to raise a PR?

[br3aker]

Sure, will do it!

Non-related question: is it okay to open separate issue for some small internal api improvements or should I directly raise a PR?

[JimBobSquarePants]

Lovely thanks! Really appreciate it!😀

It depends on the scope of the changes normally. I think since you’re still learning the inner workings it’s best to raise an issue first until you’re more comfortable with the codebase.

[br3aker]

Will do another PR after this one then, thanks!

[antonfirsov]

We need disposal checks at many places to properly fix this. Probably the most expensive on is that goes to the image[x,y] and the frame[x,y] indexers, penalizing them further with yet another condition check.

I believe this is fine, since developers should use GetPixelRowSpan(y) for perf anyways. There's no way to make a feature-rich imaging & memory management backend to provide cheap access to individual [x,y] pixels.

@JimBobSquarePants I recalled this, because I remember we used to advertise our benchmark results on image[x,y] compared to systemDrawingBitmap.GetPixel(x,y) :)

[br3aker]

@antonfirsov I won't touch GetPixelRowSpan(y) simply because it will automatically fail with NullReferenceException due to FrameCollection being empty so PixelSource property would throw.

In fact, it is already checked inside [x, y] and GetPixelRowSpan while querying PixelSource:

private IPixelSource<TPixel> PixelSource => this.Frames?.RootFrame ?? throw new ObjectDisposedException(nameof(Image<TPixel>));

I will remove that check so it will fail only due to collection being empty which happens after disposal anyway,

[antonfirsov]

In fact, it is already checked inside [x, y] and GetPixelRowSpan while querying PixelSource:

Ah sure, it's still too much of morning time here for me, nevermind my comment then :)

I will remove that check so it will fail only due to collection being empty which happens after disposal anyway,

Don't do it! Let's make sure, that stuff keeps throwing ObjectDisposedException after disposal. There's no other legal way for this.Frames being empty.

[br3aker]

Actually just spotted another frame[x, y] possible performance gain: x/y indices are checked twice:

ImageSharp/src/ImageSharp/Image{TPixel}.cs

Lines 168 to 169 in 993f96e

	this.VerifyCoords(x, y);
	return this.PixelSource.PixelBuffer.GetElementUnsafe(x, y);

And during Buffer2D<T>[x, y] call. Will experiment with it and post benchmarks as part of upcoming PR.