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

Dispose source image object in image transfomers. #3328

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@codemzs
Copy link
Member

codemzs commented Apr 13, 2019

fixes #3327

@@ -218,6 +218,9 @@ protected override Delegate MakeGetter(DataViewRow input, int iinfo, Func<int, b
g.DrawImage(src, srcRectangle, 0, 0, src.Width, src.Height, GraphicsUnit.Pixel, attributes);
}
Contracts.Assert(dst.Width == src.Width && dst.Height == src.Height);

src.Dispose();

This comment has been minimized.

Copy link
@eerhardt

eerhardt Apr 13, 2019

Member

Is Disposing it the correct thing to do here? Are you given ownership of the src object? If so, then it is fine. But if getSrc caches that object and tries using it, people will get an ObjectDisposedException if they try to use it.

Also - there is a case on line 210 where this method could return early without disposing src.

This comment has been minimized.

Copy link
@codemzs

codemzs Apr 13, 2019

Author Member

I’m aware about line 210 but that is an error condition and not sure bitmap object has the ownership of the file but I’ll check. I did also consider the caching issue and didn’t see it being cached anywhere. None of the image transforms seem to cache the bitmap object.

This comment has been minimized.

Copy link
@wschin

wschin Apr 15, 2019

Member

ML.NET pipeline runs in a read-only mode. Disposing something means read-and-write.

This comment has been minimized.

Copy link
@codemzs

codemzs Apr 15, 2019

Author Member

You need to dispose otherwise it is a bug. Not entirely sure what your argument here is but we need a solution.

This comment has been minimized.

Copy link
@TomFinley

TomFinley Apr 15, 2019

Contributor

Sure. We need a solution. My question is more, why the existing solution and infrastructure is insufficient, since in principle the src should be disposed by the getter, then finally disposed when the row cursor is itself disposed. I am wondering what has broken down. #3327 is not sufficient.


In reply to: 275461003 [](ancestors = 275461003)

@@ -218,6 +218,9 @@ protected override Delegate MakeGetter(DataViewRow input, int iinfo, Func<int, b
g.DrawImage(src, srcRectangle, 0, 0, src.Width, src.Height, GraphicsUnit.Pixel, attributes);
}
Contracts.Assert(dst.Width == src.Width && dst.Height == src.Height);

src.Dispose();
src = null;

This comment has been minimized.

Copy link
@TomFinley

TomFinley Apr 15, 2019

Contributor

So, we see the correct pattern above at line 205, where dst is disposed if non-null. So the getter should in principle be disposing. Is it not? If it is not it should be.

So this seems to be disposing potentially in both places, but this makes me think we just have a bad getter delegate somewhere.

This comment has been minimized.

Copy link
@codemzs

codemzs Apr 15, 2019

Author Member

That is dst, this is src.

This comment has been minimized.

Copy link
@TomFinley

TomFinley Apr 15, 2019

Contributor

The point is that 205 is showing the correct pattern, and we also have the dispose method. If this pattern is being adhered to everywhere, the dispose method is being composed correctly, and the cursor is being disposed, src should be being disposed already. Investigate why this is not so.


In reply to: 275461358 [](ancestors = 275461358)

@TomFinley

This comment has been minimized.

Copy link
Contributor

TomFinley commented Apr 15, 2019

                    getSrc(ref src);

So to be clear, I'd expect this getter (whatever it is!) to dispose of the thing if necessary. I'd also expect the disposer above to handle any final cleanup on the DataViewRow object that is created using this infrastructure.

Is there some reason this does not work?


Refers to: src/Microsoft.ML.ImageAnalytics/ImageGrayscale.cs:208 in 4bd3d1b. [](commit_id = 4bd3d1b, deletion_comment = False)

@codemzs codemzs closed this Apr 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.