Update stencil front/back validation based on spec changes

RESOLVED FIXED in Firefox 61

Status

()

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

unspecified
mozilla61
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: gfx-noted)

Attachments

(1 attachment)

Comment hidden (mozreview-request)

Comment 2

Last year
mozreview-review
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`
Attachment #8957750 - Flags: review?(kvark) → review-
Assignee

Comment 3

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

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

Updated

Last year
Attachment #8957750 - Flags: review- → review?(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.
Attachment #8957750 - Flags: review?(kvark)
Assignee

Comment 5

Last year
mozreview-review-reply
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.
Assignee

Comment 6

Last year
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.
Attachment #8957750 - Flags: review?(kvark)

Comment 7

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

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

alright, thanks for clarification!
Attachment #8957750 - Flags: review?(kvark) → review+

Comment 8

Last year
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/586d0eef1de5
Update stencil front/back mismatch validation. - r=kvark
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)
Flags: needinfo?(jgilbert)

Comment 10

Last year
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32da6aeeb1e8
Update stencil front/back mismatch validation. - r=kvark

Comment 11

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/32da6aeeb1e8
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee

Updated

Last year
Flags: needinfo?(jgilbert)
You need to log in before you can comment on or make changes to this bug.