After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 725298 - dashdemux: fails after MPD refresh
dashdemux: fails after MPD refresh
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal normal
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Reported: 2014-02-27 10:10 UTC by Chris Bass
Modified: 2014-12-02 16:27 UTC
See Also:
GNOME target: ---
GNOME version: ---

Fix segfault on MPD refresh (819 bytes, patch)
2014-02-27 10:10 UTC, Chris Bass
rejected Details | Review
Fix MPD refresh segfault. (5.37 KB, patch)
2014-09-30 13:28 UTC, David Waring
none Details | Review
Fix MPD refresh (7.06 KB, patch)
2014-10-10 10:44 UTC, David Waring
none Details | Review
Fix period selection (771 bytes, patch)
2014-12-02 10:06 UTC, David Waring
committed Details | Review

Description Chris Bass 2014-02-27 10:10:59 UTC
Created attachment 270451 [details] [review]
Fix segfault on MPD refresh

A segfault can occur when an MPD is refreshed after the time specified in the minimumUpdatePeriod MPD attribute.

The reason for the segfault is that a new GstMpdClient is created for the updated MPD, and demux->client is swapped for this new GstMpdClient without first acquiring dashdemux's client lock (hence code in other threads accessing demux->client at the same time may try to access the freed GstMpdClient).

The attached patch fixes the segfault by surrounding the lines that perform the client swap with GST_DASH_DEMUX_CLIENT_LOCK/UNLOCK, but note that this does not really fix the problem: now the client just sits there without downloading any more segments following the MPD refresh.
Comment 1 Thiago Sousa Santos 2014-02-27 13:59:45 UTC
Review of attachment 270451 [details] [review]:

This is because gst_dash_demux_refresh_mpd is called with the MPD_CLIENT_LOCK already. So this patch will cause a deadlock.

The segfault must come from some other access to the mpd client that is not holding the client lock appropriately.
Comment 2 Thiago Sousa Santos 2014-02-27 14:00:15 UTC
Thanks for reporting this, do you have a URL of a public live stream so we can test/debug this issue?
Comment 3 Thiago Sousa Santos 2014-02-27 14:03:14 UTC
And can you reproduce this reliably?
Comment 4 Chris Bass 2014-02-28 16:22:15 UTC
(In reply to comment #1)
> Review of attachment 270451 [details] [review]:
> This is because gst_dash_demux_refresh_mpd is called with the MPD_CLIENT_LOCK
> already. So this patch will cause a deadlock.

Yes, you're quite right.

After some further digging, I think I've got a better idea of the problem now.

As I read it, demux->streams is a list of GstDashDemuxStreams, each of which has an active_stream ptr, which points to one of the GstActiveStream structs in demux->client->active_streams.

When demux->client is freed (by gst_dash_demux_refresh_mpd, line 1323 of gstdashdemux.c), all the GstActiveStreams in demux->client->active_streams are also freed (since gst_mpd_client_free calls gst_active_streams_free); however, the active_stream ptrs in demux->streams are left pointing to GstActiveStreams in the active_streams list of the client that has just been freed.

Thus, when anything then comes to access data via the active_stream ptr of any of demux's GstDashDemuxStreams, they're accessing freed data - hence the segfault. The place where I'm typically getting segfaults is in gst_mpdparser_get_rep_idx_with_max_bandwidth, and it's because it's trying to access representation data from within the freed active stream; here's a backtrace:

  • #0 g_list_first
    from /usr/lib/
  • #1 gst_mpdparser_get_rep_idx_with_max_bandwidth
    at gstmpdparser.c line 2080
  • #2 gst_dash_demux_stream_select_representation_unlocked
    at gstdashdemux.c line 1633
  • #3 gst_dash_demux_stream_download_loop
    at gstdashdemux.c line 1432
  • #4 gst_task_func
    at gsttask.c line 319
  • #5 default_func
    at gsttaskpool.c line 70
  • #6 g_thread_pool_thread_proxy
    from /usr/lib/
  • #7 g_thread_proxy
    from /usr/lib/
  • #8 start_thread
    from /lib/
  • #9 clone
    from /lib/

Comment 5 Thiago Sousa Santos 2014-02-28 22:08:23 UTC
Yes, that's a possibility. Thanks for debugging it. The active stream variables need to be updated. Do you want to work on a patch for that?

Additionally, from a quick look on the code there seems to be lots of places that would deserve locking with the DASH_DEMUX_CLIENT_LOCK to avoid any issues if some thread tries to access demux->client while

          gst_mpd_client_free (demux->client);
          demux->client = new_client;

is being done.
Comment 6 Thiago Sousa Santos 2014-02-28 22:10:39 UTC
Btw, do you have a link to a public live source of a dash stream? Or are you using some locally generated one to test this?
Comment 7 David Waring 2014-06-13 11:58:41 UTC
I think the issue is that when gst_dash_demux_refresh_mpd creates it's new GstMpdClient it then doesn't change the active_stream pointers in the demux->streams items to point to the right item in the active_streams in the new client. The function seems to assume another function has changed all the pointers previously.

I think the "/* update the streams to play from the next segment */" loop in gst_dash_demux_refresh_mpd() should be using demux_stream->active_stream as the old active stream, looking up the equivalent active stream in the new_client and resetting demux_stream->active_stream.

I'm just testing a patch which does this now and it's been working so far (and showing up other problems like forgetting the MPD URL after the first refresh)
Comment 8 Thiago Sousa Santos 2014-06-13 13:52:22 UTC
Thanks for looking at this. Do you have a link to a public dash live stream so others can test this, too?
Comment 9 David Waring 2014-06-13 14:06:47 UTC
Unfortunately I'm using an internal server at the BBC and we don't have plans as yet for any public servers, we're still trialling DASH distribution. Although longer term we probably will have a public facing DASH server available, I have no idea at this point when that will be.

My suggestion for testing is to use pre-recorded DASH content with a modified MPD to make it look like it's a live stream. Using php or some other CGI scripting from a local server to generate a "live" MPD with appropriate BaseURLs and availabilityStartTime, would give you what looks like a live stream using pre-recorded segments.
Comment 10 David Waring 2014-07-16 15:05:28 UTC
Sorry for the delay, I'm still working on the patch. Had problems in testing with period boundaries not being handled properly and didn't want to release a patch until I'm satisfied it actually works... plus I've been on holiday for two weeks.

I'll raise the period boundary issues as a new ticket so that there's visibility on that too.
Comment 11 David Waring 2014-09-30 13:28:57 UTC
Created attachment 287447 [details] [review]
Fix MPD refresh segfault.

This patch fixes the MPD refresh segfault by initialising the active streams in the demux object to point to the new streams in the client object for the updated MPD. It does assume that the updated MPD will have the same adaptation set IDs as the previous MPD did, in order to find the same active streams in the updated MPD.

It also fixes an issue with the downloaded URI(s) being accessed after the the download object has been unref()'d.

I've tested it using which has a 1 minute minimumUpdatePeriod. I played the stream and noticed 4 successful MPD updates without interruption to the stream and without segfault before I cancelled the stream.
Comment 12 David Waring 2014-10-10 10:44:23 UTC
Created attachment 288202 [details] [review]
Fix MPD refresh

New version of the patch which also fixes gst_mpd_client_set_period_id and makes it actually set the period instead of just returning TRUE if it finds the period.
Comment 13 Thiago Sousa Santos 2014-12-02 01:36:01 UTC
can you retry with latest dashdemux from git to check if this is still a problem?
Comment 14 David Waring 2014-12-02 10:06:43 UTC
Created attachment 291973 [details] [review]
Fix period selection

Fix update manifest selecting the correct period to continue on.
Comment 15 David Waring 2014-12-02 10:14:38 UTC
My tests found that MPD update no longer causes segfault, but may pick the wrong period to continue on with. Patch included above for that and a logic error I noticed in the same routine.
Comment 16 Thiago Sousa Santos 2014-12-02 16:26:19 UTC
Thanks for the quick update

commit 306ca0cdf601fcce8a3d5908d9fcc515f6561b99
Author: David Waring <>
Date:   Tue Dec 2 10:06:00 2014 +0000

    dashdemux: Fix period selection for live streams
    Fix period selection and properly error out when update cannot be done