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

Use glow instead of gl_generator for GL/GLES bindings #321

Merged
merged 1 commit into from
Jan 14, 2025

Conversation

sagudev
Copy link
Member

@sagudev sagudev commented Oct 17, 2024

See servo/servo#33539.

Note that this PR will expose glow types in surfmans public API.

@mrobinson mrobinson changed the title use glow instead of gl_generator for GL/GLES bindings Use glow instead of gl_generator for GL/GLES bindings Nov 20, 2024
@mrobinson mrobinson changed the title Use glow instead of gl_generator for GL/GLES bindings Use glow instead of gl_generator for GL/GLES bindings Nov 20, 2024
@sagudev sagudev force-pushed the no_gl_gen branch 3 times, most recently from dce071e to 3ae3cf7 Compare November 20, 2024 17:14
@sagudev
Copy link
Member Author

sagudev commented Nov 29, 2024

So the problem is that:

thread_local! {
    pub static GL_FUNCTIONS: Gl = unsafe {Gl::from_loader_function(context::get_proc_address)};
}

does not work because glow requires for context to be current, because it does some querying (version, extension). My idea is to simply replace all usages of GL_FUNCTIONS with let gl = unsafe {Gl::from_loader_function(context::get_proc_address)}; as most of the time we are already current in the calls. This would make querying every time, but functions are not hot AFAIK, so this should be ok. Alternative would be to have Gl stored on context, but this needs to be lazy value, as context creation does not make context current.

@sagudev
Copy link
Member Author

sagudev commented Jan 4, 2025

I am wondering do we really need newly-created contexts that not immediately made current, or can we just made them current on creation? Linked issue on test is about something different. This was introduced in dae72e4. cc @pcwalton @jdm

@jdm
Copy link
Member

jdm commented Jan 5, 2025

Original issue about inconsistent behavior: pcwalton#7

@jdm
Copy link
Member

jdm commented Jan 5, 2025

I believe we could change the behavior to always make new contexts current.

@sagudev sagudev force-pushed the no_gl_gen branch 6 times, most recently from 3d48909 to df06a2e Compare January 6, 2025 09:48
@sagudev
Copy link
Member Author

sagudev commented Jan 6, 2025

This is very close now, macos fails due to not clearing gl error before running create surface. Needs some cleanup (warnings) and updated servo companion PR.

@jdm
Copy link
Member

jdm commented Jan 14, 2025

If you rebase this branch on top of #326 then the crash in servo/servo#34328 will go away \o/

@mrobinson
Copy link
Member

Is this ready to be reviewed now?

@sagudev sagudev marked this pull request as ready for review January 14, 2025 09:45
@sagudev
Copy link
Member Author

sagudev commented Jan 14, 2025

Is this ready to be reviewed now?

Yes

Copy link
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

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

Great stuff. I just have a few comments. In addition to what's below, please squash your changes into a single commit. Thank you!

.gitignore Outdated Show resolved Hide resolved
examples/threads.rs Outdated Show resolved Hide resolved
examples/threads.rs Outdated Show resolved Hide resolved
examples/offscreen.rs Outdated Show resolved Hide resolved
src/info.rs Show resolved Hide resolved
src/platform/egl/surface/android_surface.rs Show resolved Hide resolved
src/platform/generic/egl/surface.rs Outdated Show resolved Hide resolved
src/platform/macos/cgl/context.rs Show resolved Hide resolved
src/platform/macos/cgl/context.rs Outdated Show resolved Hide resolved
@sagudev
Copy link
Member Author

sagudev commented Jan 14, 2025

Great stuff. I just have a few comments. In addition to what's below, please squash your changes into a single commit. Thank you!

I will squash them before adding to queue.

@sagudev sagudev requested a review from mrobinson January 14, 2025 11:11
@sagudev sagudev enabled auto-merge January 14, 2025 11:27
@sagudev sagudev added this pull request to the merge queue Jan 14, 2025
Merged via the queue into servo:main with commit e78ea1b Jan 14, 2025
29 checks passed
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 this pull request may close these issues.

3 participants