GNOME Bugzilla – Bug 752480
dashdemux: negative values for r attribute in S node are not supported
Last modified: 2016-02-25 13:30:01 UTC
According to the standard, the r attribute in the S node can have negative values: "A negative value of the @r attribute of the S element indicates that the duration indicated in @d attribute repeats until the start of the next S element, the end of the Period or until the next MPD update." The parser will read the @r attribute as unsigned integer and store it in an unsigned integer variable. This will lead to wrong behavior if a negative value is present in the xml. The parser should at least read it as a signed integer and issue a warning that the feature is not supported.
Created attachment 310905 [details] [review] read signed r values for S elements As suggested.
Review of attachment 310905 [details] [review]: Looks better than what we have.
Does anyone interested in this have a sample with that feature ?
Review of attachment 310905 [details] [review]: ::: ext/dash/gstmpdparser.c @@ +297,3 @@ + prop_string = xmlGetProp (a_node, (const xmlChar *) property_name); + if (prop_string) { + gboolean exists = FALSE; sscanf returns EOF (-1) in case of an "input failure before any data could be successfully interpreted". For example, for strings containing spaces, or empty strings, sscanf returns -1. You consider that a valid data. You should check that the return code from sscanf is 1, not !0
Created attachment 310961 [details] [review] read signed r values for S elements Thanks, fixed.
Created attachment 310962 [details] [review] catch failures to parse
Review of attachment 310962 [details] [review]: looks ok
Review of attachment 310961 [details] [review]: looks ok
Created attachment 310981 [details] [review] untested support for negative repeat field This adds an implementation for negative repeat, though I have no sample to test it with, so it's likely buggy. I'm not familiar with the DASH format, so there is also a non negligible likelihood of confusion between segments, periods, streams etc. If anyone has a shareable sample with negative repeats, that'd be good.
Review of attachment 310981 [details] [review]: ::: ext/dash/gstmpdparser.c @@ +3765,3 @@ + } else { + // 5.3.9.6.1: negative repeat means repeat till the end of the + // period, or the next update of the MPD (which I think is Actually it does not say that :) It repeats until the next S@t of the next S node, or if there is no next S node it repeats until the end of the period or the next update of the MPD. You're missing the check for the next S node, also in the other places @@ +3766,3 @@ + // 5.3.9.6.1: negative repeat means repeat till the end of the + // period, or the next update of the MPD (which I think is + // implicit, as this will all get deleted/recreated). No C++/C99 comments @@ +4148,3 @@ + // negative repeats only seem to make sense at the end of a list, + // so this one will probably not be. Needs some sanity checking + // when loading the XML data. No C++/C99 comments @@ +4150,3 @@ + // when loading the XML data. + if (segment->repeat < 0) { + GST_WARNING ("Negative repeat found on a non-end segment"); On a non-end segment, this means it repeats until the next S node's S@t
Here you can find a sample stream: http://dash.edgesuite.net/dash264/TestCases/1c/qualcomm/1/MultiRate.mpd There are more in the DASH-IF test vectors. Your patch does not make it work, it still goes EOS after the first segment instead of repeating it many, many times.
Created attachment 311295 [details] [review] mpdparser: Consider the repeat count in get_segments_count() Otherwise we will EOS before all repeats of a segment are handled.
(In reply to Sebastian Dröge (slomo) from comment #12) > Created attachment 311295 [details] [review] [review] > mpdparser: Consider the repeat count in get_segments_count() > > Otherwise we will EOS before all repeats of a segment are handled. This is not entirely correct... as in some places the segments_count() is used for the length of the array, and in some places for the number of segments.
Created attachment 311296 [details] [review] mpdparser: Consider the repeat count when checking if a segment is the last one Otherwise we play only the first repetition of the last segment and then EOS.
So with this, the above stream plays. Meaning, just the handling of the end of a r=-1 segment if there is a follow-up segment is missing in Vincent's patch. Can you update your patch?
There are also a few places everywhere that need checks for the r==-1, like gst_mpd_client_has_next_segment() (after my patch). But also I saw a few other places that need to be checked.
Review of attachment 310981 [details] [review]: ::: ext/dash/gstmpdparser.c @@ +4116,3 @@ + break; + } + } Isn't the start of the segment already in its struct? Why do you need to loop over to search here?
Created attachment 311366 [details] [review] untested support for negative repeat field The sample linked above plays, but I get a 404 every so often on segment files, not always at the same place. I don't seem to have another player that plays those to test against.
(In reply to Thiago Sousa Santos from comment #17) > Review of attachment 310981 [details] [review] [review]: > > ::: ext/dash/gstmpdparser.c > @@ +4116,3 @@ > + break; > + } > + } > > Isn't the start of the segment already in its struct? Why do you need to > loop over to search here? Indeed, fixed.
Review of attachment 311366 [details] [review]: ::: ext/dash/gstmpdparser.c @@ +3108,3 @@ + GstClockTime start, end; + + stream_period = gst_mpdparser_get_stream_period (client); That's not complete yet. You also have to consider g_ptr_array_index(segments, i+1)->start if existing. What you can have is <S t=0 d=10 r=-1 /> <S t=50 d=5 r=0 /> This would repeat the first segment 5 times, then do the last segment for 5 @@ +3657,3 @@ + || (ts < + segment->start + (segment->repeat + + 1) * segment->duration))) { Here you still assume that the r<0 segments are extending until the end of the period @@ +3783,3 @@ + const GstMediaSegment *next_segment = + g_ptr_array_index (stream->segments, segment_idx + 1); + *ts = next_segment->start; Here you do it correct @@ +4137,3 @@ + GstClockTime start, end; + + stream_period = gst_mpdparser_get_stream_period (client); Need to consider the next segment here too @@ +4178,3 @@ + GstClockTime start, end; + + stream_period = gst_mpdparser_get_stream_period (client); and here
Created attachment 311385 [details] [review] support for negative repeat field Here's a, er, repeat.
Review of attachment 311385 [details] [review]: ::: ext/dash/gstmpdparser.c @@ +3094,3 @@ +static GstClockTime +gst_mpdparser_get_end (GstMpdClient * client, GPtrArray * segments, + const GstMediaSegment * segment, gint index) get_end() is a weird name :) What about get_segment_end_time()? @@ +3128,3 @@ + GstClockTime start = s->start; + GstClockTime end = gst_mpdparser_get_end (client, segments, s, i); + repeat = (guint) (end - start) / s->duration; This might give one repeat too few in case of rounding errors. Should probably be rounded up? @@ +3683,3 @@ + if (in_segment) { + selectedChunk = segment; + repeat_index = (ts - segment->start) / segment->duration; Round up? @@ +3804,3 @@ + * implicit, as this will all get deleted/recreated), or the + * start of the next segment, if any. */ + if (segment_idx < stream->segments->len - 1) { Hm, this function is "get_last_fragment_timestamp_end". I assume there can never ever be another segment then and we always return the period end? @@ +4164,3 @@ + stream->segment_index); + stream->segment_repeat_index = + (guint) (end - start) / segment->duration; Round up? @@ +4203,3 @@ + stream->segment_index); + stream->segment_repeat_index = + (guint) (end - start) / segment->duration; Round up?
(In reply to Sebastian Dröge (slomo) from comment #22) > @@ +3804,3 @@ > + * implicit, as this will all get deleted/recreated), or the > + * start of the next segment, if any. */ > + if (segment_idx < stream->segments->len - 1) { > > Hm, this function is "get_last_fragment_timestamp_end". I assume there can > never ever be another segment then and we always return the period end? Good point, and while looking at the value segment_idx can have, it's not necessarily based on segments->len (gst_mpd_client_get_segments_counts). Seems like this format can have "implicit" segments, which I guess will have to be accounted for too somehow.
For the round up, I don't think this is right. It should round down. For instance, for a duration of 10, start of 2, end of 8, (8-2)/10 == 0. That's a segment repeated 0 times. In the case we're looking at which segment repeat index we're in, it's the same calc, and 0 based indexing. Unless I'm missing your point.
(In reply to Vincent Penquerc'h from comment #24) > For the round up, I don't think this is right. It should round down. > > For instance, for a duration of 10, start of 2, end of 8, (8-2)/10 == 0. > That's a segment repeated 0 times. > In the case we're looking at which segment repeat index we're in, it's the > same calc, and 0 based indexing. You're right, we're actually rounding up already because the repeat variable is zero based. Nevermind :)
(In reply to Vincent Penquerc'h from comment #23) > (In reply to Sebastian Dröge (slomo) from comment #22) > > @@ +3804,3 @@ > > + * implicit, as this will all get deleted/recreated), or the > > + * start of the next segment, if any. */ > > + if (segment_idx < stream->segments->len - 1) { > > > > Hm, this function is "get_last_fragment_timestamp_end". I assume there can > > never ever be another segment then and we always return the period end? > > Good point, and while looking at the value segment_idx can have, it's not > necessarily based on segments->len (gst_mpd_client_get_segments_counts). > Seems like this format can have "implicit" segments, which I guess will have > to be accounted for too somehow. With implicit segments you mean when ->segments is NULL? Those have no repeat by definition
(In reply to Sebastian Dröge (slomo) from comment #26) > With implicit segments you mean when ->segments is NULL? Those have no > repeat by definition Yes, that's what I meant. And, good, that's a relief :) I'll post an updated patch soon.
Created attachment 311391 [details] [review] support for negative repeat field
Review of attachment 311391 [details] [review]: Looks good ::: ext/dash/gstmpdparser.c @@ +3107,3 @@ + const GstMediaSegment *next_segment = + g_ptr_array_index (segments, index + 1); + if (next_segment->start < end) I think if there is another segment and its start is not earlier than the period end, something is wrong :)
Created attachment 311394 [details] [review] support for negative repeat field Indeed, this now considers one or the other, depending on whether last or not.
Vincent, as discussed with Thiago on IRC yesterday. Please merge your changes now :) They'll increase coverage of the DASH-IF test vectors quite a bit and have relatively low risk of breaking anything that wasn't broken before.
(In reply to Sebastian Dröge (slomo) from comment #31) > Vincent, as discussed with Thiago on IRC yesterday. Please merge your > changes now :) They'll increase coverage of the DASH-IF test vectors quite a > bit and have relatively low risk of breaking anything that wasn't broken > before. OK. Thanks for the reviews :) commit 50400fa2a660839c672b222094385018995aede7 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Wed Sep 9 14:49:17 2015 +0100 mpdparser: support for negative repeat count in segments Implements negative repeat segment fields, defined in 5.3.9.6.1. commit 69bfa8222f59096f9ab39cc85274c9b0573cb61d Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Tue Sep 8 15:14:13 2015 +0100 mpdparser: properly read signed r values for S elements The spec defines these as signed in 5.3.9.6.1. Since we don't support this behavior, warn and default to 0 (non repeating), which is the spec's default when the value is not present. https://bugzilla.gnome.org/show_bug.cgi?id=752480 commit 23ea8ccb43b0cca96275545ad5cfc22305d1705d Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Wed Sep 9 11:05:35 2015 +0100 mdpparser: catch failures to parse https://bugzilla.gnome.org/show_bug.cgi?id=752480
*** Bug 756851 has been marked as a duplicate of this bug. ***