Use Wayland subsurfaces for rendering when available.#188767
Use Wayland subsurfaces for rendering when available.#188767robert-ancell wants to merge 20 commits into
Conversation
Split FlViewRenderer into a minimal abstract base class with two self-contained backend subclasses, FlViewRendererOpenGL and FlViewRendererSoftware. Each subclass owns its own state and compositor and implements rendering for its backend. fl_view.cc selects which subclass to create based on the engine's renderer type.
Make FlCompositorOpenGL and FlCompositorSoftware derive directly from GObject and expose their present_layers, get_frame_size and render functions as public concrete functions. The view renderers and compositor tests now call these directly, removing the need for the FlCompositor base class, which is deleted.
Make FlCompositorSoftware pure frame storage/combining by moving the frame_mutex and task_runner into FlViewRendererSoftware. The renderer now owns the synchronization: it locks around compositor access, signals the task runner after presenting, and runs the resize wait loop in draw.
Make FlCompositorOpenGL pure frame storage/combining by moving the frame_mutex and task_runner into FlViewRendererOpenGL. The renderer now owns the synchronization: it locks around compositor access, signals the task runner after presenting, and runs the resize wait loop in draw.
The pixels buffer is only populated when shareable is FALSE, not TRUE.
Introduce an empty FlViewRenderer subclass for rendering OpenGL into a Wayland subsurface, selected when the OpenGL backend is used on a Wayland display. The renderer implementation will be added in a later stage.
Render Flutter OpenGL frames into a Wayland subsurface by compositing layers into the engine's shareable framebuffer, then blitting the composited frame to a wl_egl_window EGL surface and swapping. Adds wayland-client/wayland-egl configs and an accessor to retrieve the compositor framebuffer.
Match the FlViewRendererOpenGL and FlViewRendererSoftware convention of clearing the frame and resize mutexes in dispose() while leaving the compositor release in finalize().
Switching the EGL context on the engine's raster thread inside present_layers corrupted the Impeller GLES backend's cached context state, causing a fatal GL_INVALID_FRAMEBUFFER_OPERATION on glClear. Composite on the raster thread as before, but perform the EGL blit and swap to the subsurface on the GTK thread in draw(), where it never touches the engine's rendering context. Also stop terminating the EGL display in unrealize, since it is shared with the engine.
Move the Wayland registry binding and wl_compositor/wl_subcompositor/ wl_surface/wl_subsurface management out of FlViewRendererSubsurface and into a dedicated FlSubsurface module, mirroring flutter_subsurface.c in the prototype. The renderer now creates an FlSubsurface and uses it to obtain the wl_surface for its EGL window and to reposition the subsurface.
Destroying an FlFramebuffer makes OpenGL calls (glDeleteFramebuffers etc.) that epoxy resolves lazily and which require a current context. The sibling was held in a g_autoptr and therefore destroyed at scope exit, after the EGL context had already been cleared, tripping epoxy's "Couldn't find current GLX or EGL context" assertion. Release it explicitly while the context is still current.
The resize barrier (ported from the prototype's flutter_view_resize) blocked the GTK main thread in size_allocate on a plain GCond until the raster thread presented a frame of the new size. Unlike the OpenGL renderer's fl_task_runner_wait, this wait did not pump the platform task runner, so it could deadlock frame production and lock up resizing. A Wayland subsurface commits its own buffer independently of the parent surface, so no such synchronization is needed: drop the barrier and the now-unused task runner, and just resize the wl_egl_window.
A wl_surface has an infinite input region by default, so the subsurface placed over the view captured pointer, touch and keyboard events instead of letting them reach the parent GTK surface, breaking all input. Set an empty input region on the subsurface so events pass through to GTK, which handles input for the view.
Add a method on the FlViewRenderer base class to emit the "first-frame" signal, so subclasses no longer emit it by name directly. Update the OpenGL, software and subsurface renderers to use it.
The subsurface renderer created its own EGL context unshared with the engine and relied on EGLImage (via fl_framebuffer_create_sibling) to access the engine's composited frame. This required EGLImage support. Create the subsurface's EGL context sharing the engine's render context so the engine frame texture can be read directly. The compositor's frame sharing strategy is now an explicit enum (EGL image, shared context, or CPU copy); the shared-context mode creates a framebuffer with no EGLImage and skips the CPU readback.
The EGL config is only used while setting up the EGL context and window surface, so it does not need to be stored on the renderer.
The EGL display is owned by the engine's FlOpenGLManager, not the renderer. Rather than caching a copy, fetch it through a get_egl_display helper where needed. This removes duplicated, borrowed state and makes it clear the renderer does not own the display.
The background color storage, default, set_background_color and the paint-background logic were duplicated in each renderer backend. Move them into the FlViewRenderer base class: it now stores the color in private instance data, exposes fl_view_renderer_paint_background, and implements set_background_color directly (the per-subclass vfunc is removed). Subclasses call fl_view_renderer_paint_background in their draw implementation.
|
Still a work in progress but would appreciate some early reviews! |
There was a problem hiding this comment.
Code Review
This pull request refactors the Flutter Linux platform embedding by removing the abstract FlCompositor class and introducing dedicated subclasses of FlViewRenderer (FlViewRendererOpenGL, FlViewRendererSoftware, and FlViewRendererSubsurface) along with a new FlSubsurface helper to manage Wayland subsurfaces. Feedback on these changes highlights a compilation error from calling wl_compositor_destroy, potential null pointer dereferences during Wayland and GTK initialization, an OpenGL ES version mismatch when using glBlitFramebuffer, and resource leaks on EGL setup failure paths. Additionally, suggestions were made to add defensive checks for coordinate translation, window dimensions, and EGL calls, as well as to optimize performance by reusing framebuffer objects instead of recreating them on every frame.
| g_clear_pointer(&self->subsurface, wl_subsurface_destroy); | ||
| g_clear_pointer(&self->surface, wl_surface_destroy); | ||
| g_clear_pointer(&self->subcompositor, wl_subcompositor_destroy); | ||
| g_clear_pointer(&self->compositor, wl_compositor_destroy); |
There was a problem hiding this comment.
wl_compositor does not have a destructor request in the Wayland protocol, so wl_compositor_destroy is not generated in wayland-client-protocol.h. Calling it will cause a compilation error. Use wl_proxy_destroy instead to release the client-side proxy.
if (self->compositor != nullptr) {
wl_proxy_destroy(reinterpret_cast<struct wl_proxy*>(self->compositor));
self->compositor = nullptr;
}| GtkWidget* toplevel = gtk_widget_get_toplevel(widget); | ||
| GdkWindow* gdk_window = gtk_widget_get_window(toplevel); | ||
| struct wl_surface* parent_surface = | ||
| gdk_wayland_window_get_wl_surface(gdk_window); | ||
| self->surface = wl_compositor_create_surface(self->compositor); | ||
| self->subsurface = wl_subcompositor_get_subsurface( | ||
| self->subcompositor, self->surface, parent_surface); | ||
| wl_subsurface_set_sync(self->subsurface); |
There was a problem hiding this comment.
Defensive programming: gtk_widget_get_window can return nullptr if the toplevel window is not realized, and gdk_wayland_window_get_wl_surface can return nullptr if the window is not fully initialized. Additionally, check if wl_compositor_create_surface and wl_subcompositor_get_subsurface return nullptr to prevent potential crashes.
GtkWidget* toplevel = gtk_widget_get_toplevel(widget);
GdkWindow* gdk_window = gtk_widget_get_window(toplevel);
if (gdk_window == nullptr) {
g_warning("Toplevel window is not realized");
g_object_unref(self);
return nullptr;
}
struct wl_surface* parent_surface =
gdk_wayland_window_get_wl_surface(gdk_window);
if (parent_surface == nullptr) {
g_warning("Failed to get Wayland surface for toplevel window");
g_object_unref(self);
return nullptr;
}
self->surface = wl_compositor_create_surface(self->compositor);
if (self->surface == nullptr) {
g_warning("Failed to create Wayland surface");
g_object_unref(self);
return nullptr;
}
self->subsurface = wl_subcompositor_get_subsurface(
self->subcompositor, self->surface, parent_surface);
if (self->subsurface == nullptr) {
g_warning("Failed to create Wayland subsurface");
g_object_unref(self);
return nullptr;
}
wl_subsurface_set_sync(self->subsurface);| static const EGLint context_attributes[] = {EGL_CONTEXT_CLIENT_VERSION, 2, | ||
| EGL_NONE}; |
There was a problem hiding this comment.
glBlitFramebuffer is an OpenGL ES 3.0 feature. Requesting an OpenGL ES 2.0 context (EGL_CONTEXT_CLIENT_VERSION, 2) will cause a failure or crash on systems that strictly enforce ES 2.0. Request client version 3 instead.
static const EGLint context_attributes[] = {EGL_CONTEXT_CLIENT_VERSION, 3,
EGL_NONE};| self->egl_context = eglCreateContext(egl_display, egl_config, share_context, | ||
| context_attributes); | ||
| if (self->egl_context == EGL_NO_CONTEXT) { | ||
| g_warning("Failed to create EGL context for subsurface"); | ||
| return FALSE; | ||
| } | ||
|
|
||
| self->egl_window = | ||
| wl_egl_window_create(fl_subsurface_get_surface(self->subsurface), | ||
| width * scale, height * scale); | ||
| if (self->egl_window == nullptr) { | ||
| g_warning("Failed to create wl_egl_window for subsurface"); | ||
| return FALSE; | ||
| } | ||
|
|
||
| self->egl_surface = eglCreateWindowSurface( | ||
| egl_display, egl_config, | ||
| reinterpret_cast<EGLNativeWindowType>(self->egl_window), nullptr); | ||
| if (self->egl_surface == EGL_NO_SURFACE) { | ||
| g_warning("Failed to create EGL window surface for subsurface"); | ||
| return FALSE; | ||
| } |
There was a problem hiding this comment.
Resource leak on error paths: if wl_egl_window_create or eglCreateWindowSurface fails, previously created EGL resources (like self->egl_context or self->egl_window) are leaked. Clean them up before returning FALSE.
self->egl_context = eglCreateContext(egl_display, egl_config, share_context,
context_attributes);
if (self->egl_context == EGL_NO_CONTEXT) {
g_warning("Failed to create EGL context for subsurface");
return FALSE;
}
self->egl_window =
wl_egl_window_create(fl_subsurface_get_surface(self->subsurface),
width * scale, height * scale);
if (self->egl_window == nullptr) {
g_warning("Failed to create wl_egl_window for subsurface");
eglDestroyContext(egl_display, self->egl_context);
self->egl_context = EGL_NO_CONTEXT;
return FALSE;
}
self->egl_surface = eglCreateWindowSurface(
egl_display, egl_config,
reinterpret_cast<EGLNativeWindowType>(self->egl_window), nullptr);
if (self->egl_surface == EGL_NO_SURFACE) {
g_warning("Failed to create EGL window surface for subsurface");
wl_egl_window_destroy(self->egl_window);
self->egl_window = nullptr;
eglDestroyContext(egl_display, self->egl_context);
self->egl_context = EGL_NO_CONTEXT;
return FALSE;
}| gint x, y; | ||
| gtk_widget_translate_coordinates(widget, toplevel, 0, 0, &x, &y); | ||
| fl_subsurface_set_position(self->subsurface, x, y); |
There was a problem hiding this comment.
Defensive programming: gtk_widget_translate_coordinates can fail if the widget is not realized or not in the same screen. Check its return value before using x and y.
gint x = 0, y = 0;
if (gtk_widget_translate_coordinates(widget, toplevel, 0, 0, &x, &y)) {
fl_subsurface_set_position(self->subsurface, x, y);
}| self->egl_window = | ||
| wl_egl_window_create(fl_subsurface_get_surface(self->subsurface), | ||
| width * scale, height * scale); |
There was a problem hiding this comment.
Guard against 0 or negative dimensions when creating the wl_egl_window to prevent Wayland protocol errors or EGL failures.
int egl_width = MAX(static_cast<int>(width) * scale, 1);
int egl_height = MAX(static_cast<int>(height) * scale, 1);
self->egl_window =
wl_egl_window_create(fl_subsurface_get_surface(self->subsurface),
egl_width, egl_height);| EGLDisplay egl_display = get_egl_display(self); | ||
| if (self->egl_surface != EGL_NO_SURFACE) { | ||
| eglDestroySurface(egl_display, self->egl_surface); | ||
| self->egl_surface = EGL_NO_SURFACE; | ||
| } | ||
| if (self->egl_context != EGL_NO_CONTEXT) { | ||
| eglDestroyContext(egl_display, self->egl_context); | ||
| self->egl_context = EGL_NO_CONTEXT; | ||
| } |
There was a problem hiding this comment.
Defensive programming: Guard eglDestroySurface and eglDestroyContext against EGL_NO_DISPLAY to prevent potential crashes or errors on buggy drivers.
EGLDisplay egl_display = get_egl_display(self);
if (egl_display != EGL_NO_DISPLAY) {
if (self->egl_surface != EGL_NO_SURFACE) {
eglDestroySurface(egl_display, self->egl_surface);
self->egl_surface = EGL_NO_SURFACE;
}
if (self->egl_context != EGL_NO_CONTEXT) {
eglDestroyContext(egl_display, self->egl_context);
self->egl_context = EGL_NO_CONTEXT;
}
}| EGLDisplay egl_display = get_egl_display(self); | ||
| eglMakeCurrent(egl_display, self->egl_surface, self->egl_surface, | ||
| self->egl_context); |
There was a problem hiding this comment.
Defensive programming: eglMakeCurrent can fail. Check its return value before executing OpenGL commands.
EGLDisplay egl_display = get_egl_display(self);
if (!eglMakeCurrent(egl_display, self->egl_surface, self->egl_surface,
self->egl_context)) {
g_warning("Failed to make EGL context current for subsurface");
return TRUE;
}| GLuint read_framebuffer; | ||
| glGenFramebuffers(1, &read_framebuffer); | ||
| glBindFramebuffer(GL_READ_FRAMEBUFFER, read_framebuffer); | ||
| glFramebufferTexture2D(GL_READ_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, | ||
| GL_TEXTURE_2D, | ||
| fl_framebuffer_get_texture_id(framebuffer), 0); | ||
| glBindFramebuffer(GL_DRAW_FRAMEBUFFER, 0); | ||
| glBlitFramebuffer(0, 0, width, height, 0, 0, width, height, | ||
| GL_COLOR_BUFFER_BIT, GL_NEAREST); | ||
| eglSwapBuffers(egl_display, self->egl_surface); | ||
| glDeleteFramebuffers(1, &read_framebuffer); |
|
Rebuilding this branch |
No description provided.