GNOME Bugzilla – Bug 782095
decodebin: Crash on shutdown with chained opus file
Last modified: 2017-06-05 12:14:38 UTC
- Hey, I'm not sure whether this is a problem with totem or whatever backend it uses, but I have a perfectly audio valid file that crashes totem
- This particular file is a chained Opus file, but even without chains, any file with a preskip value larger than one frame causes a crash
I reproduce it with totem 3.24, gstreamer* 1.11.91
Thread 1 "totem" received signal SIGSEGV, Segmentation fault.
demuxer_source_pad_probe (pad=pad@entry=0x7fffa4007030, info=info@entry=0x7fffffff9ca0, user_data=0x55555646f280) at gstdecodebin2.c:2013
2013 if (parent_chain->active_group == group)
I can reproduce this (in totem, not in gst-play).
It should never crash of course, even if it's not a valid file.
Created attachment 350990 [details] [review]
fix user after free
This fixed the crash. I still see a glib warning about adding an inactive pad though, not quite sure where that's from yet.
Pushed, I'll leave this open as I'll get back to debug the cause of the warning soon.
Author: Vincent Penquerc'h <email@example.com>
Date: Wed May 3 16:02:19 2017 +0100
decodebin2: fix use after free from demuxer flush pad probe
In some cases, we could get a flush-stop event after the chain structure
containing the demuxer was freed.
Note that I just filed this related bug:
One thing I forgot to mention is that the file mentioned in this bug report should play for 90 seconds. If it only plays for one second, then it means it's never playing the other streams in the chain.
Created attachment 351041 [details] [review]
fix for the warning
This seems correct, assuming I got the semantics of the NEED_PARENT flag, I'd never seen it before.
Re-adding a pad after removing it would fail to activate the pad, as it was then in "need parent" mode, whereas it wasn't on the first addition.
I'll add a separate bug for the chained opus bug, as it only plays what seems to be the first chain.
Review of attachment 351041 [details] [review]:
@@ +828,3 @@
GST_TRACER_ELEMENT_REMOVE_PAD (element, pad);
gst_object_unparent (GST_OBJECT_CAST (pad));
+ GST_OBJECT_FLAG_UNSET (pad, GST_PAD_FLAG_NEED_PARENT);
Not sure if this is ok: the reason for that flag is to guarantee that the pad functions are only ever called with the parent!=NULL, so that you don't get any spurious calls with parent==NULL when the pad is just removed from the element.
So this would be exactly the case here...
OK, then gst_pad_set_active should not check the flag then, since the pad is supposed to be set to active before being added I think. Would such a patch instead be OK ?
Or maybe just have whoever re-adds a pad should unset it before doing so. It seems like a strange behaviour to remove and re-add a pad.
I don't get the warning if I revert that patch on master. Tempting to just obsolete the patch...
Comment on attachment 351041 [details] [review]
fix for the warning
The warning doesn't happen after the oggdemux/opusdec fix.