[Impeller] eliminate sub-render pass for blended color + texture vertices. #51778
Conversation
jonahwilliams
changed the title
impeller/aiks/canvas.cc
Outdated
| @@ -885,6 +889,28 @@ void Canvas::DrawVertices(const std::shared_ptr<VerticesGeometry>& vertices, | |||
| return; | |||
| } | |||
|
|
|||
| // If there is are per-vertex colors, an image, and the blend mode | |||
| // is simple we can draw without a sub-renderpass. | |||
| if (blend_mode <= BlendMode::kModulate && vertices->HasVertexColors() && | |||
There was a problem hiding this comment.
We can do this for the rest of the blend modes once I make some updates to the advanced blends. After that, the only time we'd use the old path is for a gradient/shader color source. There is probably an easier refactor there to eagerly convert those to a texture and then let them go through this code path instead.
|
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. |
impeller/aiks/canvas.cc
Outdated
| contents->SetAlpha(paint.color.alpha); | ||
| contents->SetGeometry(vertices); | ||
|
|
||
| auto color_contents = std::reinterpret_pointer_cast<TiledTextureContents>( |
There was a problem hiding this comment.
This is very unsafe. This should be a static_cast (https://google.github.io/styleguide/cppguide.html#Casting).
If we did have that visitor we could have a debug assert or use it directly at runtime. I'd like us to have some sort of debug assert at least to know this is safe. A debug only Contents::IsTiledTextureContents would work. I think a visitor is the more complete solution though.
There was a problem hiding this comment.
Switched to static cast.
The cast is safe as long as it is guared by the paint.color_source.GetType() == ColorSource::Type::kImage above. I would actually like to combine the color sources into a std::variant so we can lose the heap allocation, that would let us have a visitor
There was a problem hiding this comment.
Can we add a bool Contents::IsTiledTextureContents with a TODO for the std::variant?
My concern is that while paint.color_source.GetType() == ColorSource::Type::kImage may be a proper guard now, someone reading the code cannot know that it is a proper guard and we have no backstop for future changes that may invalidate that guarantee.
A bad cast in C++ is undefined behavior, it won't fail gracefully like in other languages.
There was a problem hiding this comment.
If we guarding against arbitrary future code changes, then i could also change IsTiledTextureContents() to return true for a different type.
There was a problem hiding this comment.
If we guarding against arbitrary future code changes
I wouldn't say "guarding". What we are building with our tests is the confidence to change the code and know that it is still correct after a change. A downcast that is determined to be safe from a non-local derivative isn't helping us.
We agree the visitor or std::variant would be better solutions. That's why I think we should have the TODO. If you want to make that change instead that'd be fine with me. I assumed that was out of scope, IsTiledTextureContents seems the easiest solution. It's ugly, but downcasting is ugly and we have a way to make it better if we want. I don't think there is another alternative to assert this is a safe downcast, we could enable RTTI for debug builds of the test runner, but I think it's best if we keep the test runners executing like how they will in production.
There was a problem hiding this comment.
Its not that its ugly @gaaclarke , its just that I don't believe it provides any additional verification.
There was a problem hiding this comment.
I'll do this the right way after #51853 , but I still need to figure out why the golden looks wrong on metal.
DrawAtlas already has the optimization in #51778 and so it is still rendering incorrectly with wide gamut.
|
Golden file changes are available for triage from new commit, Click here to view. |
|
Golden currently looks wrong for metal, investigating. |
This would also me to create a type safe visitor to pull out the data required for #51778
|
Done, golden problem was mismatched colors/vertices (the assers are in dart:ui) |
|
Golden file changes are available for triage from new commit, Click here to view. |
impeller/aiks/canvas.cc
Outdated
| // is simple we can draw without a sub-renderpass. | ||
| if (blend_mode <= BlendMode::kModulate && vertices->HasVertexColors()) { | ||
| if (auto maybe_image_data = GetImageColorSourceData(paint.color_source)) { | ||
| auto image_data = maybe_image_data.value(); |
There was a problem hiding this comment.
Did you intend to copy the image data here?
jonahwilliams
added
the
autosubmit
…146240) flutter/engine@349608d...18fdcad 2024-04-03 [email protected] Roll Skia from 8d7482b998d0 to 7d6dce620e46 (1 revision) (flutter/engine#51878) 2024-04-03 [email protected] Be slightly more lenient about the assertion, as it differs on different backends. (flutter/engine#51877) 2024-04-03 [email protected] Roll Skia from afa233bb1979 to 8d7482b998d0 (2 revisions) (flutter/engine#51876) 2024-04-03 [email protected] [Impeller] Enable logging a warning when the user opts out of using Impeller. (flutter/engine#51849) 2024-04-03 [email protected] [Impeller] delete unused code. (flutter/engine#51871) 2024-04-03 [email protected] [Impeller] eliminate sub-render pass for blended color + texture vertices. (flutter/engine#51778) 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
The graph goes down, not up. I'd call it more of a dump than a bump. |
Fixes flutter/flutter#145707
Part of flutter/flutter#131345
If we're drawing vertices with per-color, a non-advanced blend, and a texture (with or without coordinates), use the porter duff shader to perform the blend without creating a sub render pass. IN addition to being more performant, this eliminates any potential rendering bugs caused by overlapping vertices.
This is not yet fixed for advanced blends.