Closed
Bug 1136428
Opened 10 years ago
Closed 9 years ago
WebGL 1.0.3 conformance error: conformance/extensions/webgl-draw-buffers.html
Categories
(Core :: Graphics: CanvasWebGL, defect)
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: lukebenes, Assigned: kyle_fung)
Details
(Whiteboard: webgl-conformance gfx-noted)
Attachments
(4 files, 4 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150224030228
Steps to reproduce:
https://www.khronos.org/registry/webgl/sdk/tests/conformance/extensions/webgl-draw-buffers.html?webglVersion=1
Actual results:
conformance/extensions/webgl-draw-buffers.html (258 of 260 passed)
failed: at (0, 0) expected: 0,0,0,0 was 255,0,0,255
failed: getError expected: NO_ERROR. Was INVALID_OPERATION : there should be no errors
Expected results:
Chrome passes all the tests. Firefox fails the test with the ANGLE backend.
Updated•10 years ago
|
Component: Untriaged → Canvas: WebGL
Product: Firefox → Core
Updated•10 years ago
|
Blocks: webgl-1.0.2
Whiteboard: webgl-conformance
Updated•10 years ago
|
Whiteboard: webgl-conformance → webgl-conformance gfx-noted
No longer blocks: webgl-1.0.2
Summary: WebGL conformance error: conformance/extensions/webgl-draw-buffers.html → WebGL 1.0.3 conformance error: conformance/extensions/webgl-draw-buffers.html
Updated•10 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 1•9 years ago
|
||
This was tested to fail on the following devices:
- GIADA, MACBOOK_AIR_OSX, MACBOOK_PRO_OSX, MACBOOK_PRO_WIN, MACMINI, MACPRO, SURFACE, WINDBOX, HASWELL, HPOMEN
On MACBOOK_AIR_OSX, the test even crashed. See https://bugzilla.mozilla.org/show_bug.cgi?id=1178600.
The test passed on the following devices:
- SPARK, MACBOOK_AIR_WIN, NEXUS-4, NEXUS-5
See https://bugzilla.mozilla.org/show_bug.cgi?id=1178601 for hardware configuration details of these systems.
> failed: getError expected: NO_ERROR. Was INVALID_OPERATION : there should be no errors
This is due to us not using gl->fDrawBuffers(n, buffers) properly in WebGLContext::ForceClearFramebufferWithDefaultValues.
According to the spec for gles we cannot use DrawBuffers on the default framebuffer while passing in n != 1 and buffers[0] != GL_BACK or buffers[0] != GL_NONE. We are passing in n = 8 and buffers[0] = COLOR_ATTACHMENT0 hence the INVALID_OPERATION.
I have a patch for this and I'll upload it when it's finalized.
Spec: https://www.khronos.org/registry/gles/extensions/EXT/EXT_draw_buffers.txt
> failed: at (0, 0) expected: 0,0,0,0 was 255,0,0,255
This is failing because the WebGL context is losing the state of its draw buffers after publishing a frame in GLScreenBuffer::PublishFrame.
Within PublishFrame, GLScreenBuffer::Attach is called and the context will lose its draw buffers state after the call to Move(read).
> bool
> GLScreenBuffer::Attach(SharedSurface* surf, const gfx::IntSize& size)
> {
> ScopedBindFramebuffer autoFB(mGL);
>
> if (mRead && SharedSurf())
> SharedSurf()->UnlockProd();
>
> surf->LockProd();
> ...
> if (!drawOk || !readOk) {
> surf->UnlockProd();
> return false;
> }
>
> if (draw)
> mDraw = Move(draw);
>
> mRead = Move(read);
This Move causes ReadBuffer::~ReadBuffer() to be invoked which will delete the framebuffer without retaining its draw buffers state. So when we make a new framebuffer, it does not have the exact same state as the previous framebuffer.
Due to the framebuffers not retaining their state after a composite, in the test, the drawbuffer is no longer set to GL_NONE as it should be, but reset to GL_BACK. Hence we render to the backbuffer instead of to nothing and the canvas is red rather than clear.
This only happens when we use ANGLE shared surfaces, if I turn off surface sharing, the test works fine on ANGLE.
Jeff, is there a clean way to resolve this?
Flags: needinfo?(jgilbert)
Comment 4•9 years ago
|
||
It looks like you just need expand what I did with mUserReadBufferMode to support draw_buffer.
Flags: needinfo?(jgilbert)
Not entirely sure if this is the way to go by setting the draw buffer through the ReadBuffer object.
Attachment #8641144 -
Flags: feedback?(jgilbert)
It looks like checking the framebuffer will throw an error as well. This happens with ANGLE on or off. Chrome runs this without problems.
(In reply to kfung from comment #6)
> Created attachment 8641270 [details]
> framebuffer-test.html
>
> It looks like checking the framebuffer will throw an error as well. This
> happens with ANGLE on or off. Chrome runs this without problems.
Oh yeah, this needs to be run in the WebGL 1.0.3 test suite. This file needs to be in "conformance-suites/1.0.3/conformance/extensions/"
The problem seems to be that we think there are more color attachments than there are supported in WebGLFrameBuffer::FinalizeAttachments, where we loop through the color attachments of the WebGLFramebuffer and call glFramebufferRenderbuffer on each attachment.
The variable moreColorAttachmentCount is set to 15, and the glFramebufferRenderbuffer call fails on the 8th iteratoin (on LOCAL_GL_COLOR_ATTACHMENT8).
>const size_t moreColorAttachmentCount = mMoreColorAttachments.Length();
>for (size_t i = 0; i < moreColorAttachmentCount; i++) {
> GLenum attachPoint = LOCAL_GL_COLOR_ATTACHMENT0 + 1 + i;
> mMoreColorAttachments[i].FinalizeAttachment(gl, attachPoint);
>}
It seems that mMoreColorAttachments needs to have its length initialization fixed.
Issue in comment 8 addressed here. Replaced some uses of kMaxColorAttachments with mContext->mGLMaxColorAttachments.
Attachment #8641779 -
Flags: review?(jgilbert)
Assignee | ||
Comment 10•9 years ago
|
||
Second issue from comment 3 addressed. Caches the draw buffer mode of GLScreenBuffer and resets it after surface attachment operations.
Attachment #8641144 -
Attachment is obsolete: true
Attachment #8641144 -
Flags: feedback?(jgilbert)
Attachment #8641834 -
Flags: review?(jgilbert)
Comment 11•9 years ago
|
||
Comment on attachment 8641779 [details] [diff] [review]
max-color-attachments.patch
Review of attachment 8641779 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/canvas/WebGLFramebuffer.cpp
@@ +508,5 @@
> }
>
> if (attachPoint >= LOCAL_GL_COLOR_ATTACHMENT1) {
> size_t colorAttachmentId = attachPoint.get() - LOCAL_GL_COLOR_ATTACHMENT0;
> + if (colorAttachmentId < (size_t) mContext->mGLMaxColorAttachments) {
No space between type and casted value.
Attachment #8641779 -
Flags: review?(jgilbert) → review+
Comment 12•9 years ago
|
||
Comment on attachment 8641834 [details] [diff] [review]
preserve-draw-buffer.patch
Review of attachment 8641834 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/gl/GLScreenBuffer.cpp
@@ +855,5 @@
> mGL->fReadBuffer(internalMode);
> }
>
> +void
> +ReadBuffer::SetDrawBuffer(GLenum userMode) const
No need to duplicate code. This should be:
static void SetDrawBuffer(GLContext* gl, GLuint drawFB, GLenum userMode);
Attachment #8641834 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 13•9 years ago
|
||
Addressed the issue from comment 2 here. Only reset the draw buffer state to an array of 1 draw buffers if the default framebuffer is used (The OpenGL ES spec glDrawBuffers cannot be used on arrays larger than 1).
Attachment #8643156 -
Flags: review?(jgilbert)
Assignee | ||
Comment 14•9 years ago
|
||
Jeff, I noticed that we are failing the preserve tests, even with the above fixes, but only when the canvas is out of view. It seems that we do not call WebGLContext::ForceClearFramebufferWithDefaultValues when the canvas is out of the view of the window, and so the canvas does not get cleared to (0, 0, 0, 0).
I think this is because we are not compositing the canvas when the canvas is out of view.
Is this behavior conformant to the spec (Section 2.2 The Draw Buffer)?
Flags: needinfo?(jgilbert)
Comment 15•9 years ago
|
||
(In reply to kfung from comment #14)
> Jeff, I noticed that we are failing the preserve tests, even with the above
> fixes, but only when the canvas is out of view. It seems that we do not call
> WebGLContext::ForceClearFramebufferWithDefaultValues when the canvas is out
> of the view of the window, and so the canvas does not get cleared to (0, 0,
> 0, 0).
>
> I think this is because we are not compositing the canvas when the canvas is
> out of view.
>
> Is this behavior conformant to the spec (Section 2.2 The Draw Buffer)?
Yes it is conformant. We don't always composite the canvas, and that section applies only when we choose to composite it.
Flags: needinfo?(jgilbert)
Comment 16•9 years ago
|
||
Comment on attachment 8643156 [details] [diff] [review]
invalid-op-force-clear.patch
Review of attachment 8643156 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/canvas/WebGLContext.cpp
@@ +1380,5 @@
> if (drawBuffersIsEnabled) {
>
> GLenum drawBuffersCommand[WebGLContext::kMaxColorAttachments] = { LOCAL_GL_NONE };
>
> + for (int32_t i = 0; i < mGLMaxDrawBuffers; i++) {
Oh man, this stuff is *so slow*. I'm not sure why we even support multiple draw buffers here.
@@ +1448,5 @@
> // Restore GL state after clearing.
> if (initializeColorBuffer) {
> + if (drawBuffersIsEnabled && usingDefaultFrameBuffer) {
> + gl->Screen()->SetDrawBuffer(currentDrawBuffers[0]);
> + } else if (shouldOverrideDrawBuffers) {
There should really be two tiers of branch here, the outer one being for `drawBuffersIsEnabled`. (mirroring the logical arrangement above)
Attachment #8643156 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 17•9 years ago
|
||
Fixed styling issue.
Attachment #8641779 -
Attachment is obsolete: true
Assignee | ||
Comment 18•9 years ago
|
||
Addressed control flow issue.
Attachment #8643156 -
Attachment is obsolete: true
Assignee | ||
Comment 19•9 years ago
|
||
Took out duplicated code.
Attachment #8641834 -
Attachment is obsolete: true
Assignee | ||
Comment 20•9 years ago
|
||
Keywords: checkin-needed
Comment 21•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e1b414266f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/af87603b35cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/0993bd17c823
Keywords: checkin-needed
Comment 22•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2e1b414266f1
https://hg.mozilla.org/mozilla-central/rev/af87603b35cd
https://hg.mozilla.org/mozilla-central/rev/0993bd17c823
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Reporter | ||
Comment 23•8 years ago
|
||
Due to a recent regression this is Failing again now. See Bug 1290774
Comment 24•8 years ago
|
||
(In reply to Luke from comment #23)
> Due to a recent regression this is Failing again now. See Bug 1290774
It's not a regression. The test was changed.
You need to log in
before you can comment on or make changes to this bug.
Description
•