[Bug 777376] matrixmix: New element that mixes audio channels

[GStreamer (GNOME Bugzilla)]

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

Sebastian Dröge (slomo) <slomo at coaxion.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #343630|none                        |needs-work
             status|                            |

--- Comment #4 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Review of attachment 343630:
 --> (https://bugzilla.gnome.org/review?bug=777376&attachment=343630)

Looks generally good (maybe a test missing?)

::: gst/matrixmix/gstmatrixmix.c
@@ +29,3 @@
+ * matrix coefficients must be between -1 and 1. In the auto mode,
+ * input/output channels are automatically negotiated and the transformation
+ * matrix is a truncated or zero-padded identity matrix.

Isn't it always a truncated identity matrix (truncated at the bottom or on the
right)?

@@ +90,3 @@
+#define DEFAULT_SOURCE_CLOCK NULL
+#define DEFAULT_DAILY_JAM NULL
+#define DEFAULT_POST_MESSAGES FALSE

Unused defines

@@ +96,3 @@
+    GST_PAD_SRC,
+    GST_PAD_ALWAYS,
+    GST_STATIC_CAPS ("audio/x-raw, channels = [1, max], format = (string) {"

layout = (string) interleaved. You don't handle non-interleaved channels
(nothing does yet). Also on the other template

@@ +150,3 @@
+      g_param_spec_uint ("out-channels", "Output audio channels",
+          "How many audio channels we have on the output side",
+          0, 64, 2, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

Should we remove these properties and infer it from the matrix dimensions?
Right now it's only there as a sanity check.

Also these have no effect in auto-mode, right? What does 0 mean for these?

@@ +169,3 @@
+      g_param_spec_enum ("mode",
+          "Channel/matrix mode",
+          "Whether to auto-negotiate input/output channels and matrix",

Or maybe have matrix==NULL be the auto mode and not this property? On the other
hand that could cause unintentional usage of it when failing to set the matrix


To summarize: the in/out-channels and mode properties could go away, but
they're currently there as sanity checks. Opinions?

@@ +199,3 @@
+  matrixmix->channel_mask = 0;
+  matrixmix->s16_array = NULL;
+  matrixmix->s32_array = NULL;

These are not really "arrays", but the matrix in different formats, right?

@@ +263,3 @@
+        }
+      }
+      break;

Have to update the s16/s32 matrizes too here possibly, also maybe sanity check
if the values are in [-1,1]

@@ +336,3 @@
+    case GST_AUDIO_FORMAT_F32BE:{
+      gfloat *inarray, *outarray;
+      inarray = (gfloat *) inmap.data;

inarray(s) can be const

@@ +341,3 @@
+      for (sample = 0;
+          sample < outmap.size / (sizeof (gfloat) * self->out_channels);
+          sample++) {

Move sizeof(gfloat)*self->out_channels, self->out_channels and
self->in_channels out of the loop into local variables (speed reasons)

@@ +392,3 @@
+          }
+          outarray[sample * self->out_channels + out] =
+              (gint16) (outval >> self->n);

And here also move self->n out of the loop

@@ +395,3 @@
+          if (out == 0 && sample == 0)
+            GST_DEBUG_OBJECT (self, "%i -> %i %i -> %i", inarray[0],
+                self->s16_array[0], outval, outarray[0]);

Leftover debug output

@@ +420,3 @@
+          if (out == 0 && sample == 0)
+            GST_DEBUG_OBJECT (self, "%i -> %li %li -> %i", inarray[0],
+                self->s32_array[0], outval, outarray[0]);

Leftover debug output

@@ +548,3 @@
+  }
+
+  for (i = 0; i < gst_caps_get_size (outcaps); i++) {

gst_caps_get_size() should also be moved here (and above) out of the loop

@@ +555,3 @@
+        gst_structure_set (s, "channels", G_TYPE_INT, self->in_channels,
NULL);
+      } else if (direction == GST_PAD_SINK) {
+        /* g_assert (gst_structure_get_int (s, "channels") ==
self->in_channels); */

The assertions can go away

::: gst/matrixmix/gstmatrixmix.h
@@ +51,3 @@
+struct _GstMatrixMix
+{
+  GstBaseTransform videofilter;

video

@@ +60,3 @@
+  GstMatrixMixMode mode;
+  gint32 *s16_array;
+  gint64 *s32_array;

You could in theory make these two into a union... but that seems not worth the
effort

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