Bug 54224 – toDataURL() fails if skia device is backed by GPU

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Mike Reed]

The current impl assumes it can read the address returned by the device's bitmap, and had it to the image encoders. The fix is to check if we need to call device->readPixels() first, which will always succeed, even if the device is backed by some other means than a CPU bitmap (e.g. opengl fbo)

[Mike Reed]

Created attachment 82008 [details]
fix for 54224 - call readPixels() if need be, if the address from getPixels() is null

[James Robinson]

Comment on attachment 82008 [details]
fix for 54224 - call readPixels() if need be, if the address from getPixels() is null

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

R=me

> WebCore/ChangeLog:9
> +        No new tests. Existing canvas tests exercise this. 

Do we know which test(s) cover this?

[Jakob Petsovits]

Don't you need to unlock the bitmap to be able to access the pixel data? iirc, there were cases where a perfectly valid memory-backed bitmap returns 0 for getPixels() because the bitmap wasn't locked. Maybe that doesn't apply in this specific case, depends on what bitmap the canvas is created on.

[Mike Reed]

Created attachment 82014 [details]
add explicit test that exercises this code path, explicitly lockPixels() before calling getPixels()

[James Robinson]

Comment on attachment 82014 [details]
add explicit test that exercises this code path, explicitly lockPixels() before calling getPixels()

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

Great!  Thanks for adding the test reference, it'll be useful to have this information if that test every breaks in the future.  Would you like me to c-q+ this as well?

> WebCore/platform/graphics/skia/ImageBufferSkia.cpp:354
> +    // if we can't see the pixels directly, call readPixels() to get a copy.
> +    // this could happen if the device is backed by a GPU.
> +    bitmap.lockPixels(); // balanced by our destructor, or explicitly if getPixels() fails

nit: In WebKit, comments start with an uppercase letter and end with a period.

[Mike Reed]

can this be queued for commit? thanks.

[WebKit Commit Bot]

Comment on attachment 82014 [details]
add explicit test that exercises this code path, explicitly lockPixels() before calling getPixels()

Clearing flags on attachment: 82014

Committed r78356: <http://trac.webkit.org/changeset/78356>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.