Bug 58088

Summary: [CG] Use vImage (un)premultiplyImageData functions for get/putImageData with IOSurfaces
Product: WebKit Reporter: Matthew Delaney <mdelaney7>
Component: CanvasAssignee: Matthew Delaney <mdelaney7>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, dglazkov, mdelaney7, sam, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 58084    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch simon.fraser: review+

Description Matthew Delaney 2011-04-07 15:03:32 PDT
Use the Accelerate framework's vImage (un)premultiplyImageData functions for get/putImageData when using IOSurfaces in ImageBufferCG/ImageBufferData
Comment 1 Matthew Delaney 2011-04-14 11:46:45 PDT
Created attachment 89615 [details]
Patch
Comment 2 WebKit Review Bot 2011-04-14 12:16:17 PDT
Attachment 89615 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8397703
Comment 3 Matthew Delaney 2011-04-14 13:12:33 PDT
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%.
Comment 4 Matthew Delaney 2011-04-14 13:13:31 PDT
Created attachment 89636 [details]
Patch
Comment 5 Sam Weinig 2011-04-14 16:56:41 PDT
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?
Comment 6 Matthew Delaney 2011-04-14 17:10:00 PDT
> > 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.
Comment 7 WebKit Review Bot 2011-04-14 17:14:20 PDT
Attachment 89636 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8399808
Comment 8 Matthew Delaney 2011-04-14 17:44:17 PDT
Created attachment 89703 [details]
Patch
Comment 9 Simon Fraser (smfr) 2011-04-16 14:42:32 PDT
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.
Comment 10 Matthew Delaney 2011-04-18 09:03:31 PDT
> 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.
Comment 11 Simon Fraser (smfr) 2011-04-18 09:15:27 PDT
(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.
Comment 12 Matthew Delaney 2011-04-18 11:08:45 PDT
Created attachment 90054 [details]
Patch
Comment 13 Simon Fraser (smfr) 2011-04-18 11:16:26 PDT
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
Comment 14 Matthew Delaney 2011-04-18 12:19:12 PDT
(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.
Comment 15 Matthew Delaney 2011-04-18 12:25:19 PDT
Created attachment 90067 [details]
Patch
Comment 16 Matthew Delaney 2011-04-18 12:58:29 PDT
Committed r84172: <http://trac.webkit.org/changeset/84172>