Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Commit 0e586d1

Browse files
author
Jonah Williams
authored
[Impeller] Add interface for submitting multiple command buffers at once. (#50139)
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
1 parent 2878cdd commit 0e586d1

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

54 files changed

+602
-305
lines changed

ci/licenses_golden/licenses_flutter

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5449,6 +5449,8 @@ ORIGIN: ../../../flutter/impeller/renderer/backend/vulkan/command_encoder_vk.cc
54495449
ORIGIN: ../../../flutter/impeller/renderer/backend/vulkan/command_encoder_vk.h + ../../../flutter/LICENSE
54505450
ORIGIN: ../../../flutter/impeller/renderer/backend/vulkan/command_pool_vk.cc + ../../../flutter/LICENSE
54515451
ORIGIN: ../../../flutter/impeller/renderer/backend/vulkan/command_pool_vk.h + ../../../flutter/LICENSE
5452+
ORIGIN: ../../../flutter/impeller/renderer/backend/vulkan/command_queue_vk.cc + ../../../flutter/LICENSE
5453+
ORIGIN: ../../../flutter/impeller/renderer/backend/vulkan/command_queue_vk.h + ../../../flutter/LICENSE
54525454
ORIGIN: ../../../flutter/impeller/renderer/backend/vulkan/compute_pass_vk.cc + ../../../flutter/LICENSE
54535455
ORIGIN: ../../../flutter/impeller/renderer/backend/vulkan/compute_pass_vk.h + ../../../flutter/LICENSE
54545456
ORIGIN: ../../../flutter/impeller/renderer/backend/vulkan/compute_pipeline_vk.cc + ../../../flutter/LICENSE
@@ -5521,6 +5523,8 @@ ORIGIN: ../../../flutter/impeller/renderer/command.cc + ../../../flutter/LICENSE
55215523
ORIGIN: ../../../flutter/impeller/renderer/command.h + ../../../flutter/LICENSE
55225524
ORIGIN: ../../../flutter/impeller/renderer/command_buffer.cc + ../../../flutter/LICENSE
55235525
ORIGIN: ../../../flutter/impeller/renderer/command_buffer.h + ../../../flutter/LICENSE
5526+
ORIGIN: ../../../flutter/impeller/renderer/command_queue.cc + ../../../flutter/LICENSE
5527+
ORIGIN: ../../../flutter/impeller/renderer/command_queue.h + ../../../flutter/LICENSE
55245528
ORIGIN: ../../../flutter/impeller/renderer/compute_pass.cc + ../../../flutter/LICENSE
55255529
ORIGIN: ../../../flutter/impeller/renderer/compute_pass.h + ../../../flutter/LICENSE
55265530
ORIGIN: ../../../flutter/impeller/renderer/compute_pipeline_builder.cc + ../../../flutter/LICENSE
@@ -8298,6 +8302,8 @@ FILE: ../../../flutter/impeller/renderer/backend/vulkan/command_encoder_vk.cc
82988302
FILE: ../../../flutter/impeller/renderer/backend/vulkan/command_encoder_vk.h
82998303
FILE: ../../../flutter/impeller/renderer/backend/vulkan/command_pool_vk.cc
83008304
FILE: ../../../flutter/impeller/renderer/backend/vulkan/command_pool_vk.h
8305+
FILE: ../../../flutter/impeller/renderer/backend/vulkan/command_queue_vk.cc
8306+
FILE: ../../../flutter/impeller/renderer/backend/vulkan/command_queue_vk.h
83018307
FILE: ../../../flutter/impeller/renderer/backend/vulkan/compute_pass_vk.cc
83028308
FILE: ../../../flutter/impeller/renderer/backend/vulkan/compute_pass_vk.h
83038309
FILE: ../../../flutter/impeller/renderer/backend/vulkan/compute_pipeline_vk.cc
@@ -8371,6 +8377,8 @@ FILE: ../../../flutter/impeller/renderer/command.cc
83718377
FILE: ../../../flutter/impeller/renderer/command.h
83728378
FILE: ../../../flutter/impeller/renderer/command_buffer.cc
83738379
FILE: ../../../flutter/impeller/renderer/command_buffer.h
8380+
FILE: ../../../flutter/impeller/renderer/command_queue.cc
8381+
FILE: ../../../flutter/impeller/renderer/command_queue.h
83748382
FILE: ../../../flutter/impeller/renderer/compute_pass.cc
83758383
FILE: ../../../flutter/impeller/renderer/compute_pass.h
83768384
FILE: ../../../flutter/impeller/renderer/compute_pipeline_builder.cc

impeller/aiks/aiks_context.cc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ bool AiksContext::Render(const Picture& picture,
5353
}
5454

5555
fml::ScopedCleanupClosure closure([&]() {
56+
content_context_->FlushCommandBuffers();
5657
if (reset_host_buffer) {
5758
content_context_->GetTransientsBuffer().Reset();
5859
}

impeller/aiks/aiks_unittests.cc

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -141,8 +141,7 @@ TEST_P(AiksTest, CanRenderColorFilterWithInvertColorsDrawPaint) {
141141
namespace {
142142
bool GenerateMipmap(const std::shared_ptr<Context>& context,
143143
std::shared_ptr<Texture> texture,
144-
std::string label,
145-
bool async_submit) {
144+
std::string label) {
146145
auto buffer = context->CreateCommandBuffer();
147146
if (!buffer) {
148147
return false;
@@ -152,23 +151,19 @@ bool GenerateMipmap(const std::shared_ptr<Context>& context,
152151
return false;
153152
}
154153
pass->GenerateMipmap(std::move(texture), std::move(label));
155-
if (async_submit) {
156-
return buffer->EncodeAndSubmit(pass, context->GetResourceAllocator());
157-
}
158154

159155
pass->EncodeCommands(context->GetResourceAllocator());
160-
return buffer->SubmitCommands();
156+
return context->GetCommandQueue()->Submit({buffer}).ok();
161157
}
162158

163159
void CanRenderTiledTexture(AiksTest* aiks_test,
164160
Entity::TileMode tile_mode,
165-
bool async_submit = false,
166161
Matrix local_matrix = {}) {
167162
auto context = aiks_test->GetContext();
168163
ASSERT_TRUE(context);
169164
auto texture = aiks_test->CreateTextureForFixture("table_mountain_nx.png",
170165
/*enable_mipmapping=*/true);
171-
GenerateMipmap(context, texture, "table_mountain_nx", async_submit);
166+
GenerateMipmap(context, texture, "table_mountain_nx");
172167
Canvas canvas;
173168
canvas.Scale(aiks_test->GetContentScale());
174169
canvas.Translate({100.0f, 100.0f, 0});
@@ -215,10 +210,6 @@ TEST_P(AiksTest, CanRenderTiledTextureClamp) {
215210
CanRenderTiledTexture(this, Entity::TileMode::kClamp);
216211
}
217212

218-
TEST_P(AiksTest, CanRenderTiledTextureClampAsync) {
219-
CanRenderTiledTexture(this, Entity::TileMode::kClamp, /*async_submit=*/true);
220-
}
221-
222213
TEST_P(AiksTest, CanRenderTiledTextureRepeat) {
223214
CanRenderTiledTexture(this, Entity::TileMode::kRepeat);
224215
}
@@ -232,7 +223,7 @@ TEST_P(AiksTest, CanRenderTiledTextureDecal) {
232223
}
233224

234225
TEST_P(AiksTest, CanRenderTiledTextureClampWithTranslate) {
235-
CanRenderTiledTexture(this, Entity::TileMode::kClamp, /*async_submit=*/false,
226+
CanRenderTiledTexture(this, Entity::TileMode::kClamp,
236227
Matrix::MakeTranslation({172.f, 172.f, 0.f}));
237228
}
238229

impeller/aiks/testing/context_mock.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,11 @@ class ContextMock : public Context {
111111
(),
112112
(const, override));
113113

114+
MOCK_METHOD(std::shared_ptr<CommandQueue>,
115+
GetCommandQueue,
116+
(),
117+
(const override));
118+
114119
MOCK_METHOD(void, Shutdown, (), (override));
115120
};
116121

impeller/aiks/testing/context_spy.cc

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,23 @@
33
// found in the LICENSE file.
44

55
#include <memory>
6+
#include "impeller/renderer/command_buffer.h"
7+
#include "impeller/renderer/command_queue.h"
68

79
#include "impeller/aiks/testing/context_spy.h"
810

911
namespace impeller {
1012
namespace testing {
1113

14+
fml::Status NoopCommandQueue::Submit(
15+
const std::vector<std::shared_ptr<CommandBuffer>>& buffers,
16+
const CompletionCallback& completion_callback) {
17+
if (completion_callback) {
18+
completion_callback(CommandBuffer::Status::kCompleted);
19+
}
20+
return fml::Status();
21+
}
22+
1223
std::shared_ptr<ContextSpy> ContextSpy::Make() {
1324
return std::shared_ptr<ContextSpy>(new ContextSpy());
1425
}
@@ -50,6 +61,10 @@ std::shared_ptr<ContextMock> ContextSpy::MakeContext(
5061
return real_context->GetPipelineLibrary();
5162
});
5263

64+
ON_CALL(*mock_context, GetCommandQueue).WillByDefault([shared_this]() {
65+
return shared_this->command_queue_;
66+
});
67+
5368
ON_CALL(*mock_context, CreateCommandBuffer)
5469
.WillByDefault([real_context, shared_this]() {
5570
auto real_buffer = real_context->CreateCommandBuffer();

impeller/aiks/testing/context_spy.h

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,18 @@
99

1010
#include "impeller/aiks/testing/context_mock.h"
1111
#include "impeller/entity/contents/test/recording_render_pass.h"
12+
#include "impeller/renderer/command_queue.h"
1213

1314
namespace impeller {
1415
namespace testing {
1516

17+
class NoopCommandQueue : public CommandQueue {
18+
public:
19+
fml::Status Submit(
20+
const std::vector<std::shared_ptr<CommandBuffer>>& buffers,
21+
const CompletionCallback& completion_callback = {}) override;
22+
};
23+
1624
/// Forwards calls to a real Context but can store information about how
1725
/// the Context was used.
1826
class ContextSpy : public std::enable_shared_from_this<ContextSpy> {
@@ -23,6 +31,8 @@ class ContextSpy : public std::enable_shared_from_this<ContextSpy> {
2331
const std::shared_ptr<Context>& real_context);
2432

2533
std::vector<std::shared_ptr<RecordingRenderPass>> render_passes_;
34+
std::shared_ptr<CommandQueue> command_queue_ =
35+
std::make_shared<NoopCommandQueue>();
2636

2737
private:
2838
ContextSpy() = default;

impeller/entity/contents/content_context.cc

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,8 @@ ContentContext::ContentContext(
200200
? std::make_shared<RenderTargetCache>(
201201
context_->GetResourceAllocator())
202202
: std::move(render_target_allocator)),
203-
host_buffer_(HostBuffer::Create(context_->GetResourceAllocator())) {
203+
host_buffer_(HostBuffer::Create(context_->GetResourceAllocator())),
204+
pending_command_buffers_(std::make_unique<PendingCommandBuffers>()) {
204205
if (!context_ || !context_->IsValid()) {
205206
return;
206207
}
@@ -477,9 +478,10 @@ fml::StatusOr<RenderTarget> ContentContext::MakeSubpass(
477478
return fml::Status(fml::StatusCode::kUnknown, "");
478479
}
479480

480-
if (!sub_command_buffer->EncodeAndSubmit(sub_renderpass)) {
481+
if (!sub_renderpass->EncodeCommands()) {
481482
return fml::Status(fml::StatusCode::kUnknown, "");
482483
}
484+
RecordCommandBuffer(std::move(sub_command_buffer));
483485

484486
return subpass_target;
485487
}
@@ -532,4 +534,15 @@ void ContentContext::ClearCachedRuntimeEffectPipeline(
532534
}
533535
}
534536

537+
void ContentContext::RecordCommandBuffer(
538+
std::shared_ptr<CommandBuffer> command_buffer) const {
539+
pending_command_buffers_->command_buffers.push_back(
540+
std::move(command_buffer));
541+
}
542+
543+
void ContentContext::FlushCommandBuffers() const {
544+
auto buffers = std::move(pending_command_buffers_->command_buffers);
545+
GetContext()->GetCommandQueue()->Submit(buffers);
546+
}
547+
535548
} // namespace impeller

impeller/entity/contents/content_context.h

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
#include "impeller/core/host_buffer.h"
1919
#include "impeller/entity/entity.h"
2020
#include "impeller/renderer/capabilities.h"
21+
#include "impeller/renderer/command_buffer.h"
2122
#include "impeller/renderer/pipeline.h"
2223
#include "impeller/renderer/pipeline_descriptor.h"
2324
#include "impeller/renderer/render_target.h"
@@ -268,6 +269,12 @@ using TiledTextureExternalPipeline =
268269
TiledTextureFillExternalFragmentShader>;
269270
#endif // IMPELLER_ENABLE_OPENGLES
270271

272+
// A struct used to isolate command buffer storage from the content
273+
// context options to preserve const-ness.
274+
struct PendingCommandBuffers {
275+
std::vector<std::shared_ptr<CommandBuffer>> command_buffers;
276+
};
277+
271278
/// Pipeline state configuration.
272279
///
273280
/// Each unique combination of these options requires a different pipeline state
@@ -715,6 +722,10 @@ class ContentContext {
715722

716723
void SetWireframe(bool wireframe);
717724

725+
void RecordCommandBuffer(std::shared_ptr<CommandBuffer> command_buffer) const;
726+
727+
void FlushCommandBuffers() const;
728+
718729
using SubpassCallback =
719730
std::function<bool(const ContentContext&, RenderPass&)>;
720731

@@ -1005,6 +1016,7 @@ class ContentContext {
10051016
#endif // IMPELLER_ENABLE_3D
10061017
std::shared_ptr<RenderTargetAllocator> render_target_cache_;
10071018
std::shared_ptr<HostBuffer> host_buffer_;
1019+
std::unique_ptr<PendingCommandBuffers> pending_command_buffers_;
10081020
bool wireframe_ = false;
10091021

10101022
ContentContext(const ContentContext&) = delete;

impeller/entity/contents/content_context_unittests.cc

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,15 @@
44

55
#include <cstdint>
66

7+
#include "fml/logging.h"
78
#include "gtest/gtest.h"
89

910
#include "impeller/core/allocator.h"
1011
#include "impeller/core/device_buffer_descriptor.h"
1112
#include "impeller/entity/contents/content_context.h"
1213
#include "impeller/geometry/color.h"
1314
#include "impeller/renderer/capabilities.h"
15+
#include "impeller/renderer/command_queue.h"
1416
#include "impeller/renderer/pipeline.h"
1517
#include "impeller/renderer/pipeline_descriptor.h"
1618

@@ -52,6 +54,7 @@ class FakeContext : public Context {
5254
std::shared_ptr<PipelineLibrary> GetPipelineLibrary() const {
5355
return nullptr;
5456
}
57+
std::shared_ptr<CommandQueue> GetCommandQueue() const { FML_UNREACHABLE(); }
5558
std::shared_ptr<CommandBuffer> CreateCommandBuffer() const { return nullptr; }
5659
void Shutdown() {}
5760

impeller/entity/entity_pass.cc

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -387,11 +387,12 @@ bool EntityPass::Render(ContentContext& renderer,
387387
blit_pass->AddCopy(
388388
offscreen_target.GetRenderTarget().GetRenderTargetTexture(),
389389
root_render_target.GetRenderTargetTexture());
390-
if (!command_buffer->EncodeAndSubmit(
391-
blit_pass, renderer.GetContext()->GetResourceAllocator())) {
390+
if (!blit_pass->EncodeCommands(
391+
renderer.GetContext()->GetResourceAllocator())) {
392392
VALIDATION_LOG << "Failed to encode root pass blit command.";
393393
return false;
394394
}
395+
renderer.RecordCommandBuffer(std::move(command_buffer));
395396
} else {
396397
auto render_pass = command_buffer->CreateRenderPass(root_render_target);
397398
render_pass->SetLabel("EntityPass Root Render Pass");
@@ -415,10 +416,11 @@ bool EntityPass::Render(ContentContext& renderer,
415416
}
416417
}
417418

418-
if (!command_buffer->EncodeAndSubmit(render_pass)) {
419+
if (!render_pass->EncodeCommands()) {
419420
VALIDATION_LOG << "Failed to encode root pass command buffer.";
420421
return false;
421422
}
423+
renderer.RecordCommandBuffer(std::move(command_buffer));
422424
}
423425

424426
return true;
@@ -855,8 +857,7 @@ bool EntityPass::OnRender(
855857
collapsed_parent_pass) const {
856858
TRACE_EVENT0("impeller", "EntityPass::OnRender");
857859

858-
const std::shared_ptr<Context>& context = renderer.GetContext();
859-
InlinePassContext pass_context(context, pass_target,
860+
InlinePassContext pass_context(renderer, pass_target,
860861
GetTotalPassReads(renderer), GetElementCount(),
861862
collapsed_parent_pass);
862863
if (!pass_context.IsValid()) {

0 commit comments

Comments
 (0)