Does this need to be fixed in our existing CAtransaction code?

err, DisplayLink

If you refering to [self setNeedsDisplay] probably makes no difference for metal layer. I'm not even sure it makes difference here. It seems to have helped with frameskip on touch with 120hz display locked to 60hz, but that was several prototypes ago, on iOS 16 and without constant CADisplayLink on main thread. Thinking of it, the behavior might have been affected by touch rate correction displaylink (which is on main thread and enabled only during touch). I left it there just in case, but might be just placebo and needs to be retested.

This will cause FlutterView to take a screenshot...

It was removed from the final PR, but since the layer is backed by IOSurface I don't think it has associated CGContextRef, so I'd assume [FlutterView drawLayer:inContext:] should receive a nil context? Maybe I'm wrong about this.

FWIW I tried something like this with the existing drawable/metal layer a few months back. What I found is that there would occassinally be huge amounts of latency.

Does [CATransaction setDisableActions:YES] works to prevent the transaction on background thrread state issues? If so, could we not use this to present from a background thread with the regular drawable?

I tried that, it helps, but it still not as smooth as no transactions or the approach here. Especially when touch interaction the frame pacing seems quit bad:

same with FlutterMetalLayer:

I guess I don't really understand how writing our own Metal layer solves this problem?

Not sure about the question? I wasn't able to get CAMetalLayer present from background thread with transaction smoothly (that's the first graph - with CATransaction disabled, which helped somewhat, but not completely).

The second graph is FlutterMetalLayer (from the PR), in same app, presenting drawables from background thread with CATransaction at consistent 120Hz.

But you're dispatching to the main thread to handle the presentation?

I am. But we don't really know what [CAMetalDrawable present] does underneath. Apparently it's not just setting layer contents to underlying surface, otherwise disabling the transaction would fix it.

Right, but then I don't really understand how having our own metal layer helps

When you set IOSurface (that backs texture that backs drawable) to layer contents, compositor will pick it up and display after next vsync. If you do it within transaction, it will be synchronized with all other UIKit updates. We've been doing this on macOS for couple of years now.

If CAMetalLayer let us do the same thing, there'd be no need for custom metal layer. But it doesn't. It's full of magic that lets it display content outside of transactions, at custom time, etc, but you can't simply set layer content to the drawable.

ack. I might suggest that you find a way to implement this via some opt-in Plist flag or other option that allows for an extended period of testing. But if this solves them problems we've been having with presentation on iOS, I'm all for it!

Not magic, just missing until #48190

I just assumed it worked on main :) should have checked.

That is very generous of you 😆

command_buffer presentDrawable is just a utility/helper method for what you are doing below. See https://developer.apple.com/documentation/metal/mtlcommandbuffer/1443029-presentdrawable?language=objc

This convenience method calls the drawable’s present method after the command queue schedules the command buffer for execution. The command buffer does this by adding a completion handler by calling its own addScheduledHandler: method for you.

So I think unconditionally doing this one way would be simpler, with a note why we don't use the helper.

We can either link QuarzCore.framework or check for class name directly, i.e.

if([NSStringFromClass(drawable_.layer.class) isEqualToString:@"CAMetalLayer"])

[Command_buffer present] will also try to add some undocumented handler on the drawable, I'm not sure if it's important or not.

But I agree that we should be fine just doing this unconditionally.

For methods that aren't implemented, would it be safer to error log if they are used?

Are there parts of this that overlap with the other display link manager usage? Mostly curious if we have some repeated logic we need to be aware of, if so could use a note.

Our display link usage is a bit of a mess. I'd like to clean it up at some point, hopefully also fixing flutter/flutter#110431 in the process.

As far as this PR is concerned, the main issue is that the iOS vsync waiter is scheduling the display link on UI thread, which does not trigger a core animation frame (CAMetalLayer does not seem to care). So the vsync waiter displaylink is running at 120hz while core animation only syncs to compositor at 60hz. And because I don't want to touch other parts in this PR, I added a display link and copied some of the logic from vsync waiter so that it runs at same speed.

Makes sense. I would just add a note to this part of the code with the context you provided here.

Are there parts of flutter where this is needed?

I copied the "-Wunguarded-availability-new" from some other part of engine. Everywhere CAMetalLayer is mentioned the simulator build will complain. Apparently it's an old well known bug.

:(

perhaps some kind of error log if an unsupported format was provided?

Done.

Does this descriptor benefit from lossy texture compresion and/or do we need to worry about setting MTLTextureUsageShaderRead regressing performance at all? We don't try to read from the framebuffer anymore, but I think with platform views that may still happen.

TBH I'm not sure how much of it actually applies for textures created from IOSurfaces. I went with something that looked like a safe default. Btw. does skia not require MTLTextureUsageShaderRead?

Btw. The texture that backs CAMetalLayer drawables also has MTLTextureUsageShaderRead set. Edit: Only when CALayer.framebufferOnly is set to NO.

Its fine to leave it as defaults.

Btw. does skia not require MTLTextureUsageShaderRead?

I'm not sure, does this correspond to the framebufferOnly setting on the CAMetalLayer?

It looks like we need it (though backdrop filters won't read from the framebuffer, i guess platform views might?)

Right. Documentation says that with framebufferOnly the CALayer textures will only have rendertarget usage. I'll look into it.

I don't see how platform views would read from it. They don't have access to the metal texture, or even the underlying iOSurface, only compositor does.

Btw. we set layer.framebufferOnly to flutter::Settings::kSurfaceDataAccessible ? NO : YES; inside FlutterView.mm, but then unconditionally set to NO inside ios_surface_metal_impeller. Is that correct?

And actually seems necessary, otherwise:

[MTLDebugBlitCommandEncoder internalValidateCopyFromTexture:sourceSlice:sourceLevel:sourceOrigin:sourceSize:toTexture:destinationSlice:destinationLevel:destinationOrigin:options:]:388: failed assertion `Copy From Texture Validation
destinationTexture must not be a framebufferOnly texture.

Interesting, so we can't even blit to the framebuffer only texture! That is good to know. If we ever get back to fixing it we'd have to look into switching to a draw to see if that works

maybe im missing some context - i'm having trouble understanding why are we pausing display link here.

@hellohuanlin for context please see https://github.com/flutter/engine/blob/a53ca8a17c85065cd69a6ce10d6f9bc7de9e899a/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm .

IN short, DisplayLink ticks at a given rate and if you don't want to continually tick (because you know when you need to schedule frames) then you have to pause it.

Displaylink on main thread is actually driving vsync. That includes submitting a core animation frame. I haven't really profiled the overhead from it, but very likely there is some, compared to main thread just sleeping on idle. The questionable part here is pausing after 3 frames. It might not be necessary - it's more of a better safe than sorry thing.

do we need to check NPE here?

What for? Neither _availableTextures nor res can be nil at that place. Also invoking selectors on nil objects is permitted in objc.

oh i was talking about res - is it possible that it's not set?

reading it again - it looks like res will always be set. However, it's possible that the texture is in use (texture.surface.isInUse). Will that be fine?

Clarified this in a comment.

why are we changing the refresh rate here?

There is a weird behavior on iPhone, where for several seconds after unlock, and only with variable refresh rate enabled, the main thread display link seems to fire on completely different interval than actual system refresh rate. This does not happen when non-variable refresh rate (hence forcing the refresh rate if the layer detects multiple buffers submitted during single display link interval).

It is documented after top of the class (perhaps not the best place):

  // Used to track whether the content was set during this display link.
  // When unlocking phone the layer (main thread) display link and raster thread
  // display link get out of sync for several seconds. Even worse, layer display
  // link does not seem to reflect actual vsync. Forcing the layer link
  // to max rate (instead range) temporarily seems to fix the issue.
  BOOL _didSetContentsDuringThisDisplayLinkPeriod;

This behavior can be observed through animation hitches instruments tool for example - after modifying vsync_waiter_ios to fire on main thread, you will see that UI and raster threads are bing woken up at completely different period than system vsync.

It's strage that this happens and the only workaround I found was to disable variable refresh rate for a bit once this starts happening.