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

Getting text tracks immediately after selecting text tracks returns the wrong active track #147

Closed
sanbornhilland opened this issue Aug 4, 2015 · 9 comments
Assignees
Labels
status: archived Archived and locked; will not be updated
Milestone

Comments

@sanbornhilland
Copy link
Contributor

  1. Call player.selectTextTrack(id) where id is not the currently active track
  2. Immediately call player.getTextTracks()

The result is that in the returned array the track that is set to active=true is the original text track and not the track you tried to set to. NOTE: the text tracks are displaying and switching fine. But it looks like because the text_stream switching function is promise based, it's not guaranteed that right after you select a track it will show as active when you retrieve the array of tracks. This is fine but it doesn't look to me like there is any check you can do to determine that the text_stream.switch() has resolved (no promise is returned and no adaptation event is fired) so you can't be sure when you call getTextTracks it will be accurate.

@tdrews
Copy link
Contributor

tdrews commented Aug 4, 2015

There are definitely some improvements that we can make here. Perhaps one way of going about it is

  1. As audio and video tracks become active before an AdaptationEvent is fired, we could instead make audio and video tracks active immediately before firing an AdaptationEvent (or some type of TrackSwitchedEvent).
  2. We could fire some type of TrackSwitchedEvent immediately before a new text track becomes active.

I think this would make things consistent (i.e., Adaptation/TrackSwitched events would only be fired once that track begins presenting) and also enable applications to detect when a new text track begins presenting. I'm open to other options though; any concerns with this approach?

@vasanthap
Copy link
Contributor

I actually tested this scenario a while back, we had a good reason why we thought the behavior was expected (sorry I cannot remember it now) and that is why delay was added to these tests.

With that having said, I like your suggestion. If you implement that, please add/update integration test for the scenario.

@sanbornhilland
Copy link
Contributor Author

I'm not quite clear on (1).

(2) sounds good. I'm not convinced there is reason to add another event unless there is a specific reason why you don't want to fire adaptation events when text tracks switch. So the simplest might be to just fire adaptation events on text track switching as well as audio and video. But maybe I'm missing something here.

@tdrews
Copy link
Contributor

tdrews commented Aug 5, 2015

With regards to (1), audio and video tracks are considered active once their metadata is fetched, and although this is asynchronous, it's nearly instantaneous since the metadata will have already been fetched from StreamVideoSource. However, an AdaptationEvent is not fired until Stream.onUpdate_() is called, which may occur a little after the track becomes active. So, I think we can make this more consistent by syncing up track activation and AdaptationEvent firing.

With regards to (2), I was thinking that AdaptationEvent is becoming a bit of a misnomer, since switching audio languages or subtitle tracks isn't necessarily adapting to a different bit-rate. However, on second thought, it's probably best that we don't change the event name/type yet. We can just fire an AdaptationEvent from TextStream as you suggested.

@sanbornhilland
Copy link
Contributor Author

O, ic. Yes it is becoming a misnomer. I'm still of the mind though that only one event is needed. Perhaps it could be called something like TrackUpdateEvent. And its fired when any changes happen to any of the tracks for whatever reason. Perhaps that's too blunt, I'm not sure, and more fined grained control could be required.

@dougdoe
Copy link

dougdoe commented Aug 12, 2015

Hi @tdrews, I'm just wondering if you are planning to move forward with the suggestions mentioned above? If so, is it possible to assign and set a milestone, assuming you have one in mind? (We have an outstanding issue on our side that I'm attempting to organize.)

Also, is there any suggested workaround in the meantime? Thanks.

@tdrews
Copy link
Contributor

tdrews commented Aug 12, 2015

We'll do this for v1.5.0.

@tdrews tdrews added this to the v1.5.0 milestone Aug 12, 2015
@TheModMaker TheModMaker self-assigned this Aug 14, 2015
@tdrews
Copy link
Contributor

tdrews commented Aug 18, 2015

Note: we are considering track activation timing consistency as a separate issue but have not decided yet how we want to address it.

@sanbornhilland
Copy link
Contributor Author

Okay, thanks for the heads up. For now I am going to recommend avoiding calling getTextTracks() immediately after selecting a new track and instead, syncing on adaptation events.

tdrews pushed a commit that referenced this issue Aug 24, 2015
* Make IStream more precise by adding more documentation.
* Make misc. docs related to Stream more consistent.
* Remove |minBufferTime| from IStream; use a configuration
  option instead.

Issue #147

Change-Id: Ie6261ced49a8f815d270e0ee94af5dc40acff63f
@shaka-project shaka-project locked and limited conversation to collaborators Mar 22, 2018
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Apr 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Development

No branches or pull requests

6 participants