Closed
Bug 552537
Opened 15 years ago
Closed 15 years ago
Image rendering not taking advantage of Quartz caching
Categories
(Core :: Graphics: Canvas2D, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: roc, Assigned: roc)
References
Details
Attachments
(4 files)
No description provided.
Assignee | ||
Comment 1•15 years ago
|
||
This is a copy of http://webkit.org/demos/canvas-perf/canvas.html
Assignee | ||
Comment 2•15 years ago
|
||
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.
Assignee | ||
Comment 3•15 years ago
|
||
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.
Assignee | ||
Comment 4•15 years ago
|
||
Each call to CGContextDrawImage is using the same surface->cgContext but different surface->sourceImage.
Assignee | ||
Comment 5•15 years ago
|
||
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)
Assignee | ||
Updated•15 years ago
|
Whiteboard: [needs review]
Comment 6•15 years ago
|
||
I believe CGImages are valid, even across modification, for the lifetime of the CGBitmapContext that they are associated with.
Assignee | ||
Comment 7•15 years ago
|
||
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.
Assignee | ||
Comment 8•15 years ago
|
||
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•15 years ago
|
||
(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.
Assignee | ||
Comment 10•15 years ago
|
||
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.
Assignee | ||
Comment 11•15 years ago
|
||
Oh, I guess we're still significantly slower on the ImageData copy.
Assignee | ||
Comment 12•15 years ago
|
||
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•15 years ago
|
||
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+
Assignee | ||
Comment 14•15 years ago
|
||
_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]
Comment 15•15 years ago
|
||
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.
Assignee | ||
Comment 16•15 years ago
|
||
(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•15 years ago
|
||
(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);
Assignee | ||
Comment 18•15 years ago
|
||
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•15 years ago
|
||
Ah true. I'll have to think/investigate further.
Assignee | ||
Comment 20•15 years ago
|
||
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.
Assignee | ||
Comment 21•15 years ago
|
||
Jeff, can we get this in? This might affect my work on scrolling.
Whiteboard: [waiting for cairo update]
Assignee | ||
Updated•15 years ago
|
Whiteboard: [needs landing]
Assignee | ||
Comment 22•15 years ago
|
||
The patch, at least the updated version of it in my tree, depends on bug 522859's first patch.
Whiteboard: [needs landing]
Assignee | ||
Updated•15 years ago
|
Whiteboard: [depends on 522859]
Assignee | ||
Comment 23•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/7881873b2b5d
http://hg.mozilla.org/mozilla-central/rev/e0ead431e036
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
You need to log in
before you can comment on or make changes to this bug.
Description
•