Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reland [DisplayList] Add support for clipOval to leverage Impeller optimization #53642

Merged
merged 3 commits into from
Jun 28, 2024
Merged

Reland [DisplayList] Add support for clipOval to leverage Impeller optimization #53642

merged 3 commits into from
Jun 28, 2024

Conversation

flar
Copy link
Contributor

@flar flar commented Jun 28, 2024

Reland of #53622 that checks the inverse fill flag of the paths.

@flar flar requested a review from jonahwilliams June 28, 2024 19:51
@flar
Copy link
Contributor Author

flar commented Jun 28, 2024

In looking back at this, there is no code in the engine or dart::ui code that can produce an inverse fill path, so this is a check against an unused feature of SkPaths. In reality, we would have never encountered this problem and so it is unlikely it caused the golden changes that led to the revert of the first version.

It would be nice to have our own Path objects that simply omit this inverse fill feature, but until then these tests will protect operation of the DL code.

@jonahwilliams
Copy link
Member

What was the reason for the revert?

@flar
Copy link
Contributor Author

flar commented Jun 28, 2024

What was the reason for the revert?

There were golden changes (you can see the list in the engine roll PR).
I didn't understand why these shape promotions might have caused the golden changes so I went looking for mistakes.
I noticed we didn't vet the paths against the inverse fill type and thought we should.
So, I reverted out of suspicion to get the engine rolling again while I investigated.

I now expect the same golden failures on the new version but time will tell...

@jonahwilliams
Copy link
Member

Those golden diffs seem trivial so I probably would have just accepted them. OK, good to know there were no major issues.

jonahwilliams
jonahwilliams approved these changes Jun 28, 2024
View reviewed changes
Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

@flar
Copy link
Contributor Author

flar commented Jun 28, 2024

Normally, but I didn't see how promoting ClipOval would lead to the line of edge pixels all being off by one...???

@flar flar added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 28, 2024
@auto-submit auto-submit bot merged commit d6ee3ab into flutter:main Jun 28, 2024
29 checks passed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 28, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 28, 2024
@engine-flutter-autoroll
d6ee3ab13 Reland [DisplayList] Add support for clipOval to leverage I…
42731ea 
…mpeller optimization (flutter/engine#53642)
@flar
Copy link
Contributor Author

flar commented Jun 28, 2024 •
edited
Loading

I think I figured out why it might have had an impact on goldens - we now promote rect/oval/rrect SkPaths to their corresponding primitive call in drawPath, so those paths in Skia might have slightly different computed coverages...

auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jun 29, 2024
@engine-flutter-autoroll
Roll Flutter Engine from f828363d03ff to ad1343cfb99e (3 revisions) (#…
e1feb72 
…151016)

flutter/engine@f828363...ad1343c

2024-06-28 yjbanov@google.com [web] switch from .didGain/LoseAccessibilityFocus to .focus (flutter/engine#53360)
2024-06-28 flar@google.com Reland [DisplayList] Add support for clipOval to leverage Impeller optimization (flutter/engine#53642)
2024-06-28 jonahwilliams@google.com [Impeller] experimental canvas bdf support. (flutter/engine#53597)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jimgraham@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@jiahaog
Copy link
Member

jiahaog commented Jul 3, 2024

Reason for revert: This change causes 10k golden updates internally and we need to land this out of band (go/lssc). There is also an existing issue with one particular client screenshot test - see b/350129213 for more details.

@jiahaog jiahaog added the revert Label used to revert changes in a closed and merged pull request. label Jul 3, 2024
@auto-submit Auto-submit
Copy link
Contributor

auto-submit bot commented Jul 3, 2024

Time to revert pull request flutter/engine/53642 has elapsed.
You need to open the revert manually and process as a regular pull request.

@auto-submit auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Jul 3, 2024
@jiahaog jiahaog mentioned this pull request Jul 3, 2024
Merged
auto-submit bot pushed a commit that referenced this pull request Jul 3, 2024
@jiahaog
Revert "Reland [DisplayList] Add support for clipOval to leverage Imp…
a02e3f6 
…eller optimization" (#53705)

Reverts #53642

This change causes 10k golden updates internally and we need to land this out of band (go/lssc). There is also an existing issue with one particular client screenshot test - see b/350129213 for more details.
@yjbanov yjbanov mentioned this pull request Jul 3, 2024
Closed
victorsanni pushed a commit to victorsanni/flutter that referenced this pull request Jul 8, 2024
@engine-flutter-autoroll @victorsanni
Roll Flutter Engine from f828363d03ff to ad1343cfb99e (3 revisions) (f…
ab62e78 
…lutter#151016)

flutter/engine@f828363...ad1343c

2024-06-28 yjbanov@google.com [web] switch from .didGain/LoseAccessibilityFocus to .focus (flutter/engine#53360)
2024-06-28 flar@google.com Reland [DisplayList] Add support for clipOval to leverage Impeller optimization (flutter/engine#53642)
2024-06-28 jonahwilliams@google.com [Impeller] experimental canvas bdf support. (flutter/engine#53597)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jimgraham@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
This was referenced Jul 16, 2024
Open
Closed
@flar flar mentioned this pull request Jul 24, 2024
Merged
auto-submit bot pushed a commit that referenced this pull request Jul 26, 2024
@flar
[DisplayList] Optimize ClipRRect and ClipPath to ClipOval when approp…
e28f875 
…riate (#54088)

Fixes: flutter/flutter#151850

Re-adding an optimization originally included in #53642 that detects when ClipRRect and ClipPath operations are actually ovals and redirects them to the ClipOval path during recording to save space and reduce the need for dispatchers to do the same detections and optimizations.
flar
flar commented Sep 13, 2024
View reviewed changes
display_list/dl_builder.cc
@@ -1188,6 +1223,22 @@ void DisplayListBuilder::DrawDRRect(const SkRRect& outer,
drawDRRect(outer, inner);
}
void DisplayListBuilder::drawPath(const SkPath& path) {
if (!path.isInverseFillType()) {
SkRect rect;
if (path.isRect(&rect)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fails because it should ask if the rect is closed and only pass here if it is closed or if we are filling the path.

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

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller will affect goldens
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@flar @jonahwilliams @jiahaog