[wimp] MSAA support#183807
Conversation
* Use MSAA for default framebuffer by using the antialias flag when creating our WebGL context * Cache and reuse the MSAA resolve attachment for intermediate targets. * Use `glInvalidateFramebuffer` when it's available. We use the `glDiscardFramebufferEXT` when it's available, but that isn't available on WebGL, so we want to use `glInvalidateFramebuffer` in that case.
github-actions
Bot
added
engine
![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 enhances Impeller's GLES backend by introducing support for glInvalidateFramebuffer, prioritizing it over glDiscardFramebufferEXT for more efficient framebuffer invalidation. It also optimizes multisample resolve operations by caching and reusing Framebuffer Objects (FBOs) to reduce overhead. Additionally, the changes enable dynamic control over antialiasing for GL contexts in Skwasm based on the skwasm_isWimp() flag. A minor improvement opportunity exists in mock_gles.cc where the logic for building g_extensions_string is duplicated across MockGLES::Init overloads, suggesting extraction into a helper function for better maintainability.
| g_extensions_string.clear(); | ||
| for (const auto& ext : g_extensions) { | ||
| if (!g_extensions_string.empty()) | ||
| g_extensions_string += " "; | ||
| g_extensions_string += ext; | ||
| } |
There was a problem hiding this comment.
The logic to build g_extensions_string is duplicated across the two MockGLES::Init overloads (here and in the overload starting at line 355). To improve maintainability, you could extract this logic into a static helper function and call it from both Init methods.
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
Looks good to me! I just have one request to remove debug information from release builds since I worry about it incurring an extra cpu/gpu sync.
| static std::vector<const char*> g_extensions; | ||
|
|
||
| static const char* g_version; | ||
| static std::string g_extensions_string; |
There was a problem hiding this comment.
no action required: technically static functions that are not trivially destructible are forbidden. (https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables)
Since this is test code and it already does it elsewhere this is fine. This is just an fyi.
There was a problem hiding this comment.
Ah, okay. Good to know.
| return false; | ||
| } | ||
|
|
||
| auto status = gl.CheckFramebufferStatusDebug(GL_FRAMEBUFFER); |
There was a problem hiding this comment.
We probably don't want to call this in release builds, no?
There was a problem hiding this comment.
It actually is a no-op in release builds thanks to Evan's work a while ago. See the definition here: https://github.com/flutter/flutter/blob/master/engine/src/flutter/impeller/renderer/backend/gles/proc_table_gles.cc#L451
eyebrowsoffire
added
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
* Use MSAA for default framebuffer by using the antialias flag when creating our WebGL context * Cache and reuse the MSAA resolve attachment for intermediate targets. * Use `glInvalidateFramebuffer` when it's available. We use the `glDiscardFramebufferEXT` when it's available, but that isn't available on WebGL, so we want to use `glInvalidateFramebuffer` in that case.
* Use MSAA for default framebuffer by using the antialias flag when creating our WebGL context * Cache and reuse the MSAA resolve attachment for intermediate targets. * Use `glInvalidateFramebuffer` when it's available. We use the `glDiscardFramebufferEXT` when it's available, but that isn't available on WebGL, so we want to use `glInvalidateFramebuffer` in that case.
2 participants
glInvalidateFramebufferwhen it's available. We use theglDiscardFramebufferEXTwhen it's available, but that isn't available on WebGL, so we want to useglInvalidateFramebufferin that case.