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

Implement live VTT subtitle loading #2148

Merged
merged 5 commits into from Mar 11, 2019

Conversation

johnBartos
Copy link
Collaborator

@johnBartos johnBartos commented Feb 24, 2019

This PR will...

  • Start and stop subtitle loading in sync with startLoad()/stopLoad() API calls
  • Compute sliding & merge subtitle playlists during live playback
  • Fix shared references between subtitle-track-controller and subtitle-stream-controller
  • Refactoring:
    • Extend subtitle-stream-controller from base-stream-controller
    • Move more shared logic into base-stream-controller
    • Move live reload interval calculation into a shared function & import into subtitle-track-controller
    • Remove the trackId from subtitle fragments, and refer to them by their level property
    • Several other minor refactorings (flipped conditionals, destructuring args, organizing functions)
  • Add a bunch of unit tests

Why is this Pull Request needed?

Live subtitle playback is not functioning correctly - on first glance, it appears that sliding isn't being calculated between live refreshes. This is true, but in the course of fixing it I noticed that the live VTT implementation was incomplete and had several unhandled edge cases.

Are there any points in the code the reviewer needs to double check?

We (JW) QA tested this and shipped it in our 8.7.6 release. It would be good to double-check that any code specific to the JW fork didn't make it in. This was our test criteria:

  • When live captions are enabled, cues should appear within 1 segment duration
  • When VOD captions are enabled, cues should appear immediately
  • Cues should appear at the expected times for as long as captions are enabled
  • When the live stream is paused/stopped captions loading must stop
  • When captions are disabled, no cues or captions manifest should load

Test streams:

@johnBartos johnBartos added the Bug label Feb 24, 2019
@johnBartos johnBartos added this to the 0.12.3 milestone Feb 24, 2019
@johnBartos johnBartos added this to In progress in 0.13.0 via automation Feb 24, 2019
@johnBartos
Copy link
Collaborator Author

Once this is merged into master I'll cherry pick it onto the 0.12.3 branch

@mtoczko
Copy link
Collaborator

mtoczko commented Feb 25, 2019

I see an issue when stream(vtt) have pdt. In the loop loads segments n+1. And player loading segments when is paused.

@johnBartos
Copy link
Collaborator Author

@mtoczko Can you give me a test stream please?

Copy link
Member

@michaelcunningham19 michaelcunningham19 left a comment

Choose a reason for hiding this comment

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

Diff looks good to me 👍

I am curious about the above mentioned issue w/ PDT though

@mtoczko
Copy link
Collaborator

mtoczko commented Feb 26, 2019

@johnBartos
pdt: https://mtoczko.github.io/hls-test-streams/test-vtt-pdt/playlist.m3u8
whiteout pdt: https://mtoczko.github.io/hls-test-streams/test-vtt-pdt/playlist2.m3u8

Easy way to reproduce this issue is use jwplayer.

{ 
playlist: [{
                file: 'https://mtoczko.github.io/hls-test-streams/test-vtt-pdt/playlist.m3u8',
            }],
  preload: 'auto',
  autostart: false
}

Start stream, turn on subtitle and reload page.

HLS.JS
Start stream, seek to 60, turn on subtitle, seek to 50.

itsjamie
itsjamie previously approved these changes Feb 27, 2019
Copy link
Collaborator

@itsjamie itsjamie left a comment

Choose a reason for hiding this comment

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

Couple of bits and bobs of changes that could be done, but nothing that has to be imo. I got through most of the PR, and if none of the small changes got caught as incorrect behaviour by unit tests then 👍 given the manual QA that's happened.

src/controller/fragment-tracker.js Show resolved Hide resolved
src/controller/audio-stream-controller.js Show resolved Hide resolved
src/controller/audio-stream-controller.js Show resolved Hide resolved
src/controller/level-controller.js Show resolved Hide resolved
reloadInterval = minReloadInterval;
}

if (lastRequestTime) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is slightly different in the implementation that was here before.

Previously the max was taken between the rounded result in reloadInterval and minReloadInterval, now it's the max of the unrounded reloadInterval and minReloadInterval.

I don't see this resulting in any meaningful impact though. It could lead up to if I recall Javascript rounding right, a reload interval that occurs up to 0.49s early?

src/controller/stream-controller.js Show resolved Hide resolved
src/controller/level-helper.js Show resolved Hide resolved
src/controller/level-helper.js Show resolved Hide resolved
src/controller/level-helper.js Show resolved Hide resolved
src/controller/level-helper.js Show resolved Hide resolved
0.13.0 automation moved this from In progress to Reviewer approved Feb 27, 2019
@itsjamie
Copy link
Collaborator

Hm. One thing I have noticed is that during live playback that cues are missed if you are right at the live edge (within a single SD from the end).

image

@itsjamie
Copy link
Collaborator

itsjamie commented Feb 27, 2019

Also, it doesn't backfill old buffer that is outside of the current window. So if you had captions disabled on the first go around, and that is captions file is out of window now, it doesn't go fetch it.

To implement this we would have to break:

  • When captions are disabled, no cues or captions manifest should load

Possible change. If done, the biggest we could spec compliantly get away with is fetching the segment once it's elapsed a full sliding window.

In that, if the window is thirty seconds, the segment should be available on the origin for AT LEAST a full minute according to spec, so we could backfill to that limit, but we would have to be constantly fetching the manifest to do so.

I think we wait for folks to request this behaviour, since it's code we would never get rid of, its a lot of network requests, and most folks don't enable captions and then seek whole windows back in their buffer. I'm struggling to think of a site that has a sliding window, but allows seekback into local MSE buffer.

0.13.0 automation moved this from Reviewer approved to Needs review Mar 7, 2019
@johnBartos johnBartos force-pushed the upstream/bugfix/vtt-live-synchronization branch from ff28121 to 7e58729 Compare March 7, 2019 21:15
Copy link
Collaborator

@mtoczko mtoczko left a comment

Choose a reason for hiding this comment

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

LGTM

@johnBartos
Copy link
Collaborator Author

johnBartos commented Mar 9, 2019 via email

0.13.0 automation moved this from Needs review to Reviewer approved Mar 9, 2019
Copy link
Member

@michaelcunningham19 michaelcunningham19 left a comment

Choose a reason for hiding this comment

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

👍

@johnBartos johnBartos merged commit 656a681 into master Mar 11, 2019
0.13.0 automation moved this from Reviewer approved to Done Mar 11, 2019
@johnBartos johnBartos mentioned this pull request May 7, 2019
3 tasks
@robwalch robwalch deleted the upstream/bugfix/vtt-live-synchronization branch March 15, 2020 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
0.13.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants