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

Fixed many issues with subtitle handling #18

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bp2008
Copy link

@bp2008 bp2008 commented Mar 15, 2021

Summary

Most of the subtitle handling was being inherited from HTML5Video playback which is fundamentally incompatible with the way hls.js wants to do things. It caused many issues. This pull request fixes all the issues I found.

clappr/clappr#2003

Changes

Overrode several properties from the HTML5Video playback with hlsjs-specific implementations. The fixes are rather extensive; like I said, the existing subtitle implementation was fundamentally incompatible with hls.js. All issues I reported here are resolved by this pull request.

  • Fixed issue with cc-button not appearing unless DEFAULT=YES attribute was added to one of the subtitle tracks in the HLS manifest. The problem was, hlsjs-playback was not listening to the right hls.js events, and therefore was unaware that subtitles were available unless hls.js automatically activated a subtitle track.
  • Fixed issue with last subtitle track remaining active and visible when it should not (when there is more than one subtitle track).
  • Fixed synchronization issues between cc-button GUI state and actual hls.js subtitle controller state. (see related PR in clappr-plugins)
  • Added handling of the HLS EXT-X-MEDIA tag attributes DEFAULT, AUTOSELECT, and FORCED. hls.js had some primitive handling of DEFAULT but this implementation is better, and effectively overrides hls.js default behavior.
  • Adds a new attributes field to text tracks exposed via the playback's closedCaptionsTracks property.
    • Example attributes: { autoselect: true, default: true, forced: false }
    • This attributes object is utilized (if it exists) by the closed_captions plugin to add a ' (forced)' suffix to labels of forced subtitle tracks. (see related PR in clappr-plugins).

How to test

I've created a sample HLS video which can be used to test HLS playback with two subtitle languages.

TestPattern-min.zip

When extracted into the clappr repo's public folder, it can be loaded on the test page (npm run start) by entering the relative URL to index.m3u8.

Prior to the changes in this pull request, there are many issues with the subtitle handling as described in the issue I posted last week. After applying this pull request, the issues are resolved and subtitles work as intended.

Most of the subtitle handling in hlsjs-playback was inherited from the HTML5Video playback, which is fundamentally incompatible with the way hls.js wants to do things.  This commit uses hls.js Subtitle Tracks Control API and related events to solve every issue I could find.  This commit also adds better handling of `forced`,`default`,`autoselect` attributes than hls.js provides natively.
thiagopnts added a commit that referenced this pull request Oct 19, 2022
Bumped from 0.14.17 to 1.2.4. Didn't test extensively, but playback
seems fine, only noticed an issue with captions, which seems to be
related to #18
Bumping as a major to avoid this version being picked up by the other
packages, since most I saw are pulling minors
thiagopnts added a commit that referenced this pull request Oct 19, 2022
Bumped from 0.14.17 to 1.2.4. Didn't test extensively, but playback
seems fine, only noticed an issue with captions, which seems to be
related to #18
Bumping as a major to avoid this version being picked up by the other
packages, since most I saw are pulling minors

lint
@xemles
Copy link

xemles commented Dec 26, 2023

What has happened to this PR?
I seem to have very similar issues.

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.

None yet

2 participants