-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Impeller] Add interface for submitting multiple command buffers at once. #50139
[Impeller] Add interface for submitting multiple command buffers at once. #50139
Conversation
84d79a6
to
f6f72f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach looks good.
const CompletionCallback& callback = {}) override; | ||
|
||
private: | ||
std::weak_ptr<ContextVK> context_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever FML_DISALLOW_COPY_ASSIGN_AND_MOVE expands to. Not having to depend on the base class for deletes avoid potential expensive copies in case that interface changes but we put an expensive to copy ivar here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
impeller/renderer/context.h
Outdated
@@ -163,6 +162,9 @@ class Context { | |||
/// | |||
virtual std::shared_ptr<CommandBuffer> CreateCommandBuffer() const = 0; | |||
|
|||
/// @brief Return the graphics queue for submitting command buffers. | |||
virtual const std::shared_ptr<GraphicsQueue>& GetQueue() const = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetGraphicsQueue
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pending resolution about the comment later about the class name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conflicts with context_vk.h GetGraphicsQueue for the QueueVK class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
impeller/renderer/graphics_queue.h
Outdated
public: | ||
using CompletionCallback = std::function<void(CommandBuffer::Status)>; | ||
|
||
GraphicsQueue() = default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OOL these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
impeller/renderer/graphics_queue.h
Outdated
/// The order of the provided buffers determines the ordering in which | ||
/// they are submitted. | ||
/// | ||
/// Optionally accepts a ccallback that will fire with an updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"callback" misspelled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
impeller/renderer/graphics_queue.h
Outdated
|
||
/// @brief An interface for submitting command buffers to the GPU for | ||
/// encoding and execution. | ||
class GraphicsQueue { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the name, I was expecting this to be akin to the graphics vs compute vs transfer queues. But this doesn't seem to be the case. How about just calling this CommandQueue
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CommandQueue SGTM, solves naming problems too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed, other stuff WIP
impeller/renderer/graphics_queue.h
Outdated
/// they are submitted. | ||
/// | ||
/// Optionally accepts a ccallback that will fire with an updated | ||
/// status based on encoding state. Only the Metal and Vulkan backend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a better way of putting this would be "The status only indicates if the command buffer was successfully submitted. Successful completion of the command buffer can only be checked in the completion callback.".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, if the status is not OK, can the caller expect a completion callback invocation? Otherwise they might have to perform cleanup themselves if the callback is dropped on the floor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, a better interface might be to guarantee that the callback always gets invoked but with CommandBuffer::Status::kError in case of failure to submit as well. This function can return void. This avoids burden on the caller to check for error and invoke the callback themselves in case the callback has cleanup they want to perform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That change would make it much more difficult to detect immediate submission errors
if (!added_fence) { | ||
return fml::Status(fml::StatusCode::kCancelled, "Failed to add fence."); | ||
} | ||
fail_callback = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fml::ScopedCleanupClosure
has a Release
call that you can invoke instead of tracking the success case in a var.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
fml::Status GraphicsQueueVK::Submit( | ||
const std::vector<std::shared_ptr<CommandBuffer>>& buffers, | ||
const CompletionCallback& callback) { | ||
if (buffers.empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know what if the "you'll always get the callback no matter what" behavior is, asked for clarification in the headerdoc. But if it is the case, you might need a success callback invocation here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
} | ||
|
||
vk::SubmitInfo submit_info; | ||
submit_info.setCommandBuffers(vk_buffers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff.
[callback, tracked_objects = std::move(tracked_objects)]() mutable { | ||
// Ensure tracked objects are destructed before calling any final | ||
// callbacks. | ||
for (auto& tracked_object : tracked_objects) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tracked_objects.clear()
is the same as individually resetting the items in a loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -35,15 +37,18 @@ bool CommandBuffer::Submit() { | |||
for (auto& encodable : encodables_) { | |||
encodable->EncodeCommands(); | |||
} | |||
return command_buffer_->SubmitCommands(); | |||
|
|||
return context_->GetCommandQueue()->Submit({command_buffer_}).ok(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'll just need to do this in a raster task to keep it safe, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this has the exact same threading guarantees as CommandBuffer->Submit. Its the AiksContext batching that is not necessarily thread safe.
I'm not quite sure how to fix this compilation error:
|
this time its gonna work, I can feel it |
…142515) flutter/engine@438e9b4...0e586d1 2024-01-30 jonahwilliams@google.com [Impeller] Add interface for submitting multiple command buffers at once. (flutter/engine#50139) 2024-01-30 49699333+dependabot[bot]@users.noreply.github.com Bump actions/upload-artifact from 4.1.0 to 4.3.0 (flutter/engine#50165) 2024-01-30 skia-flutter-autoroll@skia.org Roll Skia from 083c1a4d9767 to 4e992fb3a9db (1 revision) (flutter/engine#50164) 2024-01-30 mbrase@google.com Use structured logging on Fuchsia (flutter/engine#49918) 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 matanl@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 causing the animated backdrop blur integration test to fail on iOS in the framework. Will investigate. |
…fers at once." (#50174) Reverts #50139 Initiated by: jonahwilliams This change reverts the following previous change: Original Description: The Impeller Vulkan backend benefits from batching submission to the vk graphics queue. Managing this automatically is non-trivial and adds surprising/fragile thread based behavior, see: #49870 Instead, introduce an impeller::CommandQueue object that command buffers must be submitted to in lieu of CommandBuffer->Submit, which has been made private. TLDR old ```c++ buffer->Submit(); ``` new ```c++ context.GetQueue()->Submit({buffer}); ``` The Metal and GLES implementations internally just call the private CommandBuffer->Submit, though there may be future opportunities to simplify here. The Vulkan implementation is where the meat is. Aiks takes advantage of this by storing all command buffers on the aiks context while rendering a frame, and then performing one submit in aiks_context render. I don't think this will introduce any thread safety problems, as we don't guarantee much about aiks context - nor do we use it in a multithreaded context as far as I know. Other tasks such as image upload still just directly submit their command buffers via the queue. Fixes flutter/flutter#141123
…142543) flutter/engine@438e9b4...0e4342c 2024-01-30 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] Add interface for submitting multiple command buffers at once." (flutter/engine#50174) 2024-01-30 skia-flutter-autoroll@skia.org Roll Skia from a3d46fac53be to 7df4d35e11cf (3 revisions) (flutter/engine#50173) 2024-01-30 skia-flutter-autoroll@skia.org Roll Skia from c2fada52fdc4 to a3d46fac53be (2 revisions) (flutter/engine#50171) 2024-01-30 skia-flutter-autoroll@skia.org Roll Skia from 7dc9ba2e8c90 to c2fada52fdc4 (1 revision) (flutter/engine#50170) 2024-01-30 skia-flutter-autoroll@skia.org Roll Skia from 1c0eae94fc09 to 7dc9ba2e8c90 (1 revision) (flutter/engine#50169) 2024-01-30 skia-flutter-autoroll@skia.org Roll Skia from 4e992fb3a9db to 1c0eae94fc09 (3 revisions) (flutter/engine#50167) 2024-01-30 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from Hqi_x_A9lYsY58VSn... to Z-xFM2ILZJw22eU8q... (flutter/engine#50166) 2024-01-30 jonahwilliams@google.com [Impeller] Add interface for submitting multiple command buffers at once. (flutter/engine#50139) 2024-01-30 49699333+dependabot[bot]@users.noreply.github.com Bump actions/upload-artifact from 4.1.0 to 4.3.0 (flutter/engine#50165) 2024-01-30 skia-flutter-autoroll@skia.org Roll Skia from 083c1a4d9767 to 4e992fb3a9db (1 revision) (flutter/engine#50164) 2024-01-30 mbrase@google.com Use structured logging on Fuchsia (flutter/engine#49918) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from Hqi_x_A9lYsY to Z-xFM2ILZJw2 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 matanl@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
The Impeller Vulkan backend benefits from batching submission to the vk graphics queue. Managing this automatically is non-trivial and adds surprising/fragile thread based behavior, see: #49870
Instead, introduce an impeller::CommandQueue object that command buffers must be submitted to in lieu of CommandBuffer->Submit, which has been made private.
TLDR
old
buffer->Submit();
new
context.GetQueue()->Submit({buffer});
The Metal and GLES implementations internally just call the private CommandBuffer->Submit, though there may be future opportunities to simplify here. The Vulkan implementation is where the meat is.
Aiks takes advantage of this by storing all command buffers on the aiks context while rendering a frame, and then performing one submit in aiks_context render. I don't think this will introduce any thread safety problems, as we don't guarantee much about aiks context - nor do we use it in a multithreaded context as far as I know.
Other tasks such as image upload still just directly submit their command buffers via the queue.
Fixes flutter/flutter#141123