Bug 238428 - [WebGPU] Implement CommandEncoder::copyBufferToTexture() according to the spec
Summary: [WebGPU] Implement CommandEncoder::copyBufferToTexture() according to the spec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on: 238311
Blocks: 238430
  Show dependency treegraph
 
Reported: 2022-03-27 19:00 PDT by Myles C. Maxfield
Modified: 2022-03-31 01:43 PDT (History)
4 users (show)

See Also:


Attachments
Patch (35.53 KB, patch)
2022-03-27 19:03 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (35.58 KB, patch)
2022-03-28 12:20 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (35.85 KB, patch)
2022-03-28 14:07 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (35.85 KB, patch)
2022-03-28 14:09 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (35.85 KB, patch)
2022-03-28 14:10 PDT, Myles C. Maxfield
kkinnunen: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2022-03-27 19:00:32 PDT
.
Comment 1 Myles C. Maxfield 2022-03-27 19:03:43 PDT
Created attachment 455875 [details]
Patch
Comment 2 Myles C. Maxfield 2022-03-28 12:20:32 PDT
Created attachment 455940 [details]
Patch
Comment 3 Myles C. Maxfield 2022-03-28 14:07:54 PDT
Created attachment 455952 [details]
Patch
Comment 4 Myles C. Maxfield 2022-03-28 14:09:04 PDT
Created attachment 455953 [details]
Patch
Comment 5 Myles C. Maxfield 2022-03-28 14:10:39 PDT
Created attachment 455954 [details]
Patch
Comment 6 Kimmo Kinnunen 2022-03-30 00:51:03 PDT
Comment on attachment 455954 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=455954&action=review

with non-ascii chars fixed
with integral auto variables declared explicitly as their intended type.

> Source/WebGPU/WebGPU/Texture.mm:2370
> +    auto blockWidth = Texture::texelBlockWidth(fromAPI(imageCopyTexture.texture).descriptor().format);

explicitly declare the intended type

> Source/WebGPU/WebGPU/Texture.mm:2556
> +    if ((imageCopyTexture.origin.x + copySize.width) > subresourceSize.width)

I think generally a+b>c is written without parenthesis. Even though the spec has the parenthesis, maybe the code would still be nicer without.

> Source/WebGPU/WebGPU/Texture.mm:2585
> +    auto blockWidth = Texture::texelBlockWidth(format);

all these calculations are important part of the validation.
the integral types should be declared explicitly to ensure less mistakes

> Source/WebGPU/WebGPU/Texture.mm:2618
> +        if (layout.bytesPerRow <= bytesInLastRow)

off-by-one
should be
  if (layout.bytesPerRow < bytesInLastRow)
      return false;

> Source/WebGPU/WebGPU/Texture.mm:2630
> +    auto requiredBytesInCopy = 0;

this is declared as int, but it does calculations with layout members that are uint64_t.
Instead, declare it explicitly as uint64_t
Comment 7 Kimmo Kinnunen 2022-03-30 00:52:58 PDT
Comment on attachment 455954 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=455954&action=review

> Source/WebGPU/WebGPU/Texture.mm:2631
> +

this is also missing the checked arithmetic fixme?
e.g. when you implement checked arithmetic, you will mark this as Checked<uint64_t> requiredBytesInCopy ?

> Source/WebGPU/WebGPU/Texture.mm:2662
> +    if (layout.offset + requiredBytesInCopy > byteSize)

missing checked arithmetic fixme?
Comment 8 Myles C. Maxfield 2022-03-31 01:33:26 PDT
Comment on attachment 455954 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=455954&action=review

>> Source/WebGPU/WebGPU/Texture.mm:2631
>> +
> 
> this is also missing the checked arithmetic fixme?
> e.g. when you implement checked arithmetic, you will mark this as Checked<uint64_t> requiredBytesInCopy ?

Right. Good catch.
Comment 9 Myles C. Maxfield 2022-03-31 01:42:45 PDT
Committed r292147 (249054@trunk): <https://commits.webkit.org/249054@trunk>
Comment 10 Radar WebKit Bug Importer 2022-03-31 01:43:17 PDT
<rdar://problem/91092275>