[Impeller] Supply surface cull rect for Vulkan & GLES dispatchers #43152
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
LGTM, what are your plans for testing?
jonahwilliams
left a comment
jonahwilliams
left a comment
There was a problem hiding this comment.
LGTM
I don't really think there are reasonable ways to test this sort of thing, besides rendering correctness and performance.
Neither of those things would show up if this was broken here. I plan on writing a test that uses it, we can consider that coverage at least. |
|
You could stand up one of the platform view performance integration tests on Impeller/Android. I think its Skia only right now. |
|
Yes that's why we have a devicelab |
|
|
||
| impeller::DlDispatcher impeller_dispatcher; | ||
| display_list->Dispatch(impeller_dispatcher); | ||
| display_list->Dispatch( |
There was a problem hiding this comment.
Note: need to do the SkConversions like in the metal surface.
|
Yeah, I don't see a good way to test this without instrumenting the code being tested. Does the Heisenberg Uncertainty Principle apply here? |
|
You can't test that the culling is happening, but you can test if the culling causes an error maybe. Draw a scene that has a mix of in-bounds, out-of-bounds, and on-the-bounds rendering ops and then make it a golden test. It would have no idea if the out-of-bounds ops got culled from that, but it would test for unintended consequences. Could we somehow create a franken-render-target that was large but reported a small bounds and then see if the primitives were rendered outside the bounds? |
|
We have benchmarks that are not currently configured to run Impeller, but could. These would show a performance improvement on this change. Benchmark improvements count as tests |
|
Looking into |
…id (#129455) For tracking perf improvements with changes like flutter/engine#43152.
…129495) flutter/engine@5b1b983...a9f446e 2023-06-24 [email protected] [Impeller] Supply surface cull rect for Vulkan & GLES dispatchers (flutter/engine#43152) 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://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
5 participants
Similar to Metal, have the DL cull entities early that lie entirely outside the surface texture.