Bug 58088 – [CG] Use vImage (un)premultiplyImageData functions for get/putImageData with IOSurfaces

• Component: Canvas
• Status: RESOLVED FIXED

[Matthew Delaney]

Use the Accelerate framework's vImage (un)premultiplyImageData functions for get/putImageData when using IOSurfaces in ImageBufferCG/ImageBufferData

[Matthew Delaney]

Created attachment 89615 [details]
Patch

[WebKit Review Bot]

Attachment 89615 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8397703

[Matthew Delaney]

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

[Matthew Delaney]

Created attachment 89636 [details]
Patch

[Sam Weinig]

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?

[Matthew Delaney]

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

[WebKit Review Bot]

Attachment 89636 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8399808

[Matthew Delaney]

Created attachment 89703 [details]
Patch

[Simon Fraser (smfr)]

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.

[Matthew Delaney]

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

[Simon Fraser (smfr)]

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

[Matthew Delaney]

Created attachment 90054 [details]
Patch

[Simon Fraser (smfr)]

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

[Matthew Delaney]

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

[Matthew Delaney]

Created attachment 90067 [details]
Patch

[Matthew Delaney]

Committed r84172: <http://trac.webkit.org/changeset/84172>