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

[Giulio Camuffo]

2013/12/11 Pekka Paalanen <ppaalanen at gmail.com>:
> On Wed, 11 Dec 2013 11:23:50 +0100
> Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
>
>> 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.
>
> No other mapped surfaces at all, or surfaces that are just not
> updating? I noticed some comment in irc about updating.

Right, there can be other mapped surfaces as long as they don't
update. If they update weston_output_repaint() is called, and that
will call update_transform() on also the view of the other surface.

>
>> > 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.
>
> There are two essentially different cases for the configure handler:
> 1. if surface is not mapped yet, ->configure() maps it
> 2. if surface is mapped, ->configure() configures it
>
> For mapping case, AFAIK we have always relied on the ->configure()
> ending up calling weston_view_update_transform() directly, to force the
> surface become mapped (which is somehow strangely equivalent to
> assigning an output). Previously it was
> weston_surface_update_transform() and before that IIRC we called
> weston_surface_assign_output() directly or something.
>
> For the second case, ->configure() must not call
> weston_view_update_transform() to avoid updating the surface state
> prematurely. Instead, it ends up calling weston_view_geometry_dirty().
> In this case, the surface state gets updated when it gets painted onto
> an output. If the surface is not on an output, there is no need to
> update its state either.
>
> Or, that is how it used to be, a long time ago. I believe there was a
> short discussion at the time whether we'd need a ->map() hook in
> addition to ->configure(), but it was deemed not useful. Or maybe we
> even had a ->map() and it got removed, can't recall.
>
> That is why the shell.c code had traces of two different code paths I
> think, one for map and the other for configure, both originating in the
> ->configure() callback.

The problem is how to find out which case it is in the configure
handler. My surface is not a wl_shell_surface, and i get it with a
request which also sends an output. So i set the surface's output to
the given one immediately, and weston_surface_is_mapped() in the
configure handler returns always true even if output_mask is still 0.
Changing the condition in weston_surface_is_mapped() to "if
(surface->output && (surface->output_mask & (1 <<
surface->output->id)))" would fix it, though. Otherwise I would need
to wrap my surface in a struct and use that to store the output and
assign it to the surface later, but that add complexity where it is
not needed, really.

>
>
> Thanks,
> pq
>
>> > 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
>