Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make GPUBufferBinding size optional #331

Closed
beaufortfrancois opened this issue Jun 13, 2019 · 7 comments · Fixed by #346
Closed

Make GPUBufferBinding size optional #331

beaufortfrancois opened this issue Jun 13, 2019 · 7 comments · Fixed by #346

Comments

@beaufortfrancois
Copy link
Contributor

How about making GPUBufferBinding size optional? If not specified, it would be the same as the buffer size passed in the binding resource of device.createBindGroup()?

I believe this would make code easier to read and digest for newcomers.

    const gpuBufferSize = 4;
    const gpuBuffer = device.createBuffer({
      size: gpuBufferSize,
      usage: GPUBufferUsage.STORAGE
    });

    const bindGroup = device.createBindGroup({
      layout: bindGroupLayout,
      bindings: [
        {
          binding: 0,
          resource: {
            buffer: myGpuBuffer,
-           size: gpuBufferSize
          }
        }
      ]
    });
@kvark
Copy link
Contributor

kvark commented Jun 13, 2019

Vulkan has VK_WHOLE_SIZE for this, so default makes sense...

@kainino0x
Copy link
Contributor

Makes sense to me, I think we talked about this once before too.

My only concern is that we should try to avoid making a distinction between undefined and 0. IMO it would be fine for the default to be 0, and 0 is a special value meaning to use the whole buffer size.

@beaufortfrancois
Copy link
Contributor Author

Having a size of 0 that means the whole buffer size doesn't seem right to me. I fail to find some examples of this usage in Web APIs.

I would go with the IDL below.

 dictionary GPUBufferBinding {
     required GPUBuffer buffer;
     u64 offset = 0;
-    required u64 size;
+    u64? size;
 };

If size is not defined, use the whole size of the buffer.
Otherwise use same rules as before. 0 is still a valid value.

@kainino0x
Copy link
Contributor

Are there examples of Web APIs making a distinction between undefined and 0?

If we want to avoid distinguishing undefined and 0, could have a special value like in Vulkan:

enum GPUBufferBindingSize { "whole-size" };
dictionary GPUBufferBinding {
    required GPUBuffer buffer;
    u64 offset = 0;
    (GPUBufferBindingSize | u64) size = "whole-size";
};

@beaufortfrancois
Copy link
Contributor Author

beaufortfrancois commented Jun 25, 2019

This (GPUBufferBindingSize | u64) trick is still yet another way of saying that undefined gets treated as "whole-size". We're basically still distinguishing undefined and 0 there.

I made a mistake though. We should remove the ?, as null should just be treated as 0. undefined should be the only special value. Then, it should be:

 dictionary GPUBufferBinding {
     required GPUBuffer buffer;
     u64 offset = 0;
-    required u64 size;
+    // If size is undefined, use the whole size of the buffer.
+    u64 size;
 };

If size is undefined, use the whole size of the buffer.
Otherwise use same rules as before. 0 is still a valid value.

@kainino0x
Copy link
Contributor

You're right that my suggestion still distinguishes 0 and undefined. Good point.

Your last suggestion looks good to me. Would you open a PR for it?

@beaufortfrancois
Copy link
Contributor Author

@kainino0x Here's the PR: #346

aarongable pushed a commit to chromium/chromium that referenced this issue Jul 15, 2019
Following WebGPU spec change at gpuweb/gpuweb#331,
bind groups should now use the whole size of the buffer if undefined.

Bug: 877147
Change-Id: I62c942c7607ce119398ab6967070c3f104443410
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1700051
Reviewed-by: François Beaufort <beaufort.francois@gmail.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: François Beaufort <beaufort.francois@gmail.com>
Cr-Commit-Position: refs/heads/master@{#677285}
ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this issue Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants