[webkit-reviews] review denied: [Bug 58088] [CG] Use vImage (un)premultiplyImageData functions for get/putImageData with IOSurfaces : [Attachment 89703] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Matthew Delaney
<mdelaney at apple.com>'s request for review:
Bug 58088: [CG] Use vImage (un)premultiplyImageData functions for
get/putImageData with IOSurfaces
https://bugs.webkit.org/show_bug.cgi?id=58088

Attachment 89703: Patch
https://bugs.webkit.org/attachment.cgi?id=89703&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
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.