Upstream Android modifications to the image encoders/decoders.

Upstream Android modifications to the image encoders/decoders.

This CL does not update the libjpeg as that change is large enough
to warrant its own CL.


Committed: http://code.google.com/p/skia/source/detail?r=8155

[djsollen, 2013-03-11 17:47:23]

https://codereview.chromium.org/12604006/diff/1/DEPS
File DEPS (right):

https://codereview.chromium.org/12604006/diff/1/DEPS#newcode18
DEPS:18: "third_party/externals/libwebp" :
"http://src.chromium.org/svn/trunk/src/third_party/libwebp@186718",
need to add this to svn::internals as well.

https://codereview.chromium.org/12604006/diff/1/include/images/SkBitmapRegion...
File include/images/SkBitmapRegionDecoder.h (right):

https://codereview.chromium.org/12604006/diff/1/include/images/SkBitmapRegion...
include/images/SkBitmapRegionDecoder.h:31: virtual bool decodeRegion(SkBitmap*
bitmap, SkIRect rect,
const SkIRect&

https://codereview.chromium.org/12604006/diff/1/src/images/SkBitmapRegionDeco...
File src/images/SkBitmapRegionDecoder.cpp (right):

https://codereview.chromium.org/12604006/diff/1/src/images/SkBitmapRegionDeco...
src/images/SkBitmapRegionDecoder.cpp:3: bool
SkBitmapRegionDecoder::decodeRegion(SkBitmap* bitmap, SkIRect rect,
const SkIRect&

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

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libwebp.cpp:315: SkIRect region) {
const SkIRect&

[robertphillips, 2013-03-11 18:25:55]

Made it halfway through

https://codereview.chromium.org/12604006/diff/1/include/images/SkBitmapRegion...
File include/images/SkBitmapRegionDecoder.h (right):

https://codereview.chromium.org/12604006/diff/1/include/images/SkBitmapRegion...
include/images/SkBitmapRegionDecoder.h:1: /*
Is this the right header?

https://codereview.chromium.org/12604006/diff/1/include/images/SkBitmapRegion...
include/images/SkBitmapRegionDecoder.h:12: #include "SkBitmap.h"
I think we only need "class SkIRect;" here.

https://codereview.chromium.org/12604006/diff/1/include/images/SkBitmapRegion...
include/images/SkBitmapRegionDecoder.h:16: 
comment?

https://codereview.chromium.org/12604006/diff/1/include/images/SkBitmapRegion...
include/images/SkBitmapRegionDecoder.h:19: SkBitmapRegionDecoder(SkImageDecoder
*decoder, SkStream *stream,
line up next line?

https://codereview.chromium.org/12604006/diff/1/include/images/SkBitmapRegion...
include/images/SkBitmapRegionDecoder.h:33: 
consts?

https://codereview.chromium.org/12604006/diff/1/include/images/SkImageDecoder.h
File include/images/SkImageDecoder.h (right):

https://codereview.chromium.org/12604006/diff/1/include/images/SkImageDecoder...
include/images/SkImageDecoder.h:47: */
Can we use SK_ARRAY_COUNT here or a kNumFormats constant based off of
kLastKnownFormat? I would like some bullet proofing against this being left
behind if a new format is added.

https://codereview.chromium.org/12604006/diff/1/include/images/SkImageDecoder...
include/images/SkImageDecoder.h:55: */
this->

https://codereview.chromium.org/12604006/diff/1/include/images/SkImageDecoder...
include/images/SkImageDecoder.h:365: * @param sampleSize the sample size that
src is sampled from the original Bitmap.
Btimap -> bitmap

https://codereview.chromium.org/12604006/diff/1/src/images/SkBitmapRegionDeco...
File src/images/SkBitmapRegionDecoder.cpp (right):

https://codereview.chromium.org/12604006/diff/1/src/images/SkBitmapRegionDeco...
src/images/SkBitmapRegionDecoder.cpp:1: #include "SkBitmapRegionDecoder.h"
Missing header

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

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder.cpp...
src/images/SkImageDecoder.cpp:155: SkAutoLockPixels alp(*bm);
NULL goes on lhs

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder.cpp...
src/images/SkImageDecoder.cpp:159: }
Move tmp down here?

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder.cpp...
src/images/SkImageDecoder.cpp:173: 
just "return this->onDecodeRegion(bm, rect));"

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

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libico.cpp:240: }
Is this expensive enough to make duplicating the code worthwhile?

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

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libpng.cpp:25: 
comment?
Can this be nested in SkPNGImageDecoder?

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libpng.cpp:32: ~SkPNGImageIndex() {
NULL !=

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libpng.cpp:55: #if defined(SK_BUILD_FOR_ANDROID) &&
!defined(SK_BUILD_FOR_ANDROID_NDK)
OVERRIDES?

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libpng.cpp:56: virtual bool onBuildTileIndex(SkStream
*stream, int *width, int *height);
_const_ SkIRect& region?

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libpng.cpp:69: SkPNGImageIndex* fImageIndex;
INHERITED?

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libpng.cpp:257: 
!onDecodeInit?

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libpng.cpp:277: 
!getBitmapConfig?

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libpng.cpp:290: if (reuseBitmap &&
(sampler.scaledWidth() != decodedBitmap->width() ||
intend the next line some more?

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libpng.cpp:320: SkBitmap::kIndex8_Config == config ?
move this next line up to the end of the prior line?

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libpng.cpp:333: /* Turn on interlace handling. 
REQUIRED if you are not using
indent these next lines 1 char?

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libpng.cpp:533: 
just "return this->chooseFromOneChoise ..."?

[djsollen, 2013-03-11 19:43:23]

https://codereview.chromium.org/12604006/diff/1/include/images/SkBitmapRegion...
File include/images/SkBitmapRegionDecoder.h (right):

https://codereview.chromium.org/12604006/diff/1/include/images/SkBitmapRegion...
include/images/SkBitmapRegionDecoder.h:1: /*
On 2013/03/11 18:25:55, robertphillips wrote:
> Is this the right header?

Yes, as it is was originally release as part of AOSP.

https://codereview.chromium.org/12604006/diff/1/include/images/SkBitmapRegion...
include/images/SkBitmapRegionDecoder.h:12: #include "SkBitmap.h"
On 2013/03/11 18:25:55, robertphillips wrote:
> I think we only need "class SkIRect;" here.

Done.

https://codereview.chromium.org/12604006/diff/1/include/images/SkBitmapRegion...
include/images/SkBitmapRegionDecoder.h:33: 
On 2013/03/11 18:25:55, robertphillips wrote:
> consts?

Done.

https://codereview.chromium.org/12604006/diff/1/include/images/SkImageDecoder.h
File include/images/SkImageDecoder.h (right):

https://codereview.chromium.org/12604006/diff/1/include/images/SkImageDecoder...
include/images/SkImageDecoder.h:47: */
On 2013/03/11 18:25:55, robertphillips wrote:
> Can we use SK_ARRAY_COUNT here or a kNumFormats constant based off of
> kLastKnownFormat? I would like some bullet proofing against this being left
> behind if a new format is added.

Done.

https://codereview.chromium.org/12604006/diff/1/include/images/SkImageDecoder...
include/images/SkImageDecoder.h:365: * @param sampleSize the sample size that
src is sampled from the original Bitmap.
On 2013/03/11 18:25:55, robertphillips wrote:
> Btimap -> bitmap

Done.

https://codereview.chromium.org/12604006/diff/1/src/images/SkBitmapRegionDeco...
File src/images/SkBitmapRegionDecoder.cpp (right):

https://codereview.chromium.org/12604006/diff/1/src/images/SkBitmapRegionDeco...
src/images/SkBitmapRegionDecoder.cpp:1: #include "SkBitmapRegionDecoder.h"
On 2013/03/11 18:25:55, robertphillips wrote:
> Missing header

Done.

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

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder.cpp...
src/images/SkImageDecoder.cpp:155: SkAutoLockPixels alp(*bm);
On 2013/03/11 18:25:55, robertphillips wrote:
> NULL goes on lhs

Done.

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder.cpp...
src/images/SkImageDecoder.cpp:159: }
On 2013/03/11 18:25:55, robertphillips wrote:
> Move tmp down here?

Done.

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder.cpp...
src/images/SkImageDecoder.cpp:173: 
On 2013/03/11 18:25:55, robertphillips wrote:
> just "return this->onDecodeRegion(bm, rect));"

Done.

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

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libico.cpp:240: }
I supposed this was to work around some android bug so I didn't want to change
the code until I could verify. I believe the issue is that they don't want to
update the bitmap and then return false because the bitmap reuse is not
supported.

On 2013/03/11 18:25:55, robertphillips wrote:
> Is this expensive enough to make duplicating the code worthwhile?

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

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libpng.cpp:32: ~SkPNGImageIndex() {
On 2013/03/11 18:25:55, robertphillips wrote:
> NULL !=

Done.

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libpng.cpp:55: #if defined(SK_BUILD_FOR_ANDROID) &&
!defined(SK_BUILD_FOR_ANDROID_NDK)
On 2013/03/11 18:25:55, robertphillips wrote:
> OVERRIDES?

Done.

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libpng.cpp:56: virtual bool onBuildTileIndex(SkStream
*stream, int *width, int *height);
On 2013/03/11 18:25:55, robertphillips wrote:
> _const_ SkIRect& region?

Done.

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libpng.cpp:257: 
On 2013/03/11 18:25:55, robertphillips wrote:
> !onDecodeInit?

Done.

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libpng.cpp:277: 
On 2013/03/11 18:25:55, robertphillips wrote:
> !getBitmapConfig?

Done.

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libpng.cpp:290: if (reuseBitmap &&
(sampler.scaledWidth() != decodedBitmap->width() ||
On 2013/03/11 18:25:55, robertphillips wrote:
> intend the next line some more?

Done.

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libpng.cpp:320: SkBitmap::kIndex8_Config == config ?
On 2013/03/11 18:25:55, robertphillips wrote:
> move this next line up to the end of the prior line?

Done.

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libpng.cpp:333: /* Turn on interlace handling. 
REQUIRED if you are not using
On 2013/03/11 18:25:55, robertphillips wrote:
> indent these next lines 1 char?

Done.

https://codereview.chromium.org/12604006/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libpng.cpp:533: 
On 2013/03/11 18:25:55, robertphillips wrote:
> just "return this->chooseFromOneChoise ..."?

Done.

[robertphillips, 2013-03-11 20:10:16]

Here's some more.

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
File src/images/SkImageDecoder_libwebp.cpp (right):

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:61: // Parse headers of RIFF container,
and check for valid Webp (VP8) content.
// Return false if file ...

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:62: static bool
webp_parse_header(SkStream* stream, int* width, int* height,
move to prior line?

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:67: const uint32_t read_bytes =
(contentSize < WEBP_VP8_HEADER_SIZE) ?
tab the next line over?

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:74: VP8StatusCode status =
WebPGetFeatures(buffer, read_bytes, &features);
reverse order to have constant on lhs?

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:98: public:
override?

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:102: 
ctor to init inputStream?

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:109: bool setDecodeConfig(SkBitmap*
decodedBitmap, int width, int height);
f prefix on all variables?

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:113: int hasAlpha;
inherited?

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:117: 
wrap this guy in "#ifdef TIME_DECODE"?

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:119: 
Do we not have this guy somewhere else? It seems like it would be a common use
case.

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:123: fLabel(label) {
NULL ==

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:166: // and decodes this block to
appropriate color-space as per config object.
_const_ WebPDecoderConfig? It actually looks like this should be passed as a
pointer.

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:168: WebPIDecoder* idec =
WebPIDecode(NULL, 0, &config);
NULL on lhs

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:176: const uint32_t read_buffer_size =
(contentSize < WEBP_IDECODE_BUFFER_SZ) ?
indent?

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:179: unsigned char* input =
(uint8_t*)srcStorage.get();
NULL on left?

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:185: 
bytesRemaining

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:187: while (bytes_remaining > 0) {
bytesToRead

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:191: 
bytesRead

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:192: const size_t bytes_read =
stream->read(input, bytes_to_read);
0 on LHS

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:197: VP8StatusCode status =
WebPIAppend(idec, input, bytes_read);
constants on LHS

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:218: WEBP_CSP_MODE mode =
webp_decode_mode(decodedBitmap, hasAlpha);
constant on LHS

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:233: if (width != decodedBitmap->width()
||
move to prior line

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:247: if (!webp_get_config_resize(
indentation

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:306: 
is_config_compatible

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:379: cropBitmap(decodedBitmap, bitmap,
sampleSize, region.x(), region.y(),
add 1 space

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:501: 
should this return a bool?

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:509: protected:
override

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:510: virtual bool onEncode(SkWStream*
stream, const SkBitmap& bm, int quality);
private:
 inherited

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:540: const uint8_t* src =
(uint8_t*)bm.getPixels();
rgbStride

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:544: // to RGB color space.
SkNew

[djsollen, 2013-03-12 15:52:31]

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
File src/images/SkImageDecoder_libwebp.cpp (right):

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:62: static bool
webp_parse_header(SkStream* stream, int* width, int* height,
On 2013/03/11 20:10:16, robertphillips wrote:
> move to prior line?

Done.

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:67: const uint32_t read_bytes =
(contentSize < WEBP_VP8_HEADER_SIZE) ?
On 2013/03/11 20:10:16, robertphillips wrote:
> tab the next line over?

Done.

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:74: VP8StatusCode status =
WebPGetFeatures(buffer, read_bytes, &features);
On 2013/03/11 20:10:16, robertphillips wrote:
> reverse order to have constant on lhs?

Done.

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:98: public:
On 2013/03/11 20:10:16, robertphillips wrote:
> override?

Done.

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:102: 
On 2013/03/11 20:10:16, robertphillips wrote:
> ctor to init inputStream?

Done.

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:109: bool setDecodeConfig(SkBitmap*
decodedBitmap, int width, int height);
On 2013/03/11 20:10:16, robertphillips wrote:
> f prefix on all variables?

Done.

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:117: 
On 2013/03/11 20:10:16, robertphillips wrote:
> wrap this guy in "#ifdef TIME_DECODE"?

Done.

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:123: fLabel(label) {
On 2013/03/11 20:10:16, robertphillips wrote:
> NULL == 

Done.

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:168: WebPIDecoder* idec =
WebPIDecode(NULL, 0, &config);
On 2013/03/11 20:10:16, robertphillips wrote:
> NULL on lhs

Done.

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:176: const uint32_t read_buffer_size =
(contentSize < WEBP_IDECODE_BUFFER_SZ) ?
On 2013/03/11 20:10:16, robertphillips wrote:
> indent?

Done.

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:179: unsigned char* input =
(uint8_t*)srcStorage.get();
On 2013/03/11 20:10:16, robertphillips wrote:
> NULL on left?

Done.

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:185: 
On 2013/03/11 20:10:16, robertphillips wrote:
> bytesRemaining

Done.

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:197: VP8StatusCode status =
WebPIAppend(idec, input, bytes_read);
On 2013/03/11 20:10:16, robertphillips wrote:
> constants on LHS

Done.

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:218: WEBP_CSP_MODE mode =
webp_decode_mode(decodedBitmap, hasAlpha);
On 2013/03/11 20:10:16, robertphillips wrote:
> constant on LHS

Done.

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:233: if (width != decodedBitmap->width()
||
On 2013/03/11 20:10:16, robertphillips wrote:
> move to prior line

Done.

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:247: if (!webp_get_config_resize(
On 2013/03/11 20:10:16, robertphillips wrote:
> indentation

Done.

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:306: 
On 2013/03/11 20:10:16, robertphillips wrote:
> is_config_compatible

Done.

https://codereview.chromium.org/12604006/diff/13001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:379: cropBitmap(decodedBitmap, bitmap,
sampleSize, region.x(), region.y(),
On 2013/03/11 20:10:16, robertphillips wrote:
> add 1 space

Done.

[robertphillips, 2013-03-13 13:41:29]

first batch

https://codereview.chromium.org/12604006/diff/25001/include/images/SkBitmapRe...
File include/images/SkBitmapRegionDecoder.h (right):

https://codereview.chromium.org/12604006/diff/25001/include/images/SkBitmapRe...
include/images/SkBitmapRegionDecoder.h:40: bool decodeRegion(SkBitmap* bitmap,
const SkIRect& rect,
remove extras spaces?

https://codereview.chromium.org/12604006/diff/25001/include/images/SkImageDec...
File include/images/SkImageDecoder.h (right):

https://codereview.chromium.org/12604006/diff/25001/include/images/SkImageDec...
include/images/SkImageDecoder.h:47: */
So you don't think replacing the '8' with "kLastKnownFormat+1" would be
worthwhile?

https://codereview.chromium.org/12604006/diff/25001/include/images/SkImageDec...
include/images/SkImageDecoder.h:57: SkASSERT(SK_ARRAY_COUNT(kFormatName) ==
kLastKnownFormat);
this->getFormat()

https://codereview.chromium.org/12604006/diff/25001/include/images/SkImageDec...
include/images/SkImageDecoder.h:348: 
Isn't the pattern to have a non-virtual decodeRegion/buildTileIndex and then
virtual on* methods? Do we need virtuals in all four locations?

https://codereview.chromium.org/12604006/diff/25001/include/images/SkImageDec...
include/images/SkImageDecoder.h:368: * @param sampleSize the sample size that
src is sampled from the original bitmap.
make order in the comment match the order in the function call? bitimap ->
bitmap

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:68: 
Isn't the order supposed to be member variables then functions?

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:344: if (SkBitmap::kIndex8_Config == config
&& 1 == sampleSize) {
preincrement - ignore if you desire

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:376: uint8_t* base =
(uint8_t*)storage.get();
rb -> rowBytes?

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:430: bool
SkPNGImageDecoder::getBitmapConfig(png_structp png_ptr, png_infop info_ptr,
indent the following two lines?

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:435: png_get_IHDR(png_ptr, info_ptr,
&origWidth, &origHeight, &bit_depth,
indent next line?

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:447: if (pos_le(info_ptr->sig_bit.red,
SK_R16_BITS) &&
fix indent?

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:537: SkColorTable **colorTablep) {
numPalette, numTrans

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:600: 
!onDecodeInit?

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:609: 
bitDepth, colorType, interlaceType

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:619: 
rm this's on next two lines?

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:628: bool
SkPNGImageDecoder::onDecodeRegion(SkBitmap* bm, const SkIRect& region) {
removes this's on next two lines?

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:635: png_uint_32 origWidth, origHeight;
bitDepth, colorType, interlaceType

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:652: 
!getBitmapConfig?

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:660: 
do we actually need the decodedBitmap pointer? Why not "SkAutoTDelete<SkBitmap>
adb(new SkBitmap);". Also SkNEW?

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:676: decodePalette(png_ptr, info_ptr,
&hasAlpha,
one line?

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:688: (h == decodedBitmap->height()) &&
0 - ?

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:726: png_configure_decoder(png_ptr,
&actualTop, i);
pre-increment?

[robertphillips, 2013-03-13 13:55:14]

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
File src/images/SkImageDecoder_libwebp.cpp (right):

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:57: 
k prefix? g prefix?

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:117: private:
members first?

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:121: int fOrigHeight;
should fHasAlpha be a bool?

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:160: 
bool hasAlpha?

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:166: // factor (alpha).
constant on LHS

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:189: const uint32_t readBufferSize =
(contentSize < WEBP_IDECODE_BUFFER_SZ) ?
remove spaces?

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:218: 
return bytesRemaining <= 0;?

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:254: SkBitmap* decodedBitmap,
bool hasAlpha?

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:403: }
remove this->?

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:410: if
(SkImageDecoder::kDecodeBounds_Mode == mode) {
just "return setDecodeConfig"?

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:510: 
Should this return a bool?

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:527: const SkBitmap::Config config =
bm.getConfig();
scanlineImport?

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libwebp.cpp:538: 
webpConfig?

[robertphillips, 2013-03-13 13:55:44]

l okay tm

[robertphillips, 2013-03-13 15:18:24]

I think we should also create an issue to add more testing for image
decoding/encoding to Skia. This should also exercise all the corner cases
(bitmap reuse, block decoding, bit depths, etc.)

[djsollen, 2013-03-13 16:02:48]

https://codereview.chromium.org/12604006/diff/25001/include/images/SkBitmapRe...
File include/images/SkBitmapRegionDecoder.h (right):

https://codereview.chromium.org/12604006/diff/25001/include/images/SkBitmapRe...
include/images/SkBitmapRegionDecoder.h:40: bool decodeRegion(SkBitmap* bitmap,
const SkIRect& rect,
On 2013/03/13 13:41:29, robertphillips wrote:
> remove extras spaces?

Done.

https://codereview.chromium.org/12604006/diff/25001/include/images/SkImageDec...
File include/images/SkImageDecoder.h (right):

https://codereview.chromium.org/12604006/diff/25001/include/images/SkImageDec...
include/images/SkImageDecoder.h:47: */
On 2013/03/13 13:41:29, robertphillips wrote:
> So you don't think replacing the '8' with "kLastKnownFormat+1" would be
> worthwhile?

Done.

https://codereview.chromium.org/12604006/diff/25001/include/images/SkImageDec...
include/images/SkImageDecoder.h:57: SkASSERT(SK_ARRAY_COUNT(kFormatName) ==
kLastKnownFormat);
On 2013/03/13 13:41:29, robertphillips wrote:
> this->getFormat()

Done.

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:68: 
On 2013/03/13 13:41:29, robertphillips wrote:
> Isn't the order supposed to be member variables then functions?

That is opposite of android, but you are correct.

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:376: uint8_t* base =
(uint8_t*)storage.get();
On 2013/03/13 13:41:29, robertphillips wrote:
> rb -> rowBytes?

Done.

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:430: bool
SkPNGImageDecoder::getBitmapConfig(png_structp png_ptr, png_infop info_ptr,
On 2013/03/13 13:41:29, robertphillips wrote:
> indent the following two lines?

Done.

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:435: png_get_IHDR(png_ptr, info_ptr,
&origWidth, &origHeight, &bit_depth,
On 2013/03/13 13:41:29, robertphillips wrote:
> indent next line?

Done.

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:447: if (pos_le(info_ptr->sig_bit.red,
SK_R16_BITS) &&
On 2013/03/13 13:41:29, robertphillips wrote:
> fix indent?

Done.

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:537: SkColorTable **colorTablep) {
On 2013/03/13 13:41:29, robertphillips wrote:
> numPalette, numTrans

Done.

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:600: 
On 2013/03/13 13:41:29, robertphillips wrote:
> !onDecodeInit?

Done.

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:609: 
On 2013/03/13 13:41:29, robertphillips wrote:
> bitDepth, colorType, interlaceType

Done.

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:619: 
On 2013/03/13 13:41:29, robertphillips wrote:
> rm this's on next two lines?

Done.

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:628: bool
SkPNGImageDecoder::onDecodeRegion(SkBitmap* bm, const SkIRect& region) {
On 2013/03/13 13:41:29, robertphillips wrote:
> removes this's on next two lines?

Done.

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:635: png_uint_32 origWidth, origHeight;
On 2013/03/13 13:41:29, robertphillips wrote:
> bitDepth, colorType, interlaceType

Done.

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:652: 
On 2013/03/13 13:41:29, robertphillips wrote:
> !getBitmapConfig?

Done.

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:660: 
On 2013/03/13 13:41:29, robertphillips wrote:
> do we actually need the decodedBitmap pointer? Why not
"SkAutoTDelete<SkBitmap>
> adb(new SkBitmap);". Also SkNEW?

Done.

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:676: decodePalette(png_ptr, info_ptr,
&hasAlpha,
On 2013/03/13 13:41:29, robertphillips wrote:
> one line?

Done.

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:676: decodePalette(png_ptr, info_ptr,
&hasAlpha,
On 2013/03/13 13:41:29, robertphillips wrote:
> one line?

Done.

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:688: (h == decodedBitmap->height()) &&
On 2013/03/13 13:41:29, robertphillips wrote:
> 0 - ?

Done.

[scroggo, 2013-03-13 16:04:14]

I think Robert did a more thorough read through, but I agree with his comments
in general and added a few of my own. Did we decide to take their code as is and
iterate on it?

https://codereview.chromium.org/12604006/diff/25001/include/images/SkBitmapRe...
File include/images/SkBitmapRegionDecoder.h (right):

https://codereview.chromium.org/12604006/diff/25001/include/images/SkBitmapRe...
include/images/SkBitmapRegionDecoder.h:26: class SkBitmapRegionDecoder {
I guess we decided not to change Region to Rect so it's not confused with
SkRegion?

https://codereview.chromium.org/12604006/diff/25001/include/images/SkImageDec...
File include/images/SkImageDecoder.h (right):

https://codereview.chromium.org/12604006/diff/25001/include/images/SkImageDec...
include/images/SkImageDecoder.h:48: static const char *kFormatName[8];
Why does this have a 'k' at the front? Isn't that reserved for enums? Also, does
it need to be public?

https://codereview.chromium.org/12604006/diff/25001/include/images/SkImageDec...
include/images/SkImageDecoder.h:209: bool decode(SkStream*, SkBitmap* bitmap,
SkBitmap::Config pref, Mode, bool reuseBitmap = false);
Some thoughts on reuseBitmap:
It appears to be only meaningful if Mode == kDecodePixels, making for an ugly
API.
I dislike that passing true to reuseBitmap can turn into a mysterious failure.
At the very least,  we should document the behavior. Also, it is not clear to me
why some decoders do not allow reuse of a bitmap.
It almost seems like it could be replaced with a custom allocator which just
reuses the existing pixels. I guess the drawback to that approach is that the
subclass cannot deny reuse. I suppose some caller attempts to reuse when it's
not allowed?

https://codereview.chromium.org/12604006/diff/25001/include/images/SkImageDec...
include/images/SkImageDecoder.h:230: virtual bool decodeRegion(SkBitmap* bitmap,
const SkIRect& rect,
decodeRect?

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
File src/images/SkImageDecoder.cpp (right):

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder.cpp:185: void SkImageDecoder::cropBitmap(SkBitmap
*dst, SkBitmap *src, int sampleSize,
Like we discussed in the meeting the other day, this seems like an odd function
to have on SkImageDecoder (it does not decode an image or affect the decoding of
an image). It could take an Allocator and be a totally static function.

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
File src/images/SkImageDecoder_libpng.cpp (right):

https://codereview.chromium.org/12604006/diff/25001/src/images/SkImageDecoder...
src/images/SkImageDecoder_libpng.cpp:623: this->fImageIndex = new
SkPNGImageIndex(png_ptr, info_ptr);
SkNEW_ARGS

[djsollen, 2013-03-13 17:35:09]

https://codereview.chromium.org/12604006/diff/25001/include/images/SkBitmapRe...
File include/images/SkBitmapRegionDecoder.h (right):

https://codereview.chromium.org/12604006/diff/25001/include/images/SkBitmapRe...
include/images/SkBitmapRegionDecoder.h:26: class SkBitmapRegionDecoder {
I don't want to change class names in this CL.

https://codereview.chromium.org/12604006/diff/25001/include/images/SkImageDec...
File include/images/SkImageDecoder.h (right):

https://codereview.chromium.org/12604006/diff/25001/include/images/SkImageDec...
include/images/SkImageDecoder.h:48: static const char *kFormatName[8];
On 2013/03/13 16:04:14, scroggo wrote:
> Why does this have a 'k' at the front? Isn't that reserved for enums? Also,
does
> it need to be public?

Done.

https://codereview.chromium.org/12604006/diff/25001/include/images/SkImageDec...
include/images/SkImageDecoder.h:209: bool decode(SkStream*, SkBitmap* bitmap,
SkBitmap::Config pref, Mode, bool reuseBitmap = false);
The lack of support is not a technical problem, just that the android authors
didn't spend the time to do all codecs.

FWIW, I don't think this is a great API either, but it is what is currently in
use.

https://codereview.chromium.org/12604006/diff/25001/include/images/SkImageDec...
include/images/SkImageDecoder.h:230: virtual bool decodeRegion(SkBitmap* bitmap,
const SkIRect& rect,
I don't want to change method signatures in this CL.

https://codereview.chromium.org/12604006/diff/25001/include/images/SkImageDec...
include/images/SkImageDecoder.h:348: 
On 2013/03/13 13:41:29, robertphillips wrote:
> Isn't the pattern to have a non-virtual decodeRegion/buildTileIndex and then
> virtual on* methods? Do we need virtuals in all four locations?

Done.

https://codereview.chromium.org/12604006/diff/25001/include/images/SkImageDec...
include/images/SkImageDecoder.h:368: * @param sampleSize the sample size that
src is sampled from the original bitmap.
On 2013/03/13 13:41:29, robertphillips wrote:
> make order in the comment match the order in the function call? bitimap ->
> bitmap

Done.

[reed1, 2013-03-13 20:05:19]

Likely we will want to iterate on the api, which will mean making changes to
android's callers. Will that be straight-forward? Should we hold-off on some of
those changes until after this lands, or try to do them now?

[reed1, 2013-03-13 20:05:45]

one small example: bitmapREGIONdecoder is a bad re-use of the word region.

[djsollen, 2013-03-14 14:03:34]

On 2013/03/13 20:05:45, reed1 wrote:
> one small example: bitmapREGIONdecoder is a bad re-use of the word region.

My goal was to stage the changes in the following way...

1) This CL to get unify most codecs with Android (internal changes, but keep
their existing API)
2) Repeat step #1 with libjpeg
3) Go back and cleanup the API to our liking

The benefit of doing #3 later is that if it doesn't mesh well with the Android
framework I can still do the merge (minus that patch) until I can update the
framework to fit our new API.

[reed1, 2013-03-14 14:33:17]

lgtm

[commit-bot: I haz the power, 2013-03-14 14:42:05]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/djsollen@google.com/12604006/38001

[commit-bot: I haz the power, 2013-03-14 14:42:23]

Change committed as 8155