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

configure() shouldn't switch tracks unless needed #1138

Closed
kocoten1992 opened this issue Nov 18, 2017 · 7 comments
Closed

configure() shouldn't switch tracks unless needed #1138

kocoten1992 opened this issue Nov 18, 2017 · 7 comments
Assignees
Labels
status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Milestone

Comments

@kocoten1992
Copy link
Contributor

kocoten1992 commented Nov 18, 2017

Have you read the FAQ and checked for duplicate issues: yes (with keywork buffer and switch)

What version of Shaka Player are you using: on the demo

Can you reproduce the issue with our latest release version: yes

Can you reproduce the issue with the latest code from master: not try yet

Are you using the demo app or your own custom app: in both your demo and my app

What browser and OS are you using: Chrome, ubuntu 17.10

What did you do? (there is a snippet on comment below)

  1. go to https://shaka-player-demo.appspot.com/demo/#asset=//storage.googleapis.com/shaka-demo-assets/sintel/dash.mpd;lang=en-US;noadaptation
  2. press Load, open info section
  3. switch between 256x110 and 1920x818, just do it normally, nothing special here, like
  • click on 256x110 and wait for it to play
  • then click on 1920x818 and wait for it to play
  • then click on 256x110 and wait for it to play
    ... (repeat as many times as needed until you see the bug, less than 5 times in average case, 15 at most)

What did you expect to happen?

  • calling selectVariantTrack(track, true) should clear current buffer, video pause briefly and new resolution apply immediately

What actually happened?

  • When we are at 1920x818, calling 256x110 always work.
  • When we at 256x110, calling 1920x818 doesn't always work.

It happen in my application as well, it worth notice this: my app have 5 resolution: 1080, 720, 480, 360, 240, calling for 720 or less work, just 1080 doesn't work
I set streaming bufferGoal 120s which mean, I have to endure 120s of lower resolution until it actually switch to 1080

@kocoten1992
Copy link
Contributor Author

@TheModMaker
Copy link
Contributor

@kocoten1992 Are you manually calling selectVariantTrack in your case, or are you selecting tracks in the drop-down menu on the demo? If you call selectVariantTrack, do you call configure before it?

I believe the fact it only happens on 1080p is a red herring. I can reproduce something similar during startup on 1080p, but not after a bit of playback. I think the problem is that the demo app calls player.configure() when we change tracks. This causes a track switch because of how we handle language changes. This configure call selects the 1080p track because ABR thinks it is the best track (even when ABR is disabled). Then when you select the 1080p track manually, it is already "selected", so we don't clear the buffer.

When someone calls player.configure(), we should only switch tracks if the current track selection doesn't match the new configuration. This may be related to #856.

@TheModMaker TheModMaker added type: bug Something isn't working correctly and removed needs triage labels Nov 20, 2017
@TheModMaker TheModMaker added this to the v2.3.0 milestone Nov 20, 2017
@kocoten1992
Copy link
Contributor Author

kocoten1992 commented Nov 21, 2017

@TheModMaker thank for the word 'red herring' (new word), I've create snippet serve at backup and easier debug for this problem

http://jsbin.com/veginugace/edit?html,console,output

(On this snippet, change between 110p and 546p to see the bug)

@kocoten1992
Copy link
Contributor Author

yep, that exactly what happen, when I tried to call

shakaPlayer.configure(...)

It will trigger shaka.media.StreamingEngine.prototype.switchInternal_ , and then when I call selectVariantTrack it call switchInternal_ again, fishy thing can happen!

I think I'll fix the demo, I want the badge contributor 😄, the fix gonna be like:

// from
shakaPlayer.configure({abr: {enabled: false}})

// to
if (shakaPlayer.getConfiguration().abr.enabled) {
  shakaPlayer.configure({abr: {enabled: false}})
}

@kocoten1992
Copy link
Contributor Author

kocoten1992 commented Nov 21, 2017

P/s: but should we guard again this, I think this is a trap, at first use, I didn't know call configure would mess with selectVariantTrack at all

P/s: On the other hand, if dev try the demo and find out result diff from their, they will dig up the demo, so maybe not needed to guard.........?

@TheModMaker
Copy link
Contributor

Thanks for the offer to fix, but that would not be the preferred fix. An app should be able to call configure() like that without changing tracks. We need to change configure() to not always switch tracks. Specifically it should only switch tracks when the new configuration forbids playing the current stream (e.g. due to Restrictions).

@TheModMaker TheModMaker changed the title selectVariantTrack(track, true) doesn't work on 1080p configure() shouldn't switch tracks unless needed Nov 21, 2017
@TheModMaker TheModMaker self-assigned this Dec 4, 2017
joeyparrish pushed a commit that referenced this issue Dec 5, 2017
Configure should only switch tracks if the current variant can't be
played under the new configuration.

Closes #1138

Change-Id: I1acb8bdbb0c8b41252e978bd17ef52bec1095844
@joeyparrish
Copy link
Member

Fix cherry-picked for v2.2.8.

@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 type: bug Something isn't working correctly
Projects
None yet
Development

No branches or pull requests

5 participants