Don't hardcode ink sparkle SPIR-V #102674

zanderso · 2022-04-27T17:38:41Z

zanderso · 2022-04-27T19:18:34Z

@dnfield @jonahwilliams About the failing framework_tests_misc and framework_tests_libraries, do you happen to know how to get them to load the shader asset?

zanderso · 2022-04-27T19:23:15Z

Also cc @goderbauer @clocksmith

This also still needs tool tests.

jonahwilliams · 2022-04-27T19:28:48Z

We'd have to have each test manually load the asset. You could do that by overriding the root bundle and loading the bytes in with synchronous I/O. Unless we have a helper somewhere I've forgotten about

Uint8List data;
FakeAssetBundle  bundle;

setUpAll(() {
  data = File('assets/ink_sparkle.spriv).readAsBytesSync();
});

class FakeAssetBundle implements AssetBundle {
  FakeAssetBundle(this.data);
  
  final Uint8List data;
  
  ... override some methods...
}


testWidgets('test', (WidgetTester tester) {
  DefaultAssetBundle(... provide your bundle here);
});

jonahwilliams · 2022-04-28T06:15:41Z

The flutter tool can actually build an asset bundle for the test runner, but the problems are that this doesn't go down the same exact path as copyAssets and that technically the flutter framework doesn't have use-material-design on - since that would imply all apps that use flutter would use material design. Anyway, here is a small diff to work from, this outputs the compiled spirv in an easier to get to directory which you might be able to get at with regular asset loading:

diff --git a/packages/flutter_tools/lib/src/asset.dart b/packages/flutter_tools/lib/src/asset.dart
index 1bbb098d05..195d6e8f57 100644
--- a/packages/flutter_tools/lib/src/asset.dart
+++ b/packages/flutter_tools/lib/src/asset.dart
@@ -410,8 +410,7 @@ class ManifestAssetBundle implements AssetBundle {
       }
     }
     final List<_Asset> materialAssets = <_Asset>[
-      if (flutterManifest.usesMaterialDesign)
-        ..._getMaterialAssets(),
+      ..._getMaterialAssets(),
     ];
     for (final _Asset asset in materialAssets) {
       final File assetFile = asset.lookupAssetFile(_fileSystem);
diff --git a/packages/flutter_tools/lib/src/build_system/targets/assets.dart b/packages/flutter_tools/lib/src/build_system/targets/assets.dart
index eaf4c7db5c..f6bd39660f 100644
--- a/packages/flutter_tools/lib/src/build_system/targets/assets.dart
+++ b/packages/flutter_tools/lib/src/build_system/targets/assets.dart
@@ -112,7 +112,6 @@ Future<Depfile> copyAssets(
           ) && !await shaderCompiler.compileShader(
             input: content.file as File,
             outputPath: file.path,
-            relativePath: entry.key,
           )) {
             await (content.file as File).copy(file.path);
           }
diff --git a/packages/flutter_tools/lib/src/build_system/targets/shader_compiler.dart b/packages/flutter_tools/lib/src/build_system/targets/shader_compiler.dart
index b09b019f98..e73409339e 100644
--- a/packages/flutter_tools/lib/src/build_system/targets/shader_compiler.dart
+++ b/packages/flutter_tools/lib/src/build_system/targets/shader_compiler.dart
@@ -53,7 +53,6 @@ class ShaderCompiler {
   Future<bool> compileShader({
     required File input,
     required String outputPath,
-    required String relativePath,
   }) async {
        if (!input.path.endsWith('.frag')) {
                return false;
diff --git a/packages/flutter_tools/lib/src/bundle_builder.dart b/packages/flutter_tools/lib/src/bundle_builder.dart
index 2ed5e99387..9a5f2f1072 100644
--- a/packages/flutter_tools/lib/src/bundle_builder.dart
+++ b/packages/flutter_tools/lib/src/bundle_builder.dart
@@ -13,6 +13,7 @@ import 'build_info.dart';
 import 'build_system/build_system.dart';
 import 'build_system/depfile.dart';
 import 'build_system/targets/common.dart';
+import 'build_system/targets/shader_compiler.dart';
 import 'bundle.dart';
 import 'cache.dart';
 import 'devfs.dart';
@@ -148,6 +149,13 @@ Future<void> writeBundle(
   }
   bundleDir.createSync(recursive: true);

+  final ShaderCompiler shaderCompiler = ShaderCompiler(
+    processManager: globals.processManager,
+    logger: globals.logger,
+    fileSystem: globals.fs,
+    artifacts: globals.artifacts!,
+  );
+
   // Limit number of open files to avoid running out of file descriptors.
   final Pool pool = Pool(64);
   await Future.wait<void>(
@@ -161,7 +169,15 @@ Future<void> writeBundle(
         // and the native APIs will look for files this way.
         final File file = globals.fs.file(globals.fs.path.join(bundleDir.path, entry.key));
         file.parent.createSync(recursive: true);
-        await file.writeAsBytes(await entry.value.contentsAsBytes());
+        final DevFSContent devFSContent = entry.value;
+        if (devFSContent is DevFSFileContent) {
+          final File input = devFSContent.file as File;
+          if (!await shaderCompiler.compileShader(input: input, outputPath: file.path)) {
+            input.copySync(file.path);
+          }
+        } else {
+          await file.writeAsBytes(await entry.value.contentsAsBytes());
+        }
       } finally {
         resource.release();
       }
diff --git a/packages/flutter_tools/lib/src/commands/test.dart b/packages/flutter_tools/lib/src/commands/test.dart
index 026529165d..22ad9f197f 100644
--- a/packages/flutter_tools/lib/src/commands/test.dart
+++ b/packages/flutter_tools/lib/src/commands/test.dart
@@ -479,10 +479,10 @@ class TestCommand extends FlutterCommand with DeviceBasedDevelopmentArtifacts {
     if (build != 0) {
       throwToolExit('Error: Failed to build asset bundle');
     }
-    if (_needRebuild(assetBundle.entries)) {
+    // if (_needRebuild(assetBundle.entries)) {
       await writeBundle(globals.fs.directory(globals.fs.path.join('build', 'unit_test_assets')),
           assetBundle.entries);
-    }
+    //}
   }

   bool _needRebuild(Map<String, DevFSContent> entries) {

dnfield · 2022-04-28T06:22:29Z

packages/flutter/lib/src/material/ink_sparkle.dart

How confident can we be that this won't get accessed before the future completes?

How important is it to synchronously access this value?

In particular, it lookslike the only actual callsite of this is line 524 above which is in an async method already. We should either expose this as a future or just remove it.

dnfield · 2022-04-28T06:23:22Z

packages/flutter/lib/src/material/shaders/ink_sparkle.frag

newline at EOF

dnfield · 2022-04-29T04:43:31Z

packages/flutter/lib/src/material/ink_sparkle.dart

This still isn't safe but we're no worse off than we used to be. We should file an issue to deprecate this and remove it. It's not safe for anyone to use.

@dnfield Can you be more specific about what isn't safe, and what should be removed?

Sorry, providing sync access to a late initialized variable that gets initialized in a future.

So if some program invokes the getter below here that refers to this static late var before the future completes, it will get a late initialization exception and it has no way to know if that getter is safe to call yet or not.

Ah, I see. I think a bigger refactor to how these work is needed. In particular, the async nature of the shader load should probably be exposed in such a way that app developers know that they need to get it done before applying the shader in a widget tree. I'm not going to attempt that in this PR.

Will that still be necessary once impeller is just using these as precompiled blobs?

Do we need them as bytes, or could we load them in some other opaque format? I did re-open flutter/engine#32999 which would let you load assets into immutable buffers synchronously

I guess technically we should be doing that async anyway to avoid jank on low end devices

@dnfield What is the unsafe code pass? The only way to obtain an instance of this is to await the inkSparkle "static factory" above. At that point, it is guaranteed that _program is initialized, no?

(Side-note: I think compile should be a private method since it should only be invoked by inkSparkle?)

(we could assert that _program is initialized in the inkSparkle method before the return)

Ahhhh I see, in that case this is fine. I missed that part because of the way GitHub collapsed part of the diff...

packages/flutter/lib/src/material/shaders/ink_sparkle.frag

dnfield · 2022-04-29T04:45:55Z

packages/flutter_tools/lib/src/build_system/targets/shader_compiler.dart

Suggested change

ShaderCompiler({

ShaderCompiler({

dnfield · 2022-04-29T04:46:50Z

packages/flutter_tools/lib/src/build_system/targets/shader_compiler.dart

Suggested change

}) : assert(processManager != null),

assert(logger != null),

assert(fileSystem != null),

assert(artifacts != null),

_processManager = processManager,

}) : _processManager = processManager,

dnfield

test?

dnfield · 2022-04-29T04:47:29Z

packages/flutter_tools/lib/src/build_system/targets/shader_compiler.dart

File or link bugs

zanderso · 2022-04-29T05:24:28Z

This PR is blocked on #102805

zanderso · 2022-04-30T16:36:59Z

Tests are still failing on Windows. Another fix to fml for Windows is here flutter/engine#33029.

dnfield

LGTM as long as tests pass

zanderso · 2022-05-01T20:54:52Z

There will be non-trivial work to do this in the internal build. I filed b/231004915 to track that. I am holding off on landing this until the internal roll catches up a bit, and I get feedback on the internal issue from the internal build team.

packages/flutter/lib/src/material/shaders/ink_sparkle.frag

goderbauer · 2022-05-02T21:40:01Z

packages/flutter/lib/src/material/ink_sparkle.dart

We should just update the fragment_shader_manager to generate the right code. AFAICT, it's only used for this purpose and nothing else?

Yeah, we'll probably want to have a code gen solution, but it should likely be based on the reflection info emitted by impellerc and not on that package. So, spot fixes to fragment_shader_manager will probably not actually be too helpful.

zanderso · 2022-05-03T19:46:15Z

The g3 side of this is in cl/446113910.

zanderso · 2022-05-04T18:07:23Z

Size reductions: https://golem.corp.goog/PerformanceChanges?repository=flutter&revision=130917

zanderso force-pushed the ink-sparkle-asset branch from ef53dcc to be137a9 Compare April 27, 2022 17:56

goderbauer added tool Affects the "flutter" command-line tool. See also t: labels. framework flutter/packages/flutter repository. See also f: labels. labels Apr 27, 2022

zanderso added the work in progress; do not review label Apr 27, 2022

zanderso force-pushed the ink-sparkle-asset branch from be137a9 to 8cc2ecd Compare April 28, 2022 05:49

dnfield reviewed Apr 28, 2022

View reviewed changes

packages/flutter/lib/src/material/shaders/ink_sparkle.frag Outdated

Copy link

Contributor

dnfield Apr 28, 2022

Choose a reason for hiding this comment

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

newline at EOF

zanderso force-pushed the ink-sparkle-asset branch 3 times, most recently from 4d38885 to dbc26ce Compare April 29, 2022 03:36

dnfield reviewed Apr 29, 2022

View reviewed changes

packages/flutter/lib/src/material/shaders/ink_sparkle.frag Outdated Show resolved Hide resolved

dnfield reviewed Apr 29, 2022

View reviewed changes

packages/flutter_tools/lib/src/build_system/targets/shader_compiler.dart Outdated

Comment on lines 68 to 71

Copy link

Contributor

dnfield Apr 29, 2022

Choose a reason for hiding this comment

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

File or link bugs

zanderso force-pushed the ink-sparkle-asset branch from dbc26ce to 77e80eb Compare April 29, 2022 17:42

zanderso mentioned this pull request Apr 29, 2022

[Impeller] Implement the FragmentProgram API. #102853

Closed

10 tasks

zanderso force-pushed the ink-sparkle-asset branch from 77e80eb to 5a81b45 Compare April 30, 2022 03:37

zanderso force-pushed the ink-sparkle-asset branch from 5a81b45 to 37df309 Compare April 30, 2022 20:11

dnfield approved these changes Apr 30, 2022

View reviewed changes

zanderso force-pushed the ink-sparkle-asset branch from 37df309 to e687e7b Compare May 1, 2022 15:16

clocksmith mentioned this pull request May 2, 2022

Change InkSparkle SPIR-V byte code from static const to a file to prepare for Impeller #99783

Closed

goderbauer reviewed May 2, 2022

View reviewed changes

zanderso force-pushed the ink-sparkle-asset branch from e687e7b to 9ec26a0 Compare May 3, 2022 18:24

zanderso force-pushed the ink-sparkle-asset branch 2 times, most recently from f97075f to 2bab21f Compare May 3, 2022 22:28

Don't hardcode ink sparkle spir-v

7470402

zanderso force-pushed the ink-sparkle-asset branch from 2bab21f to 7470402 Compare May 4, 2022 02:47

zanderso merged commit 577919d into flutter:master May 4, 2022

zanderso deleted the ink-sparkle-asset branch May 4, 2022 16:28

engine-flutter-autoroll mentioned this pull request May 4, 2022

Roll Flutter from 75629e3e0a50 to 577919dbfe63 (3 revisions) flutter/packages#1750

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 4, 2022

577919d Don't hardcode ink sparkle spir-v (flutter/flutter#102674)

d505614

engine-flutter-autoroll mentioned this pull request May 4, 2022

Roll Flutter from 75629e3e0a50 to 82b91b0b45e5 (4 revisions) flutter/packages#1751

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 4, 2022

577919d Don't hardcode ink sparkle spir-v (flutter/flutter#102674)

bd71053

engine-flutter-autoroll mentioned this pull request May 4, 2022

Roll Flutter from b5321d182a6b to 82b91b0b45e5 (8 revisions) flutter/plugins#5626

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request May 4, 2022

577919d Don't hardcode ink sparkle spir-v (flutter/flutter#102674)

733c155

engine-flutter-autoroll mentioned this pull request May 4, 2022

Roll Flutter from 75629e3e0a50 to 74e9a6d9cd8a (5 revisions) flutter/packages#1752

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 4, 2022

577919d Don't hardcode ink sparkle spir-v (flutter/flutter#102674)

7fba214

engine-flutter-autoroll mentioned this pull request May 4, 2022

Roll Flutter from 75629e3e0a50 to 3c9f417326fa (6 revisions) flutter/packages#1753

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 4, 2022

577919d Don't hardcode ink sparkle spir-v (flutter/flutter#102674)

36c58b9

engine-flutter-autoroll mentioned this pull request May 4, 2022

Roll Flutter from 75629e3e0a50 to 098aa45f2593 (7 revisions) flutter/packages#1754

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 4, 2022

577919d Don't hardcode ink sparkle spir-v (flutter/flutter#102674)

c6a31b5

engine-flutter-autoroll mentioned this pull request May 4, 2022

Roll Flutter from 75629e3e0a50 to 2e1c146eec61 (10 revisions) flutter/packages#1755

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 4, 2022

577919d Don't hardcode ink sparkle spir-v (flutter/flutter#102674)

ce4c2d5

egramond pushed a commit to egramond/flutter that referenced this pull request May 5, 2022

Don't hardcode ink sparkle spir-v (flutter#102674)

3203969

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. c: contributor-productivity Team-specific productivity, code health, technical debt. labels May 23, 2022

zanderso mentioned this pull request May 30, 2022

[FragmentShader] Create package for transpiling GLSL into Flutter-ready SPIR-V code #93498

Closed

-  }) : assert(processManager != null),
-       assert(logger != null),
-       assert(fileSystem != null),
-       assert(artifacts != null),
-       _processManager = processManager,
+  }) : _processManager = processManager,

Don't hardcode ink sparkle SPIR-V #102674

Don't hardcode ink sparkle SPIR-V #102674

Uh oh!

Conversation

zanderso commented Apr 27, 2022 • edited by guidezpl Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zanderso commented Apr 27, 2022

Uh oh!

zanderso commented Apr 27, 2022

Uh oh!

jonahwilliams commented Apr 27, 2022

Uh oh!

jonahwilliams commented Apr 28, 2022

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!

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!

goderbauer Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zanderso commented Apr 29, 2022

Uh oh!

zanderso commented Apr 30, 2022

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

zanderso commented May 1, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zanderso commented May 3, 2022

Uh oh!

zanderso commented May 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zanderso commented Apr 27, 2022 •

edited by guidezpl

Loading

goderbauer Apr 29, 2022 •

edited

Loading