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

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Luke]

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.

Comment 1

[Jukka Jylänki]

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.

Comment 2

[Kyle Fung]

>    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

Comment 3

[Kyle Fung]

>    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?

Comment 4

[Kelsey Gilbert [:jgilbert]]

It looks like you just need expand what I did with mUserReadBufferMode to support draw_buffer.

Comment 5

[Kyle Fung]

Attachment: draw-buffer-preserve.patch

Not entirely sure if this is the way to go by setting the draw buffer through the ReadBuffer object.

Comment 6

[Kyle Fung]

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

Comment 7

[Kyle Fung]

(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/"

Comment 8

[Kyle Fung]

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.

Comment 9

[Kyle Fung]

Attachment: max-color-attachments.patch

Issue in comment 8 addressed here. Replaced some uses of kMaxColorAttachments with mContext->mGLMaxColorAttachments.

Comment 10

[Kyle Fung]

Attachment: preserve-draw-buffer.patch

Second issue from comment 3 addressed. Caches the draw buffer mode of GLScreenBuffer and resets it after surface attachment operations.

Comment 11

[Kelsey Gilbert [: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.

Comment 12

[Kelsey Gilbert [:jgilbert]]

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);

Comment 13

[Kyle Fung]

Attachment: invalid-op-force-clear.patch

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

Comment 14

[Kyle Fung]

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)?

Comment 15

[Kelsey Gilbert [: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.

Comment 16

[Kelsey Gilbert [: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)

Comment 17

[Kyle Fung]

Attachment: max-color-attachments-v2.patch

Fixed styling issue.

Comment 18

[Kyle Fung]

Attachment: invalid-op-force-clear-v2.patch

Addressed control flow issue.

Comment 19

[Kyle Fung]

Attachment: preserve-draw-buffer-v2.patch

Took out duplicated code.

Comment 20

[Kyle Fung]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=40447637c646

Comment 21

[Pulsebot]

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

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2e1b414266f1
https://hg.mozilla.org/mozilla-central/rev/af87603b35cd
https://hg.mozilla.org/mozilla-central/rev/0993bd17c823

Comment 23

[Luke]

Due to a recent regression this is Failing again now. See Bug 1290774

Comment 24

[Kelsey Gilbert [:jgilbert]]

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