[WIP][iOS] Avoid nextDrawable take much time.

[luckysmg]

WIP & Needs Triage🤔🤔🤔

This needs triage and needs more investigation. Since this change may have side effect, need to check that.

If this change is reasonable, I will modify the code to make it looks can be landed.

Preview

Before

Before.mp4

After

After.mp4

This patch can solve:

• The constant junk when scrolling randomly.
• The constant junk when from bg to fg or swipe from control center.

Physical devices I using for verify:

These all devices can repo the constant junk bug and junk disappears with this patch:

• iPhone13Pro
• iPhone12
• iPhoneXS Max
• iPad Pro

The Pure Native iOS code to show this mechanism

Demo code

class TestVC: UIViewController {
    
    let metalLayer:CAMetalLayer = CAMetalLayer()
    
    var displayLink:CADisplayLink!
    
    var thread:Thread!
    
    override func touchesBegan(_ touches: Set<UITouch>, with event: UIEvent?) {
        self.displayLink.isPaused = true
    }
    
    override func touchesEnded(_ touches: Set<UITouch>, with event: UIEvent?) {
        self.displayLink.isPaused = false
    }
    
    override func viewDidLoad() {
        super.viewDidLoad()
        self.view.backgroundColor = .red
        let scale = UIScreen.main.scale
        metalLayer.frame = self.view.bounds
        metalLayer.drawableSize = CGSize(width: self.view.frame.width * scale, height: self.view.frame.height * scale)
        self.view.layer.addSublayer(metalLayer)
        
        self.addDisplayLinkInUITaskRunner()
        
        // Wait screen loading complete,and next,You can:
        
        // 1. Swipe down the control center, you will see a lot log print in console, and it will
        //                      constantly exists.
        
        // 2. Move the app to bg, and then move to fg, you will see a lot of log,
        //      it also will constantly exist for a long time.
        
        // 3. Just don't do anything, you might see it the log print constantly....
        //      And when you tap the screen, it may disappear, and After a while, you may see log again....
        
        
        // 4. Uncomment next line and rerun, and do same things in 1,2,3,
        // you will find that only a few of log will print, most of time there is not log ^_^.
        // If these are logs print, it is not constant, it will recover as normal after a short time,
        // which is what we want.
        // self.metalLayer.presentsWithTransaction = true
    }
    
    func addDisplayLinkInUITaskRunner() {
        self.thread = Thread(block: {
            RunLoop.current.add(NSMachPort(), forMode: .common)
            RunLoop.current.run()
        })
        self.thread.name = ""
        self.thread.start()
        
        self.perform(#selector(addDisplayLink), on: thread, with: nil, waitUntilDone: false)
    }
    
    @objc func addDisplayLink() {
        self.displayLink = CADisplayLink(target: self, selector: #selector(onDisplayLink))
        if #available(iOS 15.0, *) {
            self.displayLink.preferredFrameRateRange = .init(minimum: 60, maximum: 120, preferred: 120)
        } else {
            self.displayLink.preferredFramesPerSecond = 120
        }
        self.displayLink.add(to: .current, forMode: .common)
    }
    
    @objc private func onDisplayLink() {
        // Here should not be in main thread, because in flutter, layer renders in raster thread.
        assert(!Thread.current.isMainThread)
        
        let startTime = CACurrentMediaTime()
        let frameDrawable = metalLayer.nextDrawable()!
        let timeUsed = CACurrentMediaTime() - startTime
        
        // If time used to get next drawble over 3ms,
        // we print it here to indicate this method take much time!
        if (timeUsed > 0.003) {
            print("CAMetalLayer.nextDrawable take much time!! -> \(String(format: "%.2f", timeUsed * 1000)) ms")
        }
        frameDrawable.present()
    }
}

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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]

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.

[knopp]

This seems like it might need more investigation? Opting out of CATransaction should not affect frame rendering time.

[luckysmg]

This seems like it might need more investigation? Opting out of CATransaction should not affect frame rendering time.

Yes @knopp , this change needs triage and now this PR is just a draft and just show why it behaves like this. Still Needs more investigation O(∩_∩)O

[cyanglaz]

This seems also fixes flutter/flutter#115036.

Is there a benefit of not rendering with transactions?

[luckysmg]

Is there a benefit of not rendering with transactions?

I don't know if there is any benefit render without transactions in raster thread (without platform view).... The comment just say in raster thread there is not transactions like this. But from result, this did impact the time we get next drawable.

I think this change will impact the time we present MetalDrawable successfully. Because only after we present it successfully, another drawable can be swap to swap queue so that we can get nextDrawable. Otherwise the nextDrawable will block to wait for a long time...

Also. Before we can't see it in DevTools since we didn't put these time into raster time.
See #38674 about fixing that.

[knopp]

I think the transactions enabled was breaking image picker at some point?

#21526

[luckysmg]

@knopp Thx for point that out. I will have a check later. O(∩_∩)O

[delfme]

@luckysmg this is a milestone on iOS! 👏👏👏
I wish I could come over and kiss you!

But does it fix the janky screen animation issue? I'm referring to that weird behaviour by which we seem to miss first-frame on screen animations.

[luckysmg]

Updated:
Did some investigation and debugging. When set presentsWIthTransaction = true, when present the CAMetalDrawable, a transaction will trigger and will cause other CALayer layout and display in raster thread and will cause same crash in #21526

I'm not sure that if we can do something like method swizzing to hook all UIView.layoutSubView or CALayer.layoutSublayers to let them run in main thread.........

[knopp]

That is unfortunately. Conceivably we might need to do same thing we do on macOS, which is (bit simplified)

• from rater thread schedule block on main thread in which the surface is flipped
• block raster thread until it is done

[luckysmg]

That is unfortunately. Conceivably we might need to do same thing we do on macOS, which is (bit simplified)

• from rater thread schedule block on main thread in which the surface is flipped
• block raster thread until it is done

Thx. I will have a try on that. ^_^

[luckysmg]

Did some try and invesigation.

Because the PlatformView::NotifyCreated and PlatformView::NotifyDestroyed use a latch to sync platform thread and raster thread.

And in IOSSurfaceMetalSkia::PresentDrawable
I make a dirty patch:

bool IOSSurfaceMetalSkia::PresentDrawable(GrMTLHandle drawable) const {
  if (drawable == nullptr) {
    FML_DLOG(ERROR) << "Could not acquire next Metal drawable from the SkSurface.";
    return false;
  }

  auto command_buffer =
      fml::scoped_nsprotocol<id<MTLCommandBuffer>>([[command_queue_ commandBuffer] retain]);
  [command_buffer.get() commit];
  [command_buffer.get() waitUntilScheduled];

  // Use sync and we can wait this operation to complete.
  dispatch_sync(dispatch_get_main_queue(), ^{
    FML_DCHECK([NSThread isMainThread]);
    [CATransaction begin];
    [CATransaction setDisableActions:YES];
    [reinterpret_cast<id<CAMetalDrawable>>(drawable) present];
    [CATransaction commit];
  });
  return true;
}

And it is easy to get dead lock in add-to-app scene.....

Maybe I miss something.......

[knopp]

This shouldn't dead lock on PresentDrawable, right? Because at that point rasterizer is not yet presenting anything, we're just creating surface, am I wrong?

For teardown it is more complicated. The code is structured differently on desktop so I'm not sure how this would translate, but on shutdown, we first call engine->NotifyDestroyed (on platform thread), which down the line results in FlutterView getting notified, which unblocks the raster thread:

engine/shell/platform/darwin/macos/framework/Source/FlutterThreadSynchronizer.mm

Lines 105 to 110 in a19cf36

	- (void)shutdown {
	std::unique_lock<std::mutex> lock(_mutex);
	_shuttingDown = YES;
	_condBlockBeginResize.notify_all();
	[self drain];
	}

Since this is called from platform thread, it executes all blocks that raster thread is waiting for and then makes sure that raster thread won't block on anything after.

[luckysmg]

Have some try. Present on platform thread is ok, emm but when platform thread has some work to do, the drawable will be delayed to be presented, which is worse... Motion more sensitive for users on a mobile phone than on a desktop.....

Also, I surprisingly found this change will also work, and seems has no side effect. Maybe can also take a look that.

• [iOS] Ensure FlutterView's background color is not nil to avoid CAMetalLayer nextDrawable being time-consuming #39359