Bug 233310 - [WebGPU] Add converters from serializable descriptors to interface descriptors
Summary: [WebGPU] Add converters from serializable descriptors to interface descriptors
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-18 02:18 PST by Myles C. Maxfield
Modified: 2021-11-19 14:05 PST (History)
3 users (show)

See Also:


Attachments
Patch (302.01 KB, patch)
2021-11-18 02:24 PST, Myles C. Maxfield
dino: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for committing (339.69 KB, patch)
2021-11-18 13:34 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch for committing (339.92 KB, patch)
2021-11-18 20:41 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch for committing (328.00 KB, patch)
2021-11-19 13:08 PST, Myles C. Maxfield
no flags 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 2021-11-18 02:18:17 PST
[WebGPU] Add converters from serializable descriptors to interface descriptors
Comment 1 Myles C. Maxfield 2021-11-18 02:24:46 PST
Created attachment 444654 [details]
Patch
Comment 2 Dean Jackson 2021-11-18 09:18:52 PST
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()
Comment 3 Myles C. Maxfield 2021-11-18 11:43:10 PST
(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).
Comment 4 Myles C. Maxfield 2021-11-18 11:48:34 PST
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.
Comment 5 Myles C. Maxfield 2021-11-18 13:15:45 PST
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.
Comment 6 Myles C. Maxfield 2021-11-18 13:34:34 PST
Created attachment 444730 [details]
Patch for committing
Comment 7 Myles C. Maxfield 2021-11-18 20:41:53 PST
Created attachment 444777 [details]
Patch for committing
Comment 8 EWS 2021-11-18 22:31:03 PST
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].
Comment 9 Radar WebKit Bug Importer 2021-11-18 22:32:22 PST
<rdar://problem/85587265>
Comment 10 Ryan Haddad 2021-11-19 10:52:51 PST
Reverted r286047 for reason:

Breaks internal builds

Committed r286067 (244454@main): <https://commits.webkit.org/244454@main>
Comment 11 Myles C. Maxfield 2021-11-19 13:08:21 PST
Created attachment 444848 [details]
Patch for committing
Comment 12 EWS 2021-11-19 14:05:38 PST
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].