1444563 - Update stencil front/back validation based on spec changes

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, enhancement)

Description

[Kelsey Gilbert [:jgilbert]]

https://github.com/KhronosGroup/WebGL/pull/2583

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1444563 - Update stencil front/back mismatch validation. -

Review commit: https://reviewboard.mozilla.org/r/226714/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226714/

Comment 2

[Dzmitry Malyshau [:kvark]]

Comment on attachment 8957750 [details]
Bug 1444563 - Update stencil front/back mismatch validation. -

https://reviewboard.mozilla.org/r/226714/#review232792

got a few concerns

::: dom/canvas/WebGLContextDraw.cpp:225
(Diff revision 1)
>  ////////////////////////////////////////
>  
> +bool
> +WebGLContext::ValidateStencilParamsForDrawCall(const char* const funcName) const
> +{
> +    const auto stencilBits = [&]() -> uint8_t {

I had to re-read this part a few times to realize that it's not the closure you are interested in but rather the expression result. I don't see why you'd want to use the closure here at all then, it's just confusing.

::: dom/canvas/WebGLContextDraw.cpp:250
(Diff revision 1)
> +    };
> +
> +    bool ok = true;
> +    ok &= (fnMask(mStencilWriteMaskFront) == fnMask(mStencilWriteMaskBack));
> +    ok &= (fnMask(mStencilValueMaskFront) == fnMask(mStencilValueMaskBack));
> +    ok &= (fnClamp(mStencilRefFront) == fnClamp(mStencilRefBack));

can't we relax the rules a little bit? e.g. technically since only the bits masked by the `ValueMask` are important, that's what we should be using to validate both the write masks and reference  values (instead of `stencilMax`

Comment 3

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8957750 [details]
Bug 1444563 - Update stencil front/back mismatch validation. -

https://reviewboard.mozilla.org/r/226714/#review232978

Comment 4

[Dzmitry Malyshau [:kvark]]

Comment on attachment 8957750 [details]
Bug 1444563 - Update stencil front/back mismatch validation. -

I don't see any response on the issues I raised in the review, and no new code either, so I'm dropping the review request for now to avoid being bugged by bugzilla.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8957750 [details]
Bug 1444563 - Update stencil front/back mismatch validation. -

https://reviewboard.mozilla.org/r/226714/#review232792

> I had to re-read this part a few times to realize that it's not the closure you are interested in but rather the expression result. I don't see why you'd want to use the closure here at all then, it's just confusing.

It's a relatively recent modern c++ism:
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Res-lambda-init

> can't we relax the rules a little bit? e.g. technically since only the bits masked by the `ValueMask` are important, that's what we should be using to validate both the write masks and reference  values (instead of `stencilMax`

We decided to spec it very strictly, which enables a more natural mapping onto D3D. I don't remember why we didn't spec this loosely. This is what the spec says for now though.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8957750 [details]
Bug 1444563 - Update stencil front/back mismatch validation. -

Looks like I forgot to hit Publish on my remarks. They're up now.

Comment 7

[Dzmitry Malyshau [:kvark]]

Comment on attachment 8957750 [details]
Bug 1444563 - Update stencil front/back mismatch validation. -

https://reviewboard.mozilla.org/r/226714/#review233746

alright, thanks for clarification!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/586d0eef1de5
Update stencil front/back mismatch validation. - r=kvark

Comment 9

[Cristian Brindusan [:cbrindusan]]

Backed out for webgl failures on test_2_conformance__misc__webgl-specific.html

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc25038c95f8b05d38af22b30c67f7e2b1bc6fdc

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=586d0eef1de5935b10b7e08844430c9ba0207b05&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running 

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=168317244&repo=mozilla-inbound&lineNumber=2645 

 TEST-UNEXPECTED-FAIL | dom/canvas/test/webgl-conf/generated/test_conformance__misc__webgl-specific.html | getError expected: INVALID_OPERATION. Was NO_ERROR : after evaluating: gl.drawArrays(gl.TRIANGLES, 0, 0) 
[task 2018-03-15T21:11:32.085Z] 21:11:32     INFO -     reportResults@dom/canvas/test/webgl-conf/mochi-single.html?checkout/conformance/misc/webgl-specific.html:22:7
[task 2018-03-15T21:11:32.085Z] 21:11:32     INFO -     reportTestResultsToHarness@dom/canvas/test/webgl-conf/checkout/js/js-test-pre.js:116:5
[task 2018-03-15T21:11:32.086Z] 21:11:32     INFO -     testFailed@dom/canvas/test/webgl-conf/checkout/js/js-test-pre.js:246:5
[task 2018-03-15T21:11:32.087Z] 21:11:32     INFO -     glErrorShouldBeImpl@dom/canvas/test/webgl-conf/checkout/js/webgl-test-utils.js:1590:5
[task 2018-03-15T21:11:32.088Z] 21:11:32     INFO -     glErrorShouldBe@dom/canvas/test/webgl-conf/checkout/js/webgl-test-utils.js:1564:3
[task 2018-03-15T21:11:32.089Z] 21:11:32     INFO -     shouldGenerateGLError@dom/canvas/test/webgl-conf/checkout/js/webgl-test-utils.js:1534:5
[task 2018-03-15T21:11:32.089Z] 21:11:32     INFO -     @dom/canvas/test/webgl-conf/checkout/conformance/misc/webgl-specific.html:91:1
[task 2018-03-15T21:11:32.090Z] 21:11:32     INFO - TEST-PASS | dom/canvas/test/webgl-conf/generated/test_conformance__misc__webgl-specific.html | getError was expected value: NO_ERROR : after evaluating: gl.stencilFuncSeparate(gl.FRONT, gl.ALWAYS, 1, 255) 
[task 2018-03-15T21:11:32.091Z] 21:11:32     INFO - TEST-PASS | dom/canvas/test/webgl-conf/generated/test_conformance__misc__webgl-specific.html | getError was expected value: NO_ERROR : after evaluating: gl.drawArrays(gl.TRIANGLES, 0, 0) 
[task 2018-03-15T21:11:32.092Z] 21:11:32     INFO - TEST-PASS | dom/canvas/test/webgl-conf/generated/test_conformance__misc__webgl-specific.html | getError was expected value: NO_ERROR : after evaluating: gl.stencilFuncSeparate(gl.BACK, gl.ALWAYS, 1, 1) 
[task 2018-03-15T21:11:32.093Z] 21:11:32     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-03-15T21:11:32.096Z] 21:11:32     INFO - TEST-UNEXPECTED-FAIL | dom/canvas/test/webgl-conf/generated/test_conformance__misc__webgl-specific.html | getError expected: INVALID_OPERATION. Was NO_ERROR : after evaluating: gl.drawArrays(gl.TRIANGLES, 0, 0)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32da6aeeb1e8
Update stencil front/back mismatch validation. - r=kvark

Comment 11

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/32da6aeeb1e8