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 jitter and laggy when user is dragging on iOS Promotion devices #35592

Merged
merged 13 commits into from
Sep 7, 2022

Conversation

luckysmg
Copy link
Contributor

@luckysmg luckysmg commented Aug 22, 2022

Related issue to fix:

Fixes flutter/flutter#101653

Behavior

When user is dragging (means the finger is on screen and move continuously) on the screen on Promotion devices on iOS(iPhone13Pro or iPhone13ProMax), we will see a very transparent frame drop and junk behavior.

But when user is dragging on normal devices without Promotion (iPhone12 or iPhone11..), the drag behavior seems to be very smooth.

Root Cause

The touch events delivered by UIViewController like touchesBegan, touchesMoved is not called with 120HZ rate. Normally, it is called with 60HZ rate, which will cause that the signal's rate delivered by platfrom doesn't match the rate using in render pipeline in VsyncWaiterIOS. So it will have a gap, which will lead junk and laggy. This mechanism is the same as flutter/flutter#109435 .

Why

For iOS promotion devices, system detects that there is no 120HZ animation running, so it calls touch events with 60HZ rate (maybe for saving more power). We already have a 120HZ CADisplayLink in VsyncWaiterIOS, but unfortunately it is run in UITaskRunner, which is not iOS main thread.(On iOS, all native UI animations should run in main thread, so system doesn't detect that and let gesture system remain 60HZ callback rate)

Here is the iOS native demo code to show this mechanism.
You can run it on Promotion device to prove this.

Demo code
class HomeViewController: UIViewController {
    
    private lazy var mainDisplayLink = CADisplayLink(target: self, selector: #selector(self.onDisplayLinkInMainThread))
    
    private lazy var uiTaskRunnerDisplayLink = CADisplayLink(target: self, selector: #selector(self.onDisplayLinkInUITaskRunner))
    
    private var oldTime:CFAbsoluteTime = 0
    override func touchesMoved(_ touches: Set<UITouch>, with event: UIEvent?) {
        let current = CACurrentMediaTime()
        let intervalMs = current - oldTime
        print("interval of touchesMoved method is :\(intervalMs) s")
        oldTime = current
    }
    
    var thread:Thread!
    override func viewDidLoad() {
        super.viewDidLoad()
        self.view.backgroundColor = .red
        
        // Add a display link in UITaskRunner like what we do in VsyncWaiterIOS.
        // If we only run this line , the touch callback interval is 16ms.
        self.addDisplayLinkInUITaskRunner()
        
        // You can try to uncomment the line below to see the log.
        // The touch callback interval will become 8ms.
        // Which is correct and expected on Promotion devices.
//        self.addDisplayLinkInMainThread()
    }
    
    //====== Add displaylink in MainThread
    func addDisplayLinkInMainThread() {
        if #available(iOS 15.0, *) {
            self.mainDisplayLink.preferredFrameRateRange = .init(minimum: 60, maximum: 120, preferred: 120)
        } else {
            self.mainDisplayLink.preferredFramesPerSecond = 120
        }
        self.mainDisplayLink.add(to: .current, forMode: .common)
    }
    
    //====== Add displaylink not in main thread,
    // We create a new thread to simulate the UITaskRunner in flutter engine
    func addDisplayLinkInUITaskRunner() {
        thread = Thread(block: {
            RunLoop.current.add(NSMachPort(), forMode: .common)
            RunLoop.current.run()
        })
        self.thread.name = "UITaskRunner"
        self.thread.start()
        self.perform(#selector(addDisplayLink), on: thread, with: nil, waitUntilDone: false)
    }
    
    @objc func addDisplayLink() {
        if #available(iOS 15.0, *) {
            self.uiTaskRunnerDisplayLink.preferredFrameRateRange = .init(minimum: 60, maximum: 120, preferred: 120)
        } else {
            self.uiTaskRunnerDisplayLink.preferredFramesPerSecond = 120
        }
        self.uiTaskRunnerDisplayLink.add(to: .current, forMode: .common)
    }
    
    
    @objc func onDisplayLinkInUITaskRunner() {
//        print("onDislpayLink on UITaskRunner")
    }
    
    @objc func onDisplayLinkInMainThread() {
//        print("onDislpayLink on main thread")
    }
}

Solution

We can't simply move the CADisplayLink in VsyncWaiterIOS from UITaskRunner to PlatformTaskRunner(or just move its CADisplayLink to mainRunloop), because flutter UI tasks should be in UITaskRunner.

So, we can create a new VsyncClient in FlutterViewController.
And add it in mainRunloop. When user is interacting, we should activate it to tell system: "I have 120HZ animation running, plz give me the touch callback with 120HZ rate !! "

And we pause it when user ends interaction.

But we only do this when it is needed indeed.
For exmaple, if a device's max frame rate is 60HZ, means we don't need to create new VsyncClient to correct the touch rate.
Moreover, in the callback of this vsync client, we do nothing, because this VsyncClient is to trigger system to callback touch events with 120HZ rate. So I think this cost is acceptable.

Test locally to see the comparsion with this fix

Flutter demo code
import 'package:flutter/cupertino.dart';
import 'package:flutter/material.dart';

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 ListPage(),
    );
  }
}

class ListPage extends StatefulWidget {
  const ListPage({Key? key}) : super(key: key);

  @override
  State<ListPage> createState() => _ListPageState();
}

class _ListPageState extends State<ListPage> {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: ListView.builder(
          physics: const BouncingScrollPhysics(),
          itemBuilder: (BuildContext ctx, int index) {
            return GestureDetector(
              onTap: () {
                Navigator.push(context, CupertinoPageRoute(builder: (ctx) {
                  return const ListPage();
                }));
              },
              child: Container(
                padding: const EdgeInsets.all(16),
                color: Color.fromRGBO(Random.secure().nextInt(256), Random.secure().nextInt(256),
                    Random.secure().nextInt(256), 1),
                child: const Text('Tap to push new page', style: TextStyle(fontSize: 30)),
              ),
            );
          }),
    );
  }
}

The recorded screen video can't show this fix very transparently (because the video is 45FPS - 55FPS). So I very recommend you to test this fix locally with physical devices.

Flutter framework ref can run this locally:d0c8774508808a0cc56d6c1f07c4200f380a480c
Flutter engine ref: This branch
Device: iPhone13 Pro or iPhone13 Pro Max

You can just run the code to see the behavior with this fix
or delete the method createTouchRateCorrectionVSyncClientIfNeeded to see the behavior before

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.

@luckysmg luckysmg changed the title Avoid junk and laggy when user is dragging on iOS Promotion devices [iOS] Avoid junk and laggy when user is dragging on iOS Promotion devices Aug 22, 2022
@rotorgames
Copy link

Tested up on my iPhone 13 Pro iOS 15.6

Works like a charm.

@luckysmg
Copy link
Contributor Author

@rotorgames Oh, I’m very happy to see that😄
Hoping this PR can be landed quickly😄

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 providing the fix.
I tested the issue and found that there were 60hz frames even when doing automated scrolling. (Set a timer to scroll the list view periodically without touch events).
So I suspected it had more to do with gesture handled in 60hz.

This fix seems pretty similar to the workaround in flutter/flutter#101653 (comment), does this fix also have the issue where the first frame when scroll just starts is 60hz? (I can test it when I get a chance too).

@luckysmg
Copy link
Contributor Author

luckysmg commented Aug 23, 2022

@cyanglaz Thx for reviewing this😄.

I m not sure first frame is 60HZ or 120hz.But I activate the vsync client when receive first touch event.So I think it should be 120HZ.But I think it should be ok if first frame is 60HZ.Making sure the touch events delivered with 120HZ continuously is the core target of this PR

In my opinion, this solution is different from that solution you mentioned(About the Ticker).

In that solution, it will start a ticker and let VsyncWaiter not pause during scroll update, but the delivery rate of gesture events is still 60HZ, which is the root cause, and it still exists, so when you are dragging on screen, the listview's offset updates with still 60HZ instead of 120HZ
(I think that's what you mean probably😄)

But for solution in this PR, the touch events delivery rate will be 120HZ, which will solve the root cause and it is expected.

You can check this iOS native demo to prove this

Demo code
class HomeViewController: UIViewController {
    
    private lazy var mainDisplayLink = CADisplayLink(target: self, selector: #selector(self.onDisplayLinkInMainThread))
    
    private lazy var uiTaskRunnerDisplayLink = CADisplayLink(target: self, selector: #selector(self.onDisplayLinkInUITaskRunner))
    
    private var oldTime:CFAbsoluteTime = 0
    override func touchesMoved(_ touches: Set<UITouch>, with event: UIEvent?) {
        let current = CACurrentMediaTime()
        let intervalMs = current - oldTime
        print("interval of touchesMoved method is :\(intervalMs) s")
        oldTime = current
    }
    
    var thread:Thread!
    override func viewDidLoad() {
        super.viewDidLoad()
        self.view.backgroundColor = .red
        
        // Add a display link in UITaskRunner like what we do in VsyncWaiterIOS.
        // If we only run this line , the touch callback interval is 16ms.
        self.addDisplayLinkInUITaskRunner()
        
        // You can try to uncomment the line below to see the log.
        // The touch callback interval will become 8ms.
        // Which is correct and expected on Promotion devices.
//        self.addDisplayLinkInMainThread()
    }
    
    //====== Add displaylink in MainThread
    func addDisplayLinkInMainThread() {
        if #available(iOS 15.0, *) {
            self.mainDisplayLink.preferredFrameRateRange = .init(minimum: 60, maximum: 120, preferred: 120)
        } else {
            self.mainDisplayLink.preferredFramesPerSecond = 120
        }
        self.mainDisplayLink.add(to: .current, forMode: .common)
    }
    
    //====== Add displaylink not in main thread,
    // We create a new thread to simulate the UITaskRunner in flutter engine
    func addDisplayLinkInUITaskRunner() {
        thread = Thread(block: {
            RunLoop.current.add(NSMachPort(), forMode: .common)
            RunLoop.current.run()
        })
        self.thread.name = "UITaskRunner"
        self.thread.start()
        self.perform(#selector(addDisplayLink), on: thread, with: nil, waitUntilDone: false)
    }
    
    @objc func addDisplayLink() {
        if #available(iOS 15.0, *) {
            self.uiTaskRunnerDisplayLink.preferredFrameRateRange = .init(minimum: 60, maximum: 120, preferred: 120)
        } else {
            self.uiTaskRunnerDisplayLink.preferredFramesPerSecond = 120
        }
        self.uiTaskRunnerDisplayLink.add(to: .current, forMode: .common)
    }
    
    
    @objc func onDisplayLinkInUITaskRunner() {
//        print("onDislpayLink on UITaskRunner")
    }
    
    @objc func onDisplayLinkInMainThread() {
//        print("onDislpayLink on main thread")
    }
}

@luckysmg
Copy link
Contributor Author

luckysmg commented Aug 23, 2022

And I think this PR maybe not related with the automated scrolling, this PR is to make sure the delivery rate of touch events is correct, which is driven by touch events. But automated scrolling is driven by animation system...So I think that should be different from this.

@luckysmg luckysmg requested a review from cyanglaz August 25, 2022 03:16
@cyanglaz
Copy link
Contributor

cyanglaz commented Aug 25, 2022

Is this similar to rotorgames@37b78ae but using a different vsync client that is on the main thread? I wonder if the new vsync waiter needs to be on the main thread. If it doesn't, the root cause of the problem is still unclear to me.

Edit: I missed the part where the vsync waiter in rotorgames@37b78ae was changed to be on main thread. So it is a similar solution but creating a new vsync waiter on main thread. This seems better as we can keep the UIThread vsync waiter unchanged.

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.

I tested locally as well it works nicely!

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

@@ -65,6 +65,9 @@ @interface FlutterViewController () <FlutterBinaryMessenger, UIScrollViewDelegat
@property(nonatomic, assign) double targetViewInsetBottom;
@property(nonatomic, retain) VSyncClient* keyboardAnimationVSyncClient;

/// VSyncClient for touch callback's rate correction.
Copy link
Contributor

Choose a reason for hiding this comment

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

nits:

Suggested change
/// VSyncClient for touch callback's rate correction.
/// VSyncClient for touch callback's frame rate correction.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe also add a short explanation about why this is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -671,6 +674,9 @@ - (void)viewDidLoad {
// Register internal plugins.
[self addInternalPlugins];

// Create a vsync client to correct touch rate if needed.
Copy link
Contributor

Choose a reason for hiding this comment

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

nits:
how about: Create a vsync client to correct frame rate during touch events if needed.

Copy link
Contributor Author

@luckysmg luckysmg Aug 27, 2022

Choose a reason for hiding this comment

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

I have changed it to delivery frame rate of touch events, which seems to be more accurate.😄

@@ -966,6 +973,9 @@ - (void)dispatchTouches:(NSSet*)touches
}
}

// Activate or pause touch rate correction according to the touches when user is interacting.
Copy link
Contributor

Choose a reason for hiding this comment

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

"touch rate" is a little ambiguous. maybe "frame rate during touch events"?

Copy link
Contributor Author

@luckysmg luckysmg Aug 27, 2022

Choose a reason for hiding this comment

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

I have changed it to delivery frame rate of touch events, which seems to be more accurate.😄

@cyanglaz
Copy link
Contributor

@iskakaushik Could you do a secondary review?

@knopp knopp self-requested a review August 27, 2022 15:49
Copy link
Member

@knopp knopp left a comment

Choose a reason for hiding this comment

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

I'm wondering if this PR is maybe a bit of overkill? Similar effect can be achieved much easier. Look at knopp@50178b7.

We can just delay pausing the CADisplayLink for few frames. That way it is continuously enabled while user is interacting with the application and then disabled when application gets idle.

@luckysmg
Copy link
Contributor Author

luckysmg commented Aug 28, 2022

Hi @knopp
In your way. The delivery frame rate of touch events is still 60HZ, which is not expected😄.
See: #35592 (comment)
and
Root cause

@luckysmg
Copy link
Contributor Author

luckysmg commented Sep 7, 2022

I'd suggest editing the commit message. It's "jank" and in this case it's not even jank, it's jitter.

@knopp
Done. I have changed that word in title. Thx^_^

@luckysmg
Copy link
Contributor Author

luckysmg commented Sep 7, 2022

@iskakaushik Would you mind taking a look^_^

@cyanglaz
Copy link
Contributor

cyanglaz commented Sep 7, 2022

I'd suggest editing the commit message. It's "jank" and in this case it's not even jank, it's jitter.

@knopp Done. I have changed that word in title. Thx^_^

There are some usage of "junk" in the code comments as well, could you also fix them?

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.

Blocking landing until the "junk" usages are removed in comments.

@luckysmg
Copy link
Contributor Author

luckysmg commented Sep 7, 2022

@cyanglaz
Done. Sorry for forgetting that...Now should be ok^_^

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

@cyanglaz cyanglaz added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 7, 2022
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 7, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Sep 7, 2022

auto label is removed for flutter/engine, pr: 35592, due to - The status or check suite Linux Framework Smoke Tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

@jmagman
Copy link
Member

jmagman commented Sep 7, 2022

Unfortunately I think you're hitting something relating to #35843, can you rebase onto top of tree?

@luckysmg
Copy link
Contributor Author

luckysmg commented Sep 7, 2022

Unfortunately I think you're hitting something relating to #35843, can you rebase onto top of tree?

Done^_^

@arolan
Copy link

arolan commented Jan 3, 2023

hello, our team is wondering which release of flutter this fix will be part of? Is it part of stable release 3.3.2 already? or will it be part of 3.3.7?

thank you for your help!

@jmagman
Copy link
Member

jmagman commented Jan 3, 2023

This is available in 3.4.0-27.0.pre.
https://github.com/flutter/flutter/wiki/Where's-my-Commit%3F#finding-the-framework-commit-that-contains-engine-commit-x

@arolan
Copy link

arolan commented Jan 3, 2023

thank you @jmagman 💯

@arolan
Copy link

arolan commented Jan 29, 2023

hello everyone and @jmagman do you know if this is part of stable release 3.7.0 (macos) that got released on 1/24/23?

thank you very much!

@cyanglaz
Copy link
Contributor

cyanglaz commented Jan 30, 2023

hello everyone and @jmagman do you know if this is part of stable release 3.7.0 (macos) that got released on 1/24/23?

thank you very much!

Yes it is on stable.
To be specific, the engine roll containing the fix is: flutter/flutter@10d7e31

@arolan
Copy link

arolan commented Jan 30, 2023

thank you so much @cyanglaz ! appreciate it! This is awesome news! we will try 3.7.0 then soon.

@MinseokKang003

This comment was marked as resolved.

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 platform-ios
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gesture Animation is not 120hz and feels laggy on ProMotion iPhone 13
7 participants