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.
Created attachment 100824 [details] Patch
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().
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?
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.
Created attachment 101194 [details] Patch
Comment on attachment 101194 [details] Patch Attachment 101194 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9115386
Comment on attachment 101194 [details] Patch Attachment 101194 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9120089
Comment on attachment 101194 [details] Patch Attachment 101194 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9115391
Comment on attachment 101194 [details] Patch Attachment 101194 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9110538
Created attachment 101275 [details] Patch
Comment on attachment 101275 [details] Patch Attachment 101275 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9150053
Comment on attachment 101275 [details] Patch Attachment 101275 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9154036
Comment on attachment 101275 [details] Patch Attachment 101275 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9159038
Created attachment 101333 [details] Patch
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.
Committed r91332: <http://trac.webkit.org/changeset/91332>
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
Filed https://bugs.webkit.org/show_bug.cgi?id=64874.
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.)
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.
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.
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.
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.
Created attachment 101641 [details] Patch
Reopened to fix the old patch to have the correct assert.
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.
Committed r91496: <http://trac.webkit.org/changeset/91496>