GNOME Bugzilla – Bug 777206
dashdemux: mosaic issue for MPEG DASH live streaming due to not updating headers
Last modified: 2018-11-03 14:03:18 UTC
Created attachment 343415 [details] issue description in detail as https://bugzilla.gnome.org/show_bug.cgi?id=767682 original test URL is invalid but I found a one which can reproduce the issue. Please refer to the attached in detail, or by the link below: http://programmingmemojohnchang.blogspot.tw/2017/01/potential-race-condition-issue-of.html I tested it for 1.4.5 (a little old) but I wonder it will happen for master also. Please help to check it. [Test URL & settings]: http://media-dcp.otvs.tv/storage/tears-of-steel/dash/live.main.isml/.mpd [Issue]: Wrong codec settings leads to mosaic (ex, use the codec setting of high bit-rate representation to decode data of low bit-rate representation ). [Root cause]: 1. Race condition between threads. 2. Slow start of gst_mpd_client_setup_streaming() when updating manifest.
Created attachment 343416 [details] [review] proposed patch proposed patch
Review of attachment 343416 [details] [review]: Generally makes sense but... ::: ext/dash/gstdashdemux.c @@ +1156,3 @@ + if (!GST_ADAPTIVE_DEMUX_STREAM_NEED_HEADER (stream)) { + if (dashstream->active_stream && dashstream->active_stream->cur_representation) { + if (dashstream->active_stream->cur_representation->bandwidth != stream->last_bandwidth) { This should be done not only when the bitrate changed but whenever the stream (representation) was changed
Created attachment 343465 [details] [review] modified patch Dear Sebastian: As your suggestions, the new patch adopts representation's id for the judgement. According to the description of ISO-23009-1 to representation's: "specifies an identifier for this Representation. The identifier shall be unique within a Period unless the Representation is functionally identically to another Representation in the same Period." Also, representation's id is a "mandatory" attribute so it makes us uniquely identify a specific representation. Please help to estimate the fix. Thanks!
Review of attachment 343465 [details] [review]: ::: ext/dash/gstdashdemux.c @@ +1155,3 @@ + if (gst_mpd_client_is_live(dashdemux->client)) { + if (!GST_ADAPTIVE_DEMUX_STREAM_NEED_HEADER (stream)) { + if (dashstream->active_stream && dashstream->active_stream->cur_representation) { While this probably works... don't we explictly switch the representation somewhere in this code and could just set a boolean for that which is checked here and then reset again after handling it? ::: gst-libs/gst/adaptivedemux/gstadaptivedemux.h @@ +187,3 @@ gboolean eos; + + gchar *last_representation_id; It seems like this should be in the DASH subclass instead. Representation is something DASH specific
Created attachment 343516 [details] [review] Fix the issue when updating thread sets slow start without passing necessary header & caps down. Dear Sebastian: As your suggestions; I have put "last_representation_id" under GstDashDemuxStream instead of the common part "GstAdaptiveDemuxStream" As the other part, I am not so sure about your comments. For this issue, the race condition may NOT merely happen in this case. As an example, please check the scenario below: 1. Update loop thread gets the manifest_lock & selects slow start. = 125000 bits/sec(originaly it is at 250000 bits/sec). 2. The src thread calculates the estimated bitrate & the result is as original = 250000. To this point, if we could execute the flow: if (ret == GST_FLOW_OK) { if (gst_adaptive_demux_stream_select_bitrate (demux, stream, gst_adaptive_demux_stream_update_current_bitrate (demux, stream))) { stream->need_header = TRUE; ret = (GstFlowReturn) GST_ADAPTIVE_DEMUX_FLOW_SWITCH; } ... Everything will be fine. However, the ret may not always be GST_FLOW_OK but sometimes is GST_FLOW_EOS. Hence the calculated bit-rate = 250000 will NOT be set (since gst_adaptive_demux_stream_select_bitrate() is skipped). In fact, there are several conditions involved when this issue hapeens. Also, the timing to set new caps should be taken into consideration. Notice that at gst_adaptive_demux_stream_push_buffer(): ... if (G_UNLIKELY (stream->pending_segment)) { GST_ADAPTIVE_DEMUX_SEGMENT_LOCK (demux); pending_segment = stream->pending_segment; stream->pending_segment = NULL; GST_ADAPTIVE_DEMUX_SEGMENT_UNLOCK (demux); } ... It passes the pending caps immediately when we are about to push a buffer down. However, the update loop thread may NOT always set the pending caps (be executed) on the beginning (at the 1st buffer) of a new segment (fragment). Hence the new caps may locate within the buffers of a segment instead of at the beginning as required. Since the caps event is expected to be serialized, we should avoid this case. As the result, I put the check for header & caps when live streaming within gst_dash_demux_stream_update_fragment_info. It is the place when we finally decide which fragment (representation of a specific bit-rate) will be downloaded. So no matter how the execution sequence of mentioned threads changes, we could make sure the correctness. Could you please give a more detailed description about your first comments? Thank you.
Created attachment 343518 [details] [review] Fix the issue when updating thread sets slow start without passing necessary header & caps down. Dear Sebastian: As your suggestions; I have put "last_representation_id" under GstDashDemuxStream instead of the common part "GstAdaptiveDemuxStream" As the other part, I am not so sure about your comments. For this issue, the race condition may NOT merely happen in this case. As an example, please check the scenario below: 1. Update loop thread gets the manifest_lock & selects slow start. = 125000 bits/sec(originaly it is at 250000 bits/sec). 2. The src thread calculates the estimated bitrate & the result is as original = 250000. To this point, if we could execute the flow: if (ret == GST_FLOW_OK) { if (gst_adaptive_demux_stream_select_bitrate (demux, stream, gst_adaptive_demux_stream_update_current_bitrate (demux, stream))) { stream->need_header = TRUE; ret = (GstFlowReturn) GST_ADAPTIVE_DEMUX_FLOW_SWITCH; } ... Everything will be fine. However, the ret may not always be GST_FLOW_OK but sometimes is GST_FLOW_EOS. Hence the calculated bit-rate = 250000 will NOT be set (since gst_adaptive_demux_stream_select_bitrate() is skipped). In fact, there are several conditions involved when this issue hapeens. Also, the timing to set new caps should be taken into consideration. Notice that at gst_adaptive_demux_stream_push_buffer(): ... if (G_UNLIKELY (stream->pending_segment)) { GST_ADAPTIVE_DEMUX_SEGMENT_LOCK (demux); pending_segment = stream->pending_segment; stream->pending_segment = NULL; GST_ADAPTIVE_DEMUX_SEGMENT_UNLOCK (demux); } ... It passes the pending caps immediately when we are about to push a buffer down. However, the update loop thread may NOT always set the pending caps (be executed) on the beginning (at the 1st buffer) of a new segment (fragment). Hence the new caps may locate within the buffers of a segment instead of at the beginning as required. Since the caps event is expected to be serialized, we should avoid this case. As the result, I put the check for header & caps when live streaming within gst_dash_demux_stream_update_fragment_info. It is the place when we finally decide which fragment (representation of a specific bit-rate) will be downloaded. So no matter how the execution sequence of mentioned threads changes, we could make sure the correctness. Could you please give a more detailed description about your first comments? Thank you.
Review of attachment 343518 [details] [review]: ::: ext/dash/gstdashdemux.c @@ +1154,3 @@ if (gst_mpd_client_get_next_fragment_timestamp (dashdemux->client, dashstream->index, &ts)) { + if (gst_mpd_client_is_live(dashdemux->client)) { Why only for live streams? Should be the same for VOD streams @@ +1160,3 @@ + if (!g_strcmp0 (dashstream->active_stream->cur_representation->id, dashstream->last_representation_id)) { + stream->need_header = TRUE; + GstCaps *caps; Put the variable declarations before any code. @@ +1164,3 @@ + dashstream->active_stream->cur_representation->bandwidth); + caps = gst_dash_demux_get_input_caps (dashdemux, dashstream->active_stream); + gst_adaptive_demux_stream_set_caps (stream, caps); You're probably leaking the caps here @@ +1169,3 @@ + } + if (dashstream->last_representation_id) + g_free (dashstream->last_representation_id); g_free() is NULL-safe, see below @@ +2752,3 @@ + if (dash_stream->last_representation_id) { + g_free (dash_stream->last_representation_id); g_free() is NULL-safe, you don't need to check for NULL first
Thanks, I think that makes sense then
Created attachment 343607 [details] [review] Fix the issue when updating thread sets slow start without passing necessary header & caps down Dear Sebastian: ::: ext/dash/gstdashdemux.c @@ +1154,3 @@ if (gst_mpd_client_get_next_fragment_timestamp (dashdemux->client, dashstream->index, &ts)) { + if (gst_mpd_client_is_live(dashdemux->client)) { Why only for live streams? Should be the same for VOD streams? => it is because the issue only happens when we have an update loop thread which may set representation as slow start. For VOD, since there are only download loop thread & source thread there, it has no problem. As the result, I make this check to avoid redundant actions for VOD. @@ +1164,3 @@ + dashstream->active_stream->cur_representation->bandwidth); + caps = gst_dash_demux_get_input_caps (dashdemux, dashstream->active_stream); + gst_adaptive_demux_stream_set_caps (stream, caps); You're probably leaking the caps here => according to the latest version of gst_adaptive_demux_stream_set_caps() /* must be called with manifest_lock taken */ void gst_adaptive_demux_stream_set_caps (GstAdaptiveDemuxStream * stream, GstCaps * caps) { GST_DEBUG_OBJECT (stream->pad, "setting new caps for stream %" GST_PTR_FORMAT, caps); gst_caps_replace (&stream->pending_caps, caps); gst_caps_unref (caps); } It unrefs the caps at the end. Also, from the usage case within gst_dash_demux_stream_select_bitrate() ... if (gst_mpd_client_setup_representation (demux->client, active_stream, rep)) { GstCaps *caps; GST_INFO_OBJECT (demux, "Switching bitrate to %d", active_stream->cur_representation->bandwidth); caps = gst_dash_demux_get_input_caps (demux, active_stream); gst_adaptive_demux_stream_set_caps (stream, caps); ret = TRUE; } else { GST_WARNING_OBJECT (demux, "Can not switch representation, aborting..."); } ... It seems to pass the caps and the caps will be unreffered within gst_adaptive_demux_stream_set_caps(). Thanks a lot for your suggestion. John Chang
(In reply to John Chang from comment #9) > Created attachment 343607 [details] [review] [review] > Fix the issue when updating thread sets slow start without passing necessary > header & caps down > > Dear Sebastian: > > ::: ext/dash/gstdashdemux.c > @@ +1154,3 @@ > if (gst_mpd_client_get_next_fragment_timestamp (dashdemux->client, > dashstream->index, &ts)) { > + if (gst_mpd_client_is_live(dashdemux->client)) { > > Why only for live streams? Should be the same for VOD streams? > > => it is because the issue only happens when we have an update loop thread > which may set representation as slow start. For VOD, since there are only > download loop thread & source thread there, it has no problem. > As the result, I make this check to avoid redundant actions for VOD. So the problem here is only if the manifest updates in a way that requires to get headers again, without doing any explicit representation change inside dashdemux? And whenever we explicitly switch, everything's good already?
(In reply to Sebastian Dröge (slomo) from comment #10) > (In reply to John Chang from comment #9) > > So the problem here is only if the manifest updates in a way that requires > to get headers again, without doing any explicit representation change > inside dashdemux? > And whenever we explicitly switch, everything's good already? Dear Sebastian: Yes, it is. The manifest update thread may have problem by missing doing explicit representation change currently. As within gst_mpd_client_setup_streaming () by the logic in /* slow start */, it may switch representation without informing the change. The other way to check is by checking the variable : "need_header" (GST_ADAPTIVE_DEMUX_STREAM_NEED_HEADER), it is not set for updating manifest when slow start makes us switch to the lowest representation. Thank you. John
commit c9fbf3459a719b2c68ba69ddabd373ea9bf804a2 Author: WeiChungChang <r97922153@gmail.com> Date: Tue Jan 17 10:33:03 2017 +0800 dashdemux: Fix issue when manifest update sets slow start without passing necessary header & caps changes downstream https://bugzilla.gnome.org/show_bug.cgi?id=777206
Review of attachment 343607 [details] [review]: ::: ext/dash/gstdashdemux.c @@ +1158,3 @@ + if (dashstream->active_stream && dashstream->active_stream->cur_representation) { + /* id specifies an identifier for this Representation. The identifier shall be unique within a Period unless the Representation is functionally identically to another Representation in the same Period.*/ + if (!g_strcmp0 (dashstream->active_stream->cur_representation->id, dashstream->last_representation_id)) { This actually looks wrong. It will go into this if-branch only if the two representation IDs are the same (i.e. always), while it should be the other way around (if the representation changed). Reverting for now
*** Bug 767682 has been marked as a duplicate of this bug. ***
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/507.