ronantakizawa/codereview-bench huggingface dataset:
Repository: google/filament
File: filament/backend/src/webgpu/WebGPUDriver.cpp (C++)

Code before:
}

void WebGPUDriver::beginFrame(int64_t monotonic_clock_ns,
        int64_t refreshIntervalNs, uint32_t frameId) {
    wgpu::CommandEncoderDescriptor commandEncoderDescriptor{};
    commandEncoderDescriptor.nextInChain = nullptr;
    commandEncoderDescriptor.label = "webgpu_command_encoder";
    mCommandEncoder = mDevice.CreateCommandEncoder(&commandEncoderDescriptor);
}

void WebGPUDriver::setFrameScheduledCallback(Handle<HwSwapChain> sch,
        CallbackHandler* handler, FrameScheduledCallback&& callback, uint64_t flags) {

}

void WebGPUDriver::setFrameCompletedCallback(Handle<HwSwapChain> sch,
        CallbackHandler* handler, utils::Invocable<void(void)>&& callback) {

}

void WebGPUDriver::setPresentationTime(int64_t monotonic_clock_ns) {
}

void WebGPUDriver::endFrame(uint32_t frameId) {
    mCommandEncoder = nullptr;
    mQueue.Submit(1, &mCommandBuffer);
    mCommandBuffer = nullptr;
    mTextureView = nullptr;
    mSwapChain->Present();
}

void WebGPUDriver::flush(int) {
}

void WebGPUDriver::finish(int) {
}

void WebGPUDriver::destroyRenderPrimitive(Handle<HwRenderPrimitive> rph) {
}

void WebGPUDriver::destroyVertexBufferInfo(Handle<HwVertexBufferInfo> vbih) {
}

void WebGPUDriver::destroyVertexBuffer(Handle<HwVertexBuffer> vbh) {
}

void WebGPUDriver::destroyIndexBuffer(Handle<HwIndexBuffer> ibh) {
}

void WebGPUDriver::destroyBufferObject(Handle<HwBufferObject> boh) {
}

Review comment (bug):
should we consider an `assert_invariant(mCommandBuffer.Get() != nullptr)` first?

Code after:

void WebGPUDriver::beginFrame(int64_t monotonic_clock_ns,
        int64_t refreshIntervalNs, uint32_t frameId) {
}

void WebGPUDriver::setFrameScheduledCallback(Handle<HwSwapChain> sch,
        CallbackHandler* handler, FrameScheduledCallback&& callback, uint64_t flags) {

}

void WebGPUDriver::setFrameCompletedCallback(Handle<HwSwapChain> sch,
        CallbackHandler* handler, utils::Invocable<void(void)>&& callback) {

}

void WebGPUDriver::setPresentationTime(int64_t monotonic_clock_ns) {
}

void WebGPUDriver::endFrame(uint32_t frameId) {
}

void WebGPUDriver::flush(int) {
}

void WebGPUDriver::finish(int) {
}

void WebGPUDriver::destroyRenderPrimitive(Handle<HwRenderPrimitive> rph) {
}

void WebGPUDriver::destroyVertexBufferInfo(Handle<HwVertexBufferInfo> vbih) {
}

void WebGPUDriver::destroyVertexBuffer(Handle<HwVertexBuffer> vbh) {
}

void WebGPUDriver::destroyIndexBuffer(Handle<HwIndexBuffer> ibh) {
}

void WebGPUDriver::destroyBufferObject(Handle<HwBufferObject> boh) {
}

void WebGPUDriver::destroyTexture(Handle<HwTexture> th) {
}

void WebGPUDriver::destroyProgram(Handle<HwProgram> ph) {
}

void WebGPUDriver::destroyRenderTarget(Handle<HwRenderTarget> rth) {
}
Repository: google/filament
File: filament/backend/src/webgpu/WebGPUDriver.h (C/C++)

Code before:
/*
 * Copyright (C) 2025 The Android Open Source Project
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *      http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

#ifndef TNT_FILAMENT_BACKEND_WEBGPUDRIVER_H
#define TNT_FILAMENT_BACKEND_WEBGPUDRIVER_H

#include "WebGPUHandles.h"

#include "webgpu/WebGPUSwapChain.h"
#include <backend/platforms/WebGPUPlatform.h>

#include "DriverBase.h"
#include "private/backend/Dispatcher.h"
#include "private/backend/Driver.h"
#include "private/backend/HandleAllocator.h"
#include <backend/DriverEnums.h>

#include <utils/compiler.h>

#include <webgpu/webgpu_cpp.h>


#include <cstdint>
#include <memory>

#ifndef FILAMENT_WEBGPU_HANDLE_ARENA_SIZE_IN_MB
#    define FILAMENT_WEBGPU_HANDLE_ARENA_SIZE_IN_MB 8
#endif

namespace filament::backend {

/**

Review comment (suggestion):
shouldn't this be grouped with the includes just below as part of the same "layer"?, e.g.:

```cpp
#include "WebGPUHandles.h"
#include "webgpu/WebGPUSwapChain.h"
#include <backend/platforms/WebGPUPlatform.h>
```

Code after:
/*
 * Copyright (C) 2025 The Android Open Source Project
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *      http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

#ifndef TNT_FILAMENT_BACKEND_WEBGPUDRIVER_H
#define TNT_FILAMENT_BACKEND_WEBGPUDRIVER_H

#include "WebGPUHandles.h"
#include "webgpu/WebGPUSwapChain.h"
#include <backend/platforms/WebGPUPlatform.h>

#include "DriverBase.h"
#include "private/backend/Dispatcher.h"
#include "private/backend/Driver.h"
#include "private/backend/HandleAllocator.h"
#include <backend/DriverEnums.h>

#include <utils/compiler.h>

#include <webgpu/webgpu_cpp.h>

#include <cstdint>
#include <memory>

#ifndef FILAMENT_WEBGPU_HANDLE_ARENA_SIZE_IN_MB
#    define FILAMENT_WEBGPU_HANDLE_ARENA_SIZE_IN_MB 8
#endif

namespace filament::backend {

/**
 * WebGPU backend (driver) implementation
 */
Repository: google/filament
File: filament/backend/src/webgpu/platform/WebGPUPlatformApple.mm (Objective-C++)

Code before:
// Platform specific includes and defines
#if defined(__APPLE__)
    #include <Cocoa/Cocoa.h>
    #import <QuartzCore/CAMetalLayer.h>
#elif defined(FILAMENT_IOS)
    // Metal is not available when building for the iOS simulator on Desktop.
    #define METAL_AVAILABLE __has_include(<QuartzCore/CAMetalLayer.h>)
    #if METAL_AVAILABLE
        #import <Metal/Metal.h>
        #import <QuartzCore/CAMetalLayer.h>
    #endif
    // is this needed?
    #define METALVIEW_TAG 255
#else
    #error Not a supported Apple + WebGPU platform
#endif

/**
 * Apple (Mac OS and IOS) specific implementation aspects of the WebGPU backend
 */

namespace filament::backend {

wgpu::Surface WebGPUPlatform::createSurface(void* nativeWindow, uint64_t /*flags*/) {
    wgpu::Surface surface = nullptr;
    // Is separate implementation for IOS needed?
    CAMetalLayer* metalLayer = (__bridge CAMetalLayer*) nativeWindow;
    wgpu::SurfaceSourceMetalLayer surfaceSourceMetalLayer{};
    surfaceSourceMetalLayer.layer = (__bridge void*) metalLayer;
    wgpu::SurfaceDescriptor surfaceDescriptor = {
        .nextInChain = &surfaceSourceMetalLayer,
        .label = "metal_surface",
    };
    surface = mInstance.CreateSurface(&surfaceDescriptor);
    FILAMENT_CHECK_POSTCONDITION(surface != nullptr) << "Unable to create Metal-backed surface.";
    return surface;
}

}// namespace filament::backend


Review comment (suggestion):
For the Metal backend at least, the `nativeWindow` is always a `CAMetalLayer` and nothing special needs to happen on iOS versus macOS.

Code after:
// Platform specific includes and defines
#include <Cocoa/Cocoa.h>
#import <QuartzCore/CAMetalLayer.h>

/**
 * Apple (Mac OS and IOS) specific implementation aspects of the WebGPU backend
 */

namespace filament::backend {

wgpu::Surface WebGPUPlatform::createSurface(void* nativeWindow, uint64_t /*flags*/) {
    wgpu::Surface surface = nullptr;
    // Both IOS and MacOS expects CAMetalLayer.
    CAMetalLayer* metalLayer = (__bridge CAMetalLayer*) nativeWindow;
    wgpu::SurfaceSourceMetalLayer surfaceSourceMetalLayer{};
    surfaceSourceMetalLayer.layer = (__bridge void*) metalLayer;
    wgpu::SurfaceDescriptor surfaceDescriptor = {
        .nextInChain = &surfaceSourceMetalLayer,
        .label = "metal_surface",
    };
    surface = mInstance.CreateSurface(&surfaceDescriptor);
    FILAMENT_CHECK_POSTCONDITION(surface != nullptr) << "Unable to create Metal-backed surface.";
    return surface;
}

}// namespace filament::backend
Repository: google/filament
File: filament/backend/src/webgpu/WebGPUHandles.cpp (C++)

Code before:
/*
* Copyright (C) 2025 The Android Open Source Project
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *      http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

#include "WebGPUHandles.h"

namespace filament::backend {

WGPUVertexBuffer::WGPUVertexBuffer(uint32_t vextexCount, uint32_t bufferCount,
        Handle<WGPUVertexBufferInfo> vbih)
    : HwVertexBuffer(vextexCount),
      vbih(vbih),
      mBuffers(MAX_VERTEX_BUFFER_COUNT) {}
void WGPUVertexBuffer::setBuffer(WGPUBufferObject* bufferObject, uint32_t index) {}

WGPUBufferObject::WGPUBufferObject(BufferObjectBinding bindingType, uint32_t byteCount)
    : HwBufferObject(byteCount),
      mBindingType(bindingType) {}
}// namespace filament::backend


Review comment (suggestion):
I think we should include `<cstdint>` _(`uint23_t` and such...)_, as well as `"DriverBase.h"`, `<backend/DriverEnums.h>`, and `<backend/Handle.h>` _(include what you use)_

Code after:
/*
* Copyright (C) 2025 The Android Open Source Project
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *      http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

#include "WebGPUHandles.h"

namespace filament::backend {

WGPUVertexBuffer::WGPUVertexBuffer(uint32_t vextexCount, uint32_t bufferCount,
        Handle<WGPUVertexBufferInfo> vbih)
    : HwVertexBuffer(vextexCount),
      vbih(vbih),
      buffers(MAX_VERTEX_BUFFER_COUNT) {}

// TODO: Empty function is a place holder for verxtex buffer updates and should be
// updated for that purpose.
void WGPUVertexBuffer::setBuffer(WGPUBufferObject* bufferObject, uint32_t index) {}

WGPUBufferObject::WGPUBufferObject(BufferObjectBinding bindingType, uint32_t byteCount)
    : HwBufferObject(byteCount),
      bufferObjectBinding(bindingType) {}
}// namespace filament::backend
Repository: google/filament
File: filament/backend/src/webgpu/WebGPUSwapChain.h (C/C++)

Code before:
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

#ifndef TNT_FILAMENT_BACKEND_WEBGPUSWAPCHAIN_H
#define TNT_FILAMENT_BACKEND_WEBGPUSWAPCHAIN_H

#include <webgpu/webgpu_cpp.h>

#include <backend/Platform.h>

#include <cstdint>

namespace filament::backend {

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

    wgpu::TextureView GetNextSurfaceTextureView(uint32_t width, uint32_t height);
    void Present();

private:
    void GetCurrentTexture(uint32_t width, uint32_t height, wgpu::SurfaceTexture*);
    wgpu::Surface mSurface = {};
    wgpu::SurfaceConfiguration mConfig = {};
    bool mConfigured = false;
};

} // namespace filament::backend

#endif //TNT_FILAMENT_BACKEND_WEBGPUSWAPCHAIN_H


Review comment (suggestion):
Looks like it was overlooked in the PR related to adding the swapchain class, but the function names should be camelCase, e.g. `getNextSurfaceTextureView` and `getCurrentTexture`.

Code after:
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

#ifndef TNT_FILAMENT_BACKEND_WEBGPUSWAPCHAIN_H
#define TNT_FILAMENT_BACKEND_WEBGPUSWAPCHAIN_H

#include <webgpu/webgpu_cpp.h>

#include <backend/Platform.h>

#include <cstdint>

namespace filament::backend {

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

    wgpu::TextureView getNextSurfaceTextureView(uint32_t width, uint32_t height);
    void present();

private:
    void getCurrentTexture(uint32_t width, uint32_t height, wgpu::SurfaceTexture*);
    wgpu::Surface mSurface = {};
    wgpu::SurfaceConfiguration mConfig = {};
    bool mConfigured = false;
};

} // namespace filament::backend

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

Code before:

    /*
     * Memory management
     */

    HandleAllocatorWGPU mHandleAllocator;

    template<typename D>
    Handle<D> allocHandle() {
        return mHandleAllocator.allocate<D>();
    }

    template<typename D, typename B, typename ... ARGS>
    D* constructHandle(Handle<B>& handle, ARGS&& ... args) noexcept {
        return mHandleAllocator.construct<D>(handle, std::forward<ARGS>(args)...);
    }
    template<typename D, typename B>
    D* handleCast(Handle<B> handle) noexcept {
        return mHandleAllocator.handle_cast<D*>(handle);
    }

    template<typename B, typename D,
            typename = std::enable_if_t<std::is_base_of_v<B, D>, D>>
    void destruct(Handle<B>& handle, D const* p) noexcept {
        return mHandleAllocator.deallocate(handle, p);
    }
};

}// namespace filament::backend

#endif // TNT_FILAMENT_BACKEND_WEBGPUDRIVER_H


Review comment (suggestion):
ii was thinking that this could be something like 
```suggestion
       template<typename D, typename B>
    void destructHandle(Handle<B>& handle) noexcept {
        auto* p = mHandleAllocator.handle_cast<D*>(handle);
        mHandleAllocator.deallocate(handle, p);
    }
```
which is from the metal driver

Code after:

    /*
     * Memory management
     */

    HandleAllocatorWGPU mHandleAllocator;

    template<typename D>
    Handle<D> allocHandle() {
        return mHandleAllocator.allocate<D>();
    }

    template<typename D, typename B, typename ... ARGS>
    D* constructHandle(Handle<B>& handle, ARGS&& ... args) noexcept {
        return mHandleAllocator.construct<D>(handle, std::forward<ARGS>(args)...);
    }
    template<typename D, typename B>
    D* handleCast(Handle<B> handle) noexcept {
        return mHandleAllocator.handle_cast<D*>(handle);
    }

    template<typename D, typename B>
    void destructHandle(Handle<B>& handle) noexcept {
        auto* p = mHandleAllocator.handle_cast<D*>(handle);
        return mHandleAllocator.deallocate(handle, p);
    }
};

}// namespace filament::backend

#endif // TNT_FILAMENT_BACKEND_WEBGPUDRIVER_H
Repository: google/filament
File: filament/backend/src/webgpu/WebGPUDriver.cpp (C++)

Code before:

void WebGPUDriver::destroyVertexBufferInfo(Handle<HwVertexBufferInfo> vbih) {
}

void WebGPUDriver::destroyVertexBuffer(Handle<HwVertexBuffer> vbh) {
}

void WebGPUDriver::destroyIndexBuffer(Handle<HwIndexBuffer> ibh) {
}

void WebGPUDriver::destroyBufferObject(Handle<HwBufferObject> boh) {
}

void WebGPUDriver::destroyTexture(Handle<HwTexture> th) {
}

void WebGPUDriver::destroyProgram(Handle<HwProgram> ph) {
}

void WebGPUDriver::destroyRenderTarget(Handle<HwRenderTarget> rth) {
}

void WebGPUDriver::destroySwapChain(Handle<HwSwapChain> sch) {
    if (sch) {
        auto* hwSwapChain = handleCast<WGPUHwSwapChain>(sch);
        destruct(sch, hwSwapChain);
    }
    mHwSwapChain = nullptr;
}

void WebGPUDriver::destroyStream(Handle<HwStream> sh) {
}

void WebGPUDriver::destroyTimerQuery(Handle<HwTimerQuery> tqh) {
}

void WebGPUDriver::destroyDescriptorSetLayout(Handle<HwDescriptorSetLayout> tqh) {
}

void WebGPUDriver::destroyDescriptorSet(Handle<HwDescriptorSet> tqh) {
}

Handle<HwSwapChain> WebGPUDriver::createSwapChainS() noexcept {
    return allocHandle<WGPUHwSwapChain>();
}

Handle<HwSwapChain> WebGPUDriver::createSwapChainHeadlessS() noexcept {
    return Handle<HwSwapChain>((Handle<HwSwapChain>::HandleId) mNextFakeHandle++);
}

Handle<HwTexture> WebGPUDriver::createTextureS() noexcept {

Review comment (suggestion):
and then if `destruct` is changed then this could maybe just be 
```suggestion
     if (sch) {
        destructHandle<HwSwapChain>(sch);
    }
```

Code after:

void WebGPUDriver::destroyVertexBufferInfo(Handle<HwVertexBufferInfo> vbih) {
}

void WebGPUDriver::destroyVertexBuffer(Handle<HwVertexBuffer> vbh) {
}

void WebGPUDriver::destroyIndexBuffer(Handle<HwIndexBuffer> ibh) {
}

void WebGPUDriver::destroyBufferObject(Handle<HwBufferObject> boh) {
}

void WebGPUDriver::destroyTexture(Handle<HwTexture> th) {
}

void WebGPUDriver::destroyProgram(Handle<HwProgram> ph) {
}

void WebGPUDriver::destroyRenderTarget(Handle<HwRenderTarget> rth) {
}

void WebGPUDriver::destroySwapChain(Handle<HwSwapChain> sch) {
    if (sch) {
        destructHandle<WGPUSwapChain>(sch);
    }
    mHwSwapChain = nullptr;
}

void WebGPUDriver::destroyStream(Handle<HwStream> sh) {
}

void WebGPUDriver::destroyTimerQuery(Handle<HwTimerQuery> tqh) {
}

void WebGPUDriver::destroyDescriptorSetLayout(Handle<HwDescriptorSetLayout> tqh) {
}

void WebGPUDriver::destroyDescriptorSet(Handle<HwDescriptorSet> tqh) {
}

Handle<HwSwapChain> WebGPUDriver::createSwapChainS() noexcept {
    return allocHandle<WGPUSwapChain>();
}

Handle<HwSwapChain> WebGPUDriver::createSwapChainHeadlessS() noexcept {
    return Handle<HwSwapChain>((Handle<HwSwapChain>::HandleId) mNextFakeHandle++);
}

Handle<HwTexture> WebGPUDriver::createTextureS() noexcept {
    return allocHandle<WGPUTexture>();
Repository: google/filament
File: filament/backend/src/webgpu/WebGPUHandles.cpp (C++)

Code before:
                        any(fEntry.flags & DescriptorFlags::DYNAMIC_OFFSET);
                wEntry.buffer.type = wgpu::BufferBindingType::Uniform;
                // TODO: Ideally we fill minBindingSize
                break;
            }

            case DescriptorType::INPUT_ATTACHMENT: {
                // TODO: support INPUT_ATTACHMENT. Metal does not currently.
                PANIC_POSTCONDITION("Input Attachment is not supported");
                break;
            }

            case DescriptorType::SHADER_STORAGE_BUFFER: {
                // TODO: Vulkan does not support this, can we?
                PANIC_POSTCONDITION("Shader storage is not supported");
                break;
            }
        }

        // Currently flags are only used to specify dynamic offset.

        // UNUSED
        // fEntry.count
    }

    layoutDescriptor.entryCount = wEntries.size();
    layoutDescriptor.entries = wEntries.data();
    // TODO Do we need to defer this until we have more info on textures and samplers??
    wLayout = device->CreateBindGroupLayout(&layoutDescriptor);
}
WebGPUDescriptorSetLayout::~WebGPUDescriptorSetLayout() {}
}// namespace filament::backend


Review comment (bug):
consider defining the descriptor concisely with a designated initializer, e.g.:

```cpp
wgpu::BindGroupLayoutDescriptor layoutDescriptor {
    // TODO: layoutDescriptor has a "Label". Ideally we can get info on what this layout is for
    // debugging. For now, hack an incrementing value.
    .label {"layout_" + std::to_string(++layoutNum)},
    .entryCount = wEntries.size(),
    .entries = wEntries.data()
};
```

Code after:
            }

            case DescriptorType::INPUT_ATTACHMENT: {
                // TODO: support INPUT_ATTACHMENT. Metal does not currently.
                PANIC_POSTCONDITION("Input Attachment is not supported");
                break;
            }

            case DescriptorType::SHADER_STORAGE_BUFFER: {
                // TODO: Vulkan does not support this, can we?
                PANIC_POSTCONDITION("Shader storage is not supported");
                break;
            }
        }

        // Currently flags are only used to specify dynamic offset.

        // UNUSED
        // fEntry.count
    }

    wgpu::BindGroupLayoutDescriptor layoutDescriptor{
        // TODO: layoutDescriptor has a "Label". Ideally we can get info on what this layout is for
        // debugging. For now, hack an incrementing value.
        .label{ "layout_" + std::to_string(++layoutNum) },
        .entryCount = wEntries.size(),
        .entries = wEntries.data()
    };
    // TODO Do we need to defer this until we have more info on textures and samplers??
    mLayout = device->CreateBindGroupLayout(&layoutDescriptor);
}
WebGPUDescriptorSetLayout::~WebGPUDescriptorSetLayout() {}
}// namespace filament::backend