http-streaming
http-streaming copied to clipboard
Add an option to support forced subtitles
Description
Proposal to introduce a new useForcedSubtitles
option to access forced subtitles when available.
Current State
Currently, VHS ignores forced subtitle tracks. This behavior makes it impossible to access such subtitles, when available, from player.textTracks
.
As a result, content with occasional foreign language dialogue cannot be translated, forcing the user to enable subtitles for the entire playback time.
Safari
Referring to the question asked in the Apple Developer Forums. Forced subtitles, if available, should be displayed automatically either according to the system language or the selected audio language. This seems to be confirmed by the Advice about subtitles section.
However, it seems that Safari does not implement this behavior, as confirmed by an Apple Media Engineer in point 2 of the response.
Finally, although Safari does not appear to implement this behavior correctly, forced subtitles are available via document.querySelector('video').textTracks
, allowing them to be enabled programmatically.
Http Streaming
The media-groups.js
file mentions the implementation choice with reference to point 5.8 of the Apple's HLS Authoring Specification. However, this choice affects point 5.9 of the specification, especially when the content contains multiple audio and occasional translations are not burned into the video.
Specific Changes proposed
- Updated
videojs-http-streaming.js
by adding theuseForcedSubtitles
option which is false by default, which can be used as a initialization or source option. - Updated the
media-groups.js
file to support theuseForcedSubtitles
option. - Update the
README.md
file documenting the new option. - Updated the demo to add a check box for the
useForcedSubtitles
option. - Add a unit test case in the
master-playlist-controller.test.js
file to ensure that forced subtitles are loaded. - Update the settings of the
media-groups.test.js
file so that it does not have an undefined value.
Use of the new option
- At initialization
const player = videojs(videoEl, { html5 : { vhs : { useForcedSubtitles : true } } }); // Or const player = videojs(videoEl, { html5 : { vhs : { useForcedSubtitles : !videojs.browser.IS_SAFARI } } });
- Through the
src
const player = videojs(videoEl); player.src({ src :'https://example.com/playlist.m3u8', useForcedSubtitles : true }); // Or player.src({ src :'https://example.com/playlist.m3u8', useForcedSubtitles : !videojs.browser.IS_SAFARI });
Requirements Checklist
- [X] Feature implemented / Bug fixed
- [X] If necessary, more likely in a feature request than a bug fix
- [X] Unit Tests updated or fixed
- [X] Docs/guides updated
- [ ] Example created (starter template on JSBin)
- [ ] Reviewed by Two Core Contributors
π Thanks for opening this pull request! π
Things that will help get your PR across the finish line:
- Run
npm run lint -- --errors
locally to catch formatting errors earlier. - Include tests when adding/changing behavior.
- Include screenshots and animated GIFs whenever possible.
We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.
Thanks for the PR. This seems reasonable to me. Thoughts @videojs/collaborators?
Codecov Report
Merging #1329 (1c42920) into main (c90863c) will increase coverage by
0.00%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## main #1329 +/- ##
=======================================
Coverage 85.37% 85.37%
=======================================
Files 40 40
Lines 9953 9954 +1
Branches 2307 2308 +1
=======================================
+ Hits 8497 8498 +1
Misses 1456 1456
Impacted Files | Coverage Ξ | |
---|---|---|
src/media-groups.js | 98.66% <100.00%> (ΓΈ) |
|
src/videojs-http-streaming.js | 90.63% <100.00%> (+0.02%) |
:arrow_up: |
:mega: Weβre building smart automated test selection to slash your CI/CD build times. Learn more
lovely, please merge !
Would we want the forced tracks to show up in the menu and be user-selectable, or just programatically selectable as Safari?
Safari uses a kind
of "forced"
for these tracks. This is not one of the standard kinds but it's worth discussing whether or not we'd want to follow Safari's approach.
In my opinion forced captions must be displayed without any user decision. Safari's programmatic approach is better.
@mister-ben, @privaloops, thanks for the feedback.
My initial thought was to opt for the simplest approach based on what different players in the market are doing. See:
The idea of using a kind
of "forced"
is very interesting, even if it is not standard. It seems that there was a discussion on this subject within the whatwg(issue 4472) which tends to the same conclusion. This discussion is based on another W3C discussion.
The proposed amendment would be as follows kind: properties.forced ? 'forced' :'subtitles',
https://github.com/amtins/http-streaming/blob/91fa3082aeada6e7175e1dfa57790ca56eb5bfdb/src/media-groups.js#L599-L605
However, the use of "forced"
for the kind
implies a modification of video.js
to add this value in the track-enum, otherwise all forced subtitles will continue to have a kind
of "subtitles"
even if this PR is updated.
Finally, I can also make a PR in the video.js
repo to add the "forced"
value. The result will be that the forced subtitles will not be visible in the subtitles menu but will remain available via the API player.textTracks()
allowing developers to make their own UX.
Hey! We've detected some video files in a comment on this issue. If you'd like to permanently archive these videos and tie them to this project, a maintainer of the project can reply to this issue with the following commands:
- for https://hls-js.netlify.app/demo/?src=https%3A%2F%2Fd2zihajmogu5jn.cloudfront.net%2Fbipbop-advanced%2Fbipbop_16x9_variant.m3u8: say
@video-archivist-bot save nEMaoE
- for http://player.kaltura.com/modules/KalturaSupport/tests/HLSPlaybackTester.html#%7B%22engine%22%3Atrue%2C%22stvod%22%3Atrue%2C%22ncweb%22%3Atrue%2C%22url%22%3A%22https%3A%2F%2Fd2zihajmogu5jn.cloudfront.net%2Fbipbop-advanced%2Fbipbop_16x9_variant.m3u8: say
@video-archivist-bot save OEDzWw
Hi @mister-ben, @gkatsev, is there anything I can do to help this PR be merged?
Updating the branch as it would be great to get this merged before the next release.
Congrats on merging your first pull request! πππ