Bug 62406 – [chromium] Printing does not work for accelerated <canvas>

• Component: Canvas
• Status: RESOLVED FIXED

[Stephen White]

When accelerated canvas is enabled, canvas elements are omitted from the printed output.

[Stephen White]

See also http://code.google.com/p/chromium/issues/detail?id=55927

[Stephen White]

Created attachment 96648 [details]
Patch

[Stephen White]

Created attachment 96654 [details]
Patch

[James Robinson]

Comment on attachment 96654 [details]
Patch

Is there any way we can hide this in paintsIntoCanvasBuffer() or something?  Sucks to modify the GraphicsContext interface just for this...

[Stephen White]

(In reply to comment #4)
> (From update of attachment 96654 [details])
> Is there any way we can hide this in paintsIntoCanvasBuffer() or something?  Sucks to modify the GraphicsContext interface just for this...

Unfortunately, no.  paintsIntoCanvasBuffer() is on the source GraphicsContext3D/SharedGraphicsContext3D, whereas we need to check the destination GraphicsContext.  Even if SGC3D had access to the GraphicsContext, it would be the wrong one.

[James Robinson]

Le suck - what about asking our Document if we're printing?  I believe the only time we'll try to paint a canvas when printing is when it's in the DOM, and Document already has a printing() accessor.  Remember to null check on the way to the Document.

[Stephen White]

(In reply to comment #6)
> Le suck - what about asking our Document if we're printing?  I believe the only time we'll try to paint a canvas when printing is when it's in the DOM, and Document already has a printing() accessor.  Remember to null check on the way to the Document.

Great idea.  Will give that a shot.

[Stephen White]

Created attachment 96677 [details]
Patch

[James Robinson]

Comment on attachment 96677 [details]
Patch

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

R=me

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:139
> +    if (!bitmap.getPixels()) {

can you leave some sort of comment indicating when getPixels() returns NULL and why we gotta do a readPixels() when it does?

[Mike Reed]

can we not pass srcRect to the readPixels call, to avoid reading back more than we need?

[Stephen White]

(In reply to comment #10)
> can we not pass srcRect to the readPixels call, to avoid reading back more than we need?

See the FIXME.  I was going to do this in a followup just to make sure I didn't screw up the resulting srcRect adjustment, but I'll give it a shot now.

[Mike Reed]

Skia fix in sight. Monday or Tuesday.

[Stephen White]

Created attachment 96770 [details]
Patch

[James Robinson]

Comment on attachment 96770 [details]
Patch

R=me. Sounds like we'll be able to get rid of this soon with a skia change, tho, right?

[Stephen White]

(In reply to comment #14)
> (From update of attachment 96770 [details])
> R=me. Sounds like we'll be able to get rid of this soon with a skia change, tho, right?

Yes, that's my understanding.  Note that further testing has revealed that this is not a complete fix -- it works for Maps directions and some simple pages, but doesn't work for more complex pages.  I'm guessing this is because although canvas is doing a readback, the compositor isn't.  I might look into that before committing this.

[Stephen White]

Created attachment 102166 [details]
Patch

[Stephen White]

Created attachment 102169 [details]
Patch

[Stephen White]

(In reply to comment #17)
> Created an attachment (id=102169) [details]
> Patch

Updated to recent WebKit and to use the pixel readback now built-in to SkBitmap::lockPixels().

[James Robinson]

Comment on attachment 102169 [details]
Patch

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

Looks good

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:119
>      context->platformContext()->makeGrContextCurrent();

do you still need this makeCurrent()?

[Stephen White]

(In reply to comment #19)
> (From update of attachment 102169 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102169&action=review
> 
> Looks good
> 
> > Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:119
> >      context->platformContext()->makeGrContextCurrent();
> 
> do you still need this makeCurrent()?

In theory, no, since we always use the same context under the hood.  But it seems a little fragile to rely on that, so I think this is safer.

[Stephen White]

Committed r91870: <http://trac.webkit.org/changeset/91870>

[James Robinson]

Comment on attachment 102169 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:118
> +    m_context->platformContext()->makeGrContextCurrent();
> +    SkDevice* srcDevice = m_context->platformContext()->canvas()->getDevice();
> +    SkBitmap bitmap = srcDevice->accessBitmap(false);
> +    SkAutoLockPixels bitmapLock(bitmap);

I'm pretty sure that something in here is causing us to do a readback for every canvas->canvas draw now :/

[Mike Reed]

The lockpixels seems unnecessary. We should only see that in a stackframe if we're directly accessing the pixels in the same frame. Are we sure we need this here?

Calling BitmapImageSingleFrameSkia::create(bitmap, forceCopy) will perform a readback if forceCopy is set to true. I will work with Brian to see if we can change bitmap::copyTo() to not force a readback.