-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix the bug that UTCTiming is ignored even when autoCorrectDrift is off #2412
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
Fix the bug that UTCTiming is ignored even when autoCorrectDrift is off #2412
Conversation
If maxSegmentEndTime_ is set, UTCTiming is not parsed even when autoCorrectDrift_ is false. Becuase it is used in usingPresentationStartTime() which is used to determine whether parsing UTCTiming or not.
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this! I would like to see a slightly different fix, though.
lib/media/presentation_timeline.js
Outdated
@@ -215,8 +215,10 @@ shaka.media.PresentationTimeline = class { | |||
(max, r) => { return Math.max(max, r.endTime - r.startTime); }, | |||
this.maxSegmentDuration_); | |||
|
|||
this.maxSegmentEndTime_ = | |||
Math.max(this.maxSegmentEndTime_, lastReferenceEndTime); | |||
if (this.autoCorrectDrift_) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is the right fix. See this code in lib/dash/dash_parser.js
:
// We only need to do clock sync when we're using presentation start
// time. This condition also excludes VOD streams.
if (presentationTimeline.usingPresentationStartTime()) {
const timingElements = XmlUtils.findChildren(mpd, 'UTCTiming');
const offset = await this.parseUtcTiming_(baseUris, timingElements);
// Detect calls to stop().
if (!this.playerInterface_) {
return;
}
presentationTimeline.setClockOffset(offset);
}
Your fix works by changing the results of usingPresentationStartTime()
, but it's indirect. The real bug seems to be this condition in usingPresentationStartTime()
:
// If we have explicit segment times, we're not using the presentation
// start time.
if (this.maxSegmentEndTime_ != null) {
return false;
}
Rather that avoid computing maxSegmentEndTime_
, you should require both maxSegmentEndTime_
and autoCorrectDrift_
in that condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense.
I changed my fix.
ee5c071
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks for the help!
I'll ask the build bot to test it now.
All tests passed! |
Thanks for your contribution! |
This removes the tests added by #2412 since they didn't test what was wanted. This also fixes our existing tests to ensure they work with autoCorrectDrift. Lastly this adds a new test to ensure the UTCTiming is ignored when autoCorrectDrift is true. Change-Id: If511fca5d3f360cdf373229961b2a01fdbda31bb
This removes the tests added by #2412 since they didn't test what was wanted. This also fixes our existing tests to ensure they work with autoCorrectDrift. Lastly this adds a new test to ensure the UTCTiming is ignored when autoCorrectDrift is true. Backported to v2.5.x Change-Id: If511fca5d3f360cdf373229961b2a01fdbda31bb
This is the fix for this issue.
#2411
shaka.dash.DashParser.processManifest
is using thepresentationTimeline.usingPresentationStartTime()
to determine if it parses UTCTiming or not.The
usingPresentationStartTime
always returns the false when maxSegmentEndTime_ is not null.When the manifest has
SegmentTimeline
, it seems always setting a value tomaxSegmentEndTime_
.I added the code not to set
maxSegmentEndTime_
whenautoCorrectDrift
is false.