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 785471 - [API]: gst_audio_channel_mixer_new_with_matrix
[API]: gst_audio_channel_mixer_new_with_matrix
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
unspecified
Other All
: Normal enhancement
: 1.13.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-07-27 01:49 UTC by Mathieu Duponchelle
Modified: 2017-09-22 18:20 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[API]: gst_audio_channel_mixer_new_with_matrix (30.30 KB, patch)
2017-07-27 01:50 UTC, Mathieu Duponchelle
none Details | Review
[API]: gst_audio_channel_mixer_new_with_matrix (31.25 KB, patch)
2017-07-27 02:36 UTC, Mathieu Duponchelle
none Details | Review
[API]: gst_audio_channel_mixer_new_with_matrix (30.55 KB, patch)
2017-08-15 00:51 UTC, Mathieu Duponchelle
none Details | Review
[API]: GST_AUDIO_CONVERTER_OPT_MIX_MATRIX (7.59 KB, patch)
2017-08-15 00:51 UTC, Mathieu Duponchelle
none Details | Review
audioconvert: refactor format removal. (3.95 KB, patch)
2017-08-15 00:51 UTC, Mathieu Duponchelle
none Details | Review
audioconvert: [API]: expose mix-matrix property. (7.10 KB, patch)
2017-08-15 00:51 UTC, Mathieu Duponchelle
none Details | Review
[API]: gst_audio_channel_mixer_new_with_matrix (31.88 KB, patch)
2017-09-21 12:04 UTC, Mathieu Duponchelle
none Details | Review
[API]: GST_AUDIO_CONVERTER_OPT_MIX_MATRIX (7.59 KB, patch)
2017-09-21 12:04 UTC, Mathieu Duponchelle
committed Details | Review
audioconvert: refactor format removal. (3.95 KB, patch)
2017-09-21 12:04 UTC, Mathieu Duponchelle
committed Details | Review
audioconvert: [API]: expose mix-matrix property. (8.98 KB, patch)
2017-09-21 12:05 UTC, Mathieu Duponchelle
none Details | Review
audioconvert: [API]: expose mix-matrix property. (9.00 KB, patch)
2017-09-21 17:29 UTC, Mathieu Duponchelle
committed Details | Review
[API]: gst_audio_channel_mixer_new_with_matrix (32.32 KB, patch)
2017-09-22 14:23 UTC, Mathieu Duponchelle
committed Details | Review

Description Mathieu Duponchelle 2017-07-27 01:49:56 UTC
+ Refactor previous constructor to call on that new constructor

+ Reimplement is_passthrough to strictly check whether the matrix
  is an identity matrix, comparing channel-masks was incorrect:
  the mixer may be remixing from a list of positions to the same
  list of positions, but ordered differently, and reciprocally,
  the mixer may be remixing from a list of positions to another
  list of positions identically ordered

+ Remove unused tmp field, must have been a refactoring leftover
Comment 1 Mathieu Duponchelle 2017-07-27 01:50:02 UTC
Created attachment 356449 [details] [review]
[API]: gst_audio_channel_mixer_new_with_matrix
Comment 2 Mathieu Duponchelle 2017-07-27 02:36:53 UTC
Created attachment 356450 [details] [review]
[API]: gst_audio_channel_mixer_new_with_matrix

+ Refactor previous constructor to call on that new constructor

+ Reimplement is_passthrough to strictly check whether the matrix
  is an identity matrix, comparing channel-masks was incorrect:
  the mixer may be remixing from a list of positions to the same
  list of positions, but ordered differently, and reciprocally,
  the mixer may be remixing from a list of positions to another
  list of positions identically ordered

+ Remove unused tmp field, must have been a refactoring leftover
Comment 3 Sebastian Dröge (slomo) 2017-07-27 11:11:17 UTC
As a proof-of-concept, can you move audiomixmatrix to this and remove its custom mixing code? :)
Comment 4 Mathieu Duponchelle 2017-07-27 15:37:16 UTC
(In reply to Sebastian Dröge (slomo) from comment #3)
> As a proof-of-concept, can you move audiomixmatrix to this and remove its
> custom mixing code? :)

My end goal here is to be able to use this in audiomixer, through the audio_converter API, I'm currently exposing a mix-matrix configuration key for AudioConverter, which IMO should then be directly exposed in audioconvert, rendering audiomixmatrix obsolete (I'm lifting the GstValueList -> gfloat ** matrix from there).
Comment 5 Mathieu Duponchelle 2017-08-15 00:51:09 UTC
Created attachment 357578 [details] [review]
[API]: gst_audio_channel_mixer_new_with_matrix

+ Refactor previous constructor to call on that new constructor

+ Reimplement is_passthrough to strictly check whether the matrix
  is an identity matrix, comparing channel-masks was incorrect:
  the mixer may be remixing from a list of positions to the same
  list of positions, but ordered differently, and reciprocally,
  the mixer may be remixing from a list of positions to another
  list of positions identically ordered

+ Remove unused tmp field, must have been a refactoring leftover
Comment 6 Mathieu Duponchelle 2017-08-15 00:51:14 UTC
Created attachment 357579 [details] [review]
[API]: GST_AUDIO_CONVERTER_OPT_MIX_MATRIX

Taken from audiomixmatrix, credits to Vivia Nikolaidou
Comment 7 Mathieu Duponchelle 2017-08-15 00:51:20 UTC
Created attachment 357580 [details] [review]
audioconvert: refactor format removal.

remove_format_info was a bit confusing to read, this removes
it in favor of standard gst_caps_map_in_place calls.

This no longer simplifies the resulting caps, but I
consider this should be the job of basetransform.
Comment 8 Mathieu Duponchelle 2017-08-15 00:51:25 UTC
Created attachment 357581 [details] [review]
audioconvert: [API]: expose mix-matrix property.

This obsoletes audiomixmatrix
Comment 9 Mathieu Duponchelle 2017-08-15 00:53:36 UTC
(In reply to Sebastian Dröge (slomo) from comment #3)
> As a proof-of-concept, can you move audiomixmatrix to this and remove its
> custom mixing code? :)

Sort of done, with these patches, a mix-matrix property can be set on audioconvert :)

I'll open a separate issue for exposing this on audiomixer's pads.
Comment 10 Sebastian Dröge (slomo) 2017-09-18 16:30:50 UTC
Review of attachment 357578 [details] [review]:

Looks good in general

::: gst-libs/gst/audio/audio-channel-mixer.c
@@ +800,3 @@
  * @out_channels: number of output channels
+ * @matrix: (transfer full): channel conversion matrix, m[@in_channels][@out_channels].
+ *   If identity matrix, passthrough applies.

Maybe allow NULL for identity?

@@ +942,3 @@
+    for (j = 0; j < mix->out_channels; j++) {
+      if ((i == j && mix->matrix[i][j] != 1.0) ||
+          (i != j && mix->matrix[i][j] != 0.0)) {

Comparing floats is not a good idea. Best if you remember from the constructor with a flag if you're passthrough or not, based on the positions... or if we only get a matrix, assume passthrough only if matrix == NULL and as fallback only compare the matrix.

::: gst-libs/gst/audio/audio-channel-mixer.h
@@ +60,3 @@
+                                                                gint in_channels,
+                                                                gint out_channels,
+                                                                gfloat **matrix);

const?
Comment 11 Sebastian Dröge (slomo) 2017-09-18 16:32:55 UTC
Comment on attachment 357580 [details] [review]
audioconvert: refactor format removal.

That's exactly why I wanted map_in_place() :)
Comment 12 Sebastian Dröge (slomo) 2017-09-18 16:36:39 UTC
Review of attachment 357581 [details] [review]:

::: gst/audioconvert/gstaudioconvert.c
@@ +198,3 @@
+          "Transformation matrix for input/output channels",
+          gst_param_spec_array ("matrix-in1", "rows", "rows",
+              g_param_spec_float ("matrix-in2", "cols", "cols",

matrix-rows and matrix-cols instead of matrix-in* maybe

@@ +303,3 @@
+   * or if a mix matrix was manually specified */
+  if (G_IS_VALUE (&this->mix_matrix) ||
+      !gst_structure_get (s, "channel-mask", GST_TYPE_BITMASK, &mask, NULL) ||

If a matrix was manually specified, we can only support N input and M output channels, or not? By removing the channels field, you allow any number of channels.
But the channel-mask should disappear
Comment 13 Mathieu Duponchelle 2017-09-19 22:12:59 UTC
(In reply to Sebastian Dröge (slomo) from comment #12)
> Review of attachment 357581 [details] [review] [review]:
> 
> ::: gst/audioconvert/gstaudioconvert.c
> @@ +198,3 @@
> +          "Transformation matrix for input/output channels",
> +          gst_param_spec_array ("matrix-in1", "rows", "rows",
> +              g_param_spec_float ("matrix-in2", "cols", "cols",
> 
> matrix-rows and matrix-cols instead of matrix-in* maybe

Sure, done

> 
> @@ +303,3 @@
> +   * or if a mix matrix was manually specified */
> +  if (G_IS_VALUE (&this->mix_matrix) ||
> +      !gst_structure_get (s, "channel-mask", GST_TYPE_BITMASK, &mask, NULL)
> ||
> 
> If a matrix was manually specified, we can only support N input and M output
> channels, or not? By removing the channels field, you allow any number of
> channels.
> But the channel-mask should disappear

In case a matrix is specified and its dimensions do not match the input / output channels, we fail negotiation:

gst-launch-1.0 audiotestsrc ! audio/x-raw ! audioconvert mix-matrix="<<(float)1.0, (float)0.0, (float)0.0, (float)0.0>, <(float)0.0, (float)0.0, (float)0.0, (float)0.0>>" ! audio/x-raw,channels=2 ! autoaudiosink

0:00:00.021796138 23185      0x24f00f0 ERROR        audio-converter audio-converter.c:650:check_mix_matrix: Invalid mix matrix row size, should be 2
0:00:00.021823146 23185      0x24f00f0 ERROR           audioconvert gstaudioconvert.c:738:gst_audio_convert_set_caps:<audioconvert0> could not make converter
[..]
ERROR: from element /GstPipeline:pipeline0/GstAudioTestSrc:audiotestsrc0: Internal data stream error.
[..]
streaming stopped, reason not-negotiated (-4)

Is it not good enough?
Comment 14 Mathieu Duponchelle 2017-09-19 22:56:19 UTC
> @@ +942,3 @@
> +    for (j = 0; j < mix->out_channels; j++) {
> +      if ((i == j && mix->matrix[i][j] != 1.0) ||
> +          (i != j && mix->matrix[i][j] != 0.0)) {
> 
> Comparing floats is not a good idea.

Yep very true, the issue here is even more pronounced as I'm not even comparing to 1.0f or 0.0f, must have worked by chance when I tested. I'd still like to determine whether a given user-defined matrix is passthrough or not accurately.

I gave a look at https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/, but while it seems comparing floats to known numbers is possible it's a lot of complication for not much benefit, an alternative would be to expect an integer matrix, but that's maybe not a very nice API.

I'll think more about it tomorrow :)
Comment 15 Mathieu Duponchelle 2017-09-20 14:54:14 UTC
(In reply to Sebastian Dröge (slomo) from comment #10)
> Review of attachment 357578 [details] [review] [review]:
> 
> Looks good in general
> 
> ::: gst-libs/gst/audio/audio-channel-mixer.c
> @@ +800,3 @@
>   * @out_channels: number of output channels
> + * @matrix: (transfer full): channel conversion matrix,
> m[@in_channels][@out_channels].
> + *   If identity matrix, passthrough applies.
> 
> Maybe allow NULL for identity?

Done

> 
> @@ +942,3 @@
> +    for (j = 0; j < mix->out_channels; j++) {
> +      if ((i == j && mix->matrix[i][j] != 1.0) ||
> +          (i != j && mix->matrix[i][j] != 0.0)) {
> 
> Comparing floats is not a good idea. Best if you remember from the
> constructor with a flag if you're passthrough or not, based on the
> positions... or if we only get a matrix, assume passthrough only if matrix
> == NULL and as fallback only compare the matrix.

I have updated the patch to compare values to 0.0f and 1.0f explicitly, this will work for:

* The matrices we generate in audio_channel_mixer
* The matrices that may be passed through the command line

This may not work for matrices generated through calculations, it is the responsibility of the user to explicitly assign 0.0 or 1.0 to values within an epsilon distance of their choosing if they want passthrough to potentially happen.


This is documented in the parameter comment.

> ::: gst-libs/gst/audio/audio-channel-mixer.h
> @@ +60,3 @@
> +                                                                gint
> in_channels,
> +                                                                gint
> out_channels,
> +                                                                gfloat
> **matrix);
> 
> const?

Well that's transfer full, so I don't think we want that ?
Comment 16 Sebastian Dröge (slomo) 2017-09-20 15:04:25 UTC
(In reply to Mathieu Duponchelle from comment #13)

> In case a matrix is specified and its dimensions do not match the input /
> output channels, we fail negotiation:
> 
> gst-launch-1.0 audiotestsrc ! audio/x-raw ! audioconvert
> mix-matrix="<<(float)1.0, (float)0.0, (float)0.0, (float)0.0>, <(float)0.0,
> (float)0.0, (float)0.0, (float)0.0>>" ! audio/x-raw,channels=2 !
> autoaudiosink
> 
> 0:00:00.021796138 23185      0x24f00f0 ERROR        audio-converter
> audio-converter.c:650:check_mix_matrix: Invalid mix matrix row size, should
> be 2
> 0:00:00.021823146 23185      0x24f00f0 ERROR           audioconvert
> gstaudioconvert.c:738:gst_audio_convert_set_caps:<audioconvert0> could not
> make converter
> [..]
> ERROR: from element /GstPipeline:pipeline0/GstAudioTestSrc:audiotestsrc0:
> Internal data stream error.
> [..]
> streaming stopped, reason not-negotiated (-4)
> 
> Is it not good enough?

That's what should happen, yes, but I would've liked that to happen one step before that. You should be able to let the caps negotiation fail already instead of just failing to create the converter later. By correctly giving the caps from transform_caps.


For the matrix comparison, I guess comparing == 1.0 and == 0.0 is fine as we only care about the cases where it is explicitly those values. And then the comparison will be successful. Good enough :)
Comment 17 Sebastian Dröge (slomo) 2017-09-20 15:04:52 UTC
(In reply to Mathieu Duponchelle from comment #15)

> > ::: gst-libs/gst/audio/audio-channel-mixer.h
> > @@ +60,3 @@
> > +                                                                gint
> > in_channels,
> > +                                                                gint
> > out_channels,
> > +                                                                gfloat
> > **matrix);
> > 
> > const?
> 
> Well that's transfer full, so I don't think we want that ?

Ok
Comment 18 Mathieu Duponchelle 2017-09-20 20:54:34 UTC
> That's what should happen, yes, but I would've liked that to happen one step
> before that. You should be able to let the caps negotiation fail already
> instead of just failing to create the converter later. By correctly giving
> the caps from transform_caps.

Yeah that's what I realized earlier today, started to work on that then went to something else, but indeed we could make things a bit better, will do :)
Comment 19 Mathieu Duponchelle 2017-09-21 12:04:34 UTC
Created attachment 360187 [details] [review]
[API]: gst_audio_channel_mixer_new_with_matrix

+ Refactor previous constructor to call on that new constructor

+ Reimplement is_passthrough to strictly check whether the matrix
  is an identity matrix, comparing channel-masks was incorrect:
  the mixer may be remixing from a list of positions to the same
  list of positions, but ordered differently, and reciprocally,
  the mixer may be remixing from a list of positions to another
  list of positions identically ordered

+ Remove unused tmp field, must have been a refactoring leftover
Comment 20 Mathieu Duponchelle 2017-09-21 12:04:40 UTC
Created attachment 360188 [details] [review]
[API]: GST_AUDIO_CONVERTER_OPT_MIX_MATRIX

Taken from audiomixmatrix, credits to Vivia Nikolaidou
Comment 21 Mathieu Duponchelle 2017-09-21 12:04:55 UTC
Created attachment 360189 [details] [review]
audioconvert: refactor format removal.

remove_format_info was a bit confusing to read, this removes
it in favor of standard gst_caps_map_in_place calls.

This no longer simplifies the resulting caps, but I
consider this should be the job of basetransform.
Comment 22 Mathieu Duponchelle 2017-09-21 12:05:02 UTC
Created attachment 360190 [details] [review]
audioconvert: [API]: expose mix-matrix property.

This obsoletes audiomixmatrix
Comment 23 Mathieu Duponchelle 2017-09-21 12:08:49 UTC
> That's what should happen, yes, but I would've liked that to happen one step
> before that. You should be able to let the caps negotiation fail already
> instead of just failing to create the converter later. By correctly giving
> the caps from transform_caps.

That's done now, it also actually allows the pipeline I had used as an example to work, as the input caps are now correctly negotiated with audiotestsrc :)
Comment 24 Mathieu Duponchelle 2017-09-21 17:29:28 UTC
Created attachment 360206 [details] [review]
audioconvert: [API]: expose mix-matrix property.

This obsoletes audiomixmatrix
Comment 25 Nicolas Dufresne (ndufresne) 2017-09-21 18:34:18 UTC
That looks good to me in general. I was wondering if we could add a small utility in libgstaudio so we could convert back/forth a C style matrix to/from GstValueArray ? One idea would be:

  gfloat mix_matrix[][];
  gst_audio_set_matrix (obj, "matrix", &mix_matrix);
  gsT_audio_get_matrix (obj, "matrix", &mix_matrix);

Or anything else that is helpful for programmer and does not make someone without gvalue understanding cry.
Comment 26 Mathieu Duponchelle 2017-09-21 20:20:45 UTC
(In reply to Nicolas Dufresne (stormer) from comment #25)
> That looks good to me in general. I was wondering if we could add a small
> utility in libgstaudio so we could convert back/forth a C style matrix
> to/from GstValueArray ? One idea would be:
> 
>   gfloat mix_matrix[][];
>   gst_audio_set_matrix (obj, "matrix", &mix_matrix);
>   gsT_audio_get_matrix (obj, "matrix", &mix_matrix);
> 
> Or anything else that is helpful for programmer and does not make someone
> without gvalue understanding cry.

I wondered about that as well, especially as https://bugzilla.gnome.org/show_bug.cgi?id=786344 will expose this as well, so there's two places where the user may need that. That's a separate concern though, I'll look into it tomorrow if I get the time, but can be done in a separate issue :)
Comment 27 Sebastian Dröge (slomo) 2017-09-22 08:20:26 UTC
Such API would indeed be nice for people writing this in C.
Comment 28 Sebastian Dröge (slomo) 2017-09-22 08:26:31 UTC
Review of attachment 360206 [details] [review]:

Looks good generally, just some minor things left

::: gst/audioconvert/gstaudioconvert.c
@@ +350,3 @@
+          other_channels = gst_value_array_get_size (first_row);
+        } else {
+          GST_WARNING_OBJECT (this, "Empty mix matrix's first row");

Maybe we can catch the "invalid matrix" cases in set_property() already?

@@ +952,3 @@
+
+      g_value_init (&this->mix_matrix, GST_TYPE_ARRAY);
+      g_value_copy (value, &this->mix_matrix);

It seems like there is no way to actually unset the matrix after you set it once. You could provide an empty array I guess (i.e. a value just initialized to GST_TYPE_ARRAY), but this would still succeed the G_IS_VALUE() checks. You should probably special-case the empty array as "unset".
Comment 29 Sebastian Dröge (slomo) 2017-09-22 08:27:00 UTC
Review of attachment 360189 [details] [review]:

Get it in, independent of everything else :)
Comment 30 Sebastian Dröge (slomo) 2017-09-22 08:27:58 UTC
Review of attachment 360188 [details] [review]:

::: gst-libs/gst/audio/audio-converter.c
@@ +670,3 @@
+
+gfloat **
+mix_matrix_from_g_value (guint in_channels, guint out_channels,

static
Comment 31 Sebastian Dröge (slomo) 2017-09-22 08:33:27 UTC
Review of attachment 360187 [details] [review]:

::: gst-libs/gst/audio/audio-channel-mixer.c
@@ +124,3 @@
+       * an identity matrix */
+      if (flags & GST_AUDIO_CHANNEL_MIXER_FLAGS_UNPOSITIONED_IN) {
+        matrix[ci][co] = ci == co ? 1.0 : 0.0;

Why is this case needed now? Maybe could be in a separate commit :)

@@ +446,3 @@
         RATIO_FRONT_SIDE);
   } else if (!in_has_front && in_has_center && in_has_side && out_has_front) {
+    gst_audio_channel_mixer_fill_one_other (matrix, in_c, out_f,

This refactoring could also go into a separate commit, it would make it easier to read the actual changes and help "git bisect"

@@ +840,3 @@
+      mix->matrix[i] = g_new (gfloat, out_channels);
+      for (j = 0; j < out_channels; j++) {
+        mix->matrix[i][j] = i == j ? 1.0 : 0.0;

Is this correct? Previously we allocated the matrix with all-zeroes. It's not really identity that is needed here but we need to find out what is needed based on the two channel position arrays

@@ +907,3 @@
+ *
+ * Returns: a new #GstAudioChannelMixer object. Free with gst_audio_channel_mixer_free()
+ * after usage.

Also returns NULL if the matrix is invalid or incompatible with the channel numbers

@@ +965,3 @@
+    for (j = 0; j < mix->out_channels; j++) {
+      if ((i == j && mix->matrix[i][j] != 1.0f) ||
+          (i != j && mix->matrix[i][j] != 0.0f)) {

Needs a comment why comparing floating point numbers here is not a problem
Comment 32 Mathieu Duponchelle 2017-09-22 12:40:36 UTC
(In reply to Sebastian Dröge (slomo) from comment #28)
> Review of attachment 360206 [details] [review] [review]:
> 
> Looks good generally, just some minor things left
> 
> ::: gst/audioconvert/gstaudioconvert.c
> @@ +350,3 @@
> +          other_channels = gst_value_array_get_size (first_row);
> +        } else {
> +          GST_WARNING_OBJECT (this, "Empty mix matrix's first row");
> 
> Maybe we can catch the "invalid matrix" cases in set_property() already?
> 

We can indeed output the warning there, but what should we do then ? Not set the matrix and proceed without one? This may be surprising to the user if he doesn't check his logs.

Otherwise we can output the warning there and error out here, but that doesn't make much of a difference?

> @@ +952,3 @@
> +
> +      g_value_init (&this->mix_matrix, GST_TYPE_ARRAY);
> +      g_value_copy (value, &this->mix_matrix);
> 
> It seems like there is no way to actually unset the matrix after you set it
> once. You could provide an empty array I guess (i.e. a value just
> initialized to GST_TYPE_ARRAY), but this would still succeed the
> G_IS_VALUE() checks. You should probably special-case the empty array as
> "unset".

Thing is I'd ideally set this as CONSTRUCT_ONLY, but then this won't work from the command line, same thing for the dithering and noise shaping properties by the way
Comment 33 Mathieu Duponchelle 2017-09-22 13:04:19 UTC
Comment on attachment 360189 [details] [review]
audioconvert: refactor format removal.

Attachment 360189 [details] pushed as 7a407b6 - audioconvert: refactor format removal.
Comment 34 Mathieu Duponchelle 2017-09-22 14:22:41 UTC
Review of attachment 360187 [details] [review]:

Updated the patch, along with the requested changes I've also removed mixer->flags and mixer->format, they're not needed anymore.

::: gst-libs/gst/audio/audio-channel-mixer.c
@@ +124,3 @@
+       * an identity matrix */
+      if (flags & GST_AUDIO_CHANNEL_MIXER_FLAGS_UNPOSITIONED_IN) {
+        matrix[ci][co] = ci == co ? 1.0 : 0.0;

This is related to determining passthrough, and lets us avoid storing positions in the mixer structure.

The passthrough calculations were wrong before, as explained in the commit message, as they didn't check the actual matrix, but rather the positions, this commit fixes it, and in order not to special case anything this is needed :)

@@ +446,3 @@
         RATIO_FRONT_SIDE);
   } else if (!in_has_front && in_has_center && in_has_side && out_has_front) {
+    gst_audio_channel_mixer_fill_one_other (matrix, in_c, out_f,

Hm it's quite tricky to disentangle this, if by could you mean "should" I'll do it :)

@@ +840,3 @@
+      mix->matrix[i] = g_new (gfloat, out_channels);
+      for (j = 0; j < out_channels; j++) {
+        mix->matrix[i][j] = i == j ? 1.0 : 0.0;

Yes, that's not related to what we did before, it's to handle your previous requirement of "identity if NULL" :)

@@ +965,3 @@
+    for (j = 0; j < mix->out_channels; j++) {
+      if ((i == j && mix->matrix[i][j] != 1.0f) ||
+          (i != j && mix->matrix[i][j] != 0.0f)) {

It's documented directly in the API comment :)
Comment 35 Mathieu Duponchelle 2017-09-22 14:23:10 UTC
Created attachment 360263 [details] [review]
[API]: gst_audio_channel_mixer_new_with_matrix

+ Refactor previous constructor to call on that new constructor

+ Reimplement is_passthrough to strictly check whether the matrix
  is an identity matrix, comparing channel-masks was incorrect:
  the mixer may be remixing from a list of positions to the same
  list of positions, but ordered differently, and reciprocally,
  the mixer may be remixing from a list of positions to another
  list of positions identically ordered

+ Remove unused tmp field, must have been a refactoring leftover
Comment 36 Mathieu Duponchelle 2017-09-22 14:42:51 UTC
(In reply to Sebastian Dröge (slomo) from comment #30)
> Review of attachment 360188 [details] [review] [review]:
> 
> ::: gst-libs/gst/audio/audio-converter.c
> @@ +670,3 @@
> +
> +gfloat **
> +mix_matrix_from_g_value (guint in_channels, guint out_channels,
> 
> static

Fixed
Comment 37 Sebastian Dröge (slomo) 2017-09-22 14:52:07 UTC
(In reply to Mathieu Duponchelle from comment #32)
> (In reply to Sebastian Dröge (slomo) from comment #28)
> > Review of attachment 360206 [details] [review] [review] [review]:
> > 
> > Looks good generally, just some minor things left
> > 
> > ::: gst/audioconvert/gstaudioconvert.c
> > @@ +350,3 @@
> > +          other_channels = gst_value_array_get_size (first_row);
> > +        } else {
> > +          GST_WARNING_OBJECT (this, "Empty mix matrix's first row");
> > 
> > Maybe we can catch the "invalid matrix" cases in set_property() already?
> > 
> 
> We can indeed output the warning there, but what should we do then ? Not set
> the matrix and proceed without one? This may be surprising to the user if he
> doesn't check his logs.
> 
> Otherwise we can output the warning there and error out here, but that
> doesn't make much of a difference?

Usually we print a warning (g_warning()) and do nothing, i.e. don't set the matrix.

> > @@ +952,3 @@
> > +
> > +      g_value_init (&this->mix_matrix, GST_TYPE_ARRAY);
> > +      g_value_copy (value, &this->mix_matrix);
> > 
> > It seems like there is no way to actually unset the matrix after you set it
> > once. You could provide an empty array I guess (i.e. a value just
> > initialized to GST_TYPE_ARRAY), but this would still succeed the
> > G_IS_VALUE() checks. You should probably special-case the empty array as
> > "unset".
> 
> Thing is I'd ideally set this as CONSTRUCT_ONLY, but then this won't work
> from the command line, same thing for the dithering and noise shaping
> properties by the way

Or you just handle the empty array as unset, because that's what you would get if you only initialize the GValue but never set it :)


Good to go for me except for the two things above.
Comment 38 Mathieu Duponchelle 2017-09-22 18:19:11 UTC
(In reply to Sebastian Dröge (slomo) from comment #37)
> Usually we print a warning (g_warning()) and do nothing, i.e. don't set the
> matrix.

Done

> Or you just handle the empty array as unset, because that's what you would
> get if you only initialize the GValue but never set it :)

Done, it's indeed better

> Good to go for me except for the two things above.

yay \o/
Comment 39 Mathieu Duponchelle 2017-09-22 18:19:58 UTC
Attachment 360188 [details] pushed as 4196e67 - [API]: GST_AUDIO_CONVERTER_OPT_MIX_MATRIX
Attachment 360206 [details] pushed as 8d52a10 - audioconvert: [API]: expose mix-matrix property.
Attachment 360263 [details] pushed as 877d6fa - [API]: gst_audio_channel_mixer_new_with_matrix