Conversation
|
Thank you for submitting a pull request! If this is your first PR, make sure to add yourself to AUTHORS. |
|
Hey, really cool to notice you are writing a bachelor's thesis about WebGPU @Tandaradei! I have been trying to find a good stretch of time from other fronts to jump in to reviewing WebGPU as well. There are some hot questions we are pondering here at Unity:
Perhaps some of those perspectives may be interesting for your research as well :) If you end up producing data points, would be curious to read! Great work with getting on top of WebGPU with Emscripten! @kainino0x : on the note of point 5 above: is there any machinery to query if one is getting WebGPU-on-top-of-Vulkan vs WebGPU-on-top-of-GLES2/3 in a browser? I'd imagine e.g. shader compilation performance (and performance overrall..) will still be quite bad if browser needs to fall back to a GLES backend? |
|
Hey @juj, I will be completely honest with you, I have no answer for any of your questions. Does somebody know why there are 3 checks failing? From the error messages it seems like something WASM/SIMD related, but I have no idea what caused this to fail or how to fix it. |
|
There was some recent SIMD failures due to opcode renumbering that are not related to this PR. I think just re-basing your change to origin/master (past #11058) should help |
By default WebGPU will most likely always be on Vulkan/D3D12/Metal (it is right now). If there ends up being a GL backend it will probably require an explicit "compatibility profile" flag.
This is an improvement I've been thinking about for the long term. Right now most of the garbage-heavy entry points are relatively cold, but we still have the inherent garbage from the API of bind group creation and command encoder / pass / command buffer creation, so I'd like the group to finish discussing that topic first (e.g. gpuweb/gpuweb#214). |
|
Re-merged master into your branch because somehow the diff was including commits from master. |
kainino0x
left a comment
There was a problem hiding this comment.
Overall LGTM.
It would be ideal if we could include at least one more change: the refactor of WGPUShaderModuleDescriptor. Without this it won't be possible for an app to build the same code against both Dawn and Emscripten. (There may be other issues I'm missing, but this is the most noticeable.)
- Changed 'const * nextInChain' to 'chain' for chained descriptors
- Removed unnecessary structs/enums for web
- Changed wgpuDeviceGetDefaultQueue so it returns the same queue
when called multiple times with the same device
- Changed makeCheckDescriptor to also work with 'chain' member (hacky)
kainino0x
left a comment
There was a problem hiding this comment.
Thanks for the revisions!
I'd still like to resolve this one before merging (I could take care of it if needed). |
So no more What has to be done to finish this up? :) PS: Why is there no |
Yup, exactly. Sorry, I should point to an example.
WGSL is text so it's passed as a null-terminated string. |
In your linked example, shouldn't there be a WGPUShaderModuleSPIRVDescriptor sm_desc = {
.codeSize = spv_source_size / sizeof(uint32_t),
.code = (const uint32_t*)spv_source_data
};
WGPUShaderModuleDescriptor sm_wrapper = {
.nextInChain = &(WGPUChainedStruct){
.next = (WGPUChainedStruct const*)&sm_desc,
.sType = WGPUSType_ShaderModuleSPIRVDescriptor
},
};
WGPUShaderModule module = wgpuDeviceCreateShaderModule(device, &sm_wrapper); |
|
It's not exactly like your C code. Sorry, that example used the C++ wrapper so it's not exactly the same. Here's another example which I forgot about: |
kainino0x
left a comment
There was a problem hiding this comment.
(tiny formatting nits, will apply them myself)
|
Since I don't have any good test cases for this, let me see if I can find someone to try this out on their codebase to make sure everything seems working before landing :) |
| "usage", | ||
| "dimension", | ||
| "size", | ||
| "arrayLayerCount", |
|
To be discussed with @kainino0x and @Kangz Aside from that, and with those fixes, the PR works as expected @Kangz also mentioned the possibility of integrating |
Is this a case where the spec headers already define upcoming work (occlusion queries have already been added to the spec and headers?), but Dawn hasn't just implemented them? Do we expect that the content in the headers will stay the same? If so, then let's not delete valid work that has already been taken, but instead can add comments to webgpu.h and library_webgpu.js along the lines of If on the other hand that is work that is not likely to stay the same, then dropping it sounds viable? |
|
The problem is that there currently exists no other If this PR is merged as-is, my application will not be able support WebGPU through Emscripten anymore, because it is built against the C++ API. Some potential ways to solve this:
(Note that the library that I ported plans to use the C API at some point, but work needs to be done, and it won't be ready before a while) |
|
Hmm, if I understood the situation correctly, it does seem like the best way forward would be to include
Do you know if Dawn autogenerates its webgpu_cpp wrapper? Or is it all currently hand-maintained? |
All of |
|
Would it by thinkable to add some fields in Practical example ( |
|
I'm not too keen on adding conditionals to |
|
Thanks for the patch Hugo, I'm going to make some tweaks to it and push it to this branch. I think we'll be ready to merge then. (I may not be able to get this done today, we'll see.) |
|
Ah, @hugoam added back some things so |
|
Ohh I see. I guess we should just exactly match Dawn here for the moment then. |
a3596ef to
5f83f1f
Compare
6029c34 to
762c4ab
Compare
|
Sorry, realized that change wouldn't compile with webgpu_cpp.cpp (I ctrl-f'd wrong the first time). f494f63 now. |
|
BTW I think we should put webgpu_cpp in both webgpu-headers and Emscripten eventually, but we'll do that later. |
|
The list of members in struct_info is not enough to compute the offsets (it doesn't know the types). I think how this works is it calls offsetof in C and saves the result. So I'm pretty sure this still would work. |
|
Ahhh, I must have conflated some of the errors I faced when trying to get this to work with an overly naive explanation of how Emscripten works then ! I suppose it's all good in that case |
|
Now Dawn has just removed the deprecated fields. But we'll do a follow-up PR I suppose 😄 |
|
Yup, we knew we would need another PR here :) I wanted to get this landed as we're about to start removing the deprecated fields from Blink as well, so they won't be exposed to JS anymore, and this patch is needed for Emscripten'd content to work on Chrome. |
|
Just tried building bgfx examples with Emscripten@master and it works as expected 😄 |
|
Awesome! |
WIP
Related issue: #11007
wgpuDeviceCreateBindGroupLayoutandwgpuDeviceCreateBindGroupdefineI64ParamandreceiveI64ParamAsI32ssizeparameter tosetIndexBufferandsetVertexBufferPresentModefromVSynctoFifoI tested the changes with my pet project (https://github.com/Tandaradei/webgpu-wasm/tree/new-webgpu-version) which works fine in Chrome Canary 84.0.4133.0, but doesn't work in Firefox Nightly 77.0a1 (
TypeError: buffer.mapWriteAsync is not a function,_wgpuBufferMapWriteAsync).The project isn't using all the features from WebGPU so I'm not sure if everything works (for example the new QuerySet).