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 752492 - dashdemux: suggestedPresentationDelay should be positive
dashdemux: suggestedPresentationDelay should be positive
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.7.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Reported: 2015-07-16 14:16 UTC by Florin Apostol
Modified: 2015-10-28 15:51 UTC
See Also:
GNOME target: ---
GNOME version: ---

forbid negative values for duration (3.32 KB, patch)
2015-09-08 13:03 UTC, Vincent Penquerc'h
none Details | Review
forbid negative values for duration (3.34 KB, patch)
2015-09-08 13:30 UTC, Vincent Penquerc'h
none Details | Review
forbid negative values for duration (2.72 KB, patch)
2015-09-08 13:58 UTC, Vincent Penquerc'h
committed Details | Review

Description Florin Apostol 2015-07-16 14:16:05 UTC
suggestedPresentationDelay has a type "xs:duration". This means that a negative value is theoretically allowed. But in practice, it makes no sense.

The value is currently used only in gst_dash_demux_setup_streams function to search for the Period corresponding to now. If it is negative, that time will be in the future (the negated suggestedPresentationDelay  is added to now). The result is that gst_dash_demux_setup_streams function will fail.

If the parser will reject negative values for  suggestedPresentationDelay, we could pretend it wasn't provided and gst_dash_demux_setup_streams will be ok.

I can't imagine a valid case for a negative value for suggestedPresentationDelay. Is there one?
Comment 1 Vincent Penquerc'h 2015-09-08 12:49:39 UTC
Can a duration be negative ? I can't see where this is defined in the spec. I see "xd:duration" used, but not its definition. Even looking for the seldom used xs:dateTime doesn't find the place where those are defined.

In the absence of arguments to the contrary, I agree with your suggestion to ignore negative values.
Comment 2 Vincent Penquerc'h 2015-09-08 13:03:07 UTC
Created attachment 310898 [details] [review]
forbid negative values for duration

Here is a patch which warns/ignores negative values for duration as you suggested.
Comment 3 Florin Apostol 2015-09-08 13:23:47 UTC
Review of attachment 310898 [details] [review]:

::: ext/dash/gstmpdparser.c
@@ +947,3 @@
         sign * ((((((gint64) years * 365 + months * 30 + days) * 24 +
                     hours) * 60 + minutes) * 60 + seconds) * 1000 + decimals);
+    if (!allow_negative) {

Why do you need this? it should be removed. It will always issue warnings (including for for positive duration) if allow_negative = FALSE
Comment 4 Vincent Penquerc'h 2015-09-08 13:30:54 UTC
Created attachment 310900 [details] [review]
forbid negative values for duration

I was sure I had added this check, but evidently my brain went faster than my fingers, and did not run with GST_DEBUG=2. Fixed.
Comment 5 Florin Apostol 2015-09-08 13:37:11 UTC
you don't need that block at all. The only way for *property_value to be < 0 is to have the sign = -1, and you already have a warning for that. Your code from this if block is unreachable.
Comment 6 Vincent Penquerc'h 2015-09-08 13:42:04 UTC
While I've not actually tried it, this is a cheap way to ensure nothing in the code above can read negative numbers. "read" is a gint, sscanf reads %d and not %u. If you think this is impossible, and all possible - signs end up in the first test, I'll remove that second test.
Comment 7 Vincent Penquerc'h 2015-09-08 13:49:35 UTC
Reading the code some more, the first - check is actually outside the do loop, so I think you can have minus sign in the middle, which would be accepted (arguably a bug in itself). Not sure everything else should have ranges (ie, minutes between 0 and 59) or not, since it may make sense to have a "140 minutes" duration.
Comment 8 Florin Apostol 2015-09-08 13:54:24 UTC
the standard allows for "-" sign only at the beginning. The code reads it only once, and you have already added a warning for it. It is enough.

read, years, etc should actually be guint. A fix for this was proposed in
Comment 9 Vincent Penquerc'h 2015-09-08 13:58:29 UTC
Created attachment 310904 [details] [review]
forbid negative values for duration

OK, that test is now removed.
Comment 10 Florin Apostol 2015-09-08 14:06:05 UTC
Review of attachment 310904 [details] [review]:

looks ok for me.
Comment 11 Vincent Penquerc'h 2015-10-28 15:50:47 UTC
commit 54d93f597d541db8cf2f1b24dffc94e937f24a6d
Author: Vincent Penquerc'h <>
Date:   Tue Sep 8 14:00:54 2015 +0100

    mpdparser: forbid negative values for duration