SkCodec: add wbmp class

SkCodec: add wbmp class

Committed: https://skia.googlesource.com/skia/+/a096d7a6d03662073f4cd46f7db5fe2cf5495c36

[hal.canary, 2015-03-25 23:38:01]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-03-25 23:38:33]

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

[hal.canary, 2015-03-25 23:40:00]

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

[hal.canary, 2015-03-25 23:40:00]

Please Take a Look

[commit-bot: I haz the power, 2015-03-25 23:44:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-25 23:44:02]

This issue passed the CQ. To commit it, remove "COMMIT=false" from
the description and try again.

[msarett, 2015-03-26 12:37:36]

This looks good.

My comments are mostly questions and nits.

https://codereview.chromium.org/1006583005/diff/1/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1006583005/diff/1/dm/DMSrcSink.cpp#newcode97
dm/DMSrcSink.cpp:97: if (SkInstallDiscardablePixelRef(codec.detach(), &bitmap))
{
What does this do?  I'm just not sure I understand.

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.cpp
File src/codec/SkCodec_wbmp.cpp (right):

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.cpp#...
src/codec/SkCodec_wbmp.cpp:13: return stream->read(data, 1) == 1;
I was going to suggest using get_byte, get_short, and get_int from
SkCodecPriv.h, but it looks like you are reading a variable length quantity. 
Because of that, this probably makes sense.

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.cpp#...
src/codec/SkCodec_wbmp.cpp:78: class WbmpCodec : public SkCodec {
nit: We have been declaring the class in the header file and just using the .cpp
file for implementations.  It's probably just a matter of preference, but it
might be good to be consistent.  It might be worth getting Leon's opinion on
this.

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.cpp#...
src/codec/SkCodec_wbmp.cpp:82: ~WbmpCodec() {}
nit: Probably just a matter of preference, but we have been omitting the blank
destructor.

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.cpp#...
src/codec/SkCodec_wbmp.cpp:87: const SkImageInfo&, void*, size_t, const
Options&, SkPMColor[], int*);
nit: Should these arguments be indented twice and lined up?

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.cpp#...
src/codec/SkCodec_wbmp.cpp:100: return SkImageGenerator::kInvalidConversion;
We should also allow conversions to kN32_SkColorType.  This has been our default
even for images that are more suitable for kIndex8 because we can draw to kN32
and I think it's what android wants.  It shouldn't take too much code to support
this, just use SkSwizzler.

It's good to have kIndex8 supported as well because we are planning to add that
for other image types as well.

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.cpp#...
src/codec/SkCodec_wbmp.cpp:113: expand_bits_to_bytes(dst, src.get(),
size.width());
If you create a swizzler outside of the loop, after expanding bits to bytes, you
should be able to just call swizzler->next(src.get()) to produce each row of the
output in kN32_SkColorType.

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.h
File src/codec/SkCodec_wbmp.h (right):

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.h#ne...
src/codec/SkCodec_wbmp.h:14: namespace SkWbmpCodec {
What is the effect of declaring a namespace?  I'm just curious how this works. 
Is this in place of declaring the class?

[scroggo, 2015-03-26 14:37:56]

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

[scroggo, 2015-03-26 14:37:57]

https://codereview.chromium.org/1006583005/diff/1/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1006583005/diff/1/dm/DMSrcSink.cpp#newcode97
dm/DMSrcSink.cpp:97: if (SkInstallDiscardablePixelRef(codec.detach(), &bitmap))
{
On 2015/03/26 12:37:35, msarett wrote:
> What does this do?  I'm just not sure I understand.

SkInstallDiscardablePixelRef is declared in SkImageGenerator.h. It lets us
lazily call onGetPixels at draw time. In order to support this, we need to make
sure all of our codecs support rewinding, and can draw to the type they report.

This means we'll need to rethink when we report Unpremul (which we still cannot
draw, and will be slower in raster once we do support it) or a colortype we
don't support.

I've been trying to figure out how we can support testing index8. We currently
choose the color type above based on the destination canvas. This is one sneaky
way, so long as the image doesn't support 8888 or 565. In gold it will not
report that the image was decoded to index8 though.

Another way would be to make a different decision about what the colorType
should be.

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.cpp
File src/codec/SkCodec_wbmp.cpp (right):

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.cpp#...
src/codec/SkCodec_wbmp.cpp:51: static void expand_bits_to_bytes(uint8_t dst[],
const uint8_t src[], int bits) {
Matt, doesn't some of your code do something like this? Think it's possible to
share? I don't remember where yours is to compare though...

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.cpp#...
src/codec/SkCodec_wbmp.cpp:78: class WbmpCodec : public SkCodec {
On 2015/03/26 12:37:35, msarett wrote:
> nit: We have been declaring the class in the header file and just using the
.cpp
> file for implementations.  It's probably just a matter of preference, but it
> might be good to be consistent.  It might be worth getting Leon's opinion on
> this.

+1 for consistency.

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.cpp#...
src/codec/SkCodec_wbmp.cpp:87: const SkImageInfo&, void*, size_t, const
Options&, SkPMColor[], int*);
On 2015/03/26 12:37:35, msarett wrote:
> nit: Should these arguments be indented twice and lined up?

Yes.

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.cpp#...
src/codec/SkCodec_wbmp.cpp:100: return SkImageGenerator::kInvalidConversion;
On 2015/03/26 12:37:35, msarett wrote:
> We should also allow conversions to kN32_SkColorType.  This has been our
default
> even for images that are more suitable for kIndex8 because we can draw to kN32
> and I think it's what android wants.

That's correct. Android actually will copy an Index8 bitmap to N32 before
uploading to a texture.

> It shouldn't take too much code to support
> this, just use SkSwizzler.
> 
> It's good to have kIndex8 supported as well because we are planning to add
that
> for other image types as well.

Possibly... I wonder if Android will learn the value of Index8 (saving RAM) if
we stop using it.

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.h
File src/codec/SkCodec_wbmp.h (right):

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.h#ne...
src/codec/SkCodec_wbmp.h:14: namespace SkWbmpCodec {
On 2015/03/26 12:37:35, msarett wrote:
> What is the effect of declaring a namespace?  I'm just curious how this works.

> Is this in place of declaring the class?

Yes. It's mostly the same if all your methods are static. I believe there's some
internal disagreement on the Skia team as to the better way to do it.

[msarett, 2015-03-26 15:03:56]

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.cpp
File src/codec/SkCodec_wbmp.cpp (right):

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.cpp#...
src/codec/SkCodec_wbmp.cpp:51: static void expand_bits_to_bytes(uint8_t dst[],
const uint8_t src[], int bits) {
On 2015/03/26 14:37:57, scroggo wrote:
> Matt, doesn't some of your code do something like this? Think it's possible to
> share? I don't remember where yours is to compare though...

Yes thanks for the reminder.  The swizzler supports this type of input format
(it is called kIndex1).  However we only support conversions to from kIndex1 to
kN32 with a color table of SkPMColor (there is also 565, but I intend to delete
that).

It's probably worth considering using the swizzler and adding functionality to
support conversions to kGray etc.  I'm not 100% that it's definitely the way to
go, but it's worth thinking about.

My concerns would be:
Does using the swizzler impact the performance on wbmp decodes?
Would having these swizzles be useful to other decoders?  Bmp is the other file
format that may use small indices, but I'm not sure we would ever want to
convert this to greyscale etc.

[scroggo, 2015-03-26 15:25:11]

On 2015/03/26 15:03:56, msarett wrote:
> https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.cpp
> File src/codec/SkCodec_wbmp.cpp (right):
> 
>
https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.cpp#...
> src/codec/SkCodec_wbmp.cpp:51: static void expand_bits_to_bytes(uint8_t dst[],
> const uint8_t src[], int bits) {
> On 2015/03/26 14:37:57, scroggo wrote:
> > Matt, doesn't some of your code do something like this? Think it's possible
to
> > share? I don't remember where yours is to compare though...
> 
> Yes thanks for the reminder.  The swizzler supports this type of input format
> (it is called kIndex1).  However we only support conversions to from kIndex1
to
> kN32 with a color table of SkPMColor (there is also 565, but I intend to
delete
> that).
> 
> It's probably worth considering using the swizzler and adding functionality to
> support conversions to kGray etc.  I'm not 100% that it's definitely the way
to
> go, but it's worth thinking about.
> 
> My concerns would be:
> Does using the swizzler impact the performance on wbmp decodes?
> Would having these swizzles be useful to other decoders?  Bmp is the other
file
> format that may use small indices, but I'm not sure we would ever want to
> convert this to greyscale etc.

I think we do not want to *convert* to greyscale, but if an image is naturally
greyscale, we should be able to support that.

[reed1, 2015-03-26 15:39:10]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2015-03-26 15:39:10]

In my head, the rule is that codecs don't make quality-tradeoff-decisions, but
only offer native "converts" if they can be perfect. Skia itself has a host of
ways to convert to other depths/etc. with options for dithering/filtering/etc.

[hal.canary, 2015-03-26 17:10:45]

Patchset #2 (id:20001) has been deleted

[hal.canary, 2015-03-26 17:10:54]

https://codereview.chromium.org/1006583005/diff/1/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1006583005/diff/1/dm/DMSrcSink.cpp#newcode97
dm/DMSrcSink.cpp:97: if (SkInstallDiscardablePixelRef(codec.detach(), &bitmap))
{
On 2015/03/26 14:37:57, scroggo wrote:
> On 2015/03/26 12:37:35, msarett wrote:
> > What does this do?  I'm just not sure I understand.
> 
> SkInstallDiscardablePixelRef is declared in SkImageGenerator.h. [...]

I am removing this code for now.

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.cpp
File src/codec/SkCodec_wbmp.cpp (right):

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.cpp#...
src/codec/SkCodec_wbmp.cpp:13: return stream->read(data, 1) == 1;
On 2015/03/26 12:37:35, msarett wrote:
> I was going to suggest using get_byte, get_short, and get_int from
> SkCodecPriv.h, but it looks like you are reading a variable length quantity. 
> Because of that, this probably makes sense.

Acknowledged.

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.cpp#...
src/codec/SkCodec_wbmp.cpp:51: static void expand_bits_to_bytes(uint8_t dst[],
const uint8_t src[], int bits) {
On 2015/03/26 15:03:56, msarett wrote:
> On 2015/03/26 14:37:57, scroggo wrote:
> > Matt, doesn't some of your code do something like this? Think it's possible
to
> > share? I don't remember where yours is to compare though...
> 
> Yes thanks for the reminder.  The swizzler supports this type of input format
> (it is called kIndex1).  However we only support conversions to from kIndex1
to
> kN32 with a color table of SkPMColor (there is also 565, but I intend to
delete
> that).
> 
> It's probably worth considering using the swizzler and adding functionality to
> support conversions to kGray etc.  I'm not 100% that it's definitely the way
to
> go, but it's worth thinking about.
> 
> My concerns would be:
> Does using the swizzler impact the performance on wbmp decodes?
> Would having these swizzles be useful to other decoders?  Bmp is the other
file
> format that may use small indices, but I'm not sure we would ever want to
> convert this to greyscale etc.

I put a TODO in here.

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.cpp#...
src/codec/SkCodec_wbmp.cpp:78: class WbmpCodec : public SkCodec {
On 2015/03/26 14:37:57, scroggo wrote:
> On 2015/03/26 12:37:35, msarett wrote:
> > nit: We have been declaring the class in the header file and just using the
> .cpp
> > file for implementations.  It's probably just a matter of preference, but it
> > might be good to be consistent.  It might be worth getting Leon's opinion on
> > this.
> 
> +1 for consistency.

Done.

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.cpp#...
src/codec/SkCodec_wbmp.cpp:82: ~WbmpCodec() {}
On 2015/03/26 12:37:35, msarett wrote:
> nit: Probably just a matter of preference, but we have been omitting the blank
> destructor.

Done.

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.cpp#...
src/codec/SkCodec_wbmp.cpp:87: const SkImageInfo&, void*, size_t, const
Options&, SkPMColor[], int*);
On 2015/03/26 12:37:35, msarett wrote:
> nit: Should these arguments be indented twice and lined up?

Done.

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.cpp#...
src/codec/SkCodec_wbmp.cpp:100: return SkImageGenerator::kInvalidConversion;
On 2015/03/26 12:37:35, msarett wrote:
> We should also allow conversions to kN32_SkColorType.  This has been our
default
> even for images that are more suitable for kIndex8 because we can draw to kN32
> and I think it's what android wants.  It shouldn't take too much code to
support
> this, just use SkSwizzler.
> 
> It's good to have kIndex8 supported as well because we are planning to add
that
> for other image types as well.

Done.

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.h
File src/codec/SkCodec_wbmp.h (right):

https://codereview.chromium.org/1006583005/diff/1/src/codec/SkCodec_wbmp.h#ne...
src/codec/SkCodec_wbmp.h:14: namespace SkWbmpCodec {
On 2015/03/26 14:37:57, scroggo wrote:
> On 2015/03/26 12:37:35, msarett wrote:
> > What is the effect of declaring a namespace?  I'm just curious how this
works.
> 
> > Is this in place of declaring the class?
> 
> Yes. It's mostly the same if all your methods are static. I believe there's
some
> internal disagreement on the Skia team as to the better way to do it.

I find that namespace makes it more clear that this is not the data structure.

[scroggo, 2015-03-26 17:48:04]

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

https://codereview.chromium.org/1006583005/diff/40001/src/codec/SkCodec_wbmp....
src/codec/SkCodec_wbmp.cpp:59: static SkPMColor bit_to_pmcolor(unsigned bit) {
return bit ? WHITE : BLACK; }
Why do all these take an unsigned as a parameter? Isn't uint8_t the type of src
that gets passed in?

https://codereview.chromium.org/1006583005/diff/40001/src/codec/SkCodec_wbmp....
src/codec/SkCodec_wbmp.cpp:73: // TODO(halcanary): Add this functionality
(multiple output types) to
It's not clear to me what functionality you want to add to SkSwizzler.
SkSwizzler takes a void*, and treats the dst according to the dst format. So it
*does* output in different formats, secretly.

https://codereview.chromium.org/1006583005/diff/40001/src/codec/SkCodec_wbmp....
src/codec/SkCodec_wbmp.cpp:97: : SkCodec(info, stream) {}
nit: INHERITED.

https://codereview.chromium.org/1006583005/diff/40001/src/codec/SkCodec_wbmp....
src/codec/SkCodec_wbmp.cpp:104: void* pixels,
nit: Should line up with first parameter.

https://codereview.chromium.org/1006583005/diff/40001/src/codec/SkCodec_wbmp....
src/codec/SkCodec_wbmp.cpp:110: if (!this->rewindIfNeeded(&didTryToRewind)) {
Maybe this should return a tri state?

kCouldNotRewind
kRewound
kNoRewindNecessary

[hal.canary, 2015-03-26 19:46:34]

Patchset #4 (id:80001) has been deleted

[hal.canary, 2015-03-26 19:46:43]

Patchset #3 (id:60001) has been deleted

[hal.canary, 2015-03-26 19:46:52]

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

https://codereview.chromium.org/1006583005/diff/40001/src/codec/SkCodec_wbmp....
src/codec/SkCodec_wbmp.cpp:59: static SkPMColor bit_to_pmcolor(unsigned bit) {
return bit ? WHITE : BLACK; }
On 2015/03/26 17:48:04, scroggo wrote:
> Why do all these take an unsigned as a parameter? Isn't uint8_t the type of
src
> that gets passed in?

Will a U8CPU be okay?

https://codereview.chromium.org/1006583005/diff/40001/src/codec/SkCodec_wbmp....
src/codec/SkCodec_wbmp.cpp:73: // TODO(halcanary): Add this functionality
(multiple output types) to
On 2015/03/26 17:48:03, scroggo wrote:
> It's not clear to me what functionality you want to add to SkSwizzler.
> SkSwizzler takes a void*, and treats the dst according to the dst format. So
it
> *does* output in different formats, secretly.

grayscale and indexed output

https://codereview.chromium.org/1006583005/diff/40001/src/codec/SkCodec_wbmp....
src/codec/SkCodec_wbmp.cpp:97: : SkCodec(info, stream) {}
On 2015/03/26 17:48:03, scroggo wrote:
> nit: INHERITED.

Done.

https://codereview.chromium.org/1006583005/diff/40001/src/codec/SkCodec_wbmp....
src/codec/SkCodec_wbmp.cpp:104: void* pixels,
On 2015/03/26 17:48:03, scroggo wrote:
> nit: Should line up with first parameter.

Done.

https://codereview.chromium.org/1006583005/diff/40001/src/codec/SkCodec_wbmp....
src/codec/SkCodec_wbmp.cpp:110: if (!this->rewindIfNeeded(&didTryToRewind)) {
On 2015/03/26 17:48:03, scroggo wrote:
> Maybe this should return a tri state?
> 
> kCouldNotRewind
> kRewound
> kNoRewindNecessary

Done.

[scroggo, 2015-03-26 20:29:45]

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

https://codereview.chromium.org/1006583005/diff/40001/src/codec/SkCodec_wbmp....
src/codec/SkCodec_wbmp.cpp:59: static SkPMColor bit_to_pmcolor(unsigned bit) {
return bit ? WHITE : BLACK; }
On 2015/03/26 19:46:52, Hal Canary wrote:
> On 2015/03/26 17:48:04, scroggo wrote:
> > Why do all these take an unsigned as a parameter? Isn't uint8_t the type of
> src
> > that gets passed in?
> 
> Will a U8CPU be okay?

It seems better than unsigned. But why do we use uint8_t in some places, and
U8CPU in others?

Is it because U8CPU is a "Fast type", "not for storage", as stated in SkTypes.h?
If so, then U8CPU seems fine.

https://codereview.chromium.org/1006583005/diff/100001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1006583005/diff/100001/include/codec/SkCodec....
include/codec/SkCodec.h:154: *  onGetPixels). If it returns false, onGetPixels
should return
false -> kCouldNotRewind.

If it returns kRewound, the stream is back at the beginning, and needs to be
advanced to the point where we are ready to call getPixels (e.g. the header
needs to be read/skipped).

Maybe the names should not reflect rewinding, but where the exact location of
the stream is. Maybe something like:

kBeginning
kEndofHeader (although an SkCodec may read more than the header, this
              seems fairly descriptive)
kUndetermined - or something else? This means we don't know how to
                proceed, so we have to return kCouldNotRewind.

https://codereview.chromium.org/1006583005/diff/100001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1006583005/diff/100001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:66: return noRewind ? kNoRewindNecessary_RewindState
nit: I find this double tertiary statement harder to follow than just

if (noRewind) {
  return kNoRewindNece;
}

// other conditions, tertiary or otherwise.

https://codereview.chromium.org/1006583005/diff/100001/tests/CodexTest.cpp
File tests/CodexTest.cpp (right):

https://codereview.chromium.org/1006583005/diff/100001/tests/CodexTest.cpp#ne...
tests/CodexTest.cpp:34: if (!size.isZero()) {
is size ever zero? Why? (It doesn't look like it from the callers.) That said,
it seems like info.dimensions() should match or we should not have created an
SkCodec to begin with (I think the latter).

https://codereview.chromium.org/1006583005/diff/100001/tests/CodexTest.cpp#ne...
tests/CodexTest.cpp:48: // is a know bug that some can not.
known*

https://codereview.chromium.org/1006583005/diff/100001/tests/CodexTest.cpp#ne...
tests/CodexTest.cpp:55: // WBMP
ICO, too?

https://codereview.chromium.org/1006583005/diff/100001/tests/CodexTest.cpp#ne...
tests/CodexTest.cpp:58: // BMP
FIXME (msarett): SkBmpCodec should be able to rewind.

https://codereview.chromium.org/1006583005/diff/100001/tests/CodexTest.cpp#ne...
tests/CodexTest.cpp:61: // PNG
FIXME (scroggo): SkPngCodec should be able to rewind.

[hal.canary, 2015-03-26 21:46:10]

https://codereview.chromium.org/1006583005/diff/100001/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/1006583005/diff/100001/include/codec/SkCodec....
include/codec/SkCodec.h:154: *  onGetPixels). If it returns false, onGetPixels
should return
On 2015/03/26 20:29:44, scroggo wrote:
> false -> kCouldNotRewind.
> 
> If it returns kRewound, the stream is back at the beginning, and needs to be
> advanced to the point where we are ready to call getPixels (e.g. the header
> needs to be read/skipped).
> 
> Maybe the names should not reflect rewinding, but where the exact location of
> the stream is. Maybe something like:
> 
> kBeginning
> kEndofHeader (although an SkCodec may read more than the header, this
>               seems fairly descriptive)
> kUndetermined - or something else? This means we don't know how to
>                 proceed, so we have to return kCouldNotRewind.

Let's leave it as is.  You can change it in the future if you like.

https://codereview.chromium.org/1006583005/diff/100001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1006583005/diff/100001/src/codec/SkCodec.cpp#...
src/codec/SkCodec.cpp:66: return noRewind ? kNoRewindNecessary_RewindState
On 2015/03/26 20:29:44, scroggo wrote:
> nit: I find this double tertiary statement harder to follow than just
> 
> if (noRewind) {
>   return kNoRewindNece;
> }
> 
> // other conditions, tertiary or otherwise.

Done.

https://codereview.chromium.org/1006583005/diff/100001/tests/CodexTest.cpp
File tests/CodexTest.cpp (right):

https://codereview.chromium.org/1006583005/diff/100001/tests/CodexTest.cpp#ne...
tests/CodexTest.cpp:34: if (!size.isZero()) {
On 2015/03/26 20:29:45, scroggo wrote:
> is size ever zero? Why? (It doesn't look like it from the callers.) That said,
> it seems like info.dimensions() should match or we should not have created an
> SkCodec to begin with (I think the latter).

I didn't know the dimensions of all the files when I first wrote this.  Fixed.

https://codereview.chromium.org/1006583005/diff/100001/tests/CodexTest.cpp#ne...
tests/CodexTest.cpp:48: // is a know bug that some can not.
On 2015/03/26 20:29:45, scroggo wrote:
> known*

Done.

https://codereview.chromium.org/1006583005/diff/100001/tests/CodexTest.cpp#ne...
tests/CodexTest.cpp:55: // WBMP
On 2015/03/26 20:29:44, scroggo wrote:
> ICO, too?

Done.

https://codereview.chromium.org/1006583005/diff/100001/tests/CodexTest.cpp#ne...
tests/CodexTest.cpp:58: // BMP
On 2015/03/26 20:29:44, scroggo wrote:
> FIXME (msarett): SkBmpCodec should be able to rewind.

Done.

https://codereview.chromium.org/1006583005/diff/100001/tests/CodexTest.cpp#ne...
tests/CodexTest.cpp:61: // PNG
On 2015/03/26 20:29:44, scroggo wrote:
> FIXME (scroggo): SkPngCodec should be able to rewind.

Done.

[hal.canary, 2015-03-26 21:46:49]

Patchset #4 (id:120001) has been deleted

[scroggo, 2015-03-27 12:52:59]

https://codereview.chromium.org/1006583005/diff/140001/tests/CodexTest.cpp
File tests/CodexTest.cpp (right):

https://codereview.chromium.org/1006583005/diff/140001/tests/CodexTest.cpp#ne...
tests/CodexTest.cpp:22: size_t rowLen = bm.info().bytesPerPixel() * bm.height();
Shouldn't bm.height() be bm.width()?

[hal.canary, 2015-03-27 13:53:02]

https://codereview.chromium.org/1006583005/diff/140001/tests/CodexTest.cpp
File tests/CodexTest.cpp (right):

https://codereview.chromium.org/1006583005/diff/140001/tests/CodexTest.cpp#ne...
tests/CodexTest.cpp:22: size_t rowLen = bm.info().bytesPerPixel() * bm.height();
On 2015/03/27 12:52:59, scroggo wrote:
> Shouldn't bm.height() be bm.width()?

Done.

[scroggo, 2015-03-27 14:09:48]

lgtm

https://codereview.chromium.org/1006583005/diff/160001/src/utils/SkMD5.h
File src/utils/SkMD5.h (right):

https://codereview.chromium.org/1006583005/diff/160001/src/utils/SkMD5.h#newc...
src/utils/SkMD5.h:39: bool operator ==(Digest const& other) const {
I almost suggested you write this! +1

[hal.canary, 2015-03-27 14:33:05]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-03-27 14:33:15]

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

[commit-bot: I haz the power, 2015-03-27 14:42:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-27 14:42:53]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[hal.canary, 2015-03-27 14:53:26]

The CQ bit was checked by halcanary@google.com

[hal.canary, 2015-03-27 14:53:27]

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/1006583005/#ps180001
(title: "2015-03-27 (Friday) 10:53:14 EDT")

[commit-bot: I haz the power, 2015-03-27 14:53:43]

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

[hal.canary, 2015-03-27 15:34:40]

reed@, please okay include/codec/SkCodec.h changes

[reed1, 2015-03-27 18:14:41]

api lgtm

-- mike-I-hate-streams-reed

[commit-bot: I haz the power, 2015-03-27 19:11:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-27 19:11:41]

This issue passed the CQ dry run.

[scroggo, 2015-03-27 19:12:16]

On 2015/03/27 19:11:41, I haz the power (commit-bot) wrote:
> This issue passed the CQ dry run.

Hal, ready to commit for real?

[hal.canary, 2015-03-27 19:12:57]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-03-27 19:13:07]

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

[commit-bot: I haz the power, 2015-03-27 19:16:57]

Committed patchset #6 (id:180001) as
https://skia.googlesource.com/skia/+/a096d7a6d03662073f4cd46f7db5fe2cf5495c36