-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: only polyfill MCap for non Android-based Cast devices. #4170
Conversation
@joeyparrish I've run Selenium Lab Tests but they seem to fail for some reason, can you look at it? |
Taking a look now |
Looks like I let Karma's TLS certificate expire. Another bit of automation I haven't finished yet. 😐 It has been updated now. I'll start another test run. |
We're still dealing with some lingering infrastructure issues in the lab. Xbox disconnected (known issue), Chrome on Linux failed early with a confusing error about polyfill installation, and the Tizen TV seems to be offline (I'll go to the office and see what is wrong with it). Everything passed on Chromecast, though:
No other errors reported. |
lib/polyfill/media_capabilities.js
Outdated
@@ -37,7 +37,7 @@ shaka.polyfill.MediaCapabilities = class { | |||
// See: https://github.com/shaka-project/shaka-player/issues/3582 | |||
// TODO: re-evaluate MediaCapabilities in the future versions of PS5 | |||
// Browsers. | |||
if (!shaka.util.Platform.isChromecast() && | |||
if (!shaka.util.Platform.isAndroidCastDevice() && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems wrong. On Chromecast Ultra !isAndroidCastDevice()
is true
. So now we would be using native MCap on Chromecast Ultra, which is the opposite of what we want. In fact, you've excluded Android Cast devices from native MCap instead of including them.
I think the sense of this check is confusing. Let's rewrite it.
I suggest a local variable called canUseNativeMCap
:
let canUseNativeMCap = true;
if (isApple() || isPS5() ||
(isChromecast() && !isAndroidCastDevice())) {
canUseNativeMCap = false;
}
Or this, which may be easier to read:
let canUseNativeMCap = true;
if (isApple() || isPS5() || isChromecast()) {
canUseNativeMCap = false;
}
if (isAndroidCastDevice()) {
canUseNativeMCap = true;
}
Then you get this later:
if (canUseNativeMCap && navigator.mediaCapabilities) {
log('Native found');
return;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Related to #4164 (comment)