1017865 - Framebuffer Object Attachment fails 1.0.2 conformance test

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[u480271]

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 :

Comment 1

[Luke]

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.

Comment 2

[Kelsey Gilbert [:jgilbert]]

Ok, now I need to remember what was causing this...

Comment 3

[Luke]

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?

Comment 4

[Kelsey Gilbert [:jgilbert]]

Attachment: 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.

Comment 5

[u480271]

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.

Comment 6

[Kelsey Gilbert [:jgilbert]]

(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.

Comment 7

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff27f90ed289

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for an unexpected pass in gl tests https://hg.mozilla.org/integration/mozilla-inbound/rev/8015ad9aa698

https://treeherder.mozilla.org/logviewer.html#?job_id=7685151&repo=mozilla-inbound

Comment 9

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7657171e034
https://hg.mozilla.org/integration/mozilla-inbound/rev/90758efa9a09

Comment 10

[Kelsey Gilbert [:jgilbert]]

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)

Comment 11

[Phil Ringnalda (:philor)]

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.

Comment 12

[Kelsey Gilbert [:jgilbert]]

(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.

Comment 13

[Kelsey Gilbert [:jgilbert]]

Attachment: 0002-Mark-obj-del-behav-as-passing-on-OSX-10.10.patch

Comment 14

[Kelsey Gilbert [:jgilbert]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=44a40ac4dfd0

 https://hg.mozilla.org/integration/mozilla-inbound/rev/319a40f0fce4

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/319a40f0fce4