Change computeOutputColorType() to mimic old behavior

Change computeOutputColorType() to mimic old behavior

This will prevent behavior changes in BitmapFactory when
switching from SkImageDecoder to SkAndroidCodec.

We will only choose kGray8 if it is explicitly requested (and
also supported).

Additionally, we will always decode GIFS and WBMPS to
kIndex8.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/457f54df1e6706f560e291ab5e3b90bf0d20ded0

[msarett, 2015-12-11 16:29:57]

Description was changed from

==========
computeOutputColorType() should not choose kGray8

We will only choose kGray8 if it is explicitly requested (and also
supported).

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

to

==========
computeOutputColorType() should rarely choose kGray8

We will only choose kGray8 if it is explicitly requested (and also
supported).

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

[msarett, 2015-12-11 16:41:13]

This gets us closer to matching the old behavior of BitmapFactory, but I took a
closer look at SkImageDecoder and there are still a lot of consistencies that we
might (or maybe not) want to be concerned about.

(1) SkID_wbmp ALWAYS decodes to kIndex8.  It ignores the requested color type.

Right now, SkAndroidCodec will decode to kGray (if requested) or kN32.  I think
we should support kIndex8 somehow.  I think that SkCodec is right to recommend
kGray, but SkAndroidCodec should recommend kIndex8 (this might be tricky).

Even if we do this, memory use will go up (because it clients ask for kN32, now
they'll actually get it).

(2) SkID_libgif ALWAYS decodes to kIndex8.  It ignores the requested color type.

I think we are ok to decode to whatever is requested (and fallback to kIndex8),
but, same as with wbmp, memory use may go up.

(3) SkID_libbmp only decodes to 8888, 565.

We support kIndex8 as well.  I can't see how this could be a problem.

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

https://codereview.chromium.org/1519843002/diff/1/src/codec/SkAndroidCodec.cp...
src/codec/SkAndroidCodec.cpp:84: if (kGray_8_SkColorType == suggestedColorType)
{
As you stated in another review, we need this to match SkImageDecoder_libpng.

[msarett, 2015-12-11 16:41:49]

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

[msarett, 2015-12-11 16:41:50]

I forgot to add a reviewer.  Please read my previous comments.

[scroggo, 2015-12-11 17:41:36]

On 2015/12/11 16:41:13, msarett wrote:
> This gets us closer to matching the old behavior of BitmapFactory, but I took
a
> closer look at SkImageDecoder and there are still a lot of consistencies that
we
> might (or maybe not) want to be concerned about.
> 
> (1) SkID_wbmp ALWAYS decodes to kIndex8.  It ignores the requested color type.
> 
> Right now, SkAndroidCodec will decode to kGray (if requested) or kN32.  I
think
> we should support kIndex8 somehow.  I think that SkCodec is right to recommend
> kGray, but SkAndroidCodec should recommend kIndex8 (this might be tricky).
> 
> Even if we do this, memory use will go up (because it clients ask for kN32,
now
> they'll actually get it).

I am curious to know whether WBMP is ever actually used. It seems to be a rare
format. That said, I'd be very nervous to make such a drastic increase to memory
use. I don't think it will be that tricky:

if (codec()->getEncodedFormat() == WBMP) {
  // recommend index8
  // decode to index8, even with N32 requested
}

> 
> (2) SkID_libgif ALWAYS decodes to kIndex8.  It ignores the requested color
type.
> 
> I think we are ok to decode to whatever is requested (and fallback to
kIndex8),
> but, same as with wbmp, memory use may go up.

This is probably more common than wbmp. We could use a similar approach as for
wbmp, if we want to keep the memory down.

> 
> (3) SkID_libbmp only decodes to 8888, 565.
> 
> We support kIndex8 as well.  I can't see how this could be a problem.

I think that's a plus on memory use. I happened to come across a bug recently
where we found index8 to be slow on the GPU, but Android uploads as N32 anyway.

> 
> https://codereview.chromium.org/1519843002/diff/1/src/codec/SkAndroidCodec.cpp
> File src/codec/SkAndroidCodec.cpp (right):
> 
>
https://codereview.chromium.org/1519843002/diff/1/src/codec/SkAndroidCodec.cp...
> src/codec/SkAndroidCodec.cpp:84: if (kGray_8_SkColorType ==
suggestedColorType)
> {
> As you stated in another review, we need this to match SkImageDecoder_libpng.

[scroggo, 2015-12-11 17:41:43]

On 2015/12/11 17:41:36, scroggo wrote:
> On 2015/12/11 16:41:13, msarett wrote:
> > This gets us closer to matching the old behavior of BitmapFactory, but I
took
> a
> > closer look at SkImageDecoder and there are still a lot of consistencies
that
> we
> > might (or maybe not) want to be concerned about.
> > 
> > (1) SkID_wbmp ALWAYS decodes to kIndex8.  It ignores the requested color
type.
> > 
> > Right now, SkAndroidCodec will decode to kGray (if requested) or kN32.  I
> think
> > we should support kIndex8 somehow.  I think that SkCodec is right to
recommend
> > kGray, but SkAndroidCodec should recommend kIndex8 (this might be tricky).
> > 
> > Even if we do this, memory use will go up (because it clients ask for kN32,
> now
> > they'll actually get it).
> 
> I am curious to know whether WBMP is ever actually used. It seems to be a rare
> format. That said, I'd be very nervous to make such a drastic increase to
memory
> use. I don't think it will be that tricky:
> 
> if (codec()->getEncodedFormat() == WBMP) {
>   // recommend index8
>   // decode to index8, even with N32 requested
> }
> 
> > 
> > (2) SkID_libgif ALWAYS decodes to kIndex8.  It ignores the requested color
> type.
> > 
> > I think we are ok to decode to whatever is requested (and fallback to
> kIndex8),
> > but, same as with wbmp, memory use may go up.
> 
> This is probably more common than wbmp. We could use a similar approach as for
> wbmp, if we want to keep the memory down.
> 
> > 
> > (3) SkID_libbmp only decodes to 8888, 565.
> > 
> > We support kIndex8 as well.  I can't see how this could be a problem.
> 
> I think that's a plus on memory use. I happened to come across a bug recently
> where we found index8 to be slow on the GPU, but Android uploads as N32
anyway.
> 
> > 
> >
https://codereview.chromium.org/1519843002/diff/1/src/codec/SkAndroidCodec.cpp
> > File src/codec/SkAndroidCodec.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1519843002/diff/1/src/codec/SkAndroidCodec.cp...
> > src/codec/SkAndroidCodec.cpp:84: if (kGray_8_SkColorType ==
> suggestedColorType)
> > {
> > As you stated in another review, we need this to match
SkImageDecoder_libpng.

lgtm

[msarett, 2015-12-11 18:10:17]

PTAL

I've added compatibility fixes for GIF and WBMP.  You're right, it wasn't too
tricky :).

I *think* this is the right thing to do...  I doubt it's very common to modify
the default settings.  And I don't think we want to modify the default behavior
to use 4x more memory.

[scroggo, 2015-12-11 18:20:34]

Please update the check-in comment (something like, mimic old behavior, I'm
guessing).

Otherwise lgtm at patch set 2

[msarett, 2015-12-11 18:23:29]

Description was changed from

==========
computeOutputColorType() should rarely choose kGray8

We will only choose kGray8 if it is explicitly requested (and also
supported).

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

to

==========
Change computeOutputColorType() to mimic old behavior

This will prevent behavior changes in BitmapFactory when
switching from SkImageDecoder to SkAndroidCodec.

We will only choose kGray8 if it is explicitly requested (and
also supported).

Additionally, we will always decode GIFS and WBMPS to
kIndex8.

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

[msarett, 2015-12-11 18:24:03]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-12-11 18:24:06]

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

[commit-bot: I haz the power, 2015-12-11 18:37:48]

Description was changed from

==========
Change computeOutputColorType() to mimic old behavior

This will prevent behavior changes in BitmapFactory when
switching from SkImageDecoder to SkAndroidCodec.

We will only choose kGray8 if it is explicitly requested (and
also supported).

Additionally, we will always decode GIFS and WBMPS to
kIndex8.

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

to

==========
Change computeOutputColorType() to mimic old behavior

This will prevent behavior changes in BitmapFactory when
switching from SkImageDecoder to SkAndroidCodec.

We will only choose kGray8 if it is explicitly requested (and
also supported).

Additionally, we will always decode GIFS and WBMPS to
kIndex8.

BUG=skia:

Committed:
https://skia.googlesource.com/skia/+/457f54df1e6706f560e291ab5e3b90bf0d20ded0
==========

[commit-bot: I haz the power, 2015-12-11 18:37:48]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/457f54df1e6706f560e291ab5e3b90bf0d20ded0