Improvement of segment timeline $Time$ accuracy#706
Merged
TheModMaker merged 3 commits intoshaka-project:masterfrom Feb 22, 2017
Merged
Conversation
The current code essentially does this
timeReplacement =
(startTime / timescale + presentationTimeOffset) * timescale
When timescale is large enough (e.g. 10 MHz in order to support MS
Smooth Streaming as well), "startTime / timescale * timescale" may not
always be exactly "startTime" because of floating point precision,
which could produce incorrect segment URLs.
Keep startTime and presentationTimeOffset unchanged in the timeline
just to avoid the multiply/divide dance.
TheModMaker
suggested changes
Feb 22, 2017
Contributor
TheModMaker
left a comment
TheModMaker
left a comment
There was a problem hiding this comment.
Looks good, just a few changes. Even though the compiler doesn't complain about these extra fields, we should add them. It may break the compiled version and it's good documentation.
| var item = { | ||
| start: startTime / timescale, | ||
| end: endTime / timescale, | ||
| unscaledStart: startTime |
Contributor
There was a problem hiding this comment.
Add this field to the shaka.dash.MpdUtils.TimeRange definition.
Contributor
Author
There was a problem hiding this comment.
OK. Missed that. Will fix. Thanks.
| segmentDuration: segmentDuration, | ||
| startNumber: startNumber, | ||
| presentationTimeOffset: pto, | ||
| unscaledPresentationTimeOffset: Number(presentationTimeOffset), |
Contributor
There was a problem hiding this comment.
Add this field to the shaka.dash.MpdUtils.SegmentInfo definition.
| timescale: segmentInfo.timescale, | ||
| startNumber: segmentInfo.startNumber, | ||
| presentationTimeOffset: segmentInfo.presentationTimeOffset, | ||
| unscaledPresentationTimeOffset: segmentInfo.unscaledPresentationTimeOffset, |
Contributor
There was a problem hiding this comment.
Add this field to the shaka.dash.SegmentTemplate.SegmentTemplateInfo definition.
Contributor
|
Let me run it through our build bot |
Collaborator
|
Testing in progress... |
Collaborator
|
All tests passed! |
TheModMaker
approved these changes
Feb 22, 2017
TheModMaker
pushed a commit
that referenced
this pull request
Feb 24, 2017
Fix incorrect timeReplacement when large timescale
The current code essentially does this
timeReplacement =
(startTime / timescale + presentationTimeOffset) * timescale
When timescale is large enough (e.g. 10 MHz in order to support MS
Smooth Streaming as well), "startTime / timescale * timescale" may not
always be exactly "startTime" because of floating point precision,
which could produce incorrect segment URLs.
Keep startTime and presentationTimeOffset unchanged in the timeline
just to avoid the multiply/divide dance.
Closes #690
github-actions
bot
added
the
status: archived
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR solves #690 (and replaces PR #702)
With the following SegmentTimeline snippet
the second segment erroneously gets the wrong timestamp 5369174663790087 due to rounding errors.
This is because the calculation is done using values scaled to seconds as follows:
(5369174623790086 / 10000000 + 40000000 / 10000000) * 10000000 = 5369174663790087
The problem with this rounding error is that the timestamp is used in the$Time$ .mp4 URL template, and results in an HTTP 404 error when requesting the segment from the server.
Somewhat higher precision (roughly 1 more bit) can be achieved by avoiding the scaling and just do an integer addition like:
5369174623790086 + 40000000 = 5369174663790086
This is fixed in this PR by storing the unscaled values unscaledStart and unscaledPresentationOffset and using them for generating the timeReplacement for the segment request URL.
A lot of other calculations use times scaled to seconds as before, but since they don't need to be fully accurate, no change is needed.
Unfortunately, we haven't found a good way to add a unit or functional test for this PR, but no existing tests are broken. The fix solves the issue we have seen and makes our streams play in shaka player as they do in dash.js. We don't have reliable public URL, though.