Repository navigation
Conversation
github-actions
Bot
added
engine
jtmcdole
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Linux platform OpenGL manager to explicitly clear the GDK GL context before making the platform context current, preventing EGL bad access errors under libglvnd. It also ensures OpenGL contexts are cleared after setup and disposal, and adds a corresponding unit test. Review feedback suggests checking the return value of fl_opengl_manager_make_platform_current to prevent undefined behavior, using a local g_autoptr in the test to avoid potential memory leaks, and maintaining context symmetry by clearing the OpenGL context in the caller of setup_shader rather than inside the function.
92d4ca7 to
2f8ce20
Compare
jtmcdole
changed the title
|
Turns out, stacked prs are not going to work since Cocoon has no clue what's going on. |
2f8ce20 to
6220144
Compare
6220144 to
a0941d5
Compare
Under `libglvnd`, calling `eglMakeCurrent` on the GTK main thread while a GLX context (`GdkGLContext`) is active fails with `EGL_BAD_ACCESS` ("Another window API already has a current context"). Because EGL does not unbind foreign APIs, the GLX context must be explicitly unbound first via `gdk_gl_context_clear_current()` so that an `X_GLXMakeContextCurrent(None)` request is dispatched to the X server, preventing client-server state desynchronization.
Use `fl_opengl_manager_make_platform_current()` instead of `fl_opengl_manager_make_current()` in `fl_compositor_opengl_new()` and clear the EGL context via `fl_opengl_manager_clear_current()` after compositor/shader setup and disposal.
a0941d5 to
8a756b2
Compare
|
I'm going to have to land this one PR at a time since ci.yaml doesn't support stacked branches. Good feature request I guess..... |
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
Are there integration tests for this too? This sort of thing makes me a bit nervous since I'm not as familiar with the testing. LGTM otherwise.
| fl_opengl_manager_make_current(opengl_manager); | ||
| self->can_blit = fl_opengl_manager_can_blit(opengl_manager); | ||
| self->can_fence = fl_opengl_manager_can_fence(opengl_manager); | ||
| if (fl_opengl_manager_make_platform_current(opengl_manager)) { |
There was a problem hiding this comment.
Thinking about this suggests this code shouldn't be here and instead it should lazy-load these when required. But this is existing behaviour so doesn't have to be part of this PR.
| // explicitly unbound first via gdk_gl_context_clear_current() so that an | ||
| // X_GLXMakeContextCurrent(None) request is dispatched to the X server, | ||
| // preventing client-server state desynchronization. | ||
| gdk_gl_context_clear_current(); |
There was a problem hiding this comment.
This has a bad code smell - the opengl manager shouldn't need to know anything about GDK. This should have been called somewhere else.
There was a problem hiding this comment.
I've pushed a change that I think fixes this.
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If both you and the PR author believe this PR qualifies for a test exemption, escalate to the tech lead for the changed area to review and apply the |
robert-ancell
left a comment
robert-ancell
left a comment
There was a problem hiding this comment.
Got to love context headaches.
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
Fixes a
Gdk-ERROR: BadAccess (GLX minor_code 26)crash when realizing secondaryFlViews on X11.Under
libglvnd, callingeglMakeCurrenton the GTK main thread while a GLX context (GdkGLContext) is current fails withEGL_BAD_ACCESS("Another window API already has a current context"). Because EGL does not unbind foreign APIs, the GLX context must be explicitly unbound first viagdk_gl_context_clear_current()so that anX_GLXMakeContextCurrent(None)request is dispatched to the X server, preventing client-server state desynchronization.gdk_gl_context_clear_current()infl_opengl_manager_make_platform_current()beforeeglMakeCurrent().fl_opengl_manager_make_platform_current()infl_compositor_opengl_new()and callfl_opengl_manager_clear_current()after compositor/shader setup and disposal.FlCompositorOpenGLTest.ClearsGdkGLContextBeforePlatformContext.Part of stack #193534.