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

Fix issue with external textures being freed by Godot #56148

Merged

Conversation

BastiaanOlij
Copy link
Contributor

This fix applies to Godot 3.x only.

I'm not sure what changed or when but this issue was around for awhile. I think this originally worked because the allocation size was checked as part of the cleanup.

Anyway, the situation here is when an XR Interface supplies the textures into which a viewport is rendered instead of us using Godots own textures. A texture object is created that wraps around the supplier OpenGL texture id so we can do stuff like using the texture to render out a preview.

During cleanup of a render target this texture object is destroyed and it destroys it's OpenGL texture alongside it. In this case it shouldn't because this texture was never created by Godot, it was created by the XR interface. If the render target is cleaned up because settings on the viewport change that require re-creating it with different settings, the result is flashing inside of the headset as one of the textures in the texture chain has gone byebye.

The fix here is really simple, in the two places in each driver where this texture is destroyed, we set the tex_id value to zero preventing it being destroyed by Godot (it will be cleaned up by the plugin when required).

@lawnjelly
Copy link
Member

lawnjelly commented Dec 22, 2021

Are these the only external textures we use?

I was just wondering whether an alternative more general method might be to add something like an "externally_owned" flag to the Texture, and if this is set, don't delete on exit. That way you could set the flag on creation and forget about it.

(That's not to say it would be better, just wondering what the pros and cons are of either approach, just before we commit to this one). 🙂

@akien-mga akien-mga merged commit a75afd6 into godotengine:3.x Dec 22, 2021
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

@lawnjelly Sorry I merged before reading your comment but it seems that external textures are indeed only used here:

$ rg "rt->external.texture"
drivers/gles3/rasterizer_storage_gles3.cpp
6852:           Texture *t = texture_owner.get(rt->external.texture);
6858:           texture_owner.free(rt->external.texture);
7329:           return rt->external.texture;
7361:                   Texture *t = texture_owner.get(rt->external.texture);
7367:                   texture_owner.free(rt->external.texture);
7406:                   rt->external.texture = texture_owner.make_rid(t);
7412:                   t = texture_owner.get(rt->external.texture);

drivers/gles2/rasterizer_storage_gles2.cpp
5266:           Texture *t = texture_owner.get(rt->external.texture);
5272:           texture_owner.free(rt->external.texture);
5400:           return rt->external.texture;
5430:                   Texture *t = texture_owner.get(rt->external.texture);
5436:                   texture_owner.free(rt->external.texture);
5475:                   rt->external.texture = texture_owner.make_rid(t);
5482:                   t = texture_owner.get(rt->external.texture);

That doesn't mean that this can't be refactored of course.

@akien-mga
Copy link
Member

Cherry-picked for 3.4.3.

@BastiaanOlij BastiaanOlij deleted the fix_deleting_unowned_texture branch June 23, 2022 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants