fix: user can pick variant track/texttrack from streaming event

[kocoten1992]

Description

fix #3448

in 2.5.1, we can pick variant track from manifest alone.

this functionality is broken from v3.

this patch allowed user to pick variant track from streaming event.

Type of change

• Bug fix (non-breaking change which fixes an issue)

Checklist:

• I have signed the Google CLA https://cla.developers.google.com

[joeyparrish]

I think this fix is too broad. I'm not convinced that it's correct to keep the active track for all the various ways and times that chooseVariant_ is called.

Instead, you should make the call right after the streaming event conditional. If there's no active track, you would call chooseVariant_. Otherwise, skip it.

Does this help?

[kocoten1992]

@joeyparrish is this what you mean ? PTAL.

[kocoten1992]

hi @joeyparrish , thanks for the suggestion, js have such a nice gem, also I've fix style - it pass python3 build/check.py, PTAL

[kocoten1992]

hi @joeyparrish, PTAL

[kocoten1992]

@joeyparrish , PTAL, there is one fail test, https://github.com/google/shaka-player/blob/master/test/player_unit.js#L2001-L2005, I've not dig into it yet, but is it fail because this is a new case for it ?

Chrome 91.0.4472.101 (Linux x86_64) Player tracks chooses the configured text language and role at start FAILED
        Expected $.id = 50 to equal 52.
        Expected $.language = 'es' to equal 'en'.
	Expected $.roles.length = 0 to equal 1.
	Expected $.roles[0] = undefined to equal 'commentary'.
	Error: Expected $.id = 50 to equal 52.
	Expected $.language = 'es' to equal 'en'.
	Expected $.roles.length = 0 to equal 1.
	Expected $.roles[0] = undefined to equal 'commentary'.
	    at <Jasmine>
	    at _callee33$ (test/player_unit.js:2001:36 <- test/player_unit.js:2821:44)
	    at tryCatch (node_modules/babel-polyfill/dist/polyfill.js:6900:40)
	    at Generator.invoke [as _invoke] (node_modules/babel-polyfill/dist/polyfill.js:7138:22)

[joeyparrish]

Sorry I overlooked your previous comment. Your change breaks an existing test case. As far as I can tell, the test case is still valid, and you've changed the behavior. The test case shows that the initial choice is based on the configuration of language and role preferences. This is an important behavior.

I will see if I can identify the issue, and if so, I'll suggest a fix.

[shaka-bot]

Test Failure:

Chrome 91.0.4472.114 (Linux x86_64)
  Player tracks chooses the configured text language and role at start FAILED
    Expected $.id = 50 to equal 52.
    Expected $.language = 'es' to equal 'en'.
    Expected $.roles.length = 0 to equal 1.
    Expected $.roles[0] = undefined to equal 'commentary'.
    Error: Expected $.id = 50 to equal 52.
    Expected $.language = 'es' to equal 'en'.
    Expected $.roles.length = 0 to equal 1.
    Expected $.roles[0] = undefined to equal 'commentary'.
        at <Jasmine>
        at _callee33$ (test/player_unit.js:2001:36 <- test/player_unit.js:2821:44)
        at tryCatch (node_modules/babel-polyfill/dist/polyfill.js:6900:40)
        at Generator.invoke [as _invoke] (node_modules/babel-polyfill/dist/polyfill.js:7138:22)

Failed 1 tests, passed 2097, skipped 33

[joeyparrish]

What I'm seeing as I debug this is:

1. Before the test runs, there is a beforeEach section that sets a default config and loads default content that is common to most tests in this group.
2. In this particular test, we then set a different config and reload the content, to show that the config has an effect.
3. When this happens, Player thinks that it already has active streams, and therefore doesn't choose new ones based on the config. This seems wrong, and like it is probably not your fault.
4. This happens even if I await player.unload() at the top of the test. This seems wrong, too, and also unlikely to be caused by your change.

I'm trying to track down why this happens. But I'm fairly sure it has always been this way, and that your change merely uncovered it. Without your change, we would always re-evaluate the initial track selection, so the fact that there was an "active" track from a previous load() did not matter before.

[joeyparrish]

Everything I said above is only true in the environment of this unit test. There, we override streamingEngine with a fake, and one that persists. Therefore the active tracks are persisting as well. I need to fix the fake to release its track list when destroy() is called. That should make it behave more like the real thing, and that should make the test pass again.

[joeyparrish]

I have fixes for this in review internally. When they are merged, I will merge this PR. Thanks!

[joeyparrish]

Thanks!