GNOME Bugzilla – Bug 740987
Fixes to osxaudiosrc and osxaudiosink
Last modified: 2014-12-15 05:56:19 UTC
This is a patchset to consolidate some of the code paths in osxaudiosrc and osxaudiosink, and an implementation of proper caps querying for osxaudiosrc. This has been tested on OS X (10.10) but not iOS.
Created attachment 291920 [details] [review] osxaudio: Consolidate input and output code paths a bit
Created attachment 291921 [details] [review] osxaudio: Move device selection to ringbuffer->open_device() This is conceptually the right thing to do, and allows us to correctly catch errors in device selection as well, which we could not do while creating the ringbuffer.
Created attachment 291922 [details] [review] osxaudio: Make some debug code compile conditionally
Created attachment 291923 [details] [review] osxaudiosink: Move device caps probing to get_caps() This should be preferred to running the probe at device open time.
Created attachment 291924 [details] [review] osxaudio: Clean up a GstCoreAudio -> GstOsxAudioSrc/Sink reference Now that device selection has no sink/source-specific bits, we can have generic device selection for this path. We do need to now track state changes so we can look up the final device_id once the device is open, though.
Created attachment 291925 [details] [review] osxaudio: Move osxaudiosrc-specific code out of the generic path Avoids one layering violation (GstCoreAudio referring to GstOsxAudioSrc).
Created attachment 291926 [details] [review] osxaudiosink: Fix up caps querying a bit This should make caps queries correct in PAUSED and higher as well.
Created attachment 291927 [details] [review] osxaudio: Bind audio device to audio unit early We want to bind the device during open so that subsequent format queries on the audio unit are as specific as possible from that point onwards.
Created attachment 291928 [details] [review] osxaudiosrc: Implement caps probing
Created attachment 291929 [details] [review] osxaudiosink: Only fix up channels/layout for PCM caps while probing It's unlikely that setting a channel layout will do much for AC3/DTS streams. If we find at some point that it does make sense, we can perform the structure copying unconditionally (i.e., the current code is wrong, since AC3/DTS will get two structures now - one with the channel layout, one without).
Created attachment 291930 [details] [review] osxaudiosrc: Probe channel layout too
Just for completeness, I've tested the code on a USB webcam mic, a USB headset, and a BT headset, so a few different sample rates and mono and stereo input.
Review of attachment 291920 [details] [review]: Good to push after this trivial change: ::: sys/osxaudio/gstosxaudiosrc.c @@ +333,1 @@ gst_osx_audio_src_select_device (GstOsxAudioSrc * osxsrc) Why not static anymore? Also why is this a separate function anyway that only calls a single other function? :)
Review of attachment 291921 [details] [review]: ::: sys/osxaudio/gstosxaudiosink.c @@ +449,3 @@ ringbuffer->core_audio->is_src = FALSE; + gst_osx_audio_sink_set_volume (osxsink); Setting volume without having selected the device yet? Shouldn't this move to select_device()?
Review of attachment 291923 [details] [review]: ::: sys/osxaudio/gstosxaudiosink.c @@ +310,3 @@ { GstOsxAudioSink *sink = GST_OSX_AUDIO_SINK (base); + GstAudioRingBuffer *buf = GST_AUDIO_BASE_SINK (sink)->ringbuffer; This might also need to be protected by a mutex, not sure @@ +324,1 @@ if (sink->cached_caps) { Probing is protected by the mutex... but sink->cached_caps not?
Review of attachment 291924 [details] [review]: ::: sys/osxaudio/gstosxaudiosink.c @@ +276,3 @@ + ringbuffer = + GST_OSX_AUDIO_RING_BUFFER (GST_AUDIO_BASE_SINK (osxsink)->ringbuffer); + osxsink->device_id = ringbuffer->core_audio->device_id; g_object_notify() for the property maybe ::: sys/osxaudio/gstosxaudiosrc.c @@ +233,3 @@ + ringbuffer = + GST_OSX_AUDIO_RING_BUFFER (GST_AUDIO_BASE_SRC (osxsrc)->ringbuffer); + osxsrc->device_id = ringbuffer->core_audio->device_id; g_object_notify() too
Review of attachment 291926 [details] [review]: ::: sys/osxaudio/gstosxaudiosink.c @@ +361,3 @@ + if (buf->acquired) { + /* Caps are fixed, use what we have */ + ret = gst_pad_get_current_caps (GST_BASE_SINK_PAD (sink)); Maybe add an assertion/warning here if ret==NULL
Comment on attachment 291927 [details] [review] osxaudio: Bind audio device to audio unit early Also relevant for remoteio? Did you check if the remoteio bits still compile btw?
Review of attachment 291928 [details] [review]: ::: sys/osxaudio/gstosxaudiosrc.c @@ +301,3 @@ + /* Caps are fixed, use what we have */ + ret = gst_pad_get_current_caps (GST_BASE_SINK_PAD (src)); + Assertion/warning here maybe ::: sys/osxaudio/gstosxcoreaudio.c @@ +239,3 @@ + + ret = gst_caps_new_simple ("audio/x-raw", + "format", G_TYPE_STRING, format, You might want to use a GstAudioInfo here that is filled from the stuff above, and then just convert that to caps. E.g. you're currently missing the "layout" field :) Probably the same in the sink specific code?
Review of attachment 291930 [details] [review]: Looks good but are we handling channel reordering correctly in sink and src? We have a fixed channel order given by the bit position of a channel in the mask, which might be different than the order expected by coreaudio
I've update patches with all the comments so far at: http://cgit.freedesktop.org/~arun/gst-plugins-good Now tested on iOS as well. Will push out soon unless there are comments/concerns. One pending item is verifying that cached_caps are cleaned up at the correct point in src and sink -- will look at this before pushing.
Created attachment 292592 [details] [review] osxaudio: Consolidate input and output code paths a bit Incorporated review comments (mostly squashed, 2 new patches).
Created attachment 292593 [details] [review] osxaudio: Move device selection to ringbuffer->open_device() This is conceptually the right thing to do, and allows us to correctly catch errors in device selection as well, which we could not do while creating the ringbuffer.
Created attachment 292594 [details] [review] osxaudio: Make some debug code compile conditionally
Created attachment 292595 [details] [review] osxaudiosink: Move device caps probing to get_caps() This should be preferred to running the probe at device open time.
Created attachment 292596 [details] [review] osxaudio: Clean up a GstCoreAudio -> GstOsxAudioSrc/Sink reference Now that device selection has no sink/source-specific bits, we can have generic device selection for this path. We do need to now track state changes so we can look up the final device_id once the device is open, though.
Created attachment 292597 [details] [review] osxaudio: Move osxaudiosrc-specific code out of the generic path Avoids one layering violation (GstCoreAudio referring to GstOsxAudioSrc).
Created attachment 292598 [details] [review] osxaudiosink: Fix up caps querying a bit This should make caps queries correct in PAUSED and higher as well.
Created attachment 292599 [details] [review] osxaudio: Bind audio device to audio unit early We want to bind the device during open so that subsequent format queries on the audio unit are as specific as possible from that point onwards.
Created attachment 292600 [details] [review] osxaudiosrc: Implement caps probing
Created attachment 292601 [details] [review] osxaudiosink: Only fix up channels/layout for PCM caps while probing It's unlikely that setting a channel layout will do much for AC3/DTS streams. If we find at some point that it does make sense, we can perform the structure copying unconditionally (i.e., the current code is wrong, since AC3/DTS will get two structures now - one with the channel layout, one without).
Created attachment 292602 [details] [review] osxaudiosrc: Probe channel layout too
Created attachment 292603 [details] [review] osxaudio: Take lock around sink/source before accessing the ringbuffer
Created attachment 292604 [details] [review] osxaudiosink: Add some error handling around channel layout parsing For now we just spit a warning and ignore the channel layout if we can't support it.
Review of attachment 292598 [details] [review]: ::: sys/osxaudio/gstosxaudiosink.c @@ +367,3 @@ + /* Caps are fixed, use what we have */ + ret = gst_pad_get_current_caps (GST_BASE_SINK_PAD (sink)); + g_return_val_if_fail (ret, NULL); This would forget to unlock the mutex
Review of attachment 292600 [details] [review]: ::: sys/osxaudio/gstosxaudiosrc.c @@ +301,3 @@ + /* Caps are fixed, use what we have */ + ret = gst_pad_get_current_caps (GST_BASE_SINK_PAD (src)); + g_return_val_if_fail (ret, NULL); Forget to unlock @@ +316,3 @@ + if (filter) { + GstCaps *tmp; + tmp = gst_caps_intersect_full (ret, filter, GST_CAPS_INTERSECT_FIRST); We usually have the filter first here as we ideally output what the other elements want
Review of attachment 292603 [details] [review]: Contains a change in common
The following fixes have been pushed: db91486 osxaudiosink: Prefer filter caps order while getting caps f573f02 osxaudiosink: Add some error handling around channel layout parsing d18a6b0 osxaudio: Take lock around sink/source before accessing the ringbuffer 4a58ebf osxaudiosrc: Probe channel layout too df610a7 osxaudiosink: Only fix up channels/layout for PCM caps while probing bd15028 osxaudiosrc: Implement caps probing 48872db osxaudio: Bind audio device to audio unit early 2d0391c osxaudiosink: Fix up caps querying a bit f967f07 osxaudio: Move osxaudiosrc-specific code out of the generic path ffcb157 osxaudio: Clean up a GstCoreAudio -> GstOsxAudioSrc/Sink reference 5c2f041 osxaudiosink: Move device caps probing to get_caps() 945aaa0 osxaudio: Make some debug code compile conditionally b06ae28 osxaudio: Move device selection to ringbuffer->open_device() 199461b osxaudio: Consolidate input and output code paths a bit
Created attachment 292732 [details] [review] osxaudiosink: Prefer filter caps order while getting caps
Created attachment 292733 [details] [review] osxaudiosink: Add some error handling around channel layout parsing For now we just spit a warning and ignore the channel layout if we can't support it.