[PATCH weston 2/2] compositor: surface and view output comment fixes

[Pekka Paalanen]

On Mon, 16 May 2016 23:02:42 +0800
Jonas Ådahl <jadahl at gmail.com> wrote:

> On Mon, May 16, 2016 at 04:45:05PM +0200, Armin Krezović wrote:
> > On 10.05.2016 16:10, Pekka Paalanen wrote:  
> > > From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > > 
> > > weston_surface::output and weston_view::output as used for different
> > > purposes. Only the surface output is used for frame callbacks.
> > > 
> > > The uses of the view output are much more vague and hard to describe.
> > > 
> > > Also fix a comment mistake in weston_surface_assign_output().
> > >   
> > 
> > All the changes look fine to me. I have one comment below, as I'm not sure
> > if it's a mistake or not.
> > 
> > In case it isn't a mistake (or with the mistake fixed, in case it is one),
> > this patch is also:
> > 
> > Reviewed-by: Armin Krezović <krezovic.armin at gmail.com>
> >   
> > > Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > > ---
> > >  src/compositor.c | 8 +++-----
> > >  src/compositor.h | 5 +++--
> > >  2 files changed, 6 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/src/compositor.c b/src/compositor.c
> > > index ee47a82..40d8baf 100644
> > > --- a/src/compositor.c
> > > +++ b/src/compositor.c
> > > @@ -1082,16 +1082,15 @@ weston_surface_update_output_mask(struct weston_surface *es, uint32_t mask)
> > >  	}
> > >  }
> > >  
> > > -
> > >  /** Recalculate which output(s) the surface has views displayed on
> > >   *
> > >   * \param es  The surface to remap to outputs
> > >   *
> > >   * Finds the output that is showing the largest amount of one
> > >   * of the surface's various views.  This output becomes the
> > > - * surface's primary output for vsync and frame event purposes.
> > > + * surface's primary output for vsync and frame callback purposes.
> > >   *
> > > - * Also notes the primary outputs of all of the surface's views
> > > + * Also notes all outputs of all of the surface's views
> > >   * in the output_mask for the surface.
> > >   */
> > >  static void
> > > @@ -1136,8 +1135,7 @@ weston_surface_assign_output(struct weston_surface *es)
> > >   *
> > >   * Identifies the set of outputs that the view is visible on,
> > >   * noting them into the output_mask.  The output that the view
> > > - * is most visible on is set as the view's primary output for
> > > - * vsync and frame event purposes.
> > > + * is most visible on is set as the view's primary output.
> > >   *
> > >   * Also does the same for the view's surface.  See
> > >   * weston_surface_assign_output().
> > > diff --git a/src/compositor.h b/src/compositor.h
> > > index 7851000..0801f20 100644
> > > --- a/src/compositor.h
> > > +++ b/src/compositor.h
> > > @@ -949,8 +949,9 @@ struct weston_view {
> > >  	} transform;
> > >  
> > >  	/*
> > > -	 * Which output to vsync this surface to.
> > > -	 * Used to determine, whether to send or queue frame events.
> > > +	 * The primary output for this view.
> > > +	 * Used for picking the output for driving view animations, inheriting  
> > 
> > Is the correct term "driving animations" or "drawing animations" ?  
> 
> It is actually "driving" here. One surface may be drawn on multiple
> outputs, but only one output will "drive" the animation.
> 
> Driving the animation means to respond to frame callbacks, and since
> outputs may be rendered independently, we tie the animation of a surface
> to only one output.
> 
> For example: a client rendering an animation will attach and commit a
> new buffer of a frame of its animation, and ask for a frame callback.
> Then it will wait for the frame callback until it draws, attaches and
> commits the buffer of the next frame of its animation.
> 
> In order to get as good results as possible, meaning content drawn by
> the client should end up on the output as quickly as possible, we
> calculate the output it has the largest overlapping region on, and
> whenever that output was painted, we reply to the frame callback.
> 
> If we for example would wait until all outputs the surface is one is
> drawn on until replying, we would potentially delay the frame callback,
> making the latency between the client drawing and the content being
> displayed on the output, unnecessarily long, since we would effectively
> let the output drawn last, no matter how large portion of the surface is
> drawn on it, always "drive" the animation.

That is all true for client-drawn animations.

However, weston also has an internal animation framework, see
src/animation.c, which primarily does view animations: changing view
parameters over time, like size, position, or opacity. Also this
animation framework is driven by a single chosen output of a view, for
the reasons Jonas explained.

To recap, client-drawn animations are driven by weston_surface::output
repaint cycle, and server-side view animations are driven by
weston_view::output repaint cycle.

Clients only "know" about weston_surface (wl_surface), they are
completely unaware of how many weston_views it might have. Therefore it
makes sense to drive the client repaint cycle with the client
perspective, and server-side animation cycle according to the view it
is applied on.

(Btw. the reason we sync all animations everywhere to the most
appropriate output's repaint cycle instead of using a timer is to get
the frame drawn exactly for the time it will be shown, avoiding jitter
and wasted drawing. You cannot show a new frame at arbitrary times, so
better lock on to the times it can happen.)

> >   
> > > +	 * the primary output for related views in shells, etc.
> > >  	 * Must be NULL, if 'link' is not in weston_compositor::view_list.
> > >  	 */
> > >  	struct weston_output *output;
> > >   

Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160517/2967d262/attachment.sig>