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] Patch the compiler to account for subpass inputs and PSO metadata. #45739

Merged
merged 1 commit into from
Sep 12, 2023
Merged

[Impeller] Patch the compiler to account for subpass inputs and PSO metadata. #45739

merged 1 commit into from
Sep 12, 2023

Conversation

chinmaygarde
Copy link
Member

@chinmaygarde chinmaygarde self-assigned this Sep 12, 2023
bdero
bdero approved these changes Sep 12, 2023
View reviewed changes
Copy link
Member

@bdero bdero left a comment

Choose a reason for hiding this comment

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

LGTM

Would a reflection test for this in compiler_unittests.cc make sense?

impeller/aiks/aiks_unittests.cc
@@ -1636,7 +1636,7 @@ TEST_P(AiksTest, ColorWheel) {
auto callback = [&](AiksContext& renderer) -> std::optional<Picture> {
// UI state.
static bool cache_the_wheel = true;
static int current_blend_index = 3;
static int current_blend_index = 14;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Keep default to SourceOver? Although advanced blends are more contentious, so maybe someone flipping through is more likely to notice a problem with Screen as the default.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, just thought defaulting to the advanced ones was more likely to throw up issues.

impeller/compiler/reflector.cc
@@ -261,6 +262,19 @@ std::optional<nlohmann::json> Reflector::GenerateTemplateArguments() const {
}
}

{
if (auto inputs = ReflectResources(shader_resources.subpass_inputs);
Copy link
Member

Choose a reason for hiding this comment

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

Oh, convenient!

@chinmaygarde
Copy link
Member Author

Would a reflection test for this in compiler_unittests.cc make sense?

My hope is that the pipeline creation itself would be the ultimate fix. Another patch incoming that wires up the advanced blend libraries.

@chinmaygarde chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 12, 2023
@auto-submit auto-submit bot merged commit 6f223db into flutter:main Sep 12, 2023
28 checks passed
jonahwilliams
jonahwilliams reviewed Sep 13, 2023
View reviewed changes
impeller/compiler/compiler.cc
return CreateMSLCompiler(
ir, source_options,
spirv_cross::CompilerMSL::Options::make_msl_version(3, 0, 0));
auto vk_compiler = std::make_shared<spirv_cross::CompilerGLSL>(ir);
Copy link
Member

Choose a reason for hiding this comment

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

Do we know how this change had previously broken GLES and how this changed fixed it?

Copy link
Member Author

Choose a reason for hiding this comment

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

The original issue was light on details but there is now a separate CreateGLSLCompiler which is probably what's used. So this change shouldn't affect GLSL at all. I believe earlier there used to be a common compiler instance creation and I can believe was hard to configure just right for one backend without causing unintended consequences elsewhere. But again, the issue was light on details so its hard to say. I'll keep an eye out for repercussions.

@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 13, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 13, 2023
@engine-flutter-autoroll
6f223db0f [Impeller] Patch the compiler to account for subpass inputs…
d476da4 
… and PSO metadata. (flutter/engine#45739)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Sep 13, 2023
@engine-flutter-autoroll
Roll Flutter Engine from c90fadf45d44 to 7c78ea210c19 (10 revisions) (#…
68ce499 
…134597)

flutter/engine@c90fadf...7c78ea2

2023-09-12 zanderso@users.noreply.github.com Revert "Lazily allocate RasterCacheItems only when caching is enabled" (flutter/engine#45734)
2023-09-12 1961493+harryterkelsen@users.noreply.github.com Revert "Use a single OffscreenCanvas for rendering in CanvasKit" (flutter/engine#45744)
2023-09-12 chinmaygarde@google.com [Impeller] Patch the compiler to account for subpass inputs and PSO metadata. (flutter/engine#45739)
2023-09-12 chinmaygarde@google.com [Impeller] Fix swapchain recreation for non-polling cases. (flutter/engine#45740)
2023-09-12 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from QgAHx3BtJfN3TmodS... to UCa49z8fu0hD9cypj... (flutter/engine#45738)
2023-09-12 ychris@google.com [ios] upload extension safe artifacts (flutter/engine#45664)
2023-09-12 30870216+gaaclarke@users.noreply.github.com Added test to assert the vulkan embedder threadsafe vkqueue usage (flutter/engine#45732)
2023-09-12 chinmaygarde@google.com [Impeller] If validations are enabled but not found, still create the VK context. (flutter/engine#45674)
2023-09-12 skia-flutter-autoroll@skia.org Roll Skia from 211d63b1e1f5 to 2d295711337c (7 revisions) (flutter/engine#45729)
2023-09-12 1961493+harryterkelsen@users.noreply.github.com Use a single OffscreenCanvas for rendering in CanvasKit (flutter/engine#42672)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from QgAHx3BtJfN3 to UCa49z8fu0hD

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 bdero@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://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
@gaaclarke gaaclarke mentioned this pull request Sep 13, 2023
Closed
gaaclarke added a commit that referenced this pull request Sep 13, 2023
@gaaclarke
Revert "[Impeller] Patch the compiler to account for subpass inputs a…
41381d2 
…nd PSO metadata. (#45739)"

This reverts commit 6f223db.
@gaaclarke gaaclarke mentioned this pull request Sep 13, 2023
Merged
auto-submit bot pushed a commit that referenced this pull request Sep 13, 2023
@gaaclarke
Revert "[Impeller] Patch the compiler to account for subpass inputs a…
2049b69 
…nd PSO metadata." (#45777)

Reverts #45739
fixes flutter/flutter#134666
Mairramer pushed a commit to Mairramer/flutter that referenced this pull request Oct 10, 2023
@engine-flutter-autoroll @Mairramer
Roll Flutter Engine from c90fadf45d44 to 7c78ea210c19 (10 revisions) (f…
e608579 
…lutter#134597)

flutter/engine@c90fadf...7c78ea2

2023-09-12 zanderso@users.noreply.github.com Revert "Lazily allocate RasterCacheItems only when caching is enabled" (flutter/engine#45734)
2023-09-12 1961493+harryterkelsen@users.noreply.github.com Revert "Use a single OffscreenCanvas for rendering in CanvasKit" (flutter/engine#45744)
2023-09-12 chinmaygarde@google.com [Impeller] Patch the compiler to account for subpass inputs and PSO metadata. (flutter/engine#45739)
2023-09-12 chinmaygarde@google.com [Impeller] Fix swapchain recreation for non-polling cases. (flutter/engine#45740)
2023-09-12 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from QgAHx3BtJfN3TmodS... to UCa49z8fu0hD9cypj... (flutter/engine#45738)
2023-09-12 ychris@google.com [ios] upload extension safe artifacts (flutter/engine#45664)
2023-09-12 30870216+gaaclarke@users.noreply.github.com Added test to assert the vulkan embedder threadsafe vkqueue usage (flutter/engine#45732)
2023-09-12 chinmaygarde@google.com [Impeller] If validations are enabled but not found, still create the VK context. (flutter/engine#45674)
2023-09-12 skia-flutter-autoroll@skia.org Roll Skia from 211d63b1e1f5 to 2d295711337c (7 revisions) (flutter/engine#45729)
2023-09-12 1961493+harryterkelsen@users.noreply.github.com Use a single OffscreenCanvas for rendering in CanvasKit (flutter/engine#42672)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from QgAHx3BtJfN3 to UCa49z8fu0hD

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 bdero@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://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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonahwilliams jonahwilliams jonahwilliams left review comments

@bdero bdero bdero approved these changes

Assignees

@chinmaygarde chinmaygarde

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
3 participants
@chinmaygarde @bdero @jonahwilliams