[flutter_tools] support bundling SkSL shaders in flutter build apk/appbundle#56059
[flutter_tools] support bundling SkSL shaders in flutter build apk/appbundle#56059jonahwilliams merged 8 commits intoflutter:masterfrom
Conversation
|
I confirmed shader loading success too :) |
jonahwilliams
added
the
tool
| final ManifestAssetBundle assetBundle = AssetBundleFactory.instance.createBundle() | ||
| as ManifestAssetBundle; |
There was a problem hiding this comment.
I don't love this cast, I think we get way too cute with factory patterns in flutter_tools--sometimes it's better to just make what you need if you need to understand the internal details of it anyway.
| bundle = json.decode(skSLBundleFile.readAsStringSync()) | ||
| as Map<String, Object>; | ||
| } on FormatException { | ||
| logger.printError('"$bundle" was not a JSON object.'); |
There was a problem hiding this comment.
Print some of the FormatException details to give the user a hint.
| logger.printError('"$bundle" was not a JSON object.'); | ||
| throw Exception('SkSL bundle was invalid.'); | ||
| } on TypeError { | ||
| logger.printError('"$bundle" was not a JSON object.'); |
There was a problem hiding this comment.
Done, also rearranged this to use an is check instead of catching the type error.
| argParser.addOption(FlutterOptions.kBundleSkSLPathOption, | ||
| help: 'A path to a file containing precompiled SkSL shaders generated ' | ||
| 'during "flutter run". These can be included in an application to ' | ||
| 'impove the first frame render times.', |
liyuqian
left a comment
liyuqian
left a comment
There was a problem hiding this comment.
RSLGTM with one nit comment. Can't wait this to merge, and having this on iOS soon too!
| ); | ||
| final Device device = flutterDevices.first.device; | ||
|
|
||
| // Convert android sub-platforms to single target platform. |
There was a problem hiding this comment.
Nit: is this conversion just to reduce the number of platform-mismatch warnings? If so, I'd probably prefer to not convert, and print warnings if one tries to use the SkSL bundle from android_x64 on android_arm64.
There was a problem hiding this comment.
I'm not sure how this would be possible today. When building an apk or appbundle, the tool builds for all ABIs simultaneously and assumes that assets are shared. It would take a significant amount of refactoring to support ABI-specific assets.
There was a problem hiding this comment.
Are the SkSL files ABI specific? If so this needs a lot more work :(
There was a problem hiding this comment.
No, they're not ABI specific. I was only thinking about giving them warnings as platform-mismatch may make them less efficient. But there's absolutely no problem of cross-ABI compatibility (as I've used x64 SkSLs on arm).
There was a problem hiding this comment.
This might be something we could fix long term then, would you file an issue with some more details on the efficiency loss from the current implementation?
There was a problem hiding this comment.
Here's an experiment on the efficiency loss: #53607 (comment)
I don't think the current implementation has any issues. If we truly want to be 100% effective across different ABIs/devices, we probably have to rely on our medium-term solution "Test-based shader warmup #53609".
5 participants
Description
upport bundling SkSL shaders into an android APK or appbundle via the --bundle-sksl-path command line options. If provided, these are validated for platform engine revision and then placed in flutter_assets/io.flutter.shaders.json
#53115 (Not marked as fixed since there are other platforms too)
Change in version logic broke PR: #54499
This is currently only supported for Android, but will be expanded to all platforms as a follow up