[PATCH] gl-renderer: emit frame_signal after eglSwapBuffers

[Fabien DESSENNE]

On 08/17/2016 09:42 AM, Pekka Paalanen wrote:
> 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.
Hi,

This is what I have in mind, I'll give it a try.
> 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.
I am not familiar with VAAPI, and have no idea whether it is blocking or 
not. But you are right, the priority must be given to the rendering, so 
capturing an outdated frame would be a good workaround if there is 
something blocking.

Thank you for your valuable comments.

Fabien
>
> Thanks,
> pq