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

Submitted by Ucan, Emre (ADITG/ESB) on April 18, 2018, 1:44 p.m.

Details

Message ID 1524059066-2455-1-git-send-email-eucan@de.adit-jv.com
State New
Series "libweston: don't accumulate damage from transparent views"
Headers show

Commit Message

Ucan, Emre (ADITG/ESB) April 18, 2018, 1:44 p.m.
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.

Signed-off-by: Emre Ucan <eucan@de.adit-jv.com>
---
 libweston/compositor.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

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

Comments

Pekka Paalanen April 19, 2018, 9:11 a.m.
On Wed, 18 Apr 2018 15:44:26 +0200
Emre Ucan <eucan@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.
> 
> Signed-off-by: Emre Ucan <eucan@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
Ucan, Emre (ADITG/ESB) April 19, 2018, 9:26 a.m.
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.

You might say that camera application can use wl_display_sync instead wl_surface_frame. This would cause stutter and frame drop issues, because camera stream and display are not in synch.

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

Best regards

Emre Ucan
Engineering Software Base (ADITG/ESB)

Tel. +49 5121 49 6937

> -----Original Message-----
> From: Pekka Paalanen [mailto:ppaalanen@gmail.com]
> Sent: Donnerstag, 19. April 2018 11:12
> To: Ucan, Emre (ADITG/ESB)
> Cc: wayland-devel@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@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.
> >
> > Signed-off-by: Emre Ucan <eucan@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
Pekka Paalanen April 19, 2018, 10:35 a.m.
On Thu, 19 Apr 2018 09:26:59 +0000
"Ucan, Emre (ADITG/ESB)" <eucan@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@gmail.com]
> > Sent: Donnerstag, 19. April 2018 11:12
> > To: Ucan, Emre (ADITG/ESB)
> > Cc: wayland-devel@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@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@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
Ucan, Emre (ADITG/ESB) May 4, 2018, 12:09 p.m.
Hi Pekka,

Sorry for late response.

> -----Original Message-----
> From: Pekka Paalanen [mailto:ppaalanen@gmail.com]
> Sent: Donnerstag, 19. April 2018 12:36
> To: Ucan, Emre (ADITG/ESB)
> Cc: wayland-devel@lists.freedesktop.org
> Subject: Re: [PATCH weston] libweston: don't accumulate damage from
> transparent views
> 
> On Thu, 19 Apr 2018 09:26:59 +0000
> "Ucan, Emre (ADITG/ESB)" <eucan@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.

You are right that this issue can be work around for an EGL application when we set
swapInterval to 0, and send a new buffer every time internal state of the application requires to
change its contents. But this does not fix the greediness problem. Because application state can change
more often than display refresh rate.

> 
> 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?
Ideally yes. Let's think that camera is sending buffer with 60 Hz and display has also the same refresh rate.
But camera and display could be still out of sync, so that camera sends its buffer 8ms after Vsync. Therefore,
it will miss the repaint window of weston.

It is also possible that camera buffers sometimes hit, sometimes miss the repaint window. This would cause visible video stutter.
Because delay of displaying a buffer would swing between 0,5 Vsync to 1,5 Vsync ( Repaint window is 0.5 Vsync).
Therefore, it is not ok to send buffer every time when it is ready. We should send it directly after repaint, so that (hopefully) we won't miss the repaint window.
> 
> > 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.
You are right that it is a kind of hack. But I don't know how it should work otherwise especially for camera/video use-cases.

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

But your proposal is also a hack. Maybe better solution would be that we introduce a repaint_window event to wl_output interface, 
so that camera/video applications can synchronize themselves with this event instead of surface frame events. 
Compositor can send a timestamp and duration of repaint window to the clients.

> 
> > > -----Original Message-----
> > > From: Pekka Paalanen [mailto:ppaalanen@gmail.com]
> > > Sent: Donnerstag, 19. April 2018 11:12
> > > To: Ucan, Emre (ADITG/ESB)
> > > Cc: wayland-devel@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@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?

You are right that it would be a better solution to not have invisible views in the compositor view list.
Daniel also implemented to discard occluded views here: " https://patchwork.freedesktop.org/patch/202693/".
I can reuse it. But my real intention for this patch was to mitigate the performance penalty of hiding a surface via using opacity instead of visibility.
Technically, it is of course correct that invisible surfaces do not get any frame events. But we need some other solution to synchronize video applications with wayland compositors...

Best Regards,

Emre Ucan

> 
> 
> Thanks,
> pq
> 
> > > >
> > > > Signed-off-by: Emre Ucan <eucan@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
Pekka Paalanen May 4, 2018, 1:26 p.m.
On Fri, 4 May 2018 12:09:55 +0000
"Ucan, Emre (ADITG/ESB)" <eucan@de.adit-jv.com> wrote:

> Hi Pekka,
> 
> Sorry for late response.

Hi Emre,

no worries!

> > -----Original Message-----
> > From: Pekka Paalanen [mailto:ppaalanen@gmail.com]
> > Sent: Donnerstag, 19. April 2018 12:36
> > To: Ucan, Emre (ADITG/ESB)
> > Cc: wayland-devel@lists.freedesktop.org
> > Subject: Re: [PATCH weston] libweston: don't accumulate damage from
> > transparent views
> > 
> > On Thu, 19 Apr 2018 09:26:59 +0000
> > "Ucan, Emre (ADITG/ESB)" <eucan@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.  
> 
> You are right that this issue can be work around for an EGL
> application when we set swapInterval to 0, and send a new buffer
> every time internal state of the application requires to change its
> contents. But this does not fix the greediness problem. Because
> application state can change more often than display refresh rate.

The key with EGL is to never allow eglSwapBuffers() block. The app can
still ask for frame callbacks itself and throttle to those while
remaining otherwise responsive.

eglSwapBuffers() has been designed to be blocking in nature, i.e. it
has been designed to waste time. Any application that has more than one
window or needs to react to more than just Wayland input events or
needs to run a simulation (games!) would better set swapInterval to
zero. If they don't, and they also don't manually throttle to frame
callbacks, they are bound to waste time in eglSwapBuffers(), and this
is not only a Wayland issue. Wayland just allows us to make this bad
client design painfully obvious.

> > Or is the camera framerate an exact integer fraction of the display
> > refresh rate?  
> Ideally yes. Let's think that camera is sending buffer with 60 Hz and
> display has also the same refresh rate. But camera and display could
> be still out of sync, so that camera sends its buffer 8ms after
> Vsync. Therefore, it will miss the repaint window of weston.
> 
> It is also possible that camera buffers sometimes hit, sometimes miss
> the repaint window. This would cause visible video stutter. Because
> delay of displaying a buffer would swing between 0,5 Vsync to 1,5
> Vsync ( Repaint window is 0.5 Vsync). Therefore, it is not ok to send
> buffer every time when it is ready. We should send it directly after
> repaint, so that (hopefully) we won't miss the repaint window.

I don't think it changes much with respect to timings though. If the
client was posting frames as soon as it got it from the camera, the
compositor would be picking which ones make it to the screen. If the
client is throttled to frame callbacks, then yes, any frame the client
chooses will hit the screen, but now it is the client making the same
choice as the compositor in the former case. Except, the client needs
to make that decision earlier than the compositor.

Can the client here be smarter than the compositor to make a difference?
Given that we don't know about the deadline yet, discussed further
below.

> > 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.  
> 
> But your proposal is also a hack.

Right, but what else could you do, when you do not know in advance when
the window will become visible and yet you want to minimize the number of
frames drawn and discarded?

The smartest option might be to postpone the actual show in the
compositor until the client has updated, but that's a trade-off between
latency from action to effect and a possible glitch (temporary outdated
content).

> Maybe better solution would be that
> we introduce a repaint_window event to wl_output interface, so that
> camera/video applications can synchronize themselves with this event
> instead of surface frame events. Compositor can send a timestamp and
> duration of repaint window to the clients.

(That proposal does not solve the above issue.)

Yeah, there has been talk about exposing the deadline timestamp for the
next vblank (if the compositor processes a commit before this time, it
will hit the refresh). It would help to minimize latency.

However, I would not like an event that gets sent on every output
refresh regardless of client updates. Instead, something like this
might be workable:
https://patchwork.freedesktop.org/patch/174555/

We still want to minimize client and server wake-ups when possible.

Mind, that you can also drive client updates by Presentation-time
feedback events. They are still lacking the deadline information
though, but they do carry the refresh period and phase.

The feedback events are intended for video players to accurately
predict the next time of presentation when they submit a frame as a
response to each frame callback event. When you predict the time the
next frame will become visible, you can choose and even interpolate a
frame for that exact time. After all, the app could be able to compute
intermediate frames, but the display hardware can only show discrete
frames at certain times.


> > 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?  
> 
> You are right that it would be a better solution to not have
> invisible views in the compositor view list. Daniel also implemented
> to discard occluded views here: "
> https://patchwork.freedesktop.org/patch/202693/". I can reuse it. But
> my real intention for this patch was to mitigate the performance
> penalty of hiding a surface via using opacity instead of visibility.
> Technically, it is of course correct that invisible surfaces do not
> get any frame events. But we need some other solution to synchronize
> video applications with wayland compositors...

Yes. I think the best way to mitigate the performance penalty is to
completely exclude fully transparent surfaces from processing.

Is the synchronization of fully invisible video surfaces actually a
problem? There is the old content issue, but aside from that, it will
be in sync again after the next frame callback on becoming visible
again.


Thanks,
pq
Ucan, Emre (ADITG/ESB) May 8, 2018, 7:27 a.m.
Hi Pekka,

> -----Original Message-----
> From: Pekka Paalanen [mailto:ppaalanen@gmail.com]
> Sent: Freitag, 4. Mai 2018 15:27
> To: Ucan, Emre (ADITG/ESB) <eucan@de.adit-jv.com>
> Cc: wayland-devel@lists.freedesktop.org
> Subject: Re: [PATCH weston] libweston: don't accumulate damage from
> transparent views
> 
> On Fri, 4 May 2018 12:09:55 +0000
> "Ucan, Emre (ADITG/ESB)" <eucan@de.adit-jv.com> wrote:
> 
> > Hi Pekka,
> >
> > Sorry for late response.
> 
> Hi Emre,
> 
> no worries!
> 
> > > -----Original Message-----
> > > From: Pekka Paalanen [mailto:ppaalanen@gmail.com]
> > > Sent: Donnerstag, 19. April 2018 12:36
> > > To: Ucan, Emre (ADITG/ESB)
> > > Cc: wayland-devel@lists.freedesktop.org
> > > Subject: Re: [PATCH weston] libweston: don't accumulate damage from
> > > transparent views
> > >
(snip)
> > You are right that this issue can be work around for an EGL
> > application when we set swapInterval to 0, and send a new buffer
> > every time internal state of the application requires to change its
> > contents. But this does not fix the greediness problem. Because
> > application state can change more often than display refresh rate.
> 
> The key with EGL is to never allow eglSwapBuffers() block. The app can
> still ask for frame callbacks itself and throttle to those while
> remaining otherwise responsive.
> 
> eglSwapBuffers() has been designed to be blocking in nature, i.e. it
> has been designed to waste time. Any application that has more than one
> window or needs to react to more than just Wayland input events or
> needs to run a simulation (games!) would better set swapInterval to
> zero. If they don't, and they also don't manually throttle to frame
> callbacks, they are bound to waste time in eglSwapBuffers(), and this
> is not only a Wayland issue. Wayland just allows us to make this bad
> client design painfully obvious.
> 
> > > Or is the camera framerate an exact integer fraction of the display
> > > refresh rate?
> > Ideally yes. Let's think that camera is sending buffer with 60 Hz and
> > display has also the same refresh rate. But camera and display could
> > be still out of sync, so that camera sends its buffer 8ms after
> > Vsync. Therefore, it will miss the repaint window of weston.
> >
> > It is also possible that camera buffers sometimes hit, sometimes miss
> > the repaint window. This would cause visible video stutter. Because
> > delay of displaying a buffer would swing between 0,5 Vsync to 1,5
> > Vsync ( Repaint window is 0.5 Vsync). Therefore, it is not ok to send
> > buffer every time when it is ready. We should send it directly after
> > repaint, so that (hopefully) we won't miss the repaint window.
> 
> I don't think it changes much with respect to timings though. If the
> client was posting frames as soon as it got it from the camera, the
> compositor would be picking which ones make it to the screen. If the
> client is throttled to frame callbacks, then yes, any frame the client
> chooses will hit the screen, but now it is the client making the same
> choice as the compositor in the former case. Except, the client needs
> to make that decision earlier than the compositor.
> 
> Can the client here be smarter than the compositor to make a difference?
> Given that we don't know about the deadline yet, discussed further
> below.
> 
> > > 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.
> >
> > But your proposal is also a hack.
> 
> Right, but what else could you do, when you do not know in advance when
> the window will become visible and yet you want to minimize the number of
> frames drawn and discarded?
> 
> The smartest option might be to postpone the actual show in the
> compositor until the client has updated, but that's a trade-off between
> latency from action to effect and a possible glitch (temporary outdated
> content).
> 
> > Maybe better solution would be that
> > we introduce a repaint_window event to wl_output interface, so that
> > camera/video applications can synchronize themselves with this event
> > instead of surface frame events. Compositor can send a timestamp and
> > duration of repaint window to the clients.
> 
> (That proposal does not solve the above issue.)
> 
> Yeah, there has been talk about exposing the deadline timestamp for the
> next vblank (if the compositor processes a commit before this time, it
> will hit the refresh). It would help to minimize latency.
> 
> However, I would not like an event that gets sent on every output
> refresh regardless of client updates. Instead, something like this
> might be workable:
> https://patchwork.freedesktop.org/patch/174555/
> 
> We still want to minimize client and server wake-ups when possible.
> 
> Mind, that you can also drive client updates by Presentation-time
> feedback events. They are still lacking the deadline information
> though, but they do carry the refresh period and phase.
> 
> The feedback events are intended for video players to accurately
> predict the next time of presentation when they submit a frame as a
> response to each frame callback event. When you predict the time the
> next frame will become visible, you can choose and even interpolate a
> frame for that exact time. After all, the app could be able to compute
> intermediate frames, but the display hardware can only show discrete
> frames at certain times.

Nice! It is very similar to what I thought of. We can use it to continuously send new frames on the right time.
Is presentation protocol supported in any other compositor, or only in Weston?

Are you planning to push patch of Alexandros anytime soon?
> 
> 
> > > 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?
> >
> > You are right that it would be a better solution to not have
> > invisible views in the compositor view list. Daniel also implemented
> > to discard occluded views here: "
> > https://patchwork.freedesktop.org/patch/202693/". I can reuse it. But
> > my real intention for this patch was to mitigate the performance
> > penalty of hiding a surface via using opacity instead of visibility.
> > Technically, it is of course correct that invisible surfaces do not
> > get any frame events. But we need some other solution to synchronize
> > video applications with wayland compositors...
> 
> Yes. I think the best way to mitigate the performance penalty is to
> completely exclude fully transparent surfaces from processing.
> 
> Is the synchronization of fully invisible video surfaces actually a
> problem? There is the old content issue, but aside from that, it will
> be in sync again after the next frame callback on becoming visible
> again.

There are old content issue and missing deadlines issue. Both of them can be fixed by presentation protocol.

> 
> 
> Thanks,
> pq

Best Regards,
Emre