Skip to content

Conversation

thmatuza
Copy link
Contributor

This is the fix for this issue.
#2411

shaka.dash.DashParser.processManifest is using the presentationTimeline.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 to maxSegmentEndTime_.

I added the code not to set maxSegmentEndTime_ when autoCorrectDrift is false.

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.
@googlebot
Copy link

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 @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@thmatuza
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Copy link
Member

@joeyparrish joeyparrish left a 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.

@@ -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_) {
Copy link
Member

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.

Copy link
Contributor Author

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

@thmatuza thmatuza requested a review from joeyparrish February 26, 2020 22:02
Copy link
Member

@joeyparrish joeyparrish 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! Thanks for the help!

I'll ask the build bot to test it now.

@shaka-bot
Copy link
Collaborator

All tests passed!

@joeyparrish joeyparrish merged commit 83e3600 into shaka-project:master Feb 26, 2020
@joeyparrish
Copy link
Member

Thanks for your contribution!

joeyparrish pushed a commit that referenced this pull request Mar 18, 2020
…ff (#2412)

Backported to v2.5.x

Closes #2411

Change-Id: Id580fab5e505bd13810ba20dac705076ad4bb964
shaka-bot pushed a commit that referenced this pull request Mar 20, 2020
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
joeyparrish pushed a commit that referenced this pull request Mar 22, 2020
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
@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.

4 participants