[Impeller] Replace Impeller opacity peephole delegate with DL variant. #52707
Conversation
impeller/aiks/canvas.cc
Outdated
| transform_stack_.back().distributed_opacity *= paint.color.alpha; | ||
| return; | ||
| } else { | ||
| FML_LOG(ERROR) << "no dist opacity"; |
There was a problem hiding this comment.
@flar try as I might, I can't seem to get this to fire on any of the opacity peephole tests in the macrobenchmarks app. It seems like can_distribute_opacity is always false with impeller?
This is just running with the regular canvas.
There was a problem hiding this comment.
Can't get which to fire? This is the "false" part of the if. Does this mean it is always true?
There was a problem hiding this comment.
I mean its never true
There was a problem hiding this comment.
It's working for the dl_sk_dispatcher, though...?
There was a problem hiding this comment.
(or we would have had benchmark regressions).
There was a problem hiding this comment.
but its not working here :)
Is there something different in the impeller DL setup that we're missing for dl opacity peephole? or some other requirement I'm missing?
There was a problem hiding this comment.
I was finally able to get this running locally and it worked for me with Grid with alpha SaveLayer on Rows
There was a problem hiding this comment.
If you tell me which sub-test types you ran I can look and see why they don't do peephole optimizations. Keep in mind that I created buttons for scenarios that wouldn't work so that we had something to aim for in the future, so not every one of those options will result in opacity peephole success...
|
Can confirm this is working, sorry for the noise! |
jonahwilliams
changed the title
| TRACE_EVENT0("flutter", "Canvas::saveLayer"); | ||
| if (can_distribute_opacity) { | ||
| FML_DCHECK(!backdrop_filter); |
There was a problem hiding this comment.
@flar I'm surprised but I hit this check in some tests. Do I need to assume that even if can_distribute_opacity is true, there may be other paint states (blend mode/ filters) that can still require a save layer?
There was a problem hiding this comment.
Which tests? I can take a look and see if there might be something wrong with the DL checks. In particular, I think I only pass SrcOver for now even though other blend modes might cause problems. I disallow ColorFilters, but I allow ImageFilters because according to my investigations the opacity should be applied to the output of the ImageFilter so it shouldn't matter. (It's applied to the input of the ColorFilter, though, so I just disallow them even though I could maybe look at their details and find that some of them are compatible).
There was a problem hiding this comment.
Ah, are you specifically referring to backdrop filter vs opacity flag? I notice that I don't turn off the flag in that case, but perhaps I should. The flag is more about the contents and whether the contents are compatible.
I also don't think I check the attributes that apply the SL to the parent. If they aren't SrcOver then I don't think we can do the opacity stuff.
I also don't believe the "old code" (pre-reorg) does this either. Let me get the reorg back in and then I can revisit layer properties and how they affect the flag.
| Entity::TileMode::kClamp); | ||
|
|
||
| // Paint has image filter, can't elide. | ||
| delegate = std::make_shared<OpacityPeepholePassDelegate>(paint); |
There was a problem hiding this comment.
This might be the case where I differ in assumptions. DL allows IF+opacity, but it looks like AIKS expects it to be disallowed...?
There was a problem hiding this comment.
We always fold the opacity into the image filter compositing, so in effect its as if the peephole wasn't applied.
| paint.color = Color::Red(); | ||
|
|
||
| // Paint has no alpha, can't elide; | ||
| delegate = std::make_shared<OpacityPeepholePassDelegate>(paint); |
There was a problem hiding this comment.
Huh? Why can't opaque colors be peepholed?
|
|
||
| // OpacityPeepholePassDelegate will only get used if the pass's blend mode is | ||
| // SourceOver, so no need to check here. | ||
| if (paint_.color.alpha <= 0.0 || paint_.color.alpha >= 1.0 || |
There was a problem hiding this comment.
If the alpha is 0 then we should skip the pass entirely, so I'm not sure why it is saying that it "can't collapse into parent" here (i.e. return false).
Also, if the paint is opaque, then that isn't a reason not to collapse the pass...? It just means you have no partial opacity to distribute, but you can still collapse it and distribute the opacity of 1.0 (i.e. NOP on modifying the opacity).
USe the existing DL opacity peephole flags. Also wires up for experimental canvas.