Refactor: Views should share shaders#185335
Conversation
Refactor: Views should share shaders Cleanup: add explicit variables for scale and offset locations Refactor: line-length formatting fix
There was a problem hiding this comment.
Code Review
This pull request refactors the Linux platform shell to move OpenGL shader management from FlCompositorOpenGL to FlOpenGLManager, enabling shader sharing across multiple compositors. Feedback identifies a logic error in the vertical offset calculation where width was used instead of height, and potential precision issues due to integer division in scale calculations. It is also recommended to use GLint for uniform locations to correctly handle error states, cache attribute locations to improve rendering performance, and include input validation in the new public getter functions for better robustness.
| GLuint offset_location = | ||
| fl_opengl_manager_get_offset_location(self->opengl_manager); | ||
| GLuint scale_location = | ||
| fl_opengl_manager_get_scale_location(self->opengl_manager); | ||
| glUniform2f(offset_location, (2 * x / width) - 1.0, (2 * y / width) - 1.0); | ||
| glUniform2f(scale_location, texture_width / width, texture_height / height); |
There was a problem hiding this comment.
There are several issues in this block:
offset_locationandscale_locationshould beGLintto match the return type ofglGetUniformLocation(and the proposed change to the manager's getters).- The vertical offset calculation uses
widthinstead ofheight. This will cause incorrect positioning if the window is not square. - The scale calculation performs integer division because
texture_width(size_t) andwidth(int) are both integral types. This will truncate the scale factor, potentially resulting in 0 (making the layer invisible) or incorrect sizes for non-integer scales. Use floating-point division instead.
GLint offset_location =
fl_opengl_manager_get_offset_location(self->opengl_manager);
GLint scale_location =
fl_opengl_manager_get_scale_location(self->opengl_manager);
glUniform2f(offset_location, (2 * x / width) - 1.0, (2 * y / height) - 1.0);
glUniform2f(scale_location, static_cast<float>(texture_width) / width,
static_cast<float>(texture_height) / height);| * | ||
| * Returns: the location of the scale uniform. | ||
| */ | ||
| GLuint fl_opengl_manager_get_scale_location(FlOpenGLManager* manager); |
There was a problem hiding this comment.
glGetUniformLocation returns a GLint, not a GLuint. See the comment for fl_opengl_manager_get_offset_location.
GLint fl_opengl_manager_get_scale_location(FlOpenGLManager* manager);| GLuint fl_opengl_manager_get_program(FlOpenGLManager* self) { | ||
| return self->program; | ||
| } |
There was a problem hiding this comment.
Public getter functions should validate the input self using g_return_val_if_fail to ensure robustness. This should be applied to all the new getter functions.
GLuint fl_opengl_manager_get_program(FlOpenGLManager* self) {
g_return_val_if_fail(FL_IS_OPENGL_MANAGER(self), 0);
return self->program;
}| GLuint fl_opengl_manager_get_vertex_buffer(FlOpenGLManager* self) { | ||
| return self->vertex_buffer; | ||
| } |
There was a problem hiding this comment.
Public getter functions should validate the input self using g_return_val_if_fail to ensure robustness.
GLuint fl_opengl_manager_get_vertex_buffer(FlOpenGLManager* self) {
g_return_val_if_fail(FL_IS_OPENGL_MANAGER(self), 0);
return self->vertex_buffer;
}| GLuint fl_opengl_manager_get_offset_location(FlOpenGLManager* self) { | ||
| return self->offset_location; | ||
| } |
There was a problem hiding this comment.
The return type should be GLint to match glGetUniformLocation. Also, add validation for the input parameter.
GLint fl_opengl_manager_get_offset_location(FlOpenGLManager* self) {
g_return_val_if_fail(FL_IS_OPENGL_MANAGER(self), -1);
return self->offset_location;
}There was a problem hiding this comment.
Don't worry about the GLuint, but yes, input should be validated.
| GLuint fl_opengl_manager_get_scale_location(FlOpenGLManager* self) { | ||
| return self->scale_location; | ||
| } |
There was a problem hiding this comment.
The return type should be GLint to match glGetUniformLocation. Also, add validation for the input parameter.
GLint fl_opengl_manager_get_scale_location(FlOpenGLManager* self) {
g_return_val_if_fail(FL_IS_OPENGL_MANAGER(self), -1);
return self->scale_location;
}
robert-ancell
left a comment
There was a problem hiding this comment.
Thanks for this. This looks generally correct, but some small things need fixing as pointed out by Gemini. There's also some existing bugs there (GLuint instead of GLint) which would be nice to fix, but these can be done in another PR. The API to FlOpenGLManager has got more complex, instead please make gl_opengl_manager_get_shader() that returns a new object to keep it simpler.
This change moves shader creation and management from individual compositors to opengl_manager to enable sharing across views.
Previously, each view’s compositor created and managed its own shaders, resulting in redundant instantiation. This change centralizes shader ownership in opengl_manager, allowing compositors to reuse a single shared shader and access its properties.
Fixes #183911
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.