+1, but would like to do a quick API review first.

TODO: Support GAP events

API comment: I think the gst_element_add_pad() shouldn't be in the base class, as we want the subclass to be able to add things to the pad before it is exposed. Or are we expected all these things to be done in the GstAggregatorPad subclass ? Either way, either the code needs changing, or it should be documented.

(In reply to comment #3)
> API comment: I think the gst_element_add_pad() shouldn't be in the base class,
> as we want the subclass to be able to add things to the pad before it is
> exposed. Or are we expected all these things to be done in the GstAggregatorPad
> subclass ? Either way, either the code needs changing, or it should be
> documented.

Everything is supposed to be in subclasses of GstAggregatorPad, we should probably document that indeed.

The timeout seems to be using the System clock, I think that's very wrong, it should be using the pipeline clock instead, ie GST_ELEMENT_CLOCK().

The use of a GST_FLOW_EOS return value from the ->aggregate method to mean that that everything is OK, but we're at EOS is at best strange. Wouldn't it be better to have the GstAggregator base class check that if every pad is at EOS, then it can push EOS?

I think the timeout functionality should also probably be based on the buffer timestamp, not just on the time where it happens to arrive into the aggregator based element.

(In reply to comment #5)
> The timeout seems to be using the System clock, I think that's very wrong, it
> should be using the pipeline clock instead, ie GST_ELEMENT_CLOCK().

If we were basing it off buffer timestamps then yes, the pipeline clock would be more suitable.

(In reply to comment #7)
> I think the timeout functionality should also probably be based on the buffer
> timestamp, not just on the time where it happens to arrive into the aggregator
> based element.

Why, what conceptual problem does that fix?

It's actually based on the time between when the buffer is consumed by the subclass to when the next upstream buffer arrives.  If the buffer has not arrived after the timeout, then the pad is unresponsive and we don't wait for it.  I thought of it as the time between data was here and the deadline when data needs to be here in order to continue processing.

The unresponsive functionality should inform the subclass at which time it is now, according to the pipeline clock, that way, the subclass can fill in the right amount of "gap data", but not more. I guess this could also be used to process incoming GAP events and give them to the subclasses.

(In reply to comment #8)
> (In reply to comment #5)
> > The timeout seems to be using the System clock, I think that's very wrong, it
> > should be using the pipeline clock instead, ie GST_ELEMENT_CLOCK().
> 
> If we were basing it off buffer timestamps then yes, the pipeline clock would
> be more suitable.

Because we want the timeouts to follow the speed at which it is displayed, which is based on the pipeline clock. We really have no guarantees that this clock has anything to do with the system clock. Also, you add it to the latency query, and you can only do that if you use the pipeline clock, you're comparing apples and oranges.

> (In reply to comment #7)
> > I think the timeout functionality should also probably be based on the buffer
> > timestamp, not just on the time where it happens to arrive into the aggregator
> > based element.
> 
> Why, what conceptual problem does that fix?
> 
> It's actually based on the time between when the buffer is consumed by the
> subclass to when the next upstream buffer arrives.  If the buffer has not
> arrived after the timeout, then the pad is unresponsive and we don't wait for
> it.  I thought of it as the time between data was here and the deadline when
> data needs to be here in order to continue processing.

I think it should work along the same lines as rtpjitterbuffer. So the variable we want to control is how long can useful data stay in the aggregator before it is pushed out. And that should be based on the oldest valid unit of data stuck in the aggregator.

Basically, to fulfill the latency promise made in the response to the latency query, a buffer has to exit the element when the pipeline clock reaches buffer_in_running_time + element_latency + upstream_latency at the latest.

Another thing, it's missing an API for subclasses to set the amount of latency they are adding.

(In reply to comment #9)
> The unresponsive functionality should inform the subclass at which time it is
> now, according to the pipeline clock, that way, the subclass can fill in the
> right amount of "gap data", but not more. I guess this could also be used to
> process incoming GAP events and give them to the subclasses.

That's handled elsewhere by the subclasses atm.  GstAggregator currently doesn't deal at all with how the data is matched against stream times.

(In reply to comment #10)
> Because we want the timeouts to follow the speed at which it is displayed,
> which is based on the pipeline clock. We really have no guarantees that this
> clock has anything to do with the system clock. Also, you add it to the latency
> query, and you can only do that if you use the pipeline clock, you're comparing
> apples and oranges.

The speed of what? display? Why would you dynamically adjust the timeout based on some speed?

I think that some small duration in two separate clocks will amount to a close enough elapsing of time that it does not make a difference.

> > (In reply to comment #7)
> I think it should work along the same lines as rtpjitterbuffer. So the variable
> we want to control is how long can useful data stay in the aggregator before it
> is pushed out. And that should be based on the oldest valid unit of data stuck
> in the aggregator.

Which is exactly the same as 'if a buffer does not occur on a pad after the timeout, ignore it and generate a frame anyway'.

ASCII diagram :)

b1, b2, b3 = buffer
f = generated frame
| = timeout occurences

----- running time ------>
. timeout |(b1)  timeout |(b2)|(b3)
b1- - - - - - - - - b3- - - - - -  -> to sink1
- - - - - - - b2- - - - - - - - -  -> to sink2
__________f__________f________f

As is visible from the great diagram, a frame is generated at most after the timeout has occured.

(In reply to comment #11)
> Basically, to fulfill the latency promise made in the response to the latency
> query, a buffer has to exit the element when the pipeline clock reaches
> buffer_in_running_time + element_latency + upstream_latency at the latest.

Which is upheld in the current code.

(In reply to comment #12)
> Another thing, it's missing an API for subclasses to set the amount of latency
> they are adding.

Subclasses can override the latency query where they call the base class and then add their own if they need.

Another consideration that was brought to my attention is that if you take a random pipeline clock it might only advance while the pipeline is playing and we do want to timeout a pad in paused so that we are not forever waiting to generate a buffer because of a buffer that's never going to arrive.

Also rtpjitterbuffer was mentioned here... but last time I checked it was using the system clock unconditionally.

(In reply to comment #13)
> (In reply to comment #9)
> > The unresponsive functionality should inform the subclass at which time it is
> > now, according to the pipeline clock, that way, the subclass can fill in the
> > right amount of "gap data", but not more. I guess this could also be used to
> > process incoming GAP events and give them to the subclasses.
> 
> That's handled elsewhere by the subclasses atm.  GstAggregator currently
> doesn't deal at all with how the data is matched against stream times.

But knowing the deadline would be useful to the subclasses, I'm currently trying to implement a raw audio subclass based on the audiomixer code and porting interleave to us it as I need a synchronized interleave.

> (In reply to comment #10)
> > Because we want the timeouts to follow the speed at which it is displayed,
> > which is based on the pipeline clock. We really have no guarantees that this
> > clock has anything to do with the system clock. Also, you add it to the latency
> > query, and you can only do that if you use the pipeline clock, you're comparing
> > apples and oranges.
> 
> The speed of what? display? Why would you dynamically adjust the timeout based
> on some speed?

The speed of the clock. If the pipeline clock is a bit faster than the system clock, then every packet will arrive late at the sink.

> I think that some small duration in two separate clocks will amount to a close
> enough elapsing of time that it does not make a difference.

Most clocks go about the same speed, why do we even bother with having a pipeline clock then? If we want it in core, let's do it right.

> > > (In reply to comment #7)
> > I think it should work along the same lines as rtpjitterbuffer. So the variable
> > we want to control is how long can useful data stay in the aggregator before it
> > is pushed out. And that should be based on the oldest valid unit of data stuck
> > in the aggregator.
> 
> Which is exactly the same as 'if a buffer does not occur on a pad after the
> timeout, ignore it and generate a frame anyway'.
> 
> ASCII diagram :)
> 
> b1, b2, b3 = buffer
> f = generated frame
> | = timeout occurences
> 
> ----- running time ------>
> . timeout |(b1)  timeout |(b2)|(b3)
> b1- - - - - - - - - b3- - - - - -  -> to sink1
> - - - - - - - b2- - - - - - - - -  -> to sink2
> __________f__________f________f
> 
> As is visible from the great diagram, a frame is generated at most after the
> timeout has occured.

I think that the mistake you're making is assuming that the time where the buffer is entering the aggregator is a useful point to start from even though we have no idea if it's early or it's late. I think all of this should be based on the time when it should reach the sink (ie, the buffer timestamp).

> (In reply to comment #11)
> > Basically, to fulfill the latency promise made in the response to the latency
> > query, a buffer has to exit the element when the pipeline clock reaches
> > buffer_in_running_time + element_latency + upstream_latency at the latest.
> 
> Which is upheld in the current code.

It's not, you're waiting on a completely different clock which for all we know could go measurably slower.

> (In reply to comment #12)
> > Another thing, it's missing an API for subclasses to set the amount of latency
> > they are adding.
> 
> Subclasses can override the latency query where they call the base class and
> then add their own if they need.

They could, but it would be nice to a gst_aggregator_set_latency() like the encoder & decoder base classes, it's quite handy.


(In reply to comment #14)
> Another consideration that was brought to my attention is that if you take a
> random pipeline clock it might only advance while the pipeline is playing and
> we do want to timeout a pad in paused so that we are not forever waiting to
> generate a buffer because of a buffer that's never going to arrive.

The whole thing is really only for live sources. On a non-live sources, you can read more data or get a EOS, so this timeout doesn't apply. On a live stream, you won't get any buffers unless you're in playing anyway.

(In reply to comment #15)
> Also rtpjitterbuffer was mentioned here... but last time I checked it was using
> the system clock unconditionally.

That's just factually not true. rtpjitterbuffer proposes the system clock in case there is no other clock in the pipeline. You're probably confused with the optional usage of the system clock to produce the NTP times in rtpsession, but that's always compared to the pipeline clock.

(In reply to comment #16)
> But knowing the deadline would be useful to the subclasses, I'm currently
> trying to implement a raw audio subclass based on the audiomixer code and
> porting interleave to us it as I need a synchronized interleave.

So, at aggregate() time, each pad either has data or not with which your element deals with accordingly.  If a pad doesn't have data then it timed out at some point previously and you render black/silence/nothing/absence of something.

> > (In reply to comment #10)
> The speed of the clock. If the pipeline clock is a bit faster than the system
> clock, then every packet will arrive late at the sink.

So, just add the miniscule difference to the 'processing latency' and be done with it.  Besides the clock isn't going to wake you up at exactly the time you asked for anyway.

> > I think that some small duration in two separate clocks will amount to a close
> > enough elapsing of time that it does not make a difference.
> 
> Most clocks go about the same speed, why do we even bother with having a
> pipeline clock then? If we want it in core, let's do it right.

Because clocks drift over time.  For small timeouts/durations they are close enough.

> I think that the mistake you're making is assuming that the time where the
> buffer is entering the aggregator is a useful point to start from even though
> we have no idea if it's early or it's late. I think all of this should be based
> on the time when it should reach the sink (ie, the buffer timestamp).

No, it's the time that the buffer *leaves* the aggregator that is used for the start of the timeout.  The buffer entering only cancels the timeout.

> > (In reply to comment #11)
> It's not, you're waiting on a completely different clock which for all we know
> could go measurably slower.

See above.

> > (In reply to comment #12)
> They could, but it would be nice to a gst_aggregator_set_latency() like the
> encoder & decoder base classes, it's quite handy.

Sure :)

> (In reply to comment #14)
> > Another consideration that was brought to my attention is that if you take a
> > random pipeline clock it might only advance while the pipeline is playing and
> > we do want to timeout a pad in paused so that we are not forever waiting to
> > generate a buffer because of a buffer that's never going to arrive.
> 
> The whole thing is really only for live sources. On a non-live sources, you can
> read more data or get a EOS, so this timeout doesn't apply. On a live stream,
> you won't get any buffers unless you're in playing anyway.

It could also be useful for non-live sources to have a live mixer drop frames if they take too long to produce so the pipeline can still render other streams that arrive in time.  Say adding a new stream to an aggregator sink pad which could take a while to set up.

Another question is why push the "mandatory events" inside gst_aggregator_set_src_caps() instead of waiting for the next call to gst_aggregator_finish_buffer(). In audiomixer, there has to be code to delay it anyway.

(In reply to comment #18)
> Another question is why push the "mandatory events" inside
> gst_aggregator_set_src_caps() instead of waiting for the next call to
> gst_aggregator_finish_buffer(). In audiomixer, there has to be code to delay it
> anyway.

So that subclasses can perform allocation queries which require caps to be set beforehand and propagated to the surrounding elements.

gst_aggregator_iterate_sinkpads() should be renamed to gst_aggregate_sink_pads_foreach() or something like that. In python, the aggregator element will have a .iterate_sink_pads() which returns a GstIterator and a .iterate_sinkpads() which does the iteration. It should probably be name. Actually, this should probably be added to GstIterator, something like gst_iterator_foreach_once().

(In reply to comment #19)
> So that subclasses can perform allocation queries which require caps to be set
> beforehand and propagated to the surrounding elements.

The base class should probably implement most of that logic and call a "decide_allocation" into the subclass, like GstBaseSrc is doing. That seems to be logic that would be the same in all subclasses .

The locking requirements to access/modify the members of the object should be documented, is it the srcpad stream lock or the object lock ?

The segment in the GstAggregator struct, what lock protects it?

While I'm looking at the locking, it seems to be lacking. For example, priv->flow_return is unprotected too. I recommend documenting the lock, or why it's not needed, for each member variable, it will also help other people understand the code later.

I've been giving the timeout thing a bit more thought, and I've come to believe that we're looking at it from the wrong side. Instead of declaring that a sink pad is unresponsive after a timeout, the srcpad should operate on a deadline. For example, with videomixer, we generally want to produce an output buffer every 1/x FPS, so when we push out a buffer, we know exactly when is the last possible moment to push out the next buffer. For the audiomixer, we also know this deadline. This deadline is then the time when we should wake up the srcpad task loop and call the aggregate function if enough data was not yet received. Obviously, if enough data was received, the GstAggregator should call aggregate earlier so as to push out the buffer as soon as possible. 
So instead of having a "unresponsive" flag on each pad, I'd just have a deadline-reached variable for the entire element, and then aggregate everything there at this point.

Also, to make this alternate design race-free, we'd need to add a queue inside the GstAggregatorPad that would buffer up to "timeout" buffers to make sure they're directly available when the deadline is reached.

To match other elements, we should rename "timeout" into "latency".

Lookinag at the QoS event handling (bug #739996), which I think should also be in the GstAggregator class which should export a gst_aggregator_get_earliest_time().

Another bug I'm hitting.. what happens if all the pads become unresponsive at the same time. With the current design, the aggregate_func runs in a loop forever. I tried adding some code to stop the loop and wait for a buffer, but that means that some data can get stuck in the aggregator until enough data arrives on one pad.. As for audio, we may need multiple input buffers to fill an output buffer. A dead-line based approach wouldn't have that issue.

Actually, right now if all pads are unresponsive, VideoAggregator goes into a loop and audiomixer just returns EOS and stops.

There should also be decide_allocation / propose_allocation vfuncs for ALLOCATION query handling I guess. Consistency with other base classes.

Might also make sense to check other base classes for other common vfuncs that people found useful :)

I'm unsure if we should do a decide_allocation() like basesrc where one only modifies the GstQuery object, and then have the GstAggregator baseclass activate the pool & allocator, or just have a do_allocation() and leave more leeway to the subclasses on what to do.

After discussions on IRC, we should change this timeout stuff. First of all the property should be called "latency" instead. And then, whenever at least one upstream of aggregator is live, the srcpad thread of aggregator runs against a deadline. And it puts into the output buffer whatever frames are available at this time. The deadline would be based on the running time of the to be produced frame, the maximum upstream latency and the latency property, and would be according to the pipeline clock.

If none of the upstreams of aggregator is live, we can continue with what is currently done.

> After discussions on IRC, we should change this timeout stuff. First of all the
> property should be called "latency" instead. And then, whenever at least one
> upstream of aggregator is live, the srcpad thread of aggregator runs against a
> deadline. And it puts into the output buffer whatever frames are available at
> this time. The deadline would be based on the running time of the to be
> produced frame, the maximum upstream latency and the latency property, and
> would be according to the pipeline clock.
> 
> If none of the upstreams of aggregator is live, we can continue with what is
> currently done.

That has been implemented at: https://bugzilla.gnome.org/show_bug.cgi?id=741146

@tim: GstAggregator should be introspectable, I do not see what would be a problem for that. I think we should just make sure of that while we move the aggregator to core the introspection is happy so that we do not invent a new typelib in -bad for nothing. (In reply to comment #30)


What else should be done? I have the impression that all issues that were raised are now fixed.

Thibault, I don't think I commented on introspectability anywhere? I just added a dependency on bug #731301 yesterday. I don't think it's going to be a problem, and I agree there's no point in adding a short-lived typelib in bad for nothing.

One thing I'm not so sure about is the GstAggregatorClass::sinkpads_type member: is this really a good idea? And good for bindings? It seems to be only used for creating a pad, why not just make a vfunc for that instead then?

What else needs to be done IMHO:

It would be good to move the sink pad iteration to core perhaps if we can agree on a reasonable API. I wouldn't mind adding something like gst_iterator_once_foreach_pad / gst_iterator_once_foreach_element (so the callback gets the real type pointer instead of a GValue), for example.

We should also try to port a muxer over. I started with oggmux yesterday.

Another minor thing I was wondering about: why is gint64 latency from the property in the public structure? I think it should be in the private structure. Nothing uses it and we would have to define access

And I don't think all of Olivier's comments have been addressed, e.g. what about GAP event support?

(In reply to comment #32)
> Thibault, I don't think I commented on introspectability anywhere? I just added
> a dependency on bug #731301 yesterday. I don't think it's going to be a
> problem, and I agree there's no point in adding a short-lived typelib in bad
> for nothing.

Right, just wanted to make sure we are on the same page.

> One thing I'm not so sure about is the GstAggregatorClass::sinkpads_type
> member: is this really a good idea? And good for bindings? It seems to be only
> used for creating a pad, why not just make a vfunc for that instead then?

It should not be an issue for bindings, at least for python. The user can already override the request_new_pad vmethod from core and do everything himself, it is really just an helper where we handle padname and that kind of things.

> What else needs to be done IMHO:
> 
> It would be good to move the sink pad iteration to core perhaps if we can agree
> on a reasonable API. I wouldn't mind adding something like
> gst_iterator_once_foreach_pad / gst_iterator_once_foreach_element (so the
> callback gets the real type pointer instead of a GValue), for example.

I agree with that, opening a bug right now.

> We should also try to port a muxer over. I started with oggmux yesterday.

Cool, how is it going so far?

> Another minor thing I was wondering about: why is gint64 latency from the
> property in the public structure? I think it should be in the private
> structure. Nothing uses it and we would have to define access

No sure, slomo?

(In reply to comment #33)
> And I don't think all of Olivier's comments have been addressed, e.g. what
> about GAP event support?

That can be added later fmpov

(In reply to comment #34)
> (In reply to comment #32)
> (In reply to comment #33)
> > And I don't think all of Olivier's comments have been addressed, e.g. what
> > about GAP event support?
> 
> That can be added later fmpov

This may be preaching to the choir, but anyway ...

It has taken quite some time and doing to get muxers to handle sparse streams (by means of GstCollectPads[2]), so I hope this "added later" is before muxers get ported to GstAggregator so as not to loose the sparse stream (subtitle) support along the way.

(In reply to comment #34)

> > One thing I'm not so sure about is the GstAggregatorClass::sinkpads_type
> > member: is this really a good idea? And good for bindings? It seems to be only
> > used for creating a pad, why not just make a vfunc for that instead then?
> 
> It should not be an issue for bindings, at least for python. The user can
> already override the request_new_pad vmethod from core and do everything
> himself, it is really just an helper where we handle padname and that kind of
> things.

This GType stuff is always a bit awkward for bindings, I would prefer a vfunc for it too that allows to use all the magic from GstAggregator to create a pad but allows the subclass to create a pad itself.

> > It would be good to move the sink pad iteration to core perhaps if we can agree
> > on a reasonable API. I wouldn't mind adding something like
> > gst_iterator_once_foreach_pad / gst_iterator_once_foreach_element (so the
> > callback gets the real type pointer instead of a GValue), for example.
> 
> I agree with that, opening a bug right now.

I think that should be convenience API in GstElement / GstBin, not stuff in GstIterator. Unless you make it a gst_iterator_foreach_once() that is independent of the type.

> > Another minor thing I was wondering about: why is gint64 latency from the
> > property in the public structure? I think it should be in the private
> > structure. Nothing uses it and we would have to define access
> 
> No sure, slomo?

No reason, move it. Everything should be moved to the private structures unless it's actually used by subclasses.
 
> (In reply to comment #33)
> > And I don't think all of Olivier's comments have been addressed, e.g. what
> > about GAP event support?
> 
> That can be added later fmpov

See Mark's comment. No GAP support and no support for sparse streams would be a blocker for this to be moved to core.

*** Bug 532944 has been marked as a duplicate of this bug. ***

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.

Let's punt this to 1.7 then.

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.

Also, I'm not sure about the idea of putting the GType in the GstPadTemplate. GstPadTemplates go into the registry. We could put the string name of the GType, but I don't know how useful that is as gst-inspect output.

so any updates for this moving? I see in the trunk, GstAggregator is still in bad pacakge. What's the plan for moving it to core?

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 ?

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

How is this problematic? It's fine to store the type name string in the registry, but not necessarily required. gst-inspect-1.0 will load the plugin anyway, because properties are not stored in the registry, so at that point the GType will always be available.

I just gave a quick look at gstaggregator.h, and one point that I think needs discussing is whether we want subclasses to iterate sink pads through GstElement.sinkpads, or through some other mechanism. There is already a poorly-named foreach function for this purpose, gst_aggregator_iterate_sinkpads , with an associated FIXME:

/* API that should eventually land in GstElement itself (FIXME) */

Requiring subclasses to only aggregate pads explicitly listed as "aggregatable" could give us in the future the ability to exclude pads from aggregation for one reason or another, and could also simplify implementation, as the subclass would for example not need, as currently, to check whether a pad did have a buffer.

If we require that, we need to rename gst_aggregator_iterate_sinkpads to gst_aggregator_foreach_sinkpad, and provide an actual iterate function for the pads to aggregate.

If not, we will probably want to implement and expose a proper gst_element_foreach_pad method.

> and provide an actual iterate function for the pads to aggregate.

or pass these in the call to ->aggregate

@Mathieu: https://bugzilla.gnome.org/show_bug.cgi?id=742152 :)

To me https://bugzilla.gnome.org/show_bug.cgi?id=760981 is not realy a blocker for that one, what would be missing now? (I checked quickly and I think we have done pretty much everything that was preventing on to move on with this one :))

BTW,
gstreamer-bad-base-1.0.pc should also be removed from gstreamer-1.0-osx-framework.recipe and gst-plugins-bad-1.0.recipe, otherwise upstream's cerbero code will fail to execute package command.:)

Ah yes, I forgot the cerbero bit, thank you.

1) Follow up bugs:

Bug #791201 - better support for static input pads
Bug #791202 - need gst_aggregator_pad_set_waiting() for sparse streams


2) I'm still very much inclined towards renaming

 gst_aggregator_pad_steal_buffer() -> gst_aggregator_pad_pop_buffer()
 gst_aggregator_pad_get_buffer() -> gst_aggregator_pad_peek_buffer()

It is true that get/steal is used elsewhere, but in a slightly different context. I think pop/peek describe what is done functionality-wise much better and avoid confusion about the return value ownership, e.g. with GValue _get() will not return a ref whereas _steal() will but here both do return a ref.


3) Tag handling is bogus. I think I will just remove it, don't think any of the current subclasses is using that at the moment, then we can figure out what to do once someone needs it and can't handle it via the usual vfuncs.


Otherwise this is done, so let's close it.

Bug #791204 - rename pad_{get,steal}_buffer() to pad_{peek,pop}_buffer()

As discussed during the hackfest:
@@ -1858,12 +1872,13 @@ gst_aggregator_query_latency_unlocked (GstAggregator * self, GstQuery * query)
 
   gst_query_parse_latency (query, &live, &min, &max);
 
+  // FIXME: this is already done by gst_query_set_latency()
   if (G_UNLIKELY (!GST_CLOCK_TIME_IS_VALID (min))) {
     GST_ERROR_OBJECT (self, "Invalid minimum latency %" GST_TIME_FORMAT
         ". Please file a bug at " PACKAGE_BUGREPORT ".", GST_TIME_ARGS (min));
     return FALSE;
   }
-
+  // FIXME: should this be done by gst_query_set_latency() too?
   if (min > max && GST_CLOCK_TIME_IS_VALID (max)) {
     GST_ELEMENT_WARNING (self, CORE, CLOCK, (NULL),
         ("Impossible to configure latency: max %" GST_TIME_FORMAT " < min %"
@@ -2001,6 +2016,7 @@ gst_aggregator_default_src_query (GstAggregator * self, GstQuery * query)
 
       /* don't pass it along as some (file)sink might claim it does
        * whereas with a collectpads in between that will not likely work */
+      // FIXME: wat?    ^^^^^^^^^^^
       gst_query_parse_seeking (query, &format, NULL, NULL, NULL);
       gst_query_set_seeking (query, format, FALSE, 0, -1);
       res = TRUE;

This is the last undesired mentioning of collectpads. Ideally reword the comment to explain whats the matter here.

(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #52)
> -
> +  // FIXME: should this be done by gst_query_set_latency() too?
>    if (min > max && GST_CLOCK_TIME_IS_VALID (max)) {
>      GST_ELEMENT_WARNING (self, CORE, CLOCK, (NULL),
>          ("Impossible to configure latency: max %" GST_TIME_FORMAT " < min %"
> @@ -2001,6 +2016,7 @@ gst_aggregator_default_src_query (GstAggregator *
> self, GstQuery * query)

I'm still unsure if min<=max in the latency query is always invalid in the general case (ie, general enough to put the check in gst_query_set_latency()).