PlatformViews needs clipRect.

[endless7]

cc @ColdPaleLight @flar @dnfield
fix flutter/flutter#114784

[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.

[dnfield]

Is there a bug for this somewhere? Is there a test case that fails if this code isn't present?

[jmagman]

Is there a test case that fails if this code isn't present?

+1 this PR needs tests. See https://github.com/flutter/engine/pull/37434/files#diff-2110fb85ed5e9825880b29cbb4c1007a20592f08be07edb0b87e5138cb0c2dd0R1481 for a similar-ish example of how this could be done.

cc @cyanglaz

[flar]

Is there a bug for this somewhere? Is there a test case that fails if this code isn't present?

I also asked for clarification on the issue itself. The issue seems to say "this line of Skia code looks at the clip bounds" at the justification for why this is being done. There is no description for how an application is affected.

[flar]

It looks like this is dealing with the case where a partial repaint (computed by DiffContext) is causing only a small region of the display to be updated. The entire overlay is rendered as if it were a full-screen update rather than only clipping to the part that needs to be updated.

I think @cyanglaz should look at this to verify, but it sounds like the clip can help reduce operations. The associated github-issue seems focused on triggering the BBH processing in Skia, but that is a small optimization that only helps with incredibly complex scenes (such as the developer that was drawing hundreds of thousands of drawLine calls that were 99% offscreen). The main gain here for most apps would be not rendering pixels that aren't necessary for the partial-screen update - and a combination of Skia's built-in per-operation clip culling and pre-rendering BBH filtering will reduce that work further as well.

The larger question is why we are rendering into an oversized surface? If that is due to us simply ignoring the partial repaint clip when we allocate the surface on each frame then we can save even more by allocating a surface that is the right size. If this is a case of reusing either a surface from a previous (fully rendered) frame, or reusing a pool of surfaces that may be larger than we need to avoid allocation overhead on every frame - then the clip would be needed to avoid overdraw.

[cyanglaz]

I looked at the original design doc and it doesn't seem to mention why we can't clip the canvas: go/flutter-unobstructed-platform-views

Because the overlay is already clipped by the overlay_view_wrapper from iOS perspective, see: (https://github.com/flutter/engine/blob/main/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm#L743), the content that outside of the overlay is not going to be displayed with or without the canvas being clipped.

allocating a surface that is the right size

The content that we paint is in the coordinate space of the entire screen, so to make the surface smaller, we will need to update content to paint on a different coordinate, maybe something like:

  std::unique_ptr<SurfaceFrame> frame = layer->surface->AcquireFrame(SkSize::Make(rect.width:(), rect.height());
  SkCanvas* overlay_canvas = frame->SkiaCanvas();
  overlay_canvas->translate(-rect.x(), -rect.y());

[flar]

I looked at the original design doc and it doesn't seem to mention why we can't clip the canvas: go/flutter-unobstructed-platform-views

That link doesn't go anywhere. Is it supposed to be a flutter.dev go link?

Because the overlay is already clipped by the overlay_view_wrapper from iOS perspective, see: (https://github.com/flutter/engine/blob/main/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm#L743), the content that outside of the overlay is not going to be displayed with or without the canvas being clipped.

This isn't a "stuff is showing up that shouldn't be" kind of functionality bug. The output is correct.

This is a "we are rendering stuff that is not going to be shown" type of "wasted effort hurts performance" kind of bug. iOS my be clipping the results, but we are still doing work to create those pixels that we won't need in the end.

[cyanglaz]

The link is fixed and it should work now. It is an internal go link.

This is a "we are rendering stuff that is not going to be shown" type of "wasted effort hurts performance" kind of bug. iOS my be clipping the results, but we are still doing work to create those pixels that we won't need in the end.

Understood, I was just wondering if making the surface smaller and keep the content as is would increase performance, or we would actually need to clip the content to reduce unnecessary rendering. Skia or Impeller should have some optimization if the content is outside of the canvas?

[flar]

This is a "we are rendering stuff that is not going to be shown" type of "wasted effort hurts performance" kind of bug. iOS my be clipping the results, but we are still doing work to create those pixels that we won't need in the end.

Understood, I was just wondering if making the surface smaller and keep the content as is would increase performance, or we would actually need to clip the content to reduce unnecessary rendering. Skia or Impeller should have some optimization if the content is outside of the canvas?

By default a Skia canvas will have a clip to the surface it is on, but in this case we are drawing to just a part of that surface, so without the clipRect the only culling that will happen will be to the much larger surface - which is extra work. Normally a scene's bounds is only as big as its content, but some conditions can cause us to draw part of a scene. DiffContext/partial repaints are one case, but there may be others? I didn't get an answer on my question in the github issue as to whether this is triggered by partial repaints or not.

[endless7]

This is a "we are rendering stuff that is not going to be shown" type of "wasted effort hurts performance" kind of bug. iOS my be clipping the results, but we are still doing work to create those pixels that we won't need in the end.

Understood, I was just wondering if making the surface smaller and keep the content as is would increase performance, or we would actually need to clip the content to reduce unnecessary rendering. Skia or Impeller should have some optimization if the content is outside of the canvas?

By default a Skia canvas will have a clip to the surface it is on, but in this case we are drawing to just a part of that surface, so without the clipRect the only culling that will happen will be to the much larger surface - which is extra work. Normally a scene's bounds is only as big as its content, but some conditions can cause us to draw part of a scene. DiffContext/partial repaints are one case, but there may be others? I didn't get an answer on my question in the github issue as to whether this is triggered by partial repaints or not.

Doesn't the partial repaints only happen in comparing two dl? In my understanding repaint and just paint are different cases. I just wonder how the partial repaints works at this case.

[flar]

Doesn't the partial repaints only happen in comparing two dl? In my understanding repaint and just paint are different cases. I just wonder how the partial repaints works at this case.

Partial repaint happens when two layer trees are compared during this piece of code:

engine/flow/compositor_context.cc

Line 39 in e6d9fff

layer_tree.root_layer()->Diff(&context, prev_root_layer);

That line will run the entire layer tree and compare it to the previous layer tree looking for differences. It accumulates the bounds of the differences and those bounds are handed off to the code that prerolls and paints the layer tree to restrict those operations to the changed region. An individual DisplayListLayer might compare its DL against the one in the previous frame to see if they are the same, but the entire partial repaint process happens across an entire layer tree.

[knopp]

Partial repaint should be disabled when there are platform views. If it isn't that's a bug. I haven't figured out how partial repaint would interact with multiple surfaces (especially because the z-order can change between frames).

[knopp]

Also, the number of content needed to be repainted (existing damage) depends on which of two back buffers CALayer gives us. But with platform views there are multiple CALayers (for overlays) - so this may not give correct result.

It seems that right now layer diffing is disabled when thread merging is enabled. Might not be the best way to do this.

[flar]

LGTM

[flar]

It looks like this comes into play in the embedder when we slice up the display lists that live under/over/between the platform views and draw pieces of them with tiny clips. Setting the clip in that case can reduce the pixel count that the GPU has to deal with.

[flar]

Please add a test as per this comment: #37107 (comment)

[jonahwilliams]

I investigated this on a slightly simplified example from: flutter/flutter#114712

Without this change average raster is 9.5ms
With this change average raster is 7.5ms

Since this change did not show up in our benchmarks, we should add a case that has some more expensive UI.

[zanderso]

Thanks @jonahwilliams! Can this be landed?

[jonahwilliams]

Lets land the benchmark first, then this one

[jonahwilliams]

The benchmark change has landed. lets give it some time to build up a few commits and verify it is working correctly. If everything goes according to plan I'll merge this later tonight

[flar]

Approved pending Jonah's discretion...

[jonahwilliams]

We have a few runs now, will land: https://flutter-flutter-perf.skia.org/e/?queries=test%3Dplatform_views_scroll_perf_non_intersecting_impeller_ios__timeline_summary&selected=commit%3D32365%26name%3D%252Carch%253Dintel%252Cbranch%253Dmaster%252Cconfig%253Ddefault%252Cdevice_type%253DiPhone_6s%252Cdevice_version%253Dnone%252Chost_type%253Dmac%252Csub_result%253Dworst_frame_rasterizer_time_millis%252Ctest%253Dplatform_views_scroll_perf_non_intersecting_impeller_ios__timeline_summary%252C

[auto-submit]

auto label is removed for flutter/engine, pr: 37107, due to - Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

[auto-submit]

auto label is removed for flutter/engine, pr: 37107, due to Validations Fail.

[jonahwilliams]

LGTM

[flar]

Did we file a follow-on Issue to write a test case for this? Or is the benchmark considered the "test case"?

[zanderso]

SkiaPerf noticed the improvement: https://flutter-flutter-perf.skia.org/e/?begin=1669946990&end=1670255500&keys=X06713a607debd263da56e42ef298cf37&requestType=0&selected=commit%3D32366%26name%3D%252Carch%253Dintel%252Cbranch%253Dmaster%252Cconfig%253Ddefault%252Cdevice_type%253DiPhone_6s%252Cdevice_version%253Dnone%252Chost_type%253Dmac%252Csub_result%253D90th_percentile_frame_rasterizer_time_millis%252Ctest%253Dplatform_views_scroll_perf_non_intersecting_impeller_ios__timeline_summary%252C&xbaroffset=32366

[jonahwilliams]

For something that is primarily intended to be a performance improvement a benchmark is a sufficient test