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 420206 - Collectpads causes a segv. when stopping after a pad remove
Collectpads causes a segv. when stopping after a pad remove
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal normal
: 0.10.13
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2007-03-19 17:57 UTC by Sjoerd Simons
Modified: 2007-05-25 09:26 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed patch (1.01 KB, patch)
2007-03-19 17:58 UTC, Sjoerd Simons
none Details | Review
alternative patchs (2.85 KB, patch)
2007-05-24 13:54 UTC, Wim Taymans
committed Details | Review

Description Sjoerd Simons 2007-03-19 17:57:45 UTC
Hi,

  Collectpads has a nice linked list to keep track of the Collectdata. But on
  pad removal it's _only_ removed if the the pads are stopped.. 

  Attached patch changes this so the CollectData is always removed
Comment 1 Sjoerd Simons 2007-03-19 17:58:57 UTC
Created attachment 84902 [details] [review]
Proposed patch
Comment 2 Sjoerd Simons 2007-05-01 08:01:53 UTC
Any comments on the patch? It seems pretty uncontroversial to me and it fixes segv's in my application, so i'd like to see it fixed upstream :)
Comment 3 Laurent Glayal 2007-05-15 14:16:13 UTC
We tried your patch (on the latest CVS code revision), we also had segv in elements using CollectPads (adder). Your patch seems to solve parts of our problems (no more segv and/or at least reduced frequency) but i'm not sure it solves all of the problems reported in the following bugs ( race ):

#402393
#383382

Comment 4 Stefan Sauer (gstreamer, gtkdoc dev) 2007-05-19 12:32:05 UTC
I wonder why its not removed if not stopped. Maybe it should be marked as to_be_removed and as such skipped elsewhere. When its save all marked list-nodes could be removed.
Comment 5 Sjoerd Simons 2007-05-20 17:11:31 UTC
(In reply to comment #3)
> We tried your patch (on the latest CVS code revision), we also had segv in
> elements using CollectPads (adder). Your patch seems to solve parts of our
> problems (no more segv and/or at least reduced frequency) but i'm not sure it
> solves all of the problems reported in the following bugs ( race ):
> 
> #402393
> #383382
> 

If you've got some reasonably small stand-alone testcase to reproduce your 
segv's i'm willing to look at it. Bute not that it might be an issue in the adder instead of collectpads.. 

For my testcase (videomixer and modified video mixer) this patch completely solves the segv.
Comment 6 Sjoerd Simons 2007-05-20 17:21:07 UTC
(In reply to comment #4)
> I wonder why its not removed if not stopped. Maybe it should be marked as
> to_be_removed and as such skipped elsewhere. When its save all marked
> list-nodes could be removed.

I don't know why it's not removed, the locking in CollectPads seems fine to do it there.. Flagging that it's supposed to be removed add a lot of unnecessary complexity (And users of the interface should also cope with it...). 

A guarantee that no pads will be removed untill stopped seems a little strange to me ?
Comment 7 Stefan Sauer (gstreamer, gtkdoc dev) 2007-05-22 11:36:54 UTC
Sjoerd, I think the patch makes sense. If you have a test case then we can turn into a unit test maybe it would help to convince people that thsi should go in.
Comment 8 Jan Schmidt 2007-05-23 09:27:27 UTC
Reading the patch and the code, I wondered the same thing as Stefan.

I don't know what the original intention of the code is. It's Wim's code, perhaps he can remember.

I agree, it's wrong for the code to delete the data and then leave the pad in the collectpads list. I suspect it's possibly supposed to mark the pad as EOS'd so that the user can close down the stream properly? Rather than just have the stream disappear off the radar, which I can imagine is harder to bookkeep in muxers.

Comment 9 Wim Taymans 2007-05-24 11:44:29 UTC
I can't remember exactly. I do remember that we need two lists (or one with a marker, maybe?) because we cannot simply modify the list when removing a pad while potentially collect is running over the list. We cannot protect the list with a lock because collect might block (when pushing to a prerolled sink) so that the pad removal will also block.

So currently, there is one master list (pad_list). Right before calling collect, all of pad_list is copied to ->data and then collect is supposed to operate on ->data. Removing pads while collect is running should not touch this list, hence the check. I think some code used to iterate ->data to see which pads were in the collection.

 
Comment 10 Wim Taymans 2007-05-24 11:46:32 UTC
ok, it seems that when constructing ->data, we should simply ref the elements put in the list. Then we can remove them from the pad_list without having to worry about collect having both a consistent list and data.
Comment 11 Wim Taymans 2007-05-24 13:54:07 UTC
Created attachment 88737 [details] [review]
alternative patchs

This patch does proper refcounting on both lists so that one can remove stuff from either list independently.

Does this work better?
Comment 12 Wim Taymans 2007-05-25 09:26:43 UTC
        * libs/gst/base/gstcollectpads.c: (gst_collect_pads_finalize),
        (unref_data), (gst_collect_pads_remove_pad),
        (gst_collect_pads_check_pads):
        Use additional refcounting to avoid crashes when dynamically adding and
        removing pads. Fixes #420206.