Bug 83295 - [chromium] Drawing an accelerated canvas onto itself is slow.
Summary: [chromium] Drawing an accelerated canvas onto itself is slow.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Stephen White
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-05 11:09 PDT by Stephen White
Modified: 2012-04-05 13:44 PDT (History)
2 users (show)

See Also:


Attachments
Test case (1.99 KB, text/html)
2012-04-05 11:11 PDT, Stephen White
no flags Details
Patch (1.45 KB, patch)
2012-04-05 11:15 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch (1.50 KB, patch)
2012-04-05 13:00 PDT, Stephen White
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen White 2012-04-05 11:09:00 PDT
[chromium] Drawing an accelerated canvas onto itself is slow.
Comment 1 Stephen White 2012-04-05 11:11:57 PDT
Created attachment 135860 [details]
Test case

from http://code.google.com/p/chromium/issues/detail?id=106646
Comment 2 Stephen White 2012-04-05 11:15:43 PDT
Created attachment 135861 [details]
Patch
Comment 3 James Robinson 2012-04-05 11:21:57 PDT
Comment on attachment 135861 [details]
Patch

Interesting.  What if this is a gpu->cpu or cpu->gpu copy?  Should we be checking the return value?
Comment 4 Stephen White 2012-04-05 12:36:35 PDT
(In reply to comment #3)
> (From update of attachment 135861 [details])
> Interesting.  What if this is a gpu->cpu or cpu->gpu copy?  Should we be checking the return value?

Yeah, thanks for reminding me.  Mike made me remove the copyTo() fallback from the internal implementation.
Comment 5 Stephen White 2012-04-05 12:58:49 PDT
Note that it should always be GPU->GPU or CPU->CPU, since it's the same canvas.
Comment 6 Stephen White 2012-04-05 13:00:13 PDT
Created attachment 135881 [details]
Patch
Comment 7 James Robinson 2012-04-05 13:32:46 PDT
Comment on attachment 135881 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:483
> +        if (!bitmap.deepCopyTo(&temp, bitmap.config()))
> +            bitmap.copyTo(&temp, bitmap.config());

gotcha. so given that the configs match, when would deepCopyTo fail?
Comment 8 Stephen White 2012-04-05 13:40:57 PDT
(In reply to comment #7)
> (From update of attachment 135881 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135881&action=review
> 
> > Source/WebCore/platform/graphics/skia/ImageSkia.cpp:483
> > +        if (!bitmap.deepCopyTo(&temp, bitmap.config()))
> > +            bitmap.copyTo(&temp, bitmap.config());
> 
> gotcha. so given that the configs match, when would deepCopyTo fail?

Actually, it turns out that there is an internal copyTo fallback in the software case; it's only the texture-backed case that can fail.

If the target has a texture which isn't a renderable format (eg., compressed texture), deepCopy() would fail, since the copy works by rendering via FBO.  But given that this texture is a backing store, it should always succeed.  So, never in this case.  :)
Comment 9 Stephen White 2012-04-05 13:44:24 PDT
Committed r113373: <http://trac.webkit.org/changeset/113373>