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

Cannot addTextTracks after switching off of the HTML tech #2367

Closed
dmlap opened this issue Jul 16, 2015 · 15 comments
Closed

Cannot addTextTracks after switching off of the HTML tech #2367

dmlap opened this issue Jul 16, 2015 · 15 comments
Assignees
Milestone

Comments

@dmlap
Copy link
Member

dmlap commented Jul 16, 2015

Steps to Reproduce

  1. Create a player with a text track
  2. Load it on a browser that will use the HTML tech
  3. Load a video that will force a switch to the flash tech
  4. Call player.addTextTrack()

An error will be thrown because the flash tech will attempt to use the native text track list created by the HTML tech.

@dmlap dmlap added this to the v5.0.0 milestone Jul 16, 2015
@gkatsev gkatsev self-assigned this Jul 16, 2015
@gkatsev
Copy link
Member

gkatsev commented Jul 17, 2015

What should we do with the available text tracks? Delete them or recreate them? I guess this does end up falling in with #2369

@heff
Copy link
Member

heff commented Jul 17, 2015

Maybe when we unload a tech we should record all the values of the existing tracks, so they can be added back as native or custom tracks when the next tech is loaded? That would seem to at least match how the video element works. Tracks don't disappear on a source change. You could add all the tracks first before adding a source.

@gkatsev
Copy link
Member

gkatsev commented Jul 17, 2015

It kind of is doing so. I guess it needs to do it differently. The issue here is that we're literally grabbing the text track list element from the previous tech and trying to use it in the next tech which doesn't work if you're going from html to flash.

@heff
Copy link
Member

heff commented Jul 17, 2015

Yeah, sounds like we need to record the values (all attributes and cues), not keep the tracks themselves around.

@gkatsev
Copy link
Member

gkatsev commented Jul 20, 2015

Do we want to preserve the in-band metadata tracks?
I guess I'll just preserve everything in my first pass and we can decide on what to keep or not later in the PR.

@heff
Copy link
Member

heff commented Jul 20, 2015

Ultimately I don't think we should. If the user didn't add them then they wouldn't know to remove them when changing the source.

@gkatsev
Copy link
Member

gkatsev commented Jul 20, 2015

So, I just realized what we're doing here isn't what we actually want to do.
TextTrackList has several events listeners that can be added to it. If we copy it over between techs, we lose that event listener. Natively, you would still have the same listener applied after you change sources.
One thing I can think of for this is to always use our emulated text track list so that we always get events but have it proxy to the native text track list object if available and clean itself out on tech change.

@gkatsev
Copy link
Member

gkatsev commented Jul 20, 2015

And this actually would apply to the text tracks themselves, too.

@heff
Copy link
Member

heff commented Jul 20, 2015

One thing I can think of for this is to always use our emulated text track list so that we always get events

Yeah, I wondering if we were going to need to go that direction. It sounds ok to me, but I defer to your judgement.

And this actually would apply to the text tracks themselves, too

You're saying we would always need to use our emulated tracks? Why's that?

@gkatsev
Copy link
Member

gkatsev commented Jul 20, 2015

tl;dr: it depends on how disruptive we want to be to users while still using as much native functionality as possible.


Not saying we always want emulated tracks, but it depends on how far down we want to go. Text track objects have some the cuechange event on them that we can't copy if it was added via addEventListener. We also, can't move a text track object between two video elements. You get a brand new track.
Cues are, possibly, the only things that have events that we can actually move from one track to another and we don't currently support events for them right now anyway.
The question is how far down we want to go, really. I think that always having the emulated text track list might be good enough for the 80% usecase. It has the major events that are most important like addtrack, removetrack, and change. We could document that if you're using videojs, you should make sure to re-attach listeners to tracks when sources change, though, we could also possibly just document that you always need to re-register all events (that is, cuechange), but that isn't very user-friendly.

@heff
Copy link
Member

heff commented Jul 20, 2015

Wow yeah, there's a lot to consider...

Ideally the tracks would work as if you were just using the video element, including when switching sources. With the new details it feels like that'd be a lot to build. Our emulated tracks would need to be able to plug into a native track and proxy everything, or emulate everything, and translate between the two when switching sources to maintain the listeners. Tracks from source handlers might make this a little more complicated, since it's not the user adding the track. It seems possible I'm not sure how far we should try to take it.

Part of me wants to say screw it, and just clear all tracks when changing sources, but that would cause issues with advertising and other concatenation workflows.

@gkatsev
Copy link
Member

gkatsev commented Jul 20, 2015

I'm about 90% done with copying the tracks between techs.

@heff
Copy link
Member

heff commented Aug 20, 2015

@gkatsev is this issue covered now with the related PRs merged?

@gkatsev
Copy link
Member

gkatsev commented Aug 20, 2015

I believe this is fixed now.

@heff
Copy link
Member

heff commented Aug 20, 2015

@dmlap want to confirm or close?

@dmlap dmlap closed this as completed Aug 24, 2015
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 27, 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

No branches or pull requests

3 participants