Closed Bug 616655 Opened 9 years ago Closed 9 years ago

Pass the tex-input-validation.html test

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bjacob, Assigned: bjacob)

Details

Attachments

(2 files)

Attached patch pass the testSplinter Review
Several different things in this patch, making us pass tex-input-validation.html:

* a serious change in WebGLFramebuffer: it used to have a member mColorAttachmentHasAlpha, but that was not properly updated when the attachment was updated (renderbufferStorage...). It was not needed anyway. This patch adds a HasAlpha() method to WebGLFramebufferAttachment.

* fixes in copyTex[Sub]Image2D, to check that the framebuffer has the required channels. Checking for alpha was needed to pass the test. Also added a check for luminance, will update the test.

* a fix in texParameterf: it used to generate INVALID_VALUE errors while texParameteri generated INVALID_ENUM, that made us fail the test, now both are generating INVALID_ENUM which may feel weird for floats; anyway there are only enum params at the moment.
Attachment #495215 - Flags: review?(vladimir)
Comment on attachment 495215 [details] [diff] [review]
pass the test

>+    PRBool texFormatRequiresLuminance = internalformat == LOCAL_GL_LUMINANCE ||
>+                                        internalformat == LOCAL_GL_LUMINANCE_ALPHA;
>+    // a framebuffer can never have a luminance channel, afaik
>+    if (texFormatRequiresLuminance)
>+        return ErrorInvalidOperation("copyTexImage2D: texture format requires a luminance channel "
>+                                     "but the framebuffer doesn't have one");

Hm, is there an explicit test for this?  What spec language does it refer to?  GL doesn't have a notion of a "luminance channel", setting luminance is equivalent to setting rgb to the same value.  So I think it's possible to copy LUMINANCE -> RGB and vice versa; one of the ES glCopyTexImage2D examples is creating a LUMINANCE texture from a RGB framebuffer.  If there is an explicit test, can you post about it/file a bug?  I think it's wrong.

>+    PRBool texFormatRequiresLuminance = format == LOCAL_GL_LUMINANCE ||
>+                                        format == LOCAL_GL_LUMINANCE_ALPHA;
>+    // a framebuffer can never have a luminance channel, afaik
>+    if (texFormatRequiresLuminance)
>+        return ErrorInvalidOperation("copyTexSubImage2D: texture format requires a luminance channel "
>+                                     "but the framebuffer doesn't have one");

same thing

Looks great other than that, so I'll r+.. but we should figure out the luminance bit.
Attachment #495215 - Flags: review?(vladimir) → review+
Oh ok, you can probably ignore me on this one :) There is no test checking for this, actually I had just proposed one:
http://www.khronos.org/bugzilla/show_bug.cgi?id=390

But the spec proves me wrong: in the GL ES 2.0.24 spec page 69, table 3.9 and explanation "For example, an RGB color buffer can be used to create LUMINANCE or RGB textures".

Will post an update patch here; closing the khronos bug as invalid.
Attached patch updated patchSplinter Review
carrying forward r+

drop the bogus check about luminance textures.
Assignee: nobody → bjacob
Status: NEW → ASSIGNED
Attachment #495298 - Flags: review+
Attachment #495298 - Flags: approval2.0+
http://hg.mozilla.org/mozilla-central/rev/db4ae19fd5c9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.