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

StoreImage can dispose of textures that are still on use on the GPU #1774

Open
PathogenDavid opened this issue May 3, 2024 · 1 comment
Open

Comments

@PathogenDavid
Copy link
Member

Found while investigating #1773

If a texture is updated only once by StoreImage, it will be disposed while the texture is still in use on the GPU.

Repro

To reproduce this issue:

  1. Download TextureOverrideIssue.zip and open the enclosed workflow
  2. Run the workflow, observe that a still from your webcam is rendered
  3. Stop the workflow
  4. Disable the StoreImageSequence and associated BindTexture
  5. Enable the StoreImage and associated BindTexture
  6. Run the workflow

Expected behavior

It is expected that the behavior is the same for both 2 and 6.

Actual behavior

For 6, either:

  • Bonsai will crash within the GPU driver
  • The render will briefly show the still, followed by a black screen

Proposed fix

I didn't dig super far, but this seems to happen because StoreImage blindly disposes of the texture when the observable ends, which it does in this case due to the use of Take on the input.

}).Finally(() =>
{
if (texture != null)
{
texture.Dispose();
}
});

The easiest fix is probably to add reference counting to textures, and have shaders add a reference to textures which are bound to them.

This is most easily done alongside #1773

Alternatively we could remove the explicit dispose and allow textures to be deleted by a finalizer. However, doing this would mean we have to ensure the glDeleteTextures call is properly marshaled to the main thread. It'd also obviously cause textures to survive longer than necessary.

If we go with the reference counting solution, it might be a good idea to still use a finalizer to help us discover leaked textures, even if the finalizer doesn't actually delete the texture due to previously mentioned marshaling junk.

@glopesdev
Copy link
Member

@PathogenDavid Precisely because of the issues with threading finalizer calls, the decision was made when we architected the package to have all OpenGL resources be explicitly disposed, without relying on any automatic mechanism, or reference counting.

The most common scenario used to deal with resources is to rely on the ResourceManager provided by the shader window. If resources are initialized via the resource manager, either at window initialization time or later using LoadResources, then the resource manager will take care of disposing all GL handles. This reflects also a common pattern in game design of loading as much resources as possible at initialization time, or during game level loading.

The case for dynamic textures is different since it doesn't go through the resource manager. I agree it probably needs to be revisited, but the idea was that operators creating them could be seen as implementing the factory pattern. Instances can be set to dispose at predictable times using ResourceSubject, but I agree this doesn't scale well if you really want to create large numbers of dynamic resources.

In the specific case of StoreImage, because the operator creates a single texture which can be dynamically updated, we thought of tying the lifetime of the texture to this update. However, I can see several patterns where this is unexpected and probably not a great idea to begin with.

Let's discuss this and see if we can figure out a way to make this work in practice for the 2.9 release. My bias is still probably to have destruction be explicit and predictable, based on how much trouble we have gotten in the past with both reference counting and threaded finalizers for this kind of stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants