GNOME Bugzilla – Bug 783028
gstmpdparser : Crash when playing some of the Dash LIVE URL's with Gstreamer v1.12
Last modified: 2017-07-13 06:35:28 UTC
Dear Members, A crash in mpd parser is seen when playing some of the DASH Live URL's, with latest released Gstreamer v1.12. URL used for testing : http://vm2.dashif.org/livesim/segtimeline_1/testpic_2s/Manifest.mpd Function causing the crash, @ LINE#5954 : gst_mpd_client_get_next_segment_availability_start_time()
The problem seems to be the following: * Manifest is initially parsed * GstMpdClient gets created along with a list of GstActiveStreams * A GstActiveStream gets assigned to each GstAdaptiveDemuxStream in setup_streams() * Late the manifest gets updated * A new GstMpdClient gets created along with a list of GstActiveStreams * That client replaces the previous one * The previous one gets destroyed along with the list of GstActiveStreams ... And eventually code tries accessing the GstAdaptiveDemuxStream's active_stream which ... is from the previous GstMpdClient and therefore invalid. Not 100% sure who's at fault, but should gst_dash_demux_setup_streams() be called again when updating the manifest ?
Note: It also fails with http://vm2.dashif.org/livesim/segtimeline_1/testpic_6s/Manifest.mpd Confirms it's not a race, but a real issue. Might be related to segment timeline support ?
What should happen is that the AdaptiveDemux streams should remain the same and the internal dash streams are replaced. What I believe is the issue that the old ones are removed and not replaced. It seems to be related to the [streams, next_streams, prepared_streams] thing as dashdemux expects them to be in streams but I think they are as "prepared_streams" at that stage. Will continue debugging.
Also fails with 1.12
I'm also investigating this bug. This special mpd has zero minimumUpdatePeriod which is mentioned as special case by spec. Not only handle this crash, we might add handle for this zero minimumUpdatePeriod IMHO.
Created attachment 352849 [details] [review] mpdparser: remove duplicate free of client data
Created attachment 352850 [details] [review] adaptivedemux: do not erase data while updates-loop is running Make sure the manifest update loop is stopped before proceeding with the resetting of the manifest data. Otherwise, the updates loop will try to use it and it leads to a segfault
Created attachment 352851 [details] [review] dashdemux: update manifest streams correctly if pads aren't exposed In some cases, it is possible that we need to update the manifest before pads have been exposed at all. If there are no current pads, just expose the next prepared streams. This doesn't handle the case where a manifest update would happen while a live streams is changing periods, which is a type of use case that we're unaware of real usages yet.
This still has issues after those fixes. Right now it tries to start at 'now' time but that is in the future in terms of the stream so that leads to this check failing: https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/adaptivedemux/gstadaptivedemux.c?id=304a628de777ed0a180e8a23dc0e689ca9b0e24c#n3725 And it just aborts. Trying to figure out what exactly does this check mean to do. A side question is whether we should really care to seek to 'now' time or if it is simpler just to go to the last fragment and start with it. A downside here is that if the fragment is very long it might make us be too far in the past for a live stream. Maybe it is not a good idea.
For the record, that check fails because it hasn't pushed any buffers because it tries to seek beyond the last fragment timestamp so it hasn't updated its segment.position and it is 0. So it is not in the live seeking range.
(In reply to Thiago Sousa Santos from comment #9) > A side question is whether we should really care to seek to 'now' time or if > it is simpler just to go to the last fragment and start with it. A downside > here is that if the fragment is very long it might make us be too far in the > past for a live stream. Maybe it is not a good idea. The problem is that you rarely want to use the absolute latest fragment. The live seek range returned takes into account the "safety margin" that most protocols recommend (i.e. a few fragments before the latest one). (In reply to Thiago Sousa Santos from comment #10) > For the record, that check fails because it hasn't pushed any buffers > because it tries to seek beyond the last fragment timestamp so it hasn't > updated its segment.position and it is 0. So it is not in the live seeking > range. Ok, I'll look into it. Maybe we should take into account the fact that nothing has been downloaded yet.
The reason why it ends up with a position of 0 is because ... we try to seek to an invalid position. The manifest doesn't contain a suggestedPresentationDelay, which should be used to figure out how much back from "now" we should start playing. Regarding that property, the specs state that : "When not specified, then no value is provided and the client is expected to choose a suitable value." We currently don't have a default suitable value.
(In reply to Seungha Yang from comment #5) > I'm also investigating this bug. This special mpd has zero > minimumUpdatePeriod which is mentioned as special case by spec. Not only > handle this crash, we might add handle for this zero minimumUpdatePeriod > IMHO. Do you have any updates on that special case ? Can you file a separate bug ?
(In reply to Edward Hervey from comment #13) > (In reply to Seungha Yang from comment #5) > > I'm also investigating this bug. This special mpd has zero > > minimumUpdatePeriod which is mentioned as special case by spec. Not only > > handle this crash, we might add handle for this zero minimumUpdatePeriod > > IMHO. > > Do you have any updates on that special case ? Can you file a separate bug ? I'll update about it in next week with new bug. Note that, when I saw dash.js implementation, it limits minimum update period to 1 sec whatever minimumUpdatePeriod is defined.
I pushed the two first commits, since those were safe. I'm not sure the last patch is needed anymore. commit 5a693fdd9dc1e7e227309424503e3e0e78f9e9c0 Author: Thiago Santos <thiagossantos@gmail.com> Date: Mon May 29 22:28:21 2017 -0700 adaptivedemux: do not erase data while updates-loop is running Make sure the manifest update loop is stopped before proceeding with the resetting of the manifest data. Otherwise, the updates loop will try to use it and it leads to a segfault https://bugzilla.gnome.org/show_bug.cgi?id=783028 commit 95a27867411826549119711a3c35b3493ab3070e Author: Thiago Santos <thiagossantos@gmail.com> Date: Mon May 29 22:26:09 2017 -0700 mpdparser: remove duplicate free of client data https://bugzilla.gnome.org/show_bug.cgi?id=783028
Hello Edward Hervey I'll update on bug #780589 for minimumUpdatePeriod handling Anyway, I cannot access reported issue streams now... (maybe temporary server close?)
I am getting crash using that live dash url on 1.12: https://live-w.lwc.vrtcdn.be/groupc/live/d05012c2-6a5d-49ff-a711-79b32684615b/live.isml/.mpd Is the same error or a new one?
That URL doesn't crash on master, but doesn't play either. Does play with 1.10.
Can still reproduce the crash with the patch using the below patches : mpdparser: remove duplicate free of client data adaptivedemux: do not erase data while updates-loop is running Used the same URL as reported : http://vm2.dashif.org/livesim/segtimeline_1/testpic_2s/Manifest.mpd Crash still appears in the gst_mpd_client_get_next_segment_availability_start_time() function.
Existing patches backported to 1.12. Will check remaining issue afterwards
Is there is any other patch/fix available for this, as the reported issue with the URL http://vm2.dashif.org/livesim/segtimeline_1/testpic_2s/Manifest.mpd, can still be reproduced with v1.12.1 ? Also, another crash in mpdparser is observed when using the below live URL's : http://irtdashreference-i.akamaihd.net/dash/live/901161/bfs/manifestBR.mpd" http://irtdashreference-i.akamaihd.net/dash/live/901161/bfs/manifestARD.mpd" Function in which crash is observed : gst_mpdparser_get_rep_idx_with_max_bandwidth() Line : framerate = representation->RepresentationBase->frameRate;
Pushed to both master and 1.12 commit ead63a268647eff2bcf92d8dc55ca41ee44247c2 Author: Thiago Santos <thiagossantos@gmail.com> Date: Mon May 29 22:47:10 2017 -0700 dashdemux: update manifest streams correctly if pads aren't exposed In some cases, it is possible that we need to update the manifest before pads have been exposed at all. If there are no current pads, just expose the next prepared streams. This doesn't handle the case where a manifest update would happen while a live streams is changing periods, which is a type of use case that we're unaware of real usages yet. https://bugzilla.gnome.org/show_bug.cgi?id=783028