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

[GStreamer (GNOME Bugzilla)]

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

--- Comment #34 from Ilya Konstantinov <ilya.konstantinov at gmail.com> ---
(In reply to Arun Raghavan from comment #30)
> Review of attachment 298927 [details] [review]:
> 
> ::: 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.

GstBaseSrc::get_caps return template caps.
GstBaseSink::get_caps return NULL.

I don't know why they default to different behaviors; I assumed they were
written by smart people and followed the example. (In the GST_DEBUG_OBJECT, I
document what the base class does, but in reality, I should've just written "no
ring buffer, calling base class".)

> @@ +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?

Actually, I should've done without cached_caps_valid, and then introduced it in
attachment 298928 where I've introduced _audio_unit_property_listener.

The problem was that in _audio_unit_property_listener I couldn't safely
GST_LOCK_OBJECT(osxbuf) since sometimes _audio_unit_property_listener would be
called on the same thread with osxbuf already locked.

In reality, I think _audio_unit_property_listener is only called on the same
thread in reaction to *me* changing inner scope formats. Outer scope changes
(i.e. hardware format changes) are reported on an auxiliary system thread.

So perhaps if we only lock osxbuf for outer scope changes, we can do without
cached_caps_valid.

What do you think?

> ::: 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).

I suppose. Maybe it's clearer.

> ::: 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?

I like your proposal better. Since there's no "right" thing to do here, we
might as well choose the easier-to-understand option.

HOWEVER, no matter what we decide, we should still skip over
kAudioChannelLabel_Unknown channels. For example, in a fully-positioned
scenario, the Soundflower 64-channel device reports kAudioChannelLabel_Unknown
for all unassigned 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:

OK, will change.

> @@ +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?

I think you meant to comment about gst_core_audio_probe_caps. But yes, agreed.

In any case, this is why I *warn* -- because if we omit channel-mask, we might
have an element sending (L,R) to a sink accepting (RL,RR) etc.

I'm not going to bail out over this, since AU will do its own reordering *and
negotiation* -- meaning, for (L,R)->(LR,RR) nothing will be played. I'm just
warning.

> ::: 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.

That's a pretty common pattern for "flexible array member" structs. I've seen
it in the Windows NT sources, and Apple also uses it in their AU sample code
(with the non-GLib offsetof).

I wish Glib had a more indicative macro, like:

 #define G_VAR_ARRAY_SIZE (Type,VarSizeMember,Length) G_STRUCT_OFFSET(Type,
VarSizeMember[Length])

but anyway, I feel strongly about keeping it :) I can comment on it if you
wish.

> @@ +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().

Agreed.

> @@ +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?

I've listed all the known positional labels that I found no suitable GST
mapping for. It's mostly for documentation's sake, I guess.

Do you think it confuses more than it helps?

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