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

There should not be a gap in text references >1s error #1562

Closed
jakubvojacek opened this issue Aug 27, 2018 · 8 comments

Comments

Projects
None yet
3 participants
@jakubvojacek
Copy link

commented Aug 27, 2018

Have you read the FAQ and checked for duplicate open issues?:
yes
What version of Shaka Player are you using?:
2.5.0 -- [JP: assumed to be 2.5.0-beta, which is the latest release as of today]
Can you reproduce the issue with our latest release version?:
yes
Can you reproduce the issue with the latest code from master?:
yes
Are you using the demo app or your own custom app?:
both
If custom app, can you reproduce the issue using our demo app?:
yes
What browser and OS are you using?:
mac os & chrome
What are the manifest and license server URIs?:
https://goo.gl/MDjuDA

What did you do?
I tried to play the VOD with enabled subtitles.
What did you expect to happen?
To have a smooth playback
What actually happened?
The playback starts fine but if seek to middle, it stops and keeps spamming the console with Assertion failed: There should not be a gap in text references >1s. I checked the MPD and did not see any gaps in the segment template.

This content was produced with shaka-packager. Please note that this content even with subtitles enabled is playable with dashjs and on android with exoplayer. Is this an issue with the packaged content or with shaka player?

Thank you
Jakub

@joeyparrish

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

I managed to reproduce using the latest code in master, which is currently equivalent to v2.5.0-beta released a few days ago. I initially missed that subtitles must be enabled for this bug to reproduce.

Full repro steps I came up with:

  1. Paste the goo.gl URL into the demo app as a custom asset
  2. Click load
  3. Get the real URL from the error message in the JS console (since the redirect at goo.gl does not support CORS)
  4. Paste the real URL into the custom asset field
  5. Click load
  6. Wait for the content to play
  7. Enable subtitles
  8. Seek to the middle (roughly) of the seek range

Also reproduces in v2.4.4, v2.3.10, but not in v2.2.10.

@joeyparrish

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

When the assertion is triggered, the value of this.bufferedEnd_ in TextEngine is negative, which should not happen. This occurs when the first text segment is appended because this.timestampOffset_ is very negative (-57486.242).

This offset value seems to come from this part of the manifest:

      <Representation id="0" bandwidth="1148" codecs="wvtt" mimeType="application/mp4">
        <SegmentTemplate timescale="1000" presentationTimeOffset="57486242"
            initialization="--redacted--.mp4" media="--redacted--_$Number$.m4s"
            startNumber="1">
          <SegmentTimeline>
            <S t="57481600" d="3200" r="768"/>
          </SegmentTimeline>

According to the manifest, it was generated by Shaka Packager version daf856fac9, which matches a fork you've made. The fork seems to be up-to-date with the latest upstream Packager code, but I don't think your customizations could be responsible for the problem. The changes are small, and I don't see any obvious problem in the diff.

@joeyparrish

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

This change in the Player seems to cause the issue: c38d4dd

Affected versions: v2.3.10, v2.4.2-v2.4.4, v2.5.0-beta

@jakubvojacek

This comment has been minimized.

Copy link
Author

commented Aug 27, 2018

The only difference in my fork was actually suggested by @kqyang himself and should not cause any of this I believe. The change is only about reading the subtitle file indefinitely while packaging live content. As this is VOD, it should not be affected at all.

@joeyparrish

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

I found the cause in Shaka Player, and it should be easy to fix. Thanks for reporting this!

@joeyparrish joeyparrish added bug and removed needs triage labels Aug 27, 2018

@shaka-bot shaka-bot added this to the v2.5 milestone Aug 27, 2018

@joeyparrish joeyparrish self-assigned this Aug 27, 2018

@shaka-bot shaka-bot closed this in 9cce246 Aug 28, 2018

@jakubvojacek

This comment has been minimized.

Copy link
Author

commented Aug 29, 2018

Thank you, working perfectly!

@joeyparrish

This comment has been minimized.

Copy link
Member

commented Aug 29, 2018

Happy to help!

joeyparrish added a commit that referenced this issue Oct 8, 2018

Fix TextEngine buffered range calculations
This reverts commit c38d4dd, which
actually broke text range calculations in v2.3.10, and v2.4.2-v2.4.4.
The original commit was meant to account for the period start, but
resulted in a double-accounting of presentationTimeOffset.

The start and ends times passed into TextEngine's appendBuffer were
period-relative, so timestampOffset had already been applied.  To
avoid further confusion and to fix the original issue the reverted
commit tried to address, these have been changed to
presentation-relative timestamps.  Now the period start and all
offsets have been accounted for before the metadata reaches
MediaSourceEngine and TextEngine.

The tests added in the bad commit have been modified to test for the
opposite: that we do not erroneously account for timestamp offset when
calculating the buffered ranges for text.

Backported to v2.4.x

Closes #1562

Change-Id: I9fa7a3f59906c4f3e623f411e48551f86f5c2ff7

@shaka-bot shaka-bot added the archived label Oct 27, 2018

@google google locked and limited conversation to collaborators Oct 27, 2018

@joeyparrish

This comment has been minimized.

Copy link
Member

commented Nov 9, 2018

Fix cherry-picked to v2.4.5.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.