[Webkit-unassigned] [Bug 73118] Upstream BlackBerry porting of platform/image-decoders

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=73118


Daniel Bates <dbates at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #116870|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #9 from Daniel Bates <dbates at webkit.org>  2011-11-28 21:10:34 PST ---
(From update of attachment 116870)
View in context: https://bugs.webkit.org/attachment.cgi?id=116870&action=review

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.cpp:135
> +    int stride = ((width * 3) + 3) & ~3;

This line is a bit complicated. The 3 in the sub-expression "width * 3" refers to the number of color components in a stride: RGB, where as the other 3s in this line are part of a bitwise trick to ensure that the stride is a multiple of 2. If possible, it would be nice to emphasize the meaning for the first '3' and/or query libimg for the number of color components for the image format IMG_FMT_RGB888.

Nit: I suggest adding an inline comment here to explain that we ensure that the stride is a multiple of 2. I take it there are some performance implications to using a multiple of 2. It would be great if we could document the reason we make the stride a multiple of 2 in such a comment.

Also, the inner parentheses aren't necessary since multiplication has a higher precedence than addition.

One way to write this line would be:

const int ColorComponents = 3;
int stride = (ColorComponents * width + 3) & ~3; // Use a multiple of 2 bytes to improve performance

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.cpp:155
> +            aFrame->setRGBA(i, j, *(curPtr), *(curPtr + 1), *(curPtr + 2), 255);

Nit: I would write this as:

aFrame->setRGBA(i, j, curPtr[0], curPtr[1], curPtr[2], 255);

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.cpp:237
> +}

Missing inline comment // namespace WebCore

(this file is longer than JPEGImageDecoder.h and hence having such an inline comment helps demarcate this curly brace as the end of namespace WebCore)

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.h:55
> +#endif

Nit: This is a short file. That being said, it's convention to put an inline comment here with the name of the #ifndef, // JPEGImageDecoder_h

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.