SKP based shader warmup #20643

freiling · 2020-08-19T21:31:42Z

Description

This PR implements SKP based shader warmup within the flutter engine on fuchsia.

Tests

Each commit comes with tests for the contents of that commit. The font tests are disabled because it fails due to https://bugs.chromium.org/p/skia/issues/detail?id=10404 but I left it in so it can be re-enabled when the fix is integrated. Includes new test suites for flutter::AssetManager and flutter_runner::Engine because no tests existed for these classes.

arbreng · 2020-10-27T21:43:24Z

shell/common/skp_shader_warmup_unittests.cc

+#if defined(OS_FUCHSIA)
+#include "lib/sys/cpp/component_context.h"
+#include "third_party/skia/include/ports/SkFontMgr_fuchsia.h"



You need an #endif here or the namespace declarations are stripped out, this will cause problems on non-fuchsia builds

this ifdef block wraps basically the entire file including the close braces on the namespace. I'll take an action item to comment the endif with "// defined(OS_FUCHSIA)" though

arbreng · 2020-10-27T21:46:27Z

shell/platform/fuchsia/flutter/engine.cc

+  on_create_rasterizer = [this](flutter::Shell& shell) {
+    FML_DCHECK(surface_producer_);
+
+    WarmupSkps(


I highly recommend wrapping all of the logic you're adding in a config flag

Otherwise a revert will make you very sad :(

liyuqian

Looks good from the Flutter side with some minor nits.

liyuqian · 2020-10-28T17:40:13Z

shell/common/serialization_callbacks.cc

 namespace flutter {

 sk_sp<SkData> SerializeTypefaceWithoutData(SkTypeface* typeface, void* ctx) {
-  return typeface->serialize(SkTypeface::SerializeBehavior::kDontIncludeData);


I wonder why the old line didn't work. I guess they should both save space, so is it because typeface->serialize(SkTypeface::SerializeBehavior::kDontIncludeData) somehow interfered with the Fuchsia typeface while SkData::MakeEmpty() won't?

Is this line also the reason why we need DeserializeTypefaceWithoutData?

This commit is from a while back so I don't 100% remember. IIRC even serializing typefaces with SerializeBehavior::kDontIncludeData caused the deserialized SkPictures to not be renderable (or maybe failed to deserialize, i dont quite remember) and talking with the Skia people about it I was advised that if all I wanted to do was warm up shaders they could vend be a special standin typeface which warms up all the typeface shaders and had me file https://bugs.chromium.org/p/skia/issues/detail?id=10404 for it. So basically this just serialized nothing and then on the deserialize path creates the default typeface. Once that skia feature is in DeserializeTypefaceWithoutData will return the special warmup typeface rather than the default typeface.

liyuqian · 2020-10-28T17:42:33Z

shell/common/shell_unittests.cc

+                     mapping->GetSize());
+
+  ASSERT_TRUE(result == filename);
+}


Nit: add a new line here.

liyuqian · 2020-10-28T17:48:05Z

shell/common/shell_unittests.cc

+  std::string filename = "test";
+
+  bool success = fml::WriteAtomically(asset_dir_fd, filename.c_str(),
+                                      fml::DataMapping(filename));


Nit: maybe create std::string content = "test" and write it into the file, so we'll have ASSERT_TRUE(result == content); which is less confusing than ASSERT_TRUE(result == filename);.

chinmaygarde · 2020-10-29T21:37:15Z

The test failure in presubmit looks related to this patch:

Assertion failed: image && image->aspects & VK_IMAGE_ASPECT_ANY_COLOR_BIT_ANV 
(../../third_party/mesa/src/intel/vulkan/genX_cmd_buffer.c: transition_color_buffer: 1126)

Asset Manager. Gives Asset Manager the ability to find files matching a specific pattern and uses that functionality to implement an entry point in PersistantCache which retreives mappings for all skp files in the Asset Bundle.

Make Engine load all .skp files from persistent cache and draw them on an offscreen surface to warm up shaders.

freiling · 2020-10-30T23:21:27Z

The test failure in presubmit looks related to this patch:

Assertion failed: image && image->aspects & VK_IMAGE_ASPECT_ANY_COLOR_BIT_ANV 
(../../third_party/mesa/src/intel/vulkan/genX_cmd_buffer.c: transition_color_buffer: 1126)

I'm not seeing this test, do you have a link or instructions on how to get to it?

googlebot added the cla: yes label Aug 19, 2020

auto-assign bot requested a review from flar August 19, 2020 21:34

freiling force-pushed the freiling-skp-warmup branch from 8cd322f to b9f6146 Compare September 3, 2020 21:27

freiling force-pushed the freiling-skp-warmup branch 2 times, most recently from 47ae5c2 to 85b7ce9 Compare September 15, 2020 00:55

chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label Oct 1, 2020

freiling force-pushed the freiling-skp-warmup branch 7 times, most recently from 626596c to 8d1126f Compare October 27, 2020 18:21

freiling removed the Work in progress (WIP) Not ready (yet) for review! label Oct 27, 2020

freiling requested review from arbreng, chinmaygarde and liyuqian October 27, 2020 18:22

freiling changed the title ~~[WIP] SKP shader warmup~~ SKP based shader warmup Oct 27, 2020

freiling force-pushed the freiling-skp-warmup branch 2 times, most recently from 0dc5b34 to 46d6fdb Compare October 27, 2020 19:56

arbreng approved these changes Oct 27, 2020

View reviewed changes

freiling force-pushed the freiling-skp-warmup branch from 46d6fdb to d9a50db Compare October 27, 2020 22:46

liyuqian approved these changes Oct 28, 2020

View reviewed changes

freiling force-pushed the freiling-skp-warmup branch 2 times, most recently from 303c999 to 7f78a28 Compare October 28, 2020 22:06

freiling added 3 commits October 30, 2020 21:45

[fuchsia][skp warmup] dont serialize font data

d2010c9

[fuchsia] implement SKP based shader warmup

6c9924f

Make Engine load all .skp files from persistent cache and draw them on an offscreen surface to warm up shaders.

freiling force-pushed the freiling-skp-warmup branch from 7f78a28 to 1b152e0 Compare October 30, 2020 22:09

[fuchsia] runtime flag for shader warmup.

cd1fe95

freiling force-pushed the freiling-skp-warmup branch from 1b152e0 to cd1fe95 Compare November 3, 2020 23:31

freiling added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Nov 5, 2020

fluttergithubbot merged commit 3105db8 into flutter:master Nov 5, 2020

engine-flutter-autoroll mentioned this pull request Nov 5, 2020

Roll Engine from e66a720137d9 to 3105db8ee856 (1 revision) flutter/flutter#69864

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 5, 2020

3105db8 SKP based shader warmup (flutter/engine#20643)

bfcc954

engine-flutter-autoroll mentioned this pull request Nov 5, 2020

Roll Engine from e66a720137d9 to 8173f919307d (2 revisions) flutter/flutter#69869

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 5, 2020

3105db8 SKP based shader warmup (flutter/engine#20643)

e01bdf7

engine-flutter-autoroll mentioned this pull request Nov 5, 2020

Roll Engine from bf259226b224 to 0693ee04d130 (9 revisions) flutter/flutter#69906

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 5, 2020

3105db8 SKP based shader warmup (flutter/engine#20643)

c190dbe

chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020

SKP based shader warmup (flutter#20643)

6b900c1

SKP based shader warmup #20643

SKP based shader warmup #20643

Uh oh!

Conversation

freiling commented Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liyuqian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Oct 29, 2020

Uh oh!

freiling commented Oct 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

freiling commented Aug 19, 2020 •

edited

Loading