Closed Bug 1136428 Opened 5 years ago Closed 4 years ago

WebGL 1.0.3 conformance error: conformance/extensions/webgl-draw-buffers.html

Categories

(Core :: Canvas: WebGL, defect)

39 Branch
x86_64
Windows 7
defect
Not set

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)

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.
Component: Untriaged → Canvas: WebGL
Product: Firefox → Core
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
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)
Assignee: nobody → kfung
It looks like you just need expand what I did with mUserReadBufferMode to support draw_buffer.
Flags: needinfo?(jgilbert)
Attached patch draw-buffer-preserve.patch (obsolete) — Splinter Review
Not entirely sure if this is the way to go by setting the draw buffer through the ReadBuffer object.
Attachment #8641144 - Flags: feedback?(jgilbert)
Attached file 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.
(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.
Attached patch max-color-attachments.patch (obsolete) — Splinter Review
Issue in comment 8 addressed here. Replaced some uses of kMaxColorAttachments with mContext->mGLMaxColorAttachments.
Attachment #8641779 - Flags: review?(jgilbert)
Attached patch preserve-draw-buffer.patch (obsolete) — Splinter Review
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 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 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+
Attached patch invalid-op-force-clear.patch (obsolete) — Splinter Review
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)
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)
(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 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+
Fixed styling issue.
Attachment #8641779 - Attachment is obsolete: true
Addressed control flow issue.
Attachment #8643156 - Attachment is obsolete: true
Took out duplicated code.
Attachment #8641834 - Attachment is obsolete: true
Due to a recent regression this is Failing again now. See Bug 1290774
(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.