[Bug 785471] [API]: gst_audio_channel_mixer_new_with_matrix

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=785471

--- Comment #37 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
(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]:
> > 
> > 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.

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