Upstream changes from Android for decoding jpeg images.

Upstream changes from Android for decoding jpeg images.

Committed: https://code.google.com/p/skia/source/detail?r=8267

[djsollen, 2013-03-18 20:11:31]

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
File src/images/SkImageDecoder_libjpeg.cpp (right):

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:31: #endif
these lines need to be commented out. I also need to document that they are
useful to enable the same code path that the android framework uses for image
decoding.

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:359: bm->setIsOpaque(true);
the above two lines need moved to line 346

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:385: while (cinfo.output_scanline <
cinfo.output_height) {
remove space

[robertphillips, 2013-03-19 13:15:21]

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
File src/images/SkImageDecoder_libjpeg.cpp (right):

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:32: 
These enable ... for encoding/decoding

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:48: #ifdef SK_BUILD_FOR_ANDROID
I think this comment could be improved. Why 30 vs. 5.

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:51: */
left justify the # lines?

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:75: 
This seems like it could use a comment.

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:101: }
virtual

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:102: ~SkJPEGImageDecoder() {
remove "if (fImageIndex)"

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:120: int fImageHeight;
INHERITED

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:178: int row_count =
jpeg_read_tile_scanline(cinfo, index, &rowptr);
1 !=

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:349: bm->unlockPixels();
const?

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:359: bm->setIsOpaque(true);
What if the bitmap is being reused?

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:500: int status = jpeg_read_header(cinfo,
true);
constant on LHS

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:521: status = jpeg_read_header(cinfo,
true);
const on LHS

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:539: 
remove "if (fImageIndex) {" and matching "}".

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:548: bool
SkJPEGImageDecoder::onDecodeRegion(SkBitmap* bm, const SkIRect& region) {
NULL on LHS

https://codereview.chromium.org/12438025/diff/1/src/images/SkJpegUtility.cpp
File src/images/SkJpegUtility.cpp (right):

https://codereview.chromium.org/12438025/diff/1/src/images/SkJpegUtility.cpp#...
src/images/SkJpegUtility.cpp:40: src->bytes_in_buffer = 0;
true?

[djsollen, 2013-03-19 15:38:25]

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
File src/images/SkImageDecoder_libjpeg.cpp (right):

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:31: #endif
On 2013/03/18 20:11:31, djsollen wrote:
> these lines need to be commented out. I also need to document that they are
> useful to enable the same code path that the android framework uses for image
> decoding.

Done.

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:32: 
On 2013/03/19 13:15:21, robertphillips wrote:
> These enable ... for encoding/decoding

Done.

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:48: #ifdef SK_BUILD_FOR_ANDROID
On 2013/03/19 13:15:21, robertphillips wrote:
> I think this comment could be improved. Why 30 vs. 5.

Done.

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:51: */
On 2013/03/19 13:15:21, robertphillips wrote:
> left justify the # lines?

Done.

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:75: 
On 2013/03/19 13:15:21, robertphillips wrote:
> This seems like it could use a comment.

Done.

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:101: }
On 2013/03/19 13:15:21, robertphillips wrote:
> virtual

Done.

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:102: ~SkJPEGImageDecoder() {
On 2013/03/19 13:15:21, robertphillips wrote:
> remove "if (fImageIndex)"

Done.

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:120: int fImageHeight;
On 2013/03/19 13:15:21, robertphillips wrote:
> INHERITED

Done.

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:178: int row_count =
jpeg_read_tile_scanline(cinfo, index, &rowptr);
On 2013/03/19 13:15:21, robertphillips wrote:
> 1 !=

Done.

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:349: bm->unlockPixels();
On 2013/03/19 13:15:21, robertphillips wrote:
> const?

Done.

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:359: bm->setIsOpaque(true);
On 2013/03/19 13:15:21, robertphillips wrote:
> What if the bitmap is being reused?

Your right it needs to stay here.  I updated the if statements to make that a
little clearer.

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:385: while (cinfo.output_scanline <
cinfo.output_height) {
On 2013/03/18 20:11:31, djsollen wrote:
> remove space

Done.

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:500: int status = jpeg_read_header(cinfo,
true);
On 2013/03/19 13:15:21, robertphillips wrote:
> constant on LHS

Done.

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:521: status = jpeg_read_header(cinfo,
true);
On 2013/03/19 13:15:21, robertphillips wrote:
> const on LHS

Done.

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:539: 
On 2013/03/19 13:15:21, robertphillips wrote:
> remove "if (fImageIndex) {" and matching "}".

Done.

https://codereview.chromium.org/12438025/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libjpeg.cpp:548: bool
SkJPEGImageDecoder::onDecodeRegion(SkBitmap* bm, const SkIRect& region) {
On 2013/03/19 13:15:21, robertphillips wrote:
> NULL on LHS

Done.

https://codereview.chromium.org/12438025/diff/1/src/images/SkJpegUtility.cpp
File src/images/SkJpegUtility.cpp (right):

https://codereview.chromium.org/12438025/diff/1/src/images/SkJpegUtility.cpp#...
src/images/SkJpegUtility.cpp:40: src->bytes_in_buffer = 0;
On 2013/03/19 13:15:21, robertphillips wrote:
> true?

Done.

[robertphillips, 2013-03-19 15:43:30]

lgtm

[djsollen, 2013-03-20 17:45:42]

Committed patchset #2 manually as r8267 (presubmit successful).