[PATCH v2 3/3] Create a weston_surface_state structure for storing pending surface state and move the surface commit logic into weston_surface_commit_state

[Pekka Paalanen]

On Thu, 26 Jun 2014 11:19:05 -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 | 270 +++++++++++++++++++++++--------------------------------
>  src/compositor.h |  80 +++++++----------
>  2 files changed, 142 insertions(+), 208 deletions(-)

Nice diffstat. :-)

> diff --git a/src/compositor.c b/src/compositor.c
> index 4ccae79..be33a36 100644
> --- a/src/compositor.c
> +++ b/src/compositor.c
...
> @@ -389,6 +379,72 @@ struct weston_frame_callback {
>  	struct wl_list link;
>  };
>  
> +static void
> +surface_state_handle_buffer_destroy(struct wl_listener *listener, void *data)
> +{
> +	struct weston_surface_state *state =
> +		container_of(listener, struct weston_surface_state,
> +			     buffer_destroy_listener);
> +
> +	state->buffer = NULL;
> +}
> +
> +static void
> +weston_surface_state_init(struct weston_surface_state *state)
> +{
> +	state->newly_attached = 0;
> +	state->buffer = NULL;
> +	state->buffer_destroy_listener.notify =
> +		surface_state_handle_buffer_destroy;
> +	state->sx = 0;
> +	state->sy = 0;
> +
> +	pixman_region32_fini(&state->damage);
> +	pixman_region32_fini(&state->opaque);

fini? Should it not be init?

> +	region_init_infinite(&state->input);
> +
> +	wl_list_init(&state->frame_callback_list);
> +
> +	state->buffer_viewport.buffer.transform = WL_OUTPUT_TRANSFORM_NORMAL;
> +	state->buffer_viewport.buffer.scale = 1;
> +	state->buffer_viewport.buffer.src_width = wl_fixed_from_int(-1);
> +	state->buffer_viewport.surface.width = -1;
> +	state->buffer_viewport.changed = 0;
> +}
...
> @@ -2390,7 +2365,9 @@ 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,
> +		weston_surface_state_set_buffer(&sub->cached,
> +						surface->pending.buffer);
> +		weston_buffer_reference(&sub->cached_buffer_ref,
>  					surface->pending.buffer);

Alright, you solved it this way. Seems to work, I think.

>  	}
>  	sub->cached.sx += surface->pending.sx;
...
> @@ -2849,7 +2803,7 @@ weston_subsurface_create(uint32_t id, struct weston_surface *surface,
>  				       sub, subsurface_resource_destroy);
>  	weston_subsurface_link_surface(sub, surface);
>  	weston_subsurface_link_parent(sub, parent);
> -	weston_subsurface_cache_init(sub);
> +	weston_surface_state_init(&sub->cached);

Sould we explicitly init cached_buffer_ref too? For consistency and
doc value? I think I would prefer yes.

>  	sub->synchronized = 1;
>  
>  	return sub;
> diff --git a/src/compositor.h b/src/compositor.h
> index 06f8b03..bef5e1d 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -789,6 +789,32 @@ struct weston_view {
>  	uint32_t output_mask;
>  };
>  
> +struct weston_surface_state {
> +	/* 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;
> +};
> +
>  struct weston_surface {
>  	struct wl_resource *resource;
>  	struct wl_signal destroy_signal;
> @@ -833,31 +859,7 @@ 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;
>  
>  	/*
>  	 * 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;
>  
>  	int synchronized;
>  

The first two patches apply cleanly, but this third fails to apply,
maybe because the empty_region -> pixman_region32_clear patch.

If you agree with all my comments, you are welcome to have my
reviewed-by, and push the fixed series to master. Only patch 3
needs fixing.

Good work!


Thanks,
pq