Bug 233310 – [WebGPU] Add converters from serializable descriptors to interface descriptors

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

[WebGPU] Add converters from serializable descriptors to interface descriptors

[Myles C. Maxfield]

Created attachment 444654 [details]
Patch

[Dean Jackson]

Comment on attachment 444654 [details]
Patch

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

Can you test any of this? Or, what is your plan for testing?

> Source/WebKit/Shared/WebGPU/WebGPUCompilationMessage.cpp:38
> +    return PAL::WebGPU::CompilationMessage::create(String(compilationMessage.message), compilationMessage.type, compilationMessage.lineNum, compilationMessage.linePos, compilationMessage.offset, compilationMessage.length);

Surprised you have to explicitly create a new String here. What is CompilationMessage.message?

> Source/WebKit/Shared/WebGPU/WebGPUComputePassTimestampWrites.cpp:53
> +    for (const auto& backingTimestampWrite : computePassTimestampWrites) {
> +        auto timestampWrite = convertFromBacking(backingTimestampWrite);
> +        if (!timestampWrite)
> +            return std::nullopt;
> +    }

Did you forget to append the timestampWrite to timestampWrites?

> Source/WebKit/Shared/WebGPU/WebGPUExternalTextureBindingLayout.cpp:38
> +    return { { } };

??

> Source/WebKit/Shared/WebGPU/WebGPURenderPassDescriptor.cpp:56
> +    if (renderPassDescriptor.depthStencilAttachment && !depthStencilAttachment)

I think if renderPassDescriptor.depthStencilAttachment wasn't null, then depthStencilAttachment also won't be? Or could convertFromBacking still return a nullopt? i.e. can you just test depthStencilAttachment?

> Source/WebKit/Shared/WebGPU/WebGPURenderPassTimestampWrites.cpp:53
> +    for (const auto& backingTimestampWrite : renderPassTimestampWrites) {
> +        auto timestampWrite = convertFromBacking(backingTimestampWrite);
> +        if (!timestampWrite)
> +            return std::nullopt;
> +    }

You forgot to populate timestampWrites.

> Source/WebKit/Shared/WebGPU/WebGPURenderPipelineDescriptor.cpp:73
> +    auto fragment = ([&] () -> std::optional<PAL::WebGPU::FragmentState> {
> +        if (renderPipelineDescriptor.fragment)
> +            return convertFromBacking(*renderPipelineDescriptor.fragment);
> +        return std::nullopt;
> +    })();
> +    if (renderPipelineDescriptor.fragment && !fragment)
> +        return std::nullopt;

Same question as above - is it enough to simply test fragment? Or early return if !rPD.fragment?

> Source/WebKit/Shared/WebGPU/WebGPUVertexState.cpp:48
> +        if (backingBuffer) {
> +            auto buffer = convertFromBacking(*backingBuffer);
> +            if (!buffer)
> +                return std::nullopt;

Missing buffers.uncheckedAppend(buffer)?

Also, since you still append in the zero-buffer case, maybe this whole loop could be replaced with a vertexState.buffers.map()

[Myles C. Maxfield]

(In reply to Dean Jackson from comment #2)
> Can you test any of this? Or, what is your plan for testing?

Yes. The plan is to use the WebGPU Conformance Test Suit. This set of patches I'm writing now is the foundation - I'm getting the code mostly written mostly correctly before we go through all the CTS tests and make them pass (and likely add many tests to the CTS too).

[Myles C. Maxfield]

Comment on attachment 444654 [details]
Patch

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

>> Source/WebKit/Shared/WebGPU/WebGPUCompilationMessage.cpp:38
>> +    return PAL::WebGPU::CompilationMessage::create(String(compilationMessage.message), compilationMessage.type, compilationMessage.lineNum, compilationMessage.linePos, compilationMessage.offset, compilationMessage.length);
> 
> Surprised you have to explicitly create a new String here. What is CompilationMessage.message?

It's also a string. CompilationMessage::create() takes a String&&. I should just make an overload that takes a const String& so the caller doesn't have to do this.

>> Source/WebKit/Shared/WebGPU/WebGPUComputePassTimestampWrites.cpp:53
>> +    }
> 
> Did you forget to append the timestampWrite to timestampWrites?

I'll switch all these to use Vector::map().

>> Source/WebKit/Shared/WebGPU/WebGPUExternalTextureBindingLayout.cpp:38
>> +    return { { } };
> 
> ??

ExternalTextureBindingLayout has no fields in it. https://gpuweb.github.io/gpuweb/#dictdef-gpuexternaltexturebindinglayout

>> Source/WebKit/Shared/WebGPU/WebGPURenderPassDescriptor.cpp:56
>> +    if (renderPassDescriptor.depthStencilAttachment && !depthStencilAttachment)
> 
> I think if renderPassDescriptor.depthStencilAttachment wasn't null, then depthStencilAttachment also won't be? Or could convertFromBacking still return a nullopt? i.e. can you just test depthStencilAttachment?

convertFromBacking() can return null.

[Myles C. Maxfield]

Comment on attachment 444654 [details]
Patch

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

>>> Source/WebKit/Shared/WebGPU/WebGPUComputePassTimestampWrites.cpp:53
>>> +    }
>> 
>> Did you forget to append the timestampWrite to timestampWrites?
> 
> I'll switch all these to use Vector::map().

Oh, map() actually won't work for these, because if any member fails to convert, the map operation has to abort. I've converted some other places to use map(), though.

[Myles C. Maxfield]

Created attachment 444730 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 444777 [details]
Patch for committing

[EWS]

Committed r286047 (244434@main): <https://commits.webkit.org/244434@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444777 [details].

[Radar WebKit Bug Importer]

<rdar://problem/85587265>

[Ryan Haddad]

Reverted r286047 for reason:

Breaks internal builds

Committed r286067 (244454@main): <https://commits.webkit.org/244454@main>

[Myles C. Maxfield]

Created attachment 444848 [details]
Patch for committing

[EWS]

Committed r286077 (244464@main): <https://commits.webkit.org/244464@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444848 [details].