[PATCH v5] compositor: call configure on surfaces with a null buffer too
Giulio Camuffo
giuliocamuffo at gmail.com
Thu Feb 21 02:27:18 PST 2013
2013/2/21 Pekka Paalanen <ppaalanen at gmail.com>:
> On Wed, 20 Feb 2013 16:28:48 +0100
> Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
>
>> This way the shell can know when a surface has been unmapped by
>> checking the value returned by weston_surface_is_mapped(surface).
>> The configure handlers have now width and height parameters, so
>> they do not need anymore to check manually the buffer size.
>> If a surface's buffer is NULL the width and height passed to the
>> configure are both 0.
>> Configure is now only called after an attach. The variable
>> weston_surface.pending.newly_attached is set to 1 on attach, and
>> after the configure call is reset to 0.
>> ---
>> src/compositor.c | 37 ++++++++++++++++++++++++-------------
>> src/compositor.h | 4 ++--
>> src/shell.c | 48 +++++++++++++++++++++++++++++-------------------
>> src/tablet-shell.c | 8 ++------
>> tests/weston-test.c | 6 +++---
>> 5 files changed, 60 insertions(+), 43 deletions(-)
>>
>> diff --git a/src/compositor.c b/src/compositor.c
>> index 64d0830..4f3c443 100644
>> --- a/src/compositor.c
>> +++ b/src/compositor.c
>> @@ -290,6 +290,7 @@ weston_surface_create(struct weston_compositor *compositor)
>> surface->pending.buffer_transform = surface->buffer_transform;
>> surface->output = NULL;
>> surface->plane = &compositor->primary_plane;
>> + surface->pending.newly_attached = 0;
>>
>> pixman_region32_init(&surface->damage);
>> pixman_region32_init(&surface->opaque);
>> @@ -1273,12 +1274,10 @@ surface_attach(struct wl_client *client,
>> surface->pending.sx = sx;
>> surface->pending.sy = sy;
>> surface->pending.buffer = buffer;
>> + surface->pending.newly_attached = 1;
>> if (buffer) {
>> wl_signal_add(&buffer->resource.destroy_signal,
>> &surface->pending.buffer_destroy_listener);
>> - surface->pending.remove_contents = 0;
>> - } else {
>> - surface->pending.remove_contents = 1;
>> }
>> }
>>
>> @@ -1391,6 +1390,8 @@ surface_commit(struct wl_client *client, struct wl_resource *resource)
>> {
>> struct weston_surface *surface = resource->data;
>> pixman_region32_t opaque;
>> + int buffer_width = 0;
>> + int buffer_height = 0;
>>
>> if (surface->pending.sx || surface->pending.sy ||
>> (surface->pending.buffer &&
>> @@ -1401,14 +1402,22 @@ surface_commit(struct wl_client *client, struct wl_resource *resource)
>> surface->buffer_transform = surface->pending.buffer_transform;
>>
>> /* wl_surface.attach */
>> - if (surface->pending.buffer || surface->pending.remove_contents)
>> + if (surface->pending.buffer ||
>> + (surface->pending.newly_attached && surface->pending.buffer == NULL))
>
> I think you can drop the "&& surface->pending.buffer == NULL".
Doh, you're right.
>
>> weston_surface_attach(surface, surface->pending.buffer);
>>
>> - if (surface->buffer_ref.buffer && surface->configure)
>> + if (surface->buffer_ref.buffer) {
>> + buffer_width = weston_surface_buffer_width(surface);
>> + buffer_height = weston_surface_buffer_height(surface);
>> + }
>> +
>> + if (surface->configure && surface->pending.newly_attached)
>> surface->configure(surface, surface->pending.sx,
>> - surface->pending.sy);
>> + surface->pending.sy, buffer_width, buffer_height);
>> +
>> surface->pending.sx = 0;
>> surface->pending.sy = 0;
>> + surface->pending.newly_attached = 0;
>
> Yes, the logic looks just like I would expect, nice.
>
>>
>> /* wl_surface.damage */
>> pixman_region32_union(&surface->damage, &surface->damage,
>> @@ -2144,11 +2153,14 @@ pointer_handle_sprite_destroy(struct wl_listener *listener, void *data)
>>
>> static void
>> pointer_cursor_surface_configure(struct weston_surface *es,
>> - int32_t dx, int32_t dy)
>> + int32_t dx, int32_t dy, int32_t width, int32_t height)
>> {
>> struct weston_seat *seat = es->private;
>> int x, y;
>>
>> + if (width == 0)
>> + return;
>
> Hmm, I wonder, would it actually fix a bug if we did not test for
> width==0, but instead just executed this?
I'd expect it to segfault somewhere later by some code that tries to
use some surface->buffer_ref.buffer thing.
>
> I recall that before this patch, configure was not called, if a client
> explicitly attached a NULL buffer, right? That means we would just
> ignore dx,dy, and that is fine since they usually are zero anyway.
> However, would it not be more correct to actually handle dx,dy
> regardless?
Well, I don't know, you tell me that. ;)
>
> Anyway, don't change that in this patch, it should be a separate patch
> that just removes the width==0 check, if we want it. This patch should
> just keep all existing logic as close as possible to what it was.
>
>> +
>> assert(es == seat->sprite);
>>
>> seat->hotspot_x -= dx;
>> @@ -2158,8 +2170,7 @@ pointer_cursor_surface_configure(struct weston_surface *es,
>> y = wl_fixed_to_int(seat->seat.pointer->y) - seat->hotspot_y;
>>
>> weston_surface_configure(seat->sprite, x, y,
>> - es->buffer_ref.buffer->width,
>> - es->buffer_ref.buffer->height);
>> + width, height);
>>
>> empty_region(&es->pending.input);
>>
>> @@ -2226,7 +2237,8 @@ pointer_set_cursor(struct wl_client *client, struct wl_resource *resource,
>> seat->hotspot_y = y;
>>
>> if (surface->buffer_ref.buffer)
>> - pointer_cursor_surface_configure(surface, 0, 0);
>> + pointer_cursor_surface_configure(surface, 0, 0, weston_surface_buffer_width(surface),
>> + weston_surface_buffer_height(surface));
>> }
>>
>> static const struct wl_pointer_interface pointer_interface = {
>> @@ -2604,14 +2616,13 @@ weston_seat_release(struct weston_seat *seat)
>> }
>>
>> static void
>> -drag_surface_configure(struct weston_surface *es, int32_t sx, int32_t sy)
>> +drag_surface_configure(struct weston_surface *es, int32_t sx, int32_t sy, int32_t width, int32_t height)
>> {
>> empty_region(&es->pending.input);
>>
>> weston_surface_configure(es,
>> es->geometry.x + sx, es->geometry.y + sy,
>> - es->buffer_ref.buffer->width,
>> - es->buffer_ref.buffer->height);
>> + width, height);
>> }
>>
>> static int
>> diff --git a/src/compositor.h b/src/compositor.h
>> index fdde762..03d1031 100644
>> --- a/src/compositor.h
>> +++ b/src/compositor.h
>> @@ -460,7 +460,7 @@ struct weston_surface {
>> /* All the pending state, that wl_surface.commit will apply. */
>> struct {
>> /* wl_surface.attach */
>> - int remove_contents;
>> + int newly_attached;
>
> Yes! This is the change I have wanted to do for some time now. :-)
>
>> struct wl_buffer *buffer;
>> struct wl_listener buffer_destroy_listener;
>> int32_t sx;
>> @@ -487,7 +487,7 @@ struct weston_surface {
>> * a new buffer has been set up for this surface. The integer params
>> * are the sx and sy paramerters supplied to surface::attach .
>> */
>> - void (*configure)(struct weston_surface *es, int32_t sx, int32_t sy);
>> + void (*configure)(struct weston_surface *es, int32_t sx, int32_t sy, int32_t width, int32_t height);
>> void *private;
>> };
>>
>> diff --git a/src/shell.c b/src/shell.c
>> index af802a5..e355aa0 100644
>> --- a/src/shell.c
>> +++ b/src/shell.c
>> @@ -1627,7 +1627,7 @@ shell_surface_set_maximized(struct wl_client *client,
>> }
>>
>> static void
>> -black_surface_configure(struct weston_surface *es, int32_t sx, int32_t sy);
>> +black_surface_configure(struct weston_surface *es, int32_t sx, int32_t sy, int32_t width, int32_t height);
>>
>> static struct weston_surface *
>> create_black_surface(struct weston_compositor *ec,
>> @@ -2023,7 +2023,7 @@ shell_handle_surface_destroy(struct wl_listener *listener, void *data)
>> }
>>
>> static void
>> -shell_surface_configure(struct weston_surface *, int32_t, int32_t);
>> +shell_surface_configure(struct weston_surface *, int32_t, int32_t, int32_t, int32_t);
>>
>> static struct shell_surface *
>> get_shell_surface(struct weston_surface *surface)
>> @@ -2172,10 +2172,13 @@ terminate_screensaver(struct desktop_shell *shell)
>> }
>>
>> static void
>> -configure_static_surface(struct weston_surface *es, struct weston_layer *layer)
>> +configure_static_surface(struct weston_surface *es, struct weston_layer *layer, int32_t width, int32_t height)
>> {
>> struct weston_surface *s, *next;
>>
>> + if (width == 0)
>> + return;
>> +
>> wl_list_for_each_safe(s, next, &layer->surface_list, layer_link) {
>> if (s->output == es->output && s != es) {
>> weston_surface_unmap(s);
>> @@ -2183,9 +2186,7 @@ configure_static_surface(struct weston_surface *es, struct weston_layer *layer)
>> }
>> }
>>
>> - weston_surface_configure(es, es->output->x, es->output->y,
>> - weston_surface_buffer_width(es),
>> - weston_surface_buffer_height(es));
>> + weston_surface_configure(es, es->output->x, es->output->y, width, height);
>>
>> if (wl_list_empty(&es->layer_link)) {
>> wl_list_insert(&layer->surface_list, &es->layer_link);
>> @@ -2194,11 +2195,11 @@ configure_static_surface(struct weston_surface *es, struct weston_layer *layer)
>> }
>>
>> static void
>> -background_configure(struct weston_surface *es, int32_t sx, int32_t sy)
>> +background_configure(struct weston_surface *es, int32_t sx, int32_t sy, int32_t width, int32_t height)
>> {
>> struct desktop_shell *shell = es->private;
>>
>> - configure_static_surface(es, &shell->background_layer);
>> + configure_static_surface(es, &shell->background_layer, width, height);
>> }
>>
>> static void
>> @@ -2227,11 +2228,11 @@ desktop_shell_set_background(struct wl_client *client,
>> }
>>
>> static void
>> -panel_configure(struct weston_surface *es, int32_t sx, int32_t sy)
>> +panel_configure(struct weston_surface *es, int32_t sx, int32_t sy, int32_t width, int32_t height)
>> {
>> struct desktop_shell *shell = es->private;
>>
>> - configure_static_surface(es, &shell->panel_layer);
>> + configure_static_surface(es, &shell->panel_layer, width, height);
>> }
>>
>> static void
>> @@ -2260,10 +2261,13 @@ desktop_shell_set_panel(struct wl_client *client,
>> }
>>
>> static void
>> -lock_surface_configure(struct weston_surface *surface, int32_t sx, int32_t sy)
>> +lock_surface_configure(struct weston_surface *surface, int32_t sx, int32_t sy, int32_t width, int32_t height)
>> {
>> struct desktop_shell *shell = surface->private;
>>
>> + if (width == 0)
>> + return;
>> +
>> center_on_output(surface, get_default_output(shell->compositor));
>>
>> if (!weston_surface_is_mapped(surface)) {
>> @@ -2718,7 +2722,7 @@ activate(struct desktop_shell *shell, struct weston_surface *es,
>>
>> /* no-op func for checking black surface */
>> static void
>> -black_surface_configure(struct weston_surface *es, int32_t sx, int32_t sy)
>> +black_surface_configure(struct weston_surface *es, int32_t sx, int32_t sy, int32_t width, int32_t height)
>> {
>> }
>>
>> @@ -3084,14 +3088,16 @@ configure(struct desktop_shell *shell, struct weston_surface *surface,
>> }
>>
>> static void
>> -shell_surface_configure(struct weston_surface *es, int32_t sx, int32_t sy)
>> +shell_surface_configure(struct weston_surface *es, int32_t sx, int32_t sy, int32_t width, int32_t height)
>> {
>> struct shell_surface *shsurf = get_shell_surface(es);
>> struct desktop_shell *shell = shsurf->shell;
>> - int32_t width = weston_surface_buffer_width(es);
>> - int32_t height = weston_surface_buffer_height(es);
>> +
>> int type_changed = 0;
>>
>> + if (width == 0)
>> + return;
>> +
>> if (shsurf->next_type != SHELL_SURFACE_NONE &&
>> shsurf->type != shsurf->next_type) {
>> set_surface_type(shsurf);
>> @@ -3204,10 +3210,13 @@ bind_desktop_shell(struct wl_client *client,
>> }
>>
>> static void
>> -screensaver_configure(struct weston_surface *surface, int32_t sx, int32_t sy)
>> +screensaver_configure(struct weston_surface *surface, int32_t sx, int32_t sy, int32_t width, int32_t height)
>> {
>> struct desktop_shell *shell = surface->private;
>>
>> + if (width == 0)
>> + return;
>> +
>> /* XXX: starting weston-screensaver beforehand does not work */
>> if (!shell->locked)
>> return;
>> @@ -3275,13 +3284,14 @@ bind_screensaver(struct wl_client *client,
>> }
>>
>> static void
>> -input_panel_configure(struct weston_surface *surface, int32_t sx, int32_t sy)
>> +input_panel_configure(struct weston_surface *surface, int32_t sx, int32_t sy, int32_t width, int32_t height)
>> {
>> struct weston_mode *mode;
>> - int32_t width = weston_surface_buffer_width(surface);
>> - int32_t height = weston_surface_buffer_height(surface);
>> float x, y;
>>
>> + if (width == 0)
>> + return;
>> +
>> if (!weston_surface_is_mapped(surface))
>> return;
>>
>> diff --git a/src/tablet-shell.c b/src/tablet-shell.c
>> index 9e88e27..66b1a44 100644
>> --- a/src/tablet-shell.c
>> +++ b/src/tablet-shell.c
>> @@ -124,17 +124,13 @@ tablet_shell_set_state(struct tablet_shell *shell, int state)
>>
>> static void
>> tablet_shell_surface_configure(struct weston_surface *surface,
>> - int32_t sx, int32_t sy)
>> + int32_t sx, int32_t sy, int32_t width, int32_t height)
>> {
>> struct tablet_shell *shell = get_shell(surface->compositor);
>> - int32_t width, height;
>>
>> - if (weston_surface_is_mapped(surface))
>> + if (weston_surface_is_mapped(surface) || width == 0)
>> return;
>>
>> - width = surface->buffer_ref.buffer->width;
>> - height = surface->buffer_ref.buffer->height;
>
> Heh, I think that actually fixes a bug in there.
>
>> -
>> weston_surface_configure(surface, 0, 0, width, height);
>>
>> if (surface == shell->lockscreen_surface) {
>> diff --git a/tests/weston-test.c b/tests/weston-test.c
>> index a9def4b..7655d55 100644
>> --- a/tests/weston-test.c
>> +++ b/tests/weston-test.c
>> @@ -74,7 +74,7 @@ notify_pointer_position(struct weston_test *test, struct wl_resource *resource)
>> }
>>
>> static void
>> -test_surface_configure(struct weston_surface *surface, int32_t sx, int32_t sy)
>> +test_surface_configure(struct weston_surface *surface, int32_t sx, int32_t sy, int32_t width, int32_t height)
>> {
>> struct weston_test_surface *test_surface = surface->private;
>> struct weston_test *test = test_surface->test;
>> @@ -85,8 +85,8 @@ test_surface_configure(struct weston_surface *surface, int32_t sx, int32_t sy)
>>
>> surface->geometry.x = test_surface->x;
>> surface->geometry.y = test_surface->y;
>> - surface->geometry.width = surface->buffer_ref.buffer->width;
>> - surface->geometry.height = surface->buffer_ref.buffer->height;
>> + surface->geometry.width = width;
>> + surface->geometry.height = height;
>
> Ooh, and here, this one was missing the transformed buffer handling,
> too, but now it looks like it's fixed.
>
>> surface->geometry.dirty = 1;
>>
>> if (!weston_surface_is_mapped(surface))
>
> Good job! Only one tiny nitpick in the whole patch. :-)
> I didn't try to run it, but it looks fine.
>
>
> Thanks,
> pq
Giulio
More information about the wayland-devel
mailing list