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

feat: Add an option to support forced subtitles #1329

Merged
merged 2 commits into from
Apr 3, 2023

Conversation

amtins
Copy link
Contributor

@amtins amtins commented Sep 22, 2022

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

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

@welcome
Copy link

welcome bot commented Sep 22, 2022

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

@gkatsev
Copy link
Member

gkatsev commented Oct 6, 2022

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

@codecov
Copy link

codecov bot commented Oct 6, 2022

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%) ⬆️

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

@amtins amtins force-pushed the feat/forced-subtitles-support branch from 2540b44 to 91fa308 Compare November 7, 2022 08:59
@privaloops
Copy link

lovely, please merge !

@mister-ben mister-ben changed the title Add an option to support forced subtitles feat: Add an option to support forced subtitles Nov 8, 2022
@mister-ben
Copy link
Contributor

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.

@privaloops
Copy link

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

@amtins
Copy link
Contributor Author

amtins commented Nov 27, 2022

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

@video-archivist-bot
Copy link

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:

@amtins
Copy link
Contributor Author

amtins commented Dec 19, 2022

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

@amtins amtins force-pushed the feat/forced-subtitles-support branch from 91fa308 to 2540b44 Compare February 16, 2023 15:24
Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

Sorry for the delay on your PR @amtins! It looks reasonable to me, but I'm not a playback expert.

Maybe I could get @adrums86 or @dzianis-dashkevich to have a look?

Copy link
Contributor

@dzianis-dashkevich dzianis-dashkevich left a comment

Choose a reason for hiding this comment

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

LGTM

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.

My apologies for coming in late with this review. I had previously looked over this PR and simply forgot the final step, my approval 😁 ! Nice work, your contribution is greatly appreciated.

@adrums86
Copy link
Contributor

adrums86 commented Apr 1, 2023

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

@adrums86 adrums86 merged commit 6bd98d0 into videojs:main Apr 3, 2023
@welcome
Copy link

welcome bot commented Apr 3, 2023

Congrats on merging your first pull request! 🎉🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants