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 (2.x) #1364

Conversation

dzianis-dashkevich
Copy link
Contributor

These are cherry-picked commits from this PR: #1360
with fixed conflicts for 2.x

...

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

@codecov
Copy link

codecov bot commented Jan 30, 2023

Codecov Report

Merging #1364 (b11684f) into 2.x (1ed5343) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##              2.x    #1364   +/-   ##
=======================================
  Coverage   86.34%   86.35%           
=======================================
  Files          39       39           
  Lines        9859     9871   +12     
  Branches     2299     2301    +2     
=======================================
+ Hits         8513     8524   +11     
- Misses       1346     1347    +1     
Impacted Files Coverage Δ
src/master-playlist-controller.js 94.86% <100.00%> (+0.06%) ⬆️
src/videojs-http-streaming.js 91.17% <100.00%> (+0.09%) ⬆️
src/vtt-segment-loader.js 81.35% <100.00%> (-0.32%) ⬇️
src/source-updater.js 94.19% <0.00%> (-0.33%) ⬇️

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

@dzianis-dashkevich dzianis-dashkevich merged commit a7ca824 into videojs:2.x Jan 30, 2023
dzianis-dashkevich added a commit that referenced this pull request Jan 30, 2023
* fix: Add exception guard for VTT parsing state if vtt.js is not loaded for any reasons.

* fix: Do not override native for all iOS/iPadOS browsers

* fix: Add guard for vtt-segment-loader to actually load vtt.js in case we do not have it loaded

* chore: fix eslit errors

* chore: Add loadVttJs test

* chore: Add test for parse exception if no vtt.js is loaded for any reason

* chore: fix typo

* chore: remove redundant default value

---------

Co-authored-by: Dzianis Dashkevich <[email protected]>
@alex-barstow alex-barstow changed the title 2x/fix/in manifest vtt ios mse issue fix: in manifest vtt ios mse issue (2.x) Jan 30, 2023
@dzianis-dashkevich dzianis-dashkevich deleted the 2x/fix/in-manifest-vtt-ios-mse-issue branch November 30, 2023 16:30
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.

3 participants