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

[iOS] Avoid keyboard animation jank and laggy on Promotion devices. #34871

Merged
merged 28 commits into from
Aug 18, 2022

Conversation

luckysmg
Copy link
Contributor

@luckysmg luckysmg commented Jul 24, 2022

If use CADisplaylink directly, the refresh rate of CADisplaylink is not correct
It doesn't match the refresh using for flutter rendering, and this will cause some junk and laggy when start keyboard animation.

So using VsyncClient will fix this (See VsyncClient.setMaxRefreshRateIfEnabled)

List which issues are fixed by this PR. You must list at least one issue.
flutter/flutter#109435

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 tests are passing.

…keyboard animation link refresh rate using new interface
@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 (don't just cc him here, he won't see it! He's on Discord!).

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.

@luckysmg luckysmg changed the title Add interface for DisplayLinkManager to set max refresh rate and set keyboard link refresh rate using new interface Add interface for DisplayLinkManager to set max refresh rate and set keyboard animation link refresh rate using new interface Jul 25, 2022
@luckysmg luckysmg changed the title Add interface for DisplayLinkManager to set max refresh rate and set keyboard animation link refresh rate using new interface Add interface for DisplayLinkManager and set keyboard animation link refresh rate using new interface Jul 26, 2022
@luckysmg luckysmg changed the title Add interface for DisplayLinkManager and set keyboard animation link refresh rate using new interface [iOS] Add interface for DisplayLinkManager and set keyboard animation link refresh rate using new interface Jul 26, 2022
@luckysmg
Copy link
Contributor Author

Tests has added.

@jmagman jmagman requested a review from cyanglaz July 27, 2022 20:20
@jmagman
Copy link
Member

jmagman commented Jul 27, 2022

@cyanglaz could you take a look?

@luckysmg luckysmg changed the title [iOS] Add interface for DisplayLinkManager and set keyboard animation link refresh rate using new interface [iOS] Replace CADisplayLink with VsyncClient in keyboard animation. Jul 30, 2022
@luckysmg luckysmg requested a review from cyanglaz August 1, 2022 01:42
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

Thanks for updating it to use vsync client. Let's also mention the purpose of using vsync client in the PR description.

@luckysmg luckysmg requested a review from cyanglaz August 2, 2022 10:34
@luckysmg
Copy link
Contributor Author

luckysmg commented Aug 3, 2022

Thanks for updating it to use vsync client. Let's also mention the purpose of using vsync client in the PR description.

Done

Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

LGTM.
@gaaclarke Do you mind giving a secondary review?

@luckysmg
Copy link
Contributor Author

luckysmg commented Aug 10, 2022

I think this looks good. This section of code is pretty tricky and this change makes it even more complicated. I'd like to make sure that we are actually getting the benefit we want before merging it. @cyanglaz did you test this locally?

I think this PR is more suitable for this desc:
flutter/flutter#101653 (comment)

@luckysmg
Copy link
Contributor Author

luckysmg commented Aug 10, 2022

And If you want to run locally to test the benefit. You can use this demo code😄
It can be run directly without any setup:
flutter_demo.zip

You can test according to this:
flutter/flutter#101653 (comment)

The comparison is very obvious.

@luckysmg
Copy link
Contributor Author

Hi @gaaclarke @cyanglaz
I have merged main into this branch, so that maybe it will not take so much time to compile code when you switching to this branch and test locally😄.

@luckysmg luckysmg requested a review from cyanglaz August 11, 2022 23:22
@luckysmg
Copy link
Contributor Author

luckysmg commented Aug 12, 2022

Hi @gaaclarke @cyanglaz
Sorry.I think I didn't made things very clear before. Here is coming:

This PR is to mainly avoid the keyboard animation junk and laggy on Promotion devices (eg:iPhone13Pro)

The visual glitch in flutter/flutter#105687 . I think it is due to the cause below probably:
On the engine side,we use CADisplaylink or VsyncClient to track the animation interpolation manually, but it is not a iOS native view that use the way like:UIView.animate(block:),so it maybe have some diffrence...

So why's this visual glitch related with this PR?
Because the CADisplayLink's refresh rate using in FlutterViewController's keyboard animation doesn't match the refresh rate of VsyncClient using for rendering on 120HZ devices. And that mismatch will cause some junk and laggy,which will enlarge the visual glitch. (That's why 60hz devices are fine and keyboard animation is smoother, because both of them refresh rate are 60HZ)

In brief, this PR is to mainly avoid the keyboard animation junk and laggy, and will reduce the visual glitch.....

Before ,I wanted to show you a video that can show the difference before and after applying this PR.
But the recorded screen video on iPhone is usually 45FPS-55FPS, it is not enough and it is very difficult to see the differences before and after applying this PR. But it is easy to see it directly with our eyes😄.
And that's why I recommend you to test it locally to see the comparsion😄

@luckysmg luckysmg changed the title [iOS] Replace CADisplayLink with VsyncClient in keyboard animation. [iOS] Avoid keyboard animation junk and laggy on Promotion devices. Aug 12, 2022
@rotorgames
Copy link

@luckysmg Great work. I guess this PR is most important for improving frame rate performance on devices with promotions so far.

setAllowPauseAfterVsync method can help to improve scroll and gestures performance as I described in this comment. flutter/flutter#101653 (comment)

IMHO, we don't need to pause CADisplaylink during a keyboard animation and any user's gestures (like scroll and etc)

@luckysmg
Copy link
Contributor Author

luckysmg commented Aug 15, 2022

@rotorgames Oh yeah, I'll also take a look that problem you said.

@luckysmg
Copy link
Contributor Author

Here is the corresponding flutter framework ref to run locally:
ab23233

Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

Tested on my iPhone 13 pro and the Jank is gone with this fix.
@luckysmg Thank you!

@gaaclarke Do you mind to take another look?

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM =)

@jmagman jmagman added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 18, 2022
@auto-submit auto-submit bot merged commit 50cfc83 into flutter:main Aug 18, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 18, 2022
@luckysmg luckysmg deleted the opt_display_link branch August 19, 2022 01:04
@delfme
Copy link

delfme commented Nov 13, 2022

The issue with keyboard animation on flutter is also that the curve is different from native. It is strange that nodoby mentioned this so far but it seems I couldnt find an open issue for that.

Native video below:

IMG_5074.MOV

@delfme
Copy link

delfme commented Nov 13, 2022

As from video above, keyboard and textfield move up almost in sync. While in flutter they are not in sync and visual result looks weird to the end user

@luckysmg
Copy link
Contributor Author

As from video above, keyboard and textfield move up almost in sync. While in flutter they are not in sync and visual result looks weird to the end user

See

flutter/flutter#105687 (comment)

@dayoul
Copy link

dayoul commented Dec 6, 2022

Hello @luckysmg , Thank you for the PR. Is this feature merged to latest stable flutter version(3.3.9)?

@luckysmg
Copy link
Contributor Author

luckysmg commented Dec 6, 2022

No, but it should be in beta channel

@jmagman
Copy link
Member

jmagman commented Dec 6, 2022

@dayoul
Copy link

dayoul commented Dec 7, 2022

@jmagman oh I didn't know how to check the corresponding version. Thanks for the reference!

@cyanglaz cyanglaz changed the title [iOS] Avoid keyboard animation junk and laggy on Promotion devices. [iOS] Avoid keyboard animation jank and laggy on Promotion devices. Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App needs tests platform-ios
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants