GNOME Bugzilla – Bug 752492
dashdemux: suggestedPresentationDelay should be positive
Last modified: 2015-10-28 15:51:59 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?
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.
Created attachment 310898 [details] [review] forbid negative values for duration Here is a patch which warns/ignores negative values for duration as you suggested.
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
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.
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.
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.
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.
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 https://bugzilla.gnome.org/show_bug.cgi?id=752429
Created attachment 310904 [details] [review] forbid negative values for duration OK, that test is now removed.
Review of attachment 310904 [details] [review]: looks ok for me.
commit 54d93f597d541db8cf2f1b24dffc94e937f24a6d Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Tue Sep 8 14:00:54 2015 +0100 mpdparser: forbid negative values for duration https://bugzilla.gnome.org/show_bug.cgi?id=752492