Closed Bug 552537 Opened 10 years ago Closed 10 years ago

Image rendering not taking advantage of Quartz caching

Categories

(Core :: Canvas: 2D, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: roc, Assigned: roc)

References

Details

Attachments

(4 files)

Attached image image for test
No description provided.
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.
Attached file 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.
Each call to CGContextDrawImage is using the same surface->cgContext but different surface->sourceImage.
Attached patch fixSplinter Review
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.
Assignee: nobody → roc
Attachment #432713 - Flags: review?(jmuizelaar)
Whiteboard: [needs review]
I believe CGImages are valid, even across modification, for the lifetime of the CGBitmapContext that they are associated with.
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.
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.
(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.
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.
Oh, I guess we're still significantly slower on the ImageData copy.
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 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.
Attachment #432713 - Flags: review?(jmuizelaar) → review+
_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.
Whiteboard: [needs review] → [waiting for cairo update]
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.
(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?
(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);
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?
Ah true. I'll have to think/investigate further.
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.
Jeff, can we get this in? This might affect my work on scrolling.
Whiteboard: [waiting for cairo update]
Whiteboard: [needs landing]
The patch, at least the updated version of it in my tree, depends on bug 522859's first patch.
Whiteboard: [needs landing]
Whiteboard: [depends on 522859]
Blocks: 564991
Whiteboard: [depends on 522859] → [needs landing]
http://hg.mozilla.org/mozilla-central/rev/7881873b2b5d
http://hg.mozilla.org/mozilla-central/rev/e0ead431e036
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
You need to log in before you can comment on or make changes to this bug.