[weston] smoke: Don't commit an xdg_surface with a NULL buffer

Submitted by Jasper St. Pierre on Oct. 13, 2014, 1:57 a.m.

Details

Message ID 1413165451-19329-1-git-send-email-jstpierre@mecheye.net
State Accepted
Commit c8e41868fd488fe796cea4cde0b98acbee7a2551
Headers show

Not browsing as part of any series.

Commit Message

Jasper St. Pierre Oct. 13, 2014, 1:57 a.m.
Committing to an xdg_surface with a NULL buffer is currently illegal in
the mutter implementation, so this simply causes the client to error and
exit.

It seems the reason the client did this was so it could add its own
frame callback, but toytoolkit actually provides accurate everything we
need. Just use its functions instead to get the time and schedule a
redraw.
---
 clients/smoke.c | 24 ++----------------------
 1 file changed, 2 insertions(+), 22 deletions(-)

Patch hide | download patch | download mbox

diff --git a/clients/smoke.c b/clients/smoke.c
index 65b6e03..7f0d5e5 100644
--- a/clients/smoke.c
+++ b/clients/smoke.c
@@ -39,7 +39,6 @@  struct smoke {
 	struct widget *widget;
 	int width, height;
 	int current;
-	uint32_t time;
 	struct { float *d, *u, *v; } b[2];
 };
 
@@ -171,27 +170,10 @@  static void render(struct smoke *smoke, cairo_surface_t *surface)
 }
 
 static void
-frame_callback(void *data, struct wl_callback *callback, uint32_t time)
-{
-	struct smoke *smoke = data;
-
-	window_schedule_redraw(smoke->window);
-	smoke->time = time;
-
-	if (callback)
-		wl_callback_destroy(callback);
-}
-
-static const struct wl_callback_listener listener = {
-	frame_callback,
-};
-
-static void
 redraw_handler(struct widget *widget, void *data)
 {
 	struct smoke *smoke = data;
-	uint32_t time = smoke->time;
-	struct wl_callback *callback;
+	uint32_t time = widget_get_last_time(smoke->widget);
 	cairo_surface_t *surface;
 
 	diffuse(smoke, time / 30, smoke->b[0].u, smoke->b[1].u);
@@ -222,9 +204,7 @@  redraw_handler(struct widget *widget, void *data)
 
 	cairo_surface_destroy(surface);
 
-	callback = wl_surface_frame(window_get_wl_surface(smoke->window));
-	wl_callback_add_listener(callback, &listener, smoke);
-	wl_surface_commit(window_get_wl_surface(smoke->window));
+	widget_schedule_redraw(smoke->widget);
 }
 
 static void

Comments

On Sun, Oct 12, 2014 at 06:57:31PM -0700, Jasper St. Pierre wrote:
> Committing to an xdg_surface with a NULL buffer is currently illegal in
> the mutter implementation, so this simply causes the client to error and
> exit.
> 
> It seems the reason the client did this was so it could add its own
> frame callback, but toytoolkit actually provides accurate everything we
> need.

Did you mean 'provides everything'?

> Just use its functions instead to get the time and schedule a
> redraw.

Looks like a good simplification for example clients.

Reviewed-by: Bryce Harrington <b.harrington@samsung.com>

> ---
>  clients/smoke.c | 24 ++----------------------
>  1 file changed, 2 insertions(+), 22 deletions(-)
> 
> diff --git a/clients/smoke.c b/clients/smoke.c
> index 65b6e03..7f0d5e5 100644
> --- a/clients/smoke.c
> +++ b/clients/smoke.c
> @@ -39,7 +39,6 @@ struct smoke {
>  	struct widget *widget;
>  	int width, height;
>  	int current;
> -	uint32_t time;
>  	struct { float *d, *u, *v; } b[2];
>  };
>  
> @@ -171,27 +170,10 @@ static void render(struct smoke *smoke, cairo_surface_t *surface)
>  }
>  
>  static void
> -frame_callback(void *data, struct wl_callback *callback, uint32_t time)
> -{
> -	struct smoke *smoke = data;
> -
> -	window_schedule_redraw(smoke->window);
> -	smoke->time = time;
> -
> -	if (callback)
> -		wl_callback_destroy(callback);
> -}
> -
> -static const struct wl_callback_listener listener = {
> -	frame_callback,
> -};
> -
> -static void
>  redraw_handler(struct widget *widget, void *data)
>  {
>  	struct smoke *smoke = data;
> -	uint32_t time = smoke->time;
> -	struct wl_callback *callback;
> +	uint32_t time = widget_get_last_time(smoke->widget);
>  	cairo_surface_t *surface;
>  
>  	diffuse(smoke, time / 30, smoke->b[0].u, smoke->b[1].u);
> @@ -222,9 +204,7 @@ redraw_handler(struct widget *widget, void *data)
>  
>  	cairo_surface_destroy(surface);
>  
> -	callback = wl_surface_frame(window_get_wl_surface(smoke->window));
> -	wl_callback_add_listener(callback, &listener, smoke);
> -	wl_surface_commit(window_get_wl_surface(smoke->window));
> +	widget_schedule_redraw(smoke->widget);
>  }
>  
>  static void
> -- 
> 2.1.0
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
2014-10-13 4:57 GMT+03:00 Jasper St. Pierre <jstpierre@mecheye.net>:
> Committing to an xdg_surface with a NULL buffer is currently illegal in
> the mutter implementation, so this simply causes the client to error and
> exit.

The patch looks good to me.
However, this leaves me wondering how does a client hide a surface.
Does it have to destroy the xdg_surface? I'd expect a compositor to
show a window at the same position it was when hidden, but destroying
the xdg_surface loses all the state.


--
Giulio

>
> It seems the reason the client did this was so it could add its own
> frame callback, but toytoolkit actually provides accurate everything we
> need. Just use its functions instead to get the time and schedule a
> redraw.
> ---
>  clients/smoke.c | 24 ++----------------------
>  1 file changed, 2 insertions(+), 22 deletions(-)
>
> diff --git a/clients/smoke.c b/clients/smoke.c
> index 65b6e03..7f0d5e5 100644
> --- a/clients/smoke.c
> +++ b/clients/smoke.c
> @@ -39,7 +39,6 @@ struct smoke {
>         struct widget *widget;
>         int width, height;
>         int current;
> -       uint32_t time;
>         struct { float *d, *u, *v; } b[2];
>  };
>
> @@ -171,27 +170,10 @@ static void render(struct smoke *smoke, cairo_surface_t *surface)
>  }
>
>  static void
> -frame_callback(void *data, struct wl_callback *callback, uint32_t time)
> -{
> -       struct smoke *smoke = data;
> -
> -       window_schedule_redraw(smoke->window);
> -       smoke->time = time;
> -
> -       if (callback)
> -               wl_callback_destroy(callback);
> -}
> -
> -static const struct wl_callback_listener listener = {
> -       frame_callback,
> -};
> -
> -static void
>  redraw_handler(struct widget *widget, void *data)
>  {
>         struct smoke *smoke = data;
> -       uint32_t time = smoke->time;
> -       struct wl_callback *callback;
> +       uint32_t time = widget_get_last_time(smoke->widget);
>         cairo_surface_t *surface;
>
>         diffuse(smoke, time / 30, smoke->b[0].u, smoke->b[1].u);
> @@ -222,9 +204,7 @@ redraw_handler(struct widget *widget, void *data)
>
>         cairo_surface_destroy(surface);
>
> -       callback = wl_surface_frame(window_get_wl_surface(smoke->window));
> -       wl_callback_add_listener(callback, &listener, smoke);
> -       wl_surface_commit(window_get_wl_surface(smoke->window));
> +       widget_schedule_redraw(smoke->widget);
>  }
>
>  static void
> --
> 2.1.0
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
On Tue, Oct 14, 2014 at 2:45 AM, Giulio Camuffo <giuliocamuffo@gmail.com>
wrote:

> 2014-10-13 4:57 GMT+03:00 Jasper St. Pierre <jstpierre@mecheye.net>:
> > Committing to an xdg_surface with a NULL buffer is currently illegal in
> > the mutter implementation, so this simply causes the client to error and
> > exit.
>
> The patch looks good to me.
> However, this leaves me wondering how does a client hide a surface.
> Does it have to destroy the xdg_surface? I'd expect a compositor to
> show a window at the same position it was when hidden, but destroying
> the xdg_surface loses all the state
>

When combined with map / unmap animations, I think we have to decide that
"show" means "the window is presented in a compositor-chosen place". As
such, I chose to make attaching a NULL buffer against the rules.

Are there any use cases for hiding a window while keeping it in the same
place? We couldn't think of any on IRC.


> --
> Giulio
>
> >
> > It seems the reason the client did this was so it could add its own
> > frame callback, but toytoolkit actually provides accurate everything we
> > need. Just use its functions instead to get the time and schedule a
> > redraw.
> > ---
> >  clients/smoke.c | 24 ++----------------------
> >  1 file changed, 2 insertions(+), 22 deletions(-)
> >
> > diff --git a/clients/smoke.c b/clients/smoke.c
> > index 65b6e03..7f0d5e5 100644
> > --- a/clients/smoke.c
> > +++ b/clients/smoke.c
> > @@ -39,7 +39,6 @@ struct smoke {
> >         struct widget *widget;
> >         int width, height;
> >         int current;
> > -       uint32_t time;
> >         struct { float *d, *u, *v; } b[2];
> >  };
> >
> > @@ -171,27 +170,10 @@ static void render(struct smoke *smoke,
> cairo_surface_t *surface)
> >  }
> >
> >  static void
> > -frame_callback(void *data, struct wl_callback *callback, uint32_t time)
> > -{
> > -       struct smoke *smoke = data;
> > -
> > -       window_schedule_redraw(smoke->window);
> > -       smoke->time = time;
> > -
> > -       if (callback)
> > -               wl_callback_destroy(callback);
> > -}
> > -
> > -static const struct wl_callback_listener listener = {
> > -       frame_callback,
> > -};
> > -
> > -static void
> >  redraw_handler(struct widget *widget, void *data)
> >  {
> >         struct smoke *smoke = data;
> > -       uint32_t time = smoke->time;
> > -       struct wl_callback *callback;
> > +       uint32_t time = widget_get_last_time(smoke->widget);
> >         cairo_surface_t *surface;
> >
> >         diffuse(smoke, time / 30, smoke->b[0].u, smoke->b[1].u);
> > @@ -222,9 +204,7 @@ redraw_handler(struct widget *widget, void *data)
> >
> >         cairo_surface_destroy(surface);
> >
> > -       callback =
> wl_surface_frame(window_get_wl_surface(smoke->window));
> > -       wl_callback_add_listener(callback, &listener, smoke);
> > -       wl_surface_commit(window_get_wl_surface(smoke->window));
> > +       widget_schedule_redraw(smoke->widget);
> >  }
> >
> >  static void
> > --
> > 2.1.0
> >
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
On 10/16/2014 08:03 PM, Jasper St. Pierre wrote:

> Are there any use cases for hiding a window while keeping it in the same
> place? We couldn't think of any on IRC.

There may be a desire to do that with child windows. If a client has a 
button that turns a child window on/off, and the user moves the child, 
then toggles the button on/off it probably should reappear where it was.

But I think it is ok if the user moves the parent window while the 
children are hidden, the children reappear moved the same amount.

I think currently the client can control where the child first appears, 
and could make it reappear in the same location (relative to the parent 
window) but not where the user moved it. But the fix would probably be 
to add an event so the client knows where the window was moved relative 
to the parent.
Wayland does not have child windows.
On Oct 16, 2014 8:22 PM, "Bill Spitzak" <spitzak@gmail.com> wrote:

> On 10/16/2014 08:03 PM, Jasper St. Pierre wrote:
>
>  Are there any use cases for hiding a window while keeping it in the same
>> place? We couldn't think of any on IRC.
>>
>
> There may be a desire to do that with child windows. If a client has a
> button that turns a child window on/off, and the user moves the child, then
> toggles the button on/off it probably should reappear where it was.
>
> But I think it is ok if the user moves the parent window while the
> children are hidden, the children reappear moved the same amount.
>
> I think currently the client can control where the child first appears,
> and could make it reappear in the same location (relative to the parent
> window) but not where the user moved it. But the fix would probably be to
> add an event so the client knows where the window was moved relative to the
> parent.
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
On Sun, 12 Oct 2014 18:57:31 -0700
"Jasper St. Pierre" <jstpierre@mecheye.net> wrote:

> Committing to an xdg_surface with a NULL buffer is currently illegal in
> the mutter implementation, so this simply causes the client to error and
> exit.

The patch is good, but I'd like to reiterate on what this actually
fixes.

AFAICS, neither toytoolkit nor smoke ever actually does a
wl_surface(attach, NULL, x, y). If you do a WAYLAND_DEBUG=client dump
from 'smoke' without this patch, there is not a single attach with a
null buffer.

However, there is a sequence in the beginning:

[1995969.194]  -> wl_compositor@3.create_surface(new id wl_surface@14)
[1995969.214]  -> xdg_shell@11.get_xdg_surface(new id xdg_surface@15, wl_surface@14)
[1995969.227]  -> xdg_surface@15.set_title("smoke")
[1995969.254]  -> wl_compositor@3.create_region(new id wl_region@16)
[1995969.523]  -> wl_shm@6.create_pool(new id wl_shm_pool@17, fd 10, 160000)
[1995969.546]  -> wl_shm_pool@17.create_buffer(new id wl_buffer@18, 0, 200, 200, 800, 0)
[1995969.571]  -> wl_surface@14.frame(new id wl_callback@19)
[1995985.307]  -> wl_surface@14.damage(0, 0, 200, 200)
[1995985.339]  -> wl_surface@14.frame(new id wl_callback@20)
[1995985.348]  -> wl_surface@14.commit()

There is a commit of damage without ever attaching a buffer. That is
quite useless indeed, especially if asking for frame events, since the
surface won't be mapped without content, and so the frame callback
won't be replied until a buffer actually is attached and committed.

xdg_surface specification should probably say something about
committing surfaces without content, and what removing content from a
wl_surface does in the context of xdg_shell.

> It seems the reason the client did this was so it could add its own
> frame callback, but toytoolkit actually provides accurate everything we
> need. Just use its functions instead to get the time and schedule a
> redraw.

Yup, pushed. This patch fixes the strange init sequence.


Thanks,
pq


> ---
>  clients/smoke.c | 24 ++----------------------
>  1 file changed, 2 insertions(+), 22 deletions(-)
> 
> diff --git a/clients/smoke.c b/clients/smoke.c
> index 65b6e03..7f0d5e5 100644
> --- a/clients/smoke.c
> +++ b/clients/smoke.c
> @@ -39,7 +39,6 @@ struct smoke {
>  	struct widget *widget;
>  	int width, height;
>  	int current;
> -	uint32_t time;
>  	struct { float *d, *u, *v; } b[2];
>  };
>  
> @@ -171,27 +170,10 @@ static void render(struct smoke *smoke, cairo_surface_t *surface)
>  }
>  
>  static void
> -frame_callback(void *data, struct wl_callback *callback, uint32_t time)
> -{
> -	struct smoke *smoke = data;
> -
> -	window_schedule_redraw(smoke->window);
> -	smoke->time = time;
> -
> -	if (callback)
> -		wl_callback_destroy(callback);
> -}
> -
> -static const struct wl_callback_listener listener = {
> -	frame_callback,
> -};
> -
> -static void
>  redraw_handler(struct widget *widget, void *data)
>  {
>  	struct smoke *smoke = data;
> -	uint32_t time = smoke->time;
> -	struct wl_callback *callback;
> +	uint32_t time = widget_get_last_time(smoke->widget);
>  	cairo_surface_t *surface;
>  
>  	diffuse(smoke, time / 30, smoke->b[0].u, smoke->b[1].u);
> @@ -222,9 +204,7 @@ redraw_handler(struct widget *widget, void *data)
>  
>  	cairo_surface_destroy(surface);
>  
> -	callback = wl_surface_frame(window_get_wl_surface(smoke->window));
> -	wl_callback_add_listener(callback, &listener, smoke);
> -	wl_surface_commit(window_get_wl_surface(smoke->window));
> +	widget_schedule_redraw(smoke->widget);
>  }
>  
>  static void