Skip data readback for read-only descriptors (#921)

This works as an optimization but also fixes an issue where binding a single buffer multiple times (to read-only descriptors) in a pipeline would fail because Amber would try to copy back to the Amber buffer multiple times.

9 files changed, 59 insertions, 17 deletions

diff --git a/src/command.h b/src/command.h
index c6c7e08..207c336 100644
--- a/src/command.h
+++ b/src/command.h
@@ -552,10 +552,14 @@ class BufferCommand : public BindableResourceCommand {
   void SetBuffer(Buffer* buffer) { buffer_ = buffer; }
   Buffer* GetBuffer() const { return buffer_; }
 
+  void SetSampler(Sampler* sampler) { sampler_ = sampler; }
+  Sampler* GetSampler() const { return sampler_; }
+
   std::string ToString() const override { return "BufferCommand"; }
 
  private:
   Buffer* buffer_ = nullptr;
+  Sampler* sampler_ = nullptr;
   BufferType buffer_type_;
   bool is_subdata_ = false;
   uint32_t offset_ = 0;
diff --git a/src/pipeline.cc b/src/pipeline.cc
index 3e5353a..beb09b1 100644
--- a/src/pipeline.cc
+++ b/src/pipeline.cc
@@ -521,6 +521,7 @@ void Pipeline::AddBuffer(Buffer* buf,
   info.type = type;
   info.base_mip_level = base_mip_level;
   info.dynamic_offset = dynamic_offset;
+  info.sampler = buf->GetSampler();
 }
 
 void Pipeline::AddBuffer(Buffer* buf,
diff --git a/src/pipeline.h b/src/pipeline.h
index ddeb2a4..bdb1679 100644
--- a/src/pipeline.h
+++ b/src/pipeline.h
@@ -199,6 +199,7 @@ class Pipeline {
     uint32_t arg_no = 0;
     BufferType type = BufferType::kUnknown;
     InputRate input_rate = InputRate::kVertex;
+    Sampler* sampler;
   };
 
   /// Information on a sampler attached to the pipeline.
diff --git a/src/vulkan/buffer_backed_descriptor.cc b/src/vulkan/buffer_backed_descriptor.cc
index 746195a..2f7aa98 100644
--- a/src/vulkan/buffer_backed_descriptor.cc
+++ b/src/vulkan/buffer_backed_descriptor.cc
@@ -45,7 +45,11 @@ Result BufferBackedDescriptor::RecordCopyDataToResourceIfNeeded(
   for (size_t i = 0; i < resources.size(); i++) {
     if (!buffers[i]->ValuePtr()->empty()) {
       resources[i]->UpdateMemoryWithRawData(*buffers[i]->ValuePtr());
-      buffers[i]->ValuePtr()->clear();
+      // If the resource is read-only, keep the buffer data; Amber won't copy
+      // read-only resources back into the host buffers, so it makes sense to
+      // leave the buffer intact.
+      if (!IsReadOnly())
+        buffers[i]->ValuePtr()->clear();
     }
 
     resources[i]->CopyToDevice(command);
@@ -55,19 +59,25 @@ Result BufferBackedDescriptor::RecordCopyDataToResourceIfNeeded(
 }
 
 Result BufferBackedDescriptor::RecordCopyDataToHost(CommandBuffer* command) {
-  if (GetResources().empty()) {
-    return Result(
-        "Vulkan: BufferBackedDescriptor::RecordCopyDataToHost() no transfer "
-        "resources");
-  }
+  if (!IsReadOnly()) {
+    if (GetResources().empty()) {
+      return Result(
+          "Vulkan: BufferBackedDescriptor::RecordCopyDataToHost() no transfer "
+          "resources");
+    }
 
-  for (const auto& r : GetResources())
-    r->CopyToHost(command);
+    for (const auto& r : GetResources())
+      r->CopyToHost(command);
+  }
 
   return {};
 }
 
 Result BufferBackedDescriptor::MoveResourceToBufferOutput() {
+  // No need to copy results of read only resources.
+  if (IsReadOnly())
+    return {};
+
   auto resources = GetResources();
   auto buffers = GetAmberBuffers();
   if (resources.size() != buffers.size())
@@ -78,8 +88,7 @@ Result BufferBackedDescriptor::MoveResourceToBufferOutput() {
   if (resources.empty()) {
     return Result(
         "Vulkan: BufferBackedDescriptor::MoveResourceToBufferOutput() no "
-        "transfer"
-        " resource");
+        "transfer resource");
   }
 
   for (size_t i = 0; i < resources.size(); i++) {
@@ -107,5 +116,24 @@ Result BufferBackedDescriptor::MoveResourceToBufferOutput() {
   return {};
 }
 
+bool BufferBackedDescriptor::IsReadOnly() const {
+  switch (type_) {
+    case DescriptorType::kUniformBuffer:
+    case DescriptorType::kUniformBufferDynamic:
+    case DescriptorType::kUniformTexelBuffer:
+    case DescriptorType::kSampledImage:
+    case DescriptorType::kCombinedImageSampler:
+      return true;
+    case DescriptorType::kStorageBuffer:
+    case DescriptorType::kStorageBufferDynamic:
+    case DescriptorType::kStorageTexelBuffer:
+    case DescriptorType::kStorageImage:
+      return false;
+    default:
+      assert(false && "Unexpected descriptor type");
+      return false;
+  }
+}
+
 }  // namespace vulkan
 }  // namespace amber
diff --git a/src/vulkan/buffer_backed_descriptor.h b/src/vulkan/buffer_backed_descriptor.h
index 788bf4e..c626dd8 100644
--- a/src/vulkan/buffer_backed_descriptor.h
+++ b/src/vulkan/buffer_backed_descriptor.h
@@ -42,13 +42,16 @@ class BufferBackedDescriptor : public Descriptor {
   Result RecordCopyDataToResourceIfNeeded(CommandBuffer* command) override;
   Result RecordCopyDataToHost(CommandBuffer* command) override;
   Result MoveResourceToBufferOutput() override;
-  virtual std::vector<Resource*> GetResources() = 0;
   uint32_t GetDescriptorCount() override {
     return static_cast<uint32_t>(amber_buffers_.size());
   }
   const std::vector<Buffer*>& GetAmberBuffers() const { return amber_buffers_; }
   void AddAmberBuffer(Buffer* buffer) { amber_buffers_.push_back(buffer); }
   BufferBackedDescriptor* AsBufferBackedDescriptor() override { return this; }
+  bool IsReadOnly() const;
+
+ protected:
+  virtual std::vector<Resource*> GetResources() = 0;
 
  private:
   std::vector<Buffer*> amber_buffers_;
diff --git a/src/vulkan/buffer_descriptor.cc b/src/vulkan/buffer_descriptor.cc
index 001c45e..510565d 100644
--- a/src/vulkan/buffer_descriptor.cc
+++ b/src/vulkan/buffer_descriptor.cc
@@ -75,6 +75,7 @@ Result BufferDescriptor::CreateResourceIfNeeded() {
 
 Result BufferDescriptor::MoveResourceToBufferOutput() {
   Result r = BufferBackedDescriptor::MoveResourceToBufferOutput();
+
   transfer_buffers_.clear();
 
   return r;
diff --git a/src/vulkan/engine_vulkan.cc b/src/vulkan/engine_vulkan.cc
index 0e65298..035c269 100644
--- a/src/vulkan/engine_vulkan.cc
+++ b/src/vulkan/engine_vulkan.cc
@@ -253,6 +253,7 @@ Result EngineVulkan::CreatePipeline(amber::Pipeline* pipeline) {
     cmd->SetBaseMipLevel(buf_info.base_mip_level);
     cmd->SetDynamicOffset(buf_info.dynamic_offset);
     cmd->SetBuffer(buf_info.buffer);
+    cmd->SetSampler(buf_info.sampler);
 
     if (cmd->GetValues().empty()) {
       cmd->GetBuffer()->SetSizeInElements(cmd->GetBuffer()->ElementCount());
diff --git a/src/vulkan/image_descriptor.cc b/src/vulkan/image_descriptor.cc
index e1e9535..1305a52 100644
--- a/src/vulkan/image_descriptor.cc
+++ b/src/vulkan/image_descriptor.cc
@@ -125,18 +125,21 @@ Result ImageDescriptor::CreateResourceIfNeeded() {
 }
 
 Result ImageDescriptor::RecordCopyDataToHost(CommandBuffer* command) {
-  for (auto& image : transfer_images_) {
-    image->ImageBarrier(command, VK_IMAGE_LAYOUT_TRANSFER_SRC_OPTIMAL,
-                        VK_PIPELINE_STAGE_TRANSFER_BIT);
-  }
+  if (!IsReadOnly()) {
+    for (auto& image : transfer_images_) {
+      image->ImageBarrier(command, VK_IMAGE_LAYOUT_TRANSFER_SRC_OPTIMAL,
+                          VK_PIPELINE_STAGE_TRANSFER_BIT);
+    }
 
-  BufferBackedDescriptor::RecordCopyDataToHost(command);
+    BufferBackedDescriptor::RecordCopyDataToHost(command);
+  }
 
   return {};
 }
 
 Result ImageDescriptor::MoveResourceToBufferOutput() {
   Result r = BufferBackedDescriptor::MoveResourceToBufferOutput();
+
   transfer_images_.clear();
 
   return r;
diff --git a/src/vulkan/pipeline.cc b/src/vulkan/pipeline.cc
index 0bd3e36..4f4075b 100644
--- a/src/vulkan/pipeline.cc
+++ b/src/vulkan/pipeline.cc
@@ -321,7 +321,7 @@ Result Pipeline::AddBufferDescriptor(const BufferCommand* cmd) {
           cmd->GetBuffer(), desc_type, device_, cmd->GetBaseMipLevel(),
           cmd->GetDescriptorSet(), cmd->GetBinding());
       if (cmd->IsSampledImage() || cmd->IsCombinedImageSampler())
-        image_desc->SetAmberSampler(cmd->GetBuffer()->GetSampler());
+        image_desc->SetAmberSampler(cmd->GetSampler());
       descriptors.push_back(std::move(image_desc));
     } else {
       auto buffer_desc = MakeUnique<BufferDescriptor>(