fix external sample path 1

filament/backend/src/vulkan/VulkanDriver.cpp

@@ -395,6 +395,8 @@ void VulkanDriver::collectGarbage() {
     FVK_SYSTRACE_SCOPE();
     // Command buffers need to be submitted and completed before other resources can be gc'd.
     mCommands.gc();
+
+    mExternalImageManager.gc();
     mDescriptorSetCache.gc();
     mStagePool.gc();
     mBufferCache.gc();
@@ -417,7 +419,6 @@ void VulkanDriver::beginFrame(int64_t monotonic_clock_ns,
     //
     // This will let us check if any VulkanBuffer is currently in flight or not.
     mCommands.gc();
-
     if (mAppState.hasExternalSamplers()) {
         mExternalImageManager.onBeginFrame();
     }
@@ -2086,14 +2087,15 @@ void VulkanDriver::bindDescriptorSet(
 
 void VulkanDriver::draw2(uint32_t indexOffset, uint32_t indexCount, uint32_t instanceCount) {
     FVK_SYSTRACE_SCOPE();
-    VkCommandBuffer cmdbuffer = mCurrentRenderPass.commandBuffer->buffer();
-    auto const& [doBindInDraw, bundle] = mPipelineState.bindInDraw;
+    VulkanCommandBuffer* bufferHolder = mCurrentRenderPass.commandBuffer;
+    VkCommandBuffer cmdbuffer = bufferHolder->buffer();
+    auto const [doBindInDraw, bundle] = mPipelineState.bindInDraw;
 
     fvkutils::DescriptorSetMask setsWithExternalSamplers = {};
     if (doBindInDraw) {
         auto& layoutHandles = bundle.dsLayoutHandles;
-        setsWithExternalSamplers = mExternalImageManager.prepareBindSets(layoutHandles,
-                mDescriptorSetCache.getBoundSets());
+        setsWithExternalSamplers = mExternalImageManager.prepareBindSets(bufferHolder,
+                layoutHandles, mDescriptorSetCache.getBoundSets());
 
         VulkanDescriptorSetLayout::DescriptorSetLayoutArray vklayouts;
         for (size_t i = 0; i < layoutHandles.size(); i++) {
@@ -2115,7 +2117,7 @@ void VulkanDriver::draw2(uint32_t indexOffset, uint32_t indexCount, uint32_t ins
         }
         mPipelineState.bindInDraw.first = false;
     }
-    mDescriptorSetCache.commit(mCurrentRenderPass.commandBuffer, mPipelineState.pipelineLayout,
+    mDescriptorSetCache.commit(bufferHolder, mPipelineState.pipelineLayout,
             setsWithExternalSamplers, mPipelineState.descriptorSetMask);
 
     // Finally, make the actual draw call. TODO: support subranges

filament/backend/src/vulkan/VulkanExternalImageManager.cpp

@@ -16,6 +16,7 @@
 
 #include "VulkanExternalImageManager.h"
 
+#include "VulkanCommands.h"
 #include "VulkanDescriptorSetCache.h"
 #include "VulkanDescriptorSetLayoutCache.h"
 #include "VulkanSamplerCache.h"
@@ -51,19 +52,19 @@ ImageData& findImage(std::vector<ImageData>& images,
 }
 
 void copySet(VkDevice device, VkDescriptorSet srcSet, VkDescriptorSet dstSet, Bitmask bindings) {
-    // TODO: fix the size for better memory management
-    std::vector<VkCopyDescriptorSet> copies;
+    VkCopyDescriptorSet copies[sizeof(fvkutils::UniformBufferBitmask)];
+    size_t count = 0;
     bindings.forEachSetBit([&](size_t index) {
-        copies.push_back({
+        copies[count++] ={
             .sType = VK_STRUCTURE_TYPE_COPY_DESCRIPTOR_SET,
             .srcSet = srcSet,
             .srcBinding = (uint32_t) index,
             .dstSet = dstSet,
             .dstBinding = (uint32_t) index,
             .descriptorCount = 1,
-        });
+        };
     });
-    vkUpdateDescriptorSets(device, 0, nullptr, copies.size(), copies.data());
+    vkUpdateDescriptorSets(device, 0, nullptr, count, copies);
 }
 
 Bitmask foldBitsInHalf(Bitmask bitset) {
@@ -103,7 +104,8 @@ void VulkanExternalImageManager::onBeginFrame() {
     });
 }
 
-fvkutils::DescriptorSetMask VulkanExternalImageManager::prepareBindSets(LayoutArray const& layouts,
+fvkutils::DescriptorSetMask VulkanExternalImageManager::prepareBindSets(
+        VulkanCommandBuffer* bufferHolder, LayoutArray const& layouts,
         SetArray const& sets) {
     fvkutils::DescriptorSetMask shouldUseExternalSampler{};
     for (uint8_t i = 0; i < sets.size(); i++) {
@@ -113,7 +115,7 @@ fvkutils::DescriptorSetMask VulkanExternalImageManager::prepareBindSets(LayoutAr
             continue;
         }
         if (hasExternalSampler(set)) {
-            updateSetAndLayout(set, layout);
+            updateSetAndLayout(bufferHolder, set, layout);
             shouldUseExternalSampler.set(i);
         }
     }
@@ -128,6 +130,7 @@ bool VulkanExternalImageManager::hasExternalSampler(
 }
 
 void VulkanExternalImageManager::updateSetAndLayout(
+        VulkanCommandBuffer* bufferHolder,
         fvkmemory::resource_ptr<VulkanDescriptorSet> set,
         fvkmemory::resource_ptr<VulkanDescriptorSetLayout> layout) {
     utils::FixedCapacityVector<
@@ -143,15 +146,9 @@ void VulkanExternalImageManager::updateSetAndLayout(
         auto& imageData = findImage(mImages, bindingInfo.image);
         updateImage(&imageData);
 
-        auto samplerParams = bindingInfo.samplerParams;
-        // according to spec, these must match chromaFilter
-        // https://registry.khronos.org/vulkan/specs/latest/man/html/VkSamplerCreateInfo.html#VUID-VkSamplerCreateInfo-minFilter-01645
-        samplerParams.filterMag = SamplerMagFilter::NEAREST;
-        samplerParams.filterMin = SamplerMinFilter::NEAREST;
-
         auto sampler = mSamplerCache->getSampler({
-            .sampler = samplerParams,
-            .conversion = imageData.conversion,
+            .sampler = bindingInfo.samplerParams,
+            .conversion = imageData.image->getYcbcrConversion(),
         });
         actualExternalSamplers.set(bindingInfo.binding);
         samplerAndBindings.push_back({ bindingInfo.binding, sampler, bindingInfo.image });
@@ -176,29 +173,44 @@ void VulkanExternalImageManager::updateSetAndLayout(
     VkDescriptorSetLayout const newLayout = mDescriptorSetLayoutCache->getVkLayout(layout->bitmask,
             actualExternalSamplers, outSamplers);
 
-    // Need to copy the set
+    // Below, the idea is that we will bind a new set (because the sampler binding of an exisiting
+    // set needs to change to an external sampler).  We will copy from an old external sampler set
+    // (if it exists) or the non-external version of the set.
+
+    // Note that we always need to copy the set, because we are about to update a set that might
+    // have been bound already. We cannot do that without the VK_EXT_descriptor_indexing (core
+    // in 1.2).
     VkDescriptorSet const oldSet = set->getExternalSamplerVkSet();
-    if (oldLayout != newLayout || oldSet == VK_NULL_HANDLE) {
-        // Build a new descriptor set from the new layout
-        VkDescriptorSet const newSet = mDescriptorSetCache->getVkSet(layout->count, newLayout);
-        auto const ubo = layout->bitmask.ubo | layout->bitmask.dynamicUbo;
-        auto const samplers = layout->bitmask.sampler & (~actualExternalSamplers);
+    // Build a new descriptor set from the new layout
+    VkDescriptorSet const newSet = mDescriptorSetCache->getVkSet(layout->count, newLayout);
+    mInUseSets.insert({
+        newSet,
+        {
+            .fenceStatus = bufferHolder->getFenceStatus(),
+            .layout = newLayout,
+            .layoutCount = layout->count,
+        },
+    });
 
-        // Each bitmask denotes a binding index, and separated into two stages - vertex and buffer
-        // We fold the two stages into just the lower half of the bits to denote a combined set of
-        // bindings.
-        Bitmask const copyBindings = foldBitsInHalf(ubo | samplers);
-        VkDescriptorSet const srcSet = oldSet != VK_NULL_HANDLE ? oldSet : set->getVkSet();
-        copySet(mPlatform->getDevice(), srcSet, newSet, copyBindings);
+    auto const ubo = layout->bitmask.ubo | layout->bitmask.dynamicUbo;
+    auto const samplers = layout->bitmask.sampler & (~actualExternalSamplers);
 
-        set->setExternalSamplerVkSet(newSet,
-                [&descriptorSetCache = mDescriptorSetCache, layoutCount = layout->count, newLayout,
-                        newSet](VulkanDescriptorSet*) {
-                    descriptorSetCache->manualRecycle(layoutCount, newLayout, newSet);
-                });
-        if (oldLayout != newLayout) {
-            layout->setExternalSamplerVkLayout(newLayout);
-        }
+    // Each bitmask denotes a binding index, and separated into two stages - vertex and buffer
+    // We fold the two stages into just the lower half of the bits to denote a combined set of
+    // bindings.
+    Bitmask const copyBindings = foldBitsInHalf(ubo | samplers);
+    VkDescriptorSet const srcSet = oldSet != VK_NULL_HANDLE ? oldSet : set->getVkSet();
+    copySet(mPlatform->getDevice(), srcSet, newSet, copyBindings);
+
+    set->setExternalSamplerVkSet(newSet);
+    if (oldSet) {
+        // Now that we're replacing the old set, we can recycle it once it's no longer referenced in
+        // a command buffer.
+        mInUseSets[oldSet].recyclable = true;
+    }
+
+    if (oldLayout != newLayout) {
+        layout->setExternalSamplerVkLayout(newLayout);
     }
 
     // Update the external samplers in the set
@@ -240,13 +252,7 @@ void VulkanExternalImageManager::updateImage(ImageData* image) {
     image->hasBeenValidated = true;
 
     auto metadata = mPlatform->extractExternalImageMetadata(image->platformHandle);
-    auto vkYcbcr = getVkSamplerYcbcrConversion(metadata);
-    if (vkYcbcr == image->conversion) {
-        return;
-    }
-
-    image->image->setYcbcrConversion(vkYcbcr);
-    image->conversion = vkYcbcr;
+    image->image->setYcbcrConversion(getVkSamplerYcbcrConversion(metadata));
     return;
 }
 
@@ -262,6 +268,10 @@ void VulkanExternalImageManager::bindExternallySampledTexture(
     // Should we do duplicate validation here?
     auto& imageData = findImage(mImages, image);
     mSetBindings.push_back({ bindingPoint, imageData.image, set, samplerParams });
+    // according to spec, these must match chromaFilter
+    // https://registry.khronos.org/vulkan/specs/latest/man/html/VkSamplerCreateInfo.html#VUID-VkSamplerCreateInfo-minFilter-01645
+    samplerParams.filterMag = SamplerMagFilter::NEAREST;
+    samplerParams.filterMin = SamplerMinFilter::NEAREST;
 }
 
 void VulkanExternalImageManager::addExternallySampledTexture(
@@ -293,4 +303,20 @@ void VulkanExternalImageManager::clearTextureBinding(
     });
 }
 
+void VulkanExternalImageManager::gc() {
+    std::vector<VkDescriptorSet> removedSets;
+    removedSets.reserve(mInUseSets.size());
+    for (auto const& [set, boundSetInfo]: mInUseSets) {
+        // It's no longer in use by the command buffer. We're free to recycle it.
+        if (boundSetInfo.fenceStatus->getStatus() == VK_SUCCESS && boundSetInfo.recyclable) {
+            removedSets.push_back(set);
+            mDescriptorSetCache->manualRecycle(boundSetInfo.layoutCount, boundSetInfo.layout, set);
+        }
+    }
+
+    for (auto set: removedSets) {
+        mInUseSets.erase(set);
+    }
+}
+
 } // namesapce filament::backend

filament/backend/src/vulkan/VulkanExternalImageManager.h

@@ -18,6 +18,8 @@
 #define TNT_FILAMENT_BACKEND_CACHING_VULKANEXTERNALIMAGEMANAGER_H
 
 #include "VulkanHandles.h"
+#include "vulkan/VulkanAsyncHandles.h"
+#include "vulkan/VulkanCommands.h"
 
 #include <backend/DriverEnums.h>
 
@@ -58,7 +60,9 @@ public:
 
     // Returns  bitmask to indicate whether or not to use the external sampler version of each
     // descriptor set.
-    fvkutils::DescriptorSetMask prepareBindSets(LayoutArray const& layouts, SetArray const& sets);
+    fvkutils::DescriptorSetMask prepareBindSets(
+            VulkanCommandBuffer* bufferHolder,
+            LayoutArray const& layouts, SetArray const& sets);
 
     void removeDescriptorSet(fvkmemory::resource_ptr<VulkanDescriptorSet> set);
 
@@ -83,13 +87,15 @@ public:
         fvkmemory::resource_ptr<VulkanTexture> image;
         Platform::ExternalImageHandle platformHandle;
         bool hasBeenValidated = false; // indicates whether the image has been validated *this frame*
-        VkSamplerYcbcrConversion conversion = VK_NULL_HANDLE;
     };
 
     bool hasExternalSampler(fvkmemory::resource_ptr<VulkanDescriptorSet> set) const;
 
+    void gc();
+
 private:
-    void updateSetAndLayout(fvkmemory::resource_ptr<VulkanDescriptorSet> set,
+    void updateSetAndLayout(VulkanCommandBuffer* bufferHolder,
+            fvkmemory::resource_ptr<VulkanDescriptorSet> set,
             fvkmemory::resource_ptr<VulkanDescriptorSetLayout> layout);
 
     void updateImage(ImageData* imageData);
@@ -114,6 +120,15 @@ private:
     // Use vectors instead of hash maps because we only expect small number of entries.
     std::vector<SetBindingInfo> mSetBindings;
     std::vector<ImageData> mImages;
+
+    // Keep metadata to manage recycling sets that have been bound and used.
+    struct BoundSetInfo {
+        std::shared_ptr<VulkanCmdFence> fenceStatus;
+        VkDescriptorSetLayout layout = VK_NULL_HANDLE;
+        VulkanDescriptorSetLayout::Count layoutCount = {};
+        bool recyclable = false;
+    };
+    std::unordered_map<VkDescriptorSet, BoundSetInfo> mInUseSets;
 };
 
 } // filament::backend

filament/backend/src/vulkan/VulkanHandles.h

@@ -172,9 +172,6 @@ public:
         if (mOnRecycleFn) {
             mOnRecycleFn(this);
         }
-        if (mOnRecycleExternalSamplerFn) {
-            mOnRecycleExternalSamplerFn(this);
-        }
     }
 
     VkDescriptorSet getVkSet() const noexcept {
@@ -185,12 +182,8 @@ public:
         return mExternalSamplerVkSet;
     }
 
-    void setExternalSamplerVkSet(VkDescriptorSet vkset, OnRecycle onRecycle) {
+    void setExternalSamplerVkSet(VkDescriptorSet vkset) {
         mExternalSamplerVkSet = vkset;
-        if (mOnRecycleExternalSamplerFn) {
-            mOnRecycleExternalSamplerFn(this);
-        }
-        mOnRecycleExternalSamplerFn = onRecycle;
     }
 
     void setOffsets(backend::DescriptorSetOffsetArray&& offsets) noexcept {
@@ -221,7 +214,6 @@ private:
     backend::DescriptorSetOffsetArray mOffsets;
     std::vector<fvkmemory::resource_ptr<fvkmemory::Resource>> mResources;
     OnRecycle mOnRecycleFn;
-    OnRecycle mOnRecycleExternalSamplerFn;
     fvkutils::UniformBufferBitmask mUboMask;
 };
 

filament/backend/src/vulkan/VulkanTexture.cpp

@@ -853,6 +853,10 @@ void VulkanTexture::setYcbcrConversion(VkSamplerYcbcrConversion conversion) {
     }
 }
 
+VkSamplerYcbcrConversion VulkanTexture::getYcbcrConversion() const noexcept {
+    return mState->mYcbcr.conversion;
+}
+
 VulkanLayout VulkanTexture::getLayout(uint32_t layer, uint32_t level) const {
     assert_invariant(level <= 0xffff && layer <= 0xffff);
     const uint32_t key = (layer << 16) | level;

filament/backend/src/vulkan/VulkanTexture.h

@@ -219,6 +219,8 @@ struct VulkanTexture : public HwTexture, fvkmemory::Resource {
     // happens.
     void setYcbcrConversion(VkSamplerYcbcrConversion conversion);
 
+    VkSamplerYcbcrConversion getYcbcrConversion() const noexcept;
+
 #if FVK_ENABLED(FVK_DEBUG_TEXTURE)
     void print() const;
 #endif