Make sure color attachment textures have copy-able usage set (#8184)

To ensure the source of readPixels() is properly copy-able, we
want the backing textures to be created with the right BLIT_SRC usage.
However, this was not documented in the API. We workaround the issue
to tag all color attachment textures as BLIT_SRC.

This workaround will be removed in the future. For now, violations of
this condition will elicit a warning being printed out.

filament/backend/src/vulkan/platform/VulkanPlatformSwapChainImpl.cpp

@@ -31,22 +31,22 @@ namespace {
 std::tuple<VkImage, VkDeviceMemory> createImageAndMemory(VulkanContext const& context,
         VkDevice device, VkExtent2D extent, VkFormat format) {
     bool const isDepth = isVkDepthFormat(format);
-    // Filament expects blit() to work with any texture, so we almost always set these usage flags.
-    // TODO: investigate performance implications of setting these flags.
-    VkImageUsageFlags const blittable
-            = VK_IMAGE_USAGE_TRANSFER_DST_BIT | VK_IMAGE_USAGE_TRANSFER_SRC_BIT;
-    VkImageCreateInfo imageInfo{
-            .sType = VK_STRUCTURE_TYPE_IMAGE_CREATE_INFO,
-            .imageType = VK_IMAGE_TYPE_2D,
-            .format = format,
-            .extent = {extent.width, extent.height, 1},
-            .mipLevels = 1,
-            .arrayLayers = 1,
-            .samples = VK_SAMPLE_COUNT_1_BIT,
-            .tiling = VK_IMAGE_TILING_OPTIMAL,
-            .usage = blittable
-                     | (isDepth ? VK_IMAGE_USAGE_DEPTH_STENCIL_ATTACHMENT_BIT
-                                : VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT),
+    // Filament expects blit() to work with any texture, so we almost always set these usage flags
+    // (see copyFrame() and readPixels()).
+    VkImageUsageFlags const blittable =
+            VK_IMAGE_USAGE_TRANSFER_DST_BIT | VK_IMAGE_USAGE_TRANSFER_SRC_BIT;
+
+    VkImageCreateInfo imageInfo {
+        .sType = VK_STRUCTURE_TYPE_IMAGE_CREATE_INFO,
+        .imageType = VK_IMAGE_TYPE_2D,
+        .format = format,
+        .extent = {extent.width, extent.height, 1},
+        .mipLevels = 1,
+        .arrayLayers = 1,
+        .samples = VK_SAMPLE_COUNT_1_BIT,
+        .tiling = VK_IMAGE_TILING_OPTIMAL,
+        .usage = blittable | (isDepth ? VK_IMAGE_USAGE_DEPTH_STENCIL_ATTACHMENT_BIT
+                                      : VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT),
     };
     VkImage image;
     VkResult result = vkCreateImage(device, &imageInfo, VKALLOC, &image);
@@ -220,7 +220,7 @@ VkResult VulkanPlatformSurfaceSwapChain::create() {
             .imageArrayLayers = 1,
             .imageUsage
             = VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT
-              | VK_IMAGE_USAGE_TRANSFER_DST_BIT // Allows use as a blit destination.
+              | VK_IMAGE_USAGE_TRANSFER_DST_BIT // Allows use as a blit destination (for copyFrame)
               | VK_IMAGE_USAGE_TRANSFER_SRC_BIT,// Allows use as a blit source (for readPixels)
 
             // TODO: Setting the preTransform to IDENTITY means we are letting the Android

filament/include/filament/Renderer.h

@@ -508,7 +508,7 @@ class UTILS_PUBLIC Renderer : public FilamentAPI {
      *
      *  Framebuffer as seen on User buffer (PixelBufferDescriptor&)
      *  screen
-     *  
+     *
      *      +--------------------+
      *      |                    |                .stride         .alignment
      *      |                    |         ----------------------->-->
@@ -538,6 +538,9 @@ class UTILS_PUBLIC Renderer : public FilamentAPI {
      * uploaded to it via setImage, the data returned from readPixels will be y-flipped with respect
      * to the setImage call.
      *
+     * Note: the texture that backs the COLOR attachment for `renderTarget` must have
+     * TextureUsage::BLIT_SRC as part of its usage.
+     *
      * @remark
      * readPixels() is intended for debugging and testing. It will impact performance significantly.
      *

filament/src/details/RenderTarget.cpp

@@ -156,8 +156,8 @@ RenderTarget* RenderTarget::Builder::build(Engine& engine) {
 // ------------------------------------------------------------------------------------------------
 
 FRenderTarget::FRenderTarget(FEngine& engine, const RenderTarget::Builder& builder)
-    : mSupportedColorAttachmentsCount(engine.getDriverApi().getMaxDrawBuffers()) {
-
+    : mSupportedColorAttachmentsCount(engine.getDriverApi().getMaxDrawBuffers()),
+      mSupportsReadPixels(false) {
     std::copy(std::begin(builder.mImpl->mAttachments), std::end(builder.mImpl->mAttachments),
             std::begin(mAttachments));
 
@@ -189,6 +189,16 @@ FRenderTarget::FRenderTarget(FEngine& engine, const RenderTarget::Builder& build
                     (TextureUsage::SAMPLEABLE | Texture::Usage::SUBPASS_INPUT))) {
                 mSampleableAttachmentsMask |= targetBufferBit;
             }
+
+            // readPixels() only applies to the color attachment that binds at index 0.
+            if (i == 0 && any(attachment.texture->getUsage() & TextureUsage::COLOR_ATTACHMENT)) {
+
+                // TODO: the following will be changed to
+                //    mSupportsReadPixels =
+                //            any(attachment.texture->getUsage() & TextureUsage::BLIT_SRC);
+                //    in a later filament version when clients have properly added the right usage.
+                mSupportsReadPixels = attachment.texture->hasBlitSrcUsage();
+            }
         }
     }
 

filament/src/details/RenderTarget.h

@@ -68,6 +68,10 @@ class FRenderTarget : public RenderTarget {
 
     bool hasSampleableDepth() const noexcept;
 
+    bool supportsReadPixels() const noexcept {
+        return mSupportsReadPixels;
+    }
+
 private:
     friend class RenderTarget;
     static constexpr size_t ATTACHMENT_COUNT = MAX_SUPPORTED_COLOR_ATTACHMENTS_COUNT + 1u;
@@ -76,6 +80,7 @@ class FRenderTarget : public RenderTarget {
     backend::TargetBufferFlags mAttachmentMask = {};
     backend::TargetBufferFlags mSampleableAttachmentsMask = {};
     const uint8_t mSupportedColorAttachmentsCount;
+    bool mSupportsReadPixels = false;
 };
 
 FILAMENT_DOWNCAST(RenderTarget)

filament/src/details/Renderer.cpp

@@ -455,6 +455,15 @@ void FRenderer::readPixels(uint32_t xoffset, uint32_t yoffset, uint32_t width, u
 void FRenderer::readPixels(FRenderTarget* renderTarget,
         uint32_t xoffset, uint32_t yoffset, uint32_t width, uint32_t height,
         backend::PixelBufferDescriptor&& buffer) {
+
+    // TODO: change the following to an assert when client call sites have addressed the issue.
+    if (!renderTarget->supportsReadPixels()) {
+        utils::slog.w << "readPixels() must be called with a renderTarget with COLOR0 created with "
+                         "TextureUsage::BLIT_SRC.  This precondition will be asserted in a later "
+                         "release of Filament."
+                      << utils::io::endl;
+    }
+
     RendererUtils::readPixels(mEngine.getDriverApi(), renderTarget->getHwHandle(),
             xoffset, yoffset, width, height, std::move(buffer));
 }
@@ -963,15 +972,18 @@ void FRenderer::renderJob(RootArenaScope& rootArenaScope, FView& view) {
     });
     const auto picking = picking_;
 
-
     if (view.hasPicking()) {
         struct PickingResolvePassData {
             FrameGraphId<FrameGraphTexture> picking;
         };
-        fg.addPass<PickingResolvePassData>("Picking Resolve Pass",
+        fg.addPass<PickingResolvePassData>(
+                "Picking Resolve Pass",
                 [&](FrameGraph::Builder& builder, auto& data) {
-                    data.picking = builder.read(picking,
-                            FrameGraphTexture::Usage::COLOR_ATTACHMENT);
+                    // Note that BLIT_SRC is needed because this texture will be read later (via
+                    // readPixels()).
+                    data.picking =
+                            builder.read(picking, FrameGraphTexture::Usage::COLOR_ATTACHMENT |
+                                                          FrameGraphTexture::Usage::BLIT_SRC);
                     builder.declareRenderPass("Picking Resolve Target", {
                             .attachments = { .color = { data.picking }}
                     });

filament/src/details/Texture.cpp

@@ -79,6 +79,7 @@ struct Texture::BuilderDetails {
     Sampler mTarget = Sampler::SAMPLER_2D;
     InternalFormat mFormat = InternalFormat::RGBA8;
     Usage mUsage = Usage::NONE;
+    bool mHasBlitSrc = false;
     bool mTextureIsSwizzled = false;
     std::array<Swizzle, 4> mSwizzle = {
            Swizzle::CHANNEL_0, Swizzle::CHANNEL_1,
@@ -183,6 +184,15 @@ Texture* Texture::Builder::build(Engine& engine) {
         }
     }
 
+    // TODO: remove in a future filament release.
+    // Clients might not have known that textures that are read need to have BLIT_SRC as usages. For
+    // now, we workaround the issue by making sure any color attachment can be the source of a copy
+    // for readPixels().
+    mImpl->mHasBlitSrc = any(mImpl->mUsage & TextureUsage::BLIT_SRC);
+    if (!mImpl->mHasBlitSrc && any(mImpl->mUsage & TextureUsage::COLOR_ATTACHMENT)) {
+        mImpl->mUsage |= TextureUsage::BLIT_SRC;
+    }
+
     const bool sampleable = bool(mImpl->mUsage & TextureUsage::SAMPLEABLE);
     const bool swizzled = mImpl->mTextureIsSwizzled;
     const bool imported = mImpl->mImportedId;
@@ -232,6 +242,7 @@ FTexture::FTexture(FEngine& engine, const Builder& builder) {
     mLevelCount = builder->mLevels;
     mSwizzle = builder->mSwizzle;
     mTextureIsSwizzled = builder->mTextureIsSwizzled;
+    mHasBlitSrc = builder->mHasBlitSrc;
 
     bool const isImported = builder->mImportedId != 0;
     if (mTarget == SamplerType::SAMPLER_EXTERNAL && !isImported) {

filament/src/details/Texture.h

@@ -123,6 +123,11 @@ class FTexture : public Texture {
     bool textureHandleCanMutate() const noexcept;
     void updateLodRange(uint8_t level) noexcept;
 
+    // TODO: remove in a future filament release.  See below for description.
+    inline bool hasBlitSrcUsage() const noexcept {
+        return mHasBlitSrc;
+    }
+
 private:
     friend class Texture;
     struct LodRange {
@@ -163,7 +168,13 @@ class FTexture : public Texture {
     bool mTextureIsSwizzled;
 
     Usage mUsage = Usage::DEFAULT;
-    // there is 7 bytes of padding here
+
+    // TODO: remove in a future filament release.
+    // Indicates whether the user has set the TextureUsage::BLIT_SRC usage. This will be used to
+    // temporarily validate whether this texture can be used for readPixels.
+    bool mHasBlitSrc = false;
+    // there is 5 bytes of padding here
+
     FStream* mStream = nullptr; // only needed for streaming textures
 };