Bug 238251 – [WebGPU] Implement Device::createTexture() according to the spec

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

[Myles C. Maxfield]

[WebGPU] Implement Device::createTexture() according to the spec

[Myles C. Maxfield]

Created attachment 455484 [details]
Patch

[Darin Adler]

Comment on attachment 455484 [details]
Patch

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

> Source/WebGPU/WebGPU/Texture.h:60
>      const id<MTLTexture> m_texture { nil };

Why is it safe to have a raw pointer here? Doesn’t this need to be a RetainPtr?

> Source/WebGPU/WebGPU/Texture.h:62
> +    const WGPUTextureDescriptor m_descriptor { }; // "The GPUTextureDescriptor describing this texture."

I don’t think you need { } here. Unless WGPUTextureDescriptor is a scalar. Otherwise the descriptor should have default values for its members rather than a { } here.

> Source/WebGPU/WebGPU/Texture.mm:36
> +static std::optional<WGPUFeatureName> featureRequirementForFormat(WGPUTextureFormat format)

If WGPUTextureFormat an enum? If not, why not? If it’s an enum, maybe we want switches without defaults below so we can be reminded by compiler warnings to revisit them all any time we add a new enumeration value.

> Source/WebGPU/WebGPU/Texture.mm:495
> +    // "descriptor.mipLevelCount must be greater than zero."
> +    if (!descriptor.mipLevelCount)
> +        return false;

I’m not sure that this style where we quote every line of the specification (which may no longer match this in future as it is revised) and have code doing exactly that just below. Also, why not use "> 0" if the text version says "greater than zero"?

> Tools/TestWebKitAPI/Tests/WTF/MathExtras.cpp:631
> +    EXPECT_EQ(WTF::fastLog2(1u), 0u);
> +    EXPECT_EQ(WTF::fastLog2(2u), 1u);
> +    EXPECT_EQ(WTF::fastLog2(3u), 2u);
> +    EXPECT_EQ(WTF::fastLog2(4u), 2u);
> +    EXPECT_EQ(WTF::fastLog2(5u), 3u);
> +    EXPECT_EQ(WTF::fastLog2(6u), 3u);
> +    EXPECT_EQ(WTF::fastLog2(7u), 3u);
> +    EXPECT_EQ(WTF::fastLog2(8u), 3u);
> +    EXPECT_EQ(WTF::fastLog2(9u), 4u);

Is testing 1-9 sufficient coverage? Typically I like to test edge cases, like the highest supported value, the first unsupported, and various boundaries, etc. etc. It seems peculiar to test such a small range so close to 0.

[Myles C. Maxfield]

Comment on attachment 455484 [details]
Patch

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

>> Source/WebGPU/WebGPU/Texture.h:60
>>      const id<MTLTexture> m_texture { nil };
> 
> Why is it safe to have a raw pointer here? Doesn’t this need to be a RetainPtr?

This framework has ARC enabled.

>> Source/WebGPU/WebGPU/Texture.h:62
>> +    const WGPUTextureDescriptor m_descriptor { }; // "The GPUTextureDescriptor describing this texture."
> 
> I don’t think you need { } here. Unless WGPUTextureDescriptor is a scalar. Otherwise the descriptor should have default values for its members rather than a { } here.

WGPUTextureDescriptor is a struct, but it's a struct defined in a 3rd party header. So far, we've been avoiding modifying the header, to make future merges from upstream easier.

>> Source/WebGPU/WebGPU/Texture.mm:36
>> +static std::optional<WGPUFeatureName> featureRequirementForFormat(WGPUTextureFormat format)
> 
> If WGPUTextureFormat an enum? If not, why not? If it’s an enum, maybe we want switches without defaults below so we can be reminded by compiler warnings to revisit them all any time we add a new enumeration value.

It is, and yes, I think you're right (similar to https://bugs.webkit.org/show_bug.cgi?id=238250#c3). I'll update this code.

>> Source/WebGPU/WebGPU/Texture.mm:495
>> +        return false;
> 
> I’m not sure that this style where we quote every line of the specification (which may no longer match this in future as it is revised) and have code doing exactly that just below. Also, why not use "> 0" if the text version says "greater than zero"?

I knew you would say that 😜

I added these comments to make it super clear how the code maps to the spec, and to make it super clear where to make changes when the spec changes. I also kind of like these kind of comments during the initial development of a project, because it makes it more clear if there's some part of the implementation that's missing; it won't silently be absent but will instead have the form of a comment without any accompanying C++ code. (Of course this doesn't help the possibility of me forgetting to write the comment and the code together.)

(I also know you dislike code that naively mirrors the spec, but I haven't been doing that. For example, see how Buffer.h is missing a few fields that are present in the spec, because our implementation doesn't actually need them. I also included comments in these locations to make it clear that these omissions are intentional.)

For the "greater than zero", that is the spec describing what must be true in order for validation to return true. If the statement doesn't hold, then validation must return false, so that's what this `if` block is doing here - checking the negation of the statement. Because the types are unsigned, (!(descriptor.mipLevelCount > 0)) is the same as (!descriptor.mipLevelCount).

>> Tools/TestWebKitAPI/Tests/WTF/MathExtras.cpp:631
>> +    EXPECT_EQ(WTF::fastLog2(9u), 4u);
> 
> Is testing 1-9 sufficient coverage? Typically I like to test edge cases, like the highest supported value, the first unsupported, and various boundaries, etc. etc. It seems peculiar to test such a small range so close to 0.

Yeah, I originally added this as a form of documentation, but I think you're right that it makes sense to write proper test cases while I'm here. Will fix.

[Myles C. Maxfield]

Committed r291777 (248805@trunk): <https://commits.webkit.org/248805@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/90737153>