After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 737127 - interleave: interleaving does not respect the channel positions default order
interleave: interleaving does not respect the channel positions default order
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-09-22 16:10 UTC by Antonio Ospite
Modified: 2014-10-02 07:22 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
examples to show how interleave ignores the channel positions default ordering (2.22 KB, application/x-bzip)
2014-09-22 16:10 UTC, Antonio Ospite
  Details
interleave samples following the Default Channel Ordering (4.94 KB, patch)
2014-09-23 12:56 UTC, Antonio Ospite
needs-work Details | Review
interleave: interleave samples following the Default Channel Ordering (4.27 KB, patch)
2014-09-29 14:35 UTC, Antonio Ospite
committed Details | Review

Description Antonio Ospite 2014-09-22 16:10:47 UTC
Created attachment 286823 [details]
examples to show how interleave ignores the channel positions default ordering

Hi,

this report results from some investigation I did with regards to bug 735673 (and maybe bug 708988 is connected too).

First some background.

With the two following pipelines I was getting exactly the same file (despite the swapped channel-mask properties):

gst-launch-1.0 interleave name=i ! audioconvert ! wavenc ! filesink location=audio-interleave-test_1.wav audiotestsrc wave=0 num-buffers=100 ! audioconvert ! "audio/x-raw,channels=1,channel-mask=(bitmask)0x1" ! queue ! i.sink_0 audiotestsrc wave=2 num-buffers=100 ! audioconvert ! "audio/x-raw,channels=1,channel-mask=(bitmask)0x2" ! queue ! i.sink_1

gst-launch-1.0 interleave name=i ! audioconvert ! wavenc ! filesink location=audio-interleave-test_2.wav audiotestsrc wave=0 num-buffers=100 ! audioconvert ! "audio/x-raw,channels=1,channel-mask=(bitmask)0x2" ! queue ! i.sink_0 audiotestsrc wave=2 num-buffers=100 ! audioconvert ! "audio/x-raw,channels=1,channel-mask=(bitmask)0x1" ! queue ! i.sink_1

I would have expected the channels to be swapped in the second file.

The same behaviour can be obtained by setting the channel-positions property in code, with a different order; see the attached archive with two examples with switched channel positions.

This is the diff between the two example programs:

--- interleave_wav_left-right.c 2014-09-22 13:30:10.878415269 +0200
+++ interleave_wav_right-left.c 2014-09-22 13:31:04.774844535 +0200
@@ -73,7 +73,7 @@ make_n_channel_wav (const gint channels,
 
   filesink = gst_element_factory_make ("filesink", NULL);
   fail_unless (filesink != NULL);
-  g_object_set (G_OBJECT (filesink), "location", "left-right.wav", NULL);
+  g_object_set (G_OBJECT (filesink), "location", "right-left.wav", NULL);
   gst_bin_add (GST_BIN (pipeline), filesink);
   fail_unless (gst_element_link (wavenc, filesink));
 
@@ -112,10 +112,10 @@ GST_START_TEST (test_encode_stereo)
 
   arr = g_value_array_new (2);
   g_value_init (&val, GST_TYPE_AUDIO_CHANNEL_POSITION);
-  g_value_set_enum (&val, GST_AUDIO_CHANNEL_POSITION_FRONT_LEFT);
+  g_value_set_enum (&val, GST_AUDIO_CHANNEL_POSITION_FRONT_RIGHT);
   g_value_array_append (arr, &val);
   g_value_reset (&val);
-  g_value_set_enum (&val, GST_AUDIO_CHANNEL_POSITION_FRONT_RIGHT);
+  g_value_set_enum (&val, GST_AUDIO_CHANNEL_POSITION_FRONT_LEFT);
   g_value_array_append (arr, &val);
   g_value_unset (&val);

They also produce the exact same file.

AFAIU a "channel position" in this context is meant to indicate the loudspeaker position in the "physical" sense, it's not about the positions of the channel in the wav stream, however this still affects the order of channels in the stream: according to http://msdn.microsoft.com/en-us/library/windows/hardware/dn653308%28v=vs.85%29.aspx the "Default Channel Ordering" together with dwChannelMask are enough to represent a complete mapping between channel position and loudspeaker position.

Right now, the interleave element is able to retrieve the info about channel positions correctly (even when passing channel-mask via caps in the source pads), but then it ignores it when actually interleaving the samples.

So I think the interleave element should interleave the samples reordering the channels to follow the Default Ordering.

To be more explicit, in the example above, if sink_0 is RIGHT and sink_1 is LEFT, the interleave_N() functions should put samples from sink_1 _before_ samples from sink_0 in order to re-enforce the "Default Ordering" from the specification above. Right now, the interleaving is done following the sink order, not the "channel position order".

Please let me know if I was not able to elaborate well enough.

I'll try to some up with a proof of concept patch if you agree with the reasoning above.
Comment 1 Antonio Ospite 2014-09-23 12:56:16 UTC
Created attachment 286870 [details] [review]
interleave samples following the Default Channel Ordering

In order to have a full mapping between channel positions in the audio stream and loudspeaker positions, the channel-mask alone is not enough: the channels must be interleaved following some Default Channel Ordering as mentioned in the WAVEFORMATEXTENSIBLE[1] specification.  

As a Default Channel Ordering we can use the one implied by GstAudioChannelPosition which follows the ordering defined in SMPTE 2036-2-2008[2].

NOTE that the relative order in the Top Layer is not exactly the same as the one from the WAVEFORMATEXTENSIBLE[1] specification, is it safe to assume that users using so may channels are already aware of such discrepancies.

[1] http://msdn.microsoft.com/en-us/library/windows/hardware/dn653308%28v=vs.85%29.aspx
[2] http://www.itu.int/dms_pub/itu-r/opb/rep/R-REP-BS.2159-2-2011-PDF-E.pdf

I am attaching a proof of the concept of what I mean.

In the final patch I can move the ordering map creation in a dedicated function, and maybe use qsort instead of the naive bubblesort used for now.
Comment 2 Sebastian Dröge (slomo) 2014-09-24 07:54:58 UTC
Review of attachment 286870 [details] [review]:

Looks good to me except for some little detail :)

::: gst/interleave/interleave.c
@@ +288,3 @@
+        default_ordering_map[j] = tmp;
+      }
+    }

You could use something like g_qsort_with_data() here, but not really important other than that we don't have our own sorting here then

::: gst/interleave/interleave.h
@@ +61,3 @@
   gboolean channel_positions_from_input;
 
+  gint *default_channels_ordering_map;

You can just store a 64 element array here, no need for dynamic allocation
Comment 3 Antonio Ospite 2014-09-29 14:35:29 UTC
Created attachment 287357 [details] [review]
interleave: interleave samples following the Default Channel  Ordering

Attached a v2 with these changes:
  - using g_qsort_with_data() to sort the channel map
    NOTE, the sorting is done using the foreign keys from pos[].
  - using a static array for the channel map

I was not sure about the second one, but not having a strong opinion, I just complied to your comments.

BTW, maybe this could have also be done with the audio channels functions from gst-plugins-base: http://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst-libs/gst/audio/audio-channels.h

In fact, a similar channel map is built also in /riff/riff-media.c:
http://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst-libs/gst/riff/riff-media.c#n1056

However I don't know if I have the motivation to still work on this.

Thanks,
   Antonio
Comment 4 Sebastian Dröge (slomo) 2014-10-02 07:22:15 UTC
Thanks for the patch :)

commit 7ae7f657fae542f06ba4c22d65a2084f5a4a3a5d
Author: Antonio Ospite <ao2@ao2.it>
Date:   Tue Sep 23 10:48:09 2014 +0200

    interleave: interleave samples following the Default Channel Ordering
    
    In order to have a full mapping between channel positions in the audio
    stream and loudspeaker positions, the channel-mask alone is not enough:
    the channels must be interleaved following some Default Channel Ordering
    as mentioned in the WAVEFORMATEXTENSIBLE[1] specification.
    
    As a Default Channel Ordering use the one implied by
    GstAudioChannelPosition which follows the ordering defined in SMPTE
    2036-2-2008[2].
    
    NOTE that the relative order in the Top Layer is not exactly the same as
    the one from the WAVEFORMATEXTENSIBLE[1] specification; let's hope users
    using so may channels are already aware of such discrepancies.
    
    [1] http://msdn.microsoft.com/en-us/library/windows/hardware/dn653308%28v=vs.85%29.aspx
    [2] http://www.itu.int/dms_pub/itu-r/opb/rep/R-REP-BS.2159-2-2011-PDF-E.pdf
    
    Fixes: https://bugzilla.gnome.org/show_bug.cgi?id=737127