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

[Impeller] leave glyph atlas in transfer dst to improve vulkan throughput. #52908

Merged
merged 4 commits into from
May 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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
3 changes: 2 additions & 1 deletion impeller/renderer/backend/gles/blit_pass_gles.cc
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,8 @@ bool BlitPassGLES::OnCopyBufferToTextureCommand(
std::shared_ptr<Texture> destination,
IRect destination_region,
std::string label,
uint32_t slice) {
uint32_t slice,
bool convert_to_read) {
auto command = std::make_unique<BlitCopyBufferToTextureCommandGLES>();
command->label = label;
command->source = std::move(source);
Expand Down
3 changes: 2 additions & 1 deletion impeller/renderer/backend/gles/blit_pass_gles.h
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ class BlitPassGLES final : public BlitPass,
std::shared_ptr<Texture> destination,
IRect destination_region,
std::string label,
uint32_t slice) override;
uint32_t slice,
bool convert_to_read) override;

// |BlitPass|
bool OnGenerateMipmapCommand(std::shared_ptr<Texture> texture,
Expand Down
3 changes: 2 additions & 1 deletion impeller/renderer/backend/metal/blit_pass_mtl.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ class BlitPassMTL final : public BlitPass {
std::shared_ptr<Texture> destination,
IRect destination_region,
std::string label,
uint32_t slice) override;
uint32_t slice,
bool convert_to_read) override;

// |BlitPass|
bool OnGenerateMipmapCommand(std::shared_ptr<Texture> texture,
Expand Down
3 changes: 2 additions & 1 deletion impeller/renderer/backend/metal/blit_pass_mtl.mm
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,8 @@
std::shared_ptr<Texture> destination,
IRect destination_region,
std::string label,
uint32_t slice) {
uint32_t slice,
bool convert_to_read) {
auto source_mtl = DeviceBufferMTL::Cast(*source.buffer).GetMTLBuffer();
if (!source_mtl) {
return false;
Expand Down
31 changes: 29 additions & 2 deletions impeller/renderer/backend/vulkan/blit_pass_vk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -221,13 +221,37 @@ bool BlitPassVK::OnCopyTextureToBufferCommand(
return true;
}

bool BlitPassVK::ConvertTextureToShaderRead(
const std::shared_ptr<Texture>& texture) {
auto& encoder = *command_buffer_->GetEncoder();
const auto& cmd_buffer = encoder.GetCommandBuffer();

BarrierVK barrier;
barrier.cmd_buffer = cmd_buffer;
barrier.src_access = vk::AccessFlagBits::eTransferWrite;
barrier.src_stage = vk::PipelineStageFlagBits::eTransfer;
barrier.dst_access = vk::AccessFlagBits::eShaderRead;
barrier.dst_stage = vk::PipelineStageFlagBits::eFragmentShader;

barrier.new_layout = vk::ImageLayout::eShaderReadOnlyOptimal;

const auto& texture_vk = TextureVK::Cast(*texture);

if (!encoder.Track(texture)) {
return false;
}

return texture_vk.SetLayout(barrier);
}

// |BlitPass|
bool BlitPassVK::OnCopyBufferToTextureCommand(
BufferView source,
std::shared_ptr<Texture> destination,
IRect destination_region,
std::string label,
uint32_t slice) {
uint32_t slice,
bool convert_to_read) {
auto& encoder = *command_buffer_->GetEncoder();
const auto& cmd_buffer = encoder.GetCommandBuffer();

Expand Down Expand Up @@ -262,6 +286,9 @@ bool BlitPassVK::OnCopyBufferToTextureCommand(
image_copy.imageExtent.height = destination_region.GetHeight();
image_copy.imageExtent.depth = 1u;

// Note: this barrier should do nothing if we're already in the transfer dst
// optimal state. This is important for performance of repeated blit pass
// encoding.
if (!dst.SetLayout(dst_barrier)) {
VALIDATION_LOG << "Could not encode layout transition.";
return false;
Expand All @@ -274,7 +301,7 @@ bool BlitPassVK::OnCopyBufferToTextureCommand(
);

// Transition to shader-read.
{
if (convert_to_read) {
BarrierVK barrier;
barrier.cmd_buffer = cmd_buffer;
barrier.src_access = vk::AccessFlagBits::eTransferWrite;
Expand Down
7 changes: 6 additions & 1 deletion impeller/renderer/backend/vulkan/blit_pass_vk.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ class BlitPassVK final : public BlitPass {
bool EncodeCommands(
const std::shared_ptr<Allocator>& transients_allocator) const override;

// |BlitPass|
bool ConvertTextureToShaderRead(
const std::shared_ptr<Texture>& texture) override;

// |BlitPass|
bool OnCopyTextureToTextureCommand(std::shared_ptr<Texture> source,
std::shared_ptr<Texture> destination,
Expand All @@ -56,7 +60,8 @@ class BlitPassVK final : public BlitPass {
std::shared_ptr<Texture> destination,
IRect destination_region,
std::string label,
uint32_t slice) override;
uint32_t slice,
bool convert_to_read) override;
// |BlitPass|
bool OnGenerateMipmapCommand(std::shared_ptr<Texture> texture,
std::string label) override;
Expand Down
10 changes: 8 additions & 2 deletions impeller/renderer/blit_pass.cc
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,8 @@ bool BlitPass::AddCopy(BufferView source,
std::shared_ptr<Texture> destination,
std::optional<IRect> destination_region,
std::string label,
uint32_t slice) {
uint32_t slice,
bool convert_to_read) {
if (!destination) {
VALIDATION_LOG << "Attempted to add a texture blit with no destination.";
return false;
Expand Down Expand Up @@ -156,7 +157,12 @@ bool BlitPass::AddCopy(BufferView source,

return OnCopyBufferToTextureCommand(std::move(source), std::move(destination),
destination_region_value,
std::move(label), slice);
std::move(label), slice, convert_to_read);
}

bool BlitPass::ConvertTextureToShaderRead(
const std::shared_ptr<Texture>& texture) {
return true;
}

bool BlitPass::GenerateMipmap(std::shared_ptr<Texture> texture,
Expand Down
20 changes: 14 additions & 6 deletions impeller/renderer/blit_pass.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,19 @@ class BlitPass {

void SetLabel(std::string label);

//----------------------------------------------------------------------------
/// @brief If the texture is not already in a shader read internal
/// state, then convert it to that state.
///
/// This API is only used by Vulkan.
virtual bool ConvertTextureToShaderRead(
const std::shared_ptr<Texture>& texture);

//----------------------------------------------------------------------------
/// @brief Record a command to copy the contents of one texture to
/// another texture. The blit area is limited by the intersection
/// of the texture coverage with respect the source region and
/// destination origin.
/// No work is encoded into the command buffer at this time.
///
/// @param[in] source The texture to read for copying.
/// @param[in] destination The texture to overwrite using the source
Expand All @@ -60,7 +67,6 @@ class BlitPass {
//----------------------------------------------------------------------------
/// @brief Record a command to copy the contents of the buffer to
/// the texture.
/// No work is encoded into the command buffer at this time.
///
/// @param[in] source The texture to read for copying.
/// @param[in] destination The buffer to overwrite using the source
Expand All @@ -84,7 +90,6 @@ class BlitPass {
//----------------------------------------------------------------------------
/// @brief Record a command to copy the contents of the buffer to
/// the texture.
/// No work is encoded into the command buffer at this time.
///
/// @param[in] source The buffer view to read for copying.
/// @param[in] destination The texture to overwrite using the source
Expand All @@ -96,6 +101,8 @@ class BlitPass {
/// command.
/// @param[in] slice For cubemap textures, the slice to write
/// data to.
/// @param[in] convert_to_read Whether to convert the texture to a shader
Copy link
Member

Choose a reason for hiding this comment

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

I suppose we can convert this to a UsageHints mask if we need to add more. How about set_read_usage_hint or something similar to indicate this is advisory?

Copy link
Member Author

Choose a reason for hiding this comment

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

The odd part is that this isn't advisory for Vulkan, you'll hit an error state if you bind the dest texture without this (assuming you opted out of shader read conversion).

We need a better system for determining layout transitions but I'm not sure what this is right now. Usage hints doesn't feel like the right approach since its a union of possible states and not the current state.

Copy link
Member

Choose a reason for hiding this comment

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

You mentioned tracking the layouts at the queue level per image once. That seems like a good idea. Have we documented it in an issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

/// read state. Defaults to true.
///
/// @return If the command was valid for subsequent commitment.
///
Expand All @@ -111,11 +118,11 @@ class BlitPass {
std::shared_ptr<Texture> destination,
std::optional<IRect> destination_region = std::nullopt,
std::string label = "",
uint32_t slice = 0);
uint32_t slice = 0,
bool convert_to_read = true);

//----------------------------------------------------------------------------
/// @brief Record a command to generate all mip levels for a texture.
/// No work is encoded into the command buffer at this time.
///
/// @param[in] texture The texture to generate mipmaps for.
/// @param[in] label The optional debug label to give the command.
Expand Down Expand Up @@ -159,7 +166,8 @@ class BlitPass {
std::shared_ptr<Texture> destination,
IRect destination_region,
std::string label,
uint32_t slice) = 0;
uint32_t slice,
bool convert_to_read) = 0;

virtual bool OnGenerateMipmapCommand(std::shared_ptr<Texture> texture,
std::string label) = 0;
Expand Down
3 changes: 2 additions & 1 deletion impeller/renderer/testing/mocks.h
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@ class MockBlitPass : public BlitPass {
std::shared_ptr<Texture> destination,
IRect destination_rect,
std::string label,
uint32_t slice),
uint32_t slice,
bool convert_to_read),
(override));
MOCK_METHOD(bool,
OnGenerateMipmapCommand,
Expand Down
14 changes: 11 additions & 3 deletions impeller/typographer/backends/skia/typographer_context_skia.cc
Original file line number Diff line number Diff line change
Expand Up @@ -298,13 +298,21 @@ static bool UpdateAtlasBitmap(const GlyphAtlas& atlas,

DrawGlyph(canvas, pair.scaled_font, pair.glyph, has_color);

if (!blit_pass->AddCopy(allocator.TakeBufferView(), texture,
// convert_to_read is set to false so that the texture remains in a transfer
// dst layout until we finish writing to it below. This only has an impact
// on Vulkan where we are responsible for managing image layouts.
if (!blit_pass->AddCopy(allocator.TakeBufferView(), //
texture, //
IRect::MakeXYWH(pos->GetLeft(), pos->GetTop(),
size.width, size.height))) {
size.width, size.height), //
/*label=*/"", //
/*slice=*/0, //
/*convert_to_read=*/false //
)) {
return false;
}
}
return true;
return blit_pass->ConvertTextureToShaderRead(texture);
}

std::shared_ptr<GlyphAtlas> TypographerContextSkia::CreateGlyphAtlas(
Expand Down