982477 - Don't advertise depth functionality if not requested.

Status: RESOLVED DUPLICATE of bug 1427668

(Core :: Graphics: CanvasWebGL, defect)

Comment 1

[u480271]

Attachment: If context is created without depth, don't enable or advertise support for depth, even if a depth buffer exists.

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8389597 [details] [diff] [review]
If context is created without depth, don't enable or advertise support for depth, even if a depth buffer exists.

Review of attachment 8389597 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WebGLContext.cpp
@@ +341,5 @@
>        newOpts.alpha = attributes.mAlpha.Value();
>      }
>  
>      // enforce that if stencil is specified, we also give back depth
> +    //    newOpts.depth |= newOpts.stencil; // No no no no!

Oh god, this line's still around? Please delete it.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Theoretically, we actually want to have a more complicated fallback sequence, in case, for example, depth-without-stencil is unsupported. While I sort of want to just fix this bug, it should probably be done correctly, lets we break people's demos on certain hardware/drivers.

Comment 4

[u480271]

Attachment: Testcase for no depth for stencil only context.

Comment 5

[u480271]

Looks like some Linux sadness.

https://tbpl.mozilla.org/?tree=Try&rev=2abfa02ac38f

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8389609 [details] [diff] [review]
Testcase for no depth for stencil only context.

Review of attachment 8389609 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/test/webgl/non-conf-tests/test_webgl_stencil_with_no_depth.html
@@ +29,5 @@
> +<div id='mochi-to-testcase-output'></div>
> +
> +
> +<script>
> +// Imported from: driver-info.js

We should use the pre-mochi-to-standalone.py file.

Comment 7

[u480271]

(In reply to Jeff Gilbert [:jgilbert] from comment #6)
> We should use the pre-mochi-to-standalone.py file.

Where do I get that from?

Comment 8

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #7)
> (In reply to Jeff Gilbert [:jgilbert] from comment #6)
> > We should use the pre-mochi-to-standalone.py file.
> 
> Where do I get that from?

Oh, did you base your test off of one of the testcases I post?
If you take a test from content/canvas/test/webgl/non-conf-tests/, and run mochi-to-testcase.py on it, it'll spit out this no-include version of the test for posting to bugs. Our committed-to-tree testcases should use the includes, though.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8389597 [details] [diff] [review]
If context is created without depth, don't enable or advertise support for depth, even if a depth buffer exists.

Hm, I don't quite understand this -- the spec says that we're allowed to advertise/have more capabilities than requested.  In some cases we have to -- e.g. where we can only get depth+stencil or neither, not one or the other.

Doing things this way will cause problems.  Specifically, if the underlying buffer *does* have depth, things like enabling/disabling DEPTH_TEST will have an effect -- but the programmer will be very confused, because DEPTH_BITS will be 0, and opts.depth == false!  If we really want to make the requested options exactly what's returned, then we have to fully support emulating no-depth when we have depth.  I don't think this is worth it.

Comment 10

[Kelsey Gilbert [:jgilbert]]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #9)
> Comment on attachment 8389597 [details] [diff] [review]
> If context is created without depth, don't enable or advertise support for
> depth, even if a depth buffer exists.
> 
> Hm, I don't quite understand this -- the spec says that we're allowed to
> advertise/have more capabilities than requested.  In some cases we have to
> -- e.g. where we can only get depth+stencil or neither, not one or the other.
> 
I can take a look at the spec again, but we decided against this. In the previous case, Chrome was always giving depth-stencil. This caused demos to 'break' in Firefox, since we didn't give a stencil buffer unless you asked for one.

> Doing things this way will cause problems.  Specifically, if the underlying
> buffer *does* have depth, things like enabling/disabling DEPTH_TEST will
> have an effect -- but the programmer will be very confused, because
> DEPTH_BITS will be 0, and opts.depth == false!  If we really want to make
> the requested options exactly what's returned, then we have to fully support
> emulating no-depth when we have depth.  I don't think this is worth it.
No, because we can just disable anything depth-related for draws against a backbuffer we're pretending not to have a depth buffer for. (or the same for stencil)

Comment 11

[Kelsey Gilbert [:jgilbert]]

From the spec:
The depth, stencil and antialias attributes, when set to true, are requests, not requirements. The WebGL implementation should make a best effort to honor them. When any of these attributes is set to false, however, the WebGL implementation must not provide the associated functionality. Combinations of attributes not supported by the WebGL implementation or graphics hardware shall not cause a failure to create a WebGLRenderingContext. The actual context parameters are set to the attributes of the created drawing buffer. The alpha, premultipliedAlpha and preserveDrawingBuffer attributes must be obeyed by the WebGL implementation.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Ugh. In that case, yeah, we have to do the work to actually emulate "disabling" of those attributes.

I'm honestly not sure if that's possible or worth it.  It might be worth looking at a spec change instead here.  I guess you can get by with a lot by disabling depth test and disabling depth write always... but I don't know if you can, I mean what is the depth value that's used if there is no depth buffer and depth test is enabled?  That seems like it might be a valid thing to do.

Comment 13

[u480271]

Then we need to change the WebGL 1.0.3 conformance tests because we are failing a test because of this very issue. 

https://www.khronos.org/registry/webgl/sdk/tests/conformance/context/context-attributes-alpha-depth-stencil-antialias.html

Comment 14

[Kelsey Gilbert [:jgilbert]]

I'm quite confident we can handle the current restrictive spec fine. We should at least try to match the spec before giving up on it. There're good reasons for why we wanted this change, and unless it's not possible, I'd really really like to keep it this way.

Comment 17

[Luke]

https://www.khronos.org/registry/webgl/sdk/tests/conformance/context/context-attributes-alpha-depth-stencil-antialias.html
Only fails for the ANGLE backend. The OpenGL backend is passing on 38.0a1 (2015-02-21).

Comment 18

[u480271]

I'm no longer working on WebGL