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

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=739010
  GStreamer | gstreamer (core) | git

--- Comment #38 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2015-01-04 12:52:25 UTC ---
oggmux port to GstAggregator can be found here:
http://cgit.freedesktop.org/~tpm/gst-plugins-base/log/?h=oggmux-aggregator

Couple of issues:

 - we need a replacement/equivalent for
    gst_collectpads_set_waiting() to handle
    sparse streams. I suspect this is not too
    difficult.

 - 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
        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)

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

 - 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?

 - 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.

 - 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.

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

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

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

 - 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.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.