Bug 56431 – Add support for preserveDrawingBuffer context creation attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kenneth Russell]

The preserveDrawingBuffer context creation attribute is currently unimplemented, causing WebGL conformance suite failures. The preserveDrawingBuffer=false behavior needs to be properly supported, which requires that operations against a WebGL-rendered canvas like toDataURL, readPixels, drawImage and texImage2D return transparent black once the canvas's contents have been presented to the compositor. The current behavior, which matches preserveDrawingBuffer=true, also needs to be officially supported.

[John Bauman]

Created attachment 86130 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 86130 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86130&action=review

> Source/WebCore/html/HTMLCanvasElement.cpp:293
> +        WebGLRenderingContext* ctx = static_cast<WebGLRenderingContext*>(m_context.get());
> +
> +        ctx->markLayerComposited();

Odd to not just do this cast inline.  Then you don't need the extra lines or { }

> Source/WebCore/html/HTMLCanvasElement.cpp:315
> +        // The buffer contains the last presented data, so save a
> +        // copy of it.

We don't wrap to 80c in webkit.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:505
> +    if (m_context->layerComposited() && !m_layerCleared
> +        && !getContextAttributes()->preserveDrawingBuffer()) {

Seems we could early-return here instead, no?

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:512
> +            m_context->clearColor(m_colorMask[0] ? m_currentClearColor[0] : 0.0f,

Isn't there a clearer way to write this?  Using a Color object perhaps?

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:517
> +            m_context->clearColor(0.0f, 0.0f, 0.0f, 0.0f);

Do these really need to be 0.0f instead of 0?

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:531
> +        m_context->clearColor(m_currentClearColor[0], m_currentClearColor[1],
> +                              m_currentClearColor[2], m_currentClearColor[3]);

Again, seems long-winded.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:555
> +    clearIfComposited(0);

What's the 0 mean here?  should it be a default parameter?

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:979
> +    m_currentClearColor[0] = r;
> +    m_currentClearColor[1] = g;
> +    m_currentClearColor[2] = b;
> +    m_currentClearColor[3] = a;

I see, these are GC3DFloat values.  Seems we need a GC3DRGBA class, no?

> Source/WebCore/html/canvas/WebGLRenderingContext.h:450
> +    bool m_layerCleared;
> +    GC3Dfloat m_currentClearColor[4];
> +    bool m_scissorEnabled;

You might want to keep the bools together for possible future bitfield usage.  I'm not sure it maters, and I am not sure we'll ever have very many of these object either.  Just noting.

[John Bauman]

(In reply to comment #2)
> (From update of attachment 86130 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86130&action=review
<snip>
> 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:512
> > +            m_context->clearColor(m_colorMask[0] ? m_currentClearColor[0] : 0.0f,
> 
> Isn't there a clearer way to write this?  Using a Color object perhaps?
> 
<snip>
> 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:979
> > +    m_currentClearColor[0] = r;
> > +    m_currentClearColor[1] = g;
> > +    m_currentClearColor[2] = b;
> > +    m_currentClearColor[3] = a;
> 
> I see, these are GC3DFloat values.  Seems we need a GC3DRGBA class, no?
> 
That would certainly work, although currently this API and those types are based on GL, so I don't know if we want to add more and make them dissimilar.

[Zhenyao Mo]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 86130 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=86130&action=review
> <snip>
> > 
> > > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:512
> > > +            m_context->clearColor(m_colorMask[0] ? m_currentClearColor[0] : 0.0f,
> > 
> > Isn't there a clearer way to write this?  Using a Color object perhaps?
> > 
> <snip>
> > 
> > > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:979
> > > +    m_currentClearColor[0] = r;
> > > +    m_currentClearColor[1] = g;
> > > +    m_currentClearColor[2] = b;
> > > +    m_currentClearColor[3] = a;
> > 
> > I see, these are GC3DFloat values.  Seems we need a GC3DRGBA class, no?
> > 
> That would certainly work, although currently this API and those types are based on GL, so I don't know if we want to add more and make them dissimilar.

GC3D types should be strictly matching corresponding GL types.

[Zhenyao Mo]

Comment on attachment 86130 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86130&action=review

A few more nits besides Eric's review.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:417
> +    m_currentClearColor[0] = m_currentClearColor[1] = m_currentClearColor[2] = m_currentClearColor[3] = 0.0f;

Should be 0.  0.0 or 0.0f is against webkit coding style.  Here and a few other places.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:502
> +

Maybe clear depth and/or stentil only these buffers are there?

>> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:979
>> +    m_currentClearColor[3] = a;
> 
> I see, these are GC3DFloat values.  Seems we need a GC3DRGBA class, no?

GC3D types are strictly matching GL types.

> Source/WebCore/platform/graphics/chromium/WebGLLayerChromium.h:49
> +    virtual void textureUpdated();

setTextureUpdated()?

> Source/WebCore/platform/graphics/chromium/WebGLLayerChromium.h:59
> +    bool m_textureUpdated;

Initialize this member.

> Source/WebKit/chromium/src/GraphicsContext3DInternal.h:281
> +    bool m_layerComposited;

You need to initialize this member.

[John Bauman]

Created attachment 86353 [details]
Patch

[Kenneth Russell]

Comment on attachment 86353 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86353&action=review

Overall this looks excellent; thanks for taking care of it. There are a few comments that need to be fixed, and there are a couple of questions about completeness of the logic.

> Source/WebCore/ChangeLog:16
> +        No new tests. It seems difficult to be difficult/impossible to trigger

Grammar: "seems difficult" -> "seems"

> Source/WebCore/ChangeLog:18
> +        an early compositing operation in DumpRenderTree, making this hard to
> +        test automatically. There is a WebGL conformance test for it.

For the record, we might be able to test this automatically by making it a pixel test.

Please also mention the manual testing that was done with this patch. In particular, we should test the following cases:
  - Chromium with normal flags
  - Chromium with --disable-accelerated-compositing
  - Safari on Mac

with the conformance test as well as a few demos.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:531
> +    if (contextAttributes->stencil())
> +        clearMask |= GraphicsContext3D::STENCIL_BUFFER_BIT;

Per Vlad's email to the WebGL working group, we may need to be careful with the clearing of the stencil buffer. We prefer to use packed a depth stencil renderbuffer in implementations, so we need to make sure that if there's a stencil buffer attached, even if the user didn't request it, that we properly clear it here. If this doesn't already work (perhaps getContextAttributes() is already doing the right thing) then we should at least add a FIXME here.

> Source/WebCore/html/canvas/WebGLRenderingContext.h:481
> +    // Clear if the backbuffer if it was composited since the last operation.

Typo: "Clear if" -> "Clear".

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:1482
> +    // FIXME: Any accelerated compositor needs to be notified of the changes.

What are the consequences of this FIXME for the Safari / Mac port? We definitely do not want to regress that port; it is working well now and needs to continue to do so.

[John Bauman]

(In reply to comment #7)
> (From update of attachment 86353 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86353&action=review
> 
> Overall this looks excellent; thanks for taking care of it. There are a few comments that need to be fixed, and there are a couple of questions about completeness of the logic.
> 
> > Source/WebCore/ChangeLog:16
> > +        No new tests. It seems difficult to be difficult/impossible to trigger
> 
> Grammar: "seems difficult" -> "seems"
Oh, oops
> 
> > Source/WebCore/ChangeLog:18
> > +        an early compositing operation in DumpRenderTree, making this hard to
> > +        test automatically. There is a WebGL conformance test for it.
> 
> For the record, we might be able to test this automatically by making it a pixel test.

Unfortunately, that only does compositing after the test is finished. We need compositing to happen before that for the test to work.
> 
> Please also mention the manual testing that was done with this patch. In particular, we should test the following cases:
>   - Chromium with normal flags
>   - Chromium with --disable-accelerated-compositing
>   - Safari on Mac
> 
> with the conformance test as well as a few demos.

Ok, I'll mention that those have been tested.
> 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:531
> > +    if (contextAttributes->stencil())
> > +        clearMask |= GraphicsContext3D::STENCIL_BUFFER_BIT;
> 
> Per Vlad's email to the WebGL working group, we may need to be careful with the clearing of the stencil buffer. We prefer to use packed a depth stencil renderbuffer in implementations, so we need to make sure that if there's a stencil buffer attached, even if the user didn't request it, that we properly clear it here. If this doesn't already work (perhaps getContextAttributes() is already doing the right thing) then we should at least add a FIXME here.

getContextAttributes returns what's actually been allocated for the context, so it should work properly.
> 
> > Source/WebCore/html/canvas/WebGLRenderingContext.h:481
> > +    // Clear if the backbuffer if it was composited since the last operation.
> 
> Typo: "Clear if" -> "Clear".
> 
> > Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:1482
> > +    // FIXME: Any accelerated compositor needs to be notified of the changes.
> 
> What are the consequences of this FIXME for the Safari / Mac port? We definitely do not want to regress that port; it is working well now and needs to continue to do so.

When accelerated compositing is disabled, preserveDrawingBuffer will work correctly. When accelerated compositing is enabled, this code won't receive any notification that compositing has happened, so it should act exactly the same way it does now.

[John Bauman]

Created attachment 86409 [details]
Patch

[Kenneth Russell]

Comment on attachment 86409 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86409&action=review

Looks better. Thanks for getting this working on the Mac. There is one remaining issue which I think should be addressed before committing. Not touching the r? or cq? yet. Please let me know what you think.

> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:99
> +    , m_layerComposited(false)

You should really initialize all of the new members: m_internalColorFormat, m_activeTexture and m_boundTexture0.

[John Bauman]

Created attachment 86541 [details]
Patch

[John Bauman]

(In reply to comment #10)
> (From update of attachment 86409 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=86409&action=review
> 
> Looks better. Thanks for getting this working on the Mac. There is one remaining issue which I think should be addressed before committing. Not touching the r? or cq? yet. Please let me know what you think.
> 
> > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:99
> > +    , m_layerComposited(false)
> 
> You should really initialize all of the new members: m_internalColorFormat, m_activeTexture and m_boundTexture0.

Sure, that definitely can't hurt. I've uploaded a newer fix.

[Kenneth Russell]

Comment on attachment 86541 [details]
Patch

Thanks, looks great. r=me

[WebKit Commit Bot]

Comment on attachment 86541 [details]
Patch

Clearing flags on attachment: 86541

Committed r81740: <http://trac.webkit.org/changeset/81740>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

Why is WebGLLayerChromium::setTextureUpdated() declared virtual?  I see no sign of virtual dispatch being used currently.  Do you intend for this function to be used for other layer types?

[John Bauman]

Yeah, I was thinking (vaguely) about having other layer types use that, but I guess that since this really only affects WebGL having it be virtual is unnecessary.