GNOME Bugzilla – Bug 750797
dashdemux: period duration is not validated
Last modified: 2015-06-22 11:48:05 UTC
gst_mpd_client_setup_media_presentation tries to compute period duration. In cases when the computation is based on information from next period or from mediaPresentationDuration, the duration can be set to a negative value if the information is not correct. Scenario 1: /* try to infer this period duration from the start time of the next period */ GstPeriodNode *next_period_node = next->data; if (next_period_node->start != -1) { duration = next_period_node->start * GST_MSECOND - start; If next_period_node->start is wrongly configured less than start, duration will be negative. Scenario 2: } else if (client->mpd_node->mediaPresentationDuration != -1) { /* last Period of the Media Presentation */ duration = client->mpd_node->mediaPresentationDuration * GST_MSECOND - start; If mediaPresentationDuration is less than start, duration will be negative. duration is stored in GstClockTime which is unsigned, meaning we will end up with a huge duration.
Do you want to provide a patch for this?
Created attachment 305144 [details] [review] patch to add validation for period duration
Review of attachment 305144 [details] [review]: Do you have real life mpd samples that present this issue or is this for sake of validation? Some of the checks might be too strict as the mediaPresentationDuration can suffer some rounding and comparing for equality might cause some valid files to not play
I don't have any real life samples. This is just or the sake of validation. I tested it against the samples provided by the standard (http://standards.iso.org/ittf/PubliclyAvailableStandards/c065274_ISO_IEC_23009-1_2014_Electronic_inserts.zip) and a few other samples I had and it was ok. I admit that this might be too aggressive and in some real life cases that are malformed these checks will fail. The operations involved are based on integers, so there will be no rounding. For large values of mediaPresentationDuration there might be overflow. If there are some real life files that will suffer from overflow, wouldn't that mean that duration will be incorrectly set to a very large value for the last period? I don't know what will happen in that case.
It's not only about malformed files. There could be rounding errors when converting from the real duration of the media to integers, it happens :) There should be some tolerance here at least, at best we should somehow try to still play those files
Created attachment 305283 [details] [review] corrected the computation of period duration If I interpret the standard correctly, the optional attribute "period duration" should not be used when calculating a period's duration if: a) the next period has the "start" attribute set, or b) this is the last period and the mediaPresentationDuration@MPD is set Please see ISO/IEC 23009-1:2014(E), chapter 5.3.2.1 "The Period extends until the PeriodStart of the next Period, or until the end of the Media Presentation in the case of the last Period."
I made my first patch obsolete and instead of validating the duration after it is being computed, I decided to change the way it is being computed. It should affect only scenarios where both a start and a duration are provided for a period and the duration is incompatible with the next period's start or the MPD's mediaPresentationDuration. The patch also validates that a period's start is not before the previous period's start.
Comment on attachment 305283 [details] [review] corrected the computation of period duration Seems to make sense
commit 8336d7a60bc8340e3bc012fec8fbbbd739ef5674 Author: Florin Apostol <florin.apostol@oregan.net> Date: Mon Jun 15 12:59:55 2015 +0100 dashdemux: corrected computation of period's duration According to ISO/IEC 23009-1:2014(E), chapter 5.3.2.1 "The Period extends until the PeriodStart of the next Period, or until the end of the Media Presentation in the case of the last Period." This means that a configured value for optional attribute period duration should be ignored if the next period contains a start attribute or it is the last period and the MPD contains a mediaPresentationDuration attribute. https://bugzilla.gnome.org/show_bug.cgi?id=750797