Bug 56414

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.

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.

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.

Created attachment 85986 [details] Patch

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.

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

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.

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

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.

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

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

Comment on attachment 85986 [details] Patch Clearing flags on attachment: 85986 Committed r81317: <http://trac.webkit.org/changeset/81317>

All reviewed patches have been landed. Closing bug.

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: dumi@chromium.org) The commit-queue is continuing to process your patch.