Created attachment 281186 [details] [review]
Patch to handle more than 18 channel and wrong channel masks

This has a similar background as #733444

This patch is important for the following use cases:

1) File to file processing: since playback is irrelevant, the channel mapping
is not important.

2) input of many channels, which are in a second step (another plug-in)
rendered to the existing loudspeaker set-up. This is what happens for example
in MPEG-H, the new standard for 3D audio.

3) In the professional market, you really want to play let's say 22 channels,
but then you have the hardware and the know-how to route each channel to the
required audio output.

Review of attachment 281186 [details] [review]:

This code could benefit some unit test, this is a behaviour change (e.g. this code can hide existing layout information).

::: gst-libs/gst/riff/riff-media.c
@@ +1036,3 @@
+    gst_caps_set_simple (caps, "channel-mask", GST_TYPE_BITMASK, 0, NULL);
+    return TRUE;
+  }

Ok, this seems to match GST_AUDIO_CHANNEL_POSITION_NONE definition. Maybe a better warning/comment would help direct people to the doc.

@@ +1045,3 @@
+            "are channels! Setting channel-mask to 0.");
+        channel_mask = 0;
+        break;

I'm not sure about this one. This would mean you tolerate a file with let's say N channels having a position set, and M additional channels without, and expose it without any layout information. Is that really wanted ?

I'm tend to say it's find to accept multi-channels file without layout information, but when it contains partial layout information, dropping it does not sound right to me.

@@ +1057,3 @@
+        "supposed to be %d channels! Setting channel-mask to 0.",
+        p, num_channels);
+    channel_mask = 0;

Isn't this check somewhat redundant (warning twice if p > num_chanels). This code would gain a little cleanup. Though, same comment apply, shall we reject the layout information completely ?

Partial layout information is unlikely to be very useful (or supported by any elements for that matter). In practice you want layout information for all channels or no layout information for any of the channels, so setting all to none seems a reasonable fallback in that case.

(In reply to comment #4)
> Partial layout information is unlikely to be very useful (or supported by any
> elements for that matter). In practice you want layout information for all
> channels or no layout information for any of the channels, so setting all to
> none seems a reasonable fallback in that case.

Ok, agreed.

After looking at some WAVEFORMATEXTENSIBLE examples, I think Nicolas has a point.

WAVEFORMATEXTENSIBLE allows partial layout information (and I downloaded wav-file, which has it), so we should allow it, too, to not loose information.

Microsoft has an example: http://msdn.microsoft.com/en-us/library/windows/hardware/dn653308%28v=vs.85%29.aspx.
At this side there is an example wav file: http://www-mmsp.ece.mcgill.ca/Documents/AudioFormats/WAVE/Samples.html.
And with the MPEG-H standard, there are more use cases (Channels + Object + HOA).

If other plug-ins don't support it, we have to fix them if needed.
That means with this patch:
- files with more bits in channel_mask than channels are read, instead of giving an error.
- files with partial valid information are read and give perhaps later in the pipeline an error, instead of giving an error while reading the file.

Created attachment 287661 [details] [review]
riff-media: allow more channel_masks

Created attachment 287662 [details] [review]
audio-channels: allow partially valid channel_mask

Any comments to the patch?

(In reply to comment #6)

> That means with this patch:
> - files with more bits in channel_mask than channels are read, instead of
> giving an error.

More bits in the channel mask than the number of channels is used to mark an unfixed channel mask. audioconvert for example is then selecting a subset of those bits during negotiation.
So this part at least seems wrong.


But you only want the inverse, right? Less bits in the channel mask than channels read, and having all additional channels set to an undefined position? That change would seem possible, but I think it will need changes in audioconvert to handle that properly first.

No, in the original code the function returned FALSE if there were more bits in the mask than channels. And the only function which called it then went:

if ((channel_mask != 0 || strf->channels > 1) &&
            !gst_riff_wavext_add_channel_mask (caps, strf->channels,
                channel_mask, channel_reorder_map)) {
          GST_WARNING ("failed to add channel layout");
          gst_caps_unref (caps);
          caps = NULL;
        }

That means the reading of the file failed.

But I want to have both, that files are read if there are too many or too less bits set, which was until now not the case.

I can look into audioconvert, but I think it makes sense to first enable the reading of these files and then fix any plug-ins which cannot cope with it.

More bits than channels already has a different meaning...

What should it mean in your case if there are more channel-mask bits set than there are channels?

This is not about the channel mask used by gstreamer internally.
This is about the channel mask as read from WAVEXT files.
Currently files where the mask does not match the number of channels are not read.
I just want gstreamer to read these files.
In case of too many bits, I set the internal mask to 0, so that the files are at least read.

That makes sense, yes. Sorry for the confusion

Any comments to the patch?

commit 336d303c52012934e2c2464d58d091d915dd274c
Author: Peter G. Baum <peter@dr-baum.net>
Date:   Fri Oct 3 12:57:52 2014 +0200

    riff-media: allow more channel_masks
    
    Allow partial valid channel masks.
    Set channel mask to 0 for non-valid channel masks.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=733405

commit 59dbed24964dd4764ec0151f561dac23aed0a773
Author: Peter G. Baum <peter@dr-baum.net>
Date:   Fri Oct 3 12:54:17 2014 +0200

    audio-channels: allow partially valid channel_mask
    
    Since WAVEFORMATEXTENSIBLE allows to have more channels than
    bits in the channel mask we should allow this, too, to avoid
    loss of information.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=733405

Nice work, thanks for your patience and hard work.