Initial spec for GPUDevice.createBuffer - #419
Merged
Merged
Conversation
kainino0x
approved these changes
Aug 22, 2019
kainino0x
reviewed
Aug 22, 2019
kvark
reviewed
Aug 22, 2019
| 1. If device is lost return false. | ||
| 1. If any of the bits of |descriptor|'s {{GPUBufferDescriptor/usage}} aren't present in this device's [[allowed buffer usages]] return false. | ||
| 1. If both the {{GPUBufferUsage/MAP_READ}} and {{GPUBufferUsage/MAP_WRITE}} bits of |descriptor|'s {{GPUBufferDescriptor/usage}} attribute are set, return false. | ||
| 1. Return true. |
Contributor
There was a problem hiding this comment.
ok, this appears to be written for the implementors
Would it be simpler if we describe it as:
Buffer creation fails if one of the conditions is true:
- Device is lost
- Any of the bits ...
- Usage contains both MAP_READ and MAP_WRITE ...
Contributor
There was a problem hiding this comment.
This is an "algorithm" which means it's the procedure to do when "validating GPUBufferDescriptor". Other specs do this too. However since this has no return value it seems fine to flip it.
I think it's also that web specs are written more for browsers than for users. I'm personally ok with deviating from this because I see the tests as being the source of truth for browser implementers whenever possible.
Contributor
|
Hmm the "Diff" link shows quite a few more changes than there are in the PR :/ |
kainino0x
reviewed
Aug 22, 2019
| <dl dfn-type="abstract-op"> | ||
| : <dfn>validating GPUBufferDescriptor</dfn>(device, descriptor) | ||
| :: | ||
| <div algorithm="validation GPUBufferDescriptor(device, descriptor)"> |
Contributor
There was a problem hiding this comment.
Some other specs seem to use present progressive tense, "validating GPUBufferDescriptor". Also I don't think the args are needed inside the algorithm name.
Contributor
There was a problem hiding this comment.
Thinking about it more, it would make sense to make this be part of the "creating a GPUBuffer" algorithm. May or may not be useful to keep it separate from that.
kvark
approved these changes
Aug 23, 2019
kainino0x
approved these changes
Aug 23, 2019
kainino0x
left a comment
Contributor
There was a problem hiding this comment.
Some more thoughts, but we shouldn't block landing. I'd be happy to do the follow-up to make these suggested changes after it's landed.
| </dl> | ||
|
|
||
| Note: | ||
| {{GPUBuffer/[[size]]}} and {{GPUBuffer/[[usage]]}} are immutable once the {{GPUBuffer}} has been created. |
Contributor
There was a problem hiding this comment.
IMO would no longer be needed with "readonly" above.
Contributor
Author
|
@kvark @kainino0x @JusSn any other changes blocking this? |
kvark
reviewed
Sep 3, 2019
kvark
left a comment
Contributor
There was a problem hiding this comment.
Here comes another review pass
Kangz
force-pushed
the
initial-buffer-spec
branch
from
September 4, 2019 15:58
a46e84f to
5d275db
Compare
RafaelCintron
approved these changes
Sep 4, 2019
Contributor
|
I don't think there is anything outstanding right now and I'd really like to get this in to make it easier to iterate. Merging! |
JusSn
pushed a commit
to JusSn/gpuweb
that referenced
this pull request
Sep 19, 2019
JusSn
added a commit
that referenced
this pull request
Sep 19, 2019
* Add a component type for GPUBGLBinding compatiblity (#384) In shaders there are several texture types for each dimensionality depending on their component type. It can be either float, uint or sint, with maybe in the future depth/stencil if WebGPU allows reading such textures. The component type of a GPUTextureView's format must match the component type of its binding in the shader module. This is for several reasons: - Vulkan requires the following: "The Sampled Type of an OpTypeImage declaration must match the numeric format of the corresponding resource in type and signedness, as shown in the SPIR-V Sampled Type column of the Interpretation of Numeric Format table, or the values obtained by reading or sampling from this image are undefined." - It is also required in OpenGL for the texture units to be complete, a uint or sint texture unit used with a non-nearest sampler is incomplete and returns black texels. Similar constraints must exist in other APIs. To encode this compatibility constraint, a new member is added to GPUBindGroupLayoutBinding that is a new enum GPUTextureComponentType that give the component type of the texture. * Make GPUBGLBinding.textureDimension default to 2d. This is the most common case and avoids having an optional dictionary member with no default value (but that still requires a value for texture bindings). * Add a component type for GPUBGLBinding compatiblity (#384) In shaders there are several texture types for each dimensionality depending on their component type. It can be either float, uint or sint, with maybe in the future depth/stencil if WebGPU allows reading such textures. The component type of a GPUTextureView's format must match the component type of its binding in the shader module. This is for several reasons: - Vulkan requires the following: "The Sampled Type of an OpTypeImage declaration must match the numeric format of the corresponding resource in type and signedness, as shown in the SPIR-V Sampled Type column of the Interpretation of Numeric Format table, or the values obtained by reading or sampling from this image are undefined." - It is also required in OpenGL for the texture units to be complete, a uint or sint texture unit used with a non-nearest sampler is incomplete and returns black texels. Similar constraints must exist in other APIs. To encode this compatibility constraint, a new member is added to GPUBindGroupLayoutBinding that is a new enum GPUTextureComponentType that give the component type of the texture. * Make GPUBGLBinding.textureDimension default to 2d. This is the most common case and avoids having an optional dictionary member with no default value (but that still requires a value for texture bindings). * unifinished createBindGroupLayout algorithm * draft of BindGroupLayout details * draft of BindGroupLayout details * polish before PR * fix typo * replace u32/i32/u64 with normal int types or specific typedefs (#423) * Do not require vertexInput in GPURenderPipelineDescriptor (#378) * Add a default for GPURenderPassColorAttachmentDescriptor.storeOp (#376) Supersedes #268. * Initial spec for GPUDevice.createBuffer (#419) * Start writing spec for device/adapter, introduce internal objects (#422) * Move validation rules out of algorithm body and better describe GPUBindGroupLayout internal slots * Include limits for dynamic offset buffers * Rename 'dynamic' boolean to 'hasDynamicOffsets' * Fix indentation for ci bot * More indentation errors * Fix var typos * Fix method definition * Fix enum references * Missing </dfn> tag * Missing </dfn> tag * Remove bad [= =] * Fix old constant name * Half-formed new validation rule structure for createBindGroupLayout * An interface -> the interface * Remove old 'layout binding' reference * fix device lost validation reference * Fix 'dynamic' typo
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Putting this up to try to get the spec-writing ball rolling. Editors, PTAL @kainino0x @kvark @JusSn. Also @litherum PTAL since I think you have the most Bikeshed experience here. This is the first time I'm doing non-trivial Bikeshed and Web-style specs so I'm sure I'm holding it wrong :)
Preview | Diff