Skip to content

Commit

Permalink
Reland "[webgpu-headers] Define depthWriteEnabled as WGPUOptionalBool"
Browse files Browse the repository at this point in the history
Fixes the crash caused via ApplyClearColorValueWithDrawHelper not
explicitly setting depthWriteEnabled to false.

This is a reland of commit 8c230a7

Original change's description:
> [webgpu-headers] Define depthWriteEnabled as WGPUOptionalBool
>
> The following CLs must be merged before:
> - https://chromium-review.googlesource.com/c/chromium/src/+/5683307
> - https://skia-review.googlesource.com/c/skia/+/874238
>
> webgpu-native/webgpu-headers#308
>
> Bug: 42241167
> Change-Id: Ib6266b6981fb8caf38b734977ceaeeeed2cf55f9
> Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/197214
> Reviewed-by: Austin Eng <[email protected]>
> Commit-Queue: Fr <[email protected]>

Bug: 42241167
Change-Id: I18a1ae28d9df218da25dae57ad64c931e380221e
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/203774
Commit-Queue: Loko Kung <[email protected]>
Reviewed-by: Kai Ninomiya (OOO until 8月23日) <[email protected]>
  • Loading branch information
lokokung authored and Dawn LUCI CQ committed Aug 27, 2024
1 parent f6de1a7 commit fab658d
Show file tree
Hide file tree
Showing 40 changed files with 214 additions and 121 deletions.
3 changes: 2 additions & 1 deletion generator/dawn_json_generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,8 @@ def __init__(self, is_enabled, name, json_data):
value += prefix

if value_name == "undefined":
assert value == 0
if name != "optional bool":
assert value == 0
self.hasUndefined = True
if value != lastValue + 1:
self.contiguousFromZero = False
Expand Down
1 change: 1 addition & 0 deletions generator/templates/api.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
#endif

#define WGPU_BREAKING_CHANGE_DEPTH_CLIP_CONTROL
#define WGPU_BREAKING_CHANGE_DEPTH_WRITE_ENABLED

#include <stdint.h>
#include <stddef.h>
Expand Down
72 changes: 71 additions & 1 deletion generator/templates/api_cpp.h
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,9 @@ static T& AsNonConstReference(const T& value) {
)
{%- endmacro -%}

{% for type in by_category["enum"] %}
//* Although 'optional bool' is defined as an enum value, in C++, we manually implement it to
//* provide conversion utilities.
{% for type in by_category["enum"] if type.name.get() != "optional bool" %}
{% set CppType = as_cppType(type.name) %}
{% set CType = as_cType(type.name) %}
enum class {{CppType}} : uint32_t {
Expand Down Expand Up @@ -153,6 +155,67 @@ class {{BoolCppType}} {
{{BoolCType}} mValue = static_cast<{{BoolCType}}>(false);
};

// Special class for optional booleans in order to allow conversions.
{% set OptionalBool = types["optional bool"] %}
{% set OptionalBoolCppType = as_cppType(OptionalBool.name) %}
{% set OptionalBoolCType = as_cType(OptionalBool.name) %}
{% set OptionalBoolUndefined = as_cEnum(OptionalBool.name, find_by_name(OptionalBool.values, "undefined").name) %}
class {{OptionalBoolCppType}} {
public:
constexpr {{OptionalBoolCppType}}() = default;
// NOLINTNEXTLINE(runtime/explicit) allow implicit construction
constexpr {{OptionalBoolCppType}}(bool value) : mValue(static_cast<{{OptionalBoolCType}}>(value)) {}
// NOLINTNEXTLINE(runtime/explicit) allow implicit construction
constexpr {{OptionalBoolCppType}}(std::optional<bool> value) :
mValue(value ? static_cast<{{OptionalBoolCType}}>(*value) : {{OptionalBoolUndefined}}) {}
// NOLINTNEXTLINE(runtime/explicit) allow implicit construction
constexpr {{OptionalBoolCppType}}({{OptionalBoolCType}} value): mValue(value) {}

// Define the values that are equivalent to the enums.
{% for value in OptionalBool.values %}
static const {{OptionalBoolCppType}} {{as_cppEnum(value.name)}};
{% endfor %}

// Assignment operators.
{{OptionalBoolCppType}}& operator=(const bool& value) {
mValue = static_cast<{{OptionalBoolCType}}>(value);
return *this;
}
{{OptionalBoolCppType}}& operator=(const std::optional<bool>& value) {
mValue = value ? static_cast<{{OptionalBoolCType}}>(*value) : {{OptionalBoolUndefined}};
return *this;
}
{{OptionalBoolCppType}}& operator=(const {{OptionalBoolCType}}& value) {
mValue = value;
return *this;
}

// Conversion functions.
operator {{OptionalBoolCType}}() const { return mValue; }
operator std::optional<bool>() const {
if (mValue == {{OptionalBoolUndefined}}) {
return std::nullopt;
}
return static_cast<bool>(mValue);
}

// Comparison functions.
bool operator==({{OptionalBoolCType}} rhs) const {
return mValue == rhs;
}
bool operator!=({{OptionalBoolCType}} rhs) const {
return mValue != rhs;
}

private:
friend struct std::hash<{{OptionalBoolCppType}}>;
// Default to undefined.
{{OptionalBoolCType}} mValue = {{OptionalBoolUndefined}};
};
{% for value in OptionalBool.values %}
inline const {{OptionalBoolCppType}} {{OptionalBoolCppType}}::{{as_cppEnum(value.name)}} = {{OptionalBoolCppType}}({{as_cEnum(OptionalBool.name, value.name)}});
{% endfor %}

// Helper class to wrap Status which allows implicit conversion to bool.
// Used while callers switch to checking the Status enum instead of booleans.
// TODO(crbug.com/42241199): Remove when all callers check the enum.
Expand Down Expand Up @@ -952,6 +1015,13 @@ struct hash<{{metadata.namespace}}::{{BoolCppType}}> {
return hash<bool>()(v);
}
};
template <>
struct hash<{{metadata.namespace}}::{{OptionalBoolCppType}}> {
public:
size_t operator()(const {{metadata.namespace}}::{{OptionalBoolCppType}} &v) const {
return hash<{{OptionalBoolCType}}>()(v.mValue);
}
};
} // namespace std

#endif // {{PREFIX}}{{API}}_CPP_H_
2 changes: 1 addition & 1 deletion generator/templates/api_cpp_print.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@

namespace {{metadata.namespace}} {

{% for type in by_category["enum"] %}
{% for type in by_category["enum"] if type.name.get() != "optional bool" %}
template <typename CharT, typename Traits>
std::basic_ostream<CharT, Traits>& operator<<(std::basic_ostream<CharT, Traits>& o, {{as_cppType(type.name)}} value) {
switch (value) {
Expand Down
4 changes: 2 additions & 2 deletions generator/templates/dawn/native/ValidationUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ namespace {{native_namespace}} {
{% set namespace = metadata.namespace %}
{% for type in by_category["enum"] %}
MaybeError Validate{{type.name.CamelCase()}}({{namespace}}::{{as_cppType(type.name)}} value) {
switch (value) {
switch ({{as_cType(type.name)}}(value)) {
{% for value in type.values if value.valid %}
case {{namespace}}::{{as_cppType(type.name)}}::{{as_cppEnum(value.name)}}:
case {{as_cEnum(type.name, value.name)}}:
return {};
{% endfor %}
default:
Expand Down
6 changes: 3 additions & 3 deletions generator/templates/dawn/native/api_absl_format.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -85,17 +85,17 @@ namespace {{namespace}} {
absl::FormatSink* s) {
if (spec.conversion_char() == absl::FormatConversionChar::s) {
s->Append("{{as_cppType(type.name)}}::");
switch (value) {
switch ({{as_cType(type.name)}}(value)) {
{% for value in type.values %}
case {{as_cppType(type.name)}}::{{as_cppEnum(value.name)}}:
case {{as_cEnum(type.name, value.name)}}:
s->Append("{{as_cppEnum(value.name)}}");
return {true};
{% endfor %}
default:
break;
}
}
s->Append(absl::StrFormat("%u", static_cast<typename std::underlying_type<{{as_cppType(type.name)}}>::type>(value)));
s->Append(absl::StrFormat("%u", static_cast<{{as_cType(type.name)}}>(value)));
return {true};
}
{% endfor %}
Expand Down
2 changes: 2 additions & 0 deletions src/dawn/common/xlib_with_undefs.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@
#undef Always
#undef Bool
#undef Status
#undef False
#undef True

using XErrorHandler = int (*)(Display*, XErrorEvent*);

Expand Down
10 changes: 9 additions & 1 deletion src/dawn/dawn.json
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,14 @@
"category": "native",
"wasm type": "i"
},
"optional bool": {
"category": "enum",
"values": [
{"value": 0, "name": "false"},
{"value": 1, "name": "true"},
{"value": 2, "name": "undefined", "jsrepr": "undefined"}
]
},
"string view": {
"category": "structure",
"members": [
Expand Down Expand Up @@ -3810,7 +3818,7 @@
"extensible": "in",
"members": [
{"name": "format", "type": "texture format"},
{"name": "depth write enabled", "type": "bool", "default": "false"},
{"name": "depth write enabled", "type": "optional bool", "default": "undefined"},
{"name": "depth compare", "type": "compare function", "default": "undefined"},
{"name": "stencil front", "type": "stencil face state"},
{"name": "stencil back", "type": "stencil face state"},
Expand Down
1 change: 1 addition & 0 deletions src/dawn/native/ApplyClearColorValueWithDrawHelper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ ResultOrError<RenderPipelineBase*> GetOrCreateApplyClearValueWithDrawPipeline(
renderPipelineDesc.fragment = &fragment;
renderPipelineDesc.multisample.count = key.sampleCount;
DepthStencilState depthStencilState = {};
depthStencilState.depthWriteEnabled = false;
if (key.depthStencilFormat != wgpu::TextureFormat::Undefined) {
depthStencilState.format = key.depthStencilFormat;
renderPipelineDesc.depthStencil = &depthStencilState;
Expand Down
4 changes: 2 additions & 2 deletions src/dawn/native/BlitBufferToDepthStencil.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ ResultOrError<Ref<RenderPipelineBase>> GetOrCreateRG8ToDepth16UnormPipeline(Devi

DepthStencilState dsState = {};
dsState.format = wgpu::TextureFormat::Depth16Unorm;
dsState.depthWriteEnabled = true;
dsState.depthWriteEnabled = wgpu::OptionalBool::True;
dsState.depthCompare = wgpu::CompareFunction::Always;

RenderPipelineDescriptor renderPipelineDesc = {};
Expand Down Expand Up @@ -226,7 +226,7 @@ ResultOrError<InternalPipelineStore::BlitR8ToStencilPipelines> GetOrCreateR8ToSt

DepthStencilState dsState = {};
dsState.format = format;
dsState.depthWriteEnabled = false;
dsState.depthWriteEnabled = wgpu::OptionalBool::False;
dsState.depthCompare = wgpu::CompareFunction::Always;
dsState.stencilFront.passOp = wgpu::StencilOperation::Replace;

Expand Down
2 changes: 1 addition & 1 deletion src/dawn/native/BlitColorToColorWithDraw.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ ResultOrError<Ref<RenderPipelineBase>> GetOrCreateExpandMultisamplePipeline(
DepthStencilState depthStencilState = {};
if (pipelineKey.depthStencilFormat != wgpu::TextureFormat::Undefined) {
depthStencilState.format = pipelineKey.depthStencilFormat;
depthStencilState.depthWriteEnabled = false;
depthStencilState.depthWriteEnabled = wgpu::OptionalBool::False;
depthStencilState.depthCompare = wgpu::CompareFunction::Always;

renderPipelineDesc.depthStencil = &depthStencilState;
Expand Down
2 changes: 1 addition & 1 deletion src/dawn/native/BlitDepthToDepth.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ ResultOrError<Ref<RenderPipelineBase>> GetOrCreateDepthBlitPipeline(DeviceBase*

DepthStencilState dsState = {};
dsState.format = format;
dsState.depthWriteEnabled = true;
dsState.depthWriteEnabled = wgpu::OptionalBool::True;
dsState.depthCompare = wgpu::CompareFunction::Always;

RenderPipelineDescriptor renderPipelineDesc = {};
Expand Down
27 changes: 12 additions & 15 deletions src/dawn/native/RenderPipeline.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -337,23 +337,19 @@ MaybeError ValidateDepthStencilState(const DeviceBase* device,

DAWN_INVALID_IF(
format->HasDepth() && descriptor->depthCompare == wgpu::CompareFunction::Undefined &&
(descriptor->depthWriteEnabled ||
(descriptor->depthWriteEnabled == wgpu::OptionalBool::True ||
descriptor->stencilFront.depthFailOp != wgpu::StencilOperation::Keep ||
descriptor->stencilBack.depthFailOp != wgpu::StencilOperation::Keep),
"Depth stencil format (%s) has a depth aspect and depthCompare is %s while it's actually "
"used by depthWriteEnabled (%u), or stencil front depth fail operation (%s), or "
"used by depthWriteEnabled (%s), or stencil front depth fail operation (%s), or "
"stencil back depth fail operation (%s).",
descriptor->format, wgpu::CompareFunction::Undefined, descriptor->depthWriteEnabled,
descriptor->stencilFront.depthFailOp, descriptor->stencilBack.depthFailOp);

UnpackedPtr<DepthStencilState> unpacked;
DAWN_TRY_ASSIGN(unpacked, ValidateAndUnpack(descriptor));
if (const auto* depthWriteDefined = unpacked.Get<DepthStencilStateDepthWriteDefinedDawn>()) {
DAWN_INVALID_IF(
format->HasDepth() && !depthWriteDefined->depthWriteDefined,
"Depth stencil format (%s) has a depth aspect and depthWriteEnabled is undefined.",
descriptor->format);
}
DAWN_INVALID_IF(
format->HasDepth() && descriptor->depthWriteEnabled == wgpu::OptionalBool::Undefined,
"Depth stencil format (%s) has a depth aspect and depthWriteEnabled is undefined.",
descriptor->format);

DAWN_INVALID_IF(
!format->HasDepth() && descriptor->depthCompare != wgpu::CompareFunction::Always &&
Expand All @@ -364,8 +360,9 @@ MaybeError ValidateDepthStencilState(const DeviceBase* device,
wgpu::CompareFunction::Undefined);

DAWN_INVALID_IF(
!format->HasDepth() && descriptor->depthWriteEnabled,
"Depth stencil format (%s) doesn't have depth aspect while depthWriteEnabled (%u) is true.",
!format->HasDepth() && descriptor->depthWriteEnabled == wgpu::OptionalBool::True,
"Depth stencil format (%s) doesn't have depth aspect while depthWriteEnabled (%s) "
"is true.",
descriptor->format, descriptor->depthWriteEnabled);

if (!format->HasStencil()) {
Expand Down Expand Up @@ -978,16 +975,16 @@ RenderPipelineBase::RenderPipelineBase(DeviceBase* device,
// Reify depth option for stencil-only formats
const Format& format = device->GetValidInternalFormat(mDepthStencil.format);
if (!format.HasDepth()) {
mDepthStencil.depthWriteEnabled = false;
mDepthStencil.depthWriteEnabled = wgpu::OptionalBool::False;
mDepthStencil.depthCompare = wgpu::CompareFunction::Always;
}
if (format.HasDepth() && mDepthStencil.depthCompare == wgpu::CompareFunction::Undefined &&
!mDepthStencil.depthWriteEnabled &&
mDepthStencil.depthWriteEnabled != wgpu::OptionalBool::True &&
mDepthStencil.stencilFront.depthFailOp == wgpu::StencilOperation::Keep &&
mDepthStencil.stencilBack.depthFailOp == wgpu::StencilOperation::Keep) {
mDepthStencil.depthCompare = wgpu::CompareFunction::Always;
}
mWritesDepth = mDepthStencil.depthWriteEnabled;
mWritesDepth = mDepthStencil.depthWriteEnabled == wgpu::OptionalBool::True;
if (mDepthStencil.stencilWriteMask) {
if ((mPrimitive.cullMode != wgpu::CullMode::Front &&
(mDepthStencil.stencilFront.failOp != wgpu::StencilOperation::Keep ||
Expand Down
12 changes: 7 additions & 5 deletions src/dawn/native/d3d11/RenderPipelineD3D11.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -405,11 +405,13 @@ MaybeError RenderPipeline::InitializeDepthStencilState() {
const DepthStencilState* state = GetDepthStencilState();

D3D11_DEPTH_STENCIL_DESC depthStencilDesc = {};
depthStencilDesc.DepthEnable =
(state->depthCompare == wgpu::CompareFunction::Always && !state->depthWriteEnabled) ? FALSE
: TRUE;
depthStencilDesc.DepthWriteMask =
state->depthWriteEnabled ? D3D11_DEPTH_WRITE_MASK_ALL : D3D11_DEPTH_WRITE_MASK_ZERO;
depthStencilDesc.DepthEnable = (state->depthCompare == wgpu::CompareFunction::Always &&
state->depthWriteEnabled != wgpu::OptionalBool::True)
? FALSE
: TRUE;
depthStencilDesc.DepthWriteMask = state->depthWriteEnabled == wgpu::OptionalBool::True
? D3D11_DEPTH_WRITE_MASK_ALL
: D3D11_DEPTH_WRITE_MASK_ZERO;
depthStencilDesc.DepthFunc = ToD3D11ComparisonFunc(state->depthCompare);

depthStencilDesc.StencilEnable = UsesStencil() ? TRUE : FALSE;
Expand Down
5 changes: 3 additions & 2 deletions src/dawn/native/d3d12/RenderPipelineD3D12.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -555,11 +555,12 @@ D3D12_DEPTH_STENCIL_DESC RenderPipeline::ComputeDepthStencilDesc() {
D3D12_DEPTH_STENCIL_DESC depthStencilDescriptor = {};
depthStencilDescriptor.DepthEnable =
(descriptor->depthCompare == wgpu::CompareFunction::Always &&
!descriptor->depthWriteEnabled)
descriptor->depthWriteEnabled != wgpu::OptionalBool::True)
? FALSE
: TRUE;
depthStencilDescriptor.DepthWriteMask =
descriptor->depthWriteEnabled ? D3D12_DEPTH_WRITE_MASK_ALL : D3D12_DEPTH_WRITE_MASK_ZERO;
descriptor->depthWriteEnabled == wgpu::OptionalBool::True ? D3D12_DEPTH_WRITE_MASK_ALL
: D3D12_DEPTH_WRITE_MASK_ZERO;
depthStencilDescriptor.DepthFunc = ToD3D12ComparisonFunc(descriptor->depthCompare);

depthStencilDescriptor.StencilEnable = UsesStencil() ? TRUE : FALSE;
Expand Down
3 changes: 2 additions & 1 deletion src/dawn/native/metal/RenderPipelineMTL.mm
Original file line number Diff line number Diff line change
Expand Up @@ -546,7 +546,8 @@ MTLCullMode ToMTLCullMode(wgpu::CullMode mode) {

mtlDepthStencilDescriptor.depthCompareFunction =
ToMetalCompareFunction(descriptor->depthCompare);
mtlDepthStencilDescriptor.depthWriteEnabled = descriptor->depthWriteEnabled;
mtlDepthStencilDescriptor.depthWriteEnabled =
descriptor->depthWriteEnabled == wgpu::OptionalBool::True;

if (UsesStencil()) {
NSRef<MTLStencilDescriptor> backFaceStencilRef = AcquireNSRef([MTLStencilDescriptor new]);
Expand Down
4 changes: 2 additions & 2 deletions src/dawn/native/opengl/RenderPipelineGL.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -345,13 +345,13 @@ void RenderPipeline::ApplyDepthStencilState(const OpenGLFunctions& gl,

// Depth writes only occur if depth is enabled
if (descriptor->depthCompare == wgpu::CompareFunction::Always &&
!descriptor->depthWriteEnabled) {
descriptor->depthWriteEnabled != wgpu::OptionalBool::True) {
gl.Disable(GL_DEPTH_TEST);
} else {
gl.Enable(GL_DEPTH_TEST);
}

if (descriptor->depthWriteEnabled) {
if (descriptor->depthWriteEnabled == wgpu::OptionalBool::True) {
gl.DepthMask(GL_TRUE);
} else {
gl.DepthMask(GL_FALSE);
Expand Down
5 changes: 3 additions & 2 deletions src/dawn/native/vulkan/RenderPipelineVk.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -656,10 +656,11 @@ VkPipelineDepthStencilStateCreateInfo RenderPipeline::ComputeDepthStencilDesc()
// Depth writes only occur if depth is enabled
depthStencilState.depthTestEnable =
(descriptor->depthCompare == wgpu::CompareFunction::Always &&
!descriptor->depthWriteEnabled)
descriptor->depthWriteEnabled != wgpu::OptionalBool::True)
? VK_FALSE
: VK_TRUE;
depthStencilState.depthWriteEnable = descriptor->depthWriteEnabled ? VK_TRUE : VK_FALSE;
depthStencilState.depthWriteEnable =
descriptor->depthWriteEnabled == wgpu::OptionalBool::True ? VK_TRUE : VK_FALSE;
depthStencilState.depthCompareOp = ToVulkanCompareOp(descriptor->depthCompare);
depthStencilState.depthBoundsTestEnable = false;
depthStencilState.minDepthBounds = 0.0f;
Expand Down
1 change: 1 addition & 0 deletions src/dawn/native/vulkan/ResolveTextureLoadingUtilsVk.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ ResultOrError<Ref<RenderPipelineBase>> GetOrCreateColorBlitPipeline(
DepthStencilState depthStencilState = {};
if (pipelineKey.depthStencilFormat != wgpu::TextureFormat::Undefined) {
depthStencilState.format = pipelineKey.depthStencilFormat;
depthStencilState.depthWriteEnabled = wgpu::OptionalBool::False;

renderPipelineDesc.depthStencil = &depthStencilState;
}
Expand Down
9 changes: 5 additions & 4 deletions src/dawn/node/binding/Converter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -922,10 +922,6 @@ bool Converter::Convert(wgpu::ColorTargetState& out, const interop::GPUColorTarg
bool Converter::Convert(wgpu::DepthStencilState& out, const interop::GPUDepthStencilState& in) {
out = {};

auto depthWriteDefined = Allocate<wgpu::DepthStencilStateDepthWriteDefinedDawn>();
depthWriteDefined->depthWriteDefined = in.depthWriteEnabled.has_value();
out.nextInChain = depthWriteDefined;

return Convert(out.format, in.format) && Convert(out.depthWriteEnabled, in.depthWriteEnabled) &&
Convert(out.depthCompare, in.depthCompare) &&
Convert(out.stencilFront, in.stencilFront) && Convert(out.stencilBack, in.stencilBack) &&
Expand Down Expand Up @@ -1734,6 +1730,11 @@ bool Converter::Convert(wgpu::Bool& out, const bool& in) {
return true;
}

bool Converter::Convert(wgpu::OptionalBool& out, const std::optional<bool>& in) {
out = in;
return true;
}

char* Converter::ConvertStringReplacingNull(std::string_view in) {
char* out = Allocate<char>(in.size() + 1);
out[in.size()] = '\0';
Expand Down
Loading

0 comments on commit fab658d

Please sign in to comment.