Created attachment 301616 [details] [review]
sink: add 'frame-rendered' signal

It gives app opportunity to do something next, for example: webkit
use it to trigger UI process update.

Change-Id: I9855a67b14e936a332c4ca482f880fc1af21f860

Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>

The usual signal name for that IMHO, is "handoff". Thanks.

However, you mention WebKit as the use-case but is it really necessary there?

Relevant discussion in bug #705821

(In reply to Gwenole Beauchesne from comment #25)
> Review of attachment 259636 [details] [review] [review]:
> 
> The signal could simply be "show-frame" that takes a clean GstBuffer as
> argument. i.e. GstFlowReturn (*show_frame) (GstBaseSink * base_sink,
> GstBuffer * buf);
> By clean GstBuffer argument, I mean the one with VA backed storage, i.e. the
> one obtained with gst_vaapi_plugin_base_get_input_buffer().
> Next, the default object handler is mostly what we have already for
> gst_vaapisink_show_frame().
> 
> If the user handler returns GST_FLOW_NOT_LINKED, then the accumulator will
> stop execution, return FALSE and make the final signal handler return value
> be GST_FLOW_OK.
> 
> That way, we can handle cases like
> - pre show-frame hook
> - the actual vaapisink show-frame
> - post show-frame hook


(In reply to Zhao, Halley from comment #28)
> (In reply to comment #25)
> seems you extends it to a new usage.
> the original purpose is to emit a signal after vaapisink shows a frame,
> something like a damage event. vaapisink still owns video rendering.

(In reply to Gwenole Beauchesne from comment #29)
> (In reply to comment #28)
> This doesn't change the purpose. vaapisink still renders the frame, but you
> now have the opportunity to notify upper layer when we are about to draw and
> after we are done.

(In reply to Gwenole Beauchesne from comment #33)
> (In reply to comment #32)
> > (In reply to comment #28)
> > sorry, I don't catch your meaning.
> > is it something like the following:
> > 
> > static GstFlowReturn
> > gst_vaapisink_show_frame_new(GstBaseSink *base_sink, GstBuffer *src_buffer)
> > {
> >     // xx some work before emit 'show-frame' signal 
> > 
> >     // xx emit 'show-frame' signal
> > 
> >     if (ret == GST_FLOW_NOT_LINKED) {
> > 
> >     }
> >     else {
> >         gst_vaapisink_show_frame();
> >     }
> > 
> >     // xx some rest work
> > 
> > }
> 
> In gst_vaapisink_show_frame(),
> 
> code from: meta = gst_buffer_get_vaapi_video_meta(buffer);
> to: if (!success) goto error;
> is moved to
> 
> static GstFlowReturn
> default_show_frame(GstBaseSink *base_sink, GstBuffer *buffer)
> {
> }
> 
> this means that, gst_vaapisink_show_frame() can be reduced to:
> {
>     // ...
>     ret = gst_vaapi_plugin_base_get_input_buffer(GST_VAAPI_PLUGIN_BASE(sink),
>         src_buffer, &buffer);
>     if (ret != GST_FLOW_OK && ret != GST_FLOW_NOT_SUPPORTED)
>         return ret;
> 
>     ret = g_signal_emit(...);
> 
>     /* Retain VA surface until the next one is displayed */
>     if (sink->use_overlay)
>         gst_buffer_replace(&sink->video_buffer, buffer);
>     gst_buffer_unref(buffer);
>     return ret;
> }
> 
> The GSignalAccumulator function for "show-frame" will check the
> handler_return. If it is GST_FLOW_NOT_LINKED, this means we want to
> prematurely abort the signal handlers emission. Thus, we return FALSE to
> signal that, and set the return_accu to GST_FLOW_OK (since this is not an
> error per se).
> 
> Thanks.

(In reply to Gwenole Beauchesne from comment #2)
> The usual signal name for that IMHO, is "handoff". Thanks.

I'll rename it.

> 
> However, you mention WebKit as the use-case but is it really necessary there?

Because it was in the original commit log message. I'll change it.

(In reply to Víctor Manuel Jáquez Leal from comment #4)
> (In reply to Gwenole Beauchesne from comment #2)
> > The usual signal name for that IMHO, is "handoff". Thanks.
> 
> I'll rename it.
> 
> > 
> > However, you mention WebKit as the use-case but is it really necessary there?
> 
> Because it was in the original commit log message. I'll change it.

Yes, I am aware of that discussion, but my point is that this is not in use at the moment, and won't be. :) i.e. rendering happens through usual sinks & meta, and vaapisink is no longer abused for any specific purpose.

However, "handoff" and "preroll-handoff" could be useful for other purposes, though generally for debug & perf measurements on top of my head.

(In reply to Gwenole Beauchesne from comment #5)
> However, "handoff" and "preroll-handoff" could be useful for other purposes,
> though generally for debug & perf measurements on top of my head.

Just like fakesink, I assume.

I'm wondering if we should pass to the callback a GstSample instead of a GstBuffer

Created attachment 301659 [details] [review]
sink: add 'handoff' signal

This patch adds the signal ::handoff and the property signal-handoffs. If the
property is set TRUE, the signal ::handoff is emitted just after the buffer is
rendered.

Based on Zhao Halley <halley.zhao@intel.com>

If we want to keep consistency with existing signals, we ought to stick to GstBuffer IMHO. Patch is LGTM and I have no extra opinion if "preroll-handoff" would actually be useful. Could be added later.

(In reply to Gwenole Beauchesne from comment #8)
> If we want to keep consistency with existing signals, we ought to stick to
> GstBuffer IMHO. Patch is LGTM and I have no extra opinion if
> "preroll-handoff" would actually be useful. Could be added later.

Also, if we land bug #747935, the preroll-handoff might be not required, since if property ::show-preroll-frame is true, both signals would be emitted.

commit 562bd629a68cee3f0dd4fa4372b6784e04e31958
Author: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>
Date:   Wed Apr 15 18:16:47 2015 +0200

    vaapisink: add 'handoff' signal
    
    This patch adds the signal ::handoff and the property signal-handoffs. If the
    property is set TRUE, the signal ::handoff is emitted just after the buffer 
    is rendered.
    
    Based on Zhao Halley <halley.zhao@intel.com>