Allow removing TextTracks created by hls.js

[xxoo]

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

[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]

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.

[xxoo]

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.

[robwalch]

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.

[robwalch]

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.

[robwalch]

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]

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.

[xxoo]

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

[robwalch]

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.

[xxoo]

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.

[xxoo]

@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

[robwalch]

Why were the track kind tests removed?

[xxoo]

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

[robwalch]

Hi @xxoo,

Sorry for the delay in getting to this. Can you please resolve the merge conflicts by rebasing these changes?

[xxoo]

@robwalch
Resolved. All unit tests passed locally.

[robwalch]

Hi @xxoo,

Take a look at the diff. I think something went wrong in your branch rebase. This PR should not include 106 file changes.

[xxoo]

Sorry seems I used merge in some step. Maybe I'll have to start a new PR to fix this.

[xxoo]

All changes in this PR is moved to #7515.