[PATCH weston] compositor: update the transform when attaching a new buffer

[Giulio Camuffo]

2013/12/11 Pekka Paalanen <ppaalanen at gmail.com>:
> On Tue, 10 Dec 2013 15:55:29 +0100
> Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
>
>> if a surface has not a buffer yet and a weston_view gets created for
>> it, the surface's width and height will be 0 and the view's
>> output_mask will be 0, because the surface's area is 0. Later commits
>> on the surface with valid buffers will then trigger a surface
>> repaint, which will do nothing because of the output_mask set to 0.
>> So by calling weston_update_transform() on the views of the surface
>> at the end of weston_surface_commit() we update the output_mask of
>> the surface and of the views, so they will be repainted.
>> ---
>>  src/compositor.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/compositor.c b/src/compositor.c
>> index 6beea9c..20d9efb 100644
>> --- a/src/compositor.c
>> +++ b/src/compositor.c
>> @@ -2005,7 +2005,6 @@ weston_surface_commit(struct weston_surface
>> *surface) surface->pending.buffer = NULL;
>>       surface->pending.sx = 0;
>>       surface->pending.sy = 0;
>> -     surface->pending.newly_attached = 0;
>>
>>       /* wl_surface.damage */
>>       pixman_region32_union(&surface->damage, &surface->damage,
>> @@ -2046,7 +2045,13 @@ weston_surface_commit(struct weston_surface
>> *surface)
>>       weston_surface_commit_subsurface_order(surface);
>>
>> +     if (surface->pending.newly_attached) {
>> +             wl_list_for_each(view, &surface->views, surface_link)
>> +                     weston_view_update_transform(view);
>> +     }
>>       weston_surface_schedule_repaint(surface);
>> +
>> +     surface->pending.newly_attached = 0;
>>  }
>>
>>  static void
>
> Hi,
>
> Why is this fix needed in the first place, in what use case does this
> bug manifest? Is this a regression? What introduced it?

It isn't a regression. I noticed it now because it happens when there
are no other mapped surfaces on the same output.

>
> Weren't we relying on the weston_surface->configure call that maps the
> surface the first time to enforce a non-zero output_mask?

You mean the configure handler should call weston_update_transform? It
seems to me the handlers should call weston_view_geometry_dirty() if
needed, and then weston_update_transform() should be called by weston
itself (which is what happens, usually). The problem here is that it
wasn't called, ever.

>
> If you look at all (most?) of the functions in shell.c that actually
> map surfaces, starting from an unmapped state, you see that they a) put
> the view into a layer, and b) forcefully call
> weston_view_update_transform(), which will then set up the output_mask.
> Or at least it used to be like that, now the only case I can clearly
> see in the code is lock_surface_configure(), but looks like map() with
> at least shell_map_popup() works roughly the same.
>
> There are likely also other details that have to be right in the right
> order when a surface is mapped, but I forget. Every surface type gets
> mapped slightly differently which is why we have not had a generic
> weston_surface_map() function.
>
> I don't think calling weston_view_update_transform() from the commit
> handler *always* when there is a new attach is right. It should happen
> only when mapping a surface. I believe there used to be the idea, that
> e.g. input events are generated with the old surface state, until
> output_repaint actually puts the new state on screen. Though, is that
> idea relevant anymore?

I guess it could call it only if there wasn't a buffer before, if this
is still relevant.

>
> (That actually shows a bug I think, a newly mapped surface may get input
> events before it is painted on screen, does it not? No wait, we only do
> input /from/ output_repaint? or do we? umm...)
>
>
> Thanks,
> pq