Created attachment 84902 [details] [review]
Proposed patch

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 :)

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

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.

(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.

(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 ?

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.

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.

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.

 

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.

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?

        * 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.