[PATCH] gl-renderer: emit frame_signal after eglSwapBuffers

[Pekka Paalanen]

On Tue, 16 Aug 2016 14:46:23 +0200
Fabien DESSENNE <fabien.dessenne at st.com> wrote:

> On 08/16/2016 10:18 AM, Pekka Paalanen wrote:
> > On Fri, 12 Aug 2016 14:11:40 +0200
> > Fabien Dessenne <fabien.dessenne at st.com> wrote:
> >  
> >> Emit frame_signal at the end of the GL renderer processing, so the
> >> frame_signal clients are informed after the buffer is actually updated.
> >>
> >> Signed-off-by: Fabien Dessenne <fabien.dessenne at st.com>
> >> ---
> >>   libweston/gl-renderer.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/libweston/gl-renderer.c b/libweston/gl-renderer.c
> >> index d624453..f4d4a5e 100644
> >> --- a/libweston/gl-renderer.c
> >> +++ b/libweston/gl-renderer.c
> >> @@ -1138,7 +1138,6 @@ gl_renderer_repaint_output(struct weston_output *output,
> >>   	draw_output_borders(output, border_damage);
> >>   
> >>   	pixman_region32_copy(&output->previous_damage, output_damage);
> >> -	wl_signal_emit(&output->frame_signal, output);
> >>   
> >>   	if (gr->swap_buffers_with_damage) {
> >>   		pixman_region32_init(&buffer_damage);
> >> @@ -1185,6 +1184,7 @@ gl_renderer_repaint_output(struct weston_output *output,
> >>   	}
> >>   
> >>   	go->border_status = BORDER_STATUS_CLEAN;
> >> +	wl_signal_emit(&output->frame_signal, output);
> >>   }
> >>   
> >>   static int  
> > Hi,
> >
> > I think this will break screenshooting.
> >
> > Screenshooter relies on glReadPixels, but if you issue eglSwapBuffers
> > first, you no longer know what glReadPixels will return. Therefore the
> > signal must be emitted before the swapbuffers.  
> Oh, yes, you are right, I missed this part.
> > You forgot to explain what problem this change solves, too.  
> I am trying to improve the way to record the display since 
> glReadPixels() is quite slow: although it works fine for a one frame 
> capture, the performances are not acceptable for a continuous capture.
> 
> So I am prototyping something between screenshooter and VAAPI recorder 
> but my problem is that my recorder is always one frame late.
> 
> This issue was solved with my proposed patch, but I agree that it is not 
> acceptable.
> 
> Now I come to the conclusion that VAAPI recorder has probably the same 
> 'one frame late' issue, which I unfortunately cannot verify, but reading 
> the code let me think that there is something wrong there:
> drm_output_render_gl() calls:
> - output->base.compositor->renderer->repaint_output()
> - bo = gbm_surface_lock_front_buffer(output->gbm_surface)
> - output->next = drm_fb_get_from_bo(bo, b, output->gbm_format)
> 
> Then, "output->current" is updated with the fresh "output->next" at the 
> next page_flip_handler() call.
> 
> And before that, VAAPI recorder captures "output->current" (see 
> recorder_frame_notify) just after repaint_output() is called (triggered 
> by frame_signal emit).
> But at that time, "output->current" is not updated yet, it still refers 
> to the previous frame (it is updated at the next page_flip)
> 
> Can you let me know your opinion about it?

Hi,

I didn't verify your reasoning, but it does sound plausible, yes. You
might add a new 'post_frame_signal' perhaps for your purposes, or maybe
something DRM-backend specific because of reliance on GBM.

I can also imagine a reason why you would actually want to be one frame
late: might calling into VAAPI cause a compositor stall?

If VAAPI was blocking somehow, being a frame late allows you avoid
blocking for the client and composite rendering. It's sub-optimal, but
I am not familiar with VAAPI. If it doesn't have that issue, then
adding a new hook sounds fine.


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/20160817/4c48263a/attachment.sig>