Update wgpu backend to emscripten 2.0.20 and chrome canary 92 #4116

bfierz · 2021-05-08T16:51:08Z

This changeset introduces some implementation simplifications proposed by @Kangz here and updates the code to be compatible with Emscripten 2.0.20 as described here.

The major changes are:

Make use of automatic bind group generation instead of explicitly creating them
Update the API to the latest WebGPU changes as outlined here

Kangz · 2021-05-08T21:03:28Z

backends/imgui_impl_wgpu.cpp

@@ -331,7 +329,8 @@ void ImGui_ImplWGPU_RenderDrawData(ImDrawData* draw_data, WGPURenderPassEncoder
    // Create and grow vertex/index buffers if needed
    if (fr->VertexBuffer == NULL || fr->VertexBufferSize < draw_data->TotalVtxCount)
    {
-        SafeRelease(fr->VertexBuffer);
+        if (fr->VertexBuffer)
+            wgpuBufferDestroy(fr->VertexBuffer);


I think we still need to call SafeRelease here and for the index buffer. Destroy only destroy the data internal to the buffer but the CPU-side state tracking isn't freed. This is to allow to reclaim GPU memory faster.

Kangz · 2021-05-08T21:04:58Z

backends/imgui_impl_wgpu.cpp

@@ -419,9 +417,11 @@ void ImGui_ImplWGPU_RenderDrawData(ImDrawData* draw_data, WGPURenderPassEncoder
                }
                else
                {
-                    WGPUBindGroup image_bind_group = ImGui_ImplWGPU_CreateImageBindGroup(g_resources.ImageBindGroupLayout, (WGPUTextureView)pcmd->TextureId);
+                    WGPUBindGroupLayout bg_layout = wgpuRenderPipelineGetBindGroupLayout(g_pipelineState, 1);


nit: Maybe this bindgroup could be stored so we don't re-query it all the time. There is little cost to doing that in native, but in WASM you will get a new JS object everytime.

The bind group is stored in g_resources. Only the layout is created and released once a new bind group is created. I assumed, that the bind group would store a reference to the layout internally.

Yeah the bindgroup stores a reference to the layout internally, This was just to avoid the creation of new JS object for the layout for every texture used here. We could just store to a global variable at [1]. But this is a nit: so it's ok to skip.

Seems I've misunderstood you. Your suggestion makes perfect sense, consider it changed 👍

Kangz · 2021-05-08T21:06:37Z

backends/imgui_impl_wgpu.cpp

@@ -455,7 +455,10 @@ static void ImGui_ImplWGPU_CreateFontsTexture()
        tex_desc.dimension = WGPUTextureDimension_2D;
        tex_desc.size.width = width;
        tex_desc.size.height = height;
+        tex_desc.size.depthOrArrayLayers = 1;
+#if !defined(__EMSCRIPTEN__)


nit: Dawn in native should understand depthOrArrayLayers too so this ifdef might not be necessary.

This is actually a work-around for an issue I've encountered in Dawn (latest commit 30.4.). The validator checks for depth != 0, which triggers an error.

It shouldn't AFAIK: https://source.chromium.org/chromium/chromium/src/+/main:third_party/dawn/src/dawn_native/Texture.cpp;drc=2dd2d67dbcbbf0a241fd13d4fccbc049ee39bfd4;l=288

What error message do you get?

Thanks for pointing it out. I had another look at the code. It seems like dawn is trying to fix 'wrong' settings in the wrong way:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/dawn/src/dawn_native/Texture.cpp;drc=2dd2d67dbcbbf0a241fd13d4fccbc049ee39bfd4;l=251

Ah, that's because it is assuming there is the C++ structure defaulting happening. https://source.chromium.org/chromium/chromium/src/+/main:out/Debug/gen/third_party/dawn/src/include/dawn/webgpu_cpp.h;drc=d3d0337463160599bde3ef97db67071148652b9c;l=1203

Keeping as is sounds good, but soon (as in this week) Dawn will remove depth since we're past the deprecation period.

Well, the default initialization in the cpp header explains a lot. I am going to convert the implementation eventually to use the cpp version.

I am going to remove depth in this case, as Dawn users will probably stay at head anyways (this implementation anyway already uses most of the latest changes().

Ok sounds good!

Kangz · 2021-05-08T21:06:44Z

backends/imgui_impl_wgpu.cpp

        WGPUTextureDataLayout layout = {};
        layout.offset = 0;
        layout.bytesPerRow = width * size_pp;
        layout.rowsPerImage = height;
        WGPUExtent3D size = { static_cast<uint32_t>(width), static_cast<uint32_t>(height), 1 };
+#if !defined(__EMSCRIPTEN__)


Not setting depth here leads to wgpuQueueWriteTexture not being executed properly (no texture data written), however, not error message is produced.

Kangz

Looks great, with some small comments. Thanks for doing the update!

Kangz · 2021-05-09T18:10:59Z

backends/imgui_impl_wgpu.cpp


    ImGui_ImplWGPU_CreateFontsTexture();
    ImGui_ImplWGPU_CreateUniformBuffer();

    // Create resource bind group
+    WGPUBindGroupLayout bg_layouts[2];
+    bg_layouts[0] = wgpuRenderPipelineGetBindGroupLayout(g_pipelineState, 0);
+    bg_layouts[1] = wgpuRenderPipelineGetBindGroupLayout(g_pipelineState, 1);


Kangz · 2021-05-09T18:11:51Z

backends/imgui_impl_wgpu.cpp

@@ -419,9 +417,11 @@ void ImGui_ImplWGPU_RenderDrawData(ImDrawData* draw_data, WGPURenderPassEncoder
                }
                else
                {
-                    WGPUBindGroup image_bind_group = ImGui_ImplWGPU_CreateImageBindGroup(g_resources.ImageBindGroupLayout, (WGPUTextureView)pcmd->TextureId);
+                    WGPUBindGroupLayout bg_layout = wgpuRenderPipelineGetBindGroupLayout(g_pipelineState, 1);


Yeah the bindgroup stores a reference to the layout internally, This was just to avoid the creation of new JS object for the layout for every texture used here. We could just store to a global variable at [1]. But this is a nit: so it's ok to skip.

…tion in WebASM

bfierz · 2021-05-09T19:01:42Z

Updated the PR with all the suggestions from @Kangz. Thanks for the review.

Kangz · 2021-05-09T19:06:31Z

LGTM no additional comments from the WebGPU side

Merged 13 commits.

ocornut · 2021-05-16T17:14:41Z

All merged now (as 83bdfef) thank you both very much!

…n as introduced in 83bdfef (#6117, #4116, #3632) The feature was removed from WebGPU (gpuweb/gpuweb#2470)

bfierz added 2 commits May 7, 2021 21:02

Simplify WebGPU backend according to feedback

332c412

Use default bind group layout generation

9fb38d9

Kangz reviewed May 8, 2021

View reviewed changes

Kangz approved these changes May 8, 2021

View reviewed changes

Kangz reviewed May 9, 2021

View reviewed changes

ocornut added the backends label May 9, 2021

bfierz added 11 commits May 9, 2021 20:54

Cache the bind group layout for the image to avoid JS object realloca…

cc0c95a

…tion in WebASM

Refactor render queue access

0b3b5f6

Upgrade to new enumerations

b0c8e8c

Update calls to default queue

1a42564

Update OutputAttachement enumeration

46521b4

Update ColorPassAttachment

0acb1a5

Update BlendColor

7906278

Update texture descriptors

042cc1c

Introduce check for minimum emscripten version

d6d32b3

Update render pipeline creation to new wgpuDeviceCreateRenderPipeline2

43ced00

Release buffers after they are destroyed

03de8ae

bfierz force-pushed the feature/update-wgpu-emsdk-2.0.20 branch from 71f3005 to 03de8ae Compare May 9, 2021 18:55

ocornut pushed a commit that referenced this pull request May 16, 2021

Backends: WGPU: update to latest specs. (#4116, #3632)

83bdfef

Merged 13 commits.

ocornut closed this May 16, 2021

ocornut added the web label Jul 8, 2021

ocornut pushed a commit that referenced this pull request Jan 25, 2023

Backends: WebGU: Revert the implicit render pipeline layout generatio…

d73e328

…n as introduced in 83bdfef (#6117, #4116, #3632) The feature was removed from WebGPU (gpuweb/gpuweb#2470)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update wgpu backend to emscripten 2.0.20 and chrome canary 92 #4116

Update wgpu backend to emscripten 2.0.20 and chrome canary 92 #4116

bfierz commented May 8, 2021

Kangz May 8, 2021

Kangz May 8, 2021

bfierz May 9, 2021

Kangz May 9, 2021

bfierz May 9, 2021

Kangz May 8, 2021

bfierz May 9, 2021

Kangz May 9, 2021

bfierz May 9, 2021

Kangz May 9, 2021

bfierz May 9, 2021

Kangz May 9, 2021

Kangz May 8, 2021

bfierz May 9, 2021

Kangz left a comment

Kangz May 9, 2021

Kangz May 9, 2021

bfierz commented May 9, 2021

Kangz commented May 9, 2021

ocornut commented May 16, 2021

Update wgpu backend to emscripten 2.0.20 and chrome canary 92 #4116

Update wgpu backend to emscripten 2.0.20 and chrome canary 92 #4116

Conversation

bfierz commented May 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kangz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bfierz commented May 9, 2021

Kangz commented May 9, 2021

ocornut commented May 16, 2021