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)
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)
49.15 KB,
patch
|
u480271
:
review+
|
Details | Diff | Splinter Review |
1.91 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 2•10 years ago
|
||
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?
Assignee | ||
Updated•10 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•10 years ago
|
||
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•10 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 7•10 years ago
|
||
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
Flags: needinfo?(jgilbert)
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Comment 10•10 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)
Comment 11•10 years ago
|
||
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•10 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•10 years ago
|
||
Assignee | ||
Comment 14•10 years ago
|
||
Comment 15•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 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.
Description
•