Bug 64535 – Add fast path for ImageBuffer::draw

• Component: Canvas
• Status: RESOLVED FIXED

[Matthew Delaney]

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.

[Matthew Delaney]

Created attachment 100824 [details]
Patch

[Simon Fraser (smfr)]

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().

[Ian Henderson]

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?

[Matthew Delaney]

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.

[Matthew Delaney]

Created attachment 101194 [details]
Patch

[WebKit Review Bot]

Comment on attachment 101194 [details]
Patch

Attachment 101194 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9115386

[Gyuyoung Kim]

Comment on attachment 101194 [details]
Patch

Attachment 101194 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9120089

[Early Warning System Bot]

Comment on attachment 101194 [details]
Patch

Attachment 101194 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9115391

[Gustavo Noronha (kov)]

Comment on attachment 101194 [details]
Patch

Attachment 101194 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9110538

[Matthew Delaney]

Created attachment 101275 [details]
Patch

[WebKit Review Bot]

Comment on attachment 101275 [details]
Patch

Attachment 101275 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9150053

[Early Warning System Bot]

Comment on attachment 101275 [details]
Patch

Attachment 101275 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9154036

[WebKit Review Bot]

Comment on attachment 101275 [details]
Patch

Attachment 101275 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9159038

[Matthew Delaney]

Created attachment 101333 [details]
Patch

[Simon Fraser (smfr)]

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.

[Matthew Delaney]

Committed r91332: <http://trac.webkit.org/changeset/91332>

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Filed https://bugs.webkit.org/show_bug.cgi?id=64874.

[Adam Roben (:aroben)]

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.)

[Mike Lawther]

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.

[Ryosuke Niwa]

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.

[Abhishek Arya]

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.

[Matthew Delaney]

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.

[Matthew Delaney]

Created attachment 101641 [details]
Patch

[Matthew Delaney]

Reopened to fix the old patch to have the correct assert.

[Simon Fraser (smfr)]

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.

[Matthew Delaney]

Committed r91496: <http://trac.webkit.org/changeset/91496>