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

Include Panasonic Smart TVs and other old browsers in positive DTS gate #4780

Conversation

jeremies
Copy link

@jeremies jeremies commented Jul 11, 2022

This PR will...

Prevent us from using negative DTS values in AVC moof atoms in Panasonic Smart TVs.

Why is this Pull Request needed?

Some new Panasonic Smart TVs have a navigator.userAgent string like this one:

HbbTV/1.4.1 (;Panasonic;VIERA 2020;a.393;4301-0003 0008-0000;) PanasonicSDK/2017

These Smart TVs use an old version of Chrome like Chrome 51, and this browser needs the positive DTS gate. It's like the following pull requests for Panasonic Smart TVs:

#3224
#2995

Are there any points in the code the reviewer needs to double check?

Resolves issues:

Checklist

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • API or design changes are documented in API.md

@OrenMe
Copy link
Collaborator

OrenMe commented Jul 11, 2022

Suggest to also expose a config value so we won't need to patch version each time we discover an exotic user agent or other condition

@robwalch
Copy link
Collaborator

robwalch commented Jul 11, 2022

Maybe it makes sense to require positive DTS by default. Then the user agent check can be for allowing negative DTS specifically where mainstream browsers can take advantage of it? The reason for defaulting this was aimed at Chrome and reducing start gaps in modern web browsers, but it should not come at the cost of stability in older platforms that we failed to match in the UA check.

@robwalch robwalch added this to Top priorities in Release Planning and Backlog via automation Jul 11, 2022
Comment on lines 74 to 77
requiresPositiveDts =
(!!chromeVersion && chromeVersion < 75) ||
(!!safariWebkitVersion && safariWebkitVersion < 600);
(!!safariWebkitVersion && safariWebkitVersion < 600) ||
isPanasonicTV;
Copy link
Collaborator

Choose a reason for hiding this comment

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

To avoid another UA check how about reversing the gate?

    requiresPositiveDts = !(
      (!!chromeVersion && chromeVersion >= 75) ||
      (!!safariWebkitVersion && safariWebkitVersion >= 600)
    );

Copy link
Author

Choose a reason for hiding this comment

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

It makes sense for me to reverse the gate. I'll push the reversed gate on this branch.

@jeremies jeremies force-pushed the bugfix/use-positive-dts-with-panasonic-smart-tvs branch from 9be29cc to dfa9e51 Compare July 12, 2022 06:56
@jeremies jeremies changed the title Include Panasonic Smart TVs in positive DTS gate Include Panasonic Smart TVs and other old browsers in positive DTS gate Jul 12, 2022
@robwalch robwalch added this to the 1.2.0 milestone Jul 14, 2022
@robwalch robwalch merged commit fc2243e into video-dev:master Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Release Planning and Backlog
  
Top priorities
Development

Successfully merging this pull request may close these issues.

None yet

3 participants