Dispose device after using in ImageCropper.Helpers.cs

[0x7c13]

Dispose device after using in ImageCropper.Helpers.cs since CanvasDevice is disposble.

https://microsoft.github.io/Win2D/html/M_Microsoft_Graphics_Canvas_CanvasDevice_Dispose.htm

PR Type

What kind of change does this PR introduce?
Refactoring?

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

• Tested code with current supported SDKs
• Pull Request has been submitted to the documentation repository instructions. Link:
• Sample in sample app has been added / updated (for bug fixes / features)
  • Icon has been created (if new sample) following the Thumbnail Style Guide and templates
• Tests for the changes have been added (for bug fixes / features) (if applicable)
• Header has been added to all new source files (run build/UpdateHeaders.bat)
• Contains NO breaking changes

[ghost]

Thanks JasonStein for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

[michael-hawker]

@Kyaa-dost can you do a sanity check and just try out the Image Cropper in the Sample App with this PR applied? Thanks!

[Kyaa-dost]

@Jasonstein I have tried to build this multiple times in different machines and I keep receiving the same error. Can you please verify on your end and see if you can reproduce this?

[0x7c13]

@Kyaa-dost I don't think this error is related to this PR, are you seeing this error in Master branch as well? I can do a check later today. @michael-hawker thoughts?

[Kyaa-dost]

@Jasonstein It was only occurring for this PR earlier but now seems to be consistent with others as well. Please disregard the error. I will try to fix the error as I am receiving this in different machines.

[skendrot]

Per the documentation

It is not usually a good idea to call Dispose on a shared device, as this will break any other code that may be sharing it. Only dispose a shared device if you are certain that no other components are using it. After a shared device is disposed, subsequent calls to GetSharedDevice will replace it with a new device.

[skendrot]

We currently use the shared CanvasDevice in 5 different files. What happens when you place an Eyedropper and a RadialGradientBrush on the same page as the ImageCropper?

[michael-hawker]

@Kyaa-dost sorry, you're build issue isn't related to this PR. It's an SDK issue we hit with something introduced in another PR. We're fixing it to make this not a hassle for others at the moment in #3298. I'm going to look at that PR soon and get it merged.

Need to re-check based on other review comment

[michael-hawker]

@skendrot thanks for that catch Shawn! That's a good question, should be easy enough to drop them all on a page together in the sample app and test. @Jasonstein thoughts?

[0x7c13]

@skendrot thanks for that catch Shawn! That's a good question, should be easy enough to drop them all on a page together in the sample app and test. @Jasonstein thoughts?

Yes, also I think as soon as we are not using the shared device across different component at same time, we should be fine. But yes, if that's the case, we better just keep it and let GC do the work when necessary. We should do test anyway to verify first.

[Kyaa-dost]

@michael-hawker correct, based on the troubleshooting and the issue opened by Sergio, SDK19041 seems to do the job and using it as a workaround 🙂

[Sergio0694]

@Kyaa-dost Beware though that just using the 19041 SDK still doesn't solve all issues 😢
See microsoft/microsoft-ui-xaml#2545 for more on this.
Eg. currently I still can't run neither the Windows Community Toolkit Sample app nor my own app (Legere) with .NET Native - both build but then just crash as soon as they're opened.

[Kyaa-dost]

@michael-hawker @Jasonstein The test looks good and performing fine. We can now proceed with @skendrot idea mentioned above or discuss further. If anyone else wants to test it out please feel free to try the sample App.

[Sergio0694]

Regarding what @skendrot said from the docs, wouldn't this change potentially affect the PipelineBrush from the Uwp.UI.Media package? The shared device is used to load textures there:

https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/6b1d27c56c87a8a7945d4abcbe62405b6d9df0e5/Microsoft.Toolkit.Uwp.UI.Media/Helpers/SurfaceLoader.cs#L64-L65

Not sure we should really be using a using block there at all in this case 🤔
I'm doing the same in the SurfaceLoader class too: I get the shared device and then just treat it as a normal local variable without disposing it when it goes out of scope.

[michael-hawker]

@Sergio0694 yeah, I agree. I think the consensus is based on the docs, we just grab the Shared Device and let the GC handle things as needed.

@Jasonstein you good with us just closing this PR?

[0x7c13]

@Sergio0694 yeah, I agree. I think the consensus is based on the docs, we just grab the Shared Device and let the GC handle things as needed.

@Jasonstein you good with us just closing this PR?

Yes, please close it. I don't have any concerns.