Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@ColdPaleLight
Copy link
Member

@ColdPaleLight ColdPaleLight commented Apr 1, 2022

reland #31875
revert #32370
added commit d685191

In the RasterizeLayer method, we must use different paint_bounds according to different RasterCacheLayerStrategy

  • In the case of kLayer, we use paint_bounds of layer
  • In the case of kLayerChildren, we use origin_child_paint_bounds of container_layer
const SkRect& RasterCache::GetPaintBoundsFromLayer(
    Layer* layer,
    RasterCacheLayerStrategy strategy) const {
  switch (strategy) {
    case RasterCacheLayerStrategy::kLayer:
      return layer->paint_bounds();
    case RasterCacheLayerStrategy::kLayerChildren:
      FML_DCHECK(layer->as_container_layer());
      return layer->as_container_layer()->origin_child_paint_bounds();
  }
}

Issue flutter/flutter#101152 occurs because the paint_bounds are not correct, resulting in abnormal rendering (when running the test locally I found that the display and animation of the test are not correct). And worst_frame_rasterizer_time_millis is longer than normal.

After fixing this issue, worst_frame_rasterizer_time_millis of test color_filter_and_fade_perf_test was reduced from 140ms to 105ms on my test device (Nexus 6P, Android 8.1.0)

cc @flar
fyi @zanderso

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.

@ColdPaleLight ColdPaleLight force-pushed the reland_layer_children branch from 4d38201 to d685191 Compare April 1, 2022 13:34
Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

This looks good, but I'd like to see a change in the naming of the child bounds. Perhaps "child_paint_bounds()" would be simple enough on its own because that is exactly what it is.

Also, the tests could be more relevant to what the layers are doing.

@ColdPaleLight ColdPaleLight requested a review from flar April 2, 2022 13:44
Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

I didn't review every single additional test of child_paint_bounds(), but the implementation looked right and they pass, so I'm going to accept all of that on faith.

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants