GNOME Bugzilla – Bug 737183
audiomixer: Port to GstAggregator
Last modified: 2014-10-06 17:33:30 UTC
The audiomixer element (replaceement of adder) should be ported to GstAggregator.
Created attachment 286879 [details] [review] audiomixer: Port to GstAggregator
Created attachment 286885 [details] [review] audiomixer: Port to GstAggregator https://bugzilla.gnome.org/show_bug.cgi?id=737183 Co-Authored by: Mathieu Duponchelle <mathieu.duponchelle@opencreed.com>
Ping? :)
Review of attachment 286885 [details] [review]: Generally looks good and straightforward :) Thanks! ::: gst/audiomixer/gstaudiomixer.c @@ +413,3 @@ + audiomixer->offset = gst_util_uint64_scale (agg->segment.position, + GST_AUDIO_INFO_RATE (&audiomixer->info), GST_SECOND); Why do you set the offset when receiving a caps event? I would understand if you would update the offset with the current rate, and then update the rate from the caps... but that doesn't happen here. I think for rate changes we would need to store an offset, and then use that to offset all following timestamps Also make sure that segment.position can never be -1 here... otherwise bad stuff happens :) @@ +820,3 @@ + return FALSE; + + gst_buffer_replace (&audiomixer->current_buffer, NULL); offsets and all that should probably be also reset here ::: tests/check/elements/audiomixer.c @@ +983,3 @@ ret = gst_pad_chain (sinkpad, buffer); + main_loop = local_mainloop; + g_timeout_add (100, (GSourceFunc) _quit, main_loop); Why? A comment would be good, timeouts/sleep in tests are always suspicious :)
Bug #734499 feels relevant for the segment.position comment ;)
Created attachment 287818 [details] [review] aggregator: Query seeking when a seek failed to see if it was expected And do not worry if seeking failed on a stream that is not seekable
Created attachment 287819 [details] [review] audiomixer: Port to GstAggregator https://bugzilla.gnome.org/show_bug.cgi?id=737183 Co-Authored by: Mathieu Duponchelle <mathieu.duponchelle@opencreed.com>
Created attachment 287822 [details] [review] Updated patch with: * New offset setting happens right after actually setting srcpad caps Making * sure that a '-1' position will not be used when setting the offset Adding a * comment about why we need to do a timeout based test to check if aggregation actually happens (or not).
commit dd65d70f65c72a8d5cfc5560787ead90f4982b66 Author: Thibault Saunier <tsaunier@gnome.org> Date: Wed May 28 16:29:37 2014 +0200 audiomixer: Port to GstAggregator https://bugzilla.gnome.org/show_bug.cgi?id=737183 Co-Authored by: Mathieu Duponchelle <mathieu.duponchelle@opencreed.com>