From 98092cf7e7fc6775daaef0bd110af55817160810 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sat, 24 Feb 2024 11:42:49 -0800 Subject: [PATCH 1/9] test readback --- impeller/core/device_buffer_descriptor.h | 3 +++ impeller/entity/contents/content_context.h | 2 +- .../renderer/backend/vulkan/allocator_vk.cc | 23 +++++++++++++------ lib/ui/painting/image_encoding_impeller.cc | 1 + 4 files changed, 21 insertions(+), 8 deletions(-) diff --git a/impeller/core/device_buffer_descriptor.h b/impeller/core/device_buffer_descriptor.h index d1bf458942273..6c7c0ca1d29fc 100644 --- a/impeller/core/device_buffer_descriptor.h +++ b/impeller/core/device_buffer_descriptor.h @@ -14,6 +14,9 @@ namespace impeller { struct DeviceBufferDescriptor { StorageMode storage_mode = StorageMode::kDeviceTransient; size_t size = 0u; + // Perhaps we could combine this with storage mode and create appropriate + // host-write and host-read flags. + bool readback = false; }; } // namespace impeller diff --git a/impeller/entity/contents/content_context.h b/impeller/entity/contents/content_context.h index 6d4fc90298689..d38df9ae03b5a 100644 --- a/impeller/entity/contents/content_context.h +++ b/impeller/entity/contents/content_context.h @@ -408,7 +408,7 @@ class ContentContext { /// // TODO(bdero): Remove this setting once StC is fully de-risked // https://github.com/flutter/flutter/issues/123671 - static constexpr bool kEnableStencilThenCover = false; + static constexpr bool kEnableStencilThenCover = true; #if IMPELLER_ENABLE_3D std::shared_ptr GetSceneContext() const; diff --git a/impeller/renderer/backend/vulkan/allocator_vk.cc b/impeller/renderer/backend/vulkan/allocator_vk.cc index 2f90cd768dc4e..0a9547e6b64c1 100644 --- a/impeller/renderer/backend/vulkan/allocator_vk.cc +++ b/impeller/renderer/backend/vulkan/allocator_vk.cc @@ -30,16 +30,23 @@ ToVKBufferMemoryPropertyFlags(StorageMode mode) { } static VmaAllocationCreateFlags ToVmaAllocationBufferCreateFlags( - StorageMode mode) { + StorageMode mode, + bool readback) { VmaAllocationCreateFlags flags = 0; switch (mode) { case StorageMode::kHostVisible: - flags |= VMA_ALLOCATION_CREATE_HOST_ACCESS_SEQUENTIAL_WRITE_BIT; - flags |= VMA_ALLOCATION_CREATE_MAPPED_BIT; + if (!readback) { + flags |= VMA_ALLOCATION_CREATE_HOST_ACCESS_SEQUENTIAL_WRITE_BIT; + flags |= VMA_ALLOCATION_CREATE_MAPPED_BIT; + } else { + flags |= VMA_ALLOCATION_CREATE_HOST_ACCESS_RANDOM_BIT; + } return flags; case StorageMode::kDevicePrivate: + FML_DCHECK(!readback); return flags; case StorageMode::kDeviceTransient: + FML_DCHECK(!readback); return flags; } FML_UNREACHABLE(); @@ -62,8 +69,8 @@ static PoolVMA CreateBufferPool(VmaAllocator allocator) { allocation_info.usage = VMA_MEMORY_USAGE_AUTO; allocation_info.preferredFlags = static_cast( ToVKBufferMemoryPropertyFlags(StorageMode::kHostVisible)); - allocation_info.flags = - ToVmaAllocationBufferCreateFlags(StorageMode::kHostVisible); + allocation_info.flags = ToVmaAllocationBufferCreateFlags( + StorageMode::kHostVisible, /*readback=*/false); uint32_t memTypeIndex; auto result = vk::Result{vmaFindMemoryTypeIndexForBufferInfo( @@ -455,8 +462,10 @@ std::shared_ptr AllocatorVK::OnCreateBuffer( allocation_info.usage = ToVMAMemoryUsage(); allocation_info.preferredFlags = static_cast( ToVKBufferMemoryPropertyFlags(desc.storage_mode)); - allocation_info.flags = ToVmaAllocationBufferCreateFlags(desc.storage_mode); - if (created_buffer_pool_ && desc.storage_mode == StorageMode::kHostVisible) { + allocation_info.flags = + ToVmaAllocationBufferCreateFlags(desc.storage_mode, desc.readback); + if (created_buffer_pool_ && desc.storage_mode == StorageMode::kHostVisible && + !desc.readback) { allocation_info.pool = staging_buffer_pool_.get().pool; } diff --git a/lib/ui/painting/image_encoding_impeller.cc b/lib/ui/painting/image_encoding_impeller.cc index e2e7cf9e9642d..ec61bd93116bd 100644 --- a/lib/ui/painting/image_encoding_impeller.cc +++ b/lib/ui/painting/image_encoding_impeller.cc @@ -157,6 +157,7 @@ void ImageEncodingImpeller::ConvertDlImageToSkImage( impeller::DeviceBufferDescriptor buffer_desc; buffer_desc.storage_mode = impeller::StorageMode::kHostVisible; + buffer_desc.readback = true; buffer_desc.size = texture->GetTextureDescriptor().GetByteSizeOfBaseMipLevel(); auto buffer = From 6f0894b46c62062b9d99bdc325853b3262da5dcd Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sat, 24 Feb 2024 11:45:17 -0800 Subject: [PATCH 2/9] Remove early reset. --- lib/ui/painting/image_encoding_impeller.cc | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/lib/ui/painting/image_encoding_impeller.cc b/lib/ui/painting/image_encoding_impeller.cc index ec61bd93116bd..4bdb188d97e29 100644 --- a/lib/ui/painting/image_encoding_impeller.cc +++ b/lib/ui/painting/image_encoding_impeller.cc @@ -34,22 +34,13 @@ sk_sp ConvertBufferToSkImage( const std::shared_ptr& buffer, SkColorType color_type, SkISize dimensions) { - auto buffer_view = impeller::DeviceBuffer::AsBufferView(buffer); - SkImageInfo image_info = SkImageInfo::Make(dimensions, color_type, SkAlphaType::kPremul_SkAlphaType); SkBitmap bitmap; - auto func = [](void* addr, void* context) { - auto buffer = - static_cast*>(context); - buffer->reset(); - delete buffer; - }; auto bytes_per_pixel = image_info.bytesPerPixel(); bitmap.installPixels(image_info, buffer->OnGetContents(), - dimensions.width() * bytes_per_pixel, func, - new std::shared_ptr(buffer)); + dimensions.width() * bytes_per_pixel); bitmap.setImmutable(); sk_sp raster_image = SkImages::RasterFromBitmap(bitmap); @@ -157,7 +148,7 @@ void ImageEncodingImpeller::ConvertDlImageToSkImage( impeller::DeviceBufferDescriptor buffer_desc; buffer_desc.storage_mode = impeller::StorageMode::kHostVisible; - buffer_desc.readback = true; + buffer_desc.readback = false; // set to false for testing. buffer_desc.size = texture->GetTextureDescriptor().GetByteSizeOfBaseMipLevel(); auto buffer = From 71465637847021e5367e430090f4d7581d6aa883 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sat, 24 Feb 2024 12:08:57 -0800 Subject: [PATCH 3/9] do everything plus barrier. --- impeller/renderer/backend/vulkan/allocator_vk.cc | 2 +- impeller/renderer/backend/vulkan/blit_command_vk.cc | 12 ++++++++++++ lib/ui/painting/image_encoding_impeller.cc | 2 +- 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/impeller/renderer/backend/vulkan/allocator_vk.cc b/impeller/renderer/backend/vulkan/allocator_vk.cc index 0a9547e6b64c1..7147e3d9f16e6 100644 --- a/impeller/renderer/backend/vulkan/allocator_vk.cc +++ b/impeller/renderer/backend/vulkan/allocator_vk.cc @@ -37,10 +37,10 @@ static VmaAllocationCreateFlags ToVmaAllocationBufferCreateFlags( case StorageMode::kHostVisible: if (!readback) { flags |= VMA_ALLOCATION_CREATE_HOST_ACCESS_SEQUENTIAL_WRITE_BIT; - flags |= VMA_ALLOCATION_CREATE_MAPPED_BIT; } else { flags |= VMA_ALLOCATION_CREATE_HOST_ACCESS_RANDOM_BIT; } + flags |= VMA_ALLOCATION_CREATE_MAPPED_BIT; return flags; case StorageMode::kDevicePrivate: FML_DCHECK(!readback); diff --git a/impeller/renderer/backend/vulkan/blit_command_vk.cc b/impeller/renderer/backend/vulkan/blit_command_vk.cc index 467cada841be0..c3c2ebcf88bf1 100644 --- a/impeller/renderer/backend/vulkan/blit_command_vk.cc +++ b/impeller/renderer/backend/vulkan/blit_command_vk.cc @@ -159,6 +159,18 @@ bool BlitCopyTextureToBufferCommandVK::Encode(CommandEncoderVK& encoder) const { image_copy // ); + // If the buffer is used for readback, then apply a transfer -> host memory + // barrier. + if (destination->GetDeviceBufferDescriptor().readback) { + vk::MemoryBarrier barrier; + barrier.srcAccessMask = vk::AccessFlagBits::eTransferWrite; + barrier.dstAccessMask = vk::AccessFlagBits::eHostRead; + + cmd_buffer.pipelineBarrier(vk::PipelineStageFlagBits::eTransfer, + vk::PipelineStageFlagBits::eHost, {}, 1, + &barrier, 0, {}, 0, {}); + } + return true; } diff --git a/lib/ui/painting/image_encoding_impeller.cc b/lib/ui/painting/image_encoding_impeller.cc index 4bdb188d97e29..3a3282a59c750 100644 --- a/lib/ui/painting/image_encoding_impeller.cc +++ b/lib/ui/painting/image_encoding_impeller.cc @@ -148,7 +148,7 @@ void ImageEncodingImpeller::ConvertDlImageToSkImage( impeller::DeviceBufferDescriptor buffer_desc; buffer_desc.storage_mode = impeller::StorageMode::kHostVisible; - buffer_desc.readback = false; // set to false for testing. + buffer_desc.readback = true; // set to false for testing. buffer_desc.size = texture->GetTextureDescriptor().GetByteSizeOfBaseMipLevel(); auto buffer = From 9e95f3c404eae13df023fbbdda4b7141b49d8ee8 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Sat, 24 Feb 2024 12:32:37 -0800 Subject: [PATCH 4/9] Update image_encoding_impeller.cc --- lib/ui/painting/image_encoding_impeller.cc | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/ui/painting/image_encoding_impeller.cc b/lib/ui/painting/image_encoding_impeller.cc index 3a3282a59c750..d519d3665c42e 100644 --- a/lib/ui/painting/image_encoding_impeller.cc +++ b/lib/ui/painting/image_encoding_impeller.cc @@ -37,10 +37,16 @@ sk_sp ConvertBufferToSkImage( SkImageInfo image_info = SkImageInfo::Make(dimensions, color_type, SkAlphaType::kPremul_SkAlphaType); - SkBitmap bitmap; + auto func = [](void* addr, void* context) { + auto buffer = + static_cast*>(context); + buffer->reset(); + delete buffer; + }; auto bytes_per_pixel = image_info.bytesPerPixel(); bitmap.installPixels(image_info, buffer->OnGetContents(), - dimensions.width() * bytes_per_pixel); + dimensions.width() * bytes_per_pixel, func, + new std::shared_ptr(buffer)); bitmap.setImmutable(); sk_sp raster_image = SkImages::RasterFromBitmap(bitmap); From 3e2dcf45b2694428f519595d4904b1f1071bff9b Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Sat, 24 Feb 2024 12:33:23 -0800 Subject: [PATCH 5/9] Update image_encoding_impeller.cc --- lib/ui/painting/image_encoding_impeller.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/ui/painting/image_encoding_impeller.cc b/lib/ui/painting/image_encoding_impeller.cc index d519d3665c42e..7e07e9aa209b2 100644 --- a/lib/ui/painting/image_encoding_impeller.cc +++ b/lib/ui/painting/image_encoding_impeller.cc @@ -36,8 +36,8 @@ sk_sp ConvertBufferToSkImage( SkISize dimensions) { SkImageInfo image_info = SkImageInfo::Make(dimensions, color_type, SkAlphaType::kPremul_SkAlphaType); - - auto func = [](void* addr, void* context) { + SkBitmap bitmap; + auto func = [](void* addr, void* context) { auto buffer = static_cast*>(context); buffer->reset(); From 3fda4d401267187b23a99542e109614369ec7377 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sat, 24 Feb 2024 12:41:17 -0800 Subject: [PATCH 6/9] add invalidate --- impeller/core/device_buffer.cc | 2 ++ impeller/core/device_buffer.h | 2 ++ impeller/renderer/backend/vulkan/device_buffer_vk.cc | 8 ++++++++ impeller/renderer/backend/vulkan/device_buffer_vk.h | 3 +++ lib/ui/painting/image_encoding_impeller.cc | 1 + 5 files changed, 16 insertions(+) diff --git a/impeller/core/device_buffer.cc b/impeller/core/device_buffer.cc index d7262e72b9466..3eae7e0a5f5b0 100644 --- a/impeller/core/device_buffer.cc +++ b/impeller/core/device_buffer.cc @@ -12,6 +12,8 @@ DeviceBuffer::~DeviceBuffer() = default; void DeviceBuffer::Flush(std::optional range) const {} +void DeviceBuffer::Invalidate(std::optional range) const {} + // static BufferView DeviceBuffer::AsBufferView(std::shared_ptr buffer) { BufferView view; diff --git a/impeller/core/device_buffer.h b/impeller/core/device_buffer.h index 619069ddc7179..0306fad46e595 100644 --- a/impeller/core/device_buffer.h +++ b/impeller/core/device_buffer.h @@ -49,6 +49,8 @@ class DeviceBuffer { /// If the range is not provided, the entire buffer is flushed. virtual void Flush(std::optional range = std::nullopt) const; + virtual void Invalidate(std::optional range = std::nullopt) const; + protected: const DeviceBufferDescriptor desc_; diff --git a/impeller/renderer/backend/vulkan/device_buffer_vk.cc b/impeller/renderer/backend/vulkan/device_buffer_vk.cc index 1019aa363ea70..b31b28af20d0f 100644 --- a/impeller/renderer/backend/vulkan/device_buffer_vk.cc +++ b/impeller/renderer/backend/vulkan/device_buffer_vk.cc @@ -5,6 +5,7 @@ #include "impeller/renderer/backend/vulkan/device_buffer_vk.h" #include "flutter/fml/trace_event.h" +#include "flutter_vma/flutter_vma.h" #include "impeller/renderer/backend/vulkan/context_vk.h" #include "vulkan/vulkan_core.h" @@ -70,6 +71,13 @@ void DeviceBufferVK::Flush(std::optional range) const { flush_range.length); } +void DeviceBufferVK::Invalidate(std::optional range) const { + auto flush_range = range.value_or(Range{0, GetDeviceBufferDescriptor().size}); + ::vmaInvalidateAllocation(resource_->buffer.get().allocator, + resource_->buffer.get().allocation, + flush_range.offset, flush_range.length); +} + bool DeviceBufferVK::SetLabel(const std::string& label, Range range) { // We do not have the ability to name ranges. Just name the whole thing. return SetLabel(label); diff --git a/impeller/renderer/backend/vulkan/device_buffer_vk.h b/impeller/renderer/backend/vulkan/device_buffer_vk.h index e5603c9b9051b..744453dc2ac33 100644 --- a/impeller/renderer/backend/vulkan/device_buffer_vk.h +++ b/impeller/renderer/backend/vulkan/device_buffer_vk.h @@ -69,6 +69,9 @@ class DeviceBufferVK final : public DeviceBuffer, // |DeviceBuffer| void Flush(std::optional range) const override; + // |DeviceBuffer| + void Invalidate(std::optional range) const override; + DeviceBufferVK(const DeviceBufferVK&) = delete; DeviceBufferVK& operator=(const DeviceBufferVK&) = delete; diff --git a/lib/ui/painting/image_encoding_impeller.cc b/lib/ui/painting/image_encoding_impeller.cc index 3a3282a59c750..e64ee9c255734 100644 --- a/lib/ui/painting/image_encoding_impeller.cc +++ b/lib/ui/painting/image_encoding_impeller.cc @@ -166,6 +166,7 @@ void ImageEncodingImpeller::ConvertDlImageToSkImage( encode_task(fml::Status(fml::StatusCode::kUnknown, "")); return; } + buffer->Invalidate(); auto sk_image = ConvertBufferToSkImage(buffer, color_type, dimensions); encode_task(sk_image); }; From 9fe20e6dfee2bc43847945b35e24bc99fbb2b698 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sat, 24 Feb 2024 22:08:44 -0800 Subject: [PATCH 7/9] handle void type with location. --- impeller/renderer/backend/gles/buffer_bindings_gles.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/impeller/renderer/backend/gles/buffer_bindings_gles.cc b/impeller/renderer/backend/gles/buffer_bindings_gles.cc index 9d719b3b2cf48..4067498173b9e 100644 --- a/impeller/renderer/backend/gles/buffer_bindings_gles.cc +++ b/impeller/renderer/backend/gles/buffer_bindings_gles.cc @@ -8,6 +8,7 @@ #include #include "impeller/base/validation.h" +#include "impeller/core/shader_types.h" #include "impeller/renderer/backend/gles/device_buffer_gles.h" #include "impeller/renderer/backend/gles/formats_gles.h" #include "impeller/renderer/backend/gles/sampler_gles.h" @@ -269,7 +270,7 @@ bool BufferBindingsGLES::BindUniformBuffer(const ProcTableGLES& gl, const auto& member = metadata->members[i]; auto location = locations[i]; // Void type or inactive uniform. - if (location == -1) { + if (location == -1 || member.type == ShaderType::kVoid) { continue; } @@ -351,7 +352,7 @@ bool BufferBindingsGLES::BindUniformBuffer(const ProcTableGLES& gl, case ShaderType::kSampledImage: case ShaderType::kSampler: VALIDATION_LOG << "Could not bind uniform buffer data for key: " - << member.name; + << member.name << " : " << static_cast(member.type); return false; } } From 4bea95f62b5d389b809adb44844404a3c9327eae Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Tue, 27 Feb 2024 11:20:14 -0800 Subject: [PATCH 8/9] Update content_context.h --- impeller/entity/contents/content_context.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impeller/entity/contents/content_context.h b/impeller/entity/contents/content_context.h index d38df9ae03b5a..6d4fc90298689 100644 --- a/impeller/entity/contents/content_context.h +++ b/impeller/entity/contents/content_context.h @@ -408,7 +408,7 @@ class ContentContext { /// // TODO(bdero): Remove this setting once StC is fully de-risked // https://github.com/flutter/flutter/issues/123671 - static constexpr bool kEnableStencilThenCover = true; + static constexpr bool kEnableStencilThenCover = false; #if IMPELLER_ENABLE_3D std::shared_ptr GetSceneContext() const; From 64c632e397d0a90804f6d454b73101ba03571904 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 27 Feb 2024 11:27:52 -0800 Subject: [PATCH 9/9] add barriers to vk screenshotter. --- impeller/golden_tests/vulkan_screenshotter.mm | 2 ++ 1 file changed, 2 insertions(+) diff --git a/impeller/golden_tests/vulkan_screenshotter.mm b/impeller/golden_tests/vulkan_screenshotter.mm index c413da89990b3..a896c72f6975d 100644 --- a/impeller/golden_tests/vulkan_screenshotter.mm +++ b/impeller/golden_tests/vulkan_screenshotter.mm @@ -29,6 +29,7 @@ buffer_desc.storage_mode = StorageMode::kHostVisible; buffer_desc.size = texture->GetTextureDescriptor().GetByteSizeOfBaseMipLevel(); + buffer_desc.readback = true; std::shared_ptr device_buffer = surface_context->GetResourceAllocator()->CreateBuffer(buffer_desc); FML_CHECK(device_buffer); @@ -52,6 +53,7 @@ .ok(); FML_CHECK(success); latch.Wait(); + device_buffer->Invalidate(); // TODO(gaaclarke): Replace CoreImage requirement with something // crossplatform.