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.

Thanks for reporting this, do you have a URL of a public live stream so we can test/debug this issue?

And can you reproduce this reliably?

(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:

+ Trace 233247

[...]

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.

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?

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)

Thanks for looking at this. Do you have a link to a public dash live stream so others can test this, too?

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.

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.

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 http://rdmedia.bbc.co.uk/dash/testmpds/multiperiod/bbb.php 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.

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.

can you retry with latest dashdemux from git to check if this is still a problem?

Created attachment 291973 [details] [review]
Fix period selection

Fix update manifest selecting the correct period to continue on.

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.

Thanks for the quick update

commit 306ca0cdf601fcce8a3d5908d9fcc515f6561b99
Author: David Waring <david.waring@rd.bbc.co.uk>
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
    
    https://bugzilla.gnome.org/show_bug.cgi?id=725298