Implementing filling for SkBmpCodec

Implementing filling for SkBmpCodec

The bmp codec currently returns kIncompleteInput
when the stream is truncated, which we treat as a
partial success.  However, we neglect the fill the
remaining pixels in the image, leaving these
uninitialized.

This CL addresses this problem by initializing the
remaining pixels in the image to default values.

BUG=skia:3257
Committed: https://skia.googlesource.com/skia/+/3c309db75bb8c4c2b58724a0e2f6f3b387ca842c

[msarett, 2015-04-10 17:55:48]

Patchset #1 (id:1) has been deleted

[msarett, 2015-04-10 17:56:52]

Patchset #1 (id:20001) has been deleted

[msarett, 2015-04-10 17:58:53]

msarett@google.com changed reviewers:
+ scroggo@google.com

[scroggo, 2015-04-10 18:12:43]

Is there a test? (Or an image to be added to DM?)

https://codereview.chromium.org/1075243003/diff/40001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1075243003/diff/40001/src/codec/SkSwizzler.cp...
src/codec/SkSwizzler.cpp:440: const size_t remainingBytes = totalBytes - y *
dstRowBytes;
It seems like if (bottomUp), the remaining bytes will be totalBytes -
(dstInfo.height() * y) * dstRowBytes, right?

https://codereview.chromium.org/1075243003/diff/40001/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1075243003/diff/40001/src/codec/SkSwizzler.h#...
src/codec/SkSwizzler.h:163: uint32_t colorOrIndex, const SkPMColor* colorTable,
bool bottomUp = false);
I think it would be better to use RowOrder (which may need to move somewhere
common).

[msarett, 2015-04-10 19:47:13]

Patchset #3 (id:80001) has been deleted

[msarett, 2015-04-10 19:47:18]

Patchset #2 (id:60001) has been deleted

[msarett, 2015-04-10 20:49:37]

Patchset #3 (id:120001) has been deleted

[msarett, 2015-04-10 20:49:45]

Patchset #2 (id:100001) has been deleted

[msarett, 2015-04-10 20:52:56]

Fill() now takes a dstStartRow and numRows
as parameters.

SwizzlerTest has been updated and test images
were uploaded to gsutil.

https://codereview.chromium.org/1075243003/diff/40001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1075243003/diff/40001/src/codec/SkSwizzler.cp...
src/codec/SkSwizzler.cpp:440: const size_t remainingBytes = totalBytes - y *
dstRowBytes;
On 2015/04/10 18:12:43, scroggo wrote:
> It seems like if (bottomUp), the remaining bytes will be totalBytes -
> (dstInfo.height() * y) * dstRowBytes, right?

This is in fact correct as is, but quite confusing.

A new design will take care of this.  We will now pass in the dstStartRow and
the number of rows to fill.

https://codereview.chromium.org/1075243003/diff/40001/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1075243003/diff/40001/src/codec/SkSwizzler.h#...
src/codec/SkSwizzler.h:163: uint32_t colorOrIndex, const SkPMColor* colorTable,
bool bottomUp = false);
On 2015/04/10 18:12:43, scroggo wrote:
> I think it would be better to use RowOrder (which may need to move somewhere
> common).

A new design will take care of this.  We will now pass in the dstStartRow and
the number of rows to fill.

[scroggo, 2015-04-10 21:08:32]

lgtm

https://codereview.chromium.org/1075243003/diff/140001/src/codec/SkCodec_libb...
File src/codec/SkCodec_libbmp.cpp (right):

https://codereview.chromium.org/1075243003/diff/140001/src/codec/SkCodec_libb...
src/codec/SkCodec_libbmp.cpp:1135: void* dstStart = (kTopDown_RowOrder ==
fRowOrder) ?
nit: It seems like you could use a macro/static function to share code here?

https://codereview.chromium.org/1075243003/diff/140001/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1075243003/diff/140001/src/codec/SkSwizzler.h...
src/codec/SkSwizzler.h:196: void* getDstRow() {
Maybe this should be getNextRow? Not sure though, let's leave it getDstRow for
now, since it matches everything else.

[msarett, 2015-04-10 21:22:32]

https://codereview.chromium.org/1075243003/diff/140001/src/codec/SkCodec_libb...
File src/codec/SkCodec_libbmp.cpp (right):

https://codereview.chromium.org/1075243003/diff/140001/src/codec/SkCodec_libb...
src/codec/SkCodec_libbmp.cpp:1135: void* dstStart = (kTopDown_RowOrder ==
fRowOrder) ?
On 2015/04/10 21:08:32, scroggo wrote:
> nit: It seems like you could use a macro/static function to share code here?

Done.

https://codereview.chromium.org/1075243003/diff/140001/src/codec/SkSwizzler.h
File src/codec/SkSwizzler.h (right):

https://codereview.chromium.org/1075243003/diff/140001/src/codec/SkSwizzler.h...
src/codec/SkSwizzler.h:196: void* getDstRow() {
On 2015/04/10 21:08:32, scroggo wrote:
> Maybe this should be getNextRow? Not sure though, let's leave it getDstRow for
> now, since it matches everything else.

Acknowledged.

[msarett, 2015-04-10 21:22:37]

The CQ bit was checked by msarett@google.com

[msarett, 2015-04-10 21:22:38]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com
Link to the patchset: https://codereview.chromium.org/1075243003/#ps160001
(title: "Sharing code in static helper")

[commit-bot: I haz the power, 2015-04-10 21:23:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1075243003/160001

[commit-bot: I haz the power, 2015-04-10 21:36:56]

Committed patchset #3 (id:160001) as
https://skia.googlesource.com/skia/+/3c309db75bb8c4c2b58724a0e2f6f3b387ca842c