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

Does glow::Context being Send+Sync mean it will be made current in the owning thread after being sent? #269

Open
alexispurslane opened this issue Dec 4, 2023 · 6 comments

Comments

@alexispurslane
Copy link

To my understanding, the only way for an OpenGL context to be sent/used on another thread than the one it was created on is for one of three platform-specific functions to be called on it once it was in the new thread to "make it current," and otherwise sending contexts between threads / using them on a different thread than the one they were created on is actually undefined behavior. So since glow marks the context as valid to send, does it do this call under the hood or anything, or at least have those platform specific calls abstracted out to a common method? I just wanted to ask because I'm considering switching to this over a raw gl-rs binding (for better cross-platform compatibility, a stripped down API, and more convenience, while maintaining almost 1:1 compatibility with OpenGL documentation and low level access) but with the gl-rs binding, if I build it myself I can make a non-send/sync gl context to prevent myself from possibly ending up in UB territory because I wouldn't be able to run OpenGL commands from another thread, because I couldn't send the context they're methods of, which is something I'd want from glow if I used it instead, or some way around the possible UB.

@alexispurslane alexispurslane changed the title glow::Context becoming Send+Sync mean it will be made current in the owning thread after being sent? Does glow::Context being Send+Sync mean it will be made current in the owning thread after being sent? Dec 4, 2023
@surban
Copy link
Contributor

surban commented Dec 4, 2023

To my knowledge: no, since glow is not concerned with OpenGL context creation and management.

All functions are marked as unsafe, thus it is the caller's responsibility to ensure that the desired OpenGL context is current on the calling thread.

@grovesNL
Copy link
Owner

grovesNL commented Dec 4, 2023

Right, glow isn't responsible for managing the context, so the caller has to handle making the context current. I think marking the context Send + Sync is still reasonable, it's just up to the caller to switch contexts.

For what it's worth, making the context current adds non-zero overhead so I wouldn't want glow to automatically try to call this, because it can't know if you're about to make a bunch of calls on the same thread. I also wouldn't want to remove Send + Sync because multi-threading is supported in general.

If you want to disable multi-threaded GL contexts in your own application (i.e., by removing Send + Sync), you could do this by creating a new type over the context that isn't Send + Sync, and forcing all GL calls to go through that.

@alexispurslane
Copy link
Author

alexispurslane commented Dec 4, 2023

marking the context Send + Sync is still reasonable, it's just up to the caller to switch contexts.

Oh it absolutely is, this wasn't meant to imply otherwise at all. No hate from me here 😅 I was just wondering (honestly it was a bit of a silly question now that I think about it in the morning, given what kind of wrapper glow is, sorry~)

@grovesNL
Copy link
Owner

grovesNL commented Dec 4, 2023

Oh no worries, I didn't interpret it that way at all!

Even though glow doesn't really do much with the context today, it might still be nice to have some kind of helper functions to make it easier to switch contexts and abstract over the platform differences, but I guess that might be a better fit for glutin or something.

@alexispurslane
Copy link
Author

I think just having a single make_context_current function that abstracts over the three actual functions (but isn't automatically called or anything!) would be really nice to have, and it seems like that'd be within the purview of glow, since it's supposed to prevent platform specific GL code?

@grovesNL
Copy link
Owner

grovesNL commented Dec 4, 2023

Yeah that sounds reasonable to me. We'd probably have to update our bindings generator (reviving #241) or temporarily include handwritten bindings for now.

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

3 participants