GNOME Bugzilla – Bug 787560
audio: Add helper object for audio discontinuity detection
Last modified: 2017-09-28 11:21:06 UTC
See commit message
Created attachment 359554 [details] [review] audio: Add helper object for audio discontinuity detection This is the same code that is in decklinkaudiosrc, audioringbuffer, audiomixer and various other places. Have it once instead of copying it everywhere.
Created attachment 359620 [details] [review] audio: Add helper object for audio discontinuity detection This is the same code that is in decklinkaudiosrc, audioringbuffer, audiomixer and various other places. Have it once instead of copying it everywhere.
Created attachment 359623 [details] [review] audio: Add reverse playback support to GstAudioDiscontDetect
Now only has to be used in: - audiobasesink - audiobuffersplit - audiomixer base class - decklinkaudiosrc/sink - splitmuxsink ... and audiorate to improve on the current somewhat suboptimal behaviour.
Created attachment 359624 [details] [review] audio: Add helper object for audio discontinuity detection This is the same code that is in decklinkaudiosrc, audioringbuffer, audiomixer and various other places. Have it once instead of copying it everywhere.
Created attachment 359628 [details] [review] audio: Add helper object for audio discontinuity detection This is the same code that is in decklinkaudiosrc, audioringbuffer, audiomixer and various other places. Have it once instead of copying it everywhere.
Created attachment 359629 [details] [review] audio: Add reverse playback support to GstAudioDiscontDetect
Created attachment 359631 [details] [review] audiobuffersplit: Use new GstAudioDiscontDetect API
Created attachment 359632 [details] [review] audiobuffersplit: Drain pending samples if the caps are changing
API looks fine, but I don't quite like the name yet (surprise!). It does more than detect disconts. How about GstAudioContinuityTracker or GstAudioPositionTracker or somesuch?
Created attachment 359718 [details] [review] audio: Add helper object for audio discontinuity detection and sample alignment This is the same code that is in decklinkaudiosrc, audioringbuffer, audiomixer and various other places. Have it once instead of copying it everywhere.
Created attachment 359719 [details] [review] audio: Add reverse playback support to GstAudioStreamAlign
Created attachment 359721 [details] [review] audio: Add stream align API for getting timestamp at discont and number of samples since discont
Created attachment 359723 [details] [review] audiobuffersplit: Use new GstAudioStreamAlign API
Created attachment 359724 [details] [review] audiobuffersplit: Drain pending samples if the caps are changing
Worksforme :) If you're not going to make all elements use it, please open enhancement bugs for elements that need to be ported still.
Ok, any other comments before I merge this? :)
Review of attachment 359718 [details] [review]: Looks good in general, I have couple of questions, nothing major really. ::: gst-libs/gst/audio/gstaudiostreamalign.c @@ +26,3 @@ +#include "gstaudiostreamalign.h" + +G_DEFINE_BOXED_TYPE (GstAudioStreamAlign, gst_audio_stream_align, I was wondering between GstAudioStreamAlignment vs GstAudioStreamAlign, we have GstVidoAlginment, but GstAllocationParams.align. So I guess it makes it's pretty equal, just mentionning, nothing to change. @@ +29,3 @@ + (GBoxedCopyFunc) gst_audio_stream_align_copy, + (GBoxedFreeFunc) gst_audio_stream_align_free); + Maybe a SECTION block to help document ? @@ +72,3 @@ + align = g_new0 (GstAudioStreamAlign, 1); + align->rate = rate; + align->alignment_threshold = 40 * GST_MSECOND; Why isn't this parameters like the rate? They are configurable in the sink, then the base class fixate the defaults no ? I also remember a recent thread indicating a possible bug that after fix allow reducing greatly the thresholds, so maybe we shoulnd't scatter the defaults ?
Review of attachment 359719 [details] [review]: Looks good.
Review of attachment 359721 [details] [review]: Optional comment for this one. ::: gst-libs/gst/audio/gstaudiostreamalign.h @@ -31,2 @@ GST_EXPORT -GType gst_audio_stream_align_get_type (void); the reformatting is nice, by quite unrelated. It's nitpicking, bug maybe we could have slip it up ?
Review of attachment 359723 [details] [review]: ::: gst/audiobuffersplit/gstaudiobuffersplit.c @@ +154,3 @@ + self->stream_align = gst_audio_stream_align_new (48000); + gst_audio_stream_align_set_alignment_threshold (self->stream_align, + DEFAULT_ALIGNMENT_THRESHOLD); This is a good argument toward moving the alignment threshold as a paramter to _new(). @@ +156,3 @@ + DEFAULT_ALIGNMENT_THRESHOLD); + gst_audio_stream_align_set_discont_wait (self->stream_align, + DEFAULT_DISCONT_WAIT); And maybe this one too then ? @@ +389,3 @@ + || GST_BUFFER_FLAG_IS_SET (buffer, GST_BUFFER_FLAG_RESYNC), + GST_BUFFER_PTS (buffer), gst_buffer_get_size (buffer) / bpf, NULL, NULL, + NULL); I would create couple of local variable to make this more readable. ::: gst/audiobuffersplit/gstaudiobuffersplit.h @@ +53,3 @@ GstAdapter *adapter; + GstAudioStreamAlign *stream_align; The new API is not thread safe, and this is access through property. Maybe document the locking ? OBJECT_LOCK ?
Review of attachment 359724 [details] [review]: ::: gst/audiobuffersplit/gstaudiobuffersplit.c @@ +498,3 @@ + } + self->info = info; + gst_audio_stream_align_set_rate (self->stream_align, self->info.rate); Locking ?
Attachment 359718 [details] pushed as ec1e20f - audio: Add helper object for audio discontinuity detection and sample alignment Attachment 359719 [details] pushed as d2fd740 - audio: Add reverse playback support to GstAudioStreamAlign Attachment 359721 [details] pushed as bf68e74 - audio: Add stream align API for getting timestamp at discont and number of samples since discont
(In reply to Nicolas Dufresne (stormer) from comment #18) > @@ +26,3 @@ > +#include "gstaudiostreamalign.h" > + > +G_DEFINE_BOXED_TYPE (GstAudioStreamAlign, gst_audio_stream_align, > > I was wondering between GstAudioStreamAlignment vs GstAudioStreamAlign, we > have GstVidoAlginment, but GstAllocationParams.align. So I guess it makes > it's pretty equal, just mentionning, nothing to change. Align would be a verb, this thing is doing something (-> verb) while GstVideoAlignment is just data (-> noun). GstAllocationParams.align is an "order", please align that much (-> verb). IMHO this all makes sense as is :) > @@ +29,3 @@ > + (GBoxedCopyFunc) gst_audio_stream_align_copy, > + (GBoxedFreeFunc) gst_audio_stream_align_free); > + > > Maybe a SECTION block to help document ? Done > @@ +72,3 @@ > + align = g_new0 (GstAudioStreamAlign, 1); > + align->rate = rate; > + align->alignment_threshold = 40 * GST_MSECOND; > > Why isn't this parameters like the rate? They are configurable in the sink, > then the base class fixate the defaults no ? I also remember a recent thread > indicating a possible bug that after fix allow reducing greatly the > thresholds, so maybe we shoulnd't scatter the defaults ? Added to the constructor now, good idea :) (In reply to Nicolas Dufresne (stormer) from comment #20) > Review of attachment 359721 [details] [review] [review]: > > Optional comment for this one. > > ::: gst-libs/gst/audio/gstaudiostreamalign.h > @@ -31,2 @@ > GST_EXPORT > -GType gst_audio_stream_align_get_type > (void); > > the reformatting is nice, by quite unrelated. It's nitpicking, bug maybe we > could have slip it up ? The reformatting is only needed because of this new commit :) It requires horizontal indentation to be increased.
Attachment 359723 [details] pushed as dd490e1 - audiobuffersplit: Use new GstAudioStreamAlign API Attachment 359724 [details] pushed as d01724a - audiobuffersplit: Drain pending samples if the caps are changing