Last Comment Bug 680724 - framebuffer-object-attachment.html test fails because we don't allow zero-sized renderbuffers
: framebuffer-object-attachment.html test fails because we don't allow zero-siz...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Doug Sherk (:drs) (inactive)
:
: Milan Sreckovic [:milan]
Mentors:
https://cvs.khronos.org/svn/repos/reg...
Depends on:
Blocks: webgl-conformance
  Show dependency treegraph
 
Reported: 2011-08-20 20:22 PDT by Benoit Jacob [:bjacob] (mostly away)
Modified: 2011-08-25 18:33 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1.0 (1.15 KB, patch)
2011-08-24 16:27 PDT, Doug Sherk (:drs) (inactive)
jacob.benoit.1: review-
Details | Diff | Splinter Review
Patch v1.1 (1.12 KB, patch)
2011-08-24 19:45 PDT, Doug Sherk (:drs) (inactive)
jacob.benoit.1: review+
Details | Diff | Splinter Review
Patch v1.2 (1.24 KB, patch)
2011-08-25 08:20 PDT, Doug Sherk (:drs) (inactive)
jacob.benoit.1: review+
Details | Diff | Splinter Review

Description Benoit Jacob [:bjacob] (mostly away) 2011-08-20 20:22:47 PDT
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) 2011-08-24 15:42:02 PDT
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) 2011-08-24 16:27:07 PDT
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).
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2011-08-24 19:15:05 PDT
(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) 2011-08-24 19:28:12 PDT
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) 2011-08-24 19:45:10 PDT
Created attachment 555625 [details] [diff] [review]
Patch v1.1

Now only allows 0 dimensions, not negative.
Comment 6 Doug Sherk (:drs) (inactive) 2011-08-24 19:45:45 PDT
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) 2011-08-25 07:59:51 PDT
(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) 2011-08-25 08:01:00 PDT
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) 2011-08-25 08:20:46 PDT
Created attachment 555739 [details] [diff] [review]
Patch v1.2

Updated comment.
Comment 10 Benoit Jacob [:bjacob] (mostly away) 2011-08-25 11:25:18 PDT
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) 2011-08-25 11:30:51 PDT
Inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/97bdf9371319
Comment 12 Ed Morley [:emorley] 2011-08-25 18:33:02 PDT
http://hg.mozilla.org/mozilla-central/rev/97bdf9371319

Note You need to log in before you can comment on or make changes to this bug.