Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented Jul 19, 2023

From experimentation, providing these values through varyings runs substantially faster on Pixel devices. Alternative solutions to use f16 or push constants did not improve things substantially.

These subset of shaders are used very heavily and so they benefit from this change more substantially. I don't think this is applicable to all shaders as there is going to be an additional varying interpolation cost.

See also: go/impeller-slow-fragment-shader

jonahwilliams added 3 commits July 19, 2023 17:22
@jonahwilliams jonahwilliams marked this pull request as ready for review July 20, 2023 18:40

uniform FragInfo {
float16_t x_tile_mode;
float16_t y_tile_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.

We don't used the tiled texture shader on vulkan, but we still need to move the alpha so that this can share the same vertex shader as texture.frag.

@jonahwilliams jonahwilliams changed the title [Impeller] try removing uniform computation from some shaders. [Impeller] Provide fragment uniform data through varyings for solid color, glyph atlas, texture shaders. Jul 20, 2023
#ifdef IMPELLER_TARGET_OPENGLES
#define IMPELLER_FLAT
#else
#define IMPELLER_FLAT flat
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 call thin IMPELLER_MAYBE_FLAT. Right now, we only use flat as an optimization to reduce load on the varying unit. But someone might actually start using it in the future to disregard non-provoking vertices and will be confused as to why it doesn't work on OpenGL ES.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 20, 2023
@auto-submit auto-submit bot merged commit ab7d424 into flutter:main Jul 20, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 20, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 20, 2023
…131015)

flutter/engine@9b2ebf2...ab7d424

2023-07-20 [email protected] [Impeller] Provide fragment uniform data through varyings for solid color, glyph atlas, texture shaders. (flutter/engine#43838)
2023-07-20 [email protected] Roll ANGLE from a4c283be741f to f2e0f8a0b236 (2 revisions) (flutter/engine#43864)
2023-07-20 [email protected] Roll Skia from 18e834916f47 to 100d0f858f02 (7 revisions) (flutter/engine#43863)
2023-07-20 [email protected] Roll Fuchsia Mac SDK from SmAtKPfGzPllC9gfO... to -SaPL-46jpiYbnCAu... (flutter/engine#43862)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from SmAtKPfGzPll to -SaPL-46jpiY

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 [email protected],[email protected],[email protected] 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
harryterkelsen pushed a commit to harryterkelsen/engine that referenced this pull request Jul 20, 2023
…olor, glyph atlas, texture shaders. (flutter#43838)

From experimentation, providing these values through varyings runs substantially faster on Pixel devices. Alternative solutions to use f16 or push constants did not improve things substantially.

These subset of shaders are used very heavily and so they benefit from this change more substantially. I don't think this is applicable to all shaders as there is going to be an additional varying interpolation cost.

See also: [go/impeller-slow-fragment-shader](http://goto.google.com/impeller-slow-fragment-shader)
gaaclarke added a commit to gaaclarke/engine that referenced this pull request Jul 25, 2023
… solid color, glyph atlas, texture shaders. (flutter#43838)"

This reverts commit ab7d424.
LouiseHsu pushed a commit to LouiseHsu/flutter that referenced this pull request Jul 31, 2023
…lutter#131015)

flutter/engine@9b2ebf2...ab7d424

2023-07-20 [email protected] [Impeller] Provide fragment uniform data through varyings for solid color, glyph atlas, texture shaders. (flutter/engine#43838)
2023-07-20 [email protected] Roll ANGLE from a4c283be741f to f2e0f8a0b236 (2 revisions) (flutter/engine#43864)
2023-07-20 [email protected] Roll Skia from 18e834916f47 to 100d0f858f02 (7 revisions) (flutter/engine#43863)
2023-07-20 [email protected] Roll Fuchsia Mac SDK from SmAtKPfGzPllC9gfO... to -SaPL-46jpiYbnCAu... (flutter/engine#43862)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from SmAtKPfGzPll to -SaPL-46jpiY

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 [email protected],[email protected],[email protected] 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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App e: impeller

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants