[Impeller] drawVertices uber shader. #52315
Conversation
| void main() { | ||
| f16vec4 dst = IPHalfUnpremultiply(v_color); | ||
| f16vec4 src = IPHalfUnpremultiply(texture(texture_sampler, v_texture_coords)); | ||
| f16vec3 blend_result = |
There was a problem hiding this comment.
If this is fast enough maybe we'll just do all the advanced blends this way 😏
There was a problem hiding this comment.
This causes stack spills (yay malioc!) on OpenGL ES on the Mali T880 (the ancient one). Notably, I don't see the spills on the Vulkan version. Given we are focusing on Vulkan, I'm definitely not opposed to landing this. But maybe have a comment in there about the spill with an issue filed so we have that observation on record for OpenGL ES.
The bound pipeline is the arith where the spill happens which tracks I suppose. Personally, I'd say the bar for shaders being "fast enough" would be to not cause these spills on the T880.
There was a problem hiding this comment.
Oh, I don't see the T880 in the Vulkan malioc results. Perhaps that is too old for Vulkan? Maybe we should find an older baseline than the Mali-G78.
There was a problem hiding this comment.
We shrunk down the malioc to two models, a pixel and I think the moto g4 gpu
There was a problem hiding this comment.
Hmm, if memory serves, the reasoning was that one would capture the high end on Vulkan and the other the low one with GLES. But that was when we were still focusing on GLES. Should we have two for Vulkan (and decide what the low one should be)?
In any case, don't want to distract you. This is good. There are no spills on our current Vulkan baseline.
There was a problem hiding this comment.
That's a good idea , we should revisit the gpu list
|
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. |
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
Besides the observation about this bumping up against the limits what older Android devices are capable of, lgtm because that should be limited to GLES.
| void main() { | ||
| f16vec4 dst = IPHalfUnpremultiply(v_color); | ||
| f16vec4 src = IPHalfUnpremultiply(texture(texture_sampler, v_texture_coords)); | ||
| f16vec3 blend_result = |
There was a problem hiding this comment.
This causes stack spills (yay malioc!) on OpenGL ES on the Mali T880 (the ancient one). Notably, I don't see the spills on the Vulkan version. Given we are focusing on Vulkan, I'm definitely not opposed to landing this. But maybe have a comment in there about the spill with an issue filed so we have that observation on record for OpenGL ES.
The bound pipeline is the arith where the spill happens which tracks I suppose. Personally, I'd say the bar for shaders being "fast enough" would be to not cause these spills on the T880.
| void main() { | ||
| f16vec4 dst = IPHalfUnpremultiply(v_color); | ||
| f16vec4 src = IPHalfUnpremultiply(texture(texture_sampler, v_texture_coords)); | ||
| f16vec3 blend_result = |
There was a problem hiding this comment.
Oh, I don't see the T880 in the Vulkan malioc results. Perhaps that is too old for Vulkan? Maybe we should find an older baseline than the Mali-G78.
jonahwilliams
added
the
autosubmit
…147258) flutter/engine@f23cc4d...b686508 2024-04-23 [email protected] Roll Fuchsia Linux SDK from L533ubzhjWwW7jxbc... to le_-uFgRD5DjvvqgL... (flutter/engine#52329) 2024-04-23 [email protected] [Impeller] only use framebuffer advanced blends if available. (flutter/engine#52284) 2024-04-23 [email protected] [Impeller] drawVertices uber shader. (flutter/engine#52315) 2024-04-23 [email protected] Roll Dart SDK from acd54f86bdbb to 6a670b60eb06 (1 revision) (flutter/engine#52327) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from L533ubzhjWwW to le_-uFgRD5Dj 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
2 participants
In order to land #52303 , we need to finally fix the advanced blend draw vertices combo. Right now a ColorFilter is used for advanced blends which doesn't work if there are overlapping vertices.
See also: flutter/flutter#145707
The issue was fixed for drawVertices/drawAtlas pipeline blends using the porterduff shader. This extends this to advanced blends, but since drawVertices/atlas with an advanced blend is uncommon and because we don't 15 new shader variants, just add one special uber shader.
Part of flutter/flutter#131345