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

RESOLVED FIXED in mozilla9

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bjacob, Assigned: drs)

Tracking

Trunk
mozilla9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

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
(Assignee)

Updated

6 years ago
Assignee: nobody → dsherk
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Comment 2

6 years ago
Created attachment 555586 [details] [diff] [review]
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).
Attachment #555586 - Flags: review?(bjacob)
(Reporter)

Comment 3

6 years ago
(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.
(Reporter)

Updated

6 years ago
Attachment #555586 - Flags: review?(bjacob) → review+
(Reporter)

Comment 4

6 years ago
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.
Attachment #555586 - Flags: review+ → review-
(Assignee)

Comment 5

6 years ago
Created attachment 555625 [details] [diff] [review]
Patch v1.1

Now only allows 0 dimensions, not negative.
Attachment #555586 - Attachment is obsolete: true
Attachment #555625 - Flags: review?(bjacob)
(Assignee)

Comment 6

6 years ago
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."
(Reporter)

Comment 7

6 years ago
(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.
(Reporter)

Comment 8

6 years ago
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!
Attachment #555625 - Flags: review?(bjacob) → review+
(Assignee)

Comment 9

6 years ago
Created attachment 555739 [details] [diff] [review]
Patch v1.2

Updated comment.
Attachment #555625 - Attachment is obsolete: true
Attachment #555739 - Flags: review?(bjacob)
(Assignee)

Updated

6 years ago
Attachment #555739 - Flags: review?(bjacob)
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"
Attachment #555739 - Flags: review+
Inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/97bdf9371319
http://hg.mozilla.org/mozilla-central/rev/97bdf9371319
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.