Framebuffer Object Attachment fails 1.0.2 conformance test

RESOLVED FIXED in Firefox 39

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: kamidphish, Assigned: jgilbert)

Tracking

30 Branch
mozilla39
Points:
---

Firefox Tracking Flags

(firefox29 unaffected, firefox30 affected, firefox31 affected, firefox32 affected, firefox39 fixed)

Details

(Whiteboard: webgl-conformance, URL)

Attachments

(2 attachments)

Fix for Bug 981240 causes framebuffer-object-attachment conformance test to fail in the following areas involving depth attachments:

test: DEPTH_COMPONENT16 vs DEPTH_STENCIL with delete
PASS gl.checkFramebufferStatus(gl.FRAMEBUFFER) is gl.FRAMEBUFFER_COMPLETE
PASS should be green
test deleting second renderbuffer
PASS gl.checkFramebufferStatus(gl.FRAMEBUFFER) is gl.FRAMEBUFFER_COMPLETE
PASS getError was expected value: NO_ERROR :
FAIL should be green
at (0, 0) expected: 0,255,0,255 was 255,0,0,255

test: DEPTH_COMPONENT16 vs DEPTH_STENCIL with unbind
PASS gl.checkFramebufferStatus(gl.FRAMEBUFFER) is gl.FRAMEBUFFER_COMPLETE
PASS should be green
test unbinding second renderbuffer
PASS gl.checkFramebufferStatus(gl.FRAMEBUFFER) is gl.FRAMEBUFFER_COMPLETE
PASS getError was expected value: NO_ERROR :
FAIL should be green
at (0, 0) expected: 0,255,0,255 was 255,0,0,255

test: DEPTH_STENCIL vs DEPTH_COMPONENT16 with delete
PASS gl.checkFramebufferStatus(gl.FRAMEBUFFER) is gl.FRAMEBUFFER_COMPLETE
PASS should be green
test deleting second renderbuffer
PASS gl.checkFramebufferStatus(gl.FRAMEBUFFER) is gl.FRAMEBUFFER_COMPLETE
PASS getError was expected value: NO_ERROR :
FAIL should be green
at (0, 0) expected: 0,255,0,255 was 255,0,0,255

test: DEPTH_STENCIL vs DEPTH_COMPONENT16 with unbind
PASS gl.checkFramebufferStatus(gl.FRAMEBUFFER) is gl.FRAMEBUFFER_COMPLETE
PASS should be green
test unbinding second renderbuffer
PASS gl.checkFramebufferStatus(gl.FRAMEBUFFER) is gl.FRAMEBUFFER_COMPLETE
PASS getError was expected value: NO_ERROR :
FAIL should be green
at (0, 0) expected: 0,255,0,255 was 255,0,0,255
PASS getError was expected value: NO_ERROR :
PASS fbo = gl.createFramebuffer() is non-null.
PASS colorBuffer = gl.createRenderbuffer() is non-null.
PASS depthBuffer = gl.createRenderbuffer() is non-null.
PASS getError was expected value: NO_ERROR :
Flags: needinfo?(jgilbert)

Comment 1

4 years ago
This test is still failing with the D3D9, D3D11, and native OGL backends on Firefox 38.0a1 (2015-01-30). However it passes on both Chrome 42.0.2291.1 canary and IE 11.
(Assignee)

Comment 2

4 years ago
Ok, now I need to remember what was causing this...
Flags: needinfo?(jgilbert)
OS: Mac OS X → All
Hardware: x86 → All

Comment 3

4 years ago
The title suggests this is newer 1.0.3 failure, but it's also one of the tests preventing Firefox from passing the 1.0.2 suite.

https://www.khronos.org/registry/webgl/conformance-suites/1.0.2/conformance/renderbuffers/framebuffer-object-attachment.html

Also could a mozillian please add webgl-conformance to the whiteboard?
(Assignee)

Updated

4 years ago
Summary: Framebuffer Object Attachment fails 1.0.3 conformance test → Framebuffer Object Attachment fails 1.0.2 conformance test
Whiteboard: webgl-conformance
(Assignee)

Comment 4

4 years ago
Created attachment 8577539 [details] [diff] [review]
0001-Refactor-attach-detach-for-FB-attachments.patch

This one's longer, because I think refactoring this and pruning some of the code paths makes it more clear and less error-prone.
Attachment #8577539 - Flags: review?(dglastonbury)
Comment on attachment 8577539 [details] [diff] [review]
0001-Refactor-attach-detach-for-FB-attachments.patch

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

r+ with nits.

::: dom/canvas/WebGLFramebuffer.cpp
@@ +147,2 @@
>      mTexturePtr = nullptr;
>      mRenderbufferPtr = rb;

Should mTexImageTarget, mTexImageLevel be cleared here?

@@ +507,5 @@
>          break;
>      }
>  
> +    if (attachPoint >= LOCAL_GL_COLOR_ATTACHMENT1) {
> +        size_t colorAttachmentId = attachPoint.get() - LOCAL_GL_COLOR_ATTACHMENT0;

If you change this to LOCAL_GL_COLOR_ATTACHMENT1 then you won't need to...

@@ +510,5 @@
> +    if (attachPoint >= LOCAL_GL_COLOR_ATTACHMENT1) {
> +        size_t colorAttachmentId = attachPoint.get() - LOCAL_GL_COLOR_ATTACHMENT0;
> +        if (colorAttachmentId < WebGLContext::kMaxColorAttachments) {
> +            EnsureColorAttachPoints(colorAttachmentId);
> +            return mMoreColorAttachments[colorAttachmentId - 1];

... index with colorAttachmentId - 1 here.

::: dom/canvas/WebGLFramebuffer.h
@@ +33,4 @@
>  public:
>      MOZ_DECLARE_REFCOUNTED_TYPENAME(WebGLFramebuffer)
>  
> +    class AttachPoint

Why Attachment -> AttachPoint?

@@ +152,2 @@
>      }
> +    const AttachPoint& ColorAttachment(size_t colorAttachmentId) const {

Insert blank line above.
Attachment #8577539 - Flags: review?(dglastonbury) → review+
(Assignee)

Comment 6

4 years ago
(In reply to Dan Glastonbury :djg :kamidphish from comment #5)
> Comment on attachment 8577539 [details] [diff] [review]
> 0001-Refactor-attach-detach-for-FB-attachments.patch
> 
> Review of attachment 8577539 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with nits.
> 
> ::: dom/canvas/WebGLFramebuffer.cpp
> @@ +147,2 @@
> >      mTexturePtr = nullptr;
> >      mRenderbufferPtr = rb;
> 
> Should mTexImageTarget, mTexImageLevel be cleared here?
It's not necessary, since mTexImageTarget/Level are effectively undefined when a Renderbuffer is attached.
> 
> @@ +507,5 @@
> >          break;
> >      }
> >  
> > +    if (attachPoint >= LOCAL_GL_COLOR_ATTACHMENT1) {
> > +        size_t colorAttachmentId = attachPoint.get() - LOCAL_GL_COLOR_ATTACHMENT0;
> 
> If you change this to LOCAL_GL_COLOR_ATTACHMENT1 then you won't need to...
True!
> 
> @@ +510,5 @@
> > +    if (attachPoint >= LOCAL_GL_COLOR_ATTACHMENT1) {
> > +        size_t colorAttachmentId = attachPoint.get() - LOCAL_GL_COLOR_ATTACHMENT0;
> > +        if (colorAttachmentId < WebGLContext::kMaxColorAttachments) {
> > +            EnsureColorAttachPoints(colorAttachmentId);
> > +            return mMoreColorAttachments[colorAttachmentId - 1];
> 
> ... index with colorAttachmentId - 1 here.
True!
> 
> ::: dom/canvas/WebGLFramebuffer.h
> @@ +33,4 @@
> >  public:
> >      MOZ_DECLARE_REFCOUNTED_TYPENAME(WebGLFramebuffer)
> >  
> > +    class AttachPoint
> 
> Why Attachment -> AttachPoint?
Because these aren't really attachments, they're points that we can attach things to.
The AttachPoint objects are always alive (non-null) and embedded in the WebGLFramebuffer class. We then can attach RBs and Texs to these points.

> 
> @@ +152,2 @@
> >      }
> > +    const AttachPoint& ColorAttachment(size_t colorAttachmentId) const {
> 
> Insert blank line above.
Ok.
(Assignee)

Comment 10

4 years ago
Please don't back out unexpected passes while I'm actively responding to pings. They're often simple fixes. (I can unmark the test as failing)
Flags: needinfo?(jgilbert)
Not sure what you relanded was what you intended to reland, since it didn't actually mark the unexpected passes as passing, but it did remove an expectAssertions() call that was necessary (and a comment that... whatever).

Is test_conformance__textures__texture-npot-video.html generated? It doesn't say that it is, which is probably why dbaron put that expectAssertions() in there directly.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/fce881b139dc and https://hg.mozilla.org/integration/mozilla-inbound/rev/c39e4d4e5ba1.
(Assignee)

Comment 12

4 years ago
(In reply to Phil Ringnalda (:philor) from comment #11)
> Not sure what you relanded was what you intended to reland, since it didn't
> actually mark the unexpected passes as passing, but it did remove an
> expectAssertions() call that was necessary (and a comment that... whatever).
> 
> Is test_conformance__textures__texture-npot-video.html generated? It doesn't
> say that it is, which is probably why dbaron put that expectAssertions() in
> there directly.
> 
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/fce881b139dc and
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c39e4d4e5ba1.

Looks like wires got crossed here.

Yes, it's generated. I'll add a header. I will note that review from a module peer would have caught this earlier.
(Assignee)

Comment 13

4 years ago
Created attachment 8582236 [details] [diff] [review]
0002-Mark-obj-del-behav-as-passing-on-OSX-10.10.patch
https://hg.mozilla.org/mozilla-central/rev/319a40f0fce4
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.