Skip to content

Commit

Permalink
Fix context-attributes-alpha-depth-stencil-antialias.html.
Browse files Browse the repository at this point in the history
The WebGL conformance test:
conformance/context/
context-attributes-alpha-depth-stencil-antialias.html

has been failing in Chrome since the test was expanded in:
KhronosGroup/WebGL#3326

When the user requests depth:false and Chrome uses a combined
depth/stencil attachment for the default framebuffer, Chrome was
incorrectly allowing the depth test to affect rendering state.

Fixed: chromium:1276153
Change-Id: Ia5bdcfe6a2d6964fbbd84669245f7ae66f64058c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4189043
Reviewed-by: Brandon Jones <bajones@chromium.org>
Auto-Submit: Kenneth Russell <kbr@chromium.org>
Commit-Queue: Brandon Jones <bajones@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1096018}
  • Loading branch information
kenrussell authored and Chromium LUCI CQ committed Jan 24, 2023
1 parent ab23b08 commit d608040
Show file tree
Hide file tree
Showing 9 changed files with 60 additions and 33 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,6 @@ crbug.com/891953 [ android ] WebglExtension_OVR_multiview2 [ Failure ]
crbug.com/1131224 conformance2/rendering/framebuffer-mismatched-attachment-targets.html [ Failure ]
crbug.com/1108086 [ no-passthrough ] conformance2/renderbuffers/framebuffer-object-attachment.html [ Failure ]
crbug.com/angleproject/4807 [ win angle-d3d11 passthrough ] conformance2/glsl3/switch-case.html [ Failure ]
crbug.com/1276153 conformance/context/context-attributes-alpha-depth-stencil-antialias.html [ Failure ]
# Failing at least on Pixel 4, both validating and passthrough
crbug.com/1336010 [ android angle-disabled no-passthrough ] conformance2/extensions/oes-draw-buffers-indexed.html [ Failure ]
# Failing on at least NVIDIA and Intel
Expand Down Expand Up @@ -671,8 +670,7 @@ crbug.com/1221362 [ chromeos chromeos-board-amd64-generic ] conformance2/renderi
crbug.com/1223542 [ chromeos chromeos-board-amd64-generic ] deqp/functional/gles3/framebufferblit/rect_03.html [ Failure ]
crbug.com/1223542 [ chromeos chromeos-board-amd64-generic ] deqp/functional/gles3/framebufferblit/rect_04.html [ Failure ]

# TODO(crbug.com/1276153) uncomment after fix for updated part of test applies
# crbug.com/1232102 [ chromeos chromeos-board-amd64-generic no-passthrough ] conformance/context/context-attributes-alpha-depth-stencil-antialias.html [ Failure ]
crbug.com/1232102 [ chromeos chromeos-board-amd64-generic no-passthrough ] conformance/context/context-attributes-alpha-depth-stencil-antialias.html [ Failure ]

crbug.com/1232106 [ chromeos chromeos-board-amd64-generic no-passthrough ] conformance/extensions/webgl-compressed-texture-astc.html [ Failure ]
crbug.com/1232118 [ angle-opengles chromeos chromeos-board-amd64-generic passthrough ] conformance/uniforms/uniform-samplers-test.html [ Failure ]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -307,9 +307,6 @@ crbug.com/1175371 [ win ] conformance/extensions/khr-parallel-shader-compile.htm
# Won't investigate failure on validating command decoder. Remove once it's unshipped.
crbug.com/angleproject/5499 [ no-passthrough ] conformance/glsl/misc/shaders-with-name-conflicts.html [ Failure ]

# Skipping new tests
crbug.com/1276153 conformance/context/context-attributes-alpha-depth-stencil-antialias.html [ Failure ]

# Need to implement new error semantics
# https://github.com/KhronosGroup/WebGL/pull/2607
crbug.com/849572 [ win angle-d3d11 passthrough ] conformance/extensions/angle-instanced-arrays-out-of-bounds.html [ Failure ]
Expand All @@ -331,8 +328,7 @@ crbug.com/angleproject/6358 [ no-passthrough ] conformance/programs/program-test

# Win / Intel / Vulkan / Passthrough command decoder
# Technically flaky, but flake rate is too high for RetryOnFailure.
# TODO(crbug.com/1276153) uncomment after fix for updated part of test applies
# crbug.com/angleproject/4922 [ win intel angle-vulkan passthrough ] conformance/context/context-attributes-alpha-depth-stencil-antialias.html [ Failure ]
crbug.com/angleproject/4922 [ win intel angle-vulkan passthrough ] conformance/context/context-attributes-alpha-depth-stencil-antialias.html [ Failure ]

####################
# Fuchsia failures #
Expand All @@ -346,8 +342,7 @@ crbug.com/1371306 [ fuchsia fuchsia-board-sherlock ] conformance/textures/misc/t
[ fuchsia fuchsia-board-sherlock ] WebglExtension_EXT_float_blend [ Failure ]

# Anti-aliasing disabled on Fuchsia
# TODO(crbug.com/1276153) uncomment after fix for updated part of test applies
# [ fuchsia ] conformance/context/context-attributes-alpha-depth-stencil-antialias.html [ Failure ]
[ fuchsia ] conformance/context/context-attributes-alpha-depth-stencil-antialias.html [ Failure ]

####################
# Win failures #
Expand Down Expand Up @@ -428,12 +423,6 @@ crbug.com/982294 [ mac passthrough nvidia angle-opengl ] conformance/glsl/sample
# No active browser window, but no crash stack.
crbug.com/1240443 [ mac apple-apple-m1 passthrough angle-opengl ] conformance/renderbuffers/stencil-renderbuffer-initialization.html [ Failure ]

## Mac / M1 / Metal ##

# Introduced by upstreaming Apple's direct-to-metal backend
# TODO(crbug.com/1276153) uncomment after fix for updated part of test applies
# crbug.com/angleproject/5505 [ mac passthrough angle-metal apple-angle-metal-renderer:-apple-m1 ] conformance/context/context-attributes-alpha-depth-stencil-antialias.html [ Failure ]

####################
# Linux failures #
####################
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ void OVRMultiview2::framebufferTextureMultiviewOVR(GLenum target,
framebuffer_binding->SetAttachmentForBoundFramebuffer(
target, attachment, textarget, texture, level, base_view_index,
num_views);
scoped.Context()->ApplyStencilTest();
scoped.Context()->ApplyDepthAndStencilTest();
}

bool OVRMultiview2::Supported(WebGLRenderingContextBase* context) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -546,7 +546,7 @@ void WebGL2RenderingContextBase::framebufferTextureLayer(GLenum target,
}
framebuffer_binding->SetAttachmentForBoundFramebuffer(
target, attachment, textarget, texture, level, layer, 0);
ApplyStencilTest();
ApplyDepthAndStencilTest();
}

ScriptValue WebGL2RenderingContextBase::getInternalformatParameter(
Expand Down Expand Up @@ -1087,7 +1087,7 @@ void WebGL2RenderingContextBase::renderbufferStorageMultisample(
}
RenderbufferStorageImpl(target, samples, internalformat, width, height,
function_name);
ApplyStencilTest();
ApplyDepthAndStencilTest();
}

void WebGL2RenderingContextBase::ResetUnpackParameters() {
Expand Down
14 changes: 14 additions & 0 deletions third_party/blink/renderer/modules/webgl/webgl_framebuffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -184,9 +184,11 @@ void WebGLTextureAttachment::Unattach(gpu::gles2::GLES2Interface* gl,
WebGLFramebuffer::WebGLAttachment::WebGLAttachment() = default;

WebGLFramebuffer* WebGLFramebuffer::CreateOpaque(WebGLRenderingContextBase* ctx,
bool has_depth,
bool has_stencil) {
WebGLFramebuffer* const fb =
MakeGarbageCollected<WebGLFramebuffer>(ctx, true);
fb->SetOpaqueHasDepth(has_depth);
fb->SetOpaqueHasStencil(has_stencil);
return fb;
}
Expand Down Expand Up @@ -368,6 +370,18 @@ GLenum WebGLFramebuffer::CheckDepthStencilStatus(const char** reason) const {
return GL_FRAMEBUFFER_UNSUPPORTED;
}

bool WebGLFramebuffer::HasDepthBuffer() const {
if (opaque_) {
return opaque_has_depth_;
} else {
WebGLAttachment* attachment = GetAttachment(GL_DEPTH_ATTACHMENT);
if (!attachment) {
attachment = GetAttachment(GL_DEPTH_STENCIL_ATTACHMENT);
}
return attachment && attachment->Valid();
}
}

bool WebGLFramebuffer::HasStencilBuffer() const {
if (opaque_) {
return opaque_has_stencil_;
Expand Down
4 changes: 4 additions & 0 deletions third_party/blink/renderer/modules/webgl/webgl_framebuffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ class WebGLFramebuffer final : public WebGLContextObject {
// the browser and not inspectable or alterable via Javascript. This is
// primarily used by the VRWebGLLayer interface.
static WebGLFramebuffer* CreateOpaque(WebGLRenderingContextBase*,
bool has_depth,
bool has_stencil);

GLuint Object() const { return object_; }
Expand Down Expand Up @@ -106,13 +107,15 @@ class WebGLFramebuffer final : public WebGLContextObject {

void SetHasEverBeenBound() { has_ever_been_bound_ = true; }

bool HasDepthBuffer() const;
bool HasStencilBuffer() const;

bool HaveContentsChanged() { return contents_changed_; }
void SetContentsChanged(bool changed) { contents_changed_ = changed; }

bool Opaque() const { return opaque_; }
void MarkOpaqueBufferComplete(bool complete) { opaque_complete_ = complete; }
void SetOpaqueHasDepth(bool has_depth) { opaque_has_depth_ = has_depth; }
void SetOpaqueHasStencil(bool has_stencil) {
opaque_has_stencil_ = has_stencil;
}
Expand Down Expand Up @@ -168,6 +171,7 @@ class WebGLFramebuffer final : public WebGLContextObject {
bool web_gl1_depth_stencil_consistent_;
bool contents_changed_ = false;
const bool opaque_;
bool opaque_has_depth_ = false;
bool opaque_has_stencil_ = false;
bool opaque_complete_ = false;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1232,6 +1232,7 @@ void WebGLRenderingContextBase::InitializeNewContext() {
framebuffer_binding_ = nullptr;
renderbuffer_binding_ = nullptr;
depth_mask_ = true;
depth_enabled_ = false;
stencil_enabled_ = false;
stencil_mask_ = 0xFFFFFFFF;
stencil_mask_back_ = 0xFFFFFFFF;
Expand Down Expand Up @@ -2828,7 +2829,12 @@ void WebGLRenderingContextBase::disable(GLenum cap) {
return;
if (cap == GL_STENCIL_TEST) {
stencil_enabled_ = false;
ApplyStencilTest();
ApplyDepthAndStencilTest();
return;
}
if (cap == GL_DEPTH_TEST) {
depth_enabled_ = false;
ApplyDepthAndStencilTest();
return;
}
if (cap == GL_SCISSOR_TEST)
Expand Down Expand Up @@ -2983,7 +2989,12 @@ void WebGLRenderingContextBase::enable(GLenum cap) {
return;
if (cap == GL_STENCIL_TEST) {
stencil_enabled_ = true;
ApplyStencilTest();
ApplyDepthAndStencilTest();
return;
}
if (cap == GL_DEPTH_TEST) {
depth_enabled_ = true;
ApplyDepthAndStencilTest();
return;
}
if (cap == GL_SCISSOR_TEST)
Expand Down Expand Up @@ -3055,7 +3066,7 @@ void WebGLRenderingContextBase::framebufferRenderbuffer(
}
framebuffer_binding->SetAttachmentForBoundFramebuffer(target, attachment,
buffer);
ApplyStencilTest();
ApplyDepthAndStencilTest();
}

void WebGLRenderingContextBase::framebufferTexture2D(GLenum target,
Expand Down Expand Up @@ -3087,7 +3098,7 @@ void WebGLRenderingContextBase::framebufferTexture2D(GLenum target,
}
framebuffer_binding->SetAttachmentForBoundFramebuffer(
target, attachment, textarget, texture, level, 0, 0);
ApplyStencilTest();
ApplyDepthAndStencilTest();
}

void WebGLRenderingContextBase::frontFace(GLenum mode) {
Expand Down Expand Up @@ -3587,7 +3598,7 @@ ScriptValue WebGLRenderingContextBase::getParameter(ScriptState* script_state,
case GL_DEPTH_RANGE:
return GetWebGLFloatArrayParameter(script_state, pname);
case GL_DEPTH_TEST:
return GetBooleanParameter(script_state, pname);
return WebGLAny(script_state, depth_enabled_);
case GL_DEPTH_WRITEMASK:
return GetBooleanParameter(script_state, pname);
case GL_DITHER:
Expand Down Expand Up @@ -4514,8 +4525,12 @@ bool WebGLRenderingContextBase::isContextLost() const {
GLboolean WebGLRenderingContextBase::isEnabled(GLenum cap) {
if (isContextLost() || !ValidateCapability("isEnabled", cap))
return 0;
if (cap == GL_STENCIL_TEST)
if (cap == GL_DEPTH_TEST) {
return depth_enabled_;
}
if (cap == GL_STENCIL_TEST) {
return stencil_enabled_;
}
return ContextGL()->IsEnabled(cap);
}

Expand Down Expand Up @@ -4967,7 +4982,7 @@ void WebGLRenderingContextBase::renderbufferStorage(GLenum target,
return;
RenderbufferStorageImpl(target, 0, internalformat, width, height,
function_name);
ApplyStencilTest();
ApplyDepthAndStencilTest();
}

void WebGLRenderingContextBase::sampleCoverage(GLfloat value,
Expand Down Expand Up @@ -8758,15 +8773,20 @@ void WebGLRenderingContextBase::EmitGLWarning(const char* function_name,
NotifyWebGLWarning();
}

void WebGLRenderingContextBase::ApplyStencilTest() {
void WebGLRenderingContextBase::ApplyDepthAndStencilTest() {
bool have_stencil_buffer = false;
bool have_depth_buffer = false;

if (framebuffer_binding_) {
have_depth_buffer = framebuffer_binding_->HasDepthBuffer();
have_stencil_buffer = framebuffer_binding_->HasStencilBuffer();
} else {
have_depth_buffer = !isContextLost() && CreationAttributes().depth &&
GetDrawingBuffer()->HasDepthBuffer();
have_stencil_buffer = !isContextLost() && CreationAttributes().stencil &&
GetDrawingBuffer()->HasStencilBuffer();
}
EnableOrDisable(GL_DEPTH_TEST, depth_enabled_ && have_depth_buffer);
EnableOrDisable(GL_STENCIL_TEST, stencil_enabled_ && have_stencil_buffer);
}

Expand Down Expand Up @@ -8824,7 +8844,7 @@ void WebGLRenderingContextBase::SetFramebuffer(GLenum target,

if (target == GL_FRAMEBUFFER || target == GL_DRAW_FRAMEBUFFER) {
framebuffer_binding_ = buffer;
ApplyStencilTest();
ApplyDepthAndStencilTest();
}
if (!buffer) {
// Instead of binding fb 0, bind the drawing buffer.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -963,6 +963,7 @@ class MODULES_EXPORT WebGLRenderingContextBase : public CanvasRenderingContext,
GLboolean color_mask_[4];
GLboolean depth_mask_;

bool depth_enabled_;
bool stencil_enabled_;
GLuint stencil_mask_, stencil_mask_back_;
GLint stencil_func_ref_,
Expand Down Expand Up @@ -1825,9 +1826,10 @@ class MODULES_EXPORT WebGLRenderingContextBase : public CanvasRenderingContext,

String EnsureNotNull(const String&) const;

// Enable or disable stencil test based on user setting and
// whether the current FBO has a stencil buffer.
void ApplyStencilTest();
// Enable or disable the depth and stencil test based on the user's
// setting and whether the current FBO has a depth and stencil
// buffer.
void ApplyDepthAndStencilTest();

// Helper for enabling or disabling a capability.
void EnableOrDisable(GLenum capability, bool enable);
Expand Down
4 changes: 2 additions & 2 deletions third_party/blink/renderer/modules/xr/xr_webgl_layer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,8 @@ XRWebGLLayer* XRWebGLLayer::Create(XRSession* session,
gfx::ToFlooredSize(gfx::ScaleSize(framebuffers_size, framebuffer_scale));

// Create an opaque WebGL Framebuffer
WebGLFramebuffer* framebuffer =
WebGLFramebuffer::CreateOpaque(webgl_context, want_stencil_buffer);
WebGLFramebuffer* framebuffer = WebGLFramebuffer::CreateOpaque(
webgl_context, want_depth_buffer, want_stencil_buffer);

scoped_refptr<XRWebGLDrawingBuffer> drawing_buffer =
XRWebGLDrawingBuffer::Create(webgl_context->GetDrawingBuffer(),
Expand Down

0 comments on commit d608040

Please sign in to comment.