http-streaming icon indicating copy to clipboard operation
http-streaming copied to clipboard

Add an option to support forced subtitles

Open amtins opened this issue 2 years ago β€’ 3 comments

Description

Proposal to introduce a new useForcedSubtitles option to access forced subtitles when available.

forced-subtitles

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 the useForcedSubtitles option which is false by default, which can be used as a initialization or source option.
  • Updated the media-groups.js file to support the useForcedSubtitles 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
  • [ ] Reviewed by Two Core Contributors

amtins avatar Sep 22 '22 15:09 amtins

πŸ’– 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.

welcome[bot] avatar Sep 22 '22 15:09 welcome[bot]

Thanks for the PR. This seems reasonable to me. Thoughts @videojs/collaborators?

gkatsev avatar Oct 06 '22 22:10 gkatsev

Codecov Report

Merging #1329 (1c42920) into main (c90863c) will increase coverage by 0.00%. The diff coverage is 100.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

codecov[bot] avatar Oct 06 '22 22:10 codecov[bot]

lovely, please merge !

privaloops avatar Nov 07 '22 16:11 privaloops

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.

mister-ben avatar Nov 08 '22 10:11 mister-ben

In my opinion forced captions must be displayed without any user decision. Safari's programmatic approach is better.

privaloops avatar Nov 08 '22 14:11 privaloops

@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.

amtins avatar Nov 27 '22 19:11 amtins

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

video-archivist-bot avatar Nov 27 '22 19:11 video-archivist-bot

Hi @mister-ben, @gkatsev, is there anything I can do to help this PR be merged?

amtins avatar Dec 19 '22 13:12 amtins

Updating the branch as it would be great to get this merged before the next release.

adrums86 avatar Apr 01 '23 20:04 adrums86

Congrats on merging your first pull request! πŸŽ‰πŸŽ‰πŸŽ‰

welcome[bot] avatar Apr 03 '23 16:04 welcome[bot]