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 744211 - interleave: assertion 'self->func != NULL' failed
interleave: assertion 'self->func != NULL' failed
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other All
: Normal critical
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-02-10 05:00 UTC by Vineeth
Modified: 2015-06-05 12:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch to fix assertion error (1.24 KB, patch)
2015-04-23 01:46 UTC, Vineeth
none Details | Review
patch to fix error (1.59 KB, patch)
2015-06-02 00:26 UTC, Vineeth
none Details | Review
patch to fix error (2.19 KB, patch)
2015-06-04 05:21 UTC, Vineeth
committed Details | Review

Description Vineeth 2015-02-10 05:00:04 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.
Comment 1 Vincent Penquerc'h 2015-03-13 15:16:09 UTC
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.
Comment 2 Vineeth 2015-04-23 01:46:59 UTC
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,
Comment 3 Vineeth 2015-05-20 10:29:33 UTC
Can someone review this :)
Comment 4 Thiago Sousa Santos 2015-05-28 07:14:20 UTC
Your patch breaks one of the unit tests. Please verify if the test is wrong or if your fix introduced another bug.
Comment 5 Vineeth 2015-05-29 04:03:05 UTC
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.
Comment 6 Thiago Sousa Santos 2015-06-01 18:08:23 UTC
(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.
Comment 7 Vineeth 2015-06-02 00:26:34 UTC
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.
Comment 8 Thiago Sousa Santos 2015-06-02 18:22:31 UTC
(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?
Comment 9 Vineeth 2015-06-04 05:21:32 UTC
Created attachment 304565 [details] [review]
patch to fix error

renamed channels to channel..


I meant the documentation present in source.
Comment 10 Thiago Sousa Santos 2015-06-05 12:03:08 UTC
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