[webkit-reviews] review granted: [Bug 156129] toDataURL image upside down if premultipliedAlpha=false : [Attachment 432503] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has granted Kimmo Kinnunen
<kkinnunen at apple.com>'s request for review:
Bug 156129: toDataURL image upside down if premultipliedAlpha=false
https://bugs.webkit.org/show_bug.cgi?id=156129

Attachment 432503: Patch

https://bugs.webkit.org/attachment.cgi?id=432503&action=review




--- Comment #11 from Kenneth Russell <kbr at google.com> ---
Comment on attachment 432503
  --> https://bugs.webkit.org/attachment.cgi?id=432503
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432503&action=review

Please address Said's remaining code review comments, but this looks fine to me
overall. Would be great to get this longstanding bug fixed. r+

>> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.cpp:239
>> +	    ASSERT(results->format().pixelFormat == PixelFormat::RGBA8 ||
results->format().pixelFormat == PixelFormat::BGRA8);
> 
> GraphicsContextGLOpenGL::readPixelsForPaintResults() creates the PixelBuffer
with PixelFormat::RGBA8 and it calls gl::ReadnPixelsRobustANGLE() with GL_RGBA.
So why do we assert "|| pixelFormat == PixelFormat::BGRA8"?

I assume in order to better enforce constraints between unrelated code,
especially if the called code changes in the future - for example, to support
higher bit depth back buffers in WebGL, which is a feature being actively
developed in the ColorWeb CG.

>> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.cpp:248
>> +		memcpy(src, temp.get(), rowStride);
> 
> Can't we use vImage to accelerate this operation? I read this comment in
vImage.h:
> 
>  *	Sometimes you may want to flip an image vertically. In vImage, this can
be done cheaply by adjusting the pointer
>  *	to point to the last scanline of the image and setting rowBytes
negative, for either the source or destination
>  *	image:
> 
> I am not sure if vImage can do this in place as you did or not.

This code isn't Cocoa-specific so writing the loop manually seems most
portable.