680724 - framebuffer-object-attachment.html test fails because we don't allow zero-sized renderbuffers

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Our implementation of the WebGL renderbufferStorage function rejects 0 width and 0 height:

http://hg.mozilla.org/mozilla-central/file/6dc468c41136/content/canvas/src/WebGLContextGL.cpp#l3169

Apparently this is non-conformant behavior as it makes us fail this conformance test:

https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/framebuffer-object-attachment.html

Assuming that OpenGL drivers get it right, this should be just a matter of changing this check (content/canvas/src/WebGLContextGL.cpp, line 3169).

However: it would be nice if someone could also check the spec to see if it's actually said somewhere that 0-sized renderbuffers are allowed. After all, conformance tests are sometimes wrong. The WebGL spec is here,
http://www.khronos.org/registry/webgl/specs/latest/
however for most things it just refers to the OpenGL ES 2.0 spec which is here:
http://www.khronos.org/registry/gles/specs/2.0/es_full_spec_2.0.25.pdf

Comment 1

[Doug Sherk (:drs) (inactive)]

Ok, so after some pretty thorough looking through both the WebGL and OpenGL specs, I didn't come across anything saying that 0 height/width render buffers are invalid. I don't really understand what's going on behind the scenes, but my interpretation of what I read is the following:

* Render buffers can be altered after creation, so it's possible that you'd create one, not use it for a while, alter the dimensions to be valid, and then use it.
* Render buffers don't do anything (or at least their dimensions aren't used) until they're attached to frame buffers, which themselves have a check for whether or not the dimensions are valid.

Based on these, it makes sense to me to allow creation of 0 height/width render buffers.

Comment 2

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.0

Proposed patch, only removes the two lines mentioned by bjacob. I didn't remove the test file from failing_tests_* because it still fails for other reasons (i.e. this fixed a few of the failures, but not all of them, the rest being unrelated to this bug).

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Doug Sherk from comment #1)
> Ok, so after some pretty thorough looking through both the WebGL and OpenGL
> specs, I didn't come across anything saying that 0 height/width render
> buffers are invalid.

OK, thanks for checking that.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 555586 [details] [diff] [review]
Patch v1.0

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

::: content/canvas/src/WebGLContextGL.cpp
@@ -3166,5 @@
>      if (target != LOCAL_GL_RENDERBUFFER)
>          return ErrorInvalidEnumInfo("renderbufferStorage: target", target);
>  
> -    if (width <= 0 || height <= 0)
> -        return ErrorInvalidValue("renderbufferStorage: width and height must be > 0");

Sorry, been too quick. WebGLsizei is actually a signed type, meaning that width and height could be negative. We still want to forbid negative values.

Comment 5

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.1

Now only allows 0 dimensions, not negative.

Comment 6

[Doug Sherk (:drs) (inactive)]

See OpenGL 2.0 spec, section 2.5:

"• If a negative number is provided where an argument of type sizei is spec- ified, the error INVALID_VALUE is generated."

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Doug Sherk from comment #6)
> See OpenGL 2.0 spec, section 2.5:

For the record: what's meant here is OpenGL ES 2.0 spec, not OpenGL 2.0 spec.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 555625 [details] [diff] [review]
Patch v1.1

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

Good, with one nit:

::: content/canvas/src/WebGLContextGL.cpp
@@ +3170,1 @@
>          return ErrorInvalidValue("renderbufferStorage: width and height must be > 0");

Should be ">= 0" in the warning message!

Comment 9

[Doug Sherk (:drs) (inactive)]

Attachment: Patch v1.2

Updated comment.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 555739 [details] [diff] [review]
Patch v1.2

Since I already r+'d, you could have r+'d yourself saying "Carrying forward r+ from bjacob"

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

Inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/97bdf9371319

Comment 12

[Ed Morley [:emorley]]

http://hg.mozilla.org/mozilla-central/rev/97bdf9371319