[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 #343744|none                        |needs-work
             status|                            |

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

Just some minor things left

::: gst/audiomixmatrix/gstaudiomixmatrix.c
@@ +22,3 @@
+
+/**
+ * SECTION:element-audiomixmatrix

If you add docs, also add them to docs/plugins/*-sections.txt

@@ +29,3 @@
+ * matrix coefficients must be between -1 and 1. In the first-channels mode,
+ * input/output channels are automatically negotiated and the transformation
+ * matrix is a truncated identity matrix.

Maybe make it more explicit that the matrix is number of output channels rows
and number of input channels columns

@@ +218,3 @@
+  audiomixmatrix->channel_mask = 0;
+  audiomixmatrix->s16_array = NULL;
+  audiomixmatrix->s32_array = NULL;

These are the converted matrizes, maybe name them like that :)

@@ +239,3 @@
+  if (audiomixmatrix->s32_array) {
+    g_free (audiomixmatrix->s32_array);
+    audiomixmatrix->s32_array = NULL;

As the converted matrizes are caps dependent (in/out channels define the
shift), these should be freed in PAUSED->READY

@@ +271,3 @@
+      for (out = 0; out < audiomixmatrix->out_channels; out++) {
+        const GValue *row = gst_value_array_get_value (value, out);
+        g_return_if_fail (gst_value_array_get_size (row) >=

These two assertions should be a == imho

@@ +282,3 @@
+          audiomixmatrix->matrix[out * audiomixmatrix->in_channels + in] =
+              coefficient;
+        }

Need to update the converted s16/s32 matrizes here

@@ +353,3 @@
+    return GST_FLOW_ERROR;
+  if (!gst_buffer_map (outbuf, &outmap, GST_MAP_WRITE))
+    return GST_FLOW_ERROR;

Forgetting to unmap the inbuf here on error

@@ +371,3 @@
+            outval +=
+                inarray[sample * inchannels +
+                in] * self->matrix[out * inchannels + in];

Move self->matrix out of the loop too

@@ +415,3 @@
+          for (in = 0; in < inchannels; in++) {
+            outval += (gint32) (inarray[sample * inchannels + in] *
+                self->s16_array[out * inchannels + in]);

... and self->s16/32_array

@@ +500,3 @@
+    }
+
+  }

else if (!self->matrix || dimensions_of_self_matrix_are_wrong) { error() }

@@ +526,3 @@
+      /* converted bits - input bits - sign - bits needed for channel */
+      self->shift_bytes =
+          64 - 32 - 1 - (gint) (log (self->in_channels) / log (2));

I think the log2() here (and above) has to be rounded upwards instead of
downwards as the (gint) cast does

::: tests/examples/audiomixmatrix/test-audiomixmatrix.c
@@ +1,1 @@
+#include <gst/gst.h>

(c) and license boilerplate missing

@@ +34,3 @@
+
+  g_print ("Caps received on %s: %s\n",
+      GST_PAD_IS_SRC (pad) ? "source" : "sink", caps_str);

You can use the fancy new gst_print*() functions here with GST_PTR_FORMAT now,
but it's fine as is of course

@@ +67,3 @@
+  g_object_set (audiomixmatrix, "in-channels", 4, NULL);
+  g_object_set (audiomixmatrix, "out-channels", 2, NULL);
+  g_value_init (&v, GST_TYPE_ARRAY);

Maybe add a comment how the matrix will look after this, and what that matrix
means.

Might also want to make this a varargs utility function inside this file here,
like gst_value_array_set_float_matrix(&v, 4, 2, 1.0, 0.0, 0.0, 0.0, 0.0, 1.0,
0.0, 0.0)

@@ +106,3 @@
+  gst_value_array_append_value (&v, &v2);
+  g_value_unset (&v2);
+  g_object_set_property (G_OBJECT (audiomixmatrix), "matrix", &v);

Btw, instead of this it seems also possible to use g_object_set() with a
GArray<GValue> (if I understand the value collect function correctly), but that
doesn't make anything more beautiful. It might make sense to add helper
functions for creation of GST_TYPE_ARRAY and _LIST to gstutils.c at some point,
I'm aware of many such helpers in various element code.

@@ +125,3 @@
+  gst_object_unref (sinkpad);
+
+  gst_element_set_state (pipeline, GST_STATE_PLAYING);

Might want to check for errors :)

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