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

[Impeller] Make IPLR files multi-platform #49253

Merged
merged 13 commits into from
Dec 21, 2023
Merged

[Impeller] Make IPLR files multi-platform #49253

merged 13 commits into from
Dec 21, 2023

Conversation

dnfield
Copy link
Contributor

@dnfield dnfield commented Dec 19, 2023

This is part of the work towards supporting OpenGLES and Vulkan for runtime stage shaders.

Removes some redundant work we had around SkSL. Now only bundles the shaders we actually ask for from the command line.

@bdero, we should figure out if this is the right approach for flutter_gpu.

With this change, the IPLR format goes from having a root table of shader related information to a root table of shader information per sksl, metal, opengles, and vulkan platforms.

This may end up allowing us to revert #47278, but I'm not sure I understand all the implications of that at this point.

I have run some but not all tests locally.

@dnfield
[Impeller] Make IPLR files multi-platform
4884d22 
This is part of the work towards supporting OpenGLES and Vulkan
for runtime stage shaders.

Removes some redundant work we had around SkSL. Now only bundles
the shaders we actually ask for from the command line.

@bdero, we should figure out if this is the right approach for
flutter_gpu.
bdero
bdero approved these changes Dec 19, 2023
View reviewed changes
lib/gpu/shader_library.cc
@@ -123,8 +123,26 @@ fml::RefPtr<ShaderLibrary> ShaderLibrary::MakeFromFlatbuffer(
ShaderLibrary::ShaderMap shader_map;

for (const auto* bundled_shader : *bundle->shaders()) {
const impeller::fb::RuntimeStage* runtime_stage = bundled_shader->shader();
impeller::RuntimeStage stage(runtime_stage);
auto* runtime_stages = bundled_shader->shader();
Copy link
Member

Choose a reason for hiding this comment

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

we should figure out if this is the right approach for flutter_gpu.

I couldn't reconcile the behavioral differences between where we need to end up with Flutter GPU and RuntimeStage, so I've been working on a patch that completely separates the reflection logic/packing of the flatbuffer (and I think we'll wind up with separated flatbuffers in the end, since runtime stage doesn't need input reflection, etc). Just turned it into a draft PR: #49256

From the looks of these changes, I think this shouldn't impact Flutter GPU's ability to work on Metal. And these conflicts will be easy to deal with.

impeller/compiler/impellerc_main.cc
break;
case TargetPlatform::kRuntimeStageVulkan:
stages.AddShader(RuntimeStageBackend::kVulkan, stage_data);
break;
Copy link
Member

@bdero bdero Dec 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Just an FYI: once you get to the point of invoking the shader compiler multiple times for --iplr and packing multiple backends into one flatbuffer, you'll probably need to branch off and employ a similar approach to what happens for --shader-bundle and:

  • Avoid generating intermediates for, reflection headers, etc.
  • Remove all the unnecessary flags when iplr is enabled in the impellerc GN template.
  • Relax the restrictions in switches.cc for iplr mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I need to fix at least some of that as part of this PR. Looking into the failure on flutter_tester when we --enable-impeller

@dnfield
Copy link
Contributor Author

dnfield commented Dec 19, 2023

I'm going to try to manually verify that this did not regress fragment programs on iOS with and without impeller. I'm suspicious of the test coverage we have here for that.

@dnfield dnfield mentioned this pull request Dec 19, 2023
Closed
@dnfield
Copy link
Contributor Author

dnfield commented Dec 19, 2023

This actually improves things for Skia on iOS right now, and does not break things for Impeller in some tests I've run locally. Great.

Some of the shell test platform views aren't exposing an Impeller Context, looking into that...

@dnfield dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 19, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Dec 20, 2023
@auto-submit Auto-submit
Copy link
Contributor

auto-submit bot commented Dec 20, 2023

auto label is removed for flutter/engine/49253, due to - The status or check suite Linux mac_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

  • The status or check suite Mac mac_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux linux_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux linux_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

@dnfield dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 20, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Dec 20, 2023
@auto-submit Auto-submit
Copy link
Contributor

auto-submit bot commented Dec 20, 2023

auto label is removed for flutter/engine/49253, due to - The status or check suite Linux linux_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

  • The status or check suite Linux Framework Smoke Tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

@dnfield dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 20, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Dec 20, 2023
@auto-submit Auto-submit
Copy link
Contributor

auto-submit bot commented Dec 20, 2023

auto label is removed for flutter/engine/49253, due to - The status or check suite Linux mac_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

  • The status or check suite Mac mac_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux linux_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Linux linux_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

@dnfield dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 20, 2023
@dnfield
Copy link
Contributor Author

dnfield commented Dec 20, 2023

Failures are all flakes, reset xcode/gsutil upload logs failing... :\

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Dec 20, 2023
@auto-submit Auto-submit
Copy link
Contributor

auto-submit bot commented Dec 20, 2023

auto label is removed for flutter/engine/49253, due to - The status or check suite Linux linux_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

  • The status or check suite Linux linux_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

@dnfield dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 20, 2023
@dnfield
Copy link
Contributor Author

dnfield commented Dec 20, 2023

I've added some more skips, because FragmentProgram.fromAsset just plain doesn't work with --enable-impeller right now - it ends up trying to find the Vulkan backend shaders which we can't produce at the moment.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Dec 20, 2023
@auto-submit Auto-submit
Copy link
Contributor

auto-submit bot commented Dec 20, 2023

auto label is removed for flutter/engine/49253, due to - The status or check suite Linux Framework Smoke Tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

@dnfield dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 20, 2023
@dnfield dnfield mentioned this pull request Dec 20, 2023
Closed
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Dec 20, 2023
@auto-submit Auto-submit
Copy link
Contributor

auto-submit bot commented Dec 20, 2023

auto label is removed for flutter/engine/49253, due to - The status or check suite Linux linux_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

  • The status or check suite Linux linux_clang_tidy has failed. Please fix the issues identified (or deflake) before re-applying this label.

@dnfield dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 21, 2023
@auto-submit auto-submit bot merged commit c53fd4b into flutter:main Dec 21, 2023
28 checks passed
@dnfield dnfield deleted the impellerc branch December 21, 2023 15:08
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Dec 21, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 21, 2023
@engine-flutter-autoroll
c53fd4b1d [Impeller] Make IPLR files multi-platform (flutter/engine#4…
5bf01a6 
…9253)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Dec 21, 2023
@engine-flutter-autoroll
Roll Flutter Engine from c70f0a495ace to 1b1b2a12a597 (32 revisions) (#…
b417fb8 
…140519)

Roll Flutter Engine from c70f0a495ace to 1b1b2a12a597 (32 revisions)

flutter/engine@c70f0a4...1b1b2a1

2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 4a10533a4dc8 to bcf68d22f0fa (1 revision) (flutter/engine#49329)
2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 1d0c3ecd1349 to 4a10533a4dc8 (1 revision) (flutter/engine#49326)
2023-12-21 flar@google.com Ensure sorted rects in ui.Canvas for legacy compatibility (flutter/engine#49309)
2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 63a452b45026 to 1d0c3ecd1349 (1 revision) (flutter/engine#49318)
2023-12-21 dnfield@google.com [Impeller] Make IPLR files multi-platform (flutter/engine#49253)
2023-12-21 ditman@gmail.com [web] Defer injection of platform views until needed. (flutter/engine#48960)
2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 1aef027ec953 to 63a452b45026 (2 revisions) (flutter/engine#49311)
2023-12-21 skia-flutter-autoroll@skia.org Roll Skia from 29917d8c97ca to 4b16117e94b2 (4 revisions) (flutter/engine#49310)
2023-12-21 737941+loic-sharma@users.noreply.github.com Reland "[Windows] Move to FlutterCompositor for rendering" (flutter/engine#49262)
2023-12-21 30870216+gaaclarke@users.noreply.github.com Reland `[Impeller] new blur: refactored math and fixed expanded padding size` (flutter/engine#49302)
2023-12-20 dkwingsmt@users.noreply.github.com Multiview pipeline Pt. 1: Skip illegal render calls (flutter/engine#49266)
2023-12-20 barpac02@gmail.com SemanticsUpdateBuilder: make all args non-null (flutter/engine#49148)
2023-12-20 30870216+gaaclarke@users.noreply.github.com [Impeller] fixed Rect::Contains (flutter/engine#49294)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 97c3b7e1885a to 1aef027ec953 (1 revision) (flutter/engine#49295)
2023-12-20 30870216+gaaclarke@users.noreply.github.com Revert "[Impeller] new blur: refactored math and fixed expanded padding size" (flutter/engine#49298)
2023-12-20 30870216+gaaclarke@users.noreply.github.com [Impeller] new blur: refactored math and fixed expanded padding size (flutter/engine#49206)
2023-12-20 dkwingsmt@users.noreply.github.com Multi-view pointer event (flutter/engine#46213)
2023-12-20 1961493+harryterkelsen@users.noreply.github.com [web:multiview] Only call `Renderer.clearFragmentProgramCache` on hot restart (flutter/engine#48758)
2023-12-20 skia-flutter-autoroll@skia.org Roll Skia from 9cb1bb1164ea to 29917d8c97ca (1 revision) (flutter/engine#49289)
2023-12-20 bdero@google.com [Impeller] Add interactive Blur+Clip AiksTest. (flutter/engine#49283)
2023-12-20 sergiy.dubovik@supercell.com [macos] FlutterKeyboardManager memory leak fix (flutter/engine#48824)
2023-12-20 zanderso@users.noreply.github.com Don't guard Windows arm64 Dart SDK download on the release candidate flag (flutter/engine#49244)
2023-12-20 15619084+vashworth@users.noreply.github.com Fix testAppExtensionLaunching for Xcode 15/iOS 17 (flutter/engine#49242)
2023-12-20 skia-flutter-autoroll@skia.org Roll Skia from 8060d6b36066 to 9cb1bb1164ea (2 revisions) (flutter/engine#49288)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from ed415d966d8a to 97c3b7e1885a (1 revision) (flutter/engine#49287)
2023-12-20 skia-flutter-autoroll@skia.org Roll Skia from d0f09ad481f7 to 8060d6b36066 (1 revision) (flutter/engine#49285)
2023-12-20 kevinjchisholm@gmail.com [release] Update release config (flutter/engine#49254)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 1732c4c92ccd to ed415d966d8a (1 revision) (flutter/engine#49274)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 4c59838945d9 to 1732c4c92ccd (1 revision) (flutter/engine#49269)
2023-12-20 goderbauer@google.com Sync lints with flutter/flutter (flutter/engine#49192)
2023-12-19 1961493+harryterkelsen@users.noreply.github.com [web] Enforce onDrawFrame/onBeginFrame render rule (flutter/engine#49214)
2023-12-19 barpac02@gmail.com [Docs] Add more info about running tests on iOS (flutter/engine#48859)

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
...
CoderDake pushed a commit to CoderDake/flutter that referenced this pull request Dec 28, 2023
@engine-flutter-autoroll @CoderDake
Roll Flutter Engine from c70f0a495ace to 1b1b2a12a597 (32 revisions) (f…
e8d8e07 
…lutter#140519)

Roll Flutter Engine from c70f0a495ace to 1b1b2a12a597 (32 revisions)

flutter/engine@c70f0a4...1b1b2a1

2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 4a10533a4dc8 to bcf68d22f0fa (1 revision) (flutter/engine#49329)
2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 1d0c3ecd1349 to 4a10533a4dc8 (1 revision) (flutter/engine#49326)
2023-12-21 flar@google.com Ensure sorted rects in ui.Canvas for legacy compatibility (flutter/engine#49309)
2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 63a452b45026 to 1d0c3ecd1349 (1 revision) (flutter/engine#49318)
2023-12-21 dnfield@google.com [Impeller] Make IPLR files multi-platform (flutter/engine#49253)
2023-12-21 ditman@gmail.com [web] Defer injection of platform views until needed. (flutter/engine#48960)
2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 1aef027ec953 to 63a452b45026 (2 revisions) (flutter/engine#49311)
2023-12-21 skia-flutter-autoroll@skia.org Roll Skia from 29917d8c97ca to 4b16117e94b2 (4 revisions) (flutter/engine#49310)
2023-12-21 737941+loic-sharma@users.noreply.github.com Reland "[Windows] Move to FlutterCompositor for rendering" (flutter/engine#49262)
2023-12-21 30870216+gaaclarke@users.noreply.github.com Reland `[Impeller] new blur: refactored math and fixed expanded padding size` (flutter/engine#49302)
2023-12-20 dkwingsmt@users.noreply.github.com Multiview pipeline Pt. 1: Skip illegal render calls (flutter/engine#49266)
2023-12-20 barpac02@gmail.com SemanticsUpdateBuilder: make all args non-null (flutter/engine#49148)
2023-12-20 30870216+gaaclarke@users.noreply.github.com [Impeller] fixed Rect::Contains (flutter/engine#49294)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 97c3b7e1885a to 1aef027ec953 (1 revision) (flutter/engine#49295)
2023-12-20 30870216+gaaclarke@users.noreply.github.com Revert "[Impeller] new blur: refactored math and fixed expanded padding size" (flutter/engine#49298)
2023-12-20 30870216+gaaclarke@users.noreply.github.com [Impeller] new blur: refactored math and fixed expanded padding size (flutter/engine#49206)
2023-12-20 dkwingsmt@users.noreply.github.com Multi-view pointer event (flutter/engine#46213)
2023-12-20 1961493+harryterkelsen@users.noreply.github.com [web:multiview] Only call `Renderer.clearFragmentProgramCache` on hot restart (flutter/engine#48758)
2023-12-20 skia-flutter-autoroll@skia.org Roll Skia from 9cb1bb1164ea to 29917d8c97ca (1 revision) (flutter/engine#49289)
2023-12-20 bdero@google.com [Impeller] Add interactive Blur+Clip AiksTest. (flutter/engine#49283)
2023-12-20 sergiy.dubovik@supercell.com [macos] FlutterKeyboardManager memory leak fix (flutter/engine#48824)
2023-12-20 zanderso@users.noreply.github.com Don't guard Windows arm64 Dart SDK download on the release candidate flag (flutter/engine#49244)
2023-12-20 15619084+vashworth@users.noreply.github.com Fix testAppExtensionLaunching for Xcode 15/iOS 17 (flutter/engine#49242)
2023-12-20 skia-flutter-autoroll@skia.org Roll Skia from 8060d6b36066 to 9cb1bb1164ea (2 revisions) (flutter/engine#49288)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from ed415d966d8a to 97c3b7e1885a (1 revision) (flutter/engine#49287)
2023-12-20 skia-flutter-autoroll@skia.org Roll Skia from d0f09ad481f7 to 8060d6b36066 (1 revision) (flutter/engine#49285)
2023-12-20 kevinjchisholm@gmail.com [release] Update release config (flutter/engine#49254)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 1732c4c92ccd to ed415d966d8a (1 revision) (flutter/engine#49274)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 4c59838945d9 to 1732c4c92ccd (1 revision) (flutter/engine#49269)
2023-12-20 goderbauer@google.com Sync lints with flutter/flutter (flutter/engine#49192)
2023-12-19 1961493+harryterkelsen@users.noreply.github.com [web] Enforce onDrawFrame/onBeginFrame render rule (flutter/engine#49214)
2023-12-19 barpac02@gmail.com [Docs] Add more info about running tests on iOS (flutter/engine#48859)

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
...
jason-simmons added a commit to jason-simmons/flutter_engine that referenced this pull request Jan 30, 2024
@jason-simmons
Update Web to parse the new ImpellerC JSON shader output format
a94d5ab 
See flutter#49253

Fixes flutter/flutter#141838
@jason-simmons jason-simmons mentioned this pull request Jan 30, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdero bdero bdero approved these changes

@chinmaygarde chinmaygarde Awaiting requested review from chinmaygarde

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller
Projects
No open projects
Impeller
Archived in project
Milestone
No milestone
2 participants
@dnfield @bdero