GNOME Bugzilla – Bug 701404
dashdemux: should not buffer the entire Period
Last modified: 2013-10-03 12:25:13 UTC
Created attachment 245806 [details] [review] Proposed fix The current implementation of download_loop downloads every segment from the current Period. If a 2 hour movie is presented in the MPD as a single Period, the dashdemux will attempt to download and queue up buffers for the entire presentation. The demux should buffer at least the value of the "minBufferTime" MPD attribute. It must also respect the MAX_BUFFERING_TIME property of the element. Proposed fix attached.
Do you have a stream that has this scenario so we can test your patch?
Unfortunately, most of the streams I am testing with are DLNA compliance streams and are not legal to share. However, I will try to find a free-to-share stream that will properly demonstrate the problem and get back to you.
This is also an issue for live streams because download_loop will keep downloading segments until it gets a 404 error for a segment that has not yet been published. This is a problem because this request for a segment that doesn't exist will propagate all the way back to the origin server(s). This means that dashdemux causes extra load on the origin server(s) for segments that aren't yet available. Ideally download_loop would check before trying to download a segment that is in the future.
Review of attachment 245806 [details] [review]: ::: ext/dash/gstdashdemux.c @@ +1571,3 @@ + + /* Prevent this thread from constantly running */ + g_usleep (500000); This is definitely wrong, you probably want to use a gst_clock_id_wait() until the timeout expires, but be ready to be cancelled before in case of flush or of the element being stopped.
(In reply to comment #4) > Review of attachment 245806 [details] [review]: > > ::: ext/dash/gstdashdemux.c > @@ +1571,3 @@ > + > + /* Prevent this thread from constantly running */ > + g_usleep (500000); > > This is definitely wrong, you probably want to use a gst_clock_id_wait() until > the timeout expires, but be ready to be cancelled before in case of flush or of > the element being stopped. Understood. I think I will propose a totally different fix as part of a bug. I would like to stop using GstTask for the download_loop and just use a thread and a gst_clock_wait_id (). A new patch is incoming that has the sleep removed.
Created attachment 246653 [details] [review] Updated fix to remove sleep
Review of attachment 246653 [details] [review]: ::: ext/dash/gstdashdemux.c @@ +1567,2 @@ } else { + goto quit; This will never be reached, because you have: if (cancelled) {} else if (!cancelled) {} else {...} And doesn't quit just return, which means download_loop() will be called again immediately? Are you missing some bit ?
(In reply to comment #7) > This will never be reached, because you have: > if (cancelled) {} else if (!cancelled) {} else {...} > > And doesn't quit just return, which means download_loop() will be called again > immediately? Are you missing some bit ? This wasn't something I changed. It is there in the current master branch code. However, I agree that the final "else" statement is not required. I can add it to my current patch if you like.
Created attachment 246659 [details] [review] Additional patch to fix the fragment download error-handling This additional patch fixes up the error-handling code when downloading fragments to remove an unnecessary 'else' clause and to use positive logic when testing for 'canceled' state.
(In reply to comment #1) > Do you have a stream that has this scenario so we can test your patch? Here is a test stream that exhibits the problem: gst-launch-1.0 --gst-debug=dashdemux:5 playbin uri=http://www-itec.uni-klu.ac.at/ftp/datasets/mmsys12/BigBuckBunny/bunny_4s/BigBuckBunny_4s_isoffmain_DIS_23009_1_v_2_1c2_2011_08_30.mpd If you inspect the log after running this stream for about 3min, you will see two problems. First, the internal buffer of downloaded fragments grows large: 0:03:01.918608932 3775 0xb5b024f0 INFO dashdemux gstdashdemux.c:1566:gst_dash_demux_download_loop:<dashdemux0> Internal buffering : 76 s But the other problem is the stream_loop. The stream loop pushes buffers out the dashdemux src pad as fast as it can (as long as it has buffers in its internal queue to push). So, initially, the stream loop can keep up with the download_loop because it is just filling downstream queue(s). Once those downstream queues are full, the stream loop blocks trying to push buffers. This causes the internal queue of downloaded buffers to continue growing since the download loop never stops adding them. This is bad for another reason. With the downstream queues completely full (in my case, it was around 30 seconds of media queued up), it takes that long for a representation switch to appear to the user. So, it seems like there might be two values we want to control: 1) How much media we want to download into our internal queue of segments. This makes us resilient against fluctuations in network bandwidth. 2) How fast we want to push media buffers downstream on our src pad. The slower this value (closer to actual presentation time), the quicker the user will see Representation changes. Thoughts?
This is again something that was lost when dashdemux original code (https://github.com/Orange-OpenSource/gstdashdemux/tree/1.0) was merged upstream: we used to push only one fragment in advance to be able to switch bitrates as soon as possible.
Created attachment 247331 [details] [review] stop gstdashdemux fetching live fragments that don't yet exist This patch uses availabilityStartTime, period@Start and suggestedPresentationDelay and the host's idea of UTC to decide if a fragment is available to be requested from an HTTP server and filter out requests for fragments that are not yet available. This patch was created on top of Greg Rutz's patches. However this patch touches different parts of the code from Greg's changes and can be dealt with independently if required.
Created attachment 248348 [details] [review] dashdemux: implement queue full check function How about this patch to make the queue never go above the max buffer duration? It seems much simpler and GstDataQueue already handles flushing/blocking for us.
Review of attachment 246653 [details] [review]: This patch misses some kind of condition waiting to avoid looping continuously knowing that there isn't room for buffering ::: ext/dash/gstdashdemux.c @@ +1533,3 @@ + buffered_duration = gst_dash_demux_get_buffering_time (demux); + while ((GST_TIME_AS_MSECONDS (buffered_duration) < minBufferTime) && + buffered_duration <= demux->max_buffering_time) { The task is called repeatedly anyway, not sure we want a while here without any cancel checks. And why shouldn't it be: while ((GST_TIME_AS_MSECONDS (buffered_duration) < minBufferTime) || buffered_duration <= demux->max_buffering_time) ? After all this is a "minimum buffering time".
(In reply to comment #14) > This patch misses some kind of condition waiting to avoid looping continuously > knowing that there isn't room for buffering > > ::: ext/dash/gstdashdemux.c > @@ +1533,3 @@ > + buffered_duration = gst_dash_demux_get_buffering_time (demux); > + while ((GST_TIME_AS_MSECONDS (buffered_duration) < minBufferTime) && > + buffered_duration <= demux->max_buffering_time) { > > The task is called repeatedly anyway, not sure we want a while here without any > cancel checks. I agree. I'm not sure we should have a while here. I am working on this as part of bug 700423. I think the function should download one fragment for each stream and then return. There should be other logic that determines whether the GstTask is paused or remains running. > And why shouldn't it be: > > while ((GST_TIME_AS_MSECONDS (buffered_duration) < minBufferTime) || > buffered_duration <= demux->max_buffering_time) ? > > After all this is a "minimum buffering time". I think we need to agree on (and document) exactly how the logic will determine how much is buffered because that determines how quickly representations changes will be seen by the viewer. Maybe this: 1) If max_buffering_time is set: a) If max_buffering_time is greater than minBufferTime, buffer max_buffering time b) If max_buffering_time is less than or equal to minBufferTime, buffer minBufferTime 2) If max_buffering_time is not set, buffer minBufferTime min_buffering_time should always be respected. The value should be smartly populated by the content generator to ensure data is always available based on the representation bandwidth. I also think that users should strive to have this value as small as possible (which is why minBufferTime is used if max_buffering_time is not set). If the user wants to override this value (and thus make representation switches slower) they can set max_buffering_time.
I don't see much an advantage on buffering less than max_buffering_time, switches are faster the smaller the buffer but they can also switch to worse as much as it can switch to better. Why switching earlier is a good thing if we don't know how will it switch? Also we aren't checking how much is buffered downstream on this case. If we are going to restrict to minBufferingTime I'd suggest changing max_buffering_time to 0 by default and use it as the top limit in case the user specifies to do so. Check out the patch I attached, it already cares for the conditions and locking as GstDataQueue already has it internally.
Review of attachment 248348 [details] [review]: ::: ext/dash/gstdashdemux.c @@ +705,3 @@ stream->need_header = TRUE; stream->has_data_queued = FALSE; + stream->demux = demux; Since the GstDashDemuxStream is not actually needed by the check_full callback, we should just pass the GstDashDemux as the callback data instead.
Good catch. And I just re-read the spec. The 'minBufferTime' should be used to start up the client, it should buffer 'minBufferTime' data before starting playback, it shouldn't be used as a higher limit to buffering.
Created attachment 248687 [details] [review] dashdemux: implement queue full check function Simpler queue check function, only checks for the max buffering time as it seems minBufferTime shouldn't be used for this
Created attachment 248688 [details] [review] dashdemux: implement queue full check function Implement the queue size checking in a simple way. According to the spec, minBufferTime shouldn't be used as a maximum buffering limit.
Review of attachment 247331 [details] [review]: Some suggestions below. ::: ext/dash/gstdashdemux.c @@ +1897,3 @@ + gint64 diff = + gst_mpd_client_calculate_time_difference (cur_time, seg_end_time) + / GST_MSECOND; For this case, we can just use GDateTime and use the g_date_time_difference function instead of adding our own. @@ +1899,3 @@ + / GST_MSECOND; + if (-1 != demux->client->mpd_node->suggestedPresentationDelay) { + diff += demux->client->mpd_node->suggestedPresentationDelay; We shouldn't prevent that dashdemux downloads the segment if it is ready, instead we want to download as soon it is available and use the suggestedPresentationDelay do delay playback, not download. I'm still unsure of the best way of using suggestedPresentationDelay, maybe it should be handled in another patch as it seems a different issue. @@ +1908,3 @@ + "), delaying download", diff); + end_of_period = FALSE; + selected_stream = NULL; There is now a gst_dash_demux_download_wait function that puts the thread to wait for a specified amount of time or until the demuxer is stopped/reset ::: ext/dash/gstmpdparser.c @@ +3819,3 @@ + } + + offset = (1 + seg_idx) * seg_duration; This is only true when duration are all the same. In a SegmentList scenario the durations might be different. @@ +3824,3 @@ + gst_date_time_unref (availability_start_time); + return rv; +} Maybe we want to make the get_next_fragment_timestamp function also return the duration? And then dashdemux can sum timestamp+duration and get the end time to be mapped to UTC time instead of re-doing some of this work here.
commit 42fd04ce48a356d92fe373ac53185211839b1318 Author: Alex Ashley <bugzilla@ashley-family.net> Date: Thu Sep 26 16:13:33 2013 -0300 dashdemux: stop fetching live fragments that don't yet exist There is an issue for live streams where download_loop will keep downloading segments until it gets a 404 error for a segment that has not yet been published. This is a problem because this request for a segment that doesn't exist will propagate all the way back to the origin server(s). This means that dashdemux causes extra load on the origin server(s) for segments that aren't yet available. This patch uses availabilityStartTime, period and the host's idea of UTC to decide if a fragment is available to be requested from an HTTP server and filter out requests for fragments that are not yet available. https://bugzilla.gnome.org/show_bug.cgi?id=701404