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

Static loader is unsafe #185

Closed
emberian opened this issue Oct 7, 2014 · 11 comments · Fixed by #207
Closed

Static loader is unsafe #185

emberian opened this issue Oct 7, 2014 · 11 comments · Fixed by #207

Comments

@emberian
Copy link
Collaborator

emberian commented Oct 7, 2014

Scenario:

Thread 1              |  Thread 2
========              |  ========
driver1::make_current |
gl::load_with         |
                      |  driver2::make_current
                      |  gl::Begin() // crash!

In general, loading GL functions can be thread-specific, and only the struct loader can deal with this. Every function in the static loader should be marked unsafe, and it should be heavily discouraged.

@tomaka
Copy link
Collaborator

tomaka commented Oct 7, 2014

The struct loader is unsafe too, even if we mark it as NoSend.
You can make a context current, call Gl::load_with, then remove the current context, and all your calls will crash.

@ghost
Copy link

ghost commented Oct 7, 2014

If you couple gl-rs with a library like glfw or glutin we can potentially build a completely safe api.

// this moves the window context to be part of gl
let gl = ctx.make_current();

gl.drawElements(...);
// ect

// this unwraps the gl moving the context back into ctx
let ctx = gl.to_context();

We could supply a trait to do this conversion for you, but it requires the windowing library to work with it.

@tomaka
Copy link
Collaborator

tomaka commented Oct 7, 2014

No, because if you create multiple windows you are still unsafe.

@ghost
Copy link

ghost commented Oct 7, 2014

@tomaka the idea is that the opengl functions are queried each time you make_current.

@tomaka
Copy link
Collaborator

tomaka commented Oct 7, 2014

And how would you know that the user doesn't manually call wglMakeCurrent?
If he does, it's your code that ends up crashing. Hence the unsafe.

@brendanzab
Copy link
Owner

@csherratt Doesn't calling make_current over and over slow things down a great deal on Windows?

@brendanzab
Copy link
Owner

I am leaning towards making all functions in the static loader unsafe.

@emberian
Copy link
Collaborator Author

I think make_current should always be unsafe.

@brendanzab
Copy link
Owner

Agreed.

@brendanzab
Copy link
Owner

So the load_with function is safe, all gl functions are unsafe, and make_current is unsafe too?

@tomaka
Copy link
Collaborator

tomaka commented Oct 12, 2014

If we define unsafe as the safetiness of this function depends on factors not controllable by the function itself (in other words, the safetiness depends on the environment), then:

  • all GL functions are unsafe because they are safe only if there is a valid context
  • similarly get_proc_addr is unsafe because it requires a valid context
  • make_current is safe, because the context passed to it is a Rust object and is always valid

Alternatively you could define unsafe as calling an unsafe function can lead to unexpected or unsafe behaviors in functions marked as safe (by an unsafe function, I mean make_current here), which is probably what csherratt had in mind.

emberian added a commit to emberian/gl-rs that referenced this issue Nov 5, 2014
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

Successfully merging a pull request may close this issue.

3 participants