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

Make RID_Owner<Texture> threadsafe in TextureStorage for GLES3 #88205

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

0x0ACB
Copy link
Contributor

@0x0ACB 0x0ACB commented Feb 11, 2024

This seems to fix #88195. In the Vulkan driver the RID_Owner<Texture> is already thread safe and indeed, textures might be created from threads. Also the RID_Owner<CanvasTexture> for gles3 is thread safe as well. I think this was just forgotten to add for the Texture one in a refactor.

@0x0ACB 0x0ACB requested a review from a team as a code owner February 11, 2024 12:14
@AThousandShips AThousandShips added bug topic:rendering crash cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Feb 11, 2024
@AThousandShips AThousandShips added this to the 4.3 milestone Feb 11, 2024
@akien-mga akien-mga changed the title Make RID_Owner<Texture> threadsafe in TextureStorage for gles3 Make RID_Owner<Texture> threadsafe in TextureStorage for GLES3 Feb 11, 2024
@0x0ACB 0x0ACB force-pushed the thread_safe_texture_rid branch from b2760e6 to 89c5609 Compare February 11, 2024 16:21
@0x0ACB 0x0ACB force-pushed the thread_safe_texture_rid branch from 89c5609 to 09d2c09 Compare February 11, 2024 16:23
@RandomShaper
Copy link
Member

I'm not sure you can create textures from threads in the OpenGL ES backend. Also, there's some ongoing work (#77004) that may need to be aware of this change to potentially revert it if it turns out in the end to be possible to funnel all the renderer calls via a unique thread in the case of GL.

@0x0ACB
Copy link
Contributor Author

0x0ACB commented Feb 12, 2024

It is (currently) possible to create textures from threads, atleast that is what I found while debugging my issue. Or atleast you can create the RID from a thread.

@RandomShaper
Copy link
Member

Oh, right. Allocating a RID would need sync indeed.

PS: I'm wondering if RID_Owner could be enhanced with a locking pattern that makes that more optimal, given most of the time it will be read-written from the renderer thread. I'll add this to my TODO list.

@AThousandShips
Copy link
Member

Like:

Perhaps?

Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense, but I don't have time to repro and debug, and I'm not very familiar with the GL implementation. I'm approving, but TIWAGOS.

@RandomShaper
Copy link
Member

Like:

Perhaps?

Not quite... The requirements here would be: 1) any thread can allocate uninitialized RIDs, 2) only the "owner" thread can do everything else. So, both usages involve writing, but with different constraints.

@akien-mga akien-mga merged commit 5e58bcd into godotengine:master Feb 13, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.

@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 10, 2024
@Nicholas3413
Copy link

Please also cherry pick this for Godot 4.1.5 next. thankyou!

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.

RID_Alloc::get_or_null randomly fails leading to crashes down the line in RID_Alloc::owns
5 participants