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

Conversation

bdero
Copy link
Member

@bdero bdero commented Jul 10, 2023

Apply color filters on the CPU for solid colors and gradients. This fixes flutter/flutter#127232 for the purely color-based color sources.

We still need to make the other special color blending paths respect the geometry mask in order to fully resolve the underlying issue:

  • SaveLayers (if they have an interesting matrix filter -- without a matrix filter, it'll always work today)
  • TiledTextureContents
  • TextContents
  • VerticesContents
  • AtlasContents

@bdero bdero self-assigned this Jul 10, 2023
@flutter-dashboard

This comment was marked as outdated.

@bdero
Copy link
Member Author

bdero commented Jul 10, 2023

Let's see those goldens.

@bdero bdero force-pushed the bdero/apply-cpu-color-filters branch from d9bb8f8 to e00247f Compare July 10, 2023 19:33
@flutter-dashboard
Copy link

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #43519 at sha e00247f

@bdero
Copy link
Member Author

bdero commented Jul 10, 2023

Those goldens are looking mighty fine.

@bdero bdero changed the title WIP: [Impeller] Apply color filters on the CPU for solid colors & gradients. [Impeller] Apply color filters on the CPU for solid colors & gradients. Jul 10, 2023
@bdero bdero force-pushed the bdero/apply-cpu-color-filters branch from b11a61d to ec19ef0 Compare July 10, 2023 21:33
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

How does this work with decal gradients + color blending that changes transparent black?

@jonahwilliams
Copy link
Contributor

I'm not really sure what the correct result should be in this case, but I think If you apply a color filter to a decal texture you can observe the transparent black pixels getting changed, where a CPU blend afterwards might not do that?

@bdero
Copy link
Member Author

bdero commented Jul 10, 2023

I'm not really sure what the correct result should be in this case, but I think If you apply a color filter to a decal texture you can observe the transparent black pixels getting changed, where a CPU blend afterwards might not do that?

Not sure if I'm fully answering your question, but:

Today, when applying a color blend to the color source, pixels that lie outside the geometry can be affected, which is incorrect behavior. This patch fixes that behavior for solid colors and gradients.

Fixing it for the other drawing ops will require more expensive tricks. For example, tracking the geometry mask as a separate texture and compositing post-blend (this is pretty much our only option if a mask blur is present), or drawing the geometry outline to a stencil and then using it inside the GPU blend filter.

@jonahwilliams
Copy link
Contributor

Here is an example of what I'm talking about: https://dartpad.dev/?id=80d0af639ec8952a3e31bac084fb202b

With decal and certain gradient parameters you can inset the color source in the geometry, and in that case you'd need to handle blending being done with transparent black pixels

@bdero
Copy link
Member Author

bdero commented Jul 11, 2023

Ah yeah, that needs to be fixed. I'll fix it by throwing the blended border color in the shader uniforms. It makes sense to allow the border color to be set in our emulated decal anyhow. Metal allows this, for example.

@bdero
Copy link
Member Author

bdero commented Jul 11, 2023

Great catch, btw. :)

@bdero bdero force-pushed the bdero/apply-cpu-color-filters branch from 439bd30 to f794f94 Compare July 11, 2023 01:25
@bdero bdero force-pushed the bdero/apply-cpu-color-filters branch from f794f94 to 482fb4c Compare July 11, 2023 01:32
@bdero
Copy link
Member Author

bdero commented Jul 11, 2023

Alrighty, added CanRenderLinearGradientDecalWithColorFilter to demonstrate that this is handled now.

Before:

Screenshot 2023-07-10 at 6 38 35 PM

After:

Screenshot 2023-07-10 at 6 35 14 PM

@bdero bdero requested a review from jonahwilliams July 11, 2023 01:39
@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #43519 at sha 31b7711

@bdero
Copy link
Member Author

bdero commented Jul 11, 2023

Goldens are good.

Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

At some point we'll need to reckon with how complex we've made gradients...

@bdero bdero merged commit 81d5adb into flutter:main Jul 11, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 11, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 11, 2023
…130357)

flutter/engine@59f2346...e2df016

2023-07-11 [email protected] Roll Skia from 68bcc4470230 to 47a37395ee40 (12 revisions) (flutter/engine#43568)
2023-07-11 [email protected] Remove stray semicolons from embedded_views.cc (flutter/engine#43566)
2023-07-11 [email protected] [Impeller] Apply color filters on the CPU for solid colors & gradients. (flutter/engine#43519)

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] 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
kjlubick pushed a commit to kjlubick/engine that referenced this pull request Jul 14, 2023
…s. (flutter#43519)

Apply color filters on the CPU for solid colors and gradients. This
fixes flutter/flutter#127232 for the purely
color-based color sources.

We still need to make the other special color blending paths respect the
geometry mask in order to fully resolve the underlying issue:
* SaveLayers (if they have an interesting matrix filter -- without a
matrix filter, it'll always work today)
* TiledTextureContents
* TextContents
* VerticesContents
* AtlasContents
bdero added a commit that referenced this pull request Aug 25, 2023
Resolves flutter/flutter#132936.

* In some cases, mask blurs were getting applied twice.
* Coverage hint clipping was too aggressive on the second pass of the
gaussian blur.
* Rework the mask blur applicator to support gracefully falling back to
GPU-based color filters when necessary.

This fix requires us to cherry-pick:
#43519
Going a revert-based route to fix all of these problems would also be
pretty complex.

https://github.com/flutter/engine/assets/919017/9ee5e856-af7a-42b9-b135-ea268c2ba53f
bdero added a commit to bdero/flutter-engine that referenced this pull request Aug 25, 2023
…s. (flutter#43519)

Apply color filters on the CPU for solid colors and gradients. This
fixes flutter/flutter#127232 for the purely
color-based color sources.

We still need to make the other special color blending paths respect the
geometry mask in order to fully resolve the underlying issue:
* SaveLayers (if they have an interesting matrix filter -- without a
matrix filter, it'll always work today)
* TiledTextureContents
* TextContents
* VerticesContents
* AtlasContents

(cherry picked from commit 81d5adb)
bdero added a commit to bdero/flutter-engine that referenced this pull request Aug 25, 2023
…er#45079)

Resolves flutter/flutter#132936.

* In some cases, mask blurs were getting applied twice.
* Coverage hint clipping was too aggressive on the second pass of the
gaussian blur.
* Rework the mask blur applicator to support gracefully falling back to
GPU-based color filters when necessary.

This fix requires us to cherry-pick:
flutter#43519
Going a revert-based route to fix all of these problems would also be
pretty complex.

https://github.com/flutter/engine/assets/919017/9ee5e856-af7a-42b9-b135-ea268c2ba53f
(cherry picked from commit 53595c9)
bdero added a commit to bdero/flutter-engine that referenced this pull request Aug 28, 2023
…er#45079)

Resolves flutter/flutter#132936.

* In some cases, mask blurs were getting applied twice.
* Coverage hint clipping was too aggressive on the second pass of the
gaussian blur.
* Rework the mask blur applicator to support gracefully falling back to
GPU-based color filters when necessary.

This fix requires us to cherry-pick:
flutter#43519
Going a revert-based route to fix all of these problems would also be
pretty complex.

https://github.com/flutter/engine/assets/919017/9ee5e856-af7a-42b9-b135-ea268c2ba53f
(cherry picked from commit 53595c9)
auto-submit bot pushed a commit that referenced this pull request Aug 29, 2023
gaaclarke pushed a commit to gaaclarke/engine that referenced this pull request Aug 30, 2023
…er#45079)

Resolves flutter/flutter#132936.

* In some cases, mask blurs were getting applied twice.
* Coverage hint clipping was too aggressive on the second pass of the
gaussian blur.
* Rework the mask blur applicator to support gracefully falling back to
GPU-based color filters when necessary.

This fix requires us to cherry-pick:
flutter#43519
Going a revert-based route to fix all of these problems would also be
pretty complex.

https://github.com/flutter/engine/assets/919017/9ee5e856-af7a-42b9-b135-ea268c2ba53f
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

[Impeller] Inset box shadow renders incorrectly.

2 participants