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

[Impeller] Add rects to accumulator rather than bounds #37435

Merged
merged 1 commit into from
Nov 8, 2022
Merged

[Impeller] Add rects to accumulator rather than bounds #37435

merged 1 commit into from
Nov 8, 2022

Conversation

iskakaushik
Copy link
Contributor

When the accumulator is an RTreeBoundsAccumulator rather than a RectBoundsAccumulator just accumulating the bounds results in incorrect results as the rtree would need to be aware of the constituent non-overlapping rectangles. This would work fine for RectBoundsAccumulator as it would just adjust its bounds based on the passed rects.

Fixes: flutter/flutter#113251

@iskakaushik
Add rects to accumulator rather than bounds
868f8a2 
When the accumulator is an `RTreeBoundsAccumulator` rather than a `RectBoundsAccumulator` just accumulating the bounds results in incorrect results as the `rtree` would need to be aware of the constituent non-overlapping rectangles. This would work fine for `RectBoundsAccumulator` as it would just adjust its bounds based on the passed rects.

Fixes: flutter/flutter#113251
@iskakaushik iskakaushik requested review from jmagman and flar November 8, 2022 18:04
@flutter-dashboard
Copy link

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.

@iskakaushik iskakaushik added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 8, 2022
@auto-submit auto-submit bot merged commit 306b0fe into flutter:main Nov 8, 2022
@jmagman jmagman mentioned this pull request Nov 8, 2022
Merged
8 tasks
@iskakaushik iskakaushik changed the title Add rects to accumulator rather than bounds [Impeller] Add rects to accumulator rather than bounds Nov 8, 2022
@iskakaushik iskakaushik self-assigned this Nov 8, 2022
@jmagman
Copy link
Member

jmagman commented Nov 8, 2022

It looks like this pull request may not have tests.

flutter/cocoon#2294

@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 8, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 8, 2022
@engine-flutter-autoroll
306b0fe9c Add rects to accumulator rather than bounds (flutter/engine…
1a76e34 
…#37435)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 8, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 8, 2022
@engine-flutter-autoroll
306b0fe9c Add rects to accumulator rather than bounds (flutter/engine…
4681c03 
…#37435)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 8, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 8, 2022
@engine-flutter-autoroll
306b0fe9c Add rects to accumulator rather than bounds (flutter/engine…
51f1975 
…#37435)
flar
flar reviewed Nov 8, 2022
View reviewed changes
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 would have submitted this review with "request changes", but it is too late as someone else already approved it.

display_list/display_list_utils.cc
@@ -672,7 +672,12 @@ void DisplayListBoundsCalculator::drawPicture(const sk_sp<SkPicture> picture,
}
void DisplayListBoundsCalculator::drawDisplayList(
const sk_sp<DisplayList> display_list) {
AccumulateOpBounds(display_list->bounds(), kDrawDisplayListFlags);
const SkRect bounds = display_list->bounds();
std::list<SkRect> rects =
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an expensive operation that must now be done on any bounds calculation whether or not it is attempting to accumulate an RTree. This option must be conditional.

display_list/display_list_utils.cc
std::list<SkRect> rects =
display_list->rtree()->searchNonOverlappingDrawnRects(bounds);
for (const SkRect& rect : rects) {
AccumulateOpBounds(rect, kDrawDisplayListFlags);
Copy link
Contributor

Choose a reason for hiding this comment

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

The use of kDrawDisplayListFlags here is problematic as it isn't clear if the attributes assigned to an entire display list apply to each individual rectangle.

For now, we don't apply any attributes, but in the future we might apply attributes and they can't necessarily be distributed like that. So, this works presently, but is a potential logic time bomb. Minimally a comment should be added to mention this discrepancy.

@jmagman
Copy link
Member

jmagman commented Nov 8, 2022

@iskakaushik sounds like this needs a revert to address @flar's concerns?

@iskakaushik
Copy link
Contributor Author

yup, reverting now

iskakaushik added a commit to iskakaushik/engine that referenced this pull request Nov 8, 2022
@iskakaushik
Revert "Add rects to accumulator rather than bounds (flutter#37435)"
3e4419f 
This reverts commit 306b0fe.
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 8, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 8, 2022
@engine-flutter-autoroll
306b0fe9c Add rects to accumulator rather than bounds (flutter/engine…
696e76c 
…#37435)
@iskakaushik iskakaushik mentioned this pull request Nov 8, 2022
Open
iskakaushik added a commit to iskakaushik/engine that referenced this pull request Nov 8, 2022
@iskakaushik
[Reland] Add rects to accumulator rather than bounds (flutter#37435)
c6b5c5b 
When the accumulator is an `RTreeBoundsAccumulator` rather than a `RectBoundsAccumulator` just accumulating the bounds results in incorrect results as the `rtree` would need to be aware of the constituent non-overlapping rectangles. This would work fine for `RectBoundsAccumulator` as it would just adjust its bounds based on the passed rects.

Fixes: flutter/flutter#113251
iskakaushik added a commit to iskakaushik/engine that referenced this pull request Nov 8, 2022
@iskakaushik
[Reland] Add rects to accumulator rather than bounds (flutter#37435)
82e9f94 
When the accumulator is an `RTreeBoundsAccumulator` rather than a `RectBoundsAccumulator` just accumulating the bounds results in incorrect results as the `rtree` would need to be aware of the constituent non-overlapping rectangles. This would work fine for `RectBoundsAccumulator` as it would just adjust its bounds based on the passed rects.

Fixes: flutter/flutter#113251
iskakaushik added a commit that referenced this pull request Nov 8, 2022
@iskakaushik
Revert "Add rects to accumulator rather than bounds (#37435)" (#37444)
c171f27 
This reverts commit 306b0fe.
iskakaushik added a commit to iskakaushik/engine that referenced this pull request Nov 8, 2022
@iskakaushik
[Reland] Add rects to accumulator rather than bounds (flutter#37435)
a07335a 
When the accumulator is an `RTreeBoundsAccumulator` rather than a `RectBoundsAccumulator` just accumulating the bounds results in incorrect results as the `rtree` would need to be aware of the constituent non-overlapping rectangles. This would work fine for `RectBoundsAccumulator` as it would just adjust its bounds based on the passed rects.

Fixes: flutter/flutter#113251
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022
@engine-flutter-autoroll
306b0fe9c Add rects to accumulator rather than bounds (flutter/engine…
a38deab 
…#37435)
auto-submit bot pushed a commit that referenced this pull request Nov 9, 2022
@iskakaushik
[Reland] Add rects to accumulator rather than bounds (#37435) (#37451)
f8048be 
* [Reland] Add rects to accumulator rather than bounds (#37435)

When the accumulator is an `RTreeBoundsAccumulator` rather than a `RectBoundsAccumulator` just accumulating the bounds results in incorrect results as the `rtree` would need to be aware of the constituent non-overlapping rectangles. This would work fine for `RectBoundsAccumulator` as it would just adjust its bounds based on the passed rects.

Fixes: flutter/flutter#113251

* Add a test for nested display list rtree
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 9, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022
@engine-flutter-autoroll
306b0fe9c Add rects to accumulator rather than bounds (flutter/engine…
1f13fbb 
…#37435)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 9, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022
@engine-flutter-autoroll
306b0fe9c Add rects to accumulator rather than bounds (flutter/engine…
cf85069 
…#37435)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 9, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022
@engine-flutter-autoroll
306b0fe9c Add rects to accumulator rather than bounds (flutter/engine…
f33e514 
…#37435)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 9, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022
@engine-flutter-autoroll
306b0fe9c Add rects to accumulator rather than bounds (flutter/engine…
2b29b38 
…#37435)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 9, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022
@engine-flutter-autoroll
306b0fe9c Add rects to accumulator rather than bounds (flutter/engine…
ade5299 
…#37435)
iskakaushik added a commit to iskakaushik/engine that referenced this pull request Nov 9, 2022
@iskakaushik
Revert "[Reland] Add rects to accumulator rather than bounds (flutter…
7da29fe 
…#37435) (flutter#37451)"

This reverts commit f8048be.

pre-emptively reverting
flutter/flutter#114997 (comment)
to unblock the roll.
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 9, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022
@engine-flutter-autoroll
306b0fe9c Add rects to accumulator rather than bounds (flutter/engine…
906c013 
…#37435)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 9, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022
@engine-flutter-autoroll
306b0fe9c Add rects to accumulator rather than bounds (flutter/engine…
b4182bd 
…#37435)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 9, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022
@engine-flutter-autoroll
306b0fe9c Add rects to accumulator rather than bounds (flutter/engine…
b09c33d 
…#37435)
naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Nov 9, 2022
@iskakaushik
Add rects to accumulator rather than bounds (flutter#37435)
75e1346 
When the accumulator is an `RTreeBoundsAccumulator` rather than a `RectBoundsAccumulator` just accumulating the bounds results in incorrect results as the `rtree` would need to be aware of the constituent non-overlapping rectangles. This would work fine for `RectBoundsAccumulator` as it would just adjust its bounds based on the passed rects.

Fixes: flutter/flutter#113251
naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Nov 9, 2022
@iskakaushik
Revert "Add rects to accumulator rather than bounds (flutter#37435)" (f…
1471086 
…lutter#37444)

This reverts commit 306b0fe.
naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Nov 9, 2022
@iskakaushik
[Reland] Add rects to accumulator rather than bounds (flutter#37435) (f…
3917224 
…lutter#37451)

* [Reland] Add rects to accumulator rather than bounds (flutter#37435)

When the accumulator is an `RTreeBoundsAccumulator` rather than a `RectBoundsAccumulator` just accumulating the bounds results in incorrect results as the `rtree` would need to be aware of the constituent non-overlapping rectangles. This would work fine for `RectBoundsAccumulator` as it would just adjust its bounds based on the passed rects.

Fixes: flutter/flutter#113251

* Add a test for nested display list rtree
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 9, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022
@engine-flutter-autoroll
306b0fe9c Add rects to accumulator rather than bounds (flutter/engine…
b9529ec 
…#37435)
zanderso pushed a commit to flutter/flutter that referenced this pull request Nov 9, 2022
@engine-flutter-autoroll
Roll Flutter Engine from e7d7edab98ad to c76035429c36 (14 revisions) (#…
8e5439c 
…115008)

* de0b58e82 clang-tidy: added the ability to shard jobs (flutter/engine#37265)

* 8aefb8b61 Roll Dart SDK from d97d5ad98893 to b22b36c0f52e (1 revision) (flutter/engine#37400)

* 2f043090f Roll Skia from aef6d301c0b5 to 0c8127b3dd7d (6 revisions) (flutter/engine#37402)

* dc7cb20ad Fix boolean json property. (flutter/engine#37403)

* 512fa40b6 Adding release_build:true property to windows builders. (flutter/engine#37397)

* 0af87071f Add test for image readback (flutter/engine#37401)

* c6f26e035 Update display_list_image_filter_unittests to be permit Skia roll (flutter/engine#37327)

* 4e9e97b4b Roll Skia from 0c8127b3dd7d to 8b19fb0f57d4 (1 revision) (flutter/engine#37409)

* 42c2940ff Pass the correct name for gclient variables in ci.yaml (flutter/engine#37429)

* 2e2a2fd97 Revert "Roll Skia from 0c8127b3dd7d to 8b19fb0f57d4 (1 revision) (#37409)" (flutter/engine#37430)

* 02cb78905 Roll Fuchsia Mac SDK from sa5bVGimNo3JwLV27... to d4l6A1aPw6Z0YjxmA... (flutter/engine#37432)

* c24ae1872 [web] Use v8BreakIterator where possible (flutter/engine#37317)

* 306b0fe9c Add rects to accumulator rather than bounds (flutter/engine#37435)

* c76035429 Roll Dart SDK from b22b36c0f52e to c15cdb978761 (2 revisions) (flutter/engine#37437)
schwa423 pushed a commit to schwa423/engine that referenced this pull request Nov 16, 2022
@iskakaushik @schwa423
Add rects to accumulator rather than bounds (flutter#37435)
576c547 
When the accumulator is an `RTreeBoundsAccumulator` rather than a `RectBoundsAccumulator` just accumulating the bounds results in incorrect results as the `rtree` would need to be aware of the constituent non-overlapping rectangles. This would work fine for `RectBoundsAccumulator` as it would just adjust its bounds based on the passed rects.

Fixes: flutter/flutter#113251
schwa423 pushed a commit to schwa423/engine that referenced this pull request Nov 16, 2022
@iskakaushik @schwa423
Revert "Add rects to accumulator rather than bounds (flutter#37435)" (f…
f0c752e 
…lutter#37444)

This reverts commit 306b0fe.
schwa423 pushed a commit to schwa423/engine that referenced this pull request Nov 16, 2022
@iskakaushik @schwa423
[Reland] Add rects to accumulator rather than bounds (flutter#37435) (f…
4f9706e 
…lutter#37451)

* [Reland] Add rects to accumulator rather than bounds (flutter#37435)

When the accumulator is an `RTreeBoundsAccumulator` rather than a `RectBoundsAccumulator` just accumulating the bounds results in incorrect results as the `rtree` would need to be aware of the constituent non-overlapping rectangles. This would work fine for `RectBoundsAccumulator` as it would just adjust its bounds based on the passed rects.

Fixes: flutter/flutter#113251

* Add a test for nested display list rtree
shogohida pushed a commit to shogohida/flutter that referenced this pull request Dec 7, 2022
@engine-flutter-autoroll @shogohida
Roll Flutter Engine from e7d7edab98ad to c76035429c36 (14 revisions) (f…
e078af1 
…lutter#115008)

* de0b58e82 clang-tidy: added the ability to shard jobs (flutter/engine#37265)

* 8aefb8b61 Roll Dart SDK from d97d5ad98893 to b22b36c0f52e (1 revision) (flutter/engine#37400)

* 2f043090f Roll Skia from aef6d301c0b5 to 0c8127b3dd7d (6 revisions) (flutter/engine#37402)

* dc7cb20ad Fix boolean json property. (flutter/engine#37403)

* 512fa40b6 Adding release_build:true property to windows builders. (flutter/engine#37397)

* 0af87071f Add test for image readback (flutter/engine#37401)

* c6f26e035 Update display_list_image_filter_unittests to be permit Skia roll (flutter/engine#37327)

* 4e9e97b4b Roll Skia from 0c8127b3dd7d to 8b19fb0f57d4 (1 revision) (flutter/engine#37409)

* 42c2940ff Pass the correct name for gclient variables in ci.yaml (flutter/engine#37429)

* 2e2a2fd97 Revert "Roll Skia from 0c8127b3dd7d to 8b19fb0f57d4 (1 revision) (flutter#37409)" (flutter/engine#37430)

* 02cb78905 Roll Fuchsia Mac SDK from sa5bVGimNo3JwLV27... to d4l6A1aPw6Z0YjxmA... (flutter/engine#37432)

* c24ae1872 [web] Use v8BreakIterator where possible (flutter/engine#37317)

* 306b0fe9c Add rects to accumulator rather than bounds (flutter/engine#37435)

* c76035429 Roll Dart SDK from b22b36c0f52e to c15cdb978761 (2 revisions) (flutter/engine#37437)
gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023
@engine-flutter-autoroll @gspencergoog
Roll Flutter Engine from e7d7edab98ad to c76035429c36 (14 revisions) (f…
3193748 
…lutter#115008)

* de0b58e82 clang-tidy: added the ability to shard jobs (flutter/engine#37265)

* 8aefb8b61 Roll Dart SDK from d97d5ad98893 to b22b36c0f52e (1 revision) (flutter/engine#37400)

* 2f043090f Roll Skia from aef6d301c0b5 to 0c8127b3dd7d (6 revisions) (flutter/engine#37402)

* dc7cb20ad Fix boolean json property. (flutter/engine#37403)

* 512fa40b6 Adding release_build:true property to windows builders. (flutter/engine#37397)

* 0af87071f Add test for image readback (flutter/engine#37401)

* c6f26e035 Update display_list_image_filter_unittests to be permit Skia roll (flutter/engine#37327)

* 4e9e97b4b Roll Skia from 0c8127b3dd7d to 8b19fb0f57d4 (1 revision) (flutter/engine#37409)

* 42c2940ff Pass the correct name for gclient variables in ci.yaml (flutter/engine#37429)

* 2e2a2fd97 Revert "Roll Skia from 0c8127b3dd7d to 8b19fb0f57d4 (1 revision) (flutter#37409)" (flutter/engine#37430)

* 02cb78905 Roll Fuchsia Mac SDK from sa5bVGimNo3JwLV27... to d4l6A1aPw6Z0YjxmA... (flutter/engine#37432)

* c24ae1872 [web] Use v8BreakIterator where possible (flutter/engine#37317)

* 306b0fe9c Add rects to accumulator rather than bounds (flutter/engine#37435)

* c76035429 Roll Dart SDK from b22b36c0f52e to c15cdb978761 (2 revisions) (flutter/engine#37437)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@flar flar flar left review comments

@jmagman jmagman jmagman approved these changes

Assignees

@iskakaushik iskakaushik

Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller needs tests
Projects
None yet
Milestone
No milestone
3 participants
@iskakaushik @jmagman @flar