[Impeller] let images with opacity and filters passthrough. #39237
Conversation
|
Need to double check but I think specifically for images we document the opacity as applying at the end so it may be OK |
|
This is visually correct because we apply absorbed opacity before the filters. Not sure if the check for the mask is appropriate though |
chinmaygarde
changed the title
| } | ||
|
|
||
| bool Paint::HasFilters() const { | ||
| return color_filter.has_value(); |
There was a problem hiding this comment.
Why color filters specifically? Not all color filters absorb opacity IIRC.
There was a problem hiding this comment.
Good point, I should check for this with #39266
The general answer is that the high level framework API makes it extrenely easy to apply color blend filters to images, and no other type of color or image filter. see color and colorBlendMode in https://api.flutter.dev/flutter/widgets/Image-class.html
There was a problem hiding this comment.
As of now, all color filters absorb opacity.
impeller/aiks/paint.h
Outdated
| /// @brief Whether this paint has a filter that can apply opacity | ||
| /// | ||
| /// Only color filters can absorb opacity currently. | ||
| bool HasFilters() const; |
There was a problem hiding this comment.
This just checks for a color filter, not any filter. So perhaps name HasColorFilter instead?
impeller/aiks/canvas.cc
Outdated
| contents->SetSourceRect(source); | ||
| contents->SetSamplerDescriptor(std::move(sampler)); | ||
| contents->SetOpacity(paint.color.alpha); | ||
| contents->SetDeferApplyingOpacity(paint.HasFilters()); |
There was a problem hiding this comment.
This feature was added in order to apply the opacity in the correct order in some cases. Unfortunately, opacity is starting to get kind of hard to reason about in Impeller.
Would this still work correctly when:
- An image filter is present along with a color filter?
- An advanced blend + a color filter is being used?
cc @ColdPaleLight , who had to contend with opacity deferral here: #36279
There was a problem hiding this comment.
1/2 essentially don't matter for performance (though they certainly do for correctness). The issue we need to contend with is that Flutter makes it very easy to add a color+blend to an image, and its expected that this is only marginally more expensive than drawing the image itself (probably due to skia genering shaders for this).
So one partial solution is what I have here, but perhaps this is pushing the implementation in the wrong direction. In all of the cases I'm worried about, we'll get some drawing primitive (drawRect + tiled texture), drawImageRect, drawImageNine, and then a paint with a color filter on it.
Perhaps the way to go about this is to recognize specifically that case (requiring that color sources and color filters are typed and not just opaque procs), and then create a specialized contents for it. That would also make it easier to fix a simialr problem with tiled textures where we tile then color filter instead of color filter then tile.
There was a problem hiding this comment.
-
An image filter is present along with a color filter?
If the ImageFilter and ColorFilter work together, the ColorFilter will be applied first and then the ImageFilter will be applied. And as of now, all ColorFilter will absorb opacity, so I think in this scenario, the performance of this PR will be correct. -
An advanced blend + a color filter is being used?
Since advanced blend is still applied after ColorFilter, there will be no problem
So I think this PR is correct.
…nto shader_experiment
jonahwilliams
added
the
autosubmit
…119765) * 4b77e7793 [Impeller] improve blur performance for Android and iPad Pro. (flutter/engine#39291) * 97d27ff59 [Impeller] let images with opacity and filters passthrough. (flutter/engine#39237)
4 participants
Fixes flutter/flutter#119474
I'm not actually sure if this is correct! But this improves wonderous frame times on the wonders screen by ~20 FPS on the iPad pro.