GNOME Bugzilla – Bug 741146
aggregator: deadline based aggregation
Last modified: 2014-12-16 16:38:08 UTC
In order to correctly support live pipelines it would make sense to have the src task drive the pipeline so that aggregator can produce buffers at least at fixed intervals.
Created attachment 292171 [details] [review] aggregator: make the src pad task drive the pipelins for live sources
Review of attachment 292171 [details] [review]: ::: gst-libs/gst/base/gstaggregator.c @@ +479,3 @@ +{ + if (buffer && GST_BUFFER_TIMESTAMP_IS_VALID (buffer)) { + *start = GST_BUFFER_TIMESTAMP (buffer); This never happens as this is always called with buffer==NULL? For the "default case" of subclasses that don't give us times, we should just disable the deadline system entirely? @@ +508,3 @@ +/* called with the src STREAM lock */ +static gboolean +_wait_and_check (GstAggregator * self, GstBuffer * buffer) Where does the buffer come from? It seems to always be called with NULL? ::: gst-libs/gst/base/gstaggregator.h @@ +240,3 @@ + void (*get_times) (GstAggregator * aggregator, + GstBuffer * buffer, + GstClockTime * start, My gut feeling is that it should be get_next_time() that returns the start, because this is called before the buffer is created. And probably allow returning GST_CLOCK_TIME_NONE to mean "ASAP".
(In reply to comment #2) > Review of attachment 292171 [details] [review]: > > ::: gst-libs/gst/base/gstaggregator.c > @@ +479,3 @@ > +{ > + if (buffer && GST_BUFFER_TIMESTAMP_IS_VALID (buffer)) { > + *start = GST_BUFFER_TIMESTAMP (buffer); > > This never happens as this is always called with buffer==NULL? Currently, yes. > For the "default case" of subclasses that don't give us times, we should just > disable the deadline system entirely? The default case in basesink is to retrieve that information from the buffer. > @@ +508,3 @@ > +/* called with the src STREAM lock */ > +static gboolean > +_wait_and_check (GstAggregator * self, GstBuffer * buffer) > > Where does the buffer come from? It seems to always be called with NULL? Nowhere yet but when aggregator does the allocation query it will need to deal with buffer pools and allocating buffers like basetransform and basesrc so that would be the buffer that is used. > ::: gst-libs/gst/base/gstaggregator.h > @@ +240,3 @@ > + void (*get_times) (GstAggregator * aggregator, > + GstBuffer * buffer, > + GstClockTime * start, > > My gut feeling is that it should be get_next_time() that returns the start, > because this is called before the buffer is created. And probably allow > returning GST_CLOCK_TIME_NONE to mean "ASAP". This is modelled after basesink's get_times() and its usage. Another use case for it is calculating buffer durations for qos like what is currently done in videoaggregator.
Even when the buffer comes from a bufferpool, it wouldn't contain any useful information that would allow to get a timestamp. I think Olivier's proposal of a get_next_time() or similar makes sense in this situation, as we want to wait until we create the next buffer. Independent of that there is also something missing to make this useful in combination with videoaggregator. videoaggregator currently drops frames that arrive too late. But this can mean that an even older frame is kept around and not updated, and even if there is no older frame it means that instead of showing an old frame we show nothing. I think for live mode (and in general maybe?) it should always keep around the latest buffer, but if it's too old it should just not trigger rendering with that buffer until the deadline.
Created attachment 292280 [details] [review] aggregator: make the src pad task drive the pipeline for live sources
Created attachment 292281 [details] [review] videoaggregator: always try to use newer buffers
Review of attachment 292280 [details] [review]: Is there anything needed in audiomixer to handle this properly, other than implementing get_next_time()? Looks mostly good to me, feel free to just push after these changes ::: gst-libs/gst/base/gstaggregator.c @@ +167,3 @@ + } G_STMT_END + +#define SRC_STREAM_BROADCAST(self) { \ Use G_STMT_START/END here @@ +174,3 @@ + +#define KICK_SRC_THREAD(self) \ + do { \ And here @@ +518,3 @@ + GST_TIME_ARGS (start)); + + time = GST_ELEMENT_CAST (self)->base_time + start; Accessing the base time might need the object lock @@ +522,3 @@ + if (GST_CLOCK_TIME_IS_VALID (latency_max)) { + time += latency_max; + } else if (GST_CLOCK_TIME_IS_VALID (latency_min)) { The min latency should always be valid IIRC. It should never be GST_CLOCK_TIME_NONE, but instead the "default" value is 0 @@ +538,3 @@ + + self->priv->aggregate_id = + gst_clock_new_single_shot_id (GST_ELEMENT_CLOCK (self), time); Accessing the clock needs the object lock
commit 2764f2baf1f57bcad2fd1baaf7513b5e36ebcae5 Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue Dec 16 17:37:12 2014 +0100 audiomixer: Implement get_next_time() commit 852b08340e84d6c648c02c9e546fa107741cdefc Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue Dec 16 17:33:01 2014 +0100 aggregator: Some minor cleanup commit 8bf53a12262f1274d383ea0bf97107d883d5e35e Author: Matthew Waters <matthew@centricular.com> Date: Fri Dec 5 18:19:54 2014 +1100 aggregator: make the src pad task drive the pipeline for live pipelines This removes the uses of GAsyncQueue and replaces it with explicit GMutex, GCond and wakeup count which is used for the non-live case. For live pipelines, the aggregator waits on the clock until either data arrives on all sink pads or the expected output buffer time arrives plus the timeout/latency at which time, the subclass produces a buffer. https://bugzilla.gnome.org/show_bug.cgi?id=741146 commit a7e86751bf0c980fbdb8fb4db1acff5bec691a10 Author: Matthew Waters <matthew@centricular.com> Date: Mon Dec 8 15:18:25 2014 +1100 videoaggregator: always try to use newer buffers instead of dropping them for being too old. This ensures that the newest buffer is always used for rendering