opus: properly create channel mapping tables

There are two of them, unintuitively enough; the one passed
to the encoder should not be the one that gets written to the
file. The former maps the input to an ordering which puts
paired channels first, while the latter moves the channels
to Vorbis order. So add code to calculate both, and we now
have properly paired channels where appropriate.

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

8 files changed, 214 insertions, 52 deletions

diff --git a/ext/opus/gstopuscommon.c b/ext/opus/gstopuscommon.c
index 426c5b897..dbf585a82 100644
--- a/ext/opus/gstopuscommon.c
+++ b/ext/opus/gstopuscommon.c
@@ -17,6 +17,8 @@
  * Boston, MA 02111-1307, USA.
  */
 
+#include <stdio.h>
+#include <string.h>
 #include "gstopuscommon.h"
 
 /* http://www.xiph.org/vorbis/doc/Vorbis_I_spec.html#x1-800004.3.9 */
@@ -86,3 +88,19 @@ const char *gst_opus_channel_names[] = {
   "side right",
   "none"
 };
+
+void
+gst_opus_common_log_channel_mapping_table (GstElement * element,
+    GstDebugCategory * category, const char *msg, int n_channels,
+    const guint8 * table)
+{
+  char s[8 + 256 * 4] = "[ ";   /* enough for 256 times "255 " at most */
+  int n;
+
+  for (n = 0; n < n_channels; ++n) {
+    size_t len = strlen (s);
+    snprintf (s + len, sizeof (s) - len, "%d ", table[n]);
+  }
+  strcat (s, "]");
+  GST_CAT_LEVEL_LOG (category, GST_LEVEL_INFO, element, "%s: %s", msg, s);
+}
diff --git a/ext/opus/gstopuscommon.h b/ext/opus/gstopuscommon.h
index 65b944e9e..1fba5650d 100644
--- a/ext/opus/gstopuscommon.h
+++ b/ext/opus/gstopuscommon.h
@@ -28,6 +28,9 @@ G_BEGIN_DECLS
 
 extern const GstAudioChannelPosition gst_opus_channel_positions[][8];
 extern const char *gst_opus_channel_names[];
+extern void gst_opus_common_log_channel_mapping_table (GstElement *element,
+    GstDebugCategory * category, const char *msg,
+    int n_channels, const guint8 *table);
 
 G_END_DECLS
 
diff --git a/ext/opus/gstopusdec.c b/ext/opus/gstopusdec.c
index 625c47797..7776f5812 100644
--- a/ext/opus/gstopusdec.c
+++ b/ext/opus/gstopusdec.c
@@ -357,9 +357,16 @@ opus_dec_chain_parse_data (GstOpusDec * dec, GstBuffer * buffer)
 
     GST_DEBUG_OBJECT (dec, "Creating decoder with %d channels, %d Hz",
         dec->n_channels, dec->sample_rate);
-    dec->state = opus_multistream_decoder_create (dec->sample_rate,
-        dec->n_channels, dec->n_streams, dec->n_stereo_streams,
-        dec->channel_mapping, &err);
+#ifndef GST_DISABLE_DEBUG
+    gst_opus_common_log_channel_mapping_table (GST_ELEMENT (dec), opusdec_debug,
+        "Mapping table", dec->n_channels, dec->channel_mapping);
+#endif
+
+    GST_DEBUG_OBJECT (dec, "%d streams, %d stereo", dec->n_streams,
+        dec->n_stereo_streams);
+    dec->state =
+        opus_multistream_decoder_create (dec->sample_rate, dec->n_channels,
+        dec->n_streams, dec->n_stereo_streams, dec->channel_mapping, &err);
     if (!dec->state || err != OPUS_OK)
       goto creation_failed;
   }
diff --git a/ext/opus/gstopusenc.c b/ext/opus/gstopusenc.c
index f747a3736..e7d6842a1 100644
--- a/ext/opus/gstopusenc.c
+++ b/ext/opus/gstopusenc.c
@@ -417,7 +417,50 @@ gst_opus_enc_get_frame_samples (GstOpusEnc * enc)
 }
 
 static void
-gst_opus_enc_setup_channel_mapping (GstOpusEnc * enc, const GstAudioInfo * info)
+gst_opus_enc_setup_trivial_mapping (GstOpusEnc * enc, guint8 mapping[256])
+{
+  int n;
+
+  for (n = 0; n < 255; ++n)
+    mapping[n] = n;
+}
+
+static int
+gst_opus_enc_find_channel_position (GstOpusEnc * enc, const GstAudioInfo * info,
+    GstAudioChannelPosition position)
+{
+  int n;
+  for (n = 0; n < enc->n_channels; ++n) {
+    if (GST_AUDIO_INFO_POSITION (info, n) == position) {
+      return n;
+    }
+  }
+  return -1;
+}
+
+static int
+gst_opus_enc_find_channel_position_in_vorbis_order (GstOpusEnc * enc,
+    GstAudioChannelPosition position)
+{
+  int c;
+
+  for (c = 0; c < enc->n_channels; ++c) {
+    if (gst_opus_channel_positions[enc->n_channels - 1][c] == position) {
+      GST_INFO_OBJECT (enc,
+          "Channel position %s maps to index %d in Vorbis order",
+          gst_opus_channel_names[position], c);
+      return c;
+    }
+  }
+  GST_WARNING_OBJECT (enc,
+      "Channel position %s is not representable in Vorbis order",
+      gst_opus_channel_names[position]);
+  return -1;
+}
+
+static void
+gst_opus_enc_setup_channel_mappings (GstOpusEnc * enc,
+    const GstAudioInfo * info)
 {
 #define MAPS(idx,pos) (GST_AUDIO_INFO_POSITION (info, (idx)) == GST_AUDIO_CHANNEL_POSITION_##pos)
 
@@ -427,14 +470,15 @@ gst_opus_enc_setup_channel_mapping (GstOpusEnc * enc, const GstAudioInfo * info)
       enc->n_channels);
 
   /* Start by setting up a default trivial mapping */
-  for (n = 0; n < 255; ++n)
-    enc->channel_mapping[n] = n;
+  enc->n_stereo_streams = 0;
+  gst_opus_enc_setup_trivial_mapping (enc, enc->encoding_channel_mapping);
+  gst_opus_enc_setup_trivial_mapping (enc, enc->decoding_channel_mapping);
 
   /* For one channel, use the basic RTP mapping */
   if (enc->n_channels == 1) {
     GST_INFO_OBJECT (enc, "Mono, trivial RTP mapping");
     enc->channel_mapping_family = 0;
-    enc->channel_mapping[0] = 0;
+    /* implicit mapping for family 0 */
     return;
   }
 
@@ -444,9 +488,11 @@ gst_opus_enc_setup_channel_mapping (GstOpusEnc * enc, const GstAudioInfo * info)
     if (MAPS (0, FRONT_LEFT) && MAPS (1, FRONT_RIGHT)) {
       GST_INFO_OBJECT (enc, "Stereo, canonical mapping");
       enc->channel_mapping_family = 0;
+      enc->n_stereo_streams = 1;
       /* The channel mapping is implicit for family 0, that's why we do not
          attempt to create one for right/left - this will be mapped to the
          Vorbis mapping below. */
+      return;
     } else {
       GST_DEBUG_OBJECT (enc, "Stereo, but not canonical mapping, continuing");
     }
@@ -454,42 +500,115 @@ gst_opus_enc_setup_channel_mapping (GstOpusEnc * enc, const GstAudioInfo * info)
 
   /* For channels between 1 and 8, we use the Vorbis mapping if we can
      find a permutation that matches it. Mono will have been taken care
-     of earlier, but this code also handles it. */
+     of earlier, but this code also handles it. Same for left/right stereo.
+     There are two mappings. One maps the input channels to an ordering
+     which has the natural pairs first so they can benefit from the Opus
+     stereo channel coupling, and the other maps this ordering to the
+     Vorbis ordering. */
   if (enc->n_channels >= 1 && enc->n_channels <= 8) {
+    int c0, c1, c0v, c1v;
+    int mapped;
+    gboolean positions_done[256];
+    static const GstAudioChannelPosition pairs[][2] = {
+      {GST_AUDIO_CHANNEL_POSITION_FRONT_LEFT,
+          GST_AUDIO_CHANNEL_POSITION_FRONT_RIGHT},
+      {GST_AUDIO_CHANNEL_POSITION_REAR_LEFT,
+          GST_AUDIO_CHANNEL_POSITION_REAR_RIGHT},
+      {GST_AUDIO_CHANNEL_POSITION_FRONT_LEFT_OF_CENTER,
+          GST_AUDIO_CHANNEL_POSITION_FRONT_RIGHT_OF_CENTER},
+      {GST_AUDIO_CHANNEL_POSITION_FRONT_LEFT_OF_CENTER,
+          GST_AUDIO_CHANNEL_POSITION_FRONT_RIGHT_OF_CENTER},
+      {GST_AUDIO_CHANNEL_POSITION_SIDE_LEFT,
+          GST_AUDIO_CHANNEL_POSITION_SIDE_RIGHT},
+    };
+    size_t pair;
+
     GST_DEBUG_OBJECT (enc,
-        "In range for the Vorbis mapping, checking channel positions");
-    for (n = 0; n < enc->n_channels; ++n) {
-      GstAudioChannelPosition pos = GST_AUDIO_INFO_POSITION (info, n);
-      int c;
-
-      GST_DEBUG_OBJECT (enc, "Channel %d has position %d (%s)", n, pos,
-          gst_opus_channel_names[pos]);
-      for (c = 0; c < enc->n_channels; ++c) {
-        if (gst_opus_channel_positions[enc->n_channels - 1][c] == pos) {
-          GST_DEBUG_OBJECT (enc, "Found in Vorbis mapping as channel %d", c);
-          break;
+        "In range for the Vorbis mapping, building channel mapping tables");
+
+    enc->n_stereo_streams = 0;
+    mapped = 0;
+    for (n = 0; n < 256; ++n)
+      positions_done[n] = FALSE;
+
+    /* First, find any natural pairs, and move them to the front */
+    for (pair = 0; pair < G_N_ELEMENTS (pairs); ++pair) {
+      GstAudioChannelPosition p0 = pairs[pair][0];
+      GstAudioChannelPosition p1 = pairs[pair][1];
+      c0 = gst_opus_enc_find_channel_position (enc, info, p0);
+      c1 = gst_opus_enc_find_channel_position (enc, info, p1);
+      if (c0 >= 0 && c1 >= 0) {
+        /* We found a natural pair */
+        GST_DEBUG_OBJECT (enc, "Natural pair '%s/%s' found at %d %d",
+            gst_opus_channel_names[p0], gst_opus_channel_names[p1], c0, c1);
+        /* Find where they map in Vorbis order */
+        c0v = gst_opus_enc_find_channel_position_in_vorbis_order (enc, p0);
+        c1v = gst_opus_enc_find_channel_position_in_vorbis_order (enc, p1);
+        if (c0v < 0 || c1v < 0) {
+          GST_WARNING_OBJECT (enc,
+              "Cannot map channel positions to Vorbis order, using unknown mapping");
+          enc->channel_mapping_family = 255;
+          enc->n_stereo_streams = 0;
+          return;
         }
+
+        enc->encoding_channel_mapping[mapped] = c0;
+        enc->encoding_channel_mapping[mapped + 1] = c1;
+        enc->decoding_channel_mapping[c0v] = mapped;
+        enc->decoding_channel_mapping[c1v] = mapped + 1;
+        enc->n_stereo_streams++;
+        mapped += 2;
+        positions_done[p0] = positions_done[p1] = TRUE;
       }
-      if (c == enc->n_channels) {
-        /* We did not find that position, so use undefined */
-        GST_WARNING_OBJECT (enc,
-            "Position %d (%s) not found in Vorbis mapping, using unknown mapping",
-            pos, gst_opus_channel_positions[pos]);
-        enc->channel_mapping_family = 255;
-        return;
+    }
+
+    /* Now add all other input channels as mono streams */
+    for (n = 0; n < enc->n_channels; ++n) {
+      GstAudioChannelPosition position = GST_AUDIO_INFO_POSITION (info, n);
+
+      /* if we already mapped it while searching for pairs, nothing else
+         needs to be done */
+      if (!positions_done[position]) {
+        int cv;
+        GST_DEBUG_OBJECT (enc, "Channel position %s is not mapped yet, adding",
+            gst_opus_channel_names[position]);
+        cv = gst_opus_enc_find_channel_position_in_vorbis_order (enc, position);
+        if (cv < 0) {
+          GST_WARNING_OBJECT (enc,
+              "Cannot map channel positions to Vorbis order, using unknown mapping");
+          enc->channel_mapping_family = 255;
+          enc->n_stereo_streams = 0;
+          return;
+        }
+        enc->encoding_channel_mapping[mapped] = n;
+        enc->decoding_channel_mapping[cv] = mapped;
+        mapped++;
       }
-      GST_DEBUG_OBJECT (enc, "Mapping output channel %d to %d (%s)", c, n,
-          gst_opus_channel_names[pos]);
-      enc->channel_mapping[c] = n;
     }
-    GST_INFO_OBJECT (enc, "Permutation found, using Vorbis mapping");
+
+#ifndef GST_DISABLE_DEBUG
+    GST_INFO_OBJECT (enc,
+        "Mapping tables built: %d channels, %d stereo streams", enc->n_channels,
+        enc->n_stereo_streams);
+    gst_opus_common_log_channel_mapping_table (GST_ELEMENT (enc), opusenc_debug,
+        "Encoding mapping table", enc->n_channels,
+        enc->encoding_channel_mapping);
+    gst_opus_common_log_channel_mapping_table (GST_ELEMENT (enc), opusenc_debug,
+        "Decoding mapping table", enc->n_channels,
+        enc->decoding_channel_mapping);
+#endif
+
     enc->channel_mapping_family = 1;
     return;
   }
 
-  /* For other cases, we use undefined, with the default trivial mapping */
+  /* More than 8 channels, if future mappings are added for those */
+
+  /* For other cases, we use undefined, with the default trivial mapping
+     and all mono streams */
   GST_WARNING_OBJECT (enc, "Unknown mapping");
   enc->channel_mapping_family = 255;
+  enc->n_stereo_streams = 0;
 
 #undef MAPS
 }
@@ -505,7 +624,7 @@ gst_opus_enc_set_format (GstAudioEncoder * benc, GstAudioInfo * info)
 
   enc->n_channels = GST_AUDIO_INFO_CHANNELS (info);
   enc->sample_rate = GST_AUDIO_INFO_RATE (info);
-  gst_opus_enc_setup_channel_mapping (enc, info);
+  gst_opus_enc_setup_channel_mappings (enc, info);
   GST_DEBUG_OBJECT (benc, "Setup with %d channels, %d Hz", enc->n_channels,
       enc->sample_rate);
 
@@ -530,17 +649,24 @@ gst_opus_enc_set_format (GstAudioEncoder * benc, GstAudioInfo * info)
 static gboolean
 gst_opus_enc_setup (GstOpusEnc * enc)
 {
-  int error = OPUS_OK, n;
-  guint8 trivial_mapping[256];
-
-  GST_DEBUG_OBJECT (enc, "setup");
-
-  for (n = 0; n < 256; ++n)
-    trivial_mapping[n] = n;
+  int error = OPUS_OK;
+
+#ifndef GST_DISABLE_DEBUG
+  GST_DEBUG_OBJECT (enc,
+      "setup: %d Hz, %d channels, %d stereo streams, family %d",
+      enc->sample_rate, enc->n_channels, enc->n_stereo_streams,
+      enc->channel_mapping_family);
+  GST_INFO_OBJECT (enc, "Mapping tables built: %d channels, %d stereo streams",
+      enc->n_channels, enc->n_stereo_streams);
+  gst_opus_common_log_channel_mapping_table (GST_ELEMENT (enc), opusenc_debug,
+      "Encoding mapping table", enc->n_channels, enc->encoding_channel_mapping);
+  gst_opus_common_log_channel_mapping_table (GST_ELEMENT (enc), opusenc_debug,
+      "Decoding mapping table", enc->n_channels, enc->decoding_channel_mapping);
+#endif
 
-  enc->state =
-      opus_multistream_encoder_create (enc->sample_rate, enc->n_channels,
-      enc->n_channels, 0, trivial_mapping,
+  enc->state = opus_multistream_encoder_create (enc->sample_rate,
+      enc->n_channels, enc->n_channels - enc->n_stereo_streams,
+      enc->n_stereo_streams, enc->encoding_channel_mapping,
       enc->audio_or_voip ? OPUS_APPLICATION_AUDIO : OPUS_APPLICATION_VOIP,
       &error);
   if (!enc->state || error != OPUS_OK)
@@ -698,7 +824,8 @@ gst_opus_enc_handle_frame (GstAudioEncoder * benc, GstBuffer * buf)
     enc->headers = NULL;
 
     gst_opus_header_create_caps (&caps, &enc->headers, enc->n_channels,
-        enc->sample_rate, enc->channel_mapping_family, enc->channel_mapping,
+        enc->n_stereo_streams, enc->sample_rate, enc->channel_mapping_family,
+        enc->decoding_channel_mapping,
         gst_tag_setter_get_tag_list (GST_TAG_SETTER (enc)));
 
 
diff --git a/ext/opus/gstopusenc.h b/ext/opus/gstopusenc.h
index 8c2c3c6e8..1e39ad03d 100644
--- a/ext/opus/gstopusenc.h
+++ b/ext/opus/gstopusenc.h
@@ -79,7 +79,9 @@ struct _GstOpusEnc {
   GstTagList            *tags;
 
   guint8                channel_mapping_family;
-  guint8                channel_mapping[256];
+  guint8                encoding_channel_mapping[256];
+  guint8                decoding_channel_mapping[256];
+  guint8                n_stereo_streams;
 };
 
 struct _GstOpusEncClass {
diff --git a/ext/opus/gstopusheader.c b/ext/opus/gstopusheader.c
index 42df7b35f..7f49e4711 100644
--- a/ext/opus/gstopusheader.c
+++ b/ext/opus/gstopusheader.c
@@ -27,12 +27,17 @@
 #include "gstopusheader.h"
 
 static GstBuffer *
-gst_opus_enc_create_id_buffer (gint nchannels, gint sample_rate,
-    guint8 channel_mapping_family, const guint8 * channel_mapping)
+gst_opus_enc_create_id_buffer (gint nchannels, gint n_stereo_streams,
+    gint sample_rate, guint8 channel_mapping_family,
+    const guint8 * channel_mapping)
 {
   GstBuffer *buffer;
   GstByteWriter bw;
 
+  g_return_val_if_fail (nchannels > 0 && nchannels < 256, NULL);
+  g_return_val_if_fail (n_stereo_streams >= 0, NULL);
+  g_return_val_if_fail (n_stereo_streams <= nchannels - n_stereo_streams, NULL);
+
   gst_byte_writer_init (&bw);
 
   /* See http://wiki.xiph.org/OggOpus */
@@ -44,8 +49,8 @@ gst_opus_enc_create_id_buffer (gint nchannels, gint sample_rate,
   gst_byte_writer_put_uint16_le (&bw, 0);       /* output gain */
   gst_byte_writer_put_uint8 (&bw, channel_mapping_family);
   if (channel_mapping_family > 0) {
-    gst_byte_writer_put_uint8 (&bw, nchannels);
-    gst_byte_writer_put_uint8 (&bw, 0);
+    gst_byte_writer_put_uint8 (&bw, nchannels - n_stereo_streams);
+    gst_byte_writer_put_uint8 (&bw, n_stereo_streams);
     gst_byte_writer_put_data (&bw, channel_mapping, nchannels);
   }
 
@@ -158,7 +163,7 @@ gst_opus_header_create_caps_from_headers (GstCaps ** caps, GSList ** headers,
 
 void
 gst_opus_header_create_caps (GstCaps ** caps, GSList ** headers, gint nchannels,
-    gint sample_rate, guint8 channel_mapping_family,
+    gint n_stereo_streams, gint sample_rate, guint8 channel_mapping_family,
     const guint8 * channel_mapping, const GstTagList * tags)
 {
   GstBuffer *buf1, *buf2;
@@ -175,7 +180,7 @@ gst_opus_header_create_caps (GstCaps ** caps, GSList ** headers, gint nchannels,
 
   /* create header buffers */
   buf1 =
-      gst_opus_enc_create_id_buffer (nchannels, sample_rate,
+      gst_opus_enc_create_id_buffer (nchannels, n_stereo_streams, sample_rate,
       channel_mapping_family, channel_mapping);
   buf2 = gst_opus_enc_create_metadata_buffer (tags);
 
diff --git a/ext/opus/gstopusheader.h b/ext/opus/gstopusheader.h
index 3b2cfc265..c6278eff3 100644
--- a/ext/opus/gstopusheader.h
+++ b/ext/opus/gstopusheader.h
@@ -28,7 +28,7 @@ G_BEGIN_DECLS
 extern void gst_opus_header_create_caps_from_headers (GstCaps **caps, GSList **headers,
     GstBuffer *id_header, GstBuffer *comment_header);
 extern void gst_opus_header_create_caps (GstCaps **caps, GSList **headers,
-    gint nchannels, gint sample_rate,
+    gint nchannels, gint n_stereo_streams, gint sample_rate,
     guint8 channel_mapping_family, const guint8 *channel_mapping,
     const GstTagList *tags);
 extern gboolean gst_opus_header_is_header (GstBuffer * buf,
diff --git a/ext/opus/gstopusparse.c b/ext/opus/gstopusparse.c
index fae62007e..f3706cb72 100644
--- a/ext/opus/gstopusparse.c
+++ b/ext/opus/gstopusparse.c
@@ -307,7 +307,7 @@ gst_opus_parse_parse_frame (GstBaseParse * base, GstBaseParseFrame * frame)
       channel_mapping_family = 0;
       channel_mapping[0] = 0;
       channel_mapping[1] = 1;
-      gst_opus_header_create_caps (&caps, &parse->headers, channels, 0,
+      gst_opus_header_create_caps (&caps, &parse->headers, channels, 1, 0,
           channel_mapping_family, channel_mapping, NULL);
     }