On 7/12/17 12:07 PM, Jonas Ådahl wrote:
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.
"next" is only set on ack_configure, so the only way for it to be
outdated is (-> is client to server):
<- configure(1, fullscreen)
-> ack_configure(1)
-> commit(1)
<- configure(2, not fullscreen)
<- configure(3, fullscreen)
-> ack_configure(2) # ignored because not last one
-> commit(2)
At that point, the compositor expects a specific size (1) but client
commits a free size (2).
This will be fixed with a configure list based on serials.
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
--
Quentin “Sardem FF7” Glidic
_______________________________________________
wayland-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
