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.

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

[Impeller] organize texture shaders / delete blend.frag + external_texture_fill #52137

Merged
merged 11 commits into from
Apr 19, 2024

Conversation

jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented Apr 15, 2024

  • Deletes blend.frag/vert . This was identical to the existing texture_fill shader.
  • Move alpha to frag info and stop passing as varying.
    * Deletes strict src rect fragment shader. We should be able to compute the UVs on the CPU, which is trivial since this is only used for rectangular draws.
  • Deletes external texture fill in favor of tiled texture external fill. The former shader is marginally faster, but we don't use it since we don't correctly check for the right extensions needed for non-emulated sampling modes with external textures on GLES. Easier to just always use emulated tile modes

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@jonahwilliams
Copy link
Member Author

I think we don't need the strict src rect fragment shader, but when I run the golden NinePatchImagePrecision before/after or with the feature disabled altogether I don't notice a difference.

@jason-simmons
Copy link
Member

This PR changes the output for the example app from flutter/flutter#140393.

With texture_fill_strict_src.frag the output matches Skia's behavior. With this patch it looks like the sampler may be blending color from adjacent pixels into the corners.

@jonahwilliams
Copy link
Member Author

Does this require a specific screen size or device? Visually this looks correct for me on a Pixel 7 Pro, and the playground test you added also looks correct for me on an m1 mac. But I can reproduce the pixel bleeding if I turn of the kStrict flag entirely.

@jason-simmons
Copy link
Member

The issue is that with this PR the colors of the corners are lighter than expected.

Original With PR #52137
original with_52137

The above was taken from a Pixel 6a, but this should reproduce on any device.

The bleeding outside the corners does not happen with this PR. But the corners themselves look like they are not sampling exclusively from the color of the corners in the nine-patch image.

@jonahwilliams
Copy link
Member Author

Okay I see it now! I think I'll leave this one as is. Since we only use it in the drawRect case its not going to cause problems with the tiled texture refacor.

@jonahwilliams
Copy link
Member Author

@chinmaygarde speaking of reducing shader count

@jonahwilliams jonahwilliams changed the title [Impeller] organize texture shaders. [Impeller] organize texture shaders / delete blend.frag + external_texture_fill Apr 19, 2024
@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 19, 2024
@auto-submit auto-submit bot merged commit 55670b7 into flutter:main Apr 19, 2024
31 checks passed
@jonahwilliams jonahwilliams deleted the organize_texture_shaders branch April 19, 2024 20:51
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 19, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Apr 19, 2024
…147091)

flutter/engine@a9a6f59...55670b7

2024-04-19 [email protected] [Impeller] organize texture shaders / delete blend.frag + external_texture_fill (flutter/engine#52137)

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://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
@jonahwilliams jonahwilliams added the revert Label used to revert changes in a closed and merged pull request. label Apr 22, 2024
Copy link
Contributor

auto-submit bot commented Apr 22, 2024

Time to revert pull request flutter/engine/52137 has elapsed.
You need to open the revert manually and process as a regular pull request.

@auto-submit auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Apr 22, 2024
@jonahwilliams
Copy link
Member Author

reason for revert: investigating breakages in #52299

@jonahwilliams jonahwilliams added the revert Label used to revert changes in a closed and merged pull request. label Apr 22, 2024
Copy link
Contributor

auto-submit bot commented Apr 22, 2024

Time to revert pull request flutter/engine/52137 has elapsed.
You need to open the revert manually and process as a regular pull request.

@auto-submit auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Apr 22, 2024
gilnobrega pushed a commit to gilnobrega/flutter that referenced this pull request Apr 22, 2024
…lutter#147091)

flutter/engine@a9a6f59...55670b7

2024-04-19 [email protected] [Impeller] organize texture shaders / delete blend.frag + external_texture_fill (flutter/engine#52137)

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://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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants