552537 - Image rendering not taking advantage of Quartz caching

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: image for test

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: testcase #1

This is a copy of http://webkit.org/demos/canvas-perf/canvas.html

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Er, that testcase has the number of test runs configurable via a value "N", and N is set to 1. The webkit.org test uses 2.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: testcase #2

This does 10 runs of the scale-down-by-0.5 test. Safari's an order of magnitude faster than Firefox on this one. If you do just one run, they're about the same. This suggests a caching issue.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Each call to CGContextDrawImage is using the same surface->cgContext but different surface->sourceImage.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This massively improves the testcase. We just need to cache the CGImageRef created with CGBitmapContextCreateImage. If the surface contents change, we flush the cache, *before* making the change so that no unnecessary copy is made.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

I believe CGImages are valid, even across modification, for the lifetime of the CGBitmapContext that they are associated with.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The docs say:
> Subsequent changes to the bitmap graphics context do not affect the contents of
> the returned image. In some cases the copy operation actually follows
> copy-on-write semantics, so that the actual physical copy of the bits occur
> only if the underlying data in the bitmap graphics context is modified. As a
> consequence, you may want to use the resulting image and release it before you
> perform additional drawing into the bitmap graphics context

http://developer.apple.com/Mac/library/documentation/GraphicsImaging/Reference/CGBitmapContext/Reference/reference.html#//apple_ref/c/func/CGBitmapContextCreateImage

Therefore, it would be incorrect to reuse the bitmapContextImage if the surface has been modified since we created it. And for performance, we should release the image before the surface is modified so Quartz doesn't make a copy.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

On my local copy of the canvas-perf test, Firefox trunk vs Safari 4.0.4, on my Macbook Pro, on Leopard, under not very scientific conditions:

Firefox:
  Direct image copy: 109ms
  Indirect copy with (via ImageData): 298ms
  Copy with 2x scale: 1487ms
  Copy with 0.5x scale: 334ms
  Copy with rotate:148ms

Safari:
  Direct image copy: 58ms
  Indirect copy with (via ImageData): 249ms
  Copy with 2x scale: 1289ms
  Copy with 0.5x scale: 306ms
  Copy with rotate:207ms

Still some room for improvement, but we're not getting crushed. "Direct image copy" would probably be the think to look at next, then ImageData.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #7)
> 
> Therefore, it would be incorrect to reuse the bitmapContextImage if the surface
> has been modified since we created it. And for performance, we should release
> the image before the surface is modified so Quartz doesn't make a copy.

Indeed. I was confused by the Webkit code. They have a CGImageRef cache associated with the canvas instead of with the surface. This has the advantage of not bloating our surface structure by 4 bytes, but, as implemented by them, requires a copy on non-OSX platforms for the first draw.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If I do ten runs of each test the numbers look even better for us...

Firefox:
Direct image copy: 37.1ms
Indirect copy with (via ImageData): 317.6ms
Copy with 2x scale: 1305.9ms
Copy with 0.5x scale: 40.3ms
Copy with rotate:95.7ms

Safari:
Direct image copy: 32.1ms
Indirect copy with (via ImageData): 244ms
Copy with 2x scale: 1271.1ms
Copy with 0.5x scale: 38.4ms
Copy with rotate:208.6ms

Basically very similar except for copy-with-rotate, where we're twice as fast for some reason.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oh, I guess we're still significantly slower on the ImageData copy.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, I did some profiling and realized that the test times include the time for canvas.getContext("2d") for each of the output canvases. We allocate the canvas surface during that call, whereas Webkit seems to have already allocated it. Taking the getContext("2d") call out of the timing loop gives us even closer results.

One-run case:

Firefox:
  Direct image copy: 51ms
  Indirect copy with (via ImageData): 325ms
  Copy with 2x scale: 1243ms
  Copy with 0.5x scale: 322ms
  Copy with rotate:90ms

Safari:
  Direct image copy: 53ms
  Indirect copy with (via ImageData): 256ms
  Copy with 2x scale: 1269ms
  Copy with 0.5x scale: 307ms
  Copy with rotate:203ms

Ten-runs case:

Firefox:
  Direct image copy: 31.4ms
  Indirect copy with (via ImageData): 346.4ms
  Copy with 2x scale: 1302.2ms
  Copy with 0.5x scale: 38.9ms
  Copy with rotate:91.3ms

Safari:
  Direct image copy: 31.9ms
  Indirect copy with (via ImageData): 267.1ms
  Copy with 2x scale: 1284.1ms
  Copy with 0.5x scale: 37.7ms
  Copy with rotate:205.7ms

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 432713 [details] [diff] [review]
fix

Webkit uses willDraw instead of will_change which I sort of like better, but I don't feel strongly. Other than that it looks good. However, I'd prefer to wait with this until after the cairo update if that's ok.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

_cairo_quartz_surface_will_draw seems less clear than _cairo_surface_will_change, to me, since it's less clear that the surface will be the destination of the draw rather than the source.

No problem waiting for the cairo update.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

I was doing some experimentation and it looks like we can alias the backing of a CGBitmapContext with the backing of CGImageRef. This would let us avoid calling CGBitmapContextCreateImage and allow us to avoid having to track the dirtiness and avoid the vm_copy() that CGBitmapContextCreateImage does.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #15)
> I was doing some experimentation and it looks like we can alias the backing of
> a CGBitmapContext with the backing of CGImageRef.

How?

Comment 17

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #16)
> (In reply to comment #15)
> > I was doing some experimentation and it looks like we can alias the backing of
> > a CGBitmapContext with the backing of CGImageRef.
> 
> How?

Like this:

        unsigned int *inner_data = calloc(width*4 * height, 1);
        CGDataProviderRef dataProvider = CGDataProviderCreateWithData (DataProviderReleaseCallback,
                        inner_data,
                        height * width*4,
                        NULL);

        CGImageRef im = CGImageCreate (
                        width,
                        height,
                        8,
                        32,
                        4*width,
                        cs,
                        BITMAP_INFO,
                        dataProvider,
                        NULL,
                        false,
                        kCGRenderingIntentDefault);

        CGContextRef c = CGBitmapContextCreate(inner_data, width, height,
                        8, 4*width, cs, BITMAP_INFO);

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't see how that can work properly when Quartz caches scaled versions of the image, which is what we're trying to take advantage of here. Quartz has no way of telling when the backing data has changed and invalidating its cache. How do you think that would work?

Comment 19

[Jeff Muizelaar [:jrmuizel]]

Ah true. I'll have to think/investigate further.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think we can get away from notifying Quartz in some way when the bitmap data has changed. My patch does that by destroying and (lazily) recreating the CGImage. It seems to me that any other notification approach would have to have similar complexity (at least) --- and it isn't much.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Jeff, can we get this in? This might affect my work on scrolling.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The patch, at least the updated version of it in my tree, depends on bug 522859's first patch.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/7881873b2b5d
http://hg.mozilla.org/mozilla-central/rev/e0ead431e036