Bug 64535 - Add fast path for ImageBuffer::draw
: Add fast path for ImageBuffer::draw
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Canvas
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Matthew Delaney
:
Depends on: 64925
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-14 09:53 PDT by Matthew Delaney
Modified: 2011-07-21 13:49 PDT (History)
13 users (show)

See Also:


Attachments
Patch (2.75 KB, patch)
2011-07-14 10:22 PDT, Matthew Delaney
no flags Details | Formatted Diff | Diff
Patch (24.20 KB, patch)
2011-07-18 13:14 PDT, Matthew Delaney
no flags Details | Formatted Diff | Diff
Patch (34.62 KB, patch)
2011-07-18 23:32 PDT, Matthew Delaney
no flags Details | Formatted Diff | Diff
Patch (36.69 KB, patch)
2011-07-19 09:31 PDT, Matthew Delaney
no flags Details | Formatted Diff | Diff
Patch (35.04 KB, patch)
2011-07-21 13:34 PDT, Matthew Delaney
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matthew Delaney 2011-07-14 09:53:47 PDT
ImageBuffer::draw is a heavily used path (currently only by canvas::paint and drawing from/to a canvas) and can be optimized in the accelerated route by stripping out the call through the Image class to avoid all the context saves/restores and other "general" image drawing logic.
Comment 1 Matthew Delaney 2011-07-14 10:22:08 PDT
Created attachment 100824 [details]
Patch
Comment 2 Simon Fraser (smfr) 2011-07-14 10:28:20 PDT
Comment on attachment 100824 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:-219
> -        RefPtr<Image> copy = copyImage();
> -        ColorSpace colorSpace = (destContext == context()) ? ColorSpaceDeviceRGB : styleColorSpace;
> -        destContext->drawImage(copy.get(), colorSpace, destRect, srcRect, op, useLowQualityScale);

You're ignoring useLowQualityScale in the new code path.

The new code also doesn't do a bunch of stuff that BitmapImage::draw() does:
startAnimation()
mayFillWithSolidColor()
Bail early if part of the image will be drawn.
The shouldUseSubimage stuff.
Adjusting the color space.
Calling  imageObserver()->didDraw().
Comment 3 Ian Henderson 2011-07-14 12:17:45 PDT
Could drawImage() detect we're in this case and skip the save / restore calls?  What makes it valid to skip all the general image drawing logic?
Comment 4 Matthew Delaney 2011-07-18 13:12:33 PDT
I put a bunch of thought into this and realized that what we need here is to make the image drawing paths clearer. ImageBuffer is this weird beast that currently wraps itself in a BitmapImage when it goes to draw - this has a lot of unnecessary overhead. Since ImageBuffer is used heavily by high-performance-demanding image blitting paths such as Canvas and SVG, there should be a very fast path for blitting its image.

Basically, ImageBuffer::draw needs all of core code of BitmapImage's draw method without all the overhead of creating an BitmapImage and its setup when it draws (like startAnimation, getCurrentFrame, etc.).

BitmapImage currently represents what's supposed to be our "general image object" that wraps a CGImage or any other platform specific image. Unlike other things that are drawn through graphicscontext (text, shapes, etc.) it ends up holding the machinery for drawing itself into the graphicsContext, as opposed to the graphicsContext knowing how to draw it.

This patch refactors the core (platform specific) image drawing code from ImageCG.cpp and puts it in GraphicsContextCG.cpp so that GraphicsContext will know how to draw platform specific images into itself instead of images knowing how to draw into GraphicsContexts. To do this, we add a GraphicsContext::drawImage that takes the platform specific NativeImagePtr and it does the core work. Thus, BitmapImage still can have all of its setup and tear down machinery but then passes off its CGImageRef over to GraphicsContext to do the actual drawing. Many agree that this paradigm makes more sense and it also opens up more room for faster drawing and code reuse.

With the above change to GraphicsContext, ImageBuffer is now able to create its platform specific Image ref, not have to bottle it in a BitmapImage, and pass it directly to GraphicsContext to draw without any of the setup/teardown that BitmapImage::draw had. In my testing, this change alone easily removes 10-15% total cpu time for some sites that have canvas animations where an offscreen canvas is used as a sprite sheet to draw thousands of assets per second (thus hitting the ImageBuffer:draw path those thousands of times per second).

Other Notes:
 * This patch will currently break on other ports. I wanted to get feedback before stubbing out other ports.

 * "imageWithColorSpace" is a general image conversion routine that should live elsewhere. I currently just copied it over (I know, ::cringe::) and am looking for suggestions for where it should live. There are other static utility functions like this (mainly for images) that really should either live in a ImageUtilsCG or if they're cross platform-able, perhaps be defined off of GraphicsContext.

 * I removed useLowQuality from ImageBuffer::draw because it has not use for it now that it doesn't need to draw through BitmapImage::draw. It was redundant logic dealing with the useLowQuality from GraphicsContext::drawImageBuffer anyhow.
Comment 5 Matthew Delaney 2011-07-18 13:14:27 PDT
Created attachment 101194 [details]
Patch
Comment 6 WebKit Review Bot 2011-07-18 13:24:01 PDT
Comment on attachment 101194 [details]
Patch

Attachment 101194 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9115386
Comment 7 Gyuyoung Kim 2011-07-18 13:26:20 PDT
Comment on attachment 101194 [details]
Patch

Attachment 101194 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9120089
Comment 8 Early Warning System Bot 2011-07-18 13:29:44 PDT
Comment on attachment 101194 [details]
Patch

Attachment 101194 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9115391
Comment 9 Gustavo Noronha (kov) 2011-07-18 14:41:44 PDT
Comment on attachment 101194 [details]
Patch

Attachment 101194 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9110538
Comment 10 Matthew Delaney 2011-07-18 23:32:04 PDT
Created attachment 101275 [details]
Patch
Comment 11 WebKit Review Bot 2011-07-18 23:47:34 PDT
Comment on attachment 101275 [details]
Patch

Attachment 101275 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9150053
Comment 12 Early Warning System Bot 2011-07-18 23:49:38 PDT
Comment on attachment 101275 [details]
Patch

Attachment 101275 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9154036
Comment 13 WebKit Review Bot 2011-07-19 00:37:24 PDT
Comment on attachment 101275 [details]
Patch

Attachment 101275 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9159038
Comment 14 Matthew Delaney 2011-07-19 09:31:52 PDT
Created attachment 101333 [details]
Patch
Comment 15 Simon Fraser (smfr) 2011-07-19 13:24:21 PDT
Comment on attachment 101333 [details]
Patch

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

r=me but please consider the feedback.

> Source/WebCore/ChangeLog:12
> +        This isn't necessary, but it has been bothering me forever :-)

No need to justify this.

> Source/WebCore/ChangeLog:13
> +        (WebCore::GraphicsContext::drawImageBuffer): Removed the need to pass down useLowQualityScale since it's not needed below.

It's odd that it's not needed; is that just because no-one happens to use it in this code path? Might they in future?

> Source/WebCore/ChangeLog:18
> +        0) Changed copyImage() to deepCopy() to make it clear this copy is indeed deep (but could be COW)
> +        1) Added shallowCopy() that gives a copy that doesn't guarantee to be a deep copy.

I'm not sure these names are really accurate, but I can't think of better ones. The main difference seems to be that if you call shallowCopy(), subsequent drawing into the image buffer will change what you have.

Another approach would be:
enum BackingStoreCopy { CopyBackingStore, DontCopyBackingStore }
copyImage(BackingStoreCopy)

> Source/WebCore/ChangeLog:27
> +        2) Added in plaformDeepCopy(), and platformShallowCopy() to other methods that should use them.

plaformDeepCopy is misspelled.

> Source/WebCore/platform/graphics/GraphicsContext.h:268
> +        void drawImage(NativeImagePtr, const FloatSize& selfSize, ColorSpace styleColorSpace, const FloatRect& destRect, const FloatRect& srcRect, CompositeOperator = CompositeSourceOver);

We'd have to be careful that this overload isn't called by mistake if an Image has an operator NativeImagePtr(). I'd suggest renaming it to drawNativeImage()

> Source/WebCore/platform/graphics/ImageBuffer.h:114
> +        void draw(GraphicsContext*, ColorSpace styleColorSpace, const FloatRect& destRect, const FloatRect& srcRect = FloatRect(0, 0, -1, -1), CompositeOperator = CompositeSourceOver, bool useLowQualityScale = false);

You can remove the styleColorSpace param name.

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:240
> +            // We're drawing into our own buffer, need to deep copy.

Why this comment here and not in draw() above? I'd prefer it in both.
Comment 16 Matthew Delaney 2011-07-19 21:29:21 PDT
Committed r91332: <http://trac.webkit.org/changeset/91332>
Comment 17 Ryosuke Niwa 2011-07-20 08:01:37 PDT
css3/images/optimize-contrast-canvas.html started crashing on Chromium Mac Debug after this patch (both 10.5 and 10.6):

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=css3%2Fimages%2Foptimize-contrast-canvas.html&showExpectations=true
Comment 18 Ryosuke Niwa 2011-07-20 08:19:37 PDT
Filed https://bugs.webkit.org/show_bug.cgi?id=64874.
Comment 19 Adam Roben (:aroben) 2011-07-20 09:49:44 PDT
Comment on attachment 101333 [details]
Patch

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

>> Source/WebCore/ChangeLog:18
>> +        1) Added shallowCopy() that gives a copy that doesn't guarantee to be a deep copy.
> 
> I'm not sure these names are really accurate, but I can't think of better ones. The main difference seems to be that if you call shallowCopy(), subsequent drawing into the image buffer will change what you have.
> 
> Another approach would be:
> enum BackingStoreCopy { CopyBackingStore, DontCopyBackingStore }
> copyImage(BackingStoreCopy)

A naming scheme we use elsewhere is a little clearer, I think:

enum CopyBackingStoreOrNot { DoNotCopyBackingStore, CopyBackingStore };

(Note that this also has the nice property that DoNotCopyBackingStore is falsey, and CopyBackingStore is truthy.)
Comment 20 Mike Lawther 2011-07-21 00:01:24 PDT
r91332 was rolled out in bug 64925 due to crashing Safari Mac and Chrome Mac (see bug 64874 for some stacktraces).

'useLowQualityScale' is in use by the CSS3 value 'optimize-contrast' (landed in bug 56627) to control the use of nearest-neighbour scaling.
Comment 21 Ryosuke Niwa 2011-07-21 00:13:21 PDT
Abhishek,

Would you be able to help Matthew figure out what went wrong?  Kenneth thinks we might have a memory corruption here (see the bug 64874).  It's possible that this bug always existed in the code base and Matthew's patch only made it easier to reproduce.
Comment 22 Abhishek Arya 2011-07-21 00:28:21 PDT
Matthew, do you a chromium build on linux ? If yes, can you try running this layouttest under ASAN. [https://sites.google.com/a/chromium.org/dev/developers/testing/addresssanitizer] (dont' try the mac instructions, it does not work properly on mac right now). I think it is introduced by your patch, because our fuzz bots are running on unmodified layouttest and i have not seen a crash on that layouttest css3/images/optimize-contrast-canvas.html before. If you are only on mac, try running with and without your patch using libgmalloc, http://developer.apple.com/library/mac/#documentation/Darwin/Reference/ManPages/man3/libgmalloc.3.html

Please paste these debugging tools output here, i can help to triage.
Comment 23 Matthew Delaney 2011-07-21 11:04:41 PDT
Woah, everything blowing up. Did that really need a rollout?

First, I see this: https://bugs.webkit.org/show_bug.cgi?id=64925
...and this: https://bugs.webkit.org/show_bug.cgi?id=64874
...are dupes.

The stack traces that Kenneth posted aren't due to my patch - they're in V8 from document parsing. The one that Mike posted surely is: it was simply an ASSERTION failure about useLowQuality scale not being used at the ImageBuffer::draw level. I understand how "optimize-contrast" is using the bool, but from my refactor, it's no longer needed below ImageBuffer::draw, hence the assertion of it being unused.

Basically, I misused the ASSERT_UNUSED macro and should have just used UNUSED_PARAM(useLowQualityScale) at that point. I'll make a patch that does that now.
Comment 24 Matthew Delaney 2011-07-21 13:34:58 PDT
Created attachment 101641 [details]
Patch
Comment 25 Matthew Delaney 2011-07-21 13:36:41 PDT
Reopened to fix the old patch to have the correct assert.
Comment 26 Simon Fraser (smfr) 2011-07-21 13:37:27 PDT
Comment on attachment 101641 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageBuffer.h:91
> +        PassRefPtr<Image> copyImage(BackingStoreCopy copyPreference = CopyBackingStore) const;

I don't like copyPreference; it sounds related to preferences. copyBehavior would be better. You don't even need to include a parameter name here.
Comment 27 Matthew Delaney 2011-07-21 13:49:46 PDT
Committed r91496: <http://trac.webkit.org/changeset/91496>