Bug 64535 - Add fast path for ImageBuffer::draw
: Add fast path for ImageBuffer::draw
Status: RESOLVED FIXED
: WebKit
Canvas
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 64925
:
  Show dependency treegraph
 
Reported: 2011-07-14 09:53 PST by
Modified: 2011-07-21 13:49 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-07-14 09:53:47 PST
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 From 2011-07-14 10:22:08 PST -------
Created an attachment (id=100824) [details]
Patch
------- Comment #2 From 2011-07-14 10:28:20 PST -------
(From update of attachment 100824 [details])
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 From 2011-07-14 12:17:45 PST -------
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 From 2011-07-18 13:12:33 PST -------
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 From 2011-07-18 13:14:27 PST -------
Created an attachment (id=101194) [details]
Patch
------- Comment #6 From 2011-07-18 13:24:01 PST -------
(From update of attachment 101194 [details])
Attachment 101194 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9115386
------- Comment #7 From 2011-07-18 13:26:20 PST -------
(From update of attachment 101194 [details])
Attachment 101194 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9120089
------- Comment #8 From 2011-07-18 13:29:44 PST -------
(From update of attachment 101194 [details])
Attachment 101194 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9115391
------- Comment #9 From 2011-07-18 14:41:44 PST -------
(From update of attachment 101194 [details])
Attachment 101194 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9110538
------- Comment #10 From 2011-07-18 23:32:04 PST -------
Created an attachment (id=101275) [details]
Patch
------- Comment #11 From 2011-07-18 23:47:34 PST -------
(From update of attachment 101275 [details])
Attachment 101275 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9150053
------- Comment #12 From 2011-07-18 23:49:38 PST -------
(From update of attachment 101275 [details])
Attachment 101275 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9154036
------- Comment #13 From 2011-07-19 00:37:24 PST -------
(From update of attachment 101275 [details])
Attachment 101275 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9159038
------- Comment #14 From 2011-07-19 09:31:52 PST -------
Created an attachment (id=101333) [details]
Patch
------- Comment #15 From 2011-07-19 13:24:21 PST -------
(From update of attachment 101333 [details])
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 From 2011-07-19 21:29:21 PST -------
Committed r91332: <http://trac.webkit.org/changeset/91332>
------- Comment #17 From 2011-07-20 08:01:37 PST -------
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 From 2011-07-20 08:19:37 PST -------
Filed https://bugs.webkit.org/show_bug.cgi?id=64874.
------- Comment #19 From 2011-07-20 09:49:44 PST -------
(From update of attachment 101333 [details])
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 From 2011-07-21 00:01:24 PST -------
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 From 2011-07-21 00:13:21 PST -------
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 From 2011-07-21 00:28:21 PST -------
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 From 2011-07-21 11:04:41 PST -------
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 From 2011-07-21 13:34:58 PST -------
Created an attachment (id=101641) [details]
Patch
------- Comment #25 From 2011-07-21 13:36:41 PST -------
Reopened to fix the old patch to have the correct assert.
------- Comment #26 From 2011-07-21 13:37:27 PST -------
(From update of attachment 101641 [details])
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 From 2011-07-21 13:49:46 PST -------
Committed r91496: <http://trac.webkit.org/changeset/91496>