Use the Accelerate framework's vImage (un)premultiplyImageData functions for get/putImageData when using IOSurfaces in ImageBufferCG/ImageBufferData
Created attachment 89615 [details] Patch
Attachment 89615 [details] did not build on mac: Build output: http://queues.webkit.org/results/8397703
Last patch failed to build on mac due to the new scanline conversion functions not being used when iosurface backing store isn't used. Updating the patch now to define out those function if not using iosurface backing store. Also, I took a stab at measuring the performance increase of this new method on my iMac and saw about a 30%-50% performance improvement. I created a test that gets and puts image data to and from the same canvas thousands, tens/hundreds of thousands of times and consistently saw at least at 33% increase and at times up to 50%.
Created attachment 89636 [details] Patch
Comment on attachment 89636 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89636&action=review > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:44 > +typedef struct vImageBufferPair { > + vImage_Buffer src; > + vImage_Buffer dest; > +} vImageBufferPair; This is c++, so the typedef is not necessary. > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:66 > +#if USE(IOSURFACE_CANVAS_BACKING_STORE) Is this optimization only worth while for IOSurface backed canvas, or would anyone with vImage want it? Maybe it should be in the USE(ACCELLERATE). > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:69 > + vImageBufferPair* bufferPair = (vImageBufferPair*) data; Please use c++ style casts and don't put a space after the cast. > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:72 > + src.data = (char*) bufferPair->src.data + tileNumber * bufferPair->src.rowBytes; Cast again. > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:78 > + dest.data = (char*) bufferPair->dest.data + tileNumber * bufferPair->dest.rowBytes; And again. > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:216 > + dispatch_apply_f(height, dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), &buffPair, unpremultitplyScanline); Why is that queue the right queue to use? Will it be a concurrent queue?
> > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:66 > > +#if USE(IOSURFACE_CANVAS_BACKING_STORE) > > Is this optimization only worth while for IOSurface backed canvas, or would anyone with vImage want it? Maybe it should be in the USE(ACCELLERATE). vImage is already being used by those others not using IOSurfaces. This is adding support for using vImage for those using IOSurface since it was a little less straightforward due to the difference in pixel type in IOSurface (BGRA) from what ImageBufferData clients need (i.e. RGBA) > > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:216 > > + dispatch_apply_f(height, dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), &buffPair, unpremultitplyScanline); > > Why is that queue the right queue to use? Will it be a concurrent queue? It will be a concurrent queue; dispatch_get_global_queue says that it returns a concurrent queue. The choice of a default priority was simply due to the fact that I saw no reason for this process to have higher of lower priority than "normal". We are using it for speed/efficiency and thus want it to be fast, but I see no reason to have it run at a higher priority than other tasks.
Attachment 89636 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8399808
Created attachment 89703 [details] Patch
Comment on attachment 89703 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89703&action=review Since this looks wrong, it calls into question your testing. > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:43 > + vImage_Buffer src; > + vImage_Buffer dest; Can these be const vImage_Buffer& to avoid copying? > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:75 > + vImage_Buffer src; > + src.data = static_cast<char*>(bufferPair->src.data) + tileNumber * bufferPair->src.rowBytes; > + src.height = 1; > + src.width = bufferPair->src.width; > + src.rowBytes = bufferPair->src.rowBytes; So you only use data, width and rowBytes from the bufferPair; it might be clearer to pass these, rather than the whole vImage_Buffer. > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:87 > + vImagePermuteChannels_ARGB8888(&dest, &dest, map, kvImageDoNotTile); &dest, &dest, really? Why isn't src used? > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:110 > + vImagePermuteChannels_ARGB8888(&dest, &dest, map, kvImageDoNotTile); Ditto. > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:213 > + buffPair.src = src; > + buffPair.dest = dst; This copies those structs.
> Since this looks wrong, it calls into question your testing. Since what's wrong? > > So you only use data, width and rowBytes from the bufferPair; it might be clearer to pass these, rather than the whole vImage_Buffer. Good call. I'll reduce the struct. > > > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:87 > > + vImagePermuteChannels_ARGB8888(&dest, &dest, map, kvImageDoNotTile); > > &dest, &dest, really? Why isn't src used? > > > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:110 > > + vImagePermuteChannels_ARGB8888(&dest, &dest, map, kvImageDoNotTile); > > Ditto. Because we're just permuting the channels BGRA<->RGBA here.
(In reply to comment #10) > > Since this looks wrong, it calls into question your testing. > Since what's wrong? I was referring to the dest,dest. > > So you only use data, width and rowBytes from the bufferPair; it might be clearer to pass these, rather than the whole vImage_Buffer. > Good call. I'll reduce the struct. > > > > > > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:87 > > > + vImagePermuteChannels_ARGB8888(&dest, &dest, map, kvImageDoNotTile); > > > > &dest, &dest, really? Why isn't src used? > > > > > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:110 > > > + vImagePermuteChannels_ARGB8888(&dest, &dest, map, kvImageDoNotTile); > > > > Ditto. > Because we're just permuting the channels BGRA<->RGBA here. Then you should add a comment, since at a glance this looks like a programming error.
Created attachment 90054 [details] Patch
Comment on attachment 90054 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=90054&action=review > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:41 > +struct vImageBufferPair { This is no longer a vImageBufferPair. > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:96 > + vImagePermuteChannels_ARGB8888(&dest, &dest, map, kvImageDoNotTile); Is vImagePermuteChannels_ARGB8888 documented to work when source and destination are the same? Two spaces before kVImageDoNotTile. > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:217 > + const uint8_t map[4] = { 2, 1, 0, 3 }; 'map' is a little obscure here. You need a comment to say what this permute is doing. > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:354 > + const uint8_t map[4] = { 2, 1, 0, 3 }; > + vImagePermuteChannels_ARGB8888(&src, &dest, map, kvImageNoFlags); Ditto
(In reply to comment #13) > (From update of attachment 90054 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=90054&action=review > > > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:41 > > +struct vImageBufferPair { > > This is no longer a vImageBufferPair. Changed to ScanlineData > > > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:96 > > + vImagePermuteChannels_ARGB8888(&dest, &dest, map, kvImageDoNotTile); > > Is vImagePermuteChannels_ARGB8888 documented to work when source and destination are the same? yep. > > Two spaces before kVImageDoNotTile. Fixed. > > > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:217 > > + const uint8_t map[4] = { 2, 1, 0, 3 }; > > 'map' is a little obscure here. You need a comment to say what this permute is doing. > > > Source/WebCore/platform/graphics/cg/ImageBufferDataCG.cpp:354 > > + const uint8_t map[4] = { 2, 1, 0, 3 }; > > + vImagePermuteChannels_ARGB8888(&src, &dest, map, kvImageNoFlags); > > Ditto Added comments.
Created attachment 90067 [details] Patch
Committed r84172: <http://trac.webkit.org/changeset/84172>