Make SkAndroidCodec support wbmp

Make SkAndroidCodec support wbmp

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/33c762332403afb557dad62d0e67ac74e291ae27

[msarett, 2015-11-13 18:52:43]

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

[msarett, 2015-11-13 18:52:44]

While I'm waiting on Android feedback on various follow-ups to BRD, I'm going to
start working toward BitmapFactory.

I hope to reenable the relevant image formats for SkAndroidCodec (one at a time
with bug fixes).  Wbmp is the easiest, as I don't think there are any bugs to
fix.

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

https://codereview.chromium.org/1445643002/diff/1/src/codec/SkCodec_wbmp.cpp#...
src/codec/SkCodec_wbmp.cpp:172: if (options.fSubset) {
You could argue that this goes too far.  Instead, we could disable the
kDivisorMode for AndroidCodec in dm for wbmps.  This is definitely what I'll do
for GIF, where subsets are actually tricky.

However, I think this is fine for wbmp.  If I'm wrong, I'd like to turn this off
again, but I believe that supporting subsets is trivial.

[scroggo, 2015-11-16 14:31:36]

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

https://codereview.chromium.org/1445643002/diff/1/src/codec/SkCodec_wbmp.cpp#...
src/codec/SkCodec_wbmp.cpp:172: if (options.fSubset) {
On 2015/11/13 18:52:44, msarett wrote:
> You could argue that this goes too far.  Instead, we could disable the
> kDivisorMode for AndroidCodec in dm for wbmps.  This is definitely what I'll
do
> for GIF, where subsets are actually tricky.
> 
> However, I think this is fine for wbmp.  If I'm wrong, I'd like to turn this
off
> again, but I believe that supporting subsets is trivial.

It seems like this doesn't get us any closer to supporting BitmapFactory,
though. Why not focus on that, and then follow up with subset decodes if we
think it's important for wbmp?

[msarett, 2015-11-16 14:46:37]

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

https://codereview.chromium.org/1445643002/diff/1/src/codec/SkCodec_wbmp.cpp#...
src/codec/SkCodec_wbmp.cpp:172: if (options.fSubset) {
On 2015/11/16 14:31:35, scroggo wrote:
> On 2015/11/13 18:52:44, msarett wrote:
> > You could argue that this goes too far.  Instead, we could disable the
> > kDivisorMode for AndroidCodec in dm for wbmps.  This is definitely what I'll
> do
> > for GIF, where subsets are actually tricky.
> > 
> > However, I think this is fine for wbmp.  If I'm wrong, I'd like to turn this
> off
> > again, but I believe that supporting subsets is trivial.
> 
> It seems like this doesn't get us any closer to supporting BitmapFactory,
> though. Why not focus on that, and then follow up with subset decodes if we
> think it's important for wbmp?

sgtm

[scroggo, 2015-11-16 15:19:58]

https://codereview.chromium.org/1445643002/diff/20001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1445643002/diff/20001/dm/DM.cpp#newcode349
dm/DM.cpp:349: // Only test subsets for these selected image types.
nit: This sentence is ambiguous - I read it as meaning that for these image
types, we test subsets, but not full decodes.

https://codereview.chromium.org/1445643002/diff/20001/dm/DM.cpp#newcode380
dm/DM.cpp:380: for (uint32_t m = 0; m < numAndroidModes; m++) {
Now I think I see why you were tempted to go all the way and turn on subset
decoding - this is messy...

Two thoughts:
1. Testing subsets using SkAndroidCodec is pretty similar to using the BRD test
(which in turn uses SkAndroidCodec). Is there a benefit to having both? Or could
we cut down the androidModes to just kFullImage?
2. If we see value in having both versions of subset tests, do you think the
following is more clear?

// set new booleans "full" and "subset" how you currently set numAndroidModes,
// or possibly use static functions.

if (!full && !subset) {
  return;
}

for (sampleSize : sampleSizes) {
  for (colorType : colorTypes) {
    push_android_codec_src(path, kFullImage, colorType, sampleSize);
    if (subset) {
      push_android_codec_src(path, kDivisor, colorType, sampleSize);
    }
  }
}

[msarett, 2015-11-16 15:40:57]

https://codereview.chromium.org/1445643002/diff/20001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1445643002/diff/20001/dm/DM.cpp#newcode380
dm/DM.cpp:380: for (uint32_t m = 0; m < numAndroidModes; m++) {
On 2015/11/16 15:19:58, scroggo wrote:
> Now I think I see why you were tempted to go all the way and turn on subset
> decoding - this is messy...
> 
> Two thoughts:
> 1. Testing subsets using SkAndroidCodec is pretty similar to using the BRD
test
> (which in turn uses SkAndroidCodec). Is there a benefit to having both? Or
could
> we cut down the androidModes to just kFullImage?

I think you could make a good argument for this.  Right now, I'm not sure there
is much benefit to having both tests.  I'm hesitating on deleting this test
because I'm wondering if we might ever want subsets without a
BitmapRegionDecoder.

> 2. If we see value in having both versions of subset tests, do you think the
> following is more clear?
> 
> // set new booleans "full" and "subset" how you currently set numAndroidModes,
> // or possibly use static functions.
> 
> if (!full && !subset) {
>   return;
> }
> 
> for (sampleSize : sampleSizes) {
>   for (colorType : colorTypes) {
>     push_android_codec_src(path, kFullImage, colorType, sampleSize);
>     if (subset) {
>       push_android_codec_src(path, kDivisor, colorType, sampleSize);
>     }
>   }
> } 

Yeah I like this better.

[scroggo, 2015-11-16 20:36:48]

lgtm

[msarett, 2015-11-16 20:40:27]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-11-16 20:40:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1445643002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1445643002/40001

[commit-bot: I haz the power, 2015-11-16 20:49:04]

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

[commit-bot: I haz the power, 2015-11-16 20:49:04]

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

[msarett, 2015-11-16 21:14:04]

Description was changed from

==========
Make SkAndroidCodec support wbmp

BUG=skia:
==========

to

==========
Make SkAndroidCodec support wbmp

BUG=skia:
==========

[msarett, 2015-11-16 21:31:10]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-11-16 21:31:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1445643002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1445643002/40001

[commit-bot: I haz the power, 2015-11-16 21:43:43]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/33c762332403afb557dad62d0e67ac74e291ae27