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

Set iOS default frame rate to screen max. #29797

Merged
merged 11 commits into from
Apr 1, 2022
Merged

Conversation

cyanglaz
Copy link
Contributor

The default iOS frame rate is set to 60 by default. This PR explicitly sets the preferred rate to the maximum refresh rate supported by the display. (120 on iPhone 13 pro, 60 on slower devices)
On iOS 15 and above, we also set the frame rate range, where the preferred and max are both screen max, the min is max/2.
Also, we avoid setting the preferred frame rate to a value lower than 60.

Fixes flutter/flutter#90675

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

During the API design review presented by @iskakaushik, this didn't seem like the direction we were going to take.

@cyanglaz cyanglaz added the Work in progress (WIP) Not ready (yet) for review! label Nov 19, 2021
@cyanglaz cyanglaz marked this pull request as draft November 19, 2021 18:25
@cyanglaz
Copy link
Contributor Author

@chinmaygarde After the meeting, we had an offline discussion and we wanted to enable 120 first as an intermediate step. This will be a beta feature which user has to opt-in.

@chinmaygarde
Copy link
Member

I am not convinced that is a good idea. During the review, there was widespread opposition to this approach. And doing just that but only for temporarily and for users that opt into it via a flag isn't something we would want to do. We have also had confusion in the past about when certain flags would be necessary (like those for platform views). Can you fill me in on the context of the discussion that led to making this decision?

@cyanglaz
Copy link
Contributor Author

The flag would be necessary for iOS to enable 120HZ nonetheless. So user has to opt-in.

The reasons we wanted to do this first:

  1. We'd like to resolve Scrolling is not true 120hz and feels laggy on ProMotion iPhone 13 flutter#90675 soon enough so apps who want 120 hz can opt-in (via Apple's required info.plist)
  2. We are still deciding on the exact logics on the engine's heuristics. (Currently we have: [WIP][iOS] infer dynamic refresh rate via heuristics. #29692)

Also see Kaushik's comment in flutter/flutter#90675 (comment)

@chinmaygarde
Copy link
Member

chinmaygarde commented Nov 19, 2021

necessary for iOS to enable 120HZ nonetheless. So user has to opt-in.

Not sure I understand why if progress is made on (2) right now. This patch as a solution for (1) seems like an odd choice given the degree of opposition to this approach during the review. Why not just wait for (2) and let users test it out in the beta without a flag?

@chinmaygarde
Copy link
Member

Just chatted with Kaushik about this and I was confused about the flag referred to here. To clarify, it was CADisableMinimumFrameDurationOnPhone and not one of our own (which is what I thought it was). It also doesn't seem to be always capping to maximum. Sorry about the confusion.

@chinmaygarde chinmaygarde self-requested a review November 19, 2021 21:34
@cyanglaz
Copy link
Contributor Author

CI blocked by flutter/flutter#85555

@Hixie
Copy link
Contributor

Hixie commented Dec 3, 2021

here's a proposed api for the framework: flutter/flutter@master...Hixie:freq

@Void48

This comment has been minimized.

@pkoehler-altow-extern

This comment has been minimized.

@leonidlist

This comment has been minimized.

@luckysmg

This comment has been minimized.

@cyanglaz
Copy link
Contributor Author

Hi Everyone. Thank you for being patient. This is in development and we are waiting on some benchmark work to be done before landing this.

I will hide the comments that unrelated to the solution to keep the conversation in this PR directly related to the solution itself. Thank you for your understanding :)

@luckysmg
Copy link
Contributor

luckysmg commented Jan 18, 2022

I wrote some code to try,both in flutter framework and engine. Any advice😀?
framework:flutter/flutter#96710
engine:#30900

@flutter-dashboard
Copy link

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

@cyanglaz
Copy link
Contributor Author

Since it is still blocked by flutter/flutter#85555
I'm going to temporarily close this issue to avoid unnecessary surfacing in triage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet