Only complain on stencil writemask/value_mask/ref front/back mismatch… #2583

kdashg · 2018-01-16T22:26:26Z

… for the s stencil bits of the draw framebuffer.

No one seems to implement this yet. Is this what we want?

kdashg · 2018-01-18T19:29:47Z

Add test for DEPTH w/o STENCIL, vice versa
Use effective format RGBA8 (RGBA/UBYTE) to guarantee framebuffer completeness
Do we need to backport?

kdashg · 2018-01-19T02:12:23Z

Do we want to require the reference values to be the same? The reference values are all masked by their respectively-masked masks, so refs at least always behave as if they are masked to the available bits.
We can continue to require that the unmasked values match if we want, if it's too much to ask ANGLE to deal with resolving this difference.

RafaelCintron · 2018-01-19T02:07:24Z

sdk/tests/conformance/misc/webgl-specific.html

+debug("...with 8 stencil bits in the draw framebuffer");
+testForStencilBits(gl.INVALID_OPERATION);
+
+gl.deleteRenderbuffer(fb);


Nit: Should be deleteFramebuffer since fb is a framebuffer, not a renderbuffer.

RafaelCintron · 2018-01-19T02:10:10Z

sdk/tests/conformance/misc/webgl-specific.html

+
+    wtu.shouldGenerateGLError(gl, gl.NO_ERROR, "gl.stencilFuncSeparate(gl.FRONT, gl.ALWAYS, 1, 1023)");
+    wtu.shouldGenerateGLError(gl, gl.NO_ERROR, "gl.stencilFuncSeparate(gl.BACK, gl.ALWAYS, 257, 1023)");
+    wtu.shouldGenerateGLError(gl, gl.NO_ERROR, "gl.drawArrays(gl.POINTS, 0, 0)");


@jdashg , I tried running your new test with the ANGLE running in webglCompatibility mode and received several failures.
This fourth call to drawArrays fails with INVALID_OPERATION in the following ANGLE code in ValidateDrawBase

// Note: these separate values are not supported in WebGL, due to D3D's limitations. See // Section 6.10 of the WebGL 1.0 spec. Framebuffer *framebuffer = state.getDrawFramebuffer(); if (context->getLimitations().noSeparateStencilRefsAndMasks || extensions.webglCompatibility) { const FramebufferAttachment *dsAttachment = framebuffer->getStencilOrDepthStencilAttachment(); GLuint stencilBits = dsAttachment ? dsAttachment->getStencilSize() : 0; GLuint minimumRequiredStencilMask = (1 << stencilBits) - 1; const DepthStencilState &depthStencilState = state.getDepthStencilState(); bool differentRefs = state.getStencilRef() != state.getStencilBackRef(); bool differentWritemasks = (depthStencilState.stencilWritemask & minimumRequiredStencilMask) != (depthStencilState.stencilBackWritemask & minimumRequiredStencilMask); bool differentMasks = (depthStencilState.stencilMask & minimumRequiredStencilMask) != (depthStencilState.stencilBackMask & minimumRequiredStencilMask); if (differentRefs || differentWritemasks || differentMasks) { if (!extensions.webglCompatibility) { ERR() << "This ANGLE implementation does not support separate front/back stencil " "writemasks, reference values, or stencil mask values."; } ANGLE_VALIDATION_ERR(context, InvalidOperation(), StencilReferenceMaskOrMismatch); return false; } }

Though differentWritemasks and differentMasks are false, differentRefs is true. The stencil ref is 1 and the stencil back ref is 257. Looks like the minimum required bits only applies to the masks, not the reference value.

It seems like a bug in ANGLE that the minimumRequiredStencilMask isn't applied to the reference values. @RafaelCintron, do you agree? Is applying this mask compatible with Direct3D's semantics?

@kenrussell , D3D11 does not support creating textures with more than 8bits of stencil data. Hence, the read and write masks are only limited to 8 bits. See the DEPTH_STENCIL_DESC documentation. The reference value is an unsigned integer. See the OMSetDepthStencilState for documentation.

In StateManager11::syncDepthStencilState, ANGLE clamps the reference value before calling D3D11 with a comment stating this is because of the GL spec.

I suppose It would be nice to inform the web developer (via an error) when bits of their reference value are not taking effect but I do not feel strongly about that.

RafaelCintron · 2018-01-19T02:16:11Z

sdk/tests/conformance/misc/webgl-specific.html

+    wtu.shouldGenerateGLError(gl, gl.NO_ERROR, "gl.drawArrays(gl.POINTS, 0, 0)");
+
+    wtu.shouldGenerateGLError(gl, gl.NO_ERROR, "gl.stencilFuncSeparate(gl.BACK, gl.ALWAYS, 258, 1023)");
+    wtu.shouldGenerateGLError(gl, errIfMismatch, "gl.drawArrays(gl.POINTS, 0, 0)");


This call to drawArrays fails for the same reason as above.

RafaelCintron · 2018-01-19T19:07:12Z

Adding @Kangz @null77 and @vonture.

@jdashg , for reference, ANGLE has two tests for this functionality. One is RequiresSameStencilMaskAndRef added by @Kangz as part of the WebGL compatibility test. The other is DifferentStencilMasksTests added by Jinyoung Hur.

kenrussell · 2018-01-19T19:34:17Z

Looking at those tests it seems to me that if we change WebGL's validation semantics so that the stencil reference value is masked by minimumRequiredStencilMask, too, then things should still work fine on Direct3D. I think we should make that change to the validation, and test it.

RafaelCintron · 2018-01-19T19:38:07Z

@jdashg , since D3D11 only supports setting one reference value for depth stencil state, we need to enforce the reference values for read and write be equal.

kdashg · 2018-01-26T23:31:34Z

@RafaelCintron Is it ok if they are equal only /after/ masking? That's what I've proposed here.

kdashg · 2018-01-26T23:36:41Z

Should we just deprecate stencilFuncSeparate and stencilMaskSeparate? stencilOpSeparate seems to be the only aspect that D3D11 supports.

kdashg · 2018-01-26T23:37:39Z

It's easier/clearer if we just forbid separate values outright, instead of being tricky and only forbidding them when active.

kenrussell · 2018-01-27T05:22:08Z

@jdashg @RafaelCintron a couple of questions:

@RafaelCintron you mentioned "the reference values for read and write must be equal". I don't understand that -- did you mean "the reference values for front and back must be equal"? In that case I support @jdashg 's desire to compare them only after masking with the minimumRequiredStencilMask in ANGLE. (Or, in other words, a mask that includes the number of bits in the stencil buffer -- this change to WebGL's validation would be done on all platforms.)
@jdashg D3D11 supports different stencil functions for front and back faces. D3D11_DEPTH_STENCIL_DESC [3] has FrontFace and BackFace members of type D3D11_DEPTH_STENCILOP_DESC, and those contain the functions. I think this change and associated ANGLE/WebGL validation changes should just focus on the issue at hand of masking various values like the reference value by the number of bits actually in the stencil buffer.
@jdashg above you mentioned "instead of being tricky and only forbidding them when active" -- did you mean, if the STENCIL_TEST were active? I agree that we should stick with the current WebGL semantics in https://www.khronos.org/registry/webgl/specs/latest/1.0/#6.11 and reject all draw calls made while these illegal states are set. However, I don't see a way to reject setting the illegal states.

Some references:
[1] ANGLE's current stencil validation in ValidateDrawBase:
https://cs.chromium.org/chromium/src/third_party/angle/src/libANGLE/validationES.cpp?l=2479

[2] ANGLE's RenderStateCache::getDepthStencilState , which populates the D3D11_DEPTH_STENCIL_DESC structure:
https://cs.chromium.org/chromium/src/third_party/angle/src/libANGLE/renderer/d3d/d3d11/RenderStateCache.cpp?l=185

[3] Documentation for D3D11_DEPTH_STENCIL_DESC:
https://msdn.microsoft.com/en-us/library/windows/desktop/ff476110(v=vs.85).aspx

RafaelCintron · 2018-01-27T23:31:50Z

@kenrussell

@RafaelCintron you mentioned "the reference values for read and write must be equal". I don't understand that -- did you mean "the reference values for front and back must be equal"? In that case I support @jdashg 's desire to compare them only after masking with the minimumRequiredStencilMask in ANGLE. (Or, in other words, a mask that includes the number of bits in the stencil buffer -- this change to WebGL's validation would be done on all platforms.)

Yes, I mean the reference values for front and back must be equal. OMSetDepthStencilState only allows you to set one reference value that applies to both back and front.

As I said in my reply, I don't feel strongly about masking the reference value by the minimumRequiredStencilMask. We would need to make this change in ANGLE.

Attempting to adjust WebGL's stencil rules.

kenrussell · 2018-01-28T03:15:40Z

After attempting to make this change in ANGLE, I discovered that masking the stencil reference value by the number of stencil bits is fundamentally incompatible with the OpenGL ES spec. OpenGL ES already specifies that the stencil reference value is clamped to the range [0, 2^s-1], where s is the number of bits in the current framebuffer's stencil buffer.

The rule that the WebGL spec will need to implement is that the draw call is rejected if the stencil front and back reference values are different, after clamping to the range [0, 2^s-1]. Can we agree on these new semantics? It is going to be impossible to update ANGLE otherwise.

I attempted to update the test in:
#2592

and implement the new checks in ANGLE here:
https://chromium-review.googlesource.com/890605

In Chromium, this required this patch to be applied:
https://chromium-review.googlesource.com/890606

It doesn't work yet. Errors aren't being generated where they should be, and some assertion is being hit when the -1 reference value is sent. I don't know how to see the stack traces for those assertion failures on Windows.

I think we need to get this test passing 100% somewhere before we merge this pull request, so we know whether the new semantics are workable.

kdashg · 2018-01-30T01:35:12Z

Can we just require that they're all the same? That's way simpler, and ANGLE wouldn't have to do any crazy stuff. Since it's required in D3D11, apps that were targeting d3d11 natively would have to deal with not having this. It is definitely simpler if we just standardize on embracing d3d11's restrictions directly.

kenrussell · 2018-01-31T01:31:16Z

WebGL's current rules are that everything must match. An external developer found while running the drawElements tests on top of ANGLE that this was too restrictive, and @null77 and @vonture reviewed https://chromium-review.googlesource.com/302703 to make these checks more compatible with the dEQP.

I think I can debug the clamping of the reference values. If we get that working in ANGLE then @jdashg are you OK with changing the WebGL spec validation to include that?

It would be best to not revert https://chromium-review.googlesource.com/302703 . Because of masking and clamping, the Direct3D restrictions are actually that these writemasks and reference values only have to be equal up to the number of bits in the stencil buffer.

kdashg · 2018-01-31T02:33:54Z

I would actually prefer to retain the explicit d3d11 restriction, if deqp is the only place we've run into problems with it. I think that would be simpler, and shouldn't change how actual content is written, since it's not useful to rely on masks/values outside the number of stencil bits.

I don't think we should put in extra work just so we happen to pass deqp. I'm actually surprised that restricting things to the number of stencil bits is enough to pass deqp, since restricting to the number of stencil bits doesn't bring back differing front/back masks and references.

Attempting to adjust WebGL's stencil rules.

kainino0x · 2018-03-08T00:55:09Z

I made a PR into @jdashg's branch:
kdashg#4
(a PR into this PR).

Comments are welcome on that PR if anyone has them.

kainino0x · 2018-03-08T00:55:29Z

Associated ANGLE changes are in this patch:
#2583

kdashg · 2018-03-09T02:32:44Z

Updated. I'll check this against my local patches tomorrow.
@RafaelCintron Please re-review.

kenrussell · 2018-03-09T18:32:39Z

Looks great! Great work @jdashg and @kainino0x!

RafaelCintron · 2018-03-09T19:07:54Z

sdk/tests/conformance/misc/webgl-specific-stencil-settings.html

+    debug("With depthbuffer=" + haveDepthBuffer +
+            ", stencilbuffer=" + haveStencilBuffer +
+            ", stencilTest=" + enableStencilTest +
+            ", expecting error=" + errIfMismatch +


Nit: Consider using glEnumToString(errIfMismatch) to make the output read a bit easier. wtu.shouldGenerateGLError already uses this helper function.

Thanks, I forgot about that helper.

RafaelCintron · 2018-03-09T19:14:22Z

sdk/tests/conformance/misc/webgl-specific-stencil-settings.html

+var vertexObject = gl.createBuffer();
+gl.bindBuffer(gl.ARRAY_BUFFER, vertexObject);
+gl.enableVertexAttribArray(0);
+gl.vertexAttribPointer(0, 4, gl.FLOAT, false, 0, 0);


Nit: Since the draw primitive is not relevant for the test, consider using wtu.setupQuad and wtu.drawQuad to save lines of code.

Good point, I copied this from webgl-specific without thinking about whether it was all necessary.

(EDIT: I think I can actually remove all of this, because no actual quads are drawn.)

RafaelCintron · 2018-03-09T19:18:48Z

sdk/tests/conformance/misc/webgl-specific-stencil-settings.html

+    testStencilMaskCase([0, 256], gl.NO_ERROR);
+    testStencilMaskCase([1, 256], errIfMismatch);
+    testStencilMaskCase([1, 257], gl.NO_ERROR);
+    testStencilMaskCase([1, 258], errIfMismatch);


If the max stencil value is 255 and we're ANDing that with the front and back masks, I don't understand how any of these could pass without error. The front and back masks have to match.

I must be missing something.

As written, the front and back mask settings don't have to match - just the mask bits that actually take effect. On an 8 bit stencil buffer, only the least significant 8 bits take effect. So the check:
(0 & 255) == (256 & 255)
passes (NO_ERROR).

Ah, right. I misread the code.

kainino0x

I'll submit a follow-on PR [to @jdashg's branch again] to take care of these issues, since I introduced them.

kainino0x · 2018-03-09T20:20:08Z

sdk/tests/conformance/misc/webgl-specific-stencil-settings.html

+    debug("With depthbuffer=" + haveDepthBuffer +
+            ", stencilbuffer=" + haveStencilBuffer +
+            ", stencilTest=" + enableStencilTest +
+            ", expecting error=" + errIfMismatch +


Thanks, I forgot about that helper.

kainino0x · 2018-03-09T20:20:38Z

sdk/tests/conformance/misc/webgl-specific-stencil-settings.html

+var vertexObject = gl.createBuffer();
+gl.bindBuffer(gl.ARRAY_BUFFER, vertexObject);
+gl.enableVertexAttribArray(0);
+gl.vertexAttribPointer(0, 4, gl.FLOAT, false, 0, 0);


Good point, I copied this from webgl-specific without thinking about whether it was all necessary.

(EDIT: I think I can actually remove all of this, because no actual quads are drawn.)

kainino0x · 2018-03-09T20:23:41Z

sdk/tests/conformance/misc/webgl-specific-stencil-settings.html

+    testStencilMaskCase([0, 256], gl.NO_ERROR);
+    testStencilMaskCase([1, 256], errIfMismatch);
+    testStencilMaskCase([1, 257], gl.NO_ERROR);
+    testStencilMaskCase([1, 258], errIfMismatch);


As written, the front and back mask settings don't have to match - just the mask bits that actually take effect. On an 8 bit stencil buffer, only the least significant 8 bits take effect. So the check:
(0 & 255) == (256 & 255)
passes (NO_ERROR).

kainino0x · 2018-03-09T20:33:47Z

Here:
kdashg#5

RafaelCintron

My remaining issues should be straightforward to resolve so I am approving to unblock Kai and Jeff.

kdashg · 2018-03-09T22:16:35Z

Just waiting on CI.

kenrussell · 2018-03-09T22:20:40Z

Great. Let's make sure to squash and merge this one and compose a good commit message, to keep the commit history clearer.

… for the `s` stencil bits of the draw framebuffer. - WRITEMASK and VALUE_MASK mask, whereas REF clamps - Add and use drawNothing call

kenrussell · 2018-03-09T23:25:43Z

@jdashg and I synced up and he wants to finish testing this with Firefox before merging, so let's wait for confirmation. Or @jdashg feel free to merge yourself. Thanks.

kdashg · 2018-03-10T00:50:27Z

Confirmed fixed by my pending Gecko patches too, fwiw.

Based on kbr's patch: https://chromium-review.googlesource.com/c/angle/angle/+/890605 Implements new rules in this revised WebGL conformance test: KhronosGroup/WebGL#2583 BUG=chromium:806557 Change-Id: I84701dd7156f0bc4a45ba68e63cb962d2d54c2e5 Reviewed-on: https://chromium-review.googlesource.com/952567 Commit-Queue: Kai Ninomiya <kainino@chromium.org> Reviewed-by: Jamie Madill <jmadill@chromium.org> Reviewed-by: Corentin Wallez <cwallez@chromium.org>

…2781) The 1.0.3 version of webgl-specific.html should request stencil:true, and enable(STENCIL_TEST). Follow-on to #2583.

kdashg requested review from kenrussell, RafaelCintron and grorg January 16, 2018 22:26

RafaelCintron requested changes Jan 19, 2018

View reviewed changes

kenrussell added a commit to kenrussell/WebGL that referenced this pull request Jan 28, 2018

Building on jdashg's KhronosGroup#2583 .

9afa2c0

Attempting to adjust WebGL's stencil rules.

kenrussell mentioned this pull request Jan 28, 2018

NOT FOR COMMIT: showing changes to #2583. #2592

Closed

kainino0x pushed a commit to kainino0x/WebGL that referenced this pull request Mar 7, 2018

Building on jdashg's KhronosGroup#2583 .

17249e6

Attempting to adjust WebGL's stencil rules.

kainino0x mentioned this pull request Mar 7, 2018

fix ref to be clamped instead of ANDed, and add more test cases kdashg/WebGL#4

Merged

kenrussell approved these changes Mar 9, 2018

View reviewed changes

RafaelCintron requested changes Mar 9, 2018

View reviewed changes

kainino0x reviewed Mar 9, 2018

View reviewed changes

RafaelCintron approved these changes Mar 9, 2018

View reviewed changes

Only complain on stencil writemask/value_mask/ref front/back mismatch…

14dc185

… for the `s` stencil bits of the draw framebuffer. - WRITEMASK and VALUE_MASK mask, whereas REF clamps - Add and use drawNothing call

kdashg force-pushed the stencil-masking branch from 38a57a6 to 14dc185 Compare March 10, 2018 00:48

kdashg merged commit dba66c4 into KhronosGroup:master Mar 10, 2018

kdashg deleted the stencil-masking branch March 19, 2018 22:44

This was referenced Apr 3, 2018

Fix usage of glEnumToString in webgl-specific-stencil-settings #2625

Merged

Change dummySetProgramAndDrawNothing to draw >0 elements #2626

Merged

kenrussell mentioned this pull request Apr 7, 2018

Limit restrictions on separate stencil mask and reference values #2446

Closed

kenrussell pushed a commit that referenced this pull request Feb 1, 2019

Stencil mismatch is only an error if there are active stencil bits. (#…

5ddec66

…2781) The 1.0.3 version of webgl-specific.html should request stencil:true, and enable(STENCIL_TEST). Follow-on to #2583.

Only complain on stencil writemask/value_mask/ref front/back mismatch… #2583

Only complain on stencil writemask/value_mask/ref front/back mismatch… #2583

Conversation

kdashg commented Jan 16, 2018

kdashg commented Jan 18, 2018

kdashg commented Jan 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RafaelCintron Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RafaelCintron commented Jan 19, 2018 • edited Loading

kenrussell commented Jan 19, 2018

RafaelCintron commented Jan 19, 2018

kdashg commented Jan 26, 2018

kdashg commented Jan 26, 2018

kdashg commented Jan 26, 2018

kenrussell commented Jan 27, 2018

RafaelCintron commented Jan 27, 2018

kenrussell commented Jan 28, 2018

kdashg commented Jan 30, 2018

kenrussell commented Jan 31, 2018

kdashg commented Jan 31, 2018

kainino0x commented Mar 8, 2018

kainino0x commented Mar 8, 2018

kdashg commented Mar 9, 2018

kenrussell commented Mar 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kainino0x Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kainino0x left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kainino0x Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kainino0x commented Mar 9, 2018

RafaelCintron left a comment

Choose a reason for hiding this comment

kdashg commented Mar 9, 2018

kenrussell commented Mar 9, 2018

kenrussell commented Mar 9, 2018

kdashg commented Mar 10, 2018

RafaelCintron Jan 19, 2018 •

edited

Loading

RafaelCintron commented Jan 19, 2018 •

edited

Loading

kainino0x Mar 9, 2018 •

edited

Loading

kainino0x left a comment •

edited

Loading

kainino0x Mar 9, 2018 •

edited

Loading