Closed Bug 1017865 Opened 11 years ago Closed 10 years ago

Framebuffer Object Attachment fails 1.0.2 conformance test

Categories

(Core :: Graphics: CanvasWebGL, defect)

30 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox29 --- unaffected
firefox30 --- affected
firefox31 --- affected
firefox32 --- affected
firefox39 --- fixed

People

(Reporter: u480271, Assigned: jgilbert)

References

()

Details

(Whiteboard: webgl-conformance)

Attachments

(2 files)

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)
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.
Ok, now I need to remember what was causing this...
Flags: needinfo?(jgilbert)
OS: Mac OS X → All
Hardware: x86 → All
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?
Summary: Framebuffer Object Attachment fails 1.0.3 conformance test → Framebuffer Object Attachment fails 1.0.2 conformance test
Whiteboard: webgl-conformance
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+
(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.
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.
(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.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: