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

Improvement of segment timeline $Time$ accuracy #706

Conversation

TobbeEdgeware
Copy link
Contributor

This PR solves #690 (and replaces PR #702)

With the following SegmentTimeline snippet

<SegmentTemplate timescale="10000000"  media="$Time$.mp4">
  <SegmentTimeline>
    <S t="5369174623790086" d="40000000/>
  <S d="40000000" />
</SegmentTimeline>

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.

PhamCongDuc and others added 2 commits February 22, 2017 13:07
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.
Copy link
Contributor

@TheModMaker TheModMaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add this field to the shaka.dash.MpdUtils.TimeRange definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Missed that. Will fix. Thanks.

@@ -398,6 +402,7 @@ shaka.dash.MpdUtils.parseSegmentInfo = function(context, callback) {
segmentDuration: segmentDuration,
startNumber: startNumber,
presentationTimeOffset: pto,
unscaledPresentationTimeOffset: Number(presentationTimeOffset),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add this field to the shaka.dash.MpdUtils.SegmentInfo definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix.

@@ -174,6 +174,7 @@ shaka.dash.SegmentTemplate.parseSegmentTemplateInfo_ = function(context) {
timescale: segmentInfo.timescale,
startNumber: segmentInfo.startNumber,
presentationTimeOffset: segmentInfo.presentationTimeOffset,
unscaledPresentationTimeOffset: segmentInfo.unscaledPresentationTimeOffset,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add this field to the shaka.dash.SegmentTemplate.SegmentTemplateInfo definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix.

@ismena
Copy link
Contributor

ismena commented Feb 22, 2017

Let me run it through our build bot

@shaka-bot
Copy link
Collaborator

Testing in progress...

@shaka-bot
Copy link
Collaborator

All tests passed!

@TheModMaker TheModMaker merged commit 547a88c into shaka-project:master 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 github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants