[webkit-reviews] review denied: [Bug 59670] [Performance] [Chromium] Remove unnecessary memory copies in JPEGImageDecoder::outputScanlines() : [Attachment 91421] A quick fix v0

[bugzilla-daemon at webkit.org]

Peter Kasting <pkasting at google.com> has denied Hironori Bono
<hbono at chromium.org>'s request for review:
Bug 59670: [Performance] [Chromium] Remove unnecessary memory copies in
JPEGImageDecoder::outputScanlines()
https://bugs.webkit.org/show_bug.cgi?id=59670

Attachment 91421: A quick fix v0
https://bugs.webkit.org/attachment.cgi?id=91421&action=review

------- Additional Comments from Peter Kasting <pkasting at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=91421&action=review

> Source/WebCore/ChangeLog:9
> +	   to copy row pixels from a vector to a ImageFrame because
libjpeg-turbo

Nit: "a ImageFrame" -> "an ImageFrame"

> Source/WebCore/ChangeLog:10
> +	   can output RBA pixels used by WebKit. This change writes the output

Nit: RBA -> RGBA

> Source/WebCore/ChangeLog:12
> +	   this memory copies.

Nit: "this memory copies" -> "the extra copy"

> Source/WebCore/platform/image-decoders/ImageDecoder.h:56
> +	   friend class JPEGImageDecoder;

This isn't the right way to do this.

I'm concerned about blindly writing into the buffer.  Do all platforms
guarantee the image format is 32bpp ARGB and that the stride is not larger than
the actual line width?	If so, you can probably just make getAddr() public.  If
not, you'll need a function which returns a pointer to the image data only if
the above are met, and NULL otherwise.

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:220
> +		   m_info.out_color_space = JCS_EXT_BGRX;

I believe this is only correct for little-endian machines.  For big-endian I
think you need XRGB, because libjpeg-turbo writes using byte offsets rather
than bit shifts, so it doesn't auto-swap with machine endianness.  (I'm saying
this based on trying to decipher
http://sourceforge.net/mailarchive/forum.php?thread_name=4D850B59.1050201%40use
rs.sourceforge.net&forum_name=tigervnc-users ).