GNOME Bugzilla – Bug 744211
interleave: assertion 'self->func != NULL' failed
Last modified: 2015-06-05 12:03:52 UTC
When we try to launch with the below pipeline gst-launch-1.0 audiotestsrc num-buffers=20 ! interleave channel-positions-from-input=False ! audioconvert ! autoaudiosink the following error occurs ** (gst-launch-1.0:31758): CRITICAL **: gst_interleave_collected: assertion 'self->func != NULL' failed This happens because if (g_atomic_int_get (&self->configured_sinkpads_counter) == self->channels) { ret = gst_interleave_sink_setcaps (self, data->pad, caps, &info); ... } self->func will be defined only when self->channels is equal to sinkpads_counter but if (self->channel_positions_from_input) channels = g_atomic_int_add (&self->channels, 1); self->channels will be set only in the case of channel_positions_from_input being TRUE. Hence the error. Not exactly sure how to fix this. Can someone check this.
It seems to be like a bug introduced in the move to 0.11: Commit 4945af5eff456defc0073a38982e0e92a0a4305b ("interleave: port to 0.11") has this hunk: @@ -479,8 +461,11 @@ gst_interleave_request_new_pad (GstElement * element, GstPadTemplate * templ, if (templ->direction != GST_PAD_SINK) goto not_sink_pad; - channels = g_atomic_int_add (&self->channels, 1); padnumber = g_atomic_int_add (&self->padcounter, 1); + if (self->channel_positions_from_input) + channels = g_atomic_int_add (&self->channels, 1); + else + channels = padnumber; pad_name = g_strdup_printf ("sink_%u", padnumber); new_pad = GST_PAD_CAST (g_object_new (GST_TYPE_INTERLEAVE_PAD, Before the port, self->channels will always be incremented, regardless of self->channel_positions_from_input. Since the commit purports to port, I doubt the change of semantics was intended. No clue what this bit of code is supposed to be doing though.
Created attachment 302193 [details] [review] patch to fix assertion error Hi Vincent, I think the condition to check for the self->channel_positions_from_input to be TRUE to increment is not proper. Even when i increment without the condition, the behavior seems to be working fine. So i attached a patch for the same. Please verify,
Can someone review this :)
Your patch breaks one of the unit tests. Please verify if the test is wrong or if your fix introduced another bug.
I went through the code again and these are my observations. The main reason of this error is that, if channel_positions_from_input is set to TRUE, the channel positions is taken from input caps. But whenever channel_positions_from_input is set to FALSE, it is expecting channel-positions property to be set through pipeline. I am not sure why a property for channel_positions_from_input is needed. From code we are by default initializing it as TRUE. So if will take the channel position from input caps if provided. And when we provide property channel-positions then the value will be set to FALSE and it will take the provided channel-positions. The problem with controllable channel_positions_from_input property arises when it is set without channel-positions property or set after channel-positions property is set. These can be verified in the test cases by making some minor changes as follows. In the test case test_interleave_2ch_pipeline_input_chanpos, channel-positions is not being set. When we change the channel-positions-from-input property to FALSE, then we get the assertion errors and the test case fails. In the test case test_interleave_2ch_pipeline_custom_chanpos, channel-positions are being set, and channel-positions-from-input property is being set to TRUE before setting channel-positions. But if we set channel-positions-from-input property after channel-positions then the test case fails.
(In reply to Vineeth from comment #5) > I went through the code again and these are my observations. > The main reason of this error is that, > if channel_positions_from_input is set to TRUE, the channel positions is > taken from input caps. > But whenever channel_positions_from_input is set to FALSE, it is expecting > channel-positions property to be set through pipeline. > > I am not sure why a property for channel_positions_from_input is needed. > From code we are by default initializing it as TRUE. So if will take the > channel position from input caps if provided. > And when we provide property channel-positions then the value will be set to > FALSE and it will take the provided channel-positions. > > The problem with controllable channel_positions_from_input property arises > when > it is set without channel-positions property or set after channel-positions > property is set. > These can be verified in the test cases by making some minor changes as > follows. > > In the test case test_interleave_2ch_pipeline_input_chanpos, > channel-positions is not being set. When we change the > channel-positions-from-input property to FALSE, then we get the assertion > errors and the test case fails. In this test, the input positions is to be taken from caps. One of the channels is set as a LEFT and the other as a RIGHT. The left channels will have its values set to -1 while the right will have it set to 1. > > In the test case test_interleave_2ch_pipeline_custom_chanpos, > channel-positions are being set, and channel-positions-from-input property > is being set to TRUE before setting channel-positions. But if we set > channel-positions-from-input property after channel-positions then the test > case fails. In this one, the interleave should ignore the input caps positions and use whatever was set on its property. in this case it is CENTER_FRONT and CENTER_REAR with the -1 and 1 values (just like above). Both tests seem to make sense to me. This also made me realize that your test of setting the property to false without giving a channel-positions array is a wrong usage. The user should set a channel-positions array if it wants to ignore input caps positions so the assertion might make sense in the end as it is a programming error. This might not be a valid bug after all. One could argue that the channel-positions-from-input property is not needed as it would be possible to just check if the input-positions is NULL or not but we can't change that now as it would be a behavior break and applications relying on the current way would behave differently.
Created attachment 304393 [details] [review] patch to fix error There was similar error in audiointerleave element. The same was fixed almost similar to my patch but for a small change. http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=db5b3b5c4124f340f9bf784a6db7141868febce2 I have made changes similar to audiointerleave and it fixes the error and the test cases also run fine/ But the observation regarding setting of channel-positions-from-input as FALSE or setting it to TRUE after channel-positions in the test cases still holds true. We should probably update the property description to explain the same to user.
(In reply to Vineeth from comment #7) > Created attachment 304393 [details] [review] [review] > patch to fix error > > There was similar error in audiointerleave element. > The same was fixed almost similar to my patch but for a small change. > > http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/ > ?id=db5b3b5c4124f340f9bf784a6db7141868febce2 > I have made changes similar to audiointerleave and it fixes the error and > the test cases also run fine/ > > > But the observation regarding setting of channel-positions-from-input as > FALSE or setting it to TRUE after channel-positions in the test cases still > holds true. > We should probably update the property description to explain the same to > user. Why not also rename the variable name to 'channel' as it is not the total number of channels but the channel number of the input? The properties are documented in the source, that will show up in the element docs but not on gst-inspect that only show the shorter docs. Am I missing something here?
Created attachment 304565 [details] [review] patch to fix error renamed channels to channel.. I meant the documentation present in source.
Thanks for the update, just merged your fix. If you think the documentation can improved feel free to provide another patch. commit 0e5631c5c002c1ae8ad9db06fb8b6806ba5b625c Author: Vineeth TM <vineeth.tm@samsung.com> Date: Thu Jun 4 14:18:01 2015 +0900 interleave: error when channel-positions-from-input=False self->channels is being incremented only when channel-positions-from-input is set as TRUE. So in case of FALSE self->func is not set and hence creating assertion error. Hence removing the condition to increment self->channels. https://bugzilla.gnome.org/show_bug.cgi?id=744211