ronantakizawa/codereview-bench huggingface dataset:
Repository: google/filament
File: filament/backend/test/BackendTest.h (C/C++)

Code before:
    void renderTriangle(
            filament::backend::PipelineLayout const& pipelineLayout,
            filament::backend::Handle<filament::backend::HwRenderTarget> renderTarget,
            filament::backend::Handle<filament::backend::HwSwapChain> swapChain,
            filament::backend::Handle<filament::backend::HwProgram> program,
            const filament::backend::RenderPassParams& params);

    filament::backend::DriverApi& getDriverApi() { return *commandStream; }
    filament::backend::Driver& getDriver() { return *driver; }

    ImageExpectations& getExpectations() { return *mImageExpectations; }

    static bool matchesEnvironment(Backend backend);
    static bool matchesEnvironment(OperatingSystem operatingSystem);
    static bool matchesEnvironment(OperatingSystem operatingSystem, Backend backend);
private:

    filament::backend::Driver* driver = nullptr;
    filament::backend::CommandBufferQueue commandBufferQueue;
    std::unique_ptr<filament::backend::DriverApi> commandStream;

    filament::backend::Handle<filament::backend::HwBufferObject> uniform;

    std::optional<ImageExpectations> mImageExpectations;
};

} // namespace test

#endif


Review comment (refactor):
This is optional to simplify construction order as `initializeDriver` needs to be called before this is constructed.

I can write something gunkier in the constructor, like `mImageExpectations((initializeDriver(), getDriverApi()))` but it obfuscates that work is being done there so I'd prefer this.

Code after:
    void renderTriangle(
            filament::backend::PipelineLayout const& pipelineLayout,
            filament::backend::Handle<filament::backend::HwRenderTarget> renderTarget,
            filament::backend::Handle<filament::backend::HwSwapChain> swapChain,
            filament::backend::Handle<filament::backend::HwProgram> program,
            const filament::backend::RenderPassParams& params);

    filament::backend::DriverApi& getDriverApi() { return *commandStream; }
    filament::backend::Driver& getDriver() { return *driver; }

    ImageExpectations& getExpectations() { return *mImageExpectations; }

    static bool matchesEnvironment(Backend backend);
    static bool matchesEnvironment(OperatingSystem operatingSystem);
    static bool matchesEnvironment(OperatingSystem operatingSystem, Backend backend);
private:

    filament::backend::Driver* driver = nullptr;
    filament::backend::CommandBufferQueue commandBufferQueue;
    std::unique_ptr<filament::backend::DriverApi> commandStream;

    filament::backend::Handle<filament::backend::HwBufferObject> uniform;

    // This isn't truly optional, it just needs to delay construction until after the driver has
    // been initialized
    std::optional<ImageExpectations> mImageExpectations;
};

} // namespace test

#endif
Repository: google/filament
File: filament/backend/src/webgpu/WebGPUSwapChain.h (C/C++)

Code before:
    void present();

private:

    void setExtent(wgpu::Extent2D const&);

    wgpu::Device mDevice = nullptr;
    enum class SwapChainType {
        HEADLESS,
        SURFACE
    };

    wgpu::Surface mSurface = {};
    wgpu::SurfaceConfiguration mConfig = {};
    bool mNeedStencil = false;
    wgpu::TextureFormat mDepthFormat = wgpu::TextureFormat::Undefined;
    wgpu::Texture mDepthTexture = nullptr;
    wgpu::TextureView mDepthTextureView = nullptr;
    const SwapChainType mType;
    const uint32_t mHeadlessWidth;
    const uint32_t mHeadlessHeight;

    ///TODO: eventually config for double or triple buffering
    static constexpr uint32_t mHeadlessBufferCount = 3;
    uint32_t mHeadlessBufferIndex = 0;
    std::array<wgpu::Texture, 3> mRenderTargetTextures;
    std::array<wgpu::TextureView, 3> mRenderTargetViews;
};

} // namespace filament::backend

#endif //TNT_FILAMENT_BACKEND_WEBGPUSWAPCHAIN_H


Review comment (suggestion):
you should reference the `mHeadlessBufferCount` instead of hardcoding `3`, as they can easily get out of sync. Since you made `mHeadlessBufferCount` `constexpr` you can do this; the compiler has the info at compile time _(this is one of many examples of the advantage of using `constexpr` when you can)_.

Code after:
    void present();

private:

    void setExtent(wgpu::Extent2D const&);

    wgpu::Device mDevice = nullptr;
    enum class SwapChainType {
        HEADLESS,
        SURFACE
    };

    wgpu::Surface mSurface = {};
    wgpu::SurfaceConfiguration mConfig = {};
    bool mNeedStencil = false;
    wgpu::TextureFormat mDepthFormat = wgpu::TextureFormat::Undefined;
    wgpu::Texture mDepthTexture = nullptr;
    wgpu::TextureView mDepthTextureView = nullptr;
    const SwapChainType mType;
    const uint32_t mHeadlessWidth;
    const uint32_t mHeadlessHeight;

    static constexpr uint32_t mHeadlessBufferCount = 3;
    uint8_t mHeadlessBufferIndex = 0;
    std::array<wgpu::Texture, mHeadlessBufferCount> mRenderTargetTextures;
    std::array<wgpu::TextureView, mHeadlessBufferCount> mRenderTargetViews;
};

} // namespace filament::backend

#endif //TNT_FILAMENT_BACKEND_WEBGPUSWAPCHAIN_H
Repository: google/filament
File: filament/backend/src/webgpu/WebGPUSwapChain.h (C/C++)

Code before:

private:

    void setExtent(wgpu::Extent2D const&);

    wgpu::Device mDevice = nullptr;
    enum class SwapChainType {
        HEADLESS,
        SURFACE
    };

    wgpu::Surface mSurface = {};
    wgpu::SurfaceConfiguration mConfig = {};
    bool mNeedStencil = false;
    wgpu::TextureFormat mDepthFormat = wgpu::TextureFormat::Undefined;
    wgpu::Texture mDepthTexture = nullptr;
    wgpu::TextureView mDepthTextureView = nullptr;
    const SwapChainType mType;
    const uint32_t mHeadlessWidth;
    const uint32_t mHeadlessHeight;

    ///TODO: eventually config for double or triple buffering
    static constexpr uint32_t mHeadlessBufferCount = 3;
    uint32_t mHeadlessBufferIndex = 0;
    std::array<wgpu::Texture, 3> mRenderTargetTextures;
    std::array<wgpu::TextureView, 3> mRenderTargetViews;
};

} // namespace filament::backend

#endif //TNT_FILAMENT_BACKEND_WEBGPUSWAPCHAIN_H


Review comment (suggestion):
These arrays that may or may not be needed at runtime... in fact, not at all used in a release build with the typical present-to-screen use-case provides a hint of actually not conflating headless with "normal" swapchain modes in the same class (efficiency). We don't need to address this now (in face, should not in this PR... one thing at a time), but something to consider in a subsequent PR (maybe we can create a tech debt ticket for it). Off-the-top-of-my head I initially imagine a common interface that both the surface and headless implementations implement.

Code after:

private:

    void setExtent(wgpu::Extent2D const&);

    wgpu::Device mDevice = nullptr;
    enum class SwapChainType {
        HEADLESS,
        SURFACE
    };

    wgpu::Surface mSurface = {};
    wgpu::SurfaceConfiguration mConfig = {};
    bool mNeedStencil = false;
    wgpu::TextureFormat mDepthFormat = wgpu::TextureFormat::Undefined;
    wgpu::Texture mDepthTexture = nullptr;
    wgpu::TextureView mDepthTextureView = nullptr;
    const SwapChainType mType;
    const uint32_t mHeadlessWidth;
    const uint32_t mHeadlessHeight;

    static constexpr uint32_t mHeadlessBufferCount = 3;
    uint8_t mHeadlessBufferIndex = 0;
    std::array<wgpu::Texture, mHeadlessBufferCount> mRenderTargetTextures;
    std::array<wgpu::TextureView, mHeadlessBufferCount> mRenderTargetViews;
};

} // namespace filament::backend

#endif //TNT_FILAMENT_BACKEND_WEBGPUSWAPCHAIN_H
Repository: google/filament
File: filament/backend/src/webgpu/WebGPUSwapChain.h (C/C++)

Code before:
 * limitations under the License.
 */

#ifndef TNT_FILAMENT_BACKEND_WEBGPUSWAPCHAIN_H
#define TNT_FILAMENT_BACKEND_WEBGPUSWAPCHAIN_H

#include <webgpu/webgpu_cpp.h>

#include "DriverBase.h"
#include <backend/Platform.h>

#include <cstdint>

namespace filament::backend {

class WebGPUSwapChain final : public Platform::SwapChain, HwSwapChain {
public:
    WebGPUSwapChain(wgpu::Surface&& surface, wgpu::Extent2D const& surfaceSize,
            wgpu::Adapter const& adapter, wgpu::Device const& device, uint64_t flags);
    ~WebGPUSwapChain();

    [[nodiscard]] wgpu::TextureFormat getColorFormat() const { return mConfig.format; }

    [[nodiscard]] wgpu::TextureFormat getDepthFormat() const { return mDepthFormat; }

    [[nodiscard]] bool getNeedStencil() const { return mNeedStencil; }

    [[nodiscard]] wgpu::TextureView getCurrentSurfaceTextureView(wgpu::Extent2D const&);

    [[nodiscard]] wgpu::TextureView getDepthTextureView() const { return mDepthTextureView; }

    void present();

private:
    void setExtent(wgpu::Extent2D const&);

    wgpu::Device mDevice = nullptr;
    wgpu::Surface mSurface = {};
    wgpu::SurfaceConfiguration mConfig = {};
    bool mNeedStencil = false;
    wgpu::TextureFormat mDepthFormat = wgpu::TextureFormat::Undefined;
    wgpu::Texture mDepthTexture = nullptr;
    wgpu::TextureView mDepthTextureView = nullptr;
};

} // namespace filament::backend

#endif //TNT_FILAMENT_BACKEND_WEBGPUSWAPCHAIN_H


Review comment (suggestion):
This getter appears not to be used/needed. I would suggest removing it. If we kept it, I would recommend a more concise name, e.g. `needsStencil()`.

Code after:
 * limitations under the License.
 */

#ifndef TNT_FILAMENT_BACKEND_WEBGPUSWAPCHAIN_H
#define TNT_FILAMENT_BACKEND_WEBGPUSWAPCHAIN_H

#include <webgpu/webgpu_cpp.h>

#include "DriverBase.h"
#include <backend/Platform.h>

#include <cstdint>

namespace filament::backend {

class WebGPUSwapChain final : public Platform::SwapChain, HwSwapChain {
public:
    WebGPUSwapChain(wgpu::Surface&& surface, wgpu::Extent2D const& surfaceSize,
            wgpu::Adapter const& adapter, wgpu::Device const& device, uint64_t flags);
    ~WebGPUSwapChain();

    [[nodiscard]] wgpu::TextureFormat getColorFormat() const { return mConfig.format; }

    [[nodiscard]] wgpu::TextureFormat getDepthFormat() const { return mDepthFormat; }

    [[nodiscard]] wgpu::TextureView getCurrentSurfaceTextureView(wgpu::Extent2D const&);

    [[nodiscard]] wgpu::TextureView getDepthTextureView() const { return mDepthTextureView; }

    void present();

private:
    void setExtent(wgpu::Extent2D const&);

    wgpu::Device mDevice = nullptr;
    wgpu::Surface mSurface = {};
    wgpu::SurfaceConfiguration mConfig = {};
    bool mNeedStencil = false;
    wgpu::TextureFormat mDepthFormat = wgpu::TextureFormat::Undefined;
    wgpu::Texture mDepthTexture = nullptr;
    wgpu::TextureView mDepthTextureView = nullptr;
};

} // namespace filament::backend

#endif //TNT_FILAMENT_BACKEND_WEBGPUSWAPCHAIN_H
Repository: google/filament
File: filament/backend/src/webgpu/WebGPUPipelineCreation.h (C/C++)

Code before:

#include <cstdint>
#include <vector>

namespace wgpu {
class Device;
class PipelineLayout;
class RenderPipeline;
enum class TextureFormat : uint32_t;
}// namespace wgpu

namespace filament::backend {

struct PolygonOffset;
enum class PrimitiveType : uint8_t;
struct RasterState;
struct StencilState;

class WebGPUVertexBufferInfo;
class WebGPUProgram;

[[nodiscard]] wgpu::RenderPipeline createWebGPURenderPipeline(wgpu::Device const&,
        WebGPUProgram const&, WebGPUVertexBufferInfo const&, wgpu::PipelineLayout const&,
        RasterState const&, StencilState const&, PolygonOffset const&, PrimitiveType primitiveType,
        std::vector<wgpu::TextureFormat> const& colorFormats, wgpu::TextureFormat depthFormat,
        uint8_t samplesCount, bool filamentRequestedStencil);

}// namespace filament::backend

#endif// TNT_FILAMENT_BACKEND_WEBGPUPIPELINECREATION_H


Review comment (suggestion):
If we pass a flag in here about whether we need stencil bits, we should also pass a flag for whether you need depth bits (you can technically have depth only, stencil only, depth+stencil, or neither).

Code after:

#include <cstdint>
#include <vector>

namespace wgpu {
class Device;
class PipelineLayout;
class RenderPipeline;
enum class TextureFormat : uint32_t;
}// namespace wgpu

namespace filament::backend {

struct PolygonOffset;
enum class PrimitiveType : uint8_t;
struct RasterState;
struct StencilState;

class WebGPUVertexBufferInfo;
class WebGPUProgram;

[[nodiscard]] wgpu::RenderPipeline createWebGPURenderPipeline(wgpu::Device const&,
        WebGPUProgram const&, WebGPUVertexBufferInfo const&, wgpu::PipelineLayout const&,
        RasterState const&, StencilState const&, PolygonOffset const&, PrimitiveType primitiveType,
        std::vector<wgpu::TextureFormat> const& colorFormats, wgpu::TextureFormat const& depthStencilFormat,
        uint8_t samplesCount, bool requestedDepth, bool requestedStencil);

}// namespace filament::backend

#endif// TNT_FILAMENT_BACKEND_WEBGPUPIPELINECREATION_H
Repository: google/filament
File: shaders/src/surface_depth_main.fs (F#)

Code before:
#if defined(VARIANT_HAS_VSM)
layout(location = 0) out highp vec4 fragColor;
#elif defined(VARIANT_HAS_PICKING)
#   if __VERSION__ == 100
highp vec4 outPicking;
#   else
#       if MATERIAL_FEATURE_LEVEL == 0
layout(location = 0) out highp vec4 outPicking;
#       else
layout(location = 0) out highp ivec2 outPicking;
#       endif
#   endif
#else
// not color output
#endif

//------------------------------------------------------------------------------
// Depth
//
// note: VARIANT_HAS_VSM and VARIANT_HAS_PICKING are mutually exclusive
//------------------------------------------------------------------------------

highp vec2 computeDepthMomentsVSM(const highp float depth);

void main() {
    filament_lodBias = frameUniforms.lodBias;

    initObjectUniforms();

#if defined(MATERIAL_HAS_CUSTOM_DEPTH) || defined(BLEND_MODE_MASKED) || ((defined(BLEND_MODE_TRANSPARENT) || defined(BLEND_MODE_FADE)) && defined(MATERIAL_HAS_TRANSPARENT_SHADOW))
    MaterialInputs inputs;
    initMaterial(inputs);
    material(inputs);

    float alpha = inputs.baseColor.a;

Review comment (suggestion):
Since you're writing to a RG32UI color texture this should be `uvec2` no? Otherwise Metal complains. This is separate from the missing format issue which I'll look into.

Code after:
#if defined(VARIANT_HAS_VSM)
layout(location = 0) out highp vec4 fragColor;
#elif defined(VARIANT_HAS_PICKING)
#   if __VERSION__ == 100
highp vec4 outPicking;
#   else
#       if MATERIAL_FEATURE_LEVEL == 0
layout(location = 0) out highp vec4 outPicking;
#       else
layout(location = 0) out highp uvec2 outPicking;
#       endif
#   endif
#else
// not color output
#endif

//------------------------------------------------------------------------------
// Depth
//
// note: VARIANT_HAS_VSM and VARIANT_HAS_PICKING are mutually exclusive
//------------------------------------------------------------------------------

highp vec2 computeDepthMomentsVSM(const highp float depth);

void main() {
    filament_lodBias = frameUniforms.lodBias;

    initObjectUniforms();

#if defined(MATERIAL_HAS_CUSTOM_DEPTH) || defined(BLEND_MODE_MASKED) || ((defined(BLEND_MODE_TRANSPARENT) || defined(BLEND_MODE_FADE)) && defined(MATERIAL_HAS_TRANSPARENT_SHADOW))
    MaterialInputs inputs;
    initMaterial(inputs);
    material(inputs);

    float alpha = inputs.baseColor.a;
Repository: google/filament
File: filament/backend/src/vulkan/VulkanCommands.cpp (C++)

Code before:
            lastSubmit = VK_NULL_HANDLE;
        }
        dependency = pool->flush();
        hasFlushed = true;
    }

    if (hasFlushed) {
        mInjectedDependency = VK_NULL_HANDLE;
        mLastSubmit = dependency;
    }

    return true;
}

void VulkanCommands::wait() {
    // It's possible to call flush and wait at "terminate", in which case, we'll just return.
    if (!mPool && !mProtectedPool) {
        return;
    }

    FVK_SYSTRACE_CONTEXT();
    FVK_SYSTRACE_START("commands::wait");

    // It's not enough to wait on the fences of the buffers submitted.  Present calls are typically
    // submitted to this same queue. So the more correct option is to call vkQueueWaitIdle.
    vkQueueWaitIdle(mQueue);
    mPool->wait();
    if (mProtectedPool) {
        vkQueueWaitIdle(mProtectedQueue);
        mProtectedPool->wait();
    }
    FVK_SYSTRACE_END();
}

void VulkanCommands::gc() {
    FVK_SYSTRACE_CONTEXT();
    FVK_SYSTRACE_START("commands::gc");

    mPool->gc();
    if (mProtectedPool) {
        mProtectedPool->gc();
    }
    FVK_SYSTRACE_END();
}

void VulkanCommands::updateFences() {
    mPool->update();
    if (mProtectedPool) {
        mProtectedPool->update();
    }
}

Review comment (performance):
I dont think this is a good idea. For present there should be another way to wait that is not waiting on the queue to be idle, this is a performance killer.

Code after:
            // Note that the stage we're waiting on is the fragment shader stage.  This assumes
            // that the subsequent command buffer will only depend on
            //    1) fragment output of the previous command buffer
            //    2) reading/writing of buffers (i.e. UBO) of the previous command buffer
            // Restricting the wait stages will allow for vertex work to proceed (more overlapping
            // vertex/fragment work).
            pool->waitFor(lastSubmit,
                    VK_PIPELINE_STAGE_FRAGMENT_SHADER_BIT | VK_PIPELINE_STAGE_TRANSFER_BIT);
            lastSubmit = VK_NULL_HANDLE;
        }
        flushedFence = pool->getRecording().getVkFence();
        flushedFenceStatus = pool->getRecording().getFenceStatus();
        dependency = pool->flush();
        hasFlushed = true;
    }

    if (hasFlushed) {
        mInjectedDependency = VK_NULL_HANDLE;
        mLastSubmit = dependency;
        mLastFence = flushedFence;
        mLastFenceStatus = flushedFenceStatus;
    }

    return true;
}

void VulkanCommands::wait() {
    // It's possible to call flush and wait at "terminate", in which case, we'll just return.
    if (!mPool && !mProtectedPool) {
        return;
    }

    FVK_SYSTRACE_CONTEXT();
    FVK_SYSTRACE_START("commands::wait");

    mPool->wait();
    if (mProtectedPool) {
        mProtectedPool->wait();
    }
    FVK_SYSTRACE_END();
}

void VulkanCommands::gc() {
    FVK_SYSTRACE_CONTEXT();
    FVK_SYSTRACE_START("commands::gc");

    mPool->gc();
    if (mProtectedPool) {
        mProtectedPool->gc();
    }
    FVK_SYSTRACE_END();
Repository: google/filament
File: test/renderdiff/src/viewer_html/app.js (JavaScript)

Code before:
  static properties = {
    tests: {type: Array},
    dialogContent: {type: Object},
    selectedTests: {type: Array},
    comparisonContent: {type: Object},
    compareMode: {type: Boolean},

    // This is used to cache urls that are not found
    missingFile: {type: Object},
  };

  async _init() {
    const config = await ((await fetch("/r/")).json());
    config['results'] = config['results'].sort((a, b) => a.name < b.name ? -1 : (a.name > b.name ? 1 : 0));
    this.tests = config['results']
  }

  constructor() {
    super();
    this.tests = [];
    this.dialogContent = null;
    this.selectedTests = [];
    this.comparisonContent = null;
    this.compareMode = false;
    this.missingFile = {
      ['/d/undefined']: true,
    };
    this._init();

    this.addEventListener('dialog-closed', () => {
      this.dialogContent = null;
      this.comparisonContent = null;
    });

    this.addEventListener('url-hit', (ev) => {
      delete this.missingFile[ev.detail.value];
      this.missingFile = this.missingFile;
      this.requestUpdate();
    });

    this.addEventListener('url-miss', (ev) => {
      this.missingFile[ev.detail.value] = true;
      this.missingFile = this.missingFile;
      this.requestUpdate();
    });
  }

  updated(props) {
    if (props.has('dialogContent') || props.has('comparisonContent')) {
      let dialog = this.shadowRoot.querySelector("#dialog");
      dialog.open = !!this.dialogContent || !!this.comparisonContent;

Review comment (suggestion):
Maybe use `getDiffUrl` or something here for readability? I wasn't sure what the `/d/` meant at first.

Code after:
  static properties = {
    tests: {type: Array},
    dialogContent: {type: Object},
    selectedTests: {type: Array},
    comparisonContent: {type: Object},
    compareMode: {type: Boolean},

    // This is used to cache urls that are not found
    missingFile: {type: Object},
  };

  async _init() {
    const config = await ((await fetch("/r/")).json());
    config['results'] = config['results'].sort((a, b) => a.name < b.name ? -1 : (a.name > b.name ? 1 : 0));
    this.tests = config['results']
  }

  constructor() {
    super();
    this.tests = [];
    this.dialogContent = null;
    this.selectedTests = [];
    this.comparisonContent = null;
    this.compareMode = false;
    this.missingFile = {
      [getDiffUrl('undefined')]: true,
    };
    this._init();

    this.addEventListener('dialog-closed', () => {
      this.dialogContent = null;
      this.comparisonContent = null;
    });

    this.addEventListener('url-hit', (ev) => {
      delete this.missingFile[ev.detail.value];
      this.missingFile = this.missingFile;
      this.requestUpdate();
    });

    this.addEventListener('url-miss', (ev) => {
      this.missingFile[ev.detail.value] = true;
      this.missingFile = this.missingFile;
      this.requestUpdate();
    });
  }

  updated(props) {
    if (props.has('dialogContent') || props.has('comparisonContent')) {
      let dialog = this.shadowRoot.querySelector("#dialog");
      dialog.open = !!this.dialogContent || !!this.comparisonContent;