-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat(gles): Add GlDebugFns option to disable OpenGL debug functions
#8931
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
base: trunk
Are you sure you want to change the base?
feat(gles): Add GlDebugFns option to disable OpenGL debug functions
#8931
Conversation
Adds a new option to allow disabling OpenGL debug functions (glPushDebugGroup, glPopDebugGroup, glObjectLabel, etc.) which can crash on some Mali GPUs.
enable_debug_fns option to GlBackendOptionsenable_debug_fns option to GlBackendOptions
Replace the boolean `enable_debug_fns` field with a `debug_fns` field using the new `GlDebugFns` enum with `Auto` and `Disabled` variants. This follows the same pattern as other GL backend options. Environment variable changed from `WGPU_GL_DEBUG_FNS=true|false` to `WGPU_GL_DEBUG_FNS=auto|disabled`.
enable_debug_fns option to GlBackendOptionsGlDebugFns option to disable OpenGL debug functions
GlDebugFns option to disable OpenGL debug functionsGlDebugFns option to disable OpenGL debug functions
|
@Xavientois THanks for this PR, and thanks for being upfront about the AI usage for writing this description! I don't think we have our policy in words yet, but we basically require that for any PR or issue that you file, you must be able to defend and understand everything that is written. The general policy is that 2 people must understand well every PR: the author and the reviewer. Also, we don't really want to read through whatever an LLM wrote, especially given how common hallucinations are, or how it might drag out the word count. The PR itself looks relatively simple but I'd just like to warn you that there's basically no chance in my mind that it gets merged as-is. Maintainers will discuss this on Wednesday and get back to you. Nothing I've said here is a final decision. Thanks again for being clear about this being AI written. |
|
@inner-daemons Thanks for the quick response! I appreciate you being candid about not wanting to the LLM-generated piece. If the primary blocker is the LLM-generated content, I'd be happy to rewrite it myself. I do fully understand the change, as this is a fix that we have been using in our internal WGPU fork for over a year now, so my goal is just to include it in the upstream. Do you think it would be worthwhile for me to rewrite it myself? |
|
@Xavientois The point of contention here is mainly the description being AI generated. Code can be judged purely on its quality and is trustworthy and self explanatory. If a reviewer approves of code it doesn't particularly matter what wrote it, except that the human responsible for filing the PR understands it. But having an AI generate a PR description comes off as little bit lazy and disrespectful in all honesty, and it means there will be too many words, many of which are not necessarily true. I'd also want you to file an issue before submitting a PR like this. The issue template gives a lot of helpful information to maintainers. There might be better ways to solve these problems, and we want to know on what device the issue is occuring, what the logs are, and what alternatives you've considered. Plus, just having the bug be documented is nice. |
Summary
Adds a new
debug_fnsfield toGlBackendOptionsandGlDebugFnsenum that allows disabling OpenGL debug functions (glPushDebugGroup,glPopDebugGroup,glObjectLabel, etc.).Problem
On some Mali GPUs on Samsung Galaxy devices, we observed the following crash:
Since we do not use the debug functions in our project, we want to be able to disable them outright.
Solution
GlDebugFnsenum withAuto(default) andDisabledvariantsdebug_fns: GlDebugFnsfield toGlBackendOptionsWGPU_GL_DEBUG_FNS=auto|disabledDEBUG_FNScapability-setting code to respect this optionUsage
Via API:
Via environment variable:
Test
This PR description was written by AI.