Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

depth-clip-control feature is unimplemented #10288

Merged
merged 1 commit into from Feb 23, 2023
Merged

depth-clip-control feature is unimplemented #10288

merged 1 commit into from Feb 23, 2023

Conversation

mwyrzykowski
Copy link
Contributor

@mwyrzykowski mwyrzykowski commented Feb 17, 2023 •
edited by webkit-early-warning-system

1a696df

depth-clip-control feature is unimplemented
https://bugs.webkit.org/show_bug.cgi?id=251664
<radar://104992830>

Reviewed by Myles C. Maxfield.

Implement depth clip control which controls if fragments
with depth values outside of [0, 1] are clipped or not.

* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceImpl.cpp:
(PAL::WebGPU::convertToBacking):

* Source/WebGPU/WebGPU/HardwareCapabilities.mm:
(WebGPU::baseFeatures):
All devices support depth clamping.

* Source/WebGPU/WebGPU/RenderPassEncoder.mm:
(WebGPU::RenderPassEncoder::setPipeline):
(WebGPU::RenderPassEncoder::setStencilReference):
* Source/WebGPU/WebGPU/RenderPipeline.h:
(WebGPU::RenderPipeline::create):
(WebGPU::RenderPipeline::depthClipMode const):
* Source/WebGPU/WebGPU/RenderPipeline.mm:
(WebGPU::Device::createRenderPipeline):
(WebGPU::RenderPipeline::RenderPipeline):
Pass the feature through to the command encoder.

* Websites/webkit.org/demos/webgpu/scripts/two-cubes.js:
Update two-cubes to illustrate this feature.

Canonical link: https://commits.webkit.org/260723@main

664a490

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ❌ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ❌ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 watch ✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge ✅ 🛠 watch-sim

@mwyrzykowski mwyrzykowski self-assigned this Feb 17, 2023
@mwyrzykowski mwyrzykowski added the WebGPU For bugs in WebGPU label Feb 17, 2023
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 7d37024)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ❌ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 watch ✅ 🧪 mac-wk2-stress
✅ 🛠 watch-sim

djg
djg approved these changes Feb 17, 2023
View reviewed changes
Copy link
Contributor

@djg djg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

Source/WebGPU/WebGPU/RenderPipeline.h Outdated
@@ -43,14 +43,14 @@ class PipelineLayout;
class RenderPipeline : public WGPURenderPipelineImpl, public RefCounted<RenderPipeline> {
WTF_MAKE_FAST_ALLOCATED;
public:
static Ref<RenderPipeline> create(id<MTLRenderPipelineState> renderPipelineState, MTLPrimitiveType primitiveType, std::optional<MTLIndexType> indexType, MTLWinding frontFace, MTLCullMode cullMode, MTLDepthStencilDescriptor *depthStencilDescriptor, MTLRenderPipelineReflection *reflection, Device& device)
static Ref<RenderPipeline> create(id<MTLRenderPipelineState> renderPipelineState, MTLPrimitiveType primitiveType, std::optional<MTLIndexType> indexType, MTLWinding frontFace, MTLCullMode cullMode, MTLDepthClipMode depthClipMode, MTLDepthStencilDescriptor *depthStencilDescriptor, MTLRenderPipelineReflection *reflection, Device& device)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it preferable to keep adding arguments to create calls or should we pass arguments via a descriptor struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if RenderPipeline::create was more widely used I would say descriptor struct for sure but let me remove one of the overloads since I think that is not needed

@mwyrzykowski mwyrzykowski force-pushed the eng/WebGPU-depth-clip-control-feature-is-unimplemented branch 2 times, most recently from b770e7b to b8a5d94 Compare February 21, 2023 21:41
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash b8a5d94)

Misc iOS, tvOS & watchOS macOS Linux Windows
❌ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 watch ✅ 🧪 mac-wk2-stress
✅ 🛠 watch-sim

@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash b770e7b)

Misc iOS, tvOS & watchOS macOS Linux Windows
⏳ 🧪 style ⏳ 🛠 ios ⏳ 🛠 mac ✅ 🛠 wpe ⏳ 🛠 wincairo
⏳ 🧪 bindings ⏳ 🛠 ios-sim ❌ 🛠 mac-AS-debug ✅ 🛠 gtk
⏳ 🧪 webkitperl ⏳ 🧪 ios-wk2 ⏳ 🧪 api-mac   🧪 gtk-wk2
⏳ 🧪 webkitpy ⏳ 🧪 api-ios ⏳ 🧪 mac-wk1   🧪 api-gtk
⏳ 🛠 🧪 jsc ❌ 🛠 tv ⏳ 🧪 mac-wk2 ⏳ 🛠 jsc-armv7
⏳ 🛠 🧪 jsc-arm64 ⏳ 🛠 tv-sim ❌ 🧪 mac-AS-debug-wk2 ⏳ 🧪 jsc-armv7-tests
⏳ 🧪 services ⏳ 🛠 watch ⏳ 🧪 mac-wk2-stress ⏳ 🛠 jsc-mips
❌ 🛠 watch-sim ⏳ 🧪 jsc-mips-tests

litherum
litherum approved these changes Feb 22, 2023
View reviewed changes
Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceImpl.cpp
@@ -474,7 +481,7 @@ static auto convertToBacking(const RenderPipelineDescriptor& descriptor, Convert
static_cast<uint32_t>(backingBuffers.size()),
backingBuffers.data(),
}, {
nullptr,
descriptor.primitive && descriptor.primitive->unclippedDepth ? &depthClipControl.chain : nullptr,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should only do this if the clip control feature is enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I just discovered WebGPU requires opt-in for features

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add this validation to Device::createRenderPipeline so we return an invalid pipeline when the feature is not enabled and unclippedDepth == true which is what I believe the specification requires

Source/WebGPU/WebGPU/RenderPipeline.h
static Ref<RenderPipeline> create(id<MTLRenderPipelineState> renderPipelineState, MTLPrimitiveType primitiveType, std::optional<MTLIndexType> indexType, MTLWinding frontFace, MTLCullMode cullMode, MTLDepthStencilDescriptor *depthStencilDescriptor, MTLRenderPipelineReflection *reflection, Device& device)
static Ref<RenderPipeline> create(id<MTLRenderPipelineState> renderPipelineState, MTLPrimitiveType primitiveType, std::optional<MTLIndexType> indexType, MTLWinding frontFace, MTLCullMode cullMode, MTLDepthClipMode depthClipMode, MTLDepthStencilDescriptor *depthStencilDescriptor, MTLRenderPipelineReflection *reflection, const PipelineLayout *pipelineLayout, Device& device)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is getting pretty unwieldy, can we batch these together in a struct or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could pass the WGPURenderPipelineDescriptor and let the constructor do the unpacking. What do you think of that idea?

Source/WebGPU/WebGPU/RenderPipeline.mm Outdated
return RenderPipeline::createInvalid(*this);
MTLDepthClipMode mtlDepthClipMode = MTLDepthClipModeClip;
if (descriptor.primitive.nextInChain) {
if (descriptor.primitive.nextInChain->sType != WGPUSType_PrimitiveDepthClipControl)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

|| descriptor.primitive.nextInChain->nextInChain

Source/WebGPU/WebGPU/RenderPipeline.mm
Comment on lines +462 to +466
const PipelineLayout* pipelineLayout = nullptr;
if (descriptor.layout) {
if (auto& layout = WebGPU::fromAPI(descriptor.layout); layout.numberOfBindGroupLayouts())
pipelineLayout = &layout;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unrelated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is to avoid two ::create functions with almost identical parameters

Websites/webkit.org/demos/webgpu/scripts/two-cubes.js
@@ -182,7 +181,8 @@ async function helloCube() {
fragment: fragmentStageDescriptor,
primitive: {
topology: "triangle-list",
cullMode: "back"
cullMode: "back",
unclippedDepth: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we have to enable the feature at device creation time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that is correct. I will fix that

Source/WebGPU/WebGPU/RenderPipeline.mm
Comment on lines +447 to +448
if (depthClipControl->unclippedDepth)
mtlDepthClipMode = MTLDepthClipModeClamp;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have to do any workarounds or anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extent of the workaround is the WGSL compiler should clamp fragDepth, when written, to the viewport z-bounds. I will file a bug for this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://bugs.webkit.org/show_bug.cgi?id=252729

@mwyrzykowski mwyrzykowski force-pushed the eng/WebGPU-depth-clip-control-feature-is-unimplemented branch from b8a5d94 to 664a490 Compare February 22, 2023 08:20
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash 664a490)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ❌ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ❌ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 watch ✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge ✅ 🛠 watch-sim

@mwyrzykowski mwyrzykowski added the merge-queue Applied to send a pull request to merge-queue label Feb 23, 2023
@mwyrzykowski
depth-clip-control feature is unimplemented
1a696df 
https://bugs.webkit.org/show_bug.cgi?id=251664
<radar://104992830>

Reviewed by Myles C. Maxfield.

Implement depth clip control which controls if fragments
with depth values outside of [0, 1] are clipped or not.

* Source/WebCore/PAL/pal/graphics/WebGPU/Impl/WebGPUDeviceImpl.cpp:
(PAL::WebGPU::convertToBacking):

* Source/WebGPU/WebGPU/HardwareCapabilities.mm:
(WebGPU::baseFeatures):
All devices support depth clamping.

* Source/WebGPU/WebGPU/RenderPassEncoder.mm:
(WebGPU::RenderPassEncoder::setPipeline):
(WebGPU::RenderPassEncoder::setStencilReference):
* Source/WebGPU/WebGPU/RenderPipeline.h:
(WebGPU::RenderPipeline::create):
(WebGPU::RenderPipeline::depthClipMode const):
* Source/WebGPU/WebGPU/RenderPipeline.mm:
(WebGPU::Device::createRenderPipeline):
(WebGPU::RenderPipeline::RenderPipeline):
Pass the feature through to the command encoder.

* Websites/webkit.org/demos/webgpu/scripts/two-cubes.js:
Update two-cubes to illustrate this feature.

Canonical link: https://commits.webkit.org/260723@main
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/WebGPU-depth-clip-control-feature-is-unimplemented branch from 664a490 to 1a696df Compare February 23, 2023 04:56
@webkit-early-warning-system webkit-early-warning-system merged commit 1a696df into WebKit:main Feb 23, 2023
@webkit-commit-queue
Copy link
Collaborator

Committed 260723@main (1a696df): https://commits.webkit.org/260723@main

Reviewed commits have been landed. Closing PR #10288 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djg djg djg approved these changes

@litherum litherum litherum approved these changes

@tadeuzagallo tadeuzagallo Awaiting requested review from tadeuzagallo tadeuzagallo is a code owner

Assignees

@mwyrzykowski mwyrzykowski

Labels
WebGPU For bugs in WebGPU
Projects
None yet
Milestone
No milestone
5 participants
@mwyrzykowski @webkit-early-warning-system @webkit-commit-queue @djg @litherum