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

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

::: gst/audiomixmatrix/gstaudiomixmatrix.c
@@ +239,3 @@
+
+  if (self->matrix) {
+    g_free (self->matrix);

g_free() is NULL-safe (many other occurences)

@@ +293,3 @@
+          for (i = 0; i < self->in_channels * self->out_channels; i++) {
+            self->s32_conv_matrix[i] =
+                (gint64) ((self->matrix[i]) * (1 << self->shift_bytes));

shift_bytes depends on the channels (in channels), and is only set in
set_caps(). So usually not set here

summary: move all this code into a function, calculate shift_bytes at the top,
call this new function here, in set_caps(), and when in_channels changes

@@ +395,3 @@
+  }
+  if (!gst_buffer_map (outbuf, &outmap, GST_MAP_WRITE)) {
+    gst_buffer_unmap (outbuf, &outmap);

This should be inbuf/inmap

@@ +605,3 @@
+  gint channels;
+
+  s2 = gst_caps_get_structure (caps, 0);

If not FIRST_CHANNELS mode, all the code below is useless and not doing
anything (but don't forget the call to the base class)

@@ +611,3 @@
+    gint mindiff = channels;
+    for (i = 0; i < capssize; i++) {
+      othercaps = gst_caps_make_writable (othercaps);

This should just be once outside the loop

@@ +649,3 @@
+  if (!gst_structure_has_field (s, "channel-mask")) {
+    if (self->mode == GST_AUDIO_MIX_MATRIX_MODE_FIRST_CHANNELS ||
+        self->channel_mask == -1) {

These two ifs can be merged into one "if (x && (y || z))"

@@ +680,3 @@
+      }
+      if (gst_structure_has_field (s, "channel-mask")) {
+        gst_structure_remove_field (s, "channel-mask");

This removes channel-mask, but the fixate-caps code does not set any again
unless self->channel_mask==-1. It should set 0 if there is none

::: tests/examples/audiomixmatrix/test-audiomixmatrix.c
@@ +89,3 @@
+  g_object_set (audiomixmatrix, "in-channels", 4, NULL);
+  g_object_set (audiomixmatrix, "out-channels", 2, NULL);
+  g_value_init (&v, GST_TYPE_ARRAY);

Add a comment how the actual matrix looks later, that's not obvious from the
code

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