[Impeller] add async command submission for blit pass. #48040
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 or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use 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. |
|
Test added. |
|
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. |
There was a problem hiding this comment.
Could you edit the PR description to include a bit more details:
- What issue does this close and/or what issue does this work towards?
- What exactly is "async command submission"? Could we link to some background?
(The second one is more for git blame, the first one is more for this review)
impeller/aiks/aiks_unittests.cc
Outdated
| @@ -174,18 +175,23 @@ bool GenerateMipmap(const std::shared_ptr<Context>& context, | |||
| return false; | |||
| } | |||
| pass->GenerateMipmap(std::move(texture), std::move(label)); | |||
| if (async_submit) { | |||
| return buffer->SubmitCommandsAsync(pass, context->GetResourceAllocator()); | |||
There was a problem hiding this comment.
Just based off of reading this, maybe it should be EncodeAndSubmitCommandsAsync.
There was a problem hiding this comment.
Yeah that is a good suggestion, i will do that (not today, but when I update this tomrrow) 😄
impeller/entity/entity_pass.cc
Outdated
There was a problem hiding this comment.
Why is this change unconditional? Do we always want to do this? I guess I'm confused here - is synchronous submission being removed after this lands?
There was a problem hiding this comment.
^ What about this question? LGTM otherwise.
There was a problem hiding this comment.
Ahh yes, we always want to do this. no, its not being removed because it is still useful for workloads that happen outside of the frame workload (like image upload/mipmaps).
There is also no way to create multiple render passes from a single cmd buffer and async submit them, which we don't need to do right now, but only the old API supports that technically.
|
Sorry about that, I've updated the description with more details |
jonahwilliams
added
the
autosubmit
…138529) flutter/engine@3cfcdeb...8aff9c1 2023-11-16 skia-flutter-autoroll@skia.org Roll Dart SDK from cc6acfd7d57c to 5cccc24d127f (1 revision) (flutter/engine#48109) 2023-11-16 skia-flutter-autoroll@skia.org Roll Skia from 5d6bdbf69dea to add865f891c8 (1 revision) (flutter/engine#48108) 2023-11-16 skia-flutter-autoroll@skia.org Roll Dart SDK from 65819963fb17 to cc6acfd7d57c (5 revisions) (flutter/engine#48100) 2023-11-16 matanlurey@users.noreply.github.com Make `fml/status_or.h` compatible with `.clang_tidy`. (flutter/engine#48002) 2023-11-16 dnfield@google.com [Impeller] Gate Vulkan selection on API 29 (flutter/engine#48089) 2023-11-16 chris@bracken.jp [macOS] Clean up allocations in menu plugin test (flutter/engine#48093) 2023-11-16 matanlurey@users.noreply.github.com Re-land "Make `fml/...` compatible with `.clang_tidy` (flutter/engine#48030) 2023-11-15 ychris@google.com [ios] introduce weak_nsobject (flutter/engine#47947) 2023-11-15 skia-flutter-autoroll@skia.org Roll Skia from e954d1a1972c to 5d6bdbf69dea (2 revisions) (flutter/engine#48094) 2023-11-15 jonahwilliams@google.com [Impeller] add async command submission for blit pass. (flutter/engine#48040) 2023-11-15 matanlurey@users.noreply.github.com Make `lib/ui/compositing/...` compatible with `.clang_tidy`. (flutter/engine#48001) 2023-11-15 godofredoc@google.com Remove the linux fuchsia v1 build. (flutter/engine#48085) 2023-11-15 mdebbar@google.com [web] Apply global styles before inserting the DOM element (flutter/engine#48027) 2023-11-15 skia-flutter-autoroll@skia.org Roll Skia from b23074a79bda to e954d1a1972c (7 revisions) (flutter/engine#48092) 2023-11-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Make `lib/ui/{text|window}/...` compatible with `.clang_tidy`." (flutter/engine#48083) 2023-11-15 41930132+hellohuanlin@users.noreply.github.com [ios]fix ios 16 auto correction highlight showing on top left corner (flutter/engine#47279) 2023-11-15 skia-flutter-autoroll@skia.org Roll Skia from c42226314a4f to b23074a79bda (3 revisions) (flutter/engine#48081) 2023-11-15 matanlurey@users.noreply.github.com Make `lib/ui/{text|window}/...` compatible with `.clang_tidy`. (flutter/engine#48000) 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 jonahwilliams@google.com,rmistry@google.com,zra@google.com 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
This is a requirement for the "Remove Drawable Acquisition Latency" Change, but is otherwise a harmless improvement.
Submitting a command buffer causes the backend specific encoding logic to run. Metal is unique in that it is fairly easy to move this work into a background thread, allowing the engine to move onto creating the next command buffer. This improves throughput of the engine, at the cost of needing two slightly different APIs. Currently the GLES and Vulkan versions of this method still submit synchronously - for now that is out of scope as doing background work with those APIs has proved more challenging.
See also:
Separately, as a requirement for the design in "Remove Drawable Acquisition Latency", we need to be able to defer drawable acquisition to this background thread. While this almost already works for render passes, it does not work for blit passes today. if the engine renders a backdrop filter, then the final command buffer submitted will be a blit pass that copies an offscreen onto the drawable. Therefore we need to add an async version of the blit submission, so that we have a hook to move the drawable acquisition onto a background thread for metal.
This hadn't been done until now because most blit cmd buffers have 1 or 2 cmds on them so the benefit of moving to a background thread is minimal.
Part of flutter/flutter#138490