Bug 238428 – [WebGPU] Implement CommandEncoder::copyBufferToTexture() according to the spec

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGPU

[Myles C. Maxfield]

.

[Myles C. Maxfield]

Created attachment 455875 [details]
Patch

[Myles C. Maxfield]

Created attachment 455940 [details]
Patch

[Myles C. Maxfield]

Created attachment 455952 [details]
Patch

[Myles C. Maxfield]

Created attachment 455953 [details]
Patch

[Myles C. Maxfield]

Created attachment 455954 [details]
Patch

[Kimmo Kinnunen]

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

[Kimmo Kinnunen]

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?

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

Committed r292147 (249054@trunk): <https://commits.webkit.org/249054@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/91092275>