[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

Sun Jul 6 04:16:42 PDT 2014

On 2014年6月26日 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