[Bug 739010] [PLUGIN MOVE] Move GstAggregator to gstreamer (core)

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=739010

--- Comment #42 from Olivier Crête <olivier.crete at ocrete.ca> ---
I think we're more or less ready to move to -bad or even core. I think we
answered most of Tim's comments already and the other ones are additions.


(In reply to Tim-Philipp Müller from comment #38)
>  - we need a replacement/equivalent for
>     gst_collectpads_set_waiting() to handle
>     sparse streams. I suspect this is not too
>     difficult.

Yep, we should add that, some kind of fnuction to add a static pad.

>  - there seem to be some issues with passing
>     the flow returns upstream, probably because
>     there has so far been an assumption that every
>     call to the ::aggregate() vfunc generates at least
>     one output buffer. If that's not the case, and the
>     aggregate vfunc returns FLOW_OK when there's
>     nothing to do, then that might/will overwrite a
>     "bad" flow return from a previous _finish_buffer()
>     that had been stored in priv->flow_return.
>     Possible solutions:
>         1) add custom flow return to distinguish NO_CHANGE from OK

There is now GST_AGGREGATOR_FLOW_NEED_DATA that does that, it will get called
back later.


>         2) just require sub-classes to track last_flow themselves in this
>            case and return the latest and greatest from ::aggregate()
>         3) never overwrite (fatal) non-FLOW_OK with FLOW_OK in GstAggregator

>  - relatedly, shouldn't pads stuck in _pad_chain () be
>     woken up in order to return upstream any non-OK
>     flow return immediately? (The assumption seems
>     to be that most if not all pads will see buffers popped
>     off during aggregation, but that might not be true for
>     a muxer or subtitle overlay, and not even for a mixer)

It does that now (it called gst_aggregator_pad_set_flushing() on all pads on
non-OK).

>  - aggregator->priv->flow_return access is not thread-safe
>    in general (e.g. in gst_aggregator_pad_chain)

all locked now

>  - do we need/want a GstAggregatorPad::reset() vmethod?
>    (to reset pad state when shutting down, to make element re-usable)
>    Or do we assume that flush == reset?

Good question, isnt that the same as ->start() and ->stop()

>  - GST_EVENT_TAG handling looks bogus, stream tags
>    are merged into one global tag list?! I'm not even sure
>    if this makes sense for mixers, but muxers probably
>    want access to per-stream tags. non-global tags should
>    probably just be dropped and not merged.

Muxer type subclasses probably just shouldnt chain up ?

>  - Not sure I like the naming of
>    gst_aggregator_pad_get_buffer() / _steal_buffer().
>    This kind of naming is usually about ownership transfer
>    and thus doesn't reflect the actually more important part
>    of the calls, which is whether the buffer stays on the pad
>    or is popped off. I would suggest:
> 
>       - gst_aggregator_pad_peek_buffer()
>       - gst_aggregator_pad_pop_buffer() or _dequeue_buffer()
> 
>    and it's a bit counter-intuitive and inconvenient that get()/peek()
>    returns a ref as well (though collectpads does that too). I think
>    any caller to get/peek() holds a ref to the pad, so one could be
>    sure that any buffer the pad owns is not going to go away while
>    we'd be peeking at it.

How about _get_buffer() and _pop_buffer() ? Although I'm fine with the current
one, we have APIs named like that in GLib.

>  - it would be nice if we could incorporate queuing
>    functionality on the input side. (Can be added later)

There is that already, if the source is live and the latency property is set to
> 0.

>  - gst_aggregator_request_new_pad() should not insist
>     on sink pad template name of "sink_%u" (could be
>     audio_%u or somesuch too, etc.)

Done

>  - I think we should rather add the pad GType to
>    GstPadTemplate, which would also be useful for
>    gst-inspect.

I agree with Jan that this is problematic

>  - I wonder if the "latency" property should not rather
>    be called "extra-latency" ? Or perhaps I misunderstand
>    it. The our_latency > data.max comparison in
>    gst_aggregator_query_latency() doesn't look right if
>    it's extra latency though.

This is really the input queue max lenght, the name could maybe be improved?


(In reply to Jan Schmidt from comment #40)
> One thing that GstCollectPads allowed which GstAggregator doesn't do nicely
> is elements that want to receive data on fixed static pads.
> 
> It would be nice to decouple the request pad template and naming from the
> actual function of GstAggregator. Perhaps a default request_pads
> implementation that
> sub-classes can install on themselves, or use as a function call when
> creating their own request pads, and a separate function for 'enrolling'
> pads the sub-class created itself.

I think we can easily add that later. Just add a gst_aggregate_add_static_pad()
function of some kind ?

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.