GNOME Bugzilla – Bug 795486
aggregator: Add default implementation of get_next_time
Last modified: 2018-05-16 20:44:14 UTC
I just discovered that all 3 subclasses of GstAggregator that we have in the tree have copy pasted implementation so get_next_time(), so I moved it to the base class and removed the copies from the subclasses. So there is one patch for core, one for base, one for good and one for bad!
Created attachment 371291 [details] [review] aggregator: Add default implementation of get_next_time This is the implementation that was in all 3 subclasses: audio, video and flvmux.
Created attachment 371292 [details] [review] audioaggregator: Remove get_next_time implementation GstAggregator now does the same thing in the default implementation.
Created attachment 371293 [details] [review] flvmux: Remove get_next_time implementation GstAggregator now does the same thing in the default implementation.
Created attachment 371294 [details] [review] videoaggregator: Remove get_next_time implementation GstAggregator now does the same thing in the default implementation.
Review of attachment 371291 [details] [review]: This is sensibly changing the behavior of the base class but if all (known) subclassed were doing the same I guess it is OK.
Created attachment 371691 [details] [review] aggregator: Add get_next_time function for live streams Instead of making it the default implementation, let's make it a helper. Sebastian rightfully reminded me that there are some formats where you can't deal with "holes" in the stream.
Created attachment 371692 [details] [review] audioaggregator: Remove custom get_next_time implementation GstAggregator now offers same thing in a common implementation.
Created attachment 371693 [details] [review] flvmux: Remove custom get_next_time implementation GstAggregator now does the same thing in the simple implementation.
Created attachment 371694 [details] [review] videoaggregator: Remove custom get_next_time implementation GstAggregator now has the same thing in the simple implementation.
mxfmux uses aggregator but has no get_next_time. It can't handle timeouts in live, and would be erroring out whenever that happens. Apart from that, moving this in one way or another to the base class seems useful but IMHO a breaking change. Subclasses without a get_next_time implementation are currently waiting forever (e.g. mxfmux) in live mode, but with this change would potentially timeout.
(forgot to add this comment last weekend)
Comment on attachment 371691 [details] [review] aggregator: Add get_next_time function for live streams Attachment 371691 [details] pushed as 05298b3 - aggregator: Add get_next_time function for live streams Updated version is based on slomo's comments, I'm just adding a new function that can be used by subclassed without making it the default behaviour.
Comment on attachment 371692 [details] [review] audioaggregator: Remove custom get_next_time implementation Attachment 371692 [details] pushed as 8583f17 - audioaggregator: Remove custom get_next_time implementation
Comment on attachment 371693 [details] [review] flvmux: Remove custom get_next_time implementation Attachment 371693 [details] pushed as 87b2b35 - flvmux: Remove custom get_next_time implementation
Attachment 371694 [details] pushed as feb6002 - videoaggregator: Remove custom get_next_time implementation