GNOME Bugzilla – Bug 782095
decodebin: Crash on shutdown with chained opus file
Last modified: 2017-06-05 12:14:38 UTC
jmspeex_: - 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 - https://jmvalin.ca/misc_stuff/totem_crash2.opus - 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 Backtrace 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)
+ Trace 237418
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. commit b97cbe678f2cf21777295c0d6ac050ca4b183336 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> 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. https://bugzilla.gnome.org/show_bug.cgi?id=782095
Note that I just filed this related bug: https://bugzilla.gnome.org/show_bug.cgi?id=782132 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]: ::: gst/gstelement.c @@ +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.