engine
engine copied to clipboard
[iOS] Avoid keyboard animation junk and laggy on Promotion devices.
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. https://github.com/flutter/flutter/issues/109435
Pre-launch Checklist
- [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities.
- [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
- [x] I listed at least one issue that this PR fixes in the description above.
- [x] 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.
- [x] I updated/added relevant documentation (doc comments with
///
). - [x] I signed the [CLA].
- [x] All existing tests are passing.
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.
Tests has added.
@cyanglaz could you take a look?
Thanks for updating it to use vsync client. Let's also mention the purpose of using vsync client in the PR description.
Done
Does this fix the linked issue? The description just says it "optimizes" it.
Yes, optimizes it. Maybe that issue is due to diffrence thread. The VsyncClient
for keyboard animation in FlutterViewController
must run in mainLoop
, because the properties like view.presentationLayer
must be accessd from iOS main thread.
But VsyncClient
for flutter UI rendering is run in the runloop of UITaskRunner
.
So maybe this PR is to optimize it (At least to ensure the refresh rate that keyboard animation take matches the refresh rate using for rendering). And this optimization will be more obvious on iPhone that have Promotion
,eg:iPhone13Pro ).
You could have some debuging using physical device that have promotion to test this PR's optimization. Here is example dart code:
void main() {
runApp(const MyApp());
}
class MyApp extends StatelessWidget {
const MyApp({Key? key}) : super(key: key);
@override
Widget build(BuildContext context) {
return MaterialApp(
title: 'Flutter Demo',
theme: ThemeData(
primarySwatch: Colors.blue,
),
home: const TextFieldPage(),
);
}
}
class TextFieldPage extends StatefulWidget {
const TextFieldPage({Key? key}) : super(key: key);
@override
State<TextFieldPage> createState() => _TextFieldPageState();
}
class _TextFieldPageState extends State<TextFieldPage> {
@override
Widget build(BuildContext context) {
return Scaffold(
body: Column(
mainAxisAlignment: MainAxisAlignment.end,
children: [
const Spacer(),
const Text('Keyboard animation', style: TextStyle(fontSize: 26)),
const Spacer(),
SizedBox(
height: 60 + MediaQuery.of(context).padding.bottom,
child: const CupertinoTextField(
decoration: BoxDecoration(color: Colors.red),
)),
],
),
);
}
}
I think there is a bit of a language problem. "Optimize" means to me making something more efficient. Since the linked issue isn't an efficiency problem, it is a visual glitch. Does this PR remove the visual glitch?
I think there is a bit of a language problem. "Optimize" means to me making something more efficient. Since the linked issue isn't an efficiency problem, it is a visual glitch. Does this PR remove the visual glitch?
Oh, It will reduce some visual glitch on high refresh rate devices. But it still has a little little bit.... I have modified the desciption.
Would you mind take a look again?😄 @gaaclarke.
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: https://github.com/flutter/flutter/issues/101653#issuecomment-1169809142
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: https://github.com/flutter/flutter/issues/101653#issuecomment-1169809142
The comparison is very obvious.
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😄.
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 https://github.com/flutter/flutter/issues/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 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. https://github.com/flutter/flutter/issues/101653#issuecomment-1134786994
IMHO, we don't need to pause CADisplaylink
during a keyboard animation and any user's gestures (like scroll and etc)
@rotorgames Oh yeah, I'll also take a look that problem you said.
Here is the corresponding flutter framework ref to run locally: ab23233fa8212b2813d92191e520d3cc88ffd362
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:
https://user-images.githubusercontent.com/53510751/201529532-b1e3e89f-68ee-4c19-9471-25d0e26e5635.MOV
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
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
https://github.com/flutter/flutter/issues/105687#issuecomment-1246153912
Hello @luckysmg , Thank you for the PR. Is this feature merged to latest stable flutter version(3.3.9)?
No, but it should be in beta channel
This is available in 3.4.0-17.0.pre, see https://github.com/flutter/flutter/wiki/Where's-my-Commit%3F#finding-the-framework-commit-that-contains-engine-commit-x
@jmagman oh I didn't know how to check the corresponding version. Thanks for the reference!