Bug 34719 – WebGL rendering results must be made available to Canvas.toDataURL and 2D drawImage

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

Rendering into one Canvas element using WebGL and then drawing that Canvas into another using CanvasRenderingContext2D.drawImage() does not always work. Appropriate hooks need to be placed in the Canvas implementation to force the WebGL context to prepare its contents for drawing via the 2D Canvas APIs.

[Kenneth Russell]

The same problem exists with Canvas.toDataURL().

Both of these APIs must respect the premultipliedAlpha flag in the WebGLContextAttributes.

[Zhenyao Mo]

Created attachment 60461 [details]
patch

Tested in both Safari and Chrome in Mac in Release mode.

[Simon Fraser (smfr)]

Comment on attachment 60461 [details]
patch

paintRenderingResultsToCanvas is leaking 'pixels'.

[Zhenyao Mo]

Created attachment 60482 [details]
revised patch: responding to Simon Fraser's review

Use OwnArrayPtr for pixels to avoid leak.

[Simon Fraser (smfr)]

Comment on attachment 60482 [details]
revised patch: responding to Simon Fraser's review


WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:256
 +      // BEGIN: the same code as in WebGraphicsContext3DDefaultImpl.cpp::readBackFramebuffer
Either share the code, or remove the comment. As-is, the comment is not useful.

Can you share code with GraphicsContext3D::readPixels()?

WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:290
 +      CGImageRef cgImage = CGImageCreate(m_currentWidth,
You should use RetainPtr for the dataProvider, colorSpace and cgImage.

Do you need to save/restore the state in the imageBuffer->context()?

WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:307
 +      // We want to completely overwrite the previous frame's rendering results.

Is that described in a spec anywhere?

r=me

[Kenneth Russell]

Comment on attachment 60482 [details]
revised patch: responding to Simon Fraser's review

Generally looks good, but one high-level comment about code refactoring which I would prefer be done.


WebCore/platform/graphics/GraphicsContext3D.h:711
 +          void paintRenderingResultsToCanvas(WebGLRenderingContext*context);
Formatting: WebGLRenderingContext* context

WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:256
 +      // BEGIN: the same code as in WebGraphicsContext3DDefaultImpl.cpp::readBackFramebuffer
The CG portions of this code should be refactored rather than duplicated. Given that the original code came from GraphicsContext3D.cpp in WebKit/chromium/src, it should be possible to refactor it into WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp. It might be necessary to split it into two routines (before/after readback), or pass a callback function. If it seems reasonable to do the refactoring, please do so.

[Kenneth Russell]

(In reply to comment #5)
> WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:307
>  +      // We want to completely overwrite the previous frame's rendering results.
> 
> Is that described in a spec anywhere?

The comment refers to the fact that the kCGBlendModeCopy blend mode is used, so that transparent pixels aren't blended with any content previously in the image.

[Simon Fraser (smfr)]

(In reply to comment #7)
> (In reply to comment #5)
> > WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:307
> >  +      // We want to completely overwrite the previous frame's rendering results.
> > 
> > Is that described in a spec anywhere?
> 
> The comment refers to the fact that the kCGBlendModeCopy blend mode is used, so that transparent pixels aren't blended with any content previously in the image.

I realize that, but where is that behavior spec'd? I imagine this affect the outcome of mixing a 2D and 2D context on the same canvas, but I'd hope that transparent pixels in the 3D context wouldn't clobber earlier 2D drawing.

[Kenneth Russell]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:307
> > >  +      // We want to completely overwrite the previous frame's rendering results.
> > > 
> > > Is that described in a spec anywhere?
> > 
> > The comment refers to the fact that the kCGBlendModeCopy blend mode is used, so that transparent pixels aren't blended with any content previously in the image.
> 
> I realize that, but where is that behavior spec'd? I imagine this affect the outcome of mixing a 2D and 2D context on the same canvas, but I'd hope that transparent pixels in the 3D context wouldn't clobber earlier 2D drawing.

There have been some changes pending to the HTML Canvas specification for some time now regarding mixing of contexts, and I believe the latest resolution is that using the 2D and 3D contexts simultaneously will be explicitly disallowed in the spec. Regardless, even if you're just using the 3D context, it's still necessary to completely overwrite the rendering results. Consider the case where you draw one 3D scene with a certain set of pixels as transparent, and then perform a drawImage call with that canvas as source. Then draw a different 3D scene with a different set of transparent pixels, and perform another similar drawImage call (into a second, clean, canvas). It would be an error if portions of the first scene were visible where the pixels are transparent in the second scene because the data read back from the graphics card was blended with the previously read back data.

[Simon Fraser (smfr)]

Fair enough.

[Simon Fraser (smfr)]

This may also fix bug 30722.

[Zhenyao Mo]

(In reply to comment #5)
> (From update of attachment 60482 [details])
> 
> WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:256
>  +      // BEGIN: the same code as in WebGraphicsContext3DDefaultImpl.cpp::readBackFramebuffer
> Either share the code, or remove the comment. As-is, the comment is not useful.

Removed.

> 
> Can you share code with GraphicsContext3D::readPixels()?

readPixels() logic is different.  Here we always read from backbuffer, whereas readPixels might read from user created fbo.

> 
> WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:290
>  +      CGImageRef cgImage = CGImageCreate(m_currentWidth,
> You should use RetainPtr for the dataProvider, colorSpace and cgImage.
> 
> Do you need to save/restore the state in the imageBuffer->context()?

Done.

> 
> WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:307
>  +      // We want to completely overwrite the previous frame's rendering results.
> 
> Is that described in a spec anywhere?
> 
> r=me

[Zhenyao Mo]

(In reply to comment #6)
> (From update of attachment 60482 [details])
> Generally looks good, but one high-level comment about code refactoring which I would prefer be done.
> 
> 
> WebCore/platform/graphics/GraphicsContext3D.h:711
>  +          void paintRenderingResultsToCanvas(WebGLRenderingContext*context);
> Formatting: WebGLRenderingContext* context

Done.

> 
> WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:256
>  +      // BEGIN: the same code as in WebGraphicsContext3DDefaultImpl.cpp::readBackFramebuffer
> The CG portions of this code should be refactored rather than duplicated. Given that the original code came from GraphicsContext3D.cpp in WebKit/chromium/src, it should be possible to refactor it into WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp. It might be necessary to split it into two routines (before/after readback), or pass a callback function. If it seems reasonable to do the refactoring, please do so.

Done.

[Zhenyao Mo]

Created attachment 61159 [details]
revised patch: responding to Simon Fraser and kbr's review

[Zhenyao Mo]

Created attachment 61170 [details]
revised patch: fix a bug

in WebGLRenderingContext::markedContextChanged(), always set the dirty flag when context has changed.

Also, add another test canvas-test.html for this change.  (This test is copied from WebGL conformance tests with minor modifications).

[Zhenyao Mo]

Yike, this patch is so out-of-date.  CanvasSurface.h/cpp is gone.  Have to manually merge.

Add [e-mail] to cc list since he did the code refactoring.

[Zhenyao Mo]

Created attachment 61172 [details]
revised patch: resolving svn merge conflicts

[Zhenyao Mo]

Created attachment 61447 [details]
revised patch: deal with Qt part of GraphicsContext3D

Try my luck!

[Zhenyao Mo]

Created attachment 61449 [details]
revised patch: fix a ChangeLog issue

[Early Warning System Bot]

Attachment 61449 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3523150

[Zhenyao Mo]

Created attachment 61451 [details]
revised patch: fix the Qt bug

[Zhenyao Mo]

Created attachment 61457 [details]
revised patch

Seems like Qt/Gtk/Win bots failure may have nothing to do with CG stuff.  Revert the changes I made based on the blame-CG assumption.

[Zhenyao Mo]

Created attachment 61712 [details]
revised patch

Using kbr's script to sync the two tests in this patch with khronos conformance tests.  No other changes,

[Kenneth Russell]

Comment on attachment 61712 [details]
revised patch

Looks good. Thanks for waiting and keeping the tests in sync.

[Nate Chapin]

Comment on attachment 61712 [details]
revised patch


> +#if ENABLE(3D_CANVAS)
> +    if (sourceCanvas->is3D()) {
> +        WebGLRenderingContext* context3d = reinterpret_cast<WebGLRenderingContext*>(sourceCanvas->renderingContext());
> +        context3d->paintRenderingResultsToCanvas();
> +    }
> +#endif
> +

We probably shouldn't be doing 3D things in CanvasRenderingContext2D.



> +  intervalId = window.setInterval(function() {

If there's any way to make this test work without setInterval, please do so, lest it be slow and flaky.

[Zhenyao Mo]

Created attachment 61747 [details]
revised patch

[Kenneth Russell]

Comment on attachment 61747 [details]
revised patch

This looks better to me.

I think the setInterval call is needed in the test (I'm not the test author but there's probably a reason it was used), but the modifications that have been made should prevent any flakiness.

[Nate Chapin]

Comment on attachment 61747 [details]
revised patch

Alright, sounds good.

[Zhenyao Mo]

Committed r63502: <http://trac.webkit.org/changeset/63502>

[Zoltan Herczeg]

This patch does not builds on mac-leopard:

/Users/hzoli/WebKitHZ/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm: In member function 'void WebCore::GraphicsContext3D::paintRenderingResultsToCanvas(WebCore::WebGLRenderingContext*)':
/Users/hzoli/WebKitHZ/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:257: warning: 'mustRestoreFBO' may be used uninitialized in this function

I think it should be false by default, isn't it?

[Kenneth Russell]

(In reply to comment #30)
> This patch does not builds on mac-leopard:
> 
> /Users/hzoli/WebKitHZ/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm: In member function 'void WebCore::GraphicsContext3D::paintRenderingResultsToCanvas(WebCore::WebGLRenderingContext*)':
> /Users/hzoli/WebKitHZ/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:257: warning: 'mustRestoreFBO' may be used uninitialized in this function
> 
> I think it should be false by default, isn't it?

Yes, you're right. I'll upload a patch fixing this. Not sure why it wasn't caught by the bots.

[Kenneth Russell]

Created attachment 61996 [details]
Patch fixing compiler warning

From the ChangeLog:

Fixed compiler warning introduced by original patch. No new tests; covered by existing tests.

[Kenneth Russell]

Committed r63705: <http://trac.webkit.org/changeset/63705>

[Simon Fraser (smfr)]

Does this fix bug 30722?

[Kenneth Russell]

(In reply to comment #34)
> Does this fix bug 30722?

I'm not sure (and Mo is on vacation this week). Was there a test case for 30722?

[Simon Fraser (smfr)]

This caused bug 42852.

[Simon Fraser (smfr)]

(In reply to comment #35)
> (In reply to comment #34)
> > Does this fix bug 30722?
> 
> I'm not sure (and Mo is on vacation this week). Was there a test case for 30722?

See bug 42862.