Closed Bug 552537 Opened 15 years ago Closed 15 years ago

Image rendering not taking advantage of Quartz caching

Categories

(Core :: Graphics: Canvas2D, defect)

x86
macOS
defect
Not set
normal

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

Attachment

General

Created:
Updated:
Size: