GNOME Bugzilla – Bug 785471
[API]: gst_audio_channel_mixer_new_with_matrix
Last modified: 2017-09-22 18:20:40 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
Created attachment 356449 [details] [review] [API]: gst_audio_channel_mixer_new_with_matrix
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
As a proof-of-concept, can you move audiomixmatrix to this and remove its custom mixing code? :)
(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).
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
Created attachment 357579 [details] [review] [API]: GST_AUDIO_CONVERTER_OPT_MIX_MATRIX Taken from audiomixmatrix, credits to Vivia Nikolaidou
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.
Created attachment 357581 [details] [review] audioconvert: [API]: expose mix-matrix property. This obsoletes audiomixmatrix
(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.
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 on attachment 357580 [details] [review] audioconvert: refactor format removal. That's exactly why I wanted map_in_place() :)
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
(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?
> @@ +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 :)
(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 ?
(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 :)
(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
> 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 :)
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
Created attachment 360188 [details] [review] [API]: GST_AUDIO_CONVERTER_OPT_MIX_MATRIX Taken from audiomixmatrix, credits to Vivia Nikolaidou
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.
Created attachment 360190 [details] [review] audioconvert: [API]: expose mix-matrix property. This obsoletes audiomixmatrix
> 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 :)
Created attachment 360206 [details] [review] audioconvert: [API]: expose mix-matrix property. This obsoletes audiomixmatrix
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.
(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 :)
Such API would indeed be nice for people writing this in C.
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".
Review of attachment 360189 [details] [review]: Get it in, independent of everything else :)
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
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
(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 on attachment 360189 [details] [review] audioconvert: refactor format removal. Attachment 360189 [details] pushed as 7a407b6 - audioconvert: refactor format removal.
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 :)
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
(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
(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.
(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/
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