Bug 54224 - toDataURL() fails if skia device is backed by GPU
Summary: toDataURL() fails if skia device is backed by GPU
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-10 10:58 PST by Mike Reed
Modified: 2011-02-11 12:10 PST (History)
7 users (show)

See Also:


Attachments
fix for 54224 - call readPixels() if need be, if the address from getPixels() is null (2.37 KB, patch)
2011-02-10 11:02 PST, Mike Reed
jamesr: review+
Details | Formatted Diff | Diff
add explicit test that exercises this code path, explicitly lockPixels() before calling getPixels() (2.57 KB, patch)
2011-02-10 11:35 PST, Mike Reed
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Reed 2011-02-10 10:58:21 PST
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)
Comment 1 Mike Reed 2011-02-10 11:02:07 PST
Created attachment 82008 [details]
fix for 54224 - call readPixels() if need be, if the address from getPixels() is null
Comment 2 James Robinson 2011-02-10 11:14:21 PST
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?
Comment 3 Jakob Petsovits 2011-02-10 11:18:46 PST
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.
Comment 4 Mike Reed 2011-02-10 11:35:15 PST
Created attachment 82014 [details]
add explicit test that exercises this code path, explicitly lockPixels() before calling getPixels()
Comment 5 James Robinson 2011-02-10 15:46:18 PST
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.
Comment 6 Mike Reed 2011-02-11 06:50:31 PST
can this be queued for commit? thanks.
Comment 7 WebKit Commit Bot 2011-02-11 12:10:19 PST
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>
Comment 8 WebKit Commit Bot 2011-02-11 12:10:24 PST
All reviewed patches have been landed.  Closing bug.