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

[Pekka Paalanen]

On Wed, 11 Dec 2013 12:20:41 +0100
Giulio Camuffo <giuliocamuffo at gmail.com> wrote:

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

I guess output->repaint_needed never gets set, then. If output_mask is
zero, weston_{surface,view}_schedule_repaint() functions never call
weston_output_schedule_repaint(). I see.

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

Yeah, I think the solution to that is to make
weston_surface_is_mapped() more sane, and stop relying on the output
assignment... or make it only look at output_mask. Not sure. So many
incremental changes have piled up, that it might be time to clean it up
a bit. And what about "surface is in a layer" vs. "surface is mapped"...

weston_surface_is_mapped() existed before output_mask, and you were not
supposed to set weston_surface::output yourself. Setting it was the job
of weston_surface_assign_output() or similar, which computed the output
based on the surface geometry. So yes, it was sort of expected, that
you always have your own struct to go with a weston_surface, if you
need associated data like the intended output. That is what the
configure_private is for.

I can't say which way would be the way to go.


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