Do you want to work on this? What is the actual value that is set by iOS 12 there and how is it to be handled, do you have a link to the relevant docs?

Hi Sebastian,

We took a look and got a fix. However, we're a little unclear as to how to submit a patch. Could you point me in the right direction to the instructions?

Cheers,

Michael

(In reply to Michael from comment #2)
> We took a look and got a fix. However, we're a little unclear as to how to
> submit a patch. Could you point me in the right direction to the
> instructions?

Hi Micheal,

to submit a patch, commit your changes with a proper commit message in your local git tree. Then you can extract this change as a patch file, using "git format-patch -1". Finally, attach this patch to the bugzilla. We will proceed with a code review, if there is any change, update the patch and resubmit, otherwise, we'll merge it in to the main tree.

Alternatively, you may want to use git-bz extension, a git extension that will handle the uploading of attachment to bugzilla for you.

https://fishsoup.net/software/git-bz/

Created attachment 373418 [details] [review]
This is a patch for parsing the channel layout on osxaudio

Thanks Nicolas!

I just attached the patch. Please let me know if there are any concerns.

Cheers,

Michael

Review of attachment 373418 [details] [review]:

Ok, about the commit message, should be in the following form:


  osxcoreaudio: Short description
  <empty line>
  Longer description
  <empty line>
  <bug URI if available>

The short description should not exceed 80 characters.

::: sys/osxaudio/gstosxcoreaudio.c
@@ +457,3 @@
   }
 
+  GstAudioChannelPosition pos[GST_OSX_AUDIO_MAX_CHANNEL];

In general we try not to mix code and declaration, please move this at the start of your scope ({).

@@ +461,3 @@
     if (!gst_core_audio_parse_channel_layout (layout, &channels, &channel_mask,
             pos)) {
       GST_WARNING ("Failed to parse channel layout");

Maybe the warning should explain a bit what is going to happen, specially that it won't error out.

@@ -463,3 @@
             pos)) {
       GST_WARNING ("Failed to parse channel layout");
-      goto error;

Is this label still used ? Just double checking.

We are running into this issue in our app as well so thanks for putting this together! Coming from a total novice in terms of the inner workings of GStreamer this may be a dumb question: is it an issue if the channels, mask and pos aren't set? Could we map the possible values for mChannelLayoutTag to the number of channels, channel_mask, and pos?

So after some conversation on the Apple dev forums, it was pointed out there's a method to get the number of channels from an AudioChannelLayout: AudioChannelLayoutTag_GetNumberOfChannels(AudioChannelLayoutTag inLayoutTag) which we could use in gst_core_audio_parse_channel_layout. Unfortunately, it doesn't look like there's a method for dynamically retrieving the AudioChannelLabel which means we can't dynamically set channel_mask and pos.

I haven't updated to iOS 12 yet, based on the info you provided by email I'll try something like this:

if (layout->mChannelLayoutTag == kAudioChannelLayoutTag_Stereo){
   if (pos) {
    pos[0] = GST_AUDIO_CHANNEL_POSITION_FRONT_LEFT;
    pos[1] = GST_AUDIO_CHANNEL_POSITION_FRONT_RIGHT;
   }
   *channels = 2;
   *channel_mask = GST_AUDIO_CHANNEL_POSITION_MASK (FRONT_LEFT) |
          GST_AUDIO_CHANNEL_POSITION_MASK (FRONT_RIGHT);
   return TRUE;
}

this code should be added here:

https://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/sys/osxaudio/gstosxcoreaudio.c#n342

before 

if (layout->mChannelLayoutTag !=
      kAudioChannelLayoutTag_UseChannelDescriptions) {

any news on this? iOS 12 is released

both the attached patch and the approach explained in comment 9 solve the issue,

I think the proposed patch is good enough since gst_audio_info_set_format is a good fallback and basically it does the same of the code proposed in comment 9

It also looks more like a workaround to me, which we should probably still include though (with a GST_WARNING()). It would be better to actually try parsing the channel layout, like what Nicola mentioned in comment 9 (but probably more cases), and as a fallback what the current patch here does.

Does someone want to provide an updated patch? See also Nicolas' review comments in comment 6.

Sebastian, if you want I can provide an updated patch but I cannot test all the layouts on my devices, 

for the ones I can test parsing the layout give the same results as calling gst_audio_info_set_format,

I need some info to map iOS layout to GstAudioChannelPosition too, specifically:

how do positions W,X,Y,Z (see kAudioChannelLayoutTag_Ambisonic_B_Format), Left matrix total, Right matrix, Sides (1 position), Center surround,  map to GstAudioChannelPosition? thanks

(In reply to Nicola from comment #13)
> Sebastian, if you want I can provide an updated patch but I cannot test all
> the layouts on my devices, 
> 
> for the ones I can test parsing the layout give the same results as calling
> gst_audio_info_set_format,

For the other ones, maybe add some code if you're somewhat sure how they would look like, and add a GST_FIXME() to the code and then it can still be fixed up at a later time. Having something is better than nothing at all here :)

> I need some info to map iOS layout to GstAudioChannelPosition too,
> specifically:
> 
> how do positions W,X,Y,Z (see kAudioChannelLayoutTag_Ambisonic_B_Format),
> Left matrix total, Right matrix, Sides (1 position), Center surround,  map
> to GstAudioChannelPosition? thanks

We don't have support for ambisonics yet

Created attachment 373743 [details] [review]
proposed patch

I used several if/else since I don't like nested switch clause.

kAudioChannelLayoutTag_Mono, kAudioChannelLayoutTag_Quadraphonic, kAudioChannelLayoutTag_Pentagonal and kAudioChannelLayoutTag_Cube are untested but they should work

commit 3573f71c911de35b61cb4a82d41a39e0f5e060fc (HEAD -> master)
Author: Nicola Murino <nicola.murino@gmail.com>
Date:   Mon Sep 24 11:45:46 2018 +0200

    osxaudio: add support for parsing more channel layouts ...
    
    ... and fallback to gst_audio_info_set_format for not yet supported layouts.
    
    Fix audio playback on iOS 12.
    Based on patch from Byron Schiel <byron@canary.is>
    
    https://bugzilla.gnome.org/show_bug.cgi?id=796919

I'll backport this to 1.14 later