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

Fix: In-manifest VTT iOS MSE issue #1360

Conversation

dzianis-dashkevich
Copy link
Contributor

@dzianis-dashkevich dzianis-dashkevich commented Jan 18, 2023

Description

We have a bug with default options setup for HLS in-manifest VTT in chrome iOS (and I assume other non-safari Webkit-based iOS browsers). The problem is that we do not load vtt.js, but we use VHS to handle playback. vtt-segment-loader implicitly depends on window.WebVTT.Parser. Since vtt.js is not loaded - window.WebVTT is undefined. It means that we are not able to parse received VTT cues.

Specific Changes proposed

  • Do not override native for all iOS/iPadOS browsers by default.
  • Add fallback logic to load vtt.js in vtt-segment-loader if we do not have it loaded. (eg: force overrideNative option case).
  • Add exception guard with a specific error message in case we call parse but still do not have vtt.js loaded for any reason.
  • Fix/Add tests to cover all listed changes.

Note
Tested Chrome on iPadOS 16.2:
default run: (native playback and apple's native text tracks are shown)
set override Native to true (VHS playback and apple's native text tracks are shown)
set override Native to true and set native text tracks to false (VHS playback and styled text tracks are shown)

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors

@dzianis-dashkevich dzianis-dashkevich changed the title Fix/in manifest vtt ios mse issue Fix: In-manifest VTT iOS MSE issue Jan 18, 2023
@codecov
Copy link

codecov bot commented Jan 18, 2023

Codecov Report

Merging #1360 (5b877a9) into main (d7e8713) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1360      +/-   ##
==========================================
+ Coverage   86.01%   86.03%   +0.01%     
==========================================
  Files          40       40              
  Lines        9842     9855      +13     
  Branches     2294     2295       +1     
==========================================
+ Hits         8466     8479      +13     
  Misses       1376     1376              
Impacted Files Coverage Δ
src/playlist-controller.js 95.18% <100.00%> (+0.06%) ⬆️
src/videojs-http-streaming.js 90.61% <100.00%> (+0.13%) ⬆️
src/vtt-segment-loader.js 80.79% <100.00%> (-0.33%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@dzianis-dashkevich dzianis-dashkevich marked this pull request as ready for review January 19, 2023 05:46
.then(
() => this.segmentRequestFinished_(error, simpleSegment, result),
() => this.stopForError({ message: 'Error loading vtt.js' })
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice tidy-up job here 🙌

Copy link
Contributor

@Essk Essk left a comment

Choose a reason for hiding this comment

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

This all looks good to me

Copy link
Contributor

@adrums86 adrums86 left a comment

Choose a reason for hiding this comment

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

:shipit: !

@dzianis-dashkevich dzianis-dashkevich merged commit 6ba70e0 into videojs:main Jan 24, 2023
@dzianis-dashkevich dzianis-dashkevich deleted the fix/in-manifest-vtt-ios-mse-issue branch November 30, 2023 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants