Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions impeller/renderer/backend/vulkan/command_buffer_vk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,15 @@ const std::shared_ptr<CommandEncoderVK>& CommandBufferVK::GetEncoder() const {
}

bool CommandBufferVK::OnSubmitCommands(CompletionCallback callback) {
const auto submit = encoder_->Submit();
if (callback) {
callback(submit ? CommandBuffer::Status::kCompleted
: CommandBuffer::Status::kError);
if (!callback) {
return encoder_->Submit();
}
return submit;
return encoder_->Submit([callback](bool submit) {
if (!submit) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You missed the early return after the callback invocation on error. Thats why I prefer this stylistically too (and its drier).

callback(submitted ? kCompleted : kError);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't work - it fires the callback too early.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh NVM I see.

callback(CommandBuffer::Status::kError);
}
callback(CommandBuffer::Status::kCompleted);
});
}

void CommandBufferVK::OnWaitUntilScheduled() {}
Expand Down
26 changes: 21 additions & 5 deletions impeller/renderer/backend/vulkan/command_encoder_vk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -124,13 +124,23 @@ bool CommandEncoderVK::IsValid() const {
return is_valid_;
}

bool CommandEncoderVK::Submit() {
bool CommandEncoderVK::Submit(SubmitCallback callback) {
// Make sure to call callback with `false` if anything returns early.
bool fail_callback = !!callback;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer a ScopedCleanupClosure that returns false by default. On the one success case, release the closure and invoke it with true once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's being done below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fail_callback is used in the ScopedCleanupClosure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I think I see what you mean.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought there was some way to make this a little clearer with a second fml::ScopedCleanupClosure but I'm missing it. Maybe we can sync up offline.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh, nevermind again, I misread this and now I understand what you're saying :)

if (!IsValid()) {
if (fail_callback) {
callback(false);
}
return false;
}

// Success or failure, you only get to submit once.
fml::ScopedCleanupClosure reset([&]() { Reset(); });
fml::ScopedCleanupClosure reset([&]() {
if (fail_callback) {
callback(false);
}
Reset();
});

InsertDebugMarker("QueueSubmit");

Expand All @@ -155,10 +165,16 @@ bool CommandEncoderVK::Submit() {
return false;
}

// Submit will proceed, call callback with true when it is done and do not
// call when `reset` is collected.
fail_callback = false;

return fence_waiter_->AddFence(
std::move(fence), [tracked_objects = std::move(tracked_objects_)] {
// Nothing to do, we just drop the tracked
// objects on the floor.
std::move(fence),
[callback, tracked_objects = std::move(tracked_objects_)] {
if (callback) {
callback(true);
}
});
}

Expand Down
5 changes: 4 additions & 1 deletion impeller/renderer/backend/vulkan/command_encoder_vk.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#pragma once

#include <functional>
#include <optional>
#include <set>

Expand Down Expand Up @@ -32,11 +33,13 @@ class FenceWaiterVK;

class CommandEncoderVK {
public:
using SubmitCallback = std::function<void(bool)>;

~CommandEncoderVK();

bool IsValid() const;

bool Submit();
bool Submit(SubmitCallback callback = {});

bool Track(std::shared_ptr<SharedObjectVK> object);

Expand Down