[Bug 743758] osxaudiosrc supports only 44100 sample rate on iOS

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=743758

Arun Raghavan <arun at accosted.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #298927|none                        |needs-work
             status|                            |

--- Comment #30 from Arun Raghavan <arun at accosted.net> ---
Review of attachment 298927:
 --> (https://bugzilla.gnome.org/review?bug=743758&attachment=298927)

::: sys/osxaudio/gstosxaudiosink.c
@@ +338,3 @@
+    GST_DEBUG_OBJECT (sink, "no ring buffer, returning NULL caps");
+    return GST_BASE_SINK_CLASS (parent_class)->get_caps (sink, filter);
+  }

Should return template caps, not NULL.

@@ +358,3 @@
+    caps = gst_core_audio_probe_caps (osxbuf->core_audio, template_caps);
+    gst_caps_replace (&osxbuf->core_audio->cached_caps, caps);
+    osxbuf->core_audio->cached_caps_valid = TRUE;

Can we replace the boolean with a NULL check on cached_caps?

@@ +363,3 @@
+  } else {
+    GST_DEBUG_OBJECT (sink, "ring buffer not open, returning NULL caps");
+    caps = NULL;

Same comment as before about using template caps.

::: sys/osxaudio/gstosxaudiosrc.c
@@ +285,3 @@
+    GST_DEBUG_OBJECT (src, "ring buffer not open, using template caps");
+    caps = GST_BASE_SRC_CLASS (parent_class)->get_caps (src, filter);
+    filter = NULL;

Could just pass NULL to the base class and do filtering ourselves (though I'm
fine with either way).

::: sys/osxaudio/gstosxcoreaudio.c
@@ +202,3 @@
+/* Does the channel have at least one positioned channel?
+  (GStreamer is more strict than Core Audio, in that it requires either
+   all channels to be positioned, or all unpositioned.) */

Do we really need to support the partially-positioned case? And if yes, maybe
it makes more sense to just downgrade that to unpositioned, rather than only
picking up the positioned channels?

@@ +489,3 @@
+  /* Get the ASBD of the outer scope (i.e. input scope of Input,
+     output scope of Output).
+     This ASBD indicates the hardware format. */

GStreamer comment style is:

/* ...
 * ... */

or

/* ...
 * ...
 * ...
 */

@@ +566,3 @@
+       * so we ensure it ourselves in _set_ring_buffer_osx_channel_order
+       * when the ring buffer is acquired.
+       */

Do we need to have both the with-channel-mask and without-channel-mask
variants, then? Can't we just have the version without?

::: sys/osxaudio/gstosxcoreaudiocommon.c
@@ +263,3 @@
+  /* Construct AudioChannelLayout */
+  layoutSize =
+      G_STRUCT_OFFSET (AudioChannelLayout, mChannelDescriptions[channels]);

While I like this for being quite clever, IMO the previous way of allocating
the AudioChannelLayout was easier to read.

@@ +446,3 @@
+ * 'channel' is used for warnings only. */
+GstAudioChannelPosition
+coreaudio_channel_label_to_gst_audio_channel_position (AudioChannelLabel
label,

I wouldn't be against just calling this gst_core_audio_channel_label_to_gst(),
which is also long, but shorter, and a bit more consistent with the rest of the
code (can also make the other function
gst_audio_channel_position_to_core_audio().

@@ +505,3 @@
+    case kAudioChannelLabel_RearSurroundLeft:
+    case kAudioChannelLabel_RearSurroundRight:
+    case kAudioChannelLabel_TopCenterSurround:

I'm not quite sure what these 3 are supposed to map to. Do they overlap with
any of the others?

-- 
You are receiving this mail because:
You are the QA Contact for the bug.