Bug 57104 – getCSSCanvasContext does not work with SKIA_GPU

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Canvas

When building Chromium using SKIA_GPU, getCSSCanvasContext does not work as expected.

Created attachment 86948 [details]
patch that fixes the problem

[WebKit Review Bot]

Attachment 86948 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:162:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:164:  The parameter name "bitmap" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 86956 [details]
updated patch for style

Created attachment 86973 [details]
updated patch

This patch adds guards so the new code will only be used if ENABLE(SKIA_GPU) (just in case; I did not see any adverse effect when turned off).  Also, I follow the new path for other cases, fixing more layout tests for SKIA_GPU (yay!).

[James Robinson]

Comment on attachment 86973 [details]
updated patch

Why doesn't drawing from a texture-backed SkBitmap into a non-texture backed skia context work?  I was under the impression that skia would handle this case internally.

[Mike Reed]

Skia automatically will draw a texture-backed-bitmap to a gl context, and a ram-backed-bitmap to a gl context. It does not automatically do the reverse: texture-backed-bitmap to a ram-backed canvas.

If/when the layers and root of webkit are also gl-context backed, then we will be able to remove this code (and other) since we'll be in the optimal texture->texture drawing case.

[James Robinson]

Comment on attachment 86973 [details]
updated patch

Feels like the wrong fix.  In this case we're trying to draw from a SkBitmap into another context - skia already should know what the backing store for the bitmap is and should be capable of doing a readback if it needs the pixels available for a software draw.

[Mike Reed]

We can look at that for the future, but the raster device has no knowledge of the texture that was created by another (gpu) device.

[James Robinson]

(In reply to comment #6)
> Skia automatically will draw a texture-backed-bitmap to a gl context, and a ram-backed-bitmap to a gl context. It does not automatically do the reverse: texture-backed-bitmap to a ram-backed canvas.

Why not?  Seems like if I have a texture backed bitmap and I tell skia that I want it rendered into a ram-backed canvas that a readback is required.  What's other behaviors could be valid?

> 
> If/when the layers and root of webkit are also gl-context backed, then we will be able to remove this code (and other) since we'll be in the optimal texture->texture drawing case.

That's another argument for why this should be handled by Skia and not in WebKit code.

[James Robinson]

Comment on attachment 86973 [details]
updated patch

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

We can discuss the best fix in person on Monday.  In the meantime I've annotated some other issues that need to be addressed if we want to continue with this patch.

> Source/WebCore/ChangeLog:5
> +        Make getCSSCanvasContext work on chromium using SKIA_GPU

You need a link to the bug as well - see the (brief) section in http://www.webkit.org/coding/contributing.html about ChangeLogs.

the webkit-patch upload script will generate a properly formatted stub for you.

> Source/WebCore/ChangeLog:7
> +        No new tests. (OOPS!)

Having the (OOPS!) here will will fail a presubmit check.  Delete this line and replace it with some text describing what the change does and why it doesn't need new tests (in this case because the functionality is already handled by existing tests - it'd be helpful to lists which tests are included).