[PATCH weston] libweston: don't accumulate damage from transparent views

[Pekka Paalanen]

On Thu, 19 Apr 2018 09:26:59 +0000
"Ucan, Emre (ADITG/ESB)" <eucan at de.adit-jv.com> wrote:

> Hi Pekka,
> 
> If we remove the view from scenegraph, application will be blocked.
> Because it is not getting any surface frame events. It is not OK to
> block unexpected applications. Especially if the application is
> sending output of a camera or digital TV, weston should always get
> latest buffer from the application.

Hi Emre,

sounds like the application is broken if it gets literally blocked by
that.

Applications very much have to be able to deal with frame callbacks
not coming back in a timely fashion. This is what compositors use to
throttle down well-behaving clients. It's not blocking the app, unless
the app is badly written.

An invisible surface is a very good reason to try and throttle down the
client.

> You might say that camera application can use wl_display_sync instead
> wl_surface_frame.

Not necessarily. It would certainly make the app greedy instead of
well-behaved in my opinion.

But, this would definitely make the latest buffer from the application
always available to the compositor. You shouldn't need wl_display.sync
even, the app can simply keep on committing new buffers whenever it
wants.

If the application is a video player that just cannot avoid decoding or
receiving frames anyway, then the savings of not posting those frames
may be insignificant. The major saving the throttling aims for is with
apps that paint on demand, where painting is a heavy task that can
easily be skipped.

> This would cause stutter and frame drop issues,
> because camera stream and display are not in synch.

Are your camera and display both using the exact same refresh rate,
just not synchronized by hardware?

I fail to see how the frame callbacks would synchronize anything any
better than just always committing new buffers whenever the app has
one. It's either the compositor or the client that eventually makes the
decision to drop or repeat frames to account for differing refresh
rates. But if it is the app doing it, it could interpolate video frames
to match the display refresh cycle.

Or is the camera framerate an exact integer fraction of the display
refresh rate?

> I know that I have to modify weston_compositor_pick_view(). I will
> send a patch if this one is accepted.

It still seems like a hack to me.

If the view is not visible (does not matter how it is not visible), it
should not be getting frame callbacks, because there is no point for
the client to update surface as it is not visible. Weston is missing
some implementation bits to postpone frame callbacks for e.g. surfaces
that are completely occluded. I suppose being completely transparent
could be another case.

I know there is a problem that when the surface becomes visible again,
it would take a frame cycle to have the client send an updated buffer.
This could be worked around on either side: the compositor could be
sending frame callbacks at a slow rate even if the surface is not
visible, or the client could be updating the surface content at a slow
rate even if it doesn't get frame callbacks.

> > -----Original Message-----
> > From: Pekka Paalanen [mailto:ppaalanen at gmail.com]
> > Sent: Donnerstag, 19. April 2018 11:12
> > To: Ucan, Emre (ADITG/ESB)
> > Cc: wayland-devel at lists.freedesktop.org
> > Subject: Re: [PATCH weston] libweston: don't accumulate damage from
> > transparent views
> > 
> > On Wed, 18 Apr 2018 15:44:26 +0200
> > Emre Ucan <eucan at de.adit-jv.com> wrote:
> >   
> > > If view is set to be entirely transparent,
> > > there is no need to accumulate its damage.
> > >
> > > This is an important optimization for
> > > using view transparency. Because otherwise
> > > transparent views are rendered like an
> > > opaque view, and their pixel values
> > > are set to 0 in fragment shader.

I do see the value here, but I'm not sure the below is the right way to
do it. You suppress damage, but you don't avoid actually painting the
view if something else still causes damage.

One possibility would be to exclude the view from
weston_compositor::view_list which is used for both rendering and input
picking. This would happen in weston_compositor_build_view_list(). It
could also exclude completely occluded views, but we can leave that for
another time.

Excluding the view from the rendering list will avoid sending frame
callbacks. It will also affect Presentation feedback in the expected
way: the updates the compositor has decided to not show (e.g. by setting
view alpha to zero) will not result in a "presented" event.

Would you like to examine this path instead?


Thanks,
pq

> > >
> > > Signed-off-by: Emre Ucan <eucan at de.adit-jv.com>
> > > ---
> > >  libweston/compositor.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libweston/compositor.c b/libweston/compositor.c
> > > index a9de4ac..4bcf120 100644
> > > --- a/libweston/compositor.c
> > > +++ b/libweston/compositor.c
> > > @@ -2122,7 +2122,10 @@ compositor_accumulate_damage(struct  
> > weston_compositor *ec)  
> > >  		pixman_region32_init(&opaque);
> > >
> > >  		wl_list_for_each(ev, &ec->view_list, link) {
> > > -			if (ev->plane != plane)
> > > +			/* If view is set to be entirely
> > > transparent,
> > > +			 * there is no need to accumulate its
> > > damage.
> > > +			 */
> > > +			if (ev->plane != plane || ev->alpha ==
> > > 0.0f) continue;
> > >
> > >  			view_accumulate_damage(ev, &opaque);  
> > 
> > Hi,
> > 
> > why this instead of removing the whole view from the scenegraph?
> > 
> > You would also need to exclude the view in
> > weston_compositor_pick_view(), and adding these rendering special
> > cases around doesn't feel very good to me.
> > 
> > 
> > Thanks,
> > pq  

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180419/75f6daae/attachment-0001.sig>