-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix: Do not assume 1080p Cast devices, some are 720p #6562
fix: Do not assume 1080p Cast devices, some are 720p #6562
Conversation
maxResolution.height = 1080; | ||
// Some hub devices can only do 720p. Default to that. | ||
maxResolution.width = 1280; | ||
maxResolution.height = 720; | ||
|
||
try { | ||
if (hasCanDisplayType && await cast.__platform__.canDisplayType( |
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.
nit; Up to your preference, just in case you missed it - you might check the presence of hasCanDisplayType
just once.
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.
I tried it, but I think the extra layer of if
outside the try/catch makes the method overall harder to read and follow. The runtime difference is negligible, so I'm leaving.
maxResolution.width = 1920; | ||
maxResolution.height = 1080; | ||
// Some hub devices can only do 720p. Default to that. | ||
maxResolution.width = 1280; |
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.
Just a heads up. The physical resolution of Google Nest Hub is 1024 x 600. But I guess it can play 720p.
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.
It can decode 720p, and with the typical encoding ladder people use, you'd hate to have them play 480p instead.
Incremental code coverage: 58.33% |
No description provided.