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

[API Proposal] Making the RenderTargetBitmap inherit IDisposable #7177

Open
lindexi opened this issue Oct 10, 2022 · 5 comments
Open

[API Proposal] Making the RenderTargetBitmap inherit IDisposable #7177

lindexi opened this issue Oct 10, 2022 · 5 comments
Labels
API suggestion Early API idea and discussion, it is NOT ready for implementation
Milestone

Comments

@lindexi
Copy link
Contributor

lindexi commented Oct 10, 2022

The RenderTargetBitmap will cature the _wicSource handler which will release slowly by GC. WPF will throw COM Exception when create RenderTargetBitmap too fast.

Making the RenderTargetBitmap inherit IDisposable. Developers can better control the release of memory.

API Proposal

  1. Making the RenderTargetBitmap inherit IDisposable
public class RenderTargetBitmap : IDisposable
  1. Add the AsDisposable method
public class RenderTargetBitmap
{
    public IDisposable AsDisposable(){}
}

Add the AsDisposable method can make the Analyzer happy. The Analyzer will not keep alerting that the IDisposable object may be a potential memory leak.

See #3067 (comment)

@lindexi lindexi added Untriaged API suggestion Early API idea and discussion, it is NOT ready for implementation and removed Untriaged labels Oct 10, 2022
@lindexi
Copy link
Contributor Author

lindexi commented Oct 10, 2022

Or we should add the IDisposable to all BitmapSource ?

@amaitland
Copy link

In the context of CefSharp every time the WPF control is resized a new WritableBitmap is created and the old one discarded, a means to explicitly release the memory would be most welcomed. Semi frequently I get reports of OutOfMemoryException when attempting to create a WriteableBitmap.

@miloush
Copy link
Contributor

miloush commented Oct 10, 2022

Funnily I was looking into this couple of days back. BitmapSource itself has unmanaged references so it would make sense to make it IDisposable, and RTB and WB can chime in and dispose their extra references.

It needs to be said that introducing IDisposable is a breaking change, because IDisposable means "you should call dispose when you are done" and the existing code does not do that, with the worry being that unmanaged resources will leak. That is not exactly our case here, because the unmanaged resources already exist and are being taken care of during finalization. However, as @lindexi pointed out, analyzers do not know that. Another breaking change is that any classes that own BitmapSources as fields or properties should also implement IDisposable according to the guidance.

So we need to make a decision whether we are OK introducing such breaking change, or whether we want to work around that. Personally at the moment I would not be opposed too much to a breaking change here. For workarounds, I do not like the AsDisposable idea very much, because it adds extra layer of complexity and introduces a new pattern. We could have a Dispose or Close or similarly named method without actually implementing IDisposable.

There is also a 3rd option, which is to figure out why RTB and WB are running in these issues in the first place, for example, are they not adding enough GC pressure?

Finally, while this change looks easy in principle, there is couple of things that need to be figured out. Suddenly you can have disposed yet alive classes around and that has consequences. All the classes would need to be sprinkled with checks and ObjectDisposedExceptions. What happens to the cached bitmaps? What happens to bitmaps that are currently rendered on screen when they get disposed? Do we trigger content change on disposal? Do we need to worry about frozen clones? etc.

@JensNordenbro
Copy link

As an api consumer I see little problems in adding using in new places or even disable analyser warnings IF need be. As long as the changes arrive in a major version like net8!

@lindexi
Copy link
Contributor Author

lindexi commented Oct 31, 2022

As an api consumer I see little problems in adding using in new places or even disable analyser warnings IF need be.

I think so too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API suggestion Early API idea and discussion, it is NOT ready for implementation
Projects
None yet
Development

No branches or pull requests

5 participants