Reland "Remove more calls to SkCanvas::flush() and SkSurface::flush()" #43965

kjlubick · 2023-07-24T17:37:59Z

Relanding #43902 without the copy-pasta return statements which did not seem to cause a compile issue, but caused Fuchsia tests to hang.

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

display_list/testing/dl_rendering_unittests.cc

flar

Do the rendering tests pass if you enable metal or opengl on platforms that support them?

out/<variant_dir>/display_list_rendertests --enable-metal --enable-gl

That will run all of the tests in the dl_rendering_unittests on SW, Metal, and OpenGL and ignore the flags if the platform doesn't support that back end. (At least one of Metal or OpenGL should appear in the "running on XXX, XXX backends" print statement).

kjlubick · 2023-07-25T12:18:26Z

[ERROR:flutter/display_list/testing/dl_rendering_unittests.cc(2476)] provider OpenGL not supported (ignoring)
[ERROR:flutter/display_list/testing/dl_rendering_unittests.cc(2476)] provider Metal not supported (ignoring)

:|
That's on a VM though, maybe I can still compile flutter on my bare metal machine.

kjlubick · 2023-07-25T12:54:04Z

Not supported on my bare metal machine either (at least with a host_debug_unopt default build).

I guess we trust the CI tests that are currently passing?

flar · 2023-07-25T18:02:54Z

Not supported on my bare metal machine either (at least with a host_debug_unopt default build).

I guess we trust the CI tests that are currently passing?

They pass on a SW surface, but it doesn't test how they behave on Metal/GL surfaces. Eventually we'll enable all of the surface types by default, but for now, it won't affect CI. I'll take this as a reminder to check it myself when you merge it.

flar

LGTM

…e::flush()" (flutter/engine#43965)

…131286) flutter/engine@f5fbfa8...3fff731 2023-07-25 [email protected] Roll Skia from 7cec4e4e6f6e to 12d41b6f66ed (2 revisions) (flutter/engine#44008) 2023-07-25 [email protected] Manual roll Dart SDK from 8662af7d9aa3 to 61ab5422fb7b (12 revisions) (flutter/engine#44007) 2023-07-25 [email protected] Roll Skia from eb5b5bc4fb86 to 7cec4e4e6f6e (1 revision) (flutter/engine#44003) 2023-07-25 [email protected] Reland "Remove more calls to SkCanvas::flush() and SkSurface::flush()" (flutter/engine#43965) 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 [email protected],[email protected],[email protected] 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Client CLs: - https://crrev.com/c/4636561 - flutter/engine#43684 - flutter/engine#43902 - flutter/engine#43965 - http://ag/24023223 - http://ag/24036771 - http://cl/549054892 - http://cl/548702476 - http://cl/548179304 - http://cl/548167668 - http://cl/549996422 - http://cl/547840356 Change-Id: I657d85bf9026dabfe8c872fe31d6e4ed49a027e7 Bug: skia:14317 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/727040 Commit-Queue: Kevin Lubick <[email protected]> Reviewed-by: Brian Osman <[email protected]>

gaaclarke · 2023-07-31T18:23:29Z

This may have have caused a 30% reduction in average_frame_rasterizer_time_millis (that's good).

https://flutter-flutter-perf.skia.org/e/?begin=1666638143&end=1690827261&keys=Xc266cefa78856177cd92ded480267478&requestType=0&selected=commit%3D35961%26name%3D%252Carch%253Dintel%252Cbranch%253Dmaster%252Cconfig%253Ddefault%252Cdevice_type%253DSM-G973U1%252Cdevice_version%253Dnone%252Chost_type%253Dlinux%252Csub_result%253Daverage_frame_rasterizer_time_millis%252Ctest%253Dnew_gallery_impeller__transition_perf%252C

…lutter#131286) flutter/engine@f5fbfa8...3fff731 2023-07-25 [email protected] Roll Skia from 7cec4e4e6f6e to 12d41b6f66ed (2 revisions) (flutter/engine#44008) 2023-07-25 [email protected] Manual roll Dart SDK from 8662af7d9aa3 to 61ab5422fb7b (12 revisions) (flutter/engine#44007) 2023-07-25 [email protected] Roll Skia from eb5b5bc4fb86 to 7cec4e4e6f6e (1 revision) (flutter/engine#44003) 2023-07-25 [email protected] Reland "Remove more calls to SkCanvas::flush() and SkSurface::flush()" (flutter/engine#43965) 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 [email protected],[email protected],[email protected] 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

commit e3ca856e7ed6c285734dacf87faccdbf9a321f05 Author: Kevin Lubick <[email protected]> Date: Wed Jul 26 08:58:05 2023 -0400 Remove legacy SkImage and SkSurface methods Client CLs: - http://ag/23168890 - http://ag/23171851 - http://ag/23473641 - http://ag/23494824 - http://cl/551209733 - https://crrev.com/c/4566404 - https://crrev.com/c/4705004 - flutter/engine#42425 - flutter/engine#43786 - flutter/engine#43965 Change-Id: I06c71cf6dc77aeaa3f78dec61c7b7c6eca688884 Bug: b/40045064 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/729776 Reviewed-by: Michael Ludwig <[email protected]> Commit-Queue: Kevin Lubick <[email protected]> Reviewed-by: Brian Osman <[email protected]>

flutter#43965) Relanding flutter#43902 without the copy-pasta return statements which did not seem to cause a compile issue, but caused Fuchsia tests to hang. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] 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 `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat

kjlubick added 2 commits July 24, 2023 17:30

Reland "remove more calls to SkCanvas::flush() and SkSurface::flush()"

6e3d139

flush and submit context also

f588005

github-actions bot added platform-web Code specifically for the web engine platform-fuchsia labels Jul 24, 2023

remove extra returns

f35ad3f

kjlubick marked this pull request as ready for review July 24, 2023 18:27

kjlubick requested a review from zanderso July 24, 2023 18:27

zanderso reviewed Jul 24, 2023

View reviewed changes

display_list/testing/dl_rendering_unittests.cc Outdated Show resolved Hide resolved

zanderso requested a review from flar July 24, 2023 18:36

remove more returns

981e2cd

kjlubick changed the title ~~Try flushing again~~ Reland "Remove more calls to SkCanvas::flush() and SkSurface::flush()" Jul 24, 2023

flar reviewed Jul 24, 2023

View reviewed changes

flar approved these changes Jul 25, 2023

View reviewed changes

kjlubick merged commit bb02f1a into flutter:main Jul 25, 2023

engine-flutter-autoroll mentioned this pull request Jul 25, 2023

Roll Flutter Engine from f5fbfa859b63 to 3fff7316dc8d (4 revisions) flutter/flutter#131286

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 25, 2023

bb02f1a82 Reland "Remove more calls to SkCanvas::flush() and SkSurfac…

baa6867

…e::flush()" (flutter/engine#43965)

kjlubick deleted the try-flushing-again branch August 3, 2023 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Reland "Remove more calls to SkCanvas::flush() and SkSurface::flush()" #43965

Reland "Remove more calls to SkCanvas::flush() and SkSurface::flush()" #43965

Uh oh!

kjlubick commented Jul 24, 2023 •

edited

Loading

Uh oh!

Uh oh!

flar left a comment

Uh oh!

kjlubick commented Jul 25, 2023 •

edited

Loading

Uh oh!

kjlubick commented Jul 25, 2023 •

edited

Loading

Uh oh!

flar commented Jul 25, 2023

Uh oh!

flar left a comment

Uh oh!

gaaclarke commented Jul 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Reland "Remove more calls to SkCanvas::flush() and SkSurface::flush()" #43965

Reland "Remove more calls to SkCanvas::flush() and SkSurface::flush()" #43965

Uh oh!

Conversation

kjlubick commented Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

kjlubick commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kjlubick commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flar commented Jul 25, 2023

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Jul 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kjlubick commented Jul 24, 2023 •

edited

Loading

kjlubick commented Jul 25, 2023 •

edited

Loading

kjlubick commented Jul 25, 2023 •

edited

Loading