Move compositor frame ownership into the view renderers#189162
Move compositor frame ownership into the view renderers#189162robert-ancell wants to merge 1 commit into
Conversation
Make FlCompositorSoftware and FlCompositorOpenGL stateless so they only composite layers into a caller-provided target. The view renderers now own the Cairo surface / OpenGL framebuffer and CPU pixel buffers, size them, and read frames back as required. Add FlOpenGLFrame to encapsulate the OpenGL framebuffer and CPU pixel copy managed by the OpenGL renderer, and move the shared window-resize logic into a common fl_view_renderer_resize_to_frame method on FlViewRenderer. Drop the now unused return value from fl_compositor_opengl_composite_layers. Update the tests and compositor class descriptions to match.
There was a problem hiding this comment.
Code Review
This pull request refactors the Linux platform rendering by introducing FlOpenGLFrame to manage OpenGL framebuffers and CPU pixel copies, and updating FlCompositorOpenGL and FlCompositorSoftware to composite layers directly into caller-provided contexts. Feedback suggests comparing logical dimensions instead of physical dimensions during resize checks and frame waiting to prevent potential infinite resize loops or deadlocks caused by subpixel rounding mismatches. Additionally, defensive checks are recommended to avoid creating invalid 0x0 framebuffers or Cairo surfaces.
| size_t width = allocation.width * scale_factor; | ||
| size_t height = allocation.height * scale_factor; | ||
| gboolean frame_size_matches = width == frame_width && height == frame_height; | ||
| if (frame_size_matches) { | ||
| return FALSE; | ||
| } |
There was a problem hiding this comment.
Comparing physical dimensions (width and height) directly with frame_width and frame_height can lead to an infinite resize loop if the physical frame size is not a perfect multiple of the scale_factor (e.g., due to subpixel rounding or layout constraints). Comparing logical dimensions directly using allocation.width and allocation.height avoids this issue entirely.
size_t logical_frame_width = frame_width / scale_factor;
size_t logical_frame_height = frame_height / scale_factor;
gboolean frame_size_matches = allocation.width == logical_frame_width &&
allocation.height == logical_frame_height;
if (frame_size_matches) {
return FALSE;
}| size_t frame_width, frame_height; | ||
| fl_compositor_opengl_get_frame_size(self->compositor, &frame_width, | ||
| &frame_height); | ||
| get_frame_size(self, &frame_width, &frame_height); |
There was a problem hiding this comment.
In wait_for_frame, comparing physical dimensions (frame_width == width) can cause a deadlock/freeze if there is a subpixel rounding mismatch (e.g., if the physical frame size is not a perfect multiple of the scale factor). It is safer to compare logical dimensions instead:
size_t logical_width = gdk_window_get_width(window);
size_t logical_height = gdk_window_get_height(window);
size_t frame_width, frame_height;
get_frame_size(self, &frame_width, &frame_height);
if (frame_width / scale_factor == logical_width &&
frame_height / scale_factor == logical_height) {
break;
}| size_t frame_width, frame_height; | ||
| fl_compositor_software_get_frame_size(self->compositor, &frame_width, | ||
| &frame_height); | ||
| get_frame_size(self, &frame_width, &frame_height); |
There was a problem hiding this comment.
In wait_for_frame, comparing physical dimensions (frame_width == width) can cause a deadlock/freeze if there is a subpixel rounding mismatch (e.g., if the physical frame size is not a perfect multiple of the scale factor). It is safer to compare logical dimensions instead:
size_t logical_width = gdk_window_get_width(window);
size_t logical_height = gdk_window_get_height(window);
size_t frame_width, frame_height;
get_frame_size(self, &frame_width, &frame_height);
if (frame_width / scale_factor == logical_width &&
frame_height / scale_factor == logical_height) {
break;
}| size_t width = layers[0]->size.width; | ||
| size_t height = layers[0]->size.height; |
There was a problem hiding this comment.
Add a defensive check to ensure that width and height are greater than zero before attempting to recreate the framebuffer or allocate memory. Creating a 0x0 framebuffer is invalid in OpenGL and can lead to errors or undefined behavior.
size_t width = layers[0]->size.width;
size_t height = layers[0]->size.height;
if (width == 0 || height == 0) {
return;
}There was a problem hiding this comment.
This seems like it may be worthwile?
| size_t width = layers[0]->size.width; | ||
| size_t height = layers[0]->size.height; |
There was a problem hiding this comment.
Add a defensive check to ensure that width and height are greater than zero before attempting to recreate the Cairo surface. Creating a 0x0 surface is invalid and will put the surface in an error state. Remember to unlock the mutex before returning.
size_t width = layers[0]->size.width;
size_t height = layers[0]->size.height;
if (width == 0 || height == 0) {
g_mutex_unlock(&self->frame_mutex);
return;
}|
Next step in robert-ancell#2 |
| size_t width = layers[0]->size.width; | ||
| size_t height = layers[0]->size.height; |
There was a problem hiding this comment.
This seems like it may be worthwile?
Make FlCompositorSoftware and FlCompositorOpenGL stateless so they only composite layers into a caller-provided target. The view renderers now own the Cairo surface / OpenGL framebuffer and CPU pixel buffers, size them, and read frames back as required.
Add FlOpenGLFrame to encapsulate the OpenGL framebuffer and CPU pixel copy managed by the OpenGL renderer, and move the shared window-resize logic into a common fl_view_renderer_resize_to_frame method on FlViewRenderer. Drop the now unused return value from fl_compositor_opengl_composite_layers. Update the tests and compositor class descriptions to match.