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

[Pekka Paalanen]

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.

> > 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.


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