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.

10 files changed, 185 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>(
diff --git a/tests/cases/multiple_samplers.amber b/tests/cases/multiple_samplers.amber
new file mode 100644
index 0000000..dd37e8e
--- /dev/null
+++ b/tests/cases/multiple_samplers.amber
@@ -0,0 +1,126 @@
+#!amber
+# Copyright 2020 The Amber Authors.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     https://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+SHADER vertex vert_shader PASSTHROUGH
+
+SHADER fragment frag_shader_red GLSL
+#version 430
+layout(location = 0) out vec4 color_out;
+void main() {
+  color_out = vec4(1.0, 0.0, 0.0, 1.0);
+}
+END
+
+SHADER vertex vert_shader_tex GLSL
+#version 430
+layout(location = 0) in vec4 position;
+layout(location = 1) in vec2 texcoords_in;
+layout(location = 0) out vec2 texcoords_out;
+void main() {
+  gl_Position = position;
+  texcoords_out = texcoords_in;
+}
+END
+
+SHADER fragment frag_shader_tex GLSL
+#version 430
+layout(location = 0) in vec2 texcoords_in;
+layout(location = 0) out vec4 color_out;
+uniform layout(set=0, binding=0) sampler2D tex_sampler_white;
+uniform layout(set=0, binding=1) sampler2D tex_sampler_black;
+void main() {
+  // Use a sampler with a white border color for the right
+  // side of the color buffer and a sampler with a black
+  // border color for the left side.
+  if (gl_FragCoord.x > 128.0)
+      color_out = texture(tex_sampler_white, texcoords_in);
+  else
+      color_out = texture(tex_sampler_black, texcoords_in);
+}
+END
+
+BUFFER texture FORMAT R8G8B8A8_UNORM
+BUFFER framebuffer FORMAT B8G8R8A8_UNORM
+
+# A sampler that generates white when texture coordinates
+# go out of range [0..1].
+SAMPLER sampler_white \
+  ADDRESS_MODE_U clamp_to_border \
+  ADDRESS_MODE_V clamp_to_border \
+  BORDER_COLOR float_opaque_white
+
+# A sampler that generates black when texture coordinates
+# go out of range [0..1].
+SAMPLER sampler_black \
+  ADDRESS_MODE_U clamp_to_border \
+  ADDRESS_MODE_V clamp_to_border \
+  BORDER_COLOR float_opaque_black
+
+BUFFER position DATA_TYPE vec2<float> DATA
+-0.75 -0.75
+ 0.75 -0.75
+ 0.75  0.75
+-0.75  0.75
+END
+BUFFER texcoords DATA_TYPE vec2<float> DATA
+-0.5 -0.5
+ 1.5 -0.5
+ 1.5  1.5
+-0.5  1.5
+END
+
+# A pipeline for generating a texture. Renders pure red.
+PIPELINE graphics texgen
+  ATTACH vert_shader
+  ATTACH frag_shader_red
+  FRAMEBUFFER_SIZE 256 256
+  BIND BUFFER texture AS color LOCATION 0
+END
+
+# A pipeline for drawing a textured quad using two samplers.
+PIPELINE graphics pipeline
+  ATTACH vert_shader_tex
+  ATTACH frag_shader_tex
+  BIND BUFFER texture AS combined_image_sampler SAMPLER sampler_white DESCRIPTOR_SET 0 BINDING 0
+  BIND BUFFER texture AS combined_image_sampler SAMPLER sampler_black DESCRIPTOR_SET 0 BINDING 1
+  VERTEX_DATA position LOCATION 0
+  VERTEX_DATA texcoords LOCATION 1
+  FRAMEBUFFER_SIZE 256 256
+  BIND BUFFER framebuffer AS color LOCATION 0
+END
+
+# Generate a texture with a blue background and red quad at the lower right corner.
+CLEAR_COLOR texgen 0 0 255 255
+CLEAR texgen
+RUN texgen DRAW_RECT POS 128 128 SIZE 128 128
+
+# Clear to green and draw the generated texture in the center of the screen leaving
+# the background color to the edges. Use texture coordinates that go past the [0..1]
+# range to show border colors. The left side of the texture is using a sampler with
+# a black border, and the right side uses a sampler with a white border.
+CLEAR_COLOR pipeline 0 255 0 255
+CLEAR pipeline
+RUN pipeline DRAW_ARRAY AS TRIANGLE_FAN START_IDX 0 COUNT 4
+
+# Check for the green background.
+EXPECT framebuffer IDX 1 1 SIZE 1 1 EQ_RGBA 0 255 0 255
+# Check for the black border color.
+EXPECT framebuffer IDX 55 55 SIZE 1 1 EQ_RGBA 0 0 0 255
+# Check for the blue part of the texture.
+EXPECT framebuffer IDX 105 105 SIZE 1 1 EQ_RGBA 0 0 255 255
+# Check for the red part of the texture.
+EXPECT framebuffer IDX 150 150 SIZE 1 1 EQ_RGBA 255 0 0 255
+# Check for the white border color.
+EXPECT framebuffer IDX 200 200 SIZE 1 1 EQ_RGBA 255 255 255 255