Bug 56414 – texImage2D gets old contents of canvas

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

[John Bauman]

Currently we don't seem to be marking the copied image of a canvas as dirty when we're doing accelerated rendering, causing a texImage2D on the canvas to possibly get old data.

[John Bauman]

Looks like we're checking paintsIntoCanvasBuffer to see if we should do paintRenderingResultsToCanvas to update the image. This is true on chromium with accelerated compositing disabled (like in this test), so the paint doesn't happen.

I'm not sure what exactly paintsIntoCanvasBuffer is supposed to mean, but removing the check for it in HTMLCanvasElement::copiedImage seems to work.

[Kenneth Russell]

Do you happen to know whether it's broken when passing a 2D-rendered Canvas to WebGL via texImage2D, when passing a WebGL-rendered Canvas to WebGL via texImage2D, or both?

For the WebGL case, https://bugs.webkit.org/show_bug.cgi?id=56431 will affect the behavior here, though we can fix this bug independently.

[John Bauman]

I haven't tested reading from 2D-rendered Canvas, but I think that may be broken when the canvas is GPU-accelerated. An unaccelerated canvas should work fine.

[John Bauman]

Created attachment 85986 [details]
Patch

[James Robinson]

Comment on attachment 85986 [details]
Patch

This seems correct to me.  Should we have a more direct test for this? fast/canvas/webgl/premultiplyalpha-test.html seems to test a bunch of other stuff as well.

[James Robinson]

It looks like this will never change behavior for the Mac port since paintsIntoCanvasBuffer() always returns false on mac for WebGL content.

[John Bauman]

Right, the test already passes on Macs. The patch might change something on Qt, but hopefully it will change it to do the right thing.

[Stephen White]

Is it possible this fix would break (or slow down) accelerated canvas2D -> canvas2D draws?  (the amazon shelf demo used to be a good test of this, but unfortunately it's not available anymore).

This seems to end up here:

bool GraphicsContext3DInternal::paintsIntoCanvasBuffer() const
{
    // If the gpu compositor is on then skip the readback and software rendering path.
    return !m_webViewImpl->isAcceleratedCompositingActive();
}

[John Bauman]

HTMLCanvasElement::paint still checks paintsIntoCanvasBuffer, so that won't be affected. canvas2D->canvas2D drawImages go down a different path (not using copiedImage, but instead manually doing makeRenderingResultsAvailable if the canvas isn't accelerated), so they shouldn't change.

[James Robinson]

canvas2d->canvas2d on chromium/skia uses this:
http://trac.webkit.org/browser/trunk/Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp#L1267

which does the readback if needed and then does this:
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp#L118

which should still do the right thing

[Stephen White]

(In reply to comment #10)
> canvas2d->canvas2d on chromium/skia uses this:
> http://trac.webkit.org/browser/trunk/Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp#L1267
> 
> which does the readback if needed and then does this:
> http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp#L118
> 
> which should still do the right thing

Right, thanks.  This change makes more sense to me now.

[WebKit Commit Bot]

Comment on attachment 85986 [details]
Patch

Clearing flags on attachment: 85986

Committed r81317: <http://trac.webkit.org/changeset/81317>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 85986 [details]:

fast/workers/storage/use-same-database-in-page-and-workers.html bug 50995 (author: [e-mail])
The commit-queue is continuing to process your patch.