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

feat: only polyfill MCap for non Android-based Cast devices. #4170

Merged
merged 1 commit into from Apr 28, 2022

Conversation

avelad
Copy link
Collaborator

@avelad avelad commented Apr 27, 2022

Related to #4164 (comment)

@avelad
Copy link
Collaborator Author

avelad commented Apr 27, 2022

@joeyparrish I've run Selenium Lab Tests but they seem to fail for some reason, can you look at it?

@joeyparrish
Copy link
Member

Taking a look now

@joeyparrish
Copy link
Member

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.

@joeyparrish
Copy link
Member

joeyparrish commented Apr 27, 2022

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:

Chrome 90.0.4430.225 (Chromecast 1.56.500000): Executed 2116 of 2205 (skipped 89) SUCCESS (10 mins 53.655 secs / 10 mins 9.869 secs)

No other errors reported.

@@ -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() &&
Copy link
Member

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;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@joeyparrish joeyparrish merged commit 11321d8 into shaka-project:main Apr 28, 2022
@avelad avelad deleted the android-cast branch April 29, 2022 05:30
@avelad avelad added this to the v4.0 milestone May 4, 2022
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
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

Successfully merging this pull request may close these issues.

None yet

2 participants