Allow removing TextTracks created by hls.js #7184

xxoo · 2025-04-17T19:06:14Z

Allow removing TextTracks created by hls.js

This PR will...

use appendChild instead of addTextTrack to allow reverse operation
replace track flush and reuse workflow with simple removal
add trackNode field in MediaPlaylist to indicate associated TextTrack
fix MEDIA_DETACHED event in wrong order and sometimes not triggering

Why is this Pull Request needed?

This PR allows video elements to have the same lifecycle as hls.js instances. After detaching, you may attach them to new hls.js instances with nothing to worry about. It also fixed a bug caused by track reuse, where content overlapping occurs if a video contains multiple captions sharing the same language and label. eg: https://devstreaming-cdn.apple.com/videos/streaming/examples/bipbop_16x9/bipbop_16x9_variant.m3u8

Are there any points in the code the reviewer needs to double check?

Yes. Since the changes involve more than one place, extensive testing from multiple aspects may be necessary to ensure no issues are left behind. Especially I'm not sure whether it works well with transferMedia()

Resolves issues:

#2198

Checklist

changes have been done against master branch, and PR does not conflict
new unit / functional tests have been added (whenever applicable)
API or design changes are documented in API.md

- replace track flush and reuse workflow with simple removal - add `trackNode` field in `MediaPlaylist` to indicate associated `TextTrack` - fixed `MEDIA_DETACHED` event in wrong order and sometimes not triggering

robwalch

Thank you for the contribution @xxoo,

The ability to remove tracks would be appreciated. However, for backwards compatibility, we need to maintain addTextTrack as the default path for adding TextTracks.

There may be JavaScript platforms with limited Web support cannot create TextTrack nodes. There are also browsers where media.textTracks from DOM created tracks do not behave the same (HTMLMediaElement controls menu issues, events, etc...). One concern is that apps may provide an element with tracks already added by another means or they may add their own side car tracks and this change would introduce problems with those integrations.

Would be possible to make this change optional via a configuration option (use addTextTrack by default, but set an option to use DOM tracks instead)?

robwalch · 2025-04-17T19:20:46Z

src/utils/texttrack-utils.ts

+  // To avoid an issue of the built-in captions menu in Chrome
+  if (navigator.userAgent.includes('Chrome/')) {
+    el.src = 'data:,WEBVTT';
+  }


Interesting.

Is this to prevent loading of the current page (src="" treated like "." path)?
What other issues occur with the menu?

I recall there was some issue in Safari with DOM created tracks but it has been long enough that I don't remember the details.

So far, I haven't seen any issue related to DOM tracks on Safari. However, the issue you found on Chrome still exists. Providing a valid VTT file might help to avoid this, and the data URL approach serves exactly this purpose.

Subtitle tracks not managed by hls.js shouldn't cause problems, or at least such problems can be avoided. Because hls.js keeps a reference to each track it manages. If a user selects an unmanaged track, hls.js will leave it as is and set the current subtitle track index to -1. I think I can add some unit tests for this scenario.

According to caniuse.com, the track element and the addTextTrack method have nearly identical browser support. The only difference I've noticed is that a newly created track node might need some time to initialize its mode. I'm not sure if this behavior affects any public API usage. Perhaps we should further analyze the necessity and implications of this, as maintaining the old approach could complicate the workflow.

The only difference I've noticed is that a newly created track node might need some time to initialize its mode.

We should also have a look at whether or not these changes impact performance WRT #7093. I'd be curious to know if adding tracks this way has the same cost in Safari or not. If it resolved the issue without any changes to WebKit that would be another +1 for accepting the change.

performance WRT #7093

~~No difference.~~ In each session that I profile, there appears to be a fixed cost for each track added in Safari that can be seen in the profiling results under "Add Track Event Dispatched" Timelines tab of the Web Inspector.

That cost does appear to be reduced when appending tracks nodes with mode disabled at once compared to using addTextTrack.

src/controller/subtitle-track-controller.ts

src/controller/timeline-controller.ts

robwalch · 2025-04-18T22:47:13Z

According to caniuse.com, the track element and the addTextTrack method have nearly identical browser support.

That does not guarantee that these changes will not break someone's integration. There could be integrations of hls.js out there that depend at some level on media.addTextTrack getting called. We can't know until releasing these changes, but don't be surprised. Someone might come out of nowhere saying that ownerDocument in undefined on their media element.

That said, I marked this under milestone v1.7. This is really nice work.

xxoo · 2025-04-19T17:51:51Z

Thanks for the review. I will fix these issues soon. I also think removable tracks may help improve performance slightly when there are too many text tracks and more than one text group, as we can destroy tracks outside current groups. Shaka Player avoids this problem by adding only one text track, meaning the native captions UI or API isn't available as an option for users.

- Replace for...of with forEach - Only create track nodes for current groups - No longer depend on event order - Behavior change: Leave unmanaged tracks to the user. The result of the previous behavior is difficult to predict when there are unmanaged tracks.

xxoo · 2025-04-20T23:43:24Z

@robwalch
I just updated this PR with the following changes:

Replace for...of with forEach
Only create track nodes for current groups
No longer depend on event order
Adjusted the way hls.js handles external subtitles to fix unexpected behavior

You may reproduce the issue on master branch with the unit test "should set subtitleTrack to -1 and keep unmanagedTrack showing"

This reverts commit 3f325d0.

robwalch · 2025-04-21T16:45:13Z

src/controller/subtitle-track-controller.ts

-      if (lastTrack) {
-        // switch to -1


SUBTITLE_TRACK_SWITCH is only expected to trigger when there is a track change. The thinking here is that if this.currentTrack (lastTrack) is null then it is not set , trackId must also be -1 already, and setting subtitleTrack to -1 is a no-op.

Yes this detection was added by me. But it breaks the unit test "should trigger SUBTITLE_TRACK_SWITCH if passed -1" (which I removed before for easier debugging, but forgot to add it back).
In the past, setting to -1 when it is already -1 may still change the mode of external subtitles in toggleTrackModes(). But this behavior has changed now. Which makes setting to -1 in this situation have no effect. I removed this line only for passing this unit test. Although I totally agree with you. If this unit test is no longer necessary (I believe it is true, but maybe it is not for me to decide), I'd also like to add this detection back.

@robwalch I just reverted this commit and changed some logic to avoid issues that I found.

Support forced subtitle on Apple webkit

Prefer previously selected track in change event of video.textTracks

Disable new text tracks before appending and show the selected one after that to avoid state out of sync issue of the Chrome built-in controls

…oo/hls.js into feature/removable-text-tracks Prefer previously selected track in change event of video.textTracks Disable new text tracks before appending and show the selected one after that to avoid state out of sync issue of the Chrome built-in controls

robwalch · 2025-05-22T22:52:42Z

tests/unit/controller/timeline-controller.js

-    });
-  });
-
-  describe('text track kind', function () {


Why were the track kind tests removed?

Because TimelineController no longer creates TextTrack for MediaPlaylist. Instead, the life cycle of every <track> element is managed by SubtitleTrackController now. There is an equivalent test in tests/unit/controller/subtitle-track-controller.ts

- use appendChild instead of addTextTrack to allow reverse operation

25a670b

- replace track flush and reuse workflow with simple removal - add `trackNode` field in `MediaPlaylist` to indicate associated `TextTrack` - fixed `MEDIA_DETACHED` event in wrong order and sometimes not triggering

robwalch requested changes Apr 17, 2025

View reviewed changes

robwalch added this to the 1.7.0 milestone Apr 17, 2025

robwalch reviewed Apr 18, 2025

View reviewed changes

xxoo added 2 commits April 21, 2025 05:11

- sync api.md

8914c73

xxoo requested a review from robwalch April 20, 2025 23:43

xxoo added 2 commits April 21, 2025 14:37

fix SUBTITLE_TRACK_SWITCH event

3f325d0

Revert "fix SUBTITLE_TRACK_SWITCH event"

4994c33

This reverts commit 3f325d0.

robwalch reviewed Apr 21, 2025

View reviewed changes

Merge branch 'master' into feature/removable-text-tracks

a5fd198

satyam73 mentioned this pull request Apr 28, 2025

Setup eslint no for loops rule #7202

Merged

3 tasks

xxoo requested a review from robwalch April 29, 2025 14:05

robwalch added this to HLS.js Release Planning and Backlog Apr 29, 2025

github-project-automation bot moved this to Top priorities in HLS.js Release Planning and Backlog Apr 29, 2025

Update subtitleDisplay state in change event of TextTrackList

8754b2e

robwalch reviewed May 22, 2025

View reviewed changes

Allow removing TextTracks created by hls.js #7184

Are you sure you want to change the base?

Allow removing TextTracks created by hls.js #7184

Uh oh!

Conversation

xxoo commented Apr 17, 2025

This PR will...

Why is this Pull Request needed?

Are there any points in the code the reviewer needs to double check?

Resolves issues:

Checklist

Uh oh!

robwalch left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robwalch Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

xxoo Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

robwalch Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

robwalch Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robwalch commented Apr 18, 2025

Uh oh!

xxoo commented Apr 19, 2025

Uh oh!

xxoo commented Apr 20, 2025

Uh oh!

robwalch Apr 21, 2025

Choose a reason for hiding this comment

Uh oh!

xxoo Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xxoo Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robwalch May 22, 2025

Choose a reason for hiding this comment

Uh oh!

xxoo May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

robwalch left a comment •

edited

Loading

robwalch Apr 18, 2025 •

edited

Loading

xxoo Apr 21, 2025 •

edited

Loading

xxoo Apr 29, 2025 •

edited

Loading

xxoo May 23, 2025 •

edited

Loading