FIX(overlay_gl): modernise OpenGL overlay with core profile rendering#7138
Open
ChrisLane wants to merge 3 commits intomumble-voip:masterfrom
Open
FIX(overlay_gl): modernise OpenGL overlay with core profile rendering#7138ChrisLane wants to merge 3 commits intomumble-voip:masterfrom
ChrisLane wants to merge 3 commits intomumble-voip:masterfrom
Conversation
WalkthroughThis change modernizes the Mumble overlay system. It fixes a script output issue in the mumble-overlay launcher, updates OpenGL feature detection to require modern shader-based symbols instead of deprecated fixed-function calls, adds runtime GL function pointer resolution for UNIX platforms, and refactors the rendering pipeline to use VAO/VBO with GLSL 330 shaders instead of fixed-function vertex arrays. Socket and shared memory handles now use close-on-exec flags. The overlay state save/restore mechanism transitions from fixed-function push/pop operations to explicit state tracking. Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
overlay_gl/overlay.c (2)
893-895: Polygon mode restore may not handle asymmetric front/back modes.
glGetIntegerv(GL_POLYGON_MODE, ...)returns[front, back]polygon modes. The current restore only checkssavedPolygonMode[0](front face). If an application uses different modes for front and back faces (rare but valid), the back face mode won't be restored correctly.This is a minor edge case since asymmetric polygon modes are uncommon.
♻️ More complete restore
- if (savedPolygonMode[0] != GL_FILL) { - glPolygonMode(GL_FRONT_AND_BACK, (GLenum) savedPolygonMode[0]); - } + if (savedPolygonMode[0] != GL_FILL || savedPolygonMode[1] != GL_FILL) { + if (savedPolygonMode[0] == savedPolygonMode[1]) { + glPolygonMode(GL_FRONT_AND_BACK, (GLenum) savedPolygonMode[0]); + } else { + glPolygonMode(GL_FRONT, (GLenum) savedPolygonMode[0]); + glPolygonMode(GL_BACK, (GLenum) savedPolygonMode[1]); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@overlay_gl/overlay.c` around lines 893 - 895, Restore both front and back polygon modes instead of only checking savedPolygonMode[0]; read/remember the two-element savedPolygonMode (from GL_POLYGON_MODE) and when restoring call glPolygonMode(GL_FRONT, savedPolygonMode[0]) and glPolygonMode(GL_BACK, savedPolygonMode[1]) (or glPolygonMode(GL_FRONT_AND_BACK, ...) only when both elements equal) so asymmetric modes are preserved; update the restore logic around savedPolygonMode and glPolygonMode accordingly.
272-278: Consider expanding the failure check to cover more critical functions.The failure check validates only a subset of resolved functions. Several functions used in rendering paths are not verified:
glTexImage2D,glTexSubImage2D(texture uploads)glTexParameteri,glGetTexParameterfv(texture state)glGetUniformLocation,glUniform1i,glUniformMatrix4fv(shader uniforms)glGetShaderiv,glGetProgramiv(shader compilation checks)While shader compilation failures are handled gracefully in
newContext, texture operations inregenTextureanddrawOverlaywould crash if these pointers are NULL.♻️ Suggested addition to failure check
if (!oglBindBuffer || !oglBindTexture || !oglBindVertexArray || !oglUseProgram || !oglDrawArrays || !oglEnable || !oglDisable || !oglViewport || !oglGetIntegerv || !oglGetError || !oglGenBuffers || !oglGenTextures || !oglGenVertexArrays || !oglCreateProgram || !oglCreateShader || !oglCompileShader || !oglLinkProgram - || !oglAttachShader || !oglBlendEquation) { + || !oglAttachShader || !oglBlendEquation || !oglTexImage2D || !oglTexParameteri || !oglGetUniformLocation + || !oglUniformMatrix4fv || !oglUniform1i || !oglBufferSubData) { resolveFailed = true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@overlay_gl/overlay.c` around lines 272 - 278, The current resolve-failure check only verifies a subset of OpenGL function pointers and misses several functions used by texture uploads and uniform/shader checks, which can cause crashes in regenTexture and drawOverlay; update the failure check (the block that sets resolveFailed) to also include glTexImage2D, glTexSubImage2D, glTexParameteri, glGetTexParameterfv, glGetUniformLocation, glUniform1i, glUniformMatrix4fv, glGetShaderiv, and glGetProgramiv (the same place where oglBindBuffer/... are verified) so resolveFailed is set if any of these are NULL, and ensure code paths in regenTexture, drawOverlay, and newContext assume resolveFailed prevents usage when any pointer is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@overlay_gl/overlay.c`:
- Around line 418-429: The shader program is leaked when glLinkProgram fails
because ctx->uiProgram isn't deleted; add FDEF(glDeleteProgram) to the
function-pointer/FDEF list (alongside existing FDEFs like
glDeleteShader/glDeleteTextures) so the resolver exposes glDeleteProgram, and in
the link-failure path (after glGetProgramiv detects !status and before
returning) call glDeleteProgram(ctx->uiProgram) to delete the orphaned program,
then proceed to log the info and return.
---
Nitpick comments:
In `@overlay_gl/overlay.c`:
- Around line 893-895: Restore both front and back polygon modes instead of only
checking savedPolygonMode[0]; read/remember the two-element savedPolygonMode
(from GL_POLYGON_MODE) and when restoring call glPolygonMode(GL_FRONT,
savedPolygonMode[0]) and glPolygonMode(GL_BACK, savedPolygonMode[1]) (or
glPolygonMode(GL_FRONT_AND_BACK, ...) only when both elements equal) so
asymmetric modes are preserved; update the restore logic around savedPolygonMode
and glPolygonMode accordingly.
- Around line 272-278: The current resolve-failure check only verifies a subset
of OpenGL function pointers and misses several functions used by texture uploads
and uniform/shader checks, which can cause crashes in regenTexture and
drawOverlay; update the failure check (the block that sets resolveFailed) to
also include glTexImage2D, glTexSubImage2D, glTexParameteri,
glGetTexParameterfv, glGetUniformLocation, glUniform1i, glUniformMatrix4fv,
glGetShaderiv, and glGetProgramiv (the same place where oglBindBuffer/... are
verified) so resolveFailed is set if any of these are NULL, and ensure code
paths in regenTexture, drawOverlay, and newContext assume resolveFailed prevents
usage when any pointer is missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ff90832-845a-460f-8918-94b63b510e34
📒 Files selected for processing (4)
auxiliary_files/run_scripts/mumble-overlay.inoverlay_gl/avail_mac.hoverlay_gl/init_unix.coverlay_gl/overlay.c
ChrisLane
force-pushed
the
fix-opengl-overlay
branch
from
4e403c7 to
a814fda
Compare
ChrisLane
force-pushed
the
fix-opengl-overlay
branch
from
a814fda to
de4d364
Compare
Replace deprecated fixed-function pipeline calls (matrix stack, attribute push/pop, client state, glOrtho, glColorMaterial, glTexEnvi, glRenderMode, etc.) with modern OpenGL 3.3 core profile equivalents: explicit VAO/VBO, uniform MVP matrix, GLSL 3.30 shaders with layout attributes, and manual state save/restore. Resolve all GL function pointers at runtime via glXGetProcAddress instead of relying on compile-time symbol availability. The overlay now uses FDEF/GLRESOLVE macros and #define redirects so no GL symbols need to be exported by the driver at link time. This fixes "undefined symbol: glPopClientAttrib" (and similar) crashes on systems where Mesa only provides a core profile context (e.g. Mesa 26 on Arch Linux). Also fixes the find_odlsym() ELF dynamic-section loop to terminate on DT_NULL instead of a null pointer check, which could walk past the end of the section and cause "Unable to find symbol malloc" aborts. Other minor improvements: SOCK_CLOEXEC/O_CLOEXEC to avoid leaking file descriptors to child processes, NULL-check on malloc for blit buffer. Fixes mumble-voip#5932 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove leftover debug "echo set" from the 64-bit library path detection in the mumble-overlay launch script.
ChrisLane
force-pushed
the
fix-opengl-overlay
branch
from
de4d364 to
07effa2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Notes for Reviewers
This was built with Claude Code (Opus 4.6). OpenGL/C is not my area of expertise. I drove the design and validated behaviour through testing, but the entire implementation needs review by someone comfortable with OpenGL C code. I don't have the background to judge whether the OpenGL usage is idiomatic or just functional. Apologies if this is low quality but from my inexperienced eyes the changes seem reasonable and are at least functional.
Summary
Fixes #5932
Rewrites the OpenGL overlay renderer to use OpenGL 3.3 core profile, fixing crashes on modern Mesa drivers (e.g. Mesa 26 on Arch Linux) that no longer export legacy/compatibility GL symbols such as
glPopClientAttrib.The problem
The overlay library is injected via
LD_PRELOADand was compiled withGL_GLEXT_PROTOTYPES, causing it to link directly against GL function symbols likeglPopClientAttrib,glPushAttrib,glMatrixMode,glEnableClientState, and other fixed-function pipeline functions. Modern Mesa builds only provide an OpenGL core profile by default, which removes these deprecated symbols entirely. When the dynamic linker tried to resolve them at load time, it failed withundefined symbol: glPopClientAttrib(or similar), preventing the overlay from starting.A secondary crash ("Unable to find symbol malloc") was caused by the
find_odlsym()ELF-walking loop usingdynas its termination condition instead of checkingdyn->d_tag != DT_NULL, which could walk past the end of the dynamic section on some systems.What changed
glOrtho,glColorMaterial,glTexEnvi,glRenderMode, etc.) with core profile equivalents: explicit VAO/VBO, uniform MVP matrix, and manual state save/restorelayoutattributes anduniform mat4 mvpglXGetProcAddressinstead of relying on compile-time symbol availability - the overlay now usesFDEF/GLRESOLVEmacros and#defineredirects so no GL symbols need to be exported by the driver at link time#define GL_GLEXT_PROTOTYPESwhich was the root cause of the link-time symbol dependencyfind_odlsym()ELF dynamic-section loop to terminate onDT_NULLinstead of a null pointer checkechofrom themumble-overlayshell scriptSOCK_CLOEXEC/O_CLOEXECto avoid leaking file descriptors to child processesTesting
glxgearsglxgearswith and without the overlayChecks