Upstream BlackBerry porting of platform/image-decoders

• Component: Images
• Status: RESOLVED FIXED

[Sean Wang]

Blackberry's implementation of platform/image-decoders

[Sean Wang]

Created attachment 116587 [details]
Patch

[Daniel Bates]

Comment on attachment 116587 [details]
Patch

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

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.cpp:24
> +/**
> + * Implementation notes:
> + *
> + * Current implementation provides the source image size without doing a full
> + * decode. It seems that libimg does not support partial decoding.
> + */

Please use C++-style comments.

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.cpp:35
> +static bool      s_initialized = false;

Remove the spaces between "bool" and "s_initialized". We don't need to explicitly initialize s_initialized. s_initialized will be zero-initialized and hence false because it is a static variable. We should also move this declaration into libInit() since it is the only function that makes use of this variable.

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.cpp:53
> +    ImageReader(const char* data, size_t dataSize) :
> +        m_data(data), m_size(dataSize), m_width(0), m_height(0) { libInit(); }

This should be written:

ImageReader(const char* data, size_t dataSize)
    : m_data(data)
    , m_size(dataSize)
    , m_width(0)
    , m_height(0)
{
    libInit();
}

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.cpp:59
> +    int decode(size_t width, size_t height, ImageFrame* theFrame);

The argument name "theFrame" doesn't add any additional value to this signature that cannot be inferred from its datatype. Please remove it.

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

Remove this empty line. The "private:" and its different indentation level sufficiently demarcates this part of the class declaration. I don't feel that this empty line further improves the readability of this class.

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.cpp:120
> +int ImageReader::decode(size_t width, size_t height, ImageFrame* theFrame)

I suggest renaming theFrame to aFrame or imageFrame or frame. The "the" in "theFrame" implies that there is only one such ImageFrame object, which isn't the case.

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.cpp:132
> +    _uint8* buf = reinterpret_cast<_uint8*>(malloc(stride * height));

buf => buffer

Can we use OwnArrayPtr here? Then we can remove the free(buf) calls since OwnArrayPtr will deallocate the array on destruction.

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.cpp:216
> +    // Check to see if it's already decoded. TODO: Could size change

TODO => FIXME

Also, the FIXME should be written on its own line to make it easier to spot.

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.h:37
> +    // This class decodes the JPEG image format.

This comment doesn't say anything more than what can be inferred from the name of this class. Either improve this comment or remove it.

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.h:47
> +        virtual ImageFrame* frameBufferAtIndex(size_t index);

The argument name "index" doesn't add any additional value to this signature that cannot be inferred from the name of this method. Please remove it.

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.h:49
> +        virtual void setData(SharedBuffer* data, bool allDataReceived);

The argument name "data" doesn't add any additional value to this signature that cannot be inferred from the name of this method. Please remove it.

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.h:52
> +        ImageReader* m_reader;

We should use an OwnPtr for this.

[Sean Wang]

Created attachment 116696 [details]
The fixed patch.

[Sean Wang]

Comment on attachment 116696 [details]
The fixed patch.

make some change.

[WebKit Review Bot]

Attachment 116696 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sean Wang]

Created attachment 116697 [details]
Patch 2

[Daniel Bates]

Comment on attachment 116697 [details]
Patch 2

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

> Source/WebCore/platform/image-decoders/blackberry/JPEGImageDecoder.cpp:135
> +    _uint8* buffer = reinterpret_cast<_uint8*>(malloc(stride * height));

Can we use OwnArrayPtr here? Then we can remove the "free(buffer)" calls throughout this function.

[Sean Wang]

Created attachment 116870 [details]
Patch 3

[Daniel Bates]

Comment on attachment 116870 [details]
Patch 3

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

[Sean Wang]

Created attachment 116898 [details]
Patch 4

[Daniel Bates]

Comment on attachment 116898 [details]
Patch 4

Thank you Sean for updating the patch.

[WebKit Review Bot]

Comment on attachment 116898 [details]
Patch 4

Clearing flags on attachment: 116898

Committed r101336: <http://trac.webkit.org/changeset/101336>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.