GNOME Bugzilla – Bug 777376
matrixmix: New element that mixes audio channels
Last modified: 2017-02-23 19:04:18 UTC
This element transforms a given number of input channels into a given number of output channels according to a given transformation matrix. The 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.
Created attachment 343630 [details] [review] 0001-matrixmix-New-element-that-mixes-audio-channels.patch
Do we maybe want an "audio" in the name here?
Let's call it audiomixmatrix then :)
Review of attachment 343630 [details] [review]: 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
Created attachment 343638 [details] [review] 0001-audiomixmatrix-New-element-that-mixes-audio-channels.patch Thanks for the comments, here's the updated patch.
Created attachment 343639 [details] [review] 0001-audiomixmatrix-New-element-that-mixes-audio-channels.patch Small fix: 0 channels shouldn't be an option (they were intended to mean that mode==auto, but it's better to have auto mode explicitly)
IMHO, when constructing the matrix, the user could make a mistake. Having input and output channels as an explicit property enforce that it isn't at least too small. Also, as you said, forgetting to set the matrix shouldn't make the element auto-negotiate one for you. I think the current way might be too verbose, but it's also the most intuitive one. On the other hand, maybe the default should be 0 input channels, 0 output channels and a NULL matrix. If the element tries to go to READY like this, it prints out a big fat warning "PLEASE SET YOUR VARIABLES" and refuses to transition. With the current defaults, it just does stereo passthrough instead, but it also sets the channel-mask to 0, essentially acting as a capssetter.
(In reply to Vivia Nikolaidou from comment #7) > IMHO, when constructing the matrix, the user could make a mistake. Having > input and output channels as an explicit property enforce that it isn't at > least too small. Also, as you said, forgetting to set the matrix shouldn't > make the element auto-negotiate one for you. I think the current way might > be too verbose, but it's also the most intuitive one. Ack. So for the auto mode, what's the use case of that? It seems like a very specific thing, not necessarily what one would expect. And what happens if mode=auto and a matrix is set? IMHO this auto mode is a bit weird and better to be done at the application level. > On the other hand, maybe the default should be 0 input channels, 0 output > channels and a NULL matrix. If the element tries to go to READY like this, > it prints out a big fat warning "PLEASE SET YOUR VARIABLES" and refuses to > transition. With the current defaults, it just does stereo passthrough > instead, but it also sets the channel-mask to 0, essentially acting as a > capssetter. Yes
I think it's a fairly common use case to want to pick the first N audio channels of a stream. Also, at the application level it is going to be very complicated to do the same - you'd need a bunch of pad probes and juggling, it's much cleaner inside the element.
Maybe not call it "auto" then but "first-channels" or so (needs a better name)?
Created attachment 343661 [details] [review] 0001-audiomixmatrix-New-element-that-mixes-audio-channels.patch Good idea, thanks. That would also allow for more modes to be added later, e.g. average-of-inputs? Changed this, plus the default values for in-channels. out-channels and matrix are 0, 0 and NULL respectively. In manual mode, having either of these unchanged makes the element spit out an error message and refuse to link.
Created attachment 343669 [details] [review] 0001-audiomixmatrix-New-element-that-mixes-audio-channels.patch The channel-mask property is ignored on first-channels mode (Doesn't make sense to negotiate without channels but with a channel mask. If downstream wants a specific mask, let them have it)
Review of attachment 343669 [details] [review]: Can you set the matrix using gst-launch? If yes, can you add an example of that in the doc blurb at the top? If not, it would be nice to add a short C blurb showing how to create one. It's not evident to me how to do this, so it would be really hard for someone who is a GLib/GStreamer expert! Any reason to force the identity matrix if first-channels=1 ? Couldnt it also work with a subset of a user-set matrix ? (or a superset extended by the identity matrix?) This also looks like something where ORC could be used if someone wants to have fun ;) ::: gst/audiomixmatrix/gstaudiomixmatrix.c @@ +549,3 @@ + * hang. */ + GST_ERROR_OBJECT (self, "Invalid settings detected in manual mode. " + "Please specify in-channels, out-channels and matrix."); Shouldn't you also put an error message here using GST_ELEMENT_ERROR() ?
(In reply to Olivier Crête from comment #13) > Review of attachment 343669 [details] [review] [review]: > > Can you set the matrix using gst-launch? If yes, can you add an example of > that in the doc blurb at the top? If not, it would be nice to add a short C > blurb showing how to create one. It's not evident to me how to do this, so > it would be really hard for someone who is a GLib/GStreamer expert! ITYM is *not* a GLib/GStreamer expert :)
> Can you set the matrix using gst-launch? Not yet but soon - deserialization for GstValueArray is throwing a not-implemented error :) There's a related discussion here: https://bugzilla.gnome.org/show_bug.cgi?id=777375 > Any reason to force the identity matrix if first-channels=1 ? Couldnt it > also work with a subset of a user-set matrix ? (or a superset extended by > the identity matrix?) I think what you're describing should better be implemented in another mode :) This one is for the use-case of just picking the first N channels of an incoming stream. > ::: gst/audiomixmatrix/gstaudiomixmatrix.c > @@ +549,3 @@ > + * hang. */ > + GST_ERROR_OBJECT (self, "Invalid settings detected in manual mode. " > + "Please specify in-channels, out-channels and matrix."); > > Shouldn't you also put an error message here using GST_ELEMENT_ERROR() ? There's a comment there, and you just quoted the last part of it :) Do you think it needs rephrasing? /* Not dispatching element error because we return empty caps anyway and * we should let it fail to link. Additionally, the element error would be * printed as WARN, so a possible gst-launch pipeline would appear to * hang. */ In short, there are three things here: 1) GST_ELEMENT_ERROR gets printed as a WARN, so at this early stage of caps negotiation, the naive gst-launch user wouldn't notice anything about why the pipeline fails. 2) We return empty caps anyway, so the negotiation will fail anyway. 3) I'm not sure if this isn't a bug, but if we throw a GST_ELEMENT_ERROR in addition to returning empty caps, the pipeline seems to stall during caps negotiation instead of outright failing. As a result, if we have GST_ELEMENT_ERROR instead of GST_ERROR_OBJECT, would see the pipeline hanging for no apparent reason.
> 3) I'm not sure if this isn't a bug, but if we throw a GST_ELEMENT_ERROR in > addition to returning empty caps, the pipeline seems to stall during caps > negotiation instead of outright failing. As a result, if we have > GST_ELEMENT_ERROR instead of GST_ERROR_OBJECT, would see the pipeline > hanging for no apparent reason. Rephrasing: As a result, if we have GST_ELEMENT_ERROR instead of GST_ERROR_OBJECT, *the naive gst-launch user* would see the pipeline hanging for no apparent reason.
Why does it hang? I think posting an error message is the right thing to do, and it should not hang. gst-launch-1.0 should shut down the pipeline then, and that should be that.
Turns out that transform_caps is called with the object lock taken. I didn't have time to debug it yesterday, but now I'm on it :)
I have a fix for the deadlock btw, just waiting for Vivia to file the corresponding bug with the testcase. However: don't post error messages from transform_caps() if empty caps are generated. It is not necessarily an error and might as well be some code that is probing if some caps configuration is possible (like the code that is introducing the deadlock actually is). We don't want to break everything in that case.
Bug report for the deadlock: https://bugzilla.gnome.org/show_bug.cgi?id=777449 OK, so if you think that the patch is OK the way it is now with just a GST_ERROR_OBJECT, please review the rest of it again :)
Would still be nice to have a short C example blurb? I assume you have that code in your application already? If it's not an error, make it a WARNING then!
I have a sample C example that works, would you like it attached to the bug report, or to the sources somehow, or...?
What I'd like is to have a short snippet in the doc? How to create the matrix, something like: Gst?? matrix = gst_???_new(); /* This is the part I dont know what to do */ add numbers to the matrix /* This is the part I dont know what to do */ g_object_set(element, "matrix", matrix, NULL); And if you have a simple test app, you can put it in tests/examples/ or even better would be having check unit tests, but I guess those can be done later.
Created attachment 343744 [details] [review] 0001-audiomixmatrix-New-element-that-mixes-audio-channels.patch Here you are, added both docs and tests/examples for the matrix generation. Let me know if something is unclear!
Review of attachment 343744 [details] [review]: 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 :)
Created attachment 344340 [details] [review] 0001-audiomixmatrix-New-element-that-mixes-audio-channels.patch Thanks, patch updated. The deserialization of GstValueArray is still blocking this from being pushed, but you can review this patch for now. :)
Created attachment 344423 [details] [review] 0001-audiomixmatrix-New-element-that-mixes-audio-channels.patch Oops, seems I accidentally attached the same patch again. Here's the right one :)
Review of attachment 344423 [details] [review]: ::: 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
Created attachment 344428 [details] [review] 0001-audiomixmatrix-New-element-that-mixes-audio-channels.patch Thanks - new patch iteration :)
Comment on attachment 344428 [details] [review] 0001-audiomixmatrix-New-element-that-mixes-audio-channels.patch After the serialization is solved one way or another
(In reply to Sebastian Dröge (slomo) from comment #30) > Comment on attachment 344428 [details] [review] [review] > 0001-audiomixmatrix-New-element-that-mixes-audio-channels.patch > > After the serialization is solved one way or another , it can be merged
Created attachment 344571 [details] [review] 0001-audiomixmatrix-New-element-that-mixes-audio-channels.patch One more negotiation fix: don't pass-through the channel-mask. Also some small fixes for the example application.
Created attachment 344650 [details] [review] 0001-audiomixmatrix-New-element-that-mixes-audio-channels.patch Converted floats to doubles for easier deserialization.
Created attachment 345246 [details] [review] audiomixmatrix: New element that mixes audio channels This element transforms a given number of input channels into a given number of output channels according to a given transformation matrix. The 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.
GST_TYPE_VALUE_ARRAY is impossible to use from bindings unfortunately. I think the best we can do here is to provide two properties: one using GValueArray (bindings), another using GST_TYPE_VALUE_ARRAY (allowing to use strings in e.g. gst-launch and gst_util_set_object_arg()). Independent of that I think the deserialization of arrays/lists is good to have and improves consistency, and there is almost no additional code added (just moved from one file to another).
Created attachment 346447 [details] [review] audiomixmatrix: New element that mixes audio channels This element transforms a given number of input channels into a given number of output channels according to a given transformation matrix. The 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.
Thanks, please review the new patch :) I have added both GValueArray and GstValueArray (which point to the same variable internally), and the example also contains both methods using an ifdef.
Review of attachment 346447 [details] [review]: Generally looks good. Not sure which property should be the one with the "matrix" name though ::: tests/examples/audiomixmatrix/test-audiomixmatrix.c @@ +155,3 @@ + serialized_matrix = gst_value_serialize (&v); + gst_printerrln ("Serialized matrix: %s", serialized_matrix); + g_free (serialized_matrix); Here would be nicer probably to show why this property exists... gst_util_set_object_arg (audiomixmatrix, "matrix", "< < 1, 0> ..."); @@ +203,3 @@ + g_value_unset (&v2); + g_value_take_boxed (&v, a); + g_object_set_property (G_OBJECT (audiomixmatrix), "matrix-value-array", &v); And here, the canonical way of setting properties would be g_object_set(..., "...", a, NULL);
Created attachment 346487 [details] [review] audiomixmatrix: New element that mixes audio channels This element transforms a given number of input channels into a given number of output channels according to a given transformation matrix. The 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.
(In reply to Sebastian Dröge (slomo) from comment #38) > Review of attachment 346447 [details] [review] [review]: > > Generally looks good. Not sure which property should be the one with the > "matrix" name though > > ::: tests/examples/audiomixmatrix/test-audiomixmatrix.c > @@ +155,3 @@ > + serialized_matrix = gst_value_serialize (&v); > + gst_printerrln ("Serialized matrix: %s", serialized_matrix); > + g_free (serialized_matrix); > > Here would be nicer probably to show why this property exists... > gst_util_set_object_arg (audiomixmatrix, "matrix", "< < 1, 0> ..."); Thanks, added that as a comment :) It's easier to put this as a comment, while manually creating the GstValueArray is trickier, therefore I kept it there.
Review of attachment 346487 [details] [review]: ::: tests/examples/audiomixmatrix/test-audiomixmatrix.c @@ +202,3 @@ + g_value_array_append (a, &v2); + g_value_unset (&v2); + g_object_set (G_OBJECT (audiomixmatrix), "matrix-value-array", a, NULL); a is leaked here
Attachment 346487 [details] pushed as fa47edf - audiomixmatrix: New element that mixes audio channels