GNOME Bugzilla – Bug 601617
matroska-demuxer triggers an assert in gststructure.c
Last modified: 2010-03-26 11:22:07 UTC
Hi, I am setting a pipeline 'filesrc ! matroskademux ! queue ! identity ! fakesink' and then set its state to PAUSED(set_state, get_state) and do gst_element_seek () on the demuxer. Most of the time that works but sometimes i am getting an assert triggered in gststructure.c Tried the test with different matroska files(mjpeg) and also with a simple matroska file generated with gst-launch and 'testvideosrc ! muxer ! filesink'. Below you can see a backtrace from gdb: ----------------------------------------------- GStreamer-CRITICAL **: gst_structure_set_parent_refcount: assertion `refcount == NULL' failed aborting... Program received signal SIGABRT, Aborted.
+ Trace 219032
Also repeated the test with the latest version of the demuxer from git. Here is the part that triggers the assert: 2053 g_assert (demux->src->len == demux->num_streams); 2054 for (i = 0; i < demux->src->len; i++) { 2055 GstMatroskaTrackContext *stream; 2056 2057 stream = g_ptr_array_index (demux->src, i); 2058 gst_event_ref (event); 2059 gst_pad_push_event (stream->pad, event); 2060 2061 if (G_UNLIKELY (stream->pending_tags)) { 2062 GST_DEBUG_OBJECT (demux, "Sending pending_tags %p for pad %s:%s : %" 2063 GST_PTR_FORMAT, stream->pending_tags, 2064 GST_DEBUG_PAD_NAME (stream->pad), stream->pending_tags); 2065 gst_element_found_tags_for_pad (GST_ELEMENT (demux), stream->pad, <---------------- 2066 stream->pending_tags); 2067 stream->pending_tags = NULL; 2068 } 2069 }
This looks like the tags are already owned by another message, event or something.
Could you attach a sample application that triggers this behaviour? From looking at the code I don't see how this could ever happen
Sure, i will create an application and attach it Just an update - after i registered the bug i did a bit more testing and it seems to be a race condition. gst_matroska_demux_send_event () was called twice in two different contexts: once for 'flush-start' and second time for 'newsegment'
...and the 'flush-start' comes from the flushing seek i am doing on the demuxer
gst_matroska_demux_send_event () is not thread safe at the moment. BTW, Does it make sense to send the pending tags with 'flush-start' and 'newsegment'? Isn't that supposed to be done only for the new tag events?
Created attachment 148988 [details] [review] matroskademux: only send pending tags with newsegment events > gst_matroska_demux_send_event () is not thread safe at the moment. > BTW, Does it make sense to send the pending tags with 'flush-start' and > 'newsegment'? Isn't that supposed to be done only for the new tag events? No, you're quite right. They should not be sent with flush-start/flush-stop, only after a newsegment event. Nice catch! Does the attached patch make things work? Btw, if you issue a seek that early (ie. you don't wait for preroll), chances are it will fail. Might be a bit racy.
Access to demux->src also needs to make safe I guess.
Yes, this patch makes things work. And regarding the seek - i am changing the state of the pipeline to PAUSED and then waiting for the state transition to occur with gst_element_get_state () and also waiting for the 'no-more-pads' signal to be emitted on the demuxer. Isn't that enough? Isn't the pipeline prerolled at that stage? I have asked that question a couple of times on irc but haven't got any reply.
> And regarding the seek - i am changing the state of the pipeline to PAUSED and > then waiting for the state transition to occur with gst_element_get_state () > and also waiting for the 'no-more-pads' signal to be emitted on the demuxer. > Isn't that enough? Isn't the pipeline prerolled at that stage? > I have asked that question a couple of times on irc but haven't got any reply. no-more-pads will be emitted before the pipeline is prerolled (most likely at some point during header parsing in matroska) - it will also be called from the streaming thread btw. Waiting for preroll with _get_state() should work, but is not ideal, since it will hang forever if there's an error. I'd suggest something like msg = gst_bus_timed_pop_filtered (GST_ELEMENT_BUS (pipeline), GST_CLOCK_TIME_NONE, GST_MESSAGE_ERROR | GST_MESSAGE_ASYNC_DONE); instead (even better: use a timeout, like 5*GST_SECOND, to be on the safe side if something goes wrong).
Thank you very much for your response, will try gst_bus_timed_pop_filtered () instead. I also thought waiting for the PAUSED state to occur with gst_element_get_state () would work but my tests show that most of the time it doesn't. On the other hand, i noticed that waiting for the 'no-more-pads' signal seems to always work, although it could be pure luck. I was testing with a small test application that was running for a couple of hours.
fwiw, _get_state() should always work as long as the pipeline does get linked up correctly and no errors occur. If you use _get_state() in production code make sure to use a timeout.
Well, _get_state() always work unless i add the element i link the demuxer to dynamically from the 'pad-added' handler. If i add the element dynamically then _get_state() seems not to be enough. Anyway, sorry for changing the topic. And back to the patch - are you going to commit the one attached to this ticket so i can also apply it locally?
Ok, committed patch: commit d0b25845ec6f2da545aeea550ad37d06d765f764 Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Thu Dec 3 08:58:08 2009 +0000 matroskademux: only send pending tags with newsegment events Send pending tags only from the streaming thread, just after we've sent the newsegment event, not with e.g. flush-start. This not only does the right thing, but also makes sure we're not trampling over variables set up in the streaming thread from the seeking thread in case someone tries to issue a seek just as the demuxer is parsing the headers. Fixes #601617. Spotted by Ognyan Tonchev. Keeping bug open until the demux->src pad array access is fixed - will try to do that later.
If I did not overlook some access, following commit should cater for thread-safety: commit b1f3e4d0cfcd9a5940955232a38b6be35e31c89f Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Tue Mar 23 17:47:48 2010 +0100 matroskademux: only seek when in proper state ... and data structures can be thread-safely accessed. See #601617.