[PATCH weston 2/2] Create a weston_surface_state structure for storing pending surface state and move the surface commit logic into weston_surface_commit_state
Pekka Paalanen
ppaalanen at gmail.com
Thu Jun 26 03:20:35 PDT 2014
On Fri, 20 Jun 2014 17:25:43 -0700
Jason Ekstrand <jason at jlekstrand.net> wrote:
> From: Jason Ekstrand <jason at jlekstrand.net>
>
> This new structure is used for both weston_surface.pending and
> weston_subsurface.cached.
>
> Signed-off-by: Jason Ekstrand <jason.ekstrand at intel.com>
> ---
> src/compositor.c | 151 +++++++++++++++++++------------------------------------
> src/compositor.h | 80 +++++++++++------------------
> 2 files changed, 81 insertions(+), 150 deletions(-)
>
> diff --git a/src/compositor.c b/src/compositor.c
> index fa8730f..66e8eee 100644
> --- a/src/compositor.c
> +++ b/src/compositor.c
> @@ -324,7 +324,7 @@ surface_handle_pending_buffer_destroy(struct wl_listener *listener, void *data)
> {
> struct weston_surface *surface =
> container_of(listener, struct weston_surface,
> - pending.buffer_destroy_listener);
> + pending_buffer_destroy_listener);
>
> surface->pending.buffer = NULL;
> }
> @@ -419,7 +419,7 @@ weston_surface_create(struct weston_compositor *compositor)
>
> wl_list_init(&surface->frame_callback_list);
>
> - surface->pending.buffer_destroy_listener.notify =
> + surface->pending_buffer_destroy_listener.notify =
> surface_handle_pending_buffer_destroy;
> pixman_region32_init(&surface->pending.damage);
> pixman_region32_init(&surface->pending.opaque);
> @@ -1356,7 +1356,7 @@ static void
> weston_surface_reset_pending_buffer(struct weston_surface *surface)
> {
> if (surface->pending.buffer)
> - wl_list_remove(&surface->pending.buffer_destroy_listener.link);
> + wl_list_remove(&surface->pending_buffer_destroy_listener.link);
> surface->pending.buffer = NULL;
> surface->pending.sx = 0;
> surface->pending.sy = 0;
> @@ -1420,7 +1420,7 @@ weston_surface_destroy(struct weston_surface *surface)
> pixman_region32_fini(&surface->pending.damage);
>
> if (surface->pending.buffer)
> - wl_list_remove(&surface->pending.buffer_destroy_listener.link);
> + wl_list_remove(&surface->pending_buffer_destroy_listener.link);
>
> weston_buffer_reference(&surface->buffer_ref, NULL);
>
> @@ -1947,7 +1947,7 @@ surface_attach(struct wl_client *client,
> /* Attach, attach, without commit in between does not send
> * wl_buffer.release. */
> if (surface->pending.buffer)
> - wl_list_remove(&surface->pending.buffer_destroy_listener.link);
> + wl_list_remove(&surface->pending_buffer_destroy_listener.link);
>
> surface->pending.sx = sx;
> surface->pending.sy = sy;
> @@ -1955,7 +1955,7 @@ surface_attach(struct wl_client *client,
> surface->pending.newly_attached = 1;
> if (buffer) {
> wl_signal_add(&buffer->destroy_signal,
> - &surface->pending.buffer_destroy_listener);
> + &surface->pending_buffer_destroy_listener);
> }
> }
>
> @@ -2055,7 +2055,8 @@ weston_surface_commit_subsurface_order(struct weston_surface *surface)
> }
>
> static void
> -weston_surface_commit(struct weston_surface *surface)
> +weston_surface_commit_state(struct weston_surface *surface,
> + struct weston_surface_state *state)
> {
> struct weston_view *view;
> pixman_region32_t opaque;
> @@ -2063,37 +2064,34 @@ weston_surface_commit(struct weston_surface *surface)
> /* wl_surface.set_buffer_transform */
> /* wl_surface.set_buffer_scale */
> /* wl_viewport.set */
> - surface->buffer_viewport = surface->pending.buffer_viewport;
> + surface->buffer_viewport = state->buffer_viewport;
>
> /* wl_surface.attach */
> - if (surface->pending.newly_attached)
> - weston_surface_attach(surface, surface->pending.buffer);
> + if (state->newly_attached)
> + weston_surface_attach(surface, state->buffer);
> + state->buffer = NULL;
>
> - if (surface->pending.newly_attached ||
> - surface->pending.buffer_viewport.changed) {
> + if (state->newly_attached || state->buffer_viewport.changed) {
> weston_surface_update_size(surface);
> if (surface->configure)
> - surface->configure(surface, surface->pending.sx,
> - surface->pending.sy);
> + surface->configure(surface, state->sx, state->sy);
> }
>
> - weston_surface_reset_pending_buffer(surface);
> + state->sx = 0;
> + state->sy = 0;
> + state->newly_attached = 0;
Missed resetting viewport-changed?
>
> /* wl_surface.damage */
> pixman_region32_union(&surface->damage, &surface->damage,
> - &surface->pending.damage);
> + &state->damage);
> pixman_region32_intersect_rect(&surface->damage, &surface->damage,
> - 0, 0,
> - surface->width,
> - surface->height);
> - empty_region(&surface->pending.damage);
> + 0, 0, surface->width, surface->height);
> + pixman_region32_clear(&state->damage);
pixman_region32_clear() was added in version 0.25.2.
Funny that we don't have any version requirements in configure.ac.
Maybe we should...
Getting rid of empty_region() everywhere could be another patch.
>
> /* wl_surface.set_opaque_region */
> - pixman_region32_init_rect(&opaque, 0, 0,
> - surface->width,
> - surface->height);
> - pixman_region32_intersect(&opaque,
> - &opaque, &surface->pending.opaque);
> + pixman_region32_init(&opaque);
> + pixman_region32_intersect_rect(&opaque, &state->opaque,
> + 0, 0, surface->width, surface->height);
>
> if (!pixman_region32_equal(&opaque, &surface->opaque)) {
> pixman_region32_copy(&surface->opaque, &opaque);
> @@ -2104,17 +2102,25 @@ weston_surface_commit(struct weston_surface *surface)
> pixman_region32_fini(&opaque);
>
> /* wl_surface.set_input_region */
> - pixman_region32_fini(&surface->input);
> - pixman_region32_init_rect(&surface->input, 0, 0,
> - surface->width,
> - surface->height);
> - pixman_region32_intersect(&surface->input,
> - &surface->input, &surface->pending.input);
> + pixman_region32_intersect_rect(&surface->input, &state->input,
> + 0, 0, surface->width, surface->height);
Heh, nice simplification.
>
> /* wl_surface.frame */
> wl_list_insert_list(&surface->frame_callback_list,
> - &surface->pending.frame_callback_list);
> - wl_list_init(&surface->pending.frame_callback_list);
> + &state->frame_callback_list);
> + wl_list_init(&state->frame_callback_list);
> +}
> +
> +static void
> +weston_surface_commit(struct weston_surface *surface)
> +{
> + /* Get rid of the pending buffer destroy listener. After
> + * weston_surface_commit_state is called, surface->pending.buffer
> + * will be NULL. */
> + if (surface->pending.buffer)
> + wl_list_remove(&surface->pending_buffer_destroy_listener.link);
> +
> + weston_surface_commit_state(surface, &surface->pending);
While correct, this order of operations is slightly odd.
>
> weston_surface_commit_subsurface_order(surface);
>
> @@ -2302,73 +2308,15 @@ static void
> weston_subsurface_commit_from_cache(struct weston_subsurface *sub)
> {
> struct weston_surface *surface = sub->surface;
> - struct weston_view *view;
> - pixman_region32_t opaque;
> -
> - /* wl_surface.set_buffer_transform */
> - /* wl_surface.set_buffer_scale */
> - /* wl_viewport.set */
> - surface->buffer_viewport = sub->cached.buffer_viewport;
> -
> - /* wl_surface.attach */
> - if (sub->cached.newly_attached)
> - weston_surface_attach(surface, sub->cached.buffer_ref.buffer);
> - weston_buffer_reference(&sub->cached.buffer_ref, NULL);
>
> - if (sub->cached.newly_attached || sub->cached.buffer_viewport.changed) {
> - weston_surface_update_size(surface);
> - if (surface->configure)
> - surface->configure(surface, sub->cached.sx,
> - sub->cached.sy);
> - }
> -
> - sub->cached.sx = 0;
> - sub->cached.sy = 0;
> - sub->cached.newly_attached = 0;
> - sub->cached.buffer_viewport.changed = 0;
> -
> - /* wl_surface.damage */
> - pixman_region32_union(&surface->damage, &surface->damage,
> - &sub->cached.damage);
> - pixman_region32_intersect_rect(&surface->damage, &surface->damage,
> - 0, 0,
> - surface->width,
> - surface->height);
> - empty_region(&sub->cached.damage);
> -
> - /* wl_surface.set_opaque_region */
> - pixman_region32_init_rect(&opaque, 0, 0,
> - surface->width,
> - surface->height);
> - pixman_region32_intersect(&opaque,
> - &opaque, &sub->cached.opaque);
> -
> - if (!pixman_region32_equal(&opaque, &surface->opaque)) {
> - pixman_region32_copy(&surface->opaque, &opaque);
> - wl_list_for_each(view, &surface->views, surface_link)
> - weston_view_geometry_dirty(view);
> - }
> -
> - pixman_region32_fini(&opaque);
> -
> - /* wl_surface.set_input_region */
> - pixman_region32_fini(&surface->input);
> - pixman_region32_init_rect(&surface->input, 0, 0,
> - surface->width,
> - surface->height);
> - pixman_region32_intersect(&surface->input,
> - &surface->input, &sub->cached.input);
> -
> - /* wl_surface.frame */
> - wl_list_insert_list(&surface->frame_callback_list,
> - &sub->cached.frame_callback_list);
> - wl_list_init(&sub->cached.frame_callback_list);
> + weston_surface_commit_state(surface, &sub->cached);
> + weston_buffer_reference(&sub->cached_buffer_ref, NULL);
If the client somehow destroyed the cached wl_buffer before we end up
here, isn't weston_surface_commit_state() using a stale pointer now?
I'd propose passing the weston_buffer to weston_surface_commit_state()
as argument, and removing it from the state struct.
It will be slightly awkward, because the argument will be either used
or not based on the state struct, but at least it should work.
>
> weston_surface_commit_subsurface_order(surface);
>
> weston_surface_schedule_repaint(surface);
>
> - sub->cached.has_data = 0;
> + sub->has_cached_data = 0;
> }
>
> static void
> @@ -2390,7 +2338,8 @@ weston_subsurface_commit_to_cache(struct weston_subsurface *sub)
>
> if (surface->pending.newly_attached) {
> sub->cached.newly_attached = 1;
> - weston_buffer_reference(&sub->cached.buffer_ref,
> + sub->cached.buffer = surface->pending.buffer;
> + weston_buffer_reference(&sub->cached_buffer_ref,
> surface->pending.buffer);
> }
> sub->cached.sx += surface->pending.sx;
> @@ -2413,7 +2362,7 @@ weston_subsurface_commit_to_cache(struct weston_subsurface *sub)
> &surface->pending.frame_callback_list);
> wl_list_init(&surface->pending.frame_callback_list);
>
> - sub->cached.has_data = 1;
> + sub->has_cached_data = 1;
> }
>
> static int
> @@ -2442,7 +2391,7 @@ weston_subsurface_commit(struct weston_subsurface *sub)
> if (weston_subsurface_is_synchronized(sub)) {
> weston_subsurface_commit_to_cache(sub);
> } else {
> - if (sub->cached.has_data) {
> + if (sub->has_cached_data) {
> /* flush accumulated state from cache */
> weston_subsurface_commit_to_cache(sub);
> weston_subsurface_commit_from_cache(sub);
> @@ -2469,7 +2418,7 @@ weston_subsurface_synchronized_commit(struct weston_subsurface *sub)
> * all the way down.
> */
>
> - if (sub->cached.has_data)
> + if (sub->has_cached_data)
> weston_subsurface_commit_from_cache(sub);
>
> wl_list_for_each(tmp, &surface->subsurface_list, parent_link) {
> @@ -2681,7 +2630,8 @@ weston_subsurface_cache_init(struct weston_subsurface *sub)
> pixman_region32_init(&sub->cached.opaque);
> pixman_region32_init(&sub->cached.input);
> wl_list_init(&sub->cached.frame_callback_list);
> - sub->cached.buffer_ref.buffer = NULL;
> + sub->cached.buffer = NULL;
> + sub->cached_buffer_ref.buffer = NULL;
> }
>
> static void
> @@ -2692,7 +2642,8 @@ weston_subsurface_cache_fini(struct weston_subsurface *sub)
> wl_list_for_each_safe(cb, tmp, &sub->cached.frame_callback_list, link)
> wl_resource_destroy(cb->resource);
>
> - weston_buffer_reference(&sub->cached.buffer_ref, NULL);
> + sub->cached.buffer = NULL;
> + weston_buffer_reference(&sub->cached_buffer_ref, NULL);
> pixman_region32_fini(&sub->cached.damage);
> pixman_region32_fini(&sub->cached.opaque);
> pixman_region32_fini(&sub->cached.input);
> diff --git a/src/compositor.h b/src/compositor.h
> index 06f8b03..b15be69 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -789,6 +789,31 @@ struct weston_view {
> uint32_t output_mask;
> };
>
> +struct weston_surface_state {
> + /* wl_surface.attach */
> + int newly_attached;
> + struct weston_buffer *buffer;
Since the destroy listener is not here, I'd probably like it more if
the buffer was not here either. I like keeping the pointer and the
listener very close together.
> + int32_t sx;
> + int32_t sy;
> +
> + /* wl_surface.damage */
> + pixman_region32_t damage;
> +
> + /* wl_surface.set_opaque_region */
> + pixman_region32_t opaque;
> +
> + /* wl_surface.set_input_region */
> + pixman_region32_t input;
> +
> + /* wl_surface.frame */
> + struct wl_list frame_callback_list;
> +
> + /* wl_surface.set_buffer_transform */
> + /* wl_surface.set_scaling_factor */
> + /* wl_viewport.set */
> + struct weston_buffer_viewport buffer_viewport;
> +};
> +
> struct weston_surface {
> struct wl_resource *resource;
> struct wl_signal destroy_signal;
> @@ -833,31 +858,8 @@ struct weston_surface {
> struct wl_resource *viewport_resource;
>
> /* All the pending state, that wl_surface.commit will apply. */
> - struct {
> - /* wl_surface.attach */
> - int newly_attached;
> - struct weston_buffer *buffer;
> - struct wl_listener buffer_destroy_listener;
> - int32_t sx;
> - int32_t sy;
> -
> - /* wl_surface.damage */
> - pixman_region32_t damage;
> -
> - /* wl_surface.set_opaque_region */
> - pixman_region32_t opaque;
> -
> - /* wl_surface.set_input_region */
> - pixman_region32_t input;
> -
> - /* wl_surface.frame */
> - struct wl_list frame_callback_list;
> -
> - /* wl_surface.set_buffer_transform */
> - /* wl_surface.set_scaling_factor */
> - /* wl_viewport.set */
> - struct weston_buffer_viewport buffer_viewport;
> - } pending;
> + struct weston_surface_state pending;
> + struct wl_listener pending_buffer_destroy_listener;
>
> /*
> * If non-NULL, this function will be called on
> @@ -895,31 +897,9 @@ struct weston_subsurface {
> int set;
> } position;
>
> - struct {
> - int has_data;
> -
> - /* wl_surface.attach */
> - int newly_attached;
> - struct weston_buffer_reference buffer_ref;
> - int32_t sx;
> - int32_t sy;
> -
> - /* wl_surface.damage */
> - pixman_region32_t damage;
> -
> - /* wl_surface.set_opaque_region */
> - pixman_region32_t opaque;
> -
> - /* wl_surface.set_input_region */
> - pixman_region32_t input;
> -
> - /* wl_surface.frame */
> - struct wl_list frame_callback_list;
> -
> - /* wl_surface.set_buffer_transform */
> - /* wl_surface.set_buffer_scale */
> - struct weston_buffer_viewport buffer_viewport;
> - } cached;
> + int has_cached_data;
> + struct weston_surface_state cached;
> + struct weston_buffer_reference cached_buffer_ref;
There is possibility that cached_buffer_ref and cached.buffer disagree.
That is another reason to not have buffer in cached. These two use
cases need different handling, because the pending buffer is not yet
"referenced", but that you took quite nicely into consideration.
>
> int synchronized;
>
Other than that, I like this change, and it looks otherwise correct.
Thanks,
pq
More information about the wayland-devel
mailing list