GNOME Bugzilla – Bug 782575
wayland: EGLStream resize and subsurface fixes
Last modified: 2021-07-05 13:52:50 UTC
The way the EGLStream-based wayland driver uses wl_surface::attach in order to both create the compositor endpoint of the stream and also notify when a new buffer has been generated is a bit problematic under certain situations. First, attach requests on synchronized subsurfaces are deferred until changes to the parent surface state are applied. This causes a problem where a wl_surface::attach is issued from the client for the compositor to create its stream endpoint. The client then expects the stream to be fully created (consumer attached to it) after the subsequent roundtrip. To fix this, we can realize buffers at attach time regardless, even though the rest of the attach operation is still deferred for synchronized subsurfaces. For EGLStream-backed buffers, realizing a buffer can be change to both create the stream and attach the consumer (but without acquiring any buffer). Second, whenever an application window is resized, the underlying EGLStream is re-created. That means that a wl_surface::attach request will be issued, but no actual frame would be available for presentation. This would make the window contents to flicker while resizing. To work around this, the attach operation must fail, as we are unable to fetch a new buffer. However, it should be a non-fatal failure, as subsequent wl_surface::attach operation are expected to succeed.
Created attachment 351762 [details] [review] wayland-buffer: Create EGLStream texture at buffer_realize time When dealing with synchronized subsurfaces, we defer buffer attachments until the parent surface state is applied. That causes interaction issues with EGLStream backed buffers, as the client expects the compositor-side stream to be functional after it requests a wl_surface::attach. By allowing the compositor to realize buffers without attaching them, we could resolve the issue above if we define a realized EGLStream buffer as a functional EGLStream (EGLStream + attached consumer). This change moves the texture consumer creation part from the attach function to the realize one.
Created attachment 351763 [details] [review] wayland: Realize synchronized subsurface buffers Clients using EGLStream-backed buffers will expect the stream to be functional after wl_surface::attach(). That means the compositor-side stream must be created and a consumer attached to it. To resolve the above, this change realizes buffers even when the attach operation is deferred for synchronized subsurfaces.
Created attachment 351764 [details] [review] wayland: Refactor apply_pending_state() In preparation for handling non-fatal meta_wayland_buffer_attach() errors, this change refactors apply_pending_state() so we don't change anything of the current surface state until meta_wayland_buffer_attach() has succeeded.
Created attachment 351765 [details] [review] wayland: Handle non-fatal EGLStream buffer attach errors One of the current limitations of EGLStreams is that there's no way to resize a surface consumer without re-creating the entire stream. Therefore, while resizing, clients will send wl_surface::attach requests so the compositor can re-create its endpoint of the stream, but no buffer will be available actually. If we proceed with the rest of the attach operation we'll be presenting an empty buffer. The right way to handle this would be through a separate protocol clients can use to request a stream re-creation, and only use wl_surface::attach whenever a new buffer is available. In the meantime, if EGL_RESOURCE_BUSY_EXT is reported if the above happens and we treat it as a non-fatal error.
CC += jadahl as suggested reviewer
Review of attachment 351765 [details] [review]: ::: src/wayland/meta-wayland-surface.c @@ +751,3 @@ + * stream, but no buffer will be available actually. If we proceed + * with the rest of the attach operation we'll be presenting an + * empty buffer. How exactly does resizing work right now then? During resize, when will meta_wayland_buffer_attach() succeed? Why would the client send attach() to recreate the stream end point when there is no new content the compsitor stream end point can retrieve? Either way, can't this be handled completely inside meta-wayland-egl-stream.c, in a way that the texture displayed is not updated. It feels a bit wrong to just skip the whole commit because of this, because we'd potentially forget a bunch of other pending state.
Review of attachment 351762 [details] [review]: ::: src/wayland/meta-wayland-buffer.c @@ +120,3 @@ + + if (!buffer->texture) + { I don't see how buffer->texture can ever be non-NULL here. A buffer should only ever be realized once, and a texture will always be created after it is realized (or during, as done here).
Review of attachment 351763 [details] [review]: ::: src/wayland/meta-wayland-surface.c @@ +552,3 @@ G_CALLBACK (pending_buffer_resource_destroyed), to); + /* Some backends might need buffer realization to be done, even when the s/backends/buffer types/ as backends usually refers to src/backends/ nit: empty line between g_signal_connect() and the next statement.
(In reply to Jonas Ådahl from comment #6) > How exactly does resizing work right now then? During resize, when will > meta_wayland_buffer_attach() succeed? Why would the client send attach() to > recreate the stream end point when there is no new content the compsitor > stream end point can retrieve? The issue here is the current lack of a better mechanism to resize/switch a stream surface producer.Thus, we need to re-create the whole stream when resizing. However, we cannot create a surface producer if a consumer isn't already attached to the stream. Without a surface producer in place, there's nothing for the client to make current and generate new content. We've been using wl_surface::attach + commit as a means for both: - Make the Wayland compositor create its stream endpoint and attach a consumer (application start + window resize). No new content is still available because the client still needs to create the surface producer. - Let the compositor know when new content is available so it can acquire the next stream buffer. > Either way, can't this be handled completely inside > meta-wayland-egl-stream.c, in a way that the texture displayed is not > updated. In weston, I handled this by using two different textures (front and back) for the surface. The back texture will always be used as consumer of the stream, and we'll only make the switch when there's actually something to display. At that time, the front texture isn't needed anymore and can be destroyed. I can do something similar for mutter, but I think it would be a bit more intrusive change, as we'll need to pass the current buffer down to meta_wayland_buffer_attach(), add new fields to the buffer structure, etc. > It feels a bit wrong to just skip the whole commit because of this, because > we'd potentially forget a bunch of other pending state. Agreed. I'm currently working on adding a new separate stream protocol for clients to request stream re-creation without using wl_surface::attach + commit. I'll put together the appropriate patches once it's ready, but I'm afraid we'll still need to keep the legacy paths for backwards compatibility.
(In reply to Jonas Ådahl from comment #7) > Review of attachment 351762 [details] [review] [review]: > > ::: src/wayland/meta-wayland-buffer.c > @@ +120,3 @@ > + > + if (!buffer->texture) > + { > > I don't see how buffer->texture can ever be non-NULL here. A buffer should > only ever be realized once, and a texture will always be created after it is > realized (or during, as done here). True. I forgot to remove that check when moving that piece of code from 'attach' to 'realize'. Will update.
(In reply to mvicomoya@nvidia.com from comment #9) > (In reply to Jonas Ådahl from comment #6) > > > How exactly does resizing work right now then? During resize, when will > > meta_wayland_buffer_attach() succeed? Why would the client send attach() to > > recreate the stream end point when there is no new content the compsitor > > stream end point can retrieve? > > The issue here is the current lack of a better mechanism to resize/switch a > stream surface producer.Thus, we need to re-create the whole stream when > resizing. However, we cannot create a surface producer if a consumer isn't > already attached to the stream. > > Without a surface producer in place, there's nothing for the client to make > current and generate new content. > > We've been using wl_surface::attach + commit as a means for both: > > - Make the Wayland compositor create its stream endpoint and attach a > consumer (application start + window resize). No new content is still > available because the client still needs to create the surface producer. > > - Let the compositor know when new content is available so it can acquire > the next stream buffer. > > > Either way, can't this be handled completely inside > > meta-wayland-egl-stream.c, in a way that the texture displayed is not > > updated. > > In weston, I handled this by using two different textures (front and back) > for the surface. The back texture will always be used as consumer of the > stream, and we'll only make the switch when there's actually something to > display. At that time, the front texture isn't needed anymore and can be > destroyed. > > I can do something similar for mutter, but I think it would be a bit more > intrusive change, as we'll need to pass the current buffer down to > meta_wayland_buffer_attach(), add new fields to the buffer structure, etc. I see. So during resize, you'll attach a incomplete wl_buffer to set up the stream, but the attach fails because there is no new content. A work around that limits the work-around-ness to the egl-stream related paths could be to pass the surface to meta_wayland_buffer_attach() and let a initially-empty attach fetch the texture from the previous eglstream and use that until the next attach. Then we can still continue with the rest of the commit. Or is there a risk that we might end up setting up an incorrect window geometry here? As in, when a client eglSwapBuffer():s, will we in these cases see multiple wl_surface::commit before eglSwapBuffers() finishes? For example, if a client does this: ... assume resized buffer size is current ... render_frame(new_buffer_size); set_window_geometry(new_buffer_size); eglSwapBuffers(); it is assumed that eglSwapBuffers() will send a wl_surface.commit() that commits the state set by set_window_geometry() together with the new buffer. Or is the wl_surface.attach();wl_surface.commit() done prior to all of that during the resize request? If its done as part of the egl window resize call, if we actually do continue with the surface commit process, we'll trigger a bunch of other things that actually should only be triggered when committing (for example a synchronous subsurface will get updated too soon).
Created attachment 354595 [details] [review] Addressed comments for: 351762 - wayland-buffer: Create EGLStream texture at buffer_realize time 351763 - wayland: Realize synchronized subsurface buffers And dropped: 351764 - wayland: Refactor apply_pending_state() 351765 - wayland: Handle non-fatal EGLStream buffer attach errors Instead, a new patch is provided to start using the recently added wl_eglstream_controller protocol to resolve the same issue in a cleaner and more correct way.
Created attachment 354596 [details] [review] wayland: Realize synchronized subsurface buffers
Created attachment 354597 [details] [review] wayland: Create EGLStream-backed buffers through wl_eglstream_controller One of the current limitations of EGLStreams is that there's no way to resize a surface consumer without re-creating the entire stream. Therefore, while resizing, clients will send wl_surface::attach requests so the compositor can re-create its endpoint of the stream, but no buffer will be available actually. If we proceed with the rest of the attach operation we'll be presenting an empty buffer. In order to fix this, a separate wl_eglstream_controller protocol has been introduced that clients can use to request a stream re-creation without overloading wl_surface::attach for that purpose. This change adds the required logic to create the corresponding wl_eglstream_controller global interface that clients can bind to. Whenever a client requests a stream to be created, we just need to create and realize the new EGLStream buffer. The same buffer resource will be given at a later time to wl_surface::attach, whenever new content is made available by the application, so we can proceed to acquire the stream buffer and update the surface state.
Created attachment 354598 [details] [review] wayland-buffer: Create EGLStream texture at buffer_realize time
Created attachment 354599 [details] [review] wayland: Realize synchronized subsurface buffers
Created attachment 354600 [details] [review] wayland: Create EGLStream-backed buffers through wl_eglstream_controller
Sorry, I botched some of the comments with "git bz attach". I re-attached all three new patches so they are in order.
Created attachment 354660 [details] [review] wayland: Create EGLStream-backed buffers through wl_eglstream_controller Added following note to the commit message. Note: * wayland-eglstream-controller.xml is a copy of: https://github.com/NVIDIA/egl-wayland/blob/6fba7dff1858963cc654037b792cf58ae456c6ae/wayland-eglstream/wayland-eglstream-controller.xml
Review of attachment 354660 [details] [review]: ::: src/wayland/meta-wayland-egl-stream.c @@ +37,3 @@ +#include "wayland-eglstream-controller-server-protocol.h" + +#include <dlfcn.h> System includes above the cogl-egl.h include (with empty newline in between) @@ +53,3 @@ + if (!meta_wayland_buffer_realize (buffer)) + { + g_error ("Unable to realize EGLStream buffer"); Isn't this a bit harsh? When could this happen? @@ +104,3 @@ + * + * Failure to initialize wl_eglstream_controller is non-fatal + */ I wonder if we rather shouldn't conditionalize this on whether the EGL we expose supports this? Not exactly sure how though; the MetaRendererNative is more about how to talk to the GPU than how clients talk. I mean, it'd bad if we exposed this and then if its used things would crash and what not. @@ +106,3 @@ + */ + + void *lib = dlopen("libnvidia-egl-wayland.so.1", RTLD_NOW | RTLD_LAZY); dlopen ( @@ +128,3 @@ + dlclose(lib); + + g_warning ("WL: Unable to initialize wl_eglstream_controller."); I don't think we should warn about it always, especially on systems without any nvidia hardware at all.
Created attachment 355895 [details] [review] wayland-buffer: Create EGLStream texture at buffer_realize time When dealing with synchronized subsurfaces, we defer buffer attachments until the parent surface state is applied. That causes interaction issues with EGLStream backed buffers, as the client expects the compositor-side stream to be functional after it requests a wl_surface::attach. By allowing the compositor to realize buffers without attaching them, we could resolve the issue above if we define a realized EGLStream buffer as a functional EGLStream (EGLStream + attached consumer). This change moves the texture consumer creation part from the attach function to the realize one. Changes against 354598: - None (trivial rebase)
Created attachment 355896 [details] [review] wayland: Realize synchronized subsurface buffers Clients using EGLStream-backed buffers will expect the stream to be functional after wl_surface::attach(). That means the compositor-side stream must be created and a consumer attached to it. To resolve the above, this change realizes buffers even when the attach operation is deferred for synchronized subsurfaces. Changes against 354599: - None (trivial rebase)
Created attachment 355897 [details] [review] wayland: Create EGLStream-backed buffers through wl_eglstream_controller One of the current limitations of EGLStreams is that there's no way to resize a surface consumer without re-creating the entire stream. Therefore, while resizing, clients will send wl_surface::attach requests so the compositor can re-create its endpoint of the stream, but no buffer will be available actually. If we proceed with the rest of the attach operation we'll be presenting an empty buffer. In order to fix this, a separate wl_eglstream_controller protocol has been introduced that clients can use to request a stream re-creation without overloading wl_surface::attach for that purpose. This change adds the required logic to create the corresponding wl_eglstream_controller global interface that clients can bind to. Whenever a client requests a stream to be created, we just need to create and realize the new EGLStream buffer. The same buffer resource will be given at a later time to wl_surface::attach, whenever new content is made available by the application, so we can proceed to acquire the stream buffer and update the surface state. Note: * wayland-eglstream-controller.xml is a copy of: https://github.com/NVIDIA/egl-wayland/blob/6fba7dff1858963cc654037b792cf58ae456c6ae/wayland-eglstream/wayland-eglstream-controller.xml Changes against 354660: - Moved system include above the 'cogl-egl.h' include - Removed the g_error ("Unable to realize EGLStream buffer") call - Fixed cosmetic issues - Changed g_warning ("WL: Unable to initialize wl_eglstream_controller.") to g_debug - Only initialize wl_eglstream_controller if HAVE_EGL_DEVICE is defined
> I wonder if we rather shouldn't conditionalize this on whether the EGL we > expose supports this? Not exactly sure how though; the MetaRendererNative is > more about how to talk to the GPU than how clients talk. I mean, it'd bad if we > exposed this and then if its used things would crash and what not. Not sure I completely understand this. Whenever we create the global object, we are expected to properly handle requests to it. It's up to the clients to use this protocol or not. Thus, if libnivida-egl-wayland.so is not found, or doesn't export the wl_eglstream_controller interface, we won't create the global. Clients should either implement a fallback path, or fail initialization if they require this protocol. Also, in attachment 355897 [details] [review], I wrapped the new global initialization with "ifdef HAVE_EGL_DEVICE", as we don't need it with the non-EGLDevice paths. Am I missing something here?
(In reply to mvicomoya@nvidia.com from comment #24) > > I wonder if we rather shouldn't conditionalize this on whether the EGL we > > expose supports this? Not exactly sure how though; the MetaRendererNative is > > more about how to talk to the GPU than how clients talk. I mean, it'd bad if we > > exposed this and then if its used things would crash and what not. > > Not sure I completely understand this. Whenever we create the global object, > we are expected to properly handle requests to it. It's up to the clients to > use this protocol or not. > > Thus, if libnivida-egl-wayland.so is not found, or doesn't export the > wl_eglstream_controller interface, we won't create the global. Clients > should either implement a fallback path, or fail initialization if they > require this protocol. > > Also, in attachment 355897 [details] [review] [review], I wrapped the new global > initialization with "ifdef HAVE_EGL_DEVICE", as we don't need it with the > non-EGLDevice paths. > > Am I missing something here? No, makes sense.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/mutter/-/issues/ Thank you for your understanding and your help.