Bug 73118 - Upstream BlackBerry porting of platform/image-decoders
Summary: Upstream BlackBerry porting of platform/image-decoders
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 73144
  Show dependency treegraph
 
Reported: 2011-11-25 02:16 PST by Sean Wang
Modified: 2011-11-29 00:10 PST (History)
4 users (show)

See Also:


Attachments
Patch (12.80 KB, patch)
2011-11-25 02:39 PST, Sean Wang
dbates: review-
dbates: commit-queue-
Details | Formatted Diff | Diff
The fixed patch. (12.61 KB, patch)
2011-11-27 21:33 PST, Sean Wang
xuewen.ok: review-
xuewen.ok: commit-queue-
Details | Formatted Diff | Diff
Patch 2 (12.50 KB, patch)
2011-11-27 22:15 PST, Sean Wang
no flags Details | Formatted Diff | Diff
Patch 3 (12.40 KB, patch)
2011-11-28 19:22 PST, Sean Wang
dbates: review-
dbates: commit-queue-
Details | Formatted Diff | Diff
Patch 4 (12.22 KB, patch)
2011-11-28 22:29 PST, Sean Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sean Wang 2011-11-25 02:16:57 PST
Blackberry's implementation of platform/image-decoders
Comment 1 Sean Wang 2011-11-25 02:39:02 PST
Created attachment 116587 [details]
Patch
Comment 2 Daniel Bates 2011-11-26 11:03:12 PST
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.
Comment 3 Sean Wang 2011-11-27 21:33:48 PST
Created attachment 116696 [details]
The fixed patch.
Comment 4 Sean Wang 2011-11-27 21:35:38 PST
Comment on attachment 116696 [details]
The fixed patch.

make some change.
Comment 5 WebKit Review Bot 2011-11-27 21:36:38 PST
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.
Comment 6 Sean Wang 2011-11-27 22:15:34 PST
Created attachment 116697 [details]
Patch 2
Comment 7 Daniel Bates 2011-11-28 00:44:58 PST
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.
Comment 8 Sean Wang 2011-11-28 19:22:46 PST
Created attachment 116870 [details]
Patch 3
Comment 9 Daniel Bates 2011-11-28 21:10:34 PST
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
Comment 10 Sean Wang 2011-11-28 22:29:03 PST
Created attachment 116898 [details]
Patch 4
Comment 11 Daniel Bates 2011-11-28 23:38:25 PST
Comment on attachment 116898 [details]
Patch 4

Thank you Sean for updating the patch.
Comment 12 WebKit Review Bot 2011-11-29 00:10:46 PST
Comment on attachment 116898 [details]
Patch 4

Clearing flags on attachment: 116898

Committed r101336: <http://trac.webkit.org/changeset/101336>
Comment 13 WebKit Review Bot 2011-11-29 00:10:51 PST
All reviewed patches have been landed.  Closing bug.