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

Player reset() function causes text track issues #5140

Closed
DougGore opened this issue May 4, 2018 · 4 comments · May be fixed by CleverTap/vue-video-player#3
Closed

Player reset() function causes text track issues #5140

DougGore opened this issue May 4, 2018 · 4 comments · May be fixed by CleverTap/vue-video-player#3

Comments

@DougGore
Copy link

DougGore commented May 4, 2018

Description

The reset() function in the Player class appears to have seemingly intentional but undesired side effects. As part of the unloading and reloading of the tech plugin the unloadTech, loadTech functions take a JSON snapshot of the associated text tracks prior to the reset and restores them post reset. Unfortunately when using video.js with plugins like DASH and HLS which usually manage their own text tracks they are removing them during the tech plugin dispose stage and having them subsequently restored afterwards without the plugin itself being aware of this.

I have reproduced this in a simplified test case here, simply play the video and press "Click me". You will see an accumulation of subtitle tracks in the subtitle selector.

In the case of videojs-contrib-dash you may need to pull this change in as well: videojs/videojs-contrib-dash#251

Steps to reproduce

Explain in detail the exact steps necessary to reproduce the issue.

  1. Have a video with subtitle loaded
  2. Invoke player.reset()
  3. Load another stream or the same stream again.

Results

Expected

The expected outcome is that only the subtitle tracks associated with the new video should be displayed.

Actual

The subtitle tracks from the original video and additionally the new video will both be displayed.

Error output

No errors seen, this appears to be intentional behaviour.

Additional Information

Please include any additional information necessary here. Including the following:

versions

videojs

6.8

browsers

Tested with DASH on Chrome and native HLS on Safari

OSes

Test with Chrome on Windows 10 and Safari on OSX

plugins

videojs-contrib-dash (for DASH)
None for HLS

@gkatsev
Copy link
Member

gkatsev commented May 4, 2018

Having reset remove text tracks sounds like the correct behavior. Would you be interested in making a PR for that? I'd be happy to help you figure out what needs to happen.

@DougGore
Copy link
Author

DougGore commented May 4, 2018

Sure, I can pick this up early next week. From studying the source I think the text track backup/restore code is only used as part of the unloadTech and loadTech functions so this should be mostly a case of deleting code.

@gkatsev
Copy link
Member

gkatsev commented May 4, 2018

Maybe, though, we do want to make sure that text tracks are kept around during tech switches outside of player.reset(). If you have any questions or for more real-time chat, drop in by our slack when you're working on this.

@gstrat88
Copy link
Contributor

gstrat88 commented Jul 17, 2018

There is a bigger problem regarding hls, reset and text tracks.
Hls saves segment metadata as text tracks, so each time a reset is called these text tracks are copied. After some loads and resets the copied json is very big, causing reset to halt because of the addRemoteTextTrack execution. Solution is easy, just call tech().clearTracks("text") before reset as in my situation there are no "real" text tracks. But it seems this is all part of plot hole.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants