Merge branch 'main' into kpiascik/shadowtestassert

A zero color target fragment stage implies a depth pass by Filament.
We can create a pipeline without a fragment stage to make it valid.

BUGS=[421933134]

filament/backend/src/vulkan/VulkanStagePool.cpp

@@ -156,27 +156,27 @@ void VulkanStagePool::destroyStage(VulkanStage const*&& stage) {
     delete stage;
 }
 
-VulkanStageImage const* VulkanStagePool::acquireImage(PixelDataFormat format, PixelDataType type,
-        uint32_t width, uint32_t height) {
+fvkmemory::resource_ptr<VulkanStageImage::Resource> VulkanStagePool::acquireImage(
+        PixelDataFormat format, PixelDataType type, uint32_t width, uint32_t height) {
+    // Helper lambda so we can return stage images wrapped as resources that can
+    // be held by command buffers until no longer needed.
+    auto wrapAsResource = [this](VulkanStageImage* image) {
+        auto recycleFn = [this](VulkanStageImage* image) {
+            this->mFreeImages.insert(image);
+        };
+        return fvkmemory::resource_ptr<VulkanStageImage::Resource>::construct(
+            this->mResManager, image, recycleFn);
+    };
+
     const VkFormat vkformat = fvkutils::getVkFormat(format, type);
-    for (auto image : mFreeImages) {
-        if (image->format == vkformat && image->width == width && image->height == height) {
-            mFreeImages.erase(image);
-            image->lastAccessed = mCurrentFrame;
-            mUsedImages.push_back(image);
-            return image;
+    for (auto stageImage : mFreeImages) {
+        if (stageImage->format() == vkformat && stageImage->width() == width && stageImage->height() == height) {
+            mFreeImages.erase(stageImage);
+            stageImage->mLastAccessed = mCurrentFrame;
+            return wrapAsResource(stageImage);
         }
     }
 
-    VulkanStageImage* image = new VulkanStageImage({
-        .format = vkformat,
-        .width = width,
-        .height = height,
-        .lastAccessed = mCurrentFrame,
-    });
-
-    mUsedImages.push_back(image);
-
     const VkImageCreateInfo imageInfo = {
         .sType = VK_STRUCTURE_TYPE_IMAGE_CREATE_INFO,
         .imageType = VK_IMAGE_TYPE_2D,
@@ -194,14 +194,19 @@ VulkanStageImage const* VulkanStagePool::acquireImage(PixelDataFormat format, Pi
         .usage = VMA_MEMORY_USAGE_CPU_TO_GPU
     };
 
+    VkImage image;
+    VmaAllocation memory;
     const UTILS_UNUSED VkResult result = vmaCreateImage(mAllocator, &imageInfo, &allocInfo,
-            &image->image, &image->memory, nullptr);
+            &image, &memory, nullptr);
 
     assert_invariant(result == VK_SUCCESS);
 
     VkImageAspectFlags const aspectFlags = fvkutils::getImageAspect(vkformat);
     VkCommandBuffer const cmdbuffer = mCommands->get().buffer();
 
+    VulkanStageImage* stageImage = new VulkanStageImage(
+        vkformat, width, height, memory, image, mCurrentFrame);
+
     // We use VK_IMAGE_LAYOUT_GENERAL here because the spec says:
     // "Host access to image memory is only well-defined for linear images and for image
     // subresources of those images which are currently in either the
@@ -209,12 +214,13 @@ VulkanStageImage const* VulkanStagePool::acquireImage(PixelDataFormat format, Pi
     // vkGetImageSubresourceLayout for a linear image returns a subresource layout mapping that is
     // valid for either of those image layouts."
     fvkutils::transitionLayout(cmdbuffer, {
-            .image = image->image,
+            .image = stageImage->image(),
             .oldLayout = VulkanLayout::UNDEFINED,
             .newLayout = VulkanLayout::STAGING, // (= VK_IMAGE_LAYOUT_GENERAL)
             .subresources = { aspectFlags, 0, 1, 0, 1 },
         });
-    return image;
+
+    return wrapAsResource(stageImage);
 }
 
 void VulkanStagePool::gc() noexcept {
@@ -262,25 +268,14 @@ void VulkanStagePool::gc() noexcept {
     decltype(mFreeImages) freeImages;
     freeImages.swap(mFreeImages);
     for (auto image : freeImages) {
-        if (image->lastAccessed < evictionTime) {
-            vmaDestroyImage(mAllocator, image->image, image->memory);
+        if (image->mLastAccessed < evictionTime) {
+            vmaDestroyImage(mAllocator, image->image(), image->memory());
             delete image;
         } else {
             mFreeImages.insert(image);
         }
     }
 
-    // Reclaim images that are no longer being used by any command buffer.
-    decltype(mUsedImages) usedImages;
-    usedImages.swap(mUsedImages);
-    for (auto image : usedImages) {
-        if (image->lastAccessed < evictionTime) {
-            image->lastAccessed = mCurrentFrame;
-            mFreeImages.insert(image);
-        } else {
-            mUsedImages.push_back(image);
-        }
-    }
     FVK_SYSTRACE_END();
 }
 
@@ -290,14 +285,8 @@ void VulkanStagePool::terminate() noexcept {
     }
     mStages.clear();
 
-    for (auto image : mUsedImages) {
-        vmaDestroyImage(mAllocator, image->image, image->memory);
-        delete image;
-    }
-    mUsedImages.clear();
-
     for (auto image : mFreeImages) {
-        vmaDestroyImage(mAllocator, image->image, image->memory);
+        vmaDestroyImage(mAllocator, image->image(), image->memory());
         delete image;
     }
     mFreeImages.clear();

filament/backend/src/vulkan/VulkanStagePool.h

@@ -126,13 +126,70 @@ private:
     std::unordered_map<uint32_t, Segment*> mSegments;
 };
 
-struct VulkanStageImage {
-    VkFormat format;
-    uint32_t width;
-    uint32_t height;
-    mutable uint64_t lastAccessed;
-    VmaAllocation memory;
-    VkImage image;
+class VulkanStageImage {
+  public:
+    class Resource : public fvkmemory::Resource {
+      public:
+        using RecycleFn = std::function<void(VulkanStageImage*)>;
+
+        Resource(VulkanStageImage* image, RecycleFn&& onRecycleFn)
+            : mImage(image),
+              mOnRecycleFn(onRecycleFn)
+        {}
+
+        ~Resource() {
+            if (mOnRecycleFn) {
+                mOnRecycleFn(mImage);
+            }
+        }
+
+        inline VkFormat format() const { return mImage->format(); }
+        inline uint32_t width() const { return mImage->width(); }
+        inline uint32_t height() const { return mImage->height(); }
+        inline VmaAllocation memory() const { return mImage->memory(); }
+        inline VkImage image() const { return mImage->image(); }
+
+      private:
+        Resource() = delete;
+        Resource(const Resource& other) = delete;
+        Resource(Resource&& other) = delete;
+        Resource& operator=(const Resource& other) = delete;
+        Resource& operator=(Resource&& other) = delete;
+
+        VulkanStageImage* const mImage;
+        RecycleFn mOnRecycleFn;
+    };
+
+    VulkanStageImage(VkFormat format, uint32_t width, uint32_t height, VmaAllocation memory,
+            VkImage image, uint64_t lastAccessed)
+        : mFormat(format),
+          mWidth(width),
+          mHeight(height),
+          mMemory(memory),
+          mImage(image),
+          mLastAccessed(lastAccessed) {}
+
+    VulkanStageImage(const VulkanStageImage& other) = delete;
+    VulkanStageImage(VulkanStageImage&& other) = delete;
+    VulkanStageImage& operator=(const VulkanStageImage& other) = delete;
+    VulkanStageImage& operator=(VulkanStageImage&& other) = delete;
+
+    inline VkFormat format() const { return mFormat; }
+    inline uint32_t width() const { return mWidth; }
+    inline uint32_t height() const { return mHeight; }
+    inline VmaAllocation memory() const { return mMemory; }
+    inline VkImage image() const { return mImage; }
+
+  private:
+    const VkFormat mFormat;
+    const uint32_t mWidth;
+    const uint32_t mHeight;
+    const VmaAllocation mMemory;
+    const VkImage mImage;
+
+    uint64_t mLastAccessed;
+    // Denote as a friend so that it can update mLastAccessed.
+    friend class VulkanStagePool;
 };
 
 // Manages a pool of stages, periodically releasing stages that have been unused for a while.
@@ -153,7 +210,7 @@ public:
             uint32_t alignment = 0);
 
     // Images have VK_IMAGE_LAYOUT_GENERAL and must not be transitioned to any other layout
-    VulkanStageImage const* acquireImage(PixelDataFormat format, PixelDataType type,
+    fvkmemory::resource_ptr<VulkanStageImage::Resource> acquireImage(PixelDataFormat format, PixelDataType type,
             uint32_t width, uint32_t height);
 
     // Evicts old unused stages and bumps the current frame number.
@@ -193,8 +250,7 @@ private:
     // Use an ordered multimap for quick (capacity => stage) lookups using lower_bound().
     std::multimap<uint32_t, VulkanStage*> mStages;
 
-    std::unordered_set<VulkanStageImage const*> mFreeImages;
-    std::vector<VulkanStageImage const*> mUsedImages;
+    std::unordered_set<VulkanStageImage*> mFreeImages;
 
     // Store the current "time" (really just a frame count) and LRU eviction parameters.
     uint64_t mCurrentFrame = 0;

filament/backend/src/vulkan/VulkanTexture.cpp

@@ -602,15 +602,16 @@ void VulkanTexture::updateImageWithBlit(const PixelBufferDescriptor& data, uint3
     size_t const writeSize = bpp > 0 ? width * height * depth * bpp : data.size;
 
     void* mapped = nullptr;
-    VulkanStageImage const* stage
+    fvkmemory::resource_ptr<VulkanStageImage::Resource> stage
             = mState->mStagePool.acquireImage(data.format, data.type, width, height);
-    vmaMapMemory(mState->mAllocator, stage->memory, &mapped);
+    vmaMapMemory(mState->mAllocator, stage->memory(), &mapped);
     adjustedMemcpy(mapped, data, width, height, depth);
-    vmaUnmapMemory(mState->mAllocator, stage->memory);
-    vmaFlushAllocation(mState->mAllocator, stage->memory, 0, writeSize);
+    vmaUnmapMemory(mState->mAllocator, stage->memory());
+    vmaFlushAllocation(mState->mAllocator, stage->memory(), 0, writeSize);
 
     VulkanCommandBuffer& commands = mState->mCommands->get();
     VkCommandBuffer const cmdbuf = commands.buffer();
+    commands.acquire(stage);
     commands.acquire(fvkmemory::resource_ptr<VulkanTexture>::cast(this));
 
     // TODO: support blit-based format conversion for 3D images and cubemaps.
@@ -633,7 +634,7 @@ void VulkanTexture::updateImageWithBlit(const PixelBufferDescriptor& data, uint3
     VulkanLayout const oldLayout = getLayout(layer, miplevel);
     transitionLayout(&commands, range, newLayout);
 
-    vkCmdBlitImage(cmdbuf, stage->image, fvkutils::getVkLayout(VulkanLayout::TRANSFER_SRC),
+    vkCmdBlitImage(cmdbuf, stage->image(), fvkutils::getVkLayout(VulkanLayout::TRANSFER_SRC),
             mState->mTextureImage, fvkutils::getVkLayout(newLayout), 1, blitRegions, VK_FILTER_NEAREST);
 
     transitionLayout(&commands, range, oldLayout);

filament/backend/src/vulkan/memory/Resource.cpp

@@ -27,6 +27,7 @@ template ResourceType getTypeEnum<VulkanProgram>() noexcept;
 template ResourceType getTypeEnum<VulkanRenderTarget>() noexcept;
 template ResourceType getTypeEnum<VulkanSwapChain>() noexcept;
 template ResourceType getTypeEnum<VulkanStage::Segment>() noexcept;
+template ResourceType getTypeEnum<VulkanStageImage::Resource>() noexcept;
 template ResourceType getTypeEnum<VulkanRenderPrimitive>() noexcept;
 template ResourceType getTypeEnum<VulkanTexture>() noexcept;
 template ResourceType getTypeEnum<VulkanTextureState>() noexcept;
@@ -58,6 +59,9 @@ ResourceType getTypeEnum() noexcept {
     if constexpr (std::is_same_v<D, VulkanStage::Segment>) {
         return ResourceType::STAGE_SEGMENT;
     }
+    if constexpr (std::is_same_v<D, VulkanStageImage::Resource>) {
+        return ResourceType::STAGE_IMAGE;
+    }
     if constexpr (std::is_same_v<D, VulkanRenderPrimitive>) {
         return ResourceType::RENDER_PRIMITIVE;
     }
@@ -105,6 +109,8 @@ std::string getTypeStr(ResourceType type) {
             return "SwapChain";
         case ResourceType::STAGE_SEGMENT:
             return "Stage::Segment";
+        case ResourceType::STAGE_IMAGE:
+            return "Stage::Image";
         case ResourceType::RENDER_PRIMITIVE:
             return "RenderPrimitive";
         case ResourceType::TEXTURE:

filament/backend/src/vulkan/memory/Resource.h

@@ -51,7 +51,8 @@ enum class ResourceType : uint8_t {
     FENCE = 13,
     VULKAN_BUFFER = 14,
     STAGE_SEGMENT = 15,
-    UNDEFINED_TYPE = 16,    // Must be the last enum because we use it for iterating over the enums.
+    STAGE_IMAGE = 16,
+    UNDEFINED_TYPE = 17,    // Must be the last enum because we use it for iterating over the enums.
 };
 
 template<typename D>

filament/backend/src/vulkan/memory/ResourceManager.cpp

@@ -81,6 +81,9 @@ void ResourceManager::destroyWithType(ResourceType type, HandleId id) {
         case ResourceType::STAGE_SEGMENT:
             destruct<VulkanStage::Segment>(Handle<VulkanStage::Segment>(id));
             break;
+        case ResourceType::STAGE_IMAGE:
+            destruct<VulkanStageImage::Resource>(Handle<VulkanStageImage::Resource>(id));
+            break;
         case ResourceType::RENDER_PRIMITIVE:
             destruct<VulkanRenderPrimitive>(Handle<VulkanRenderPrimitive>(id));
             break;

filament/backend/src/webgpu/WebGPUDriver.cpp

@@ -591,7 +591,7 @@ bool WebGPUDriver::isParallelShaderCompileSupported() {
 }
 
 bool WebGPUDriver::isDepthStencilResolveSupported() {
-    return true;
+    return false;
 }
 
 bool WebGPUDriver::isDepthStencilBlitSupported(const TextureFormat format) {
@@ -1215,12 +1215,6 @@ void WebGPUDriver::bindPipeline(PipelineState const& pipelineState) {
         }
     }
 
-    // TODO: We expected this to be a sane check, however it complains when running shadowtest.
-    //if (program->fragmentShaderModule != nullptr) {
-    //    FILAMENT_CHECK_POSTCONDITION(!pipelineColorFormats.empty())
-    //            << "Render pipeline with fragment shader must have at least one color target "
-    //               "format.";
-    //}
     wgpu::RenderPipeline pipeline = createWebGPURenderPipeline(mDevice, *program, *vertexBufferInfo,
             layout, pipelineState.rasterState, pipelineState.stencilState,
             pipelineState.polygonOffset, pipelineState.primitiveType, pipelineColorFormats,

filament/backend/src/webgpu/WebGPUPipelineCreation.cpp

@@ -281,27 +281,32 @@ wgpu::RenderPipeline createWebGPURenderPipeline(wgpu::Device const& device,
         }
     };
     if (program.fragmentShaderModule != nullptr) {
-        fragmentState.module = program.fragmentShaderModule;
-        fragmentState.entryPoint = "main";
-        // see the comment about constants for the vertex state, as the same reasoning applies
-        // here
-        fragmentState.constantCount = 0,
-        fragmentState.constants = nullptr,
-        fragmentState.targetCount = colorFormats.size();
-        fragmentState.targets = colorTargets.data();
-        assert_invariant(fragmentState.targetCount <= MRT::MAX_SUPPORTED_RENDER_TARGET_COUNT);
-        // We expect a fragment shader implies at least one color target if it outputs color.
-        // This should be guaranteed by the caller ensuring colorFormats is not empty.
-        // However, this fails on shadowtest.cpp, TODO investigate why
-        // assert_invariant(fragmentState.targetCount > 0);
-        for (size_t targetIndex = 0; targetIndex < fragmentState.targetCount; targetIndex++) {
-            auto& colorTarget = colorTargets[targetIndex];
-            colorTarget.format = colorFormats[targetIndex];
-            colorTarget.blend = rasterState.hasBlending() ? &blendState : nullptr;
-            colorTarget.writeMask =
-                    rasterState.colorWrite ? wgpu::ColorWriteMask::All : wgpu::ColorWriteMask::None;
+        // According to the WebGPU spec, a pipeline cannot have a fragment stage with zero color
+        // targets. This situation can arise in Filament during depth-only passes (like shadow map
+        // generation) if the material variant still includes a fragment shader.
+        //
+        // To handle this, we check if any color targets are configured for this pipeline. If not, we
+        // create a pipeline *without* a fragment stage. This makes the pipeline valid for a
+        // depth-only pass, allowing depth writes to proceed correctly.
+        if (!colorFormats.empty()) {
+            fragmentState.module = program.fragmentShaderModule;
+            fragmentState.entryPoint = "main";
+            // see the comment about constants for the vertex state, as the same reasoning applies
+            // here
+            fragmentState.constantCount = 0,
+            fragmentState.constants = nullptr,
+            fragmentState.targetCount = colorFormats.size();
+            fragmentState.targets = colorTargets.data();
+            assert_invariant(fragmentState.targetCount <= MRT::MAX_SUPPORTED_RENDER_TARGET_COUNT);
+            for (size_t targetIndex = 0; targetIndex < fragmentState.targetCount; targetIndex++) {
+                auto& colorTarget = colorTargets[targetIndex];
+                colorTarget.format = colorFormats[targetIndex];
+                colorTarget.blend = rasterState.hasBlending() ? &blendState : nullptr;
+                colorTarget.writeMask =
+                        rasterState.colorWrite ? wgpu::ColorWriteMask::All : wgpu::ColorWriteMask::None;
+            }
+            pipelineDescriptor.fragment = &fragmentState;
         }
-        pipelineDescriptor.fragment = &fragmentState;
     }
     const wgpu::RenderPipeline pipeline = device.CreateRenderPipeline(&pipelineDescriptor);
     FILAMENT_CHECK_POSTCONDITION(pipeline)

filament/src/RenderPass.cpp

@@ -82,8 +82,6 @@ RenderPassBuilder& RenderPassBuilder::customCommand(
 
 RenderPass RenderPassBuilder::build(FEngine const& engine, DriverApi& driver) const {
     assert_invariant(mRenderableSoa);
-    assert_invariant(mScissorViewport.width  <= std::numeric_limits<int32_t>::max());
-    assert_invariant(mScissorViewport.height <= std::numeric_limits<int32_t>::max());
     return RenderPass{ engine, driver, *this };
 }
 
@@ -107,8 +105,7 @@ void RenderPass::DescriptorSetHandleDeleter::operator()(
 RenderPass::RenderPass(FEngine const& engine, DriverApi& driver,
         RenderPassBuilder const& builder) noexcept
         : mRenderableSoa(*builder.mRenderableSoa),
-          mColorPassDescriptorSet(builder.mColorPassDescriptorSet),
-          mScissorViewport(builder.mScissorViewport) {
+          mColorPassDescriptorSet(builder.mColorPassDescriptorSet) {
 
     // compute the number of commands we need
     updateSummedPrimitiveCounts(

filament/src/RenderPass.h

@@ -306,6 +306,12 @@ public:
     // allocated commands ARE NOT freed, they're owned by the Arena
     ~RenderPass() noexcept;
 
+    // Specifies the viewport for the scissor rectangle, that is, the final scissor rect is
+    // offset by the viewport's left-top and clipped to the viewport's width/height.
+    void setScissorViewport(backend::Viewport const viewport) noexcept {
+        mScissorViewport = viewport;
+    }
+
     Command const* begin() const noexcept { return mCommandBegin; }
     Command const* end() const noexcept { return mCommandEnd; }
     bool empty() const noexcept { return begin() == end(); }
@@ -461,7 +467,7 @@ private:
 
     FScene::RenderableSoa const& mRenderableSoa;
     ColorPassDescriptorSet const* const mColorPassDescriptorSet;
-    backend::Viewport const mScissorViewport{ 0, 0, INT32_MAX, INT32_MAX };
+    backend::Viewport mScissorViewport{ 0, 0, INT32_MAX, INT32_MAX };
     Command const* /* const */ mCommandBegin = nullptr;   // Pointer to the first command
     Command const* /* const */ mCommandEnd = nullptr;     // Pointer to one past the last command
     mutable BufferObjectSharedHandle mInstancedUboHandle; // ubo for instanced primitives
@@ -476,7 +482,6 @@ class RenderPassBuilder {
 
     RenderPass::Arena& mArena;
     RenderPass::CommandTypeFlags mCommandTypeFlags{};
-    backend::Viewport mScissorViewport{ 0, 0, INT32_MAX, INT32_MAX };
     FScene::RenderableSoa const* mRenderableSoa = nullptr;
     utils::Range<uint32_t> mVisibleRenderables{};
     math::float3 mCameraPosition{};
@@ -507,13 +512,6 @@ public:
         return *this;
     }
 
-    // Specifies the viewport for the scissor rectangle, that is, the final scissor rect is
-    // offset by the viewport's left-top and clipped to the viewport's width/height.
-    RenderPassBuilder& scissorViewport(backend::Viewport const viewport) noexcept {
-        mScissorViewport = viewport;
-        return *this;
-    }
-
     // specifies the geometry to generate commands for
     RenderPassBuilder& geometry(
             FScene::RenderableSoa const& soa, utils::Range<uint32_t> const vr) noexcept {

filament/src/RendererUtils.cpp

@@ -40,7 +40,6 @@
 #include <utils/Panic.h>
 
 #include <algorithm>
-#include <optional>
 #include <utility>
 
 #include <stddef.h>
@@ -270,87 +269,94 @@ RendererUtils::ColorPassOutput RendererUtils::colorPass(
     };
 }
 
-std::optional<RendererUtils::ColorPassOutput> RendererUtils::refractionPass(
+
+RenderPass::Command const* RendererUtils::getFirstRefractionCommand(
+    RenderPass const& pass) noexcept {
+
+    // find the first refractive object in channel 2
+    RenderPass::Command const* const refraction = std::partition_point(pass.begin(), pass.end(),
+        [](auto const& command) {
+            constexpr uint64_t mask  = RenderPass::CHANNEL_MASK | RenderPass::PASS_MASK;
+            constexpr uint64_t channel = uint64_t(RenderableManager::Builder::DEFAULT_CHANNEL) << RenderPass::CHANNEL_SHIFT;
+            constexpr uint64_t value = channel | uint64_t(RenderPass::Pass::REFRACT);
+            return (command.key & mask) < value;
+        });
+
+    const bool hasScreenSpaceRefraction =
+            (refraction->key & RenderPass::PASS_MASK) == uint64_t(RenderPass::Pass::REFRACT);
+
+    return hasScreenSpaceRefraction ? refraction : nullptr;
+
+}
+
+RendererUtils::ColorPassOutput RendererUtils::refractionPass(
         FrameGraph& fg, FEngine& engine, FView const& view,
         ColorPassInput colorPassInput,
         ColorPassConfig config,
         PostProcessManager::ScreenSpaceRefConfig const& ssrConfig,
         PostProcessManager::ColorGradingConfig const colorGradingConfig,
-        RenderPass const& pass) noexcept {
+        RenderPass const& pass, RenderPass::Command const* const firstRefractionCommand) noexcept {
 
-    // find the first refractive object in channel 2
-    RenderPass::Command const* const refraction = std::partition_point(pass.begin(), pass.end(),
-            [](auto const& command) {
-                constexpr uint64_t mask  = RenderPass::CHANNEL_MASK | RenderPass::PASS_MASK;
-                constexpr uint64_t channel = uint64_t(RenderableManager::Builder::DEFAULT_CHANNEL) << RenderPass::CHANNEL_SHIFT;
-                constexpr uint64_t value = channel | uint64_t(RenderPass::Pass::REFRACT);
-                return (command.key & mask) < value;
-            });
-
-    const bool hasScreenSpaceRefraction =
-            (refraction->key & RenderPass::PASS_MASK) == uint64_t(RenderPass::Pass::REFRACT);
+    assert_invariant(firstRefractionCommand);
+    RenderPass::Command const* const refraction = firstRefractionCommand;
 
     // if there wasn't any refractive object, just skip everything below.
-    if (UTILS_UNLIKELY(hasScreenSpaceRefraction)) {
-        assert_invariant(!colorPassInput.linearColor);
-        assert_invariant(!colorPassInput.depth);
-        config.hasScreenSpaceReflectionsOrRefractions = true;
+    assert_invariant(!colorPassInput.linearColor);
+    assert_invariant(!colorPassInput.depth);
+    config.hasScreenSpaceReflectionsOrRefractions = true;
 
-        PostProcessManager& ppm = engine.getPostProcessManager();
-        auto const opaquePassOutput = colorPass(fg,
-                "Color Pass (opaque)", engine, view, colorPassInput, {
-                        // When rendering the opaques, we need to conserve the sample buffer,
-                        // so create a config that specifies the sample count.
-                        .width = config.physicalViewport.width,
-                        .height = config.physicalViewport.height,
-                        .samples = config.msaa,
-                        .format = config.hdrFormat
-                },
-                config, { .asSubpass = false, .customResolve = false },
-                pass.getExecutor(pass.begin(), refraction));
+    PostProcessManager& ppm = engine.getPostProcessManager();
+    auto const opaquePassOutput = colorPass(fg,
+            "Color Pass (opaque)", engine, view, colorPassInput, {
+                    // When rendering the opaques, we need to conserve the sample buffer,
+                    // so create a config that specifies the sample count.
+                    .width = config.physicalViewport.width,
+                    .height = config.physicalViewport.height,
+                    .samples = config.msaa,
+                    .format = config.hdrFormat
+            },
+            config, { .asSubpass = false, .customResolve = false },
+            pass.getExecutor(pass.begin(), refraction));
 
 
-        // Generate the mipmap chain
-        // Note: we can run some post-processing effects while the "color pass" descriptor set
-        // in bound because only the descriptor 0 (frame uniforms) matters, and it's
-        // present in both.
-        PostProcessManager::generateMipmapSSR(ppm, fg,
-                opaquePassOutput.linearColor,
-                ssrConfig.refraction,
-                true, ssrConfig);
+    // Generate the mipmap chain
+    // Note: we can run some post-processing effects while the "color pass" descriptor set
+    // in bound because only the descriptor 0 (frame uniforms) matters, and it's
+    // present in both.
+    PostProcessManager::generateMipmapSSR(ppm, fg,
+            opaquePassOutput.linearColor,
+            ssrConfig.refraction,
+            true, ssrConfig);
 
-        // Now we're doing the refraction pass proper.
-        // This uses the same framebuffer (color and depth) used by the opaque pass.
-        // For this reason, the `colorBufferDesc` parameter of colorPass() below is only used  for
-        // the width and height.
-        colorPassInput.linearColor = opaquePassOutput.linearColor;
-        colorPassInput.depth = opaquePassOutput.depth;
+    // Now we're doing the refraction pass proper.
+    // This uses the same framebuffer (color and depth) used by the opaque pass.
+    // For this reason, the `colorBufferDesc` parameter of colorPass() below is only used  for
+    // the width and height.
+    colorPassInput.linearColor = opaquePassOutput.linearColor;
+    colorPassInput.depth = opaquePassOutput.depth;
 
-        // Since we're reusing the existing target we don't want to clear any of its buffer.
-        // Important: if this target ended up being an imported target, then the clearFlags
-        // specified here wouldn't apply (the clearFlags of the imported target take precedence),
-        // and we'd end up clearing the opaque pass. This scenario never happens because it is
-        // prevented in Renderer.cpp's final blit.
-        config.clearFlags = TargetBufferFlags::NONE;
-        auto transparentPassOutput = colorPass(fg, "Color Pass (transparent)",
-                engine, view, colorPassInput, {
-                        .width = config.physicalViewport.width,
-                        .height = config.physicalViewport.height },
-                config, colorGradingConfig,
-                pass.getExecutor(refraction, pass.end()));
+    // Since we're reusing the existing target we don't want to clear any of its buffer.
+    // Important: if this target ended up being an imported target, then the clearFlags
+    // specified here wouldn't apply (the clearFlags of the imported target take precedence),
+    // and we'd end up clearing the opaque pass. This scenario never happens because it is
+    // prevented in Renderer.cpp's final blit.
+    config.clearFlags = TargetBufferFlags::NONE;
+    auto transparentPassOutput = colorPass(fg, "Color Pass (transparent)",
+            engine, view, colorPassInput, {
+                    .width = config.physicalViewport.width,
+                    .height = config.physicalViewport.height },
+            config, colorGradingConfig,
+            pass.getExecutor(refraction, pass.end()));
 
-        if (config.msaa > 1 && !colorGradingConfig.asSubpass) {
-            // We need to do a resolve here because later passes (such as color grading or DoF) will
-            // need to sample from 'output'. However, because we have MSAA, we know we're not
-            // sampleable. And this is because in the SSR case, we had to use a renderbuffer to
-            // conserve the multi-sample buffer.
-            transparentPassOutput.linearColor = ppm.resolve(fg, "Resolved Color Buffer",
-                    transparentPassOutput.linearColor, { .levels = 1 });
-        }
-        return transparentPassOutput;
+    if (config.msaa > 1 && !colorGradingConfig.asSubpass) {
+        // We need to do a resolve here because later passes (such as color grading or DoF) will
+        // need to sample from 'output'. However, because we have MSAA, we know we're not
+        // sampleable. And this is because in the SSR case, we had to use a renderbuffer to
+        // conserve the multi-sample buffer.
+        transparentPassOutput.linearColor = ppm.resolve(fg, "Resolved Color Buffer",
+                transparentPassOutput.linearColor, { .levels = 1 });
     }
-
-    return std::nullopt;
+    return transparentPassOutput;
 }
 
 UTILS_NOINLINE

filament/src/RendererUtils.h

@@ -26,18 +26,19 @@
 #include <filament/Viewport.h>
 
 #include <backend/DriverEnums.h>
-#include <backend/PixelBufferDescriptor.h>
+#include <backend/Handle.h>
 
 #include <math/vec2.h>
 #include <math/vec4.h>
 
 #include <stdint.h>
 
-#include <optional>
-#include <utility>
-
 namespace filament {
 
+namespace backend {
+class PixelBufferDescriptor;
+}
+
 class FRenderTarget;
 class FrameGraph;
 class FrameGraph;
@@ -99,18 +100,20 @@ public:
             PostProcessManager::ColorGradingConfig colorGradingConfig,
             RenderPass::Executor passExecutor) noexcept;
 
-    static std::optional<ColorPassOutput> refractionPass(
+    static ColorPassOutput refractionPass(
             FrameGraph& fg, FEngine& engine, FView const& view,
             ColorPassInput colorPassInput,
             ColorPassConfig config,
             PostProcessManager::ScreenSpaceRefConfig const& ssrConfig,
             PostProcessManager::ColorGradingConfig colorGradingConfig,
-            RenderPass const& pass) noexcept;
+            RenderPass const& pass, RenderPass::Command const* firstRefractionCommand) noexcept;
 
     static void readPixels(backend::DriverApi& driver,
             backend::Handle<backend::HwRenderTarget> renderTargetHandle,
             uint32_t xoffset, uint32_t yoffset, uint32_t width, uint32_t height,
             backend::PixelBufferDescriptor&& buffer);
+
+    static RenderPass::Command const* getFirstRefractionCommand(RenderPass const& pass) noexcept;
 };
 
 } // namespace filament

filament/src/details/Renderer.cpp

@@ -1125,16 +1125,6 @@ void FRenderer::renderJob(RootArenaScope& rootArenaScope, FView& view) {
     // --------------------------------------------------------------------------------------------
     // Color passes
 
-    // this makes the viewport relative to xvp
-    // FIXME: we should use 'vp' when rendering directly into the swapchain, but that's hard to
-    //        know at this point. This will usually be the case when post-process is disabled.
-    // FIXME: we probably should take the dynamic scaling into account too
-    // if MSAA is enabled, we end-up rendering in an intermediate buffer. This is the only case where
-    // "!hasPostProcess" doesn't guarantee rendering into the swapchain.
-    const bool useIntermediateBuffer = hasPostProcess || msaaOptions.enabled ||
-          (isRenderingMultiview && engine.debug.stereo.combine_multiview_images);
-    passBuilder.scissorViewport(useIntermediateBuffer ? xvp : vp);
-
     // This one doesn't need to be a FrameGraph pass because it always happens by construction
     // (i.e. it won't be culled, unless everything is culled), so no need to complexify things.
     passBuilder.variant(variant);
@@ -1174,8 +1164,35 @@ void FRenderer::renderJob(RootArenaScope& rootArenaScope, FView& view) {
         passBuilder.renderFlags(renderFlags);
     }
 
+    // create the pass, which generates all its commands (this is a heavy operation)
     RenderPass const pass{ passBuilder.build(engine, driver) };
 
+    // now that we have the commands we can figure out if we have refraction commands
+    auto* const firstRefractionCommand = [&view](RenderPass const& pass) {
+        RenderPass::Command const* p = nullptr;
+        if (UTILS_UNLIKELY(view.isScreenSpaceRefractionEnabled() && !pass.empty())) {
+            p = RendererUtils::getFirstRefractionCommand(pass);
+        }
+        return p;
+    }(pass);
+
+    hasScreenSpaceRefraction = firstRefractionCommand != nullptr;
+
+    // this makes the viewport relative to xvp
+    // FIXME: we should use 'vp' when rendering directly into the swapchain, but that's hard to
+    //        know at this point. This will usually be the case when post-process is disabled.
+    // FIXME: we probably should take the dynamic scaling into account too
+    // if MSAA is enabled, we end-up rendering in an intermediate buffer. This is the only case where
+    // "!hasPostProcess" doesn't guarantee rendering into the swapchain.
+    const bool useIntermediateBuffer = hasPostProcess || msaaOptions.enabled ||
+            ssReflectionsOptions.enabled || hasScreenSpaceRefraction ||
+            (isRenderingMultiview && engine.debug.stereo.
+            combine_multiview_images);
+
+    // this is slightly ugly, but conceptually `pass` is const; it's just that we can't set
+    // the scissor viewport during construction
+    const_cast<RenderPass&>(pass).setScissorViewport(useIntermediateBuffer ? xvp : vp);
+
     FrameGraphTexture::Descriptor colorBufferDesc = {
             .width = config.physicalViewport.width,
             .height = config.physicalViewport.height,
@@ -1220,21 +1237,17 @@ void FRenderer::renderJob(RootArenaScope& rootArenaScope, FView& view) {
             },
             colorBufferDesc, config, colorGradingConfigForColor, pass.getExecutor());
 
-    if (view.isScreenSpaceRefractionEnabled() && !pass.empty()) {
+    if (UTILS_UNLIKELY(hasScreenSpaceRefraction)) {
         // This cancels the colorPass() call above if refraction is active.
         // The color pass + refraction + color-grading as subpass if needed
-        auto const output = RendererUtils::refractionPass(fg, mEngine, view, {
+        colorPassOutput = RendererUtils::refractionPass(fg, mEngine, view, {
                         .shadows = blackboard.get<FrameGraphTexture>("shadows"),
                         .ssao = blackboard.get<FrameGraphTexture>("ssao"),
                         .ssr = ssrConfig.ssr,
                         .structure = structure
                 },
-                config, ssrConfig, colorGradingConfigForColor, pass);
-
-        hasScreenSpaceRefraction = output.has_value();
-        if (hasScreenSpaceRefraction) {
-            colorPassOutput = output.value();
-        }
+                config, ssrConfig, colorGradingConfigForColor,
+                pass, firstRefractionCommand);
     }
 
     if (colorGradingConfig.customResolve) {

test/renderdiff/src/compare.py

@@ -14,15 +14,12 @@ def _compare_goldens(base_dir, comparison_dir, out_dir=None):
                   for f in all_files)
   all_results = []
   for test_dir in test_dirs:
-    results_meta = {}
     results = []
-    output_test_dir = None if not out_dir else os.path.join(out_dir, test_dir)
+    output_test_dir = None if not out_dir else os.path.abspath(os.path.join(out_dir, test_dir))
     if output_test_dir:
       mkdir_p(output_test_dir)
     base_test_dir = os.path.abspath(os.path.join(base_dir, test_dir))
     comp_test_dir = os.path.abspath(os.path.join(comparison_dir, test_dir))
-    results_meta['base_dir'] = base_test_dir
-    results_meta['comparison_dir'] = comp_test_dir
     for golden_file in \
         glob.glob(os.path.join(base_test_dir, "*.tif")):
       base_fname = os.path.abspath(golden_file)
@@ -45,8 +42,12 @@ def _compare_goldens(base_dir, comparison_dir, out_dir=None):
         result['result'] = RESULT_OK
       results.append(result)
     if output_test_dir:
-      results_meta['results'] = results
       output_fname = os.path.join(output_test_dir, "compare_results.json")
+      results_meta = {
+        'results': results,
+        'base_dir': os.path.relpath(output_fname, base_test_dir),
+        'comparison_dir': os.path.relpath(output_fname, comp_test_dir),
+      }
       with open(output_fname, 'w') as f:
         f.write(json.dumps(results_meta, indent=2))
       important_print(f'Written comparison results for {test_dir} to \n    {output_fname}')
@@ -64,7 +65,7 @@ if __name__ == '__main__':
   dest = args.dest
   if not dest:
     print('Assume the default renderdiff output folder')
-    dest = os.path.join(os.getcwd(), './out/renderdiff_tests')
+    dest = os.path.join(os.getcwd(), './out/renderdiff')
   assert os.path.exists(dest), f"Destination folder={dest} does not exist."
 
   results = _compare_goldens(args.src, dest, out_dir=args.out)

test/renderdiff/src/viewer.py

@@ -17,6 +17,9 @@ import sys
 import flask
 import pathlib
 import json
+import requests
+import io
+import zipfile
 
 from utils import ArgParseImpl
 
@@ -25,14 +28,163 @@ from flask import Flask, request, make_response, send_from_directory
 DIR = pathlib.Path(__file__).parent.absolute()
 HTML_DIR = os.path.join(DIR, "viewer_html")
 
+# Generated by gemini
+def _download_github_artifacts(pr_number, github_token, output_dir= ".") -> None:
+  """
+  Downloads artifacts associated with a specific GitHub Pull Request.
+
+  This function performs the following steps:
+  1. Fetches the details of the Pull Request to get its head commit SHA.
+  2. Searches for GitHub Actions workflow runs triggered by that specific commit.
+  3. Iterates through successful workflow runs to find and list all associated artifacts.
+  4. Downloads each artifact (which comes as a ZIP file).
+  5. Extracts the contents of each downloaded ZIP file into a unique subdirectory
+  within the specified output directory.
+
+  Args:
+    owner (str): The GitHub repository owner (e.g., "octocat").
+    repo (str): The GitHub repository name (e.g., "Spoon-Knife").
+    pr_number (int): The Pull Request number.
+    output_dir (str): The local directory where downloaded artifacts will be saved.
+            Defaults to the current directory.
+  """
+
+  # Prepare HTTP headers for GitHub API requests
+  headers = {"Accept": "application/vnd.github.v3+json"}
+  if github_token:
+    headers["Authorization"] = f"token {github_token}"
+
+  OWNER_REPO = 'google/filament'
+
+  # --- Step 1: Get PR details to find the head commit SHA ---
+  print(f"Fetching details for PR #{pr_number} in {OWNER_REPO}...")
+  pr_url = f"https://api.github.com/repos/{OWNER_REPO}/pulls/{pr_number}"
+  try:
+    response = requests.get(pr_url, headers=headers)
+    response.raise_for_status() # Raise an exception for HTTP errors (4xx or 5xx)
+    pr_data = response.json()
+    commit_sha = pr_data["head"]["sha"]
+    print(f"PR #{pr_number} is associated with commit SHA: {commit_sha}")
+  except requests.exceptions.HTTPError as e:
+    if e.response.status_code == 404:
+      print(f"Error: PR #{pr_number} not found in {OWNER_REPO}. Please check the PR number, owner, and repository name.")
+    elif e.response.status_code == 403:
+      print(f"Error: Access forbidden to PR #{pr_number}. You might be hitting API rate limits or need a valid GitHub Token.")
+    else:
+      print(f"An HTTP error occurred while fetching PR details: {e}")
+    return # Exit function on error
+  except requests.exceptions.RequestException as e:
+    print(f"A network error occurred while fetching PR details: {e}")
+    return # Exit function on error
+
+  # --- Step 2: Find workflow runs associated with the commit SHA ---
+  print(f"Searching for workflow runs for commit SHA: {commit_sha}...")
+  workflow_runs_url = f"https://api.github.com/repos/{OWNER_REPO}/actions/runs"
+  # Filter by head_sha and event='pull_request' for precision
+  params = {"head_sha": commit_sha, "event": "pull_request"}
+  try:
+    response = requests.get(workflow_runs_url, headers=headers, params=params)
+    response.raise_for_status()
+    runs_data = response.json()
+    workflow_runs = runs_data.get("workflow_runs", [])
+
+    if not workflow_runs:
+      print(f"No workflow runs found directly associated with PR #{pr_number} (commit SHA: {commit_sha}).")
+      print("This might happen if the workflow was triggered by a push after the PR was opened,")
+      print("or if the PR head branch was updated without triggering a new workflow run with this exact SHA.")
+      print("Consider checking GitHub Actions runs manually for this PR's branch on GitHub.")
+      return None
+
+    # Filter for runs that completed successfully
+    successful_runs = [run for run in workflow_runs if run.get("conclusion") == "success" and run.get("status") == "completed"]
+    if not successful_runs:
+      print(f"No *successful and completed* workflow runs found for PR #{pr_number} with commit SHA {commit_sha}. Exiting.")
+      return None
+
+  except requests.exceptions.HTTPError as e:
+    print(f"An HTTP error occurred while searching for workflow runs: {e}")
+    return None
+  except requests.exceptions.RequestException as e:
+    print(f"A network error occurred while searching for workflow runs: {e}")
+    return None
+
+  # Create the main output directory if it doesn't exist
+  os.makedirs(output_dir, exist_ok=True)
+  print(f"Ensuring output directory exists: {os.path.abspath(output_dir)}")
+
+  downloaded_any_artifact = False # Flag to track if any artifact was downloaded
+
+  # --- Step 3 & 4: List and Download Artifacts for each successful run ---
+  for run in successful_runs:
+    run_id = run["id"]
+    run_name = run["name"]
+    print(f"\nProcessing workflow run '{run_name}' (ID: {run_id})...")
+
+    artifacts_url = f"https://api.github.com/repos/{OWNER_REPO}/actions/runs/{run_id}/artifacts"
+    try:
+      response = requests.get(artifacts_url, headers=headers)
+      response.raise_for_status()
+      artifacts_data = response.json()
+      artifacts = artifacts_data.get("artifacts", [])
+
+      if not artifacts:
+        print(f"  No artifacts found for workflow run ID {run_id}.")
+        continue # Move to the next workflow run
+
+      for artifact in artifacts:
+        artifact_id = artifact["id"]
+        artifact_name = artifact["name"]
+        archive_download_url = artifact["archive_download_url"]
+
+        print(f"  Found artifact: '{artifact_name}' (ID: {artifact_id})")
+
+        # Perform the download request
+        print(f"  Downloading '{artifact_name}'...")
+        # Use a copy of headers and specific Accept for ZIP download
+        download_headers = headers.copy()
+        download_headers["Accept"] = "application/vnd.github.v3+zip"
+        download_response = requests.get(archive_download_url, headers=download_headers, stream=True)
+        download_response.raise_for_status() # Check for errors in download
+
+        # --- Step 5: Extract the contents ---
+        # Use BytesIO to handle the zip file content in memory without saving to a temporary file
+        with io.BytesIO(download_response.content) as zip_buffer:
+          try:
+            with zipfile.ZipFile(zip_buffer, 'r') as zip_ref:
+              # Create a unique subdirectory for each artifact to avoid file name conflicts
+              extract_path = os.path.join(output_dir, f"{artifact_name}_{artifact_id}")
+              os.makedirs(extract_path, exist_ok=True)
+              zip_ref.extractall(extract_path)
+              print(f"  Successfully extracted '{artifact_name}' to '{extract_path}/'")
+              downloaded_any_artifact = True
+          except zipfile.BadZipFile:
+            print(f"  Error: Downloaded file for '{artifact_name}' is not a valid zip file. Skipping extraction.")
+          except Exception as e:
+            print(f"  An error occurred during extraction of '{artifact_name}': {e}")
+    except requests.exceptions.HTTPError as e:
+      print(f"  An HTTP error occurred while fetching artifacts for run {run_id}: {e}")
+      if e.response.status_code == 403:
+        print("  This often means you need a GitHub Personal Access Token with 'repo' scope (even for public repos for artifact downloads).")
+      continue # Continue processing the next workflow run
+    except requests.exceptions.RequestException as e:
+      print(f"  A network error occurred while fetching artifacts for run {run_id}: {e}")
+      continue # Continue processing the next workflow run
+
+  if not downloaded_any_artifact:
+    print("\nNo artifacts were downloaded for the specified PR.")
+  else:
+    print("\nAll available artifacts have been processed.")
+  return 'Done'
+
 def _create_app(config):
   app = Flask(__name__)
 
   client_config = config.copy()
-  base_dir = client_config['base_dir']
-  comparison_dir = client_config['comparison_dir']
   diff_dir = client_config['diff_dir']
 
+  base_dir = os.path.join(diff_dir, client_config['base_dir'])
+  comparison_dir = os.path.join(diff_dir, client_config['comparison_dir'])
+
   del client_config['base_dir']
   del client_config['comparison_dir']
   del client_config['diff_dir']
@@ -67,12 +219,37 @@ def _create_app(config):
 if __name__ == '__main__':
   PORT = 8901
   parser = ArgParseImpl()
-  parser.add_argument('--diff', help='Diff directory', required=True)
+  parser.add_argument('--diff', type=str, help='Diff result directory')
+  parser.add_argument('--pr_number', type=str, help='Pull request artifacts to examine')
+  parser.add_argument('--github_token', type=str, help='Necessary for pull PR artifacts')
   args, _ = parser.parse_known_args(sys.argv[1:])
 
-  with open(os.path.join(args.diff, 'compare_results.json'), 'r') as f:
+  if not args.diff and not args.pr_number:
+    print('Need to specify either a diff result directory or a Pull Request number')
+    exit(1)
+
+  if args.diff and args.pr_number:
+    print('Cannot specify both a diff result directory and a Pull Request number')
+    exit(1)
+
+  fdir = args.diff
+  if args.pr_number:
+    if not args.github_token:
+      print('Must provide --github_token to be able to download artifacts')
+      exit(1)
+    output_dir = f'/tmp/filament-pr{args.pr_number}-rdiff-result'
+    res = _download_github_artifacts(args.pr_number, args.github_token, output_dir)
+    if not res:
+      print('Failed to retrieve PR artifacts')
+      exit(1)
+
+    # TODO: Clean up the following so that we're not so specific on the paths diffs/presubmit
+    directory_name = list(os.listdir(output_dir))[0]
+    fdir = os.path.join(os.path.join(output_dir, directory_name), 'diffs/presubmit')
+
+  with open(os.path.join(fdir, 'compare_results.json'), 'r') as f:
     config = json.loads(f.read())
-  config['diff_dir'] = os.path.abspath(args.diff)
+  config['diff_dir'] = os.path.abspath(fdir)
 
   app = _create_app(config)
   from waitress import serve