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 750797 - dashdemux: period duration is not validated
dashdemux: period duration is not validated
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.5.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-06-11 15:21 UTC by Florin Apostol
Modified: 2015-06-22 11:48 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch to add validation for period duration (6.87 KB, patch)
2015-06-12 13:07 UTC, Florin Apostol
none Details | Review
corrected the computation of period duration (7.44 KB, patch)
2015-06-15 12:19 UTC, Florin Apostol
committed Details | Review

Description Florin Apostol 2015-06-11 15:21:03 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.
Comment 1 Sebastian Dröge (slomo) 2015-06-11 17:43:39 UTC
Do you want to provide a patch for this?
Comment 2 Florin Apostol 2015-06-12 13:07:12 UTC
Created attachment 305144 [details] [review]
patch to add validation for period duration
Comment 3 Thiago Sousa Santos 2015-06-12 13:39:18 UTC
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
Comment 4 Florin Apostol 2015-06-12 14:07:57 UTC
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.
Comment 5 Sebastian Dröge (slomo) 2015-06-12 20:48:08 UTC
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
Comment 6 Florin Apostol 2015-06-15 12:19:42 UTC
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."
Comment 7 Florin Apostol 2015-06-15 12:25:15 UTC
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 8 Sebastian Dröge (slomo) 2015-06-22 11:47:15 UTC
Comment on attachment 305283 [details] [review]
corrected the computation of period duration

Seems to make sense
Comment 9 Sebastian Dröge (slomo) 2015-06-22 11:48:05 UTC
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