Created attachment 302567 [details] [review]
gstpad: Add a new GST_PROBE_HANDLED return value for probes

Makes sense IMHO. A "(Since 1.6)" in the gtk-doc chunk would be nice :)

is GST_PROBE_DROP returning FALSE? The documentation states otherwise.

 * @GST_PAD_PROBE_DROP: drop data in data probes. For push mode this means that
 *        the data item is not sent downstream. For pull mode, it means that the
 *        data item is not passed upstream. In both cases, this result code
 *        means that #GST_FLOW_OK or %TRUE is returned to the caller.

Perhaps that also needs fixing?

In any case I also think your idea makes sense, we just need to make sure DROP and HANDLED don't overlap.

The reason it returns FALSE for queries is that unlike with buffers and events the caller will continue to read out results from the query if it got a TRUE return value. Now of course the default content of the queries are always 'valid' in some sense, but pretty much all callers expect sensible query contents when they get a TRUE return value, so that's not really something we can change in my opinion. Also, take for example the SEEKING query, seekable is TRUE/FALSE and the query return indicates 'no response', in which case implementations may fall back to a DURATION query instead, for example.

The alternative to adding a new return value is to add a GstFlowReturn+gboolean ret_value member to the GstPadProbeInfo struct, which might be useful in general in any case to spoof flow return values, but a new return value seems more elegant to me for this particular problem.

While  we're at it, it seems there is to way to return FALSE on an event from a pad probe, or  to return something else than GST_FLOW_OK on a buffer. Maybe we should instead add the return value to GstPadProbeInfo to be used on GST_PAD_PROBE_DROP ?

Adding a new return value can't be use with GST_PAD_PROBE_PASS or REMOVE, while a new field can.

(In reply to Olivier Crête from comment #6)
> Adding a new return value can't be use with GST_PAD_PROBE_PASS or REMOVE,
> while a new field can.

What do you mean by that ? In all cases, we can't add a new return value (would break API of probes), so we would need to go via the new field if we wanted to provide more flexibility.

Also, what would the type of that field be for queries/events (where the "return" value of pushing is a boolean) ? GST_FLOW_OK => TRUE, anything else => FALSE ?

Also note that adding a new field increases quite a bit the modifications to be done in order to stay backwards-compatible with the current behaviour. Depending on where it's called from, for what kind of data (idle/buffer/query/event), and for what GstProbeReturn value. Tricky imho

I mean if you return PROBE_REMOVE, but set the event/buffer to NULL (allowed by Tim's recent patch), then you can remove the probe and drop the buffer at the same time.

I suggest just adding a gint to the struct, the access it with appropriate macros to typecast to GstFlowReturn or gboolean (just like we put event/buffer/bufferlist/queries in the same gpointer). It shouldn't be much more complex than the current code, you can pre-fill to GST_FLOW_OK, TRUE or FALSE depending on the type.

Created attachment 309328 [details] [review]
gstpad: Add a new GST_PROBE_HANDLED return value for probes

In some cases, probes might want to handle the buffer/event/query
themselves and stop the data from travelling further downstream.

While this was somewhat possible with buffer/events and using
GST_PROBE_DROP, it was not applicable to queries, and would result
in the query failing.

With this new GST_PROBE_HANDLED value, the buffer/event/query will
be considered as successfully handled, will not be pushed further
and the appropriate return value (TRUE or GST_FLOW_OK) will be returned

This also allows probes to return a non-default GstFlowReturn when dealing
with buffer push. This can be done by setting the
GST_PAD_PROBE_INFO_FLOW_RETURN() field accordingly

Review of attachment 309328 [details] [review]:

Looks good to me except for:

::: gst/gstpad.h
@@ +557,1 @@
   gpointer _gst_reserved[GST_PADDING];

Need to adjust the padding... by using a union struct :)

commit 7f0e0ff3ca4f3533e66843c084d084517cff4d47
Author: Edward Hervey <edward@centricular.com>
Date:   Wed Apr 29 15:49:17 2015 +0200

    gstpad: Add a new GST_PROBE_HANDLED return value for probes
    
    In some cases, probes might want to handle the buffer/event/query
    themselves and stop the data from travelling further downstream.
    
    While this was somewhat possible with buffer/events and using
    GST_PROBE_DROP, it was not applicable to queries, and would result
    in the query failing.
    
    With this new GST_PROBE_HANDLED value, the buffer/event/query will
    be considered as successfully handled, will not be pushed further
    and the appropriate return value (TRUE or GST_FLOW_OK) will be returned
    
    This also allows probes to return a non-default GstFlowReturn when dealing
    with buffer push. This can be done by setting the
    GST_PAD_PROBE_INFO_FLOW_RETURN() field accordingly
    
    https://bugzilla.gnome.org/show_bug.cgi?id=748643