-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Allow removing TextTracks created by hls.js #7515
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
Conversation
| dataToAttach.overrides = { | ||
| duration: schedule.duration, | ||
| endOfStream: !isAssetPlayer || isAssetAtEndOfSchedule, | ||
| cueRemoval: !isAssetPlayer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a good chance this will need to be added back. This is about primary cues not being removed when sharing the media element with interstitial asset player instances that append content over ranged of primary content. In these cases we want to maintain all TextTracks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've noticed that cueRemoval currently only affects ID3 removal. It's easy to revert this, but doing so would require retaining the TextTrack reuse mechanism. Therefore, I might need to be more explicit about when to reuse an existing TextTrack. Previously, TextTracks were never deleted, so the default behavior was to reuse existing items. Now, reuse is only required in specific circumstances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. So, I might want to consider making ID3 track reuse more standard based on attach/detach/transfer media behavior (reuse in transfer, tear down on full detach). Thanks, that helps me think about testing and changes following review and merging.
I plan to do a formal review after some testing this week, with the hopes of merging these changes before merging other work. Thanks for the patience and timely response to feedback.
|
There's a problem in Safari. When a TextTrack's mode is set to "disabled" or "hidden" and then "showing" again, all the cues have been removed. |
Thanks, I just confirmed that. And it seems very weird. Maybe I'll need to take some time for investigating. |
|
@robwalch This issue should be fixed by now. The data url looks magical. |
@xxoo Should we add the same in Shaka Player? |
|
@avelad I think Shaka Player probably doesn't need this since it always clears the existing cues when switching subtitles. But adding this makes Safari's behavior consistent with other browsers, so it's better to add it. |
Allow removing TextTracks created by hls.js
This PR will...
appendChildinstead ofaddTextTrackto allow reverse operationtrackNodefield inMediaPlaylistto indicate associatedTextTrackMEDIA_DETACHEDevent in wrong order and sometimes not triggeringWhy 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