[PATCH] gl-renderer: emit frame_signal after eglSwapBuffers

[Fabien DESSENNE]

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?
Fabien
> I only see it breaking things.
>
>
> Thanks,
> pq