Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MPD parser incorrectly treats @startNumber as a strictly positive integer value #10

Closed
barbibulle opened this issue Jan 16, 2015 · 15 comments
Labels
status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Milestone

Comments

@barbibulle
Copy link

The MPD parser expects the @startNumber attributes to be strictly positive (shaka.dash.mpd.SegmentList.prototype.parse and shaka.dash.mpd.SegmentTemplate.prototype.parse when computing firstSegmentNumber).
This is incorrect, as the DASH schema specifies:
<xs:attribute name="startNumber" type="xs:unsignedInt"/>

The default value is correctly set to 1, but values of 0 should be accepted.
Here's an example MPD with @startNumber=0:
http://www.bok.net/dash/tears_of_steel/cleartext/stream.mpd

(If the change is made to allow for 0, the stream plays correctly)

NOTE: I think this bug hides a second bug: if the stream is played starting from segment index 1 (instead of 0), as is the case when @startNumber=0 isn't accepted, the segments, starting from index 1, are correctly downloaded and sent as source buffers, but nothing is played. Instead, it would be expected that the video would play, except that the first segment (index 0) would be skipped.
It is likely that the player assumes that the media timestamps in the segments always start at 0, so starting with segment 1 (with a non-0 timestamp) is buffered but never starts playing (the decoder probably still waits for timestamp 0 to be sent).
However, it is perfectly legal for media segments to have timestamps that don't start at 0.

@joeyparrish
Copy link
Member

This is definitely a bug in the MPD parser. Thanks for pointing this out.

The fact that the 0th segment is not skipped is not a bug. If the video's playhead is at 0s, but there is no data in MediaSource for t=0s, then the video tag is in a "buffering" state. This is to be expected, since MediaSource allows for data to be inserted in an arbitrary order.

You are right that timestamps need not begin at 0. In that case, though, the correct thing to do is to use SegmentTemplate@presentationTimeOffset to indicate where they do start. This is given in the timescale, so for the MPD you linked above, setting presentationTimeOffset="3000" for both SegmentTemplates allows playback to begin with firstSegmentNumber="1".

We will get the @firstSegmentNumber parser bug fixed, though. Thanks!

@joeyparrish joeyparrish added the type: bug Something isn't working correctly label Jan 21, 2015
@joeyparrish joeyparrish added this to the v1.2 milestone Jan 21, 2015
tdrews pushed a commit that referenced this issue Jan 26, 2015
Closes b/19093388, #10.

Change-Id: Ie05b1f337f4ffd6b24d3e56666663a2ecc284deb
@barbibulle
Copy link
Author

I'm afraid that commit 5c7fca9 doesn't quite fix the problem.
One of the two occurrences of parsing @startNumber (in SegmentTemplate) is "almost" fixed, in the sense that mpd.parsePositiveInt_ was correctly replaced by mpd.parseNonNegativeInt_, which makes parseAttr_ return 0 instead of null, but the || 1 at the end still gets in the way (0 || 1 is still 1...).
this.firstSegmentNumber =
mpd.parseAttr_(elem, 'startNumber', mpd.parseNonNegativeInt_) || 1;

Also, there is a second occurrence of parsing @startNumber (in SegmentList), which suffers from the original problem of calling mpd.parsePositiveInt_

Here's an MPD with @startNumber=0:
http://www.bok.net/dash/tears_of_steel/cleartext/stream.mpd

@joeyparrish
Copy link
Member

Thanks, Gilles. We'll take a look.

@joeyparrish joeyparrish reopened this Feb 2, 2015
@tdrews
Copy link
Contributor

tdrews commented Feb 3, 2015

Yes, 5c7fca9 is definitely not correct. However, after looking at the MPD spec. (ISO/IEC 23009-1:2014) more closely, it appears that startNumber must be greater than zero.

The spec. indicates in section 5.3.9.5.3 that the $Number$ placeholder should be replaced with (k - 1) + startNumber, and that the $Time$ placeholder should be replaced with ((k - 1) + (startNumber - 1)) * segmentDuration, where k is the segment number. It seems that k is one-based, so startNumber must be greater than zero to ensure that $Number$ and $Time$ are both strictly non-negative.

@barbibulle
Copy link
Author

It seems indeed that this clause would prevent @startNumber from ever having the value 0. 
The problem is only with $Time$, though, as with @startNumber=0, you’d have $Number$ be 0 for k=1, which isn’t a problem.
The spec should be clearer on that point, as nothing specifically prohibits @startNumber to be 0 (the schema says unsignedInteger), so without language specifically constraining @startNumber to be > 0, you could interpret the spec as having $Time$ be (-1)*duration for the first segment when @startNumber=0, which would obviously be weird.
I’ll shoot an email to the DASH mailing list to point out the spec deficiency in that regard.

-- Gilles

On February 3, 2015 at 10:57:03 AM, tdrews (notifications@github.com) wrote:

Yes, 5c7fca9 is definitely not correct. However, after looking at the MPD spec. (ISO/IEC 23009-1:2014) more closely, it appears that startNumber must be greater than zero.

The spec. indicates in section 5.3.9.5.3 that the $Number$ placeholder should be replaced with (k - 1) + startNumber, and that the $Time$ placeholder should be replaced with ((k - 1) + (startNumber - 1)) * segmentDuration, where k is the segment number. It seems that k is one-based, so startNumber must be greater than zero to ensure that $Number$ and $Time$ are both strictly non-negative.


Reply to this email directly or view it on GitHub.

@joeyparrish
Copy link
Member

We definitely see the point of view that xs:unsignedInteger implies the validity of 0, so thank you for following up on this with the DASH-IF.

We will revert 5c7fca9 and consider @startNumber=0 to be invalid again until there is a clear correction to the spec one way or the other. Please reopen this issue if @startNumber=0 is clarified to be valid.

@barbibulle
Copy link
Author

MPEG has published a corrigendum to the spec, that fixes the muddy language around the computation of segment numbers. The new text (found in ISO/IEC 23009-1:2014/Cor.1:2014(E)), makes it possible for @startNumber to be 0. The problematic computation which would have led to a negative $Time$ value is now changed to:

If @duration attribute is present, then the MPD start time of the Media Segment is determined as (Number-NumberStart) times the value of the attribute @duration with NumberStart the value of the @startNumber attribute, and Number the segment number (e.g. (k-1) + NumberStart). 

So #10 should be re-opened.

-- Gilles

On February 3, 2015 at 5:13:29 PM, Joey Parrish (notifications@github.com) wrote:

We definitely see the point of view that xs:unsignedInteger implies the validity of 0, so thank you for following up on this with the DASH-IF.

We will revert 5c7fca9 and consider @startNumber=0 to be invalid again until there is a clear correction to the spec one way or the other. Please reopen this issue if @startNumber=0 is clarified to be valid.


Reply to this email directly or view it on GitHub.

@joeyparrish
Copy link
Member

joeyparrish commented Feb 4, 2015 via email

@tdrews
Copy link
Contributor

tdrews commented Feb 4, 2015

I can't seem to locate the corrigendum (ISO/IEC 23009-1:2014/Cor.1:2014(E)) on the ISO site. Where is this document available from?

Thanks.

@joeyparrish joeyparrish removed this from the v1.2 milestone Feb 11, 2015
@tdrews tdrews closed this as completed in b244def Feb 13, 2015
@barbibulle
Copy link
Author

I think there's still a problem with the updated code. It still forces startNumber to be 1, even if the MPD specifies @startNumber = 0.
For instance, with an MPD like the one at: http://www.bok.net/dash/tears_of_steel/cleartext/stream.mpd, the shaka player will start with seg-1.m4f instead of seg-0.m4f.

From the spec:
"If the Representation contains or inherits a SegmentTemplate element with $Number$ then the URL of the media segment at position k is determined by replacing the $Number$ identifier by (k-1) + @startNumber."

So if @startNumber == 0, $Number# for the first segment (i.e k=1) is 0

@joeyparrish
Copy link
Member

Our previous fix, as you pointed out, was insufficient. We reverted that change for now.

As @tdrews pointed out in his comment earlier in the thread, it seems that the only way to interpret 23009-1:2014 to ensure that $Number$ and $Time$ are non-negative is if startNumber and k both begin at one.

You mentioned a corrigendum, but it does not appear to be available in the list of publicly available standards on the ISO website. We are still unable to locate it this morning, not even on the corrigenda tab under 23009-1:2014.

We'd love to resolve this in a way that is consistent with the latest amendments to the spec. Where can we find this corrigendum?

@barbibulle
Copy link
Author

It seems indeed that the latest corrigendum isn’t available on the IEC or ISO websites. That’s surprising, given that the final text for this doc was published on Oct 30 2014.
I obtained it from the doc repo available to MPEG members. Maybe your company is an MPEG member, in which case you’ll have access to the document register. 

-- Gilles

On February 17, 2015 at 10:07:12 AM, Joey Parrish (notifications@github.com) wrote:

Our previous fix, as you pointed out, was insufficient. We reverted that change for now.

As @tdrews pointed out in his comment earlier in the thread, it seems that the only way to interpret 23009-1:2014 to ensure that $Number$ and $Time$ are non-negative is if startNumber and k both begin at one.

You mentioned a corrigendum, but it does not appear to be available in the list of publicly available standards on the ISO website. We are still unable to locate it this morning, not even on the corrigenda tab under 23009-1:2014.

We'd love to resolve this in a way that is consistent with the latest amendments to the spec. Where can we find this corrigendum?


Reply to this email directly or view it on GitHub.

@joeyparrish joeyparrish added the flag: seeking PR We are actively seeking PRs for this; we do not currently expect the core team will resolve this label Feb 25, 2015
@joeyparrish joeyparrish assigned joeyparrish and unassigned tdrews Feb 25, 2015
@joeyparrish
Copy link
Member

I'm not comfortable making changes at this time based on a document that isn't open to the public. However, I'm reopening this issue so that we don't lose sight of it. When the updated standard is published, I will be happy to implement it.

@joeyparrish joeyparrish reopened this Feb 25, 2015
@joeyparrish joeyparrish removed the type: bug Something isn't working correctly label Feb 25, 2015
@joeyparrish joeyparrish removed their assignment Apr 27, 2015
@johnluther
Copy link

Hi, @joeyparrish. I was able to access the corrigendum document for free here.

The ISO doc repository is a bit confusing, so here are the repro steps:

  1. Go to the main DASH spec page.
  2. Click the Corrigenda/Amendments tab.
  3. Click the "ISO/IEC 23009-1:2014/Cor 1:2015" link.
  4. Click the "Preview ISO/IEC 23009-1:2014/Cor 1:2015" button.

The text that @barbibulle refers to is on page 22.

Can we get this implemented now? A customer of ours has come across the issue and it is blocking them b/c their video encoding provider always sets the startNumber == 0.

Thanks--
JL

@tdrews
Copy link
Contributor

tdrews commented Jun 25, 2015

Thanks for the link. I will take a look.

@joeyparrish joeyparrish added the type: bug Something isn't working correctly label Jun 25, 2015
@joeyparrish joeyparrish removed the flag: seeking PR We are actively seeking PRs for this; we do not currently expect the core team will resolve this label Jun 25, 2015
@joeyparrish joeyparrish added this to the v1.4.0 milestone Jun 25, 2015
@shaka-project shaka-project locked and limited conversation to collaborators Mar 22, 2018
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Apr 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Development

No branches or pull requests

6 participants