On Wed, Jul 12, 2017 at 12:02:29PM +0200, Quentin Glidic wrote: > On 7/12/17 11:23 AM, Jonas Ådahl wrote: > > On Wed, Jul 12, 2017 at 09:53:04AM +0200, Quentin Glidic wrote: > > > From: Quentin Glidic <[email protected]> > > > > > > We were checking against the pending size, which lead some clients > > > (simple-egl) to crash because they sent a buffer before acknowledging > > > the latest configure event. > > > > > > Signed-off-by: Quentin Glidic <[email protected]> > > > --- > > > libweston-desktop/xdg-shell-v5.c | 6 ++++-- > > > libweston-desktop/xdg-shell-v6.c | 6 ++++-- > > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > > > > diff --git a/libweston-desktop/xdg-shell-v5.c > > > b/libweston-desktop/xdg-shell-v5.c > > > index 0fb067ab..b32b7812 100644 > > > --- a/libweston-desktop/xdg-shell-v5.c > > > +++ b/libweston-desktop/xdg-shell-v5.c > > > @@ -60,6 +60,7 @@ struct weston_desktop_xdg_surface { > > > } pending; > > > struct { > > > struct weston_desktop_xdg_surface_state state; > > > + struct weston_size size; > > > } next; > > > struct { > > > struct weston_desktop_xdg_surface_state state; > > > @@ -244,8 +245,8 @@ weston_desktop_xdg_surface_committed(struct > > > weston_desktop_surface *dsurface, > > > bool reconfigure = false; > > > if (surface->next.state.maximized || > > > surface->next.state.fullscreen) > > > - reconfigure = surface->pending.size.width != wsurface->width || > > > - surface->pending.size.height != wsurface->height; > > > + reconfigure = surface->next.size.width != wsurface->width || > > > + surface->next.size.height != wsurface->height; > > > if (reconfigure) { > > > weston_desktop_xdg_surface_schedule_configure(surface, > > > true); > > > @@ -447,6 +448,7 @@ > > > weston_desktop_xdg_surface_protocol_ack_configure(struct wl_client > > > *wl_client, > > > return; > > > surface->next.state = surface->pending.state; > > > + surface->next.size = surface->pending.size; > > > } > > > static void > > > diff --git a/libweston-desktop/xdg-shell-v6.c > > > b/libweston-desktop/xdg-shell-v6.c > > > index db894d4a..f5e46daa 100644 > > > --- a/libweston-desktop/xdg-shell-v6.c > > > +++ b/libweston-desktop/xdg-shell-v6.c > > > @@ -95,6 +95,7 @@ struct weston_desktop_xdg_toplevel { > > > } pending; > > > struct { > > > struct weston_desktop_xdg_toplevel_state state; > > > + struct weston_size size; > > > struct weston_size min_size, max_size; > > > } next; > > > struct { > > > @@ -424,6 +425,7 @@ static void > > > weston_desktop_xdg_toplevel_ack_configure(struct > > > weston_desktop_xdg_toplevel *toplevel) > > > { > > > toplevel->next.state = toplevel->pending.state; > > > + toplevel->next.size = toplevel->pending.size; > > > } > > > static void > > > @@ -626,8 +628,8 @@ weston_desktop_xdg_toplevel_committed(struct > > > weston_desktop_xdg_toplevel *toplev > > > return; > > > if ((toplevel->next.state.maximized || > > > toplevel->next.state.fullscreen) && > > > - (toplevel->pending.size.width != wsurface->width || > > > - toplevel->pending.size.height != wsurface->height)) { > > > + (toplevel->next.size.width != wsurface->width || > > > + toplevel->next.size.height != wsurface->height)) { > > > > Unrelated to this patch, but wsurface->width/height should rather be the > > geometry, not the size, because IIRC the surface size comes from the > > buffer or wp_viewporter, while the next.size.width/height is window > > geometry size. We only enforce the window geometry. > > True, just need to make sure we update the geometry before calling the > sub-type function. > > > > Shouldn't we also compare the serial here? If I understand things > > correctly, "next" is the state+size the last time a client > > ack_configure:ed a serial without any more pending one on the way. > > > So if that next state is fullscreen+WxH from when the client acked that > > at some point in time, then we quickly went toplevel->fullscreen but > > fullscreen on another output. In the intermediate state, the client acks > > the old configure, thus we won't update next, end next will still be > > fullscreen+WxH, while the surface size will be wxh (w != W, h != H). > > > > Maybe could be fixed by adding the serial to the next and pending > > structsand only enforce if the next (that is being committed) state is > > the pending one. > > I see two separate issues here: > > 1. We only consider the last configure. > I plan to fix that by keeping a list of configure states, dropping older > states until the acked one. That would fix a number of racy scenarii, not > just this one. > > 2. The shell/compositor cannot know which (fullscreen) output is being > associated with a commit. > This may be tricky to get right, because there may be more than one. The > client can only ask for one output in set_fullscreen, but the compositor is > free to ignore that and have 2 views fullscreened on differently-sized > outputs. > I would wait until we have a real use case triggering that issue. Most > clients should answer in a timely fashion to avoid that. > > This patch just fixes what size is expected on commit, so that it matches > the ack_configure.
It's indeed more correct, but don't we still need to compare the serial of pending and next here to actually check the ack_configure size? No need to check the output or whatever, just that we don't enforce an outdated size. Jonas > > > > > struct weston_desktop_client *client = > > > > > > weston_desktop_surface_get_client(toplevel->base.desktop_surface); > > > struct wl_resource *client_resource = > > > -- > > > 2.13.2 > > > > -- > > Quentin “Sardem FF7” Glidic _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
