Use DisplayListMatrixClipTracker in DisplayListBuilder #38349

ColdPaleLight · 2022-12-16T14:13:22Z

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.

This reverts commit b201dad.

ColdPaleLight · 2022-12-16T15:38:03Z

@flar After the modification, the clip-related test in canvas_test failed, because the default value of is_aa of Canvas.clipXXX methods is true. In DLBuilder, is_aa will be ignored, but it will be processed in DisplayListMatrixClipTracker. Please see https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8794613483671935009/+/u/test:_Host_Tests_for_host_debug/stdout?format=raw and search "Canvas.clipRect" for detail.

Should we modify those tests to make them pass? Or should we make DisplayListMatrixClipTracker ignore is_aa? Please let me know what you do you think. thanks!

flar

This looks great! Minor changes noted and fix or file an issue against Data4x4 if it is doing the inverse wrong (I didn't go through the math to see if it matters).

display_list/display_list_builder.cc

display_list/display_list_builder.h

flar · 2022-12-16T18:49:37Z

display_list/display_list_matrix_clip_tracker.cc

@@ -323,7 +336,9 @@ SkRect Data3x3::local_cull_rect() const {
    // cull rect.
    return DisplayListBuilder::kMaxCullRect;
  }
-  return inverse.mapRect(cull_rect_);
+  SkRect expended_rect;


What about the version in Data4x4?

This is dubious, btw, because we could be computing local bounds for something that won't yet have its final transform, so the roundOut may be too much or not enough depending on a scale that happens outside the picture recording...

There is roundOut logic in DLBuilder. If it is not change here, some tests in display_list_unittests.cc and canvas_test.dart will fail.

So we should change the tests instead of add roundOut here, right?

flar · 2022-12-16T20:10:29Z

@flar After the modification, the clip-related test in canvas_test failed, because the default value of is_aa of Canvas.clipXXX methods is true. In DLBuilder, is_aa will be ignored, but it will be processed in DisplayListMatrixClipTracker. Please see https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8794613483671935009/+/u/test:_Host_Tests_for_host_debug/stdout?format=raw and search "Canvas.clipRect" for detail.

Should we modify those tests to make them pass? Or should we make DisplayListMatrixClipTracker ignore is_aa? Please let me know what you do you think. thanks!

This is all about whether to do a roundOut when setting an AA clip. Ugh, I can see some arguments both ways. Let me think about it some more.

flar · 2022-12-16T20:16:39Z

One issue with roundOut is that it is assuming that it is working in device pixels. But, these measurements are relative to the environment in which the recorded DL will be displayed. In most cases that will involve a DPI adjustment that we don't see and so the roundOut is larger than it should be. But, we could just as well be compiling a huge picture that will be shown at a fractional scale - in which case roundOut won't be as large as it should be.

flar · 2022-12-16T22:42:45Z

I think we should proceed with clip(AA) doing a roundout (ignoring the fact that we aren't really talking about pixels in the builder/recorder) and DL bounds doing a roundout for AA primitives. And not doing those things on non-AA clips/primitives.

The test should be changed to specify non-AA for the clip that it executes.

flar

Should we duplicate the clip bounds tests and have a version that tests AA and another that tests non-AA?

display_list/display_list_builder.cc

ColdPaleLight · 2022-12-20T06:33:29Z

Should we duplicate the clip bounds tests and have a version that tests AA and another that tests non-AA?

Good idea. Tests has been added now.

flar

LGTM - just a nit

display_list/display_list_matrix_clip_tracker.cc

…tter/engine#38349)

…117421) * 2dd2afb49 Roll Skia from e8c3fa6d7d2f to c42beb57e108 (2 revisions) (flutter/engine#38416) * 333741df5 Roll Fuchsia Mac SDK from NS4fVXM2KhKcZ1uyD... to ev2n-_c3kgBw1h4RG... (flutter/engine#38418) * 73801d376 Roll Skia from c42beb57e108 to 557183808708 (2 revisions) (flutter/engine#38419) * 6b7ed7802 Use DisplayListMatrixClipTracker in DisplayListBuilder (flutter/engine#38349) * e3e288be8 Roll Skia from 557183808708 to 68dbdbdc2e49 (1 revision) (flutter/engine#38420) * 8e8d7b5d2 Roll Fuchsia Linux SDK from uKNwsaf92uZcX_QiY... to iQT5jpUhipvetxSiH... (flutter/engine#38421) * c08907c38 Roll Skia from 68dbdbdc2e49 to a8378cd12673 (1 revision) (flutter/engine#38422) * cf69289fb Roll Skia from a8378cd12673 to eca2fed907ac (3 revisions) (flutter/engine#38423) * bd4a60454 [Impeller] RRect blur improvements (flutter/engine#38417) * d4929a7a7 Roll Fuchsia Mac SDK from ev2n-_c3kgBw1h4RG... to nJJfWIwH5zElheIX8... (flutter/engine#38424) * 91dc9645f Roll Skia from eca2fed907ac to 34fb45763ef7 (3 revisions) (flutter/engine#38425) * 75d75575d Roll Skia from 34fb45763ef7 to 09d796c0a728 (8 revisions) (flutter/engine#38428)

flar · 2022-12-21T03:14:15Z

Something about this change causes the DisplayList.FlutterSvgIssue661BoundsWereEmpty test to fail now on my development MBP with an M1 chip. The bounds computed and compared here are very slightly off. It looks like the new code performs computations that are sensitive to the FP differences on Apple Silicon?

See also the PR now linked here: #38435

…lutter#117421) * 2dd2afb49 Roll Skia from e8c3fa6d7d2f to c42beb57e108 (2 revisions) (flutter/engine#38416) * 333741df5 Roll Fuchsia Mac SDK from NS4fVXM2KhKcZ1uyD... to ev2n-_c3kgBw1h4RG... (flutter/engine#38418) * 73801d376 Roll Skia from c42beb57e108 to 557183808708 (2 revisions) (flutter/engine#38419) * 6b7ed7802 Use DisplayListMatrixClipTracker in DisplayListBuilder (flutter/engine#38349) * e3e288be8 Roll Skia from 557183808708 to 68dbdbdc2e49 (1 revision) (flutter/engine#38420) * 8e8d7b5d2 Roll Fuchsia Linux SDK from uKNwsaf92uZcX_QiY... to iQT5jpUhipvetxSiH... (flutter/engine#38421) * c08907c38 Roll Skia from 68dbdbdc2e49 to a8378cd12673 (1 revision) (flutter/engine#38422) * cf69289fb Roll Skia from a8378cd12673 to eca2fed907ac (3 revisions) (flutter/engine#38423) * bd4a60454 [Impeller] RRect blur improvements (flutter/engine#38417) * d4929a7a7 Roll Fuchsia Mac SDK from ev2n-_c3kgBw1h4RG... to nJJfWIwH5zElheIX8... (flutter/engine#38424) * 91dc9645f Roll Skia from eca2fed907ac to 34fb45763ef7 (3 revisions) (flutter/engine#38425) * 75d75575d Roll Skia from 34fb45763ef7 to 09d796c0a728 (8 revisions) (flutter/engine#38428)

* Use DisplayListMatrixClipTracker in DisplayListBuilder * Ignore is_aa * Revert "Ignore is_aa" This reverts commit b201dad. * Tweak code * Use content_culled * getLocalClipBounds without device clip bounds roundsOut * Tweak code and add more tests * remove virtual

…lutter#117421) * 2dd2afb49 Roll Skia from e8c3fa6d7d2f to c42beb57e108 (2 revisions) (flutter/engine#38416) * 333741df5 Roll Fuchsia Mac SDK from NS4fVXM2KhKcZ1uyD... to ev2n-_c3kgBw1h4RG... (flutter/engine#38418) * 73801d376 Roll Skia from c42beb57e108 to 557183808708 (2 revisions) (flutter/engine#38419) * 6b7ed7802 Use DisplayListMatrixClipTracker in DisplayListBuilder (flutter/engine#38349) * e3e288be8 Roll Skia from 557183808708 to 68dbdbdc2e49 (1 revision) (flutter/engine#38420) * 8e8d7b5d2 Roll Fuchsia Linux SDK from uKNwsaf92uZcX_QiY... to iQT5jpUhipvetxSiH... (flutter/engine#38421) * c08907c38 Roll Skia from 68dbdbdc2e49 to a8378cd12673 (1 revision) (flutter/engine#38422) * cf69289fb Roll Skia from a8378cd12673 to eca2fed907ac (3 revisions) (flutter/engine#38423) * bd4a60454 [Impeller] RRect blur improvements (flutter/engine#38417) * d4929a7a7 Roll Fuchsia Mac SDK from ev2n-_c3kgBw1h4RG... to nJJfWIwH5zElheIX8... (flutter/engine#38424) * 91dc9645f Roll Skia from eca2fed907ac to 34fb45763ef7 (3 revisions) (flutter/engine#38425) * 75d75575d Roll Skia from 34fb45763ef7 to 09d796c0a728 (8 revisions) (flutter/engine#38428)

Use DisplayListMatrixClipTracker in DisplayListBuilder

0b94e62

ColdPaleLight requested a review from flar December 16, 2022 14:13

ColdPaleLight added 2 commits December 16, 2022 23:03

Ignore is_aa

b201dad

Revert "Ignore is_aa"

d71c31c

This reverts commit b201dad.

flar requested changes Dec 16, 2022

View reviewed changes

ColdPaleLight added 3 commits December 19, 2022 17:00

Tweak code

88aa3ce

Use content_culled

24c72bd

getLocalClipBounds without device clip bounds roundsOut

0810362

ColdPaleLight requested a review from flar December 19, 2022 10:19

flar requested changes Dec 19, 2022

View reviewed changes

display_list/display_list_builder.cc Show resolved Hide resolved

display_list/display_list_builder.cc Show resolved Hide resolved

display_list/display_list_builder.cc Outdated Show resolved Hide resolved

display_list/display_list_builder.cc Show resolved Hide resolved

Tweak code and add more tests

0aee025

ColdPaleLight requested a review from flar December 20, 2022 06:33

flar approved these changes Dec 20, 2022

View reviewed changes

display_list/display_list_matrix_clip_tracker.cc Outdated Show resolved Hide resolved

ColdPaleLight and others added 2 commits December 20, 2022 17:21

remove virtual

c6f674c

Merge branch 'flutter:main' into dlbuilder_refactor

9a6e830

ColdPaleLight added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 20, 2022

auto-submit bot merged commit 6b7ed78 into flutter:main Dec 20, 2022

engine-flutter-autoroll mentioned this pull request Dec 21, 2022

Roll Flutter Engine from 7ee3bf518036 to 75d75575d0ea (12 revisions) flutter/flutter#117421

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 21, 2022

6b7ed7802 Use DisplayListMatrixClipTracker in DisplayListBuilder (flu…

466f5fa

…tter/engine#38349)

flar mentioned this pull request Dec 21, 2022

Fix floating point rounding issue in unit test bounds check #38435

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use DisplayListMatrixClipTracker in DisplayListBuilder #38349

Use DisplayListMatrixClipTracker in DisplayListBuilder #38349

ColdPaleLight commented Dec 16, 2022 •

edited

Loading

ColdPaleLight commented Dec 16, 2022 •

edited

Loading

flar left a comment

flar Dec 16, 2022

flar Dec 16, 2022

ColdPaleLight Dec 19, 2022 •

edited

Loading

flar commented Dec 16, 2022

flar commented Dec 16, 2022

flar commented Dec 16, 2022

flar left a comment

ColdPaleLight commented Dec 20, 2022

flar left a comment

flar commented Dec 21, 2022 •

edited

Loading

Use DisplayListMatrixClipTracker in DisplayListBuilder #38349

Use DisplayListMatrixClipTracker in DisplayListBuilder #38349

Conversation

ColdPaleLight commented Dec 16, 2022 • edited Loading

Pre-launch Checklist

ColdPaleLight commented Dec 16, 2022 • edited Loading

flar left a comment

Choose a reason for hiding this comment

flar Dec 16, 2022

Choose a reason for hiding this comment

flar Dec 16, 2022

Choose a reason for hiding this comment

ColdPaleLight Dec 19, 2022 • edited Loading

Choose a reason for hiding this comment

flar commented Dec 16, 2022

flar commented Dec 16, 2022

flar commented Dec 16, 2022

flar left a comment

Choose a reason for hiding this comment

ColdPaleLight commented Dec 20, 2022

flar left a comment

Choose a reason for hiding this comment

flar commented Dec 21, 2022 • edited Loading

ColdPaleLight commented Dec 16, 2022 •

edited

Loading

ColdPaleLight commented Dec 16, 2022 •

edited

Loading

ColdPaleLight Dec 19, 2022 •

edited

Loading

flar commented Dec 21, 2022 •

edited

Loading