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 752480 - dashdemux: negative values for r attribute in S node are not supported
dashdemux: negative values for r attribute in S node are not supported
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.5.91
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 756851 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2015-07-16 12:21 UTC by Florin Apostol
Modified: 2016-02-25 13:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
read signed r values for S elements (3.26 KB, patch)
2015-09-08 14:17 UTC, Vincent Penquerc'h
none Details | Review
read signed r values for S elements (3.26 KB, patch)
2015-09-09 10:08 UTC, Vincent Penquerc'h
committed Details | Review
catch failures to parse (2.70 KB, patch)
2015-09-09 10:08 UTC, Vincent Penquerc'h
committed Details | Review
untested support for negative repeat field (5.28 KB, patch)
2015-09-09 13:55 UTC, Vincent Penquerc'h
needs-work Details | Review
mpdparser: Consider the repeat count in get_segments_count() (1.93 KB, patch)
2015-09-14 16:48 UTC, Sebastian Dröge (slomo)
rejected Details | Review
mpdparser: Consider the repeat count when checking if a segment is the last one (1.39 KB, patch)
2015-09-14 17:03 UTC, Sebastian Dröge (slomo)
committed Details | Review
untested support for negative repeat field (6.30 KB, patch)
2015-09-15 13:22 UTC, Vincent Penquerc'h
none Details | Review
support for negative repeat field (7.50 KB, patch)
2015-09-15 15:43 UTC, Vincent Penquerc'h
none Details | Review
support for negative repeat field (7.31 KB, patch)
2015-09-15 17:23 UTC, Vincent Penquerc'h
none Details | Review
support for negative repeat field (7.29 KB, patch)
2015-09-15 17:31 UTC, Vincent Penquerc'h
committed Details | Review

Description Florin Apostol 2015-07-16 12:21:14 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.
Comment 1 Vincent Penquerc'h 2015-09-08 14:17:35 UTC
Created attachment 310905 [details] [review]
read signed r values for S elements

As suggested.
Comment 2 Thiago Sousa Santos 2015-09-08 19:30:44 UTC
Review of attachment 310905 [details] [review]:

Looks better than what we have.
Comment 3 Vincent Penquerc'h 2015-09-09 08:51:33 UTC
Does anyone interested in this have a sample with that feature ?
Comment 4 Florin Apostol 2015-09-09 09:46:35 UTC
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
Comment 5 Vincent Penquerc'h 2015-09-09 10:08:13 UTC
Created attachment 310961 [details] [review]
read signed r values for S elements

Thanks, fixed.
Comment 6 Vincent Penquerc'h 2015-09-09 10:08:44 UTC
Created attachment 310962 [details] [review]
catch failures to parse
Comment 7 Florin Apostol 2015-09-09 10:46:09 UTC
Review of attachment 310962 [details] [review]:

looks ok
Comment 8 Florin Apostol 2015-09-09 10:47:43 UTC
Review of attachment 310961 [details] [review]:

looks ok
Comment 9 Vincent Penquerc'h 2015-09-09 13:55:37 UTC
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.
Comment 10 Sebastian Dröge (slomo) 2015-09-14 16:24:31 UTC
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
Comment 11 Sebastian Dröge (slomo) 2015-09-14 16:25:45 UTC
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.
Comment 12 Sebastian Dröge (slomo) 2015-09-14 16:48:12 UTC
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.
Comment 13 Sebastian Dröge (slomo) 2015-09-14 16:50:51 UTC
(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.
Comment 14 Sebastian Dröge (slomo) 2015-09-14 17:03:54 UTC
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.
Comment 15 Sebastian Dröge (slomo) 2015-09-14 17:04:42 UTC
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?
Comment 16 Sebastian Dröge (slomo) 2015-09-14 17:20:59 UTC
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.
Comment 17 Thiago Sousa Santos 2015-09-14 17:39:27 UTC
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?
Comment 18 Vincent Penquerc'h 2015-09-15 13:22:05 UTC
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.
Comment 19 Vincent Penquerc'h 2015-09-15 13:23:17 UTC
(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.
Comment 20 Sebastian Dröge (slomo) 2015-09-15 15:02:19 UTC
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
Comment 21 Vincent Penquerc'h 2015-09-15 15:43:32 UTC
Created attachment 311385 [details] [review]
support for negative repeat field

Here's a, er, repeat.
Comment 22 Sebastian Dröge (slomo) 2015-09-15 16:40:03 UTC
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?
Comment 23 Vincent Penquerc'h 2015-09-15 16:55:33 UTC
(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.
Comment 24 Vincent Penquerc'h 2015-09-15 17:07:38 UTC
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.
Comment 25 Sebastian Dröge (slomo) 2015-09-15 17:12:41 UTC
(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 :)
Comment 26 Sebastian Dröge (slomo) 2015-09-15 17:13:32 UTC
(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
Comment 27 Vincent Penquerc'h 2015-09-15 17:19:46 UTC
(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.
Comment 28 Vincent Penquerc'h 2015-09-15 17:23:22 UTC
Created attachment 311391 [details] [review]
support for negative repeat field
Comment 29 Sebastian Dröge (slomo) 2015-09-15 17:25:03 UTC
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 :)
Comment 30 Vincent Penquerc'h 2015-09-15 17:31:48 UTC
Created attachment 311394 [details] [review]
support for negative repeat field

Indeed, this now considers one or the other, depending on whether last or not.
Comment 31 Sebastian Dröge (slomo) 2015-09-16 07:28:52 UTC
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.
Comment 32 Vincent Penquerc'h 2015-09-16 08:50:46 UTC
(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
Comment 33 Rajat Verma 2015-10-20 11:17:18 UTC
*** Bug 756851 has been marked as a duplicate of this bug. ***