Make BitmapRegionDecoder succeed on invalid requests

Make BitmapRegionDecoder succeed on invalid requests

If the client requests a color type or alpha type that is
not supported, we should decode to the default/appropriate
color and alpha types to match existing behavior in Android.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/9a0e3467da2a61ec81f676c147c21c77de91fccf

[msarett, 2015-12-09 19:51:03]

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

[msarett, 2015-12-09 19:51:04]

In this implementation I added functions to SkAndroidCodec.
This is useful because I could then share these functions
with BitmapFactory.cpp in Android.

But I don't think these belong as part of the API.  I think
I'd prefer keeping this code inline or adding static helpers.
But then we have the same code in two places.

Let me know what you think.

[msarett, 2015-12-09 19:55:46]

Description was changed from

==========
BitmapRegionDecoder should succeed on invalid requested color types

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

to

==========
Make BitmapRegionDecoder succeed on invalid requests

If the client requests a color type or alpha type that is
not supported, we should decode to the default/appropriate
color and alpha types to match existing behavior in Android.

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

[scroggo, 2015-12-09 20:57:31]

On 2015/12/09 19:51:04, msarett wrote:
> In this implementation I added functions to SkAndroidCodec.
> This is useful because I could then share these functions
> with BitmapFactory.cpp in Android.
> 
> But I don't think these belong as part of the API. 

Why not (they are, but I think you mean non-static members?)? It seems like you
would not need to pass suggestedColorType or suggestedAlphaType (since the
object already knows it).

https://codereview.chromium.org/1513023002/diff/1/src/android/SkBitmapRegionC...
File src/android/SkBitmapRegionCanvas.cpp (right):

https://codereview.chromium.org/1513023002/diff/1/src/android/SkBitmapRegionC...
src/android/SkBitmapRegionCanvas.cpp:21: // FIXME: This class works well as a
performance/quality comparison for
Maybe this should be in the header?

https://codereview.chromium.org/1513023002/diff/1/src/android/SkBitmapRegionC...
File src/android/SkBitmapRegionCodec.cpp (right):

https://codereview.chromium.org/1513023002/diff/1/src/android/SkBitmapRegionC...
src/android/SkBitmapRegionCodec.cpp:97: if (kGray_8_SkColorType == dstColorType)
{
Comment explaining why?

[msarett, 2015-12-10 15:11:46]

Patchset #2 (id:20001) has been deleted

[msarett, 2015-12-10 15:11:51]

Patchset #2 (id:40001) has been deleted

[msarett, 2015-12-10 15:18:34]

On 2015/12/09 20:57:31, scroggo wrote:
> On 2015/12/09 19:51:04, msarett wrote:
> > In this implementation I added functions to SkAndroidCodec.
> > This is useful because I could then share these functions
> > with BitmapFactory.cpp in Android.
> > 
> > But I don't think these belong as part of the API. 
> 
> Why not (they are, but I think you mean non-static members?)? It seems like
you
> would not need to pass suggestedColorType or suggestedAlphaType (since the
> object already knows it).
> 

I was suggesting that we duplicate this code in BitmapRegionDecoder and
BitmapFactory
rather than have it in SkAndroidCodec.

But I like your suggestion.  I think this works well as non-static member
functions.

>
https://codereview.chromium.org/1513023002/diff/1/src/android/SkBitmapRegionC...
> File src/android/SkBitmapRegionCanvas.cpp (right):
> 
>
https://codereview.chromium.org/1513023002/diff/1/src/android/SkBitmapRegionC...
> src/android/SkBitmapRegionCanvas.cpp:21: // FIXME: This class works well as a
> performance/quality comparison for
> Maybe this should be in the header?
> 
>
https://codereview.chromium.org/1513023002/diff/1/src/android/SkBitmapRegionC...
> File src/android/SkBitmapRegionCodec.cpp (right):
> 
>
https://codereview.chromium.org/1513023002/diff/1/src/android/SkBitmapRegionC...
> src/android/SkBitmapRegionCodec.cpp:97: if (kGray_8_SkColorType ==
dstColorType)
> {
> Comment explaining why?

[msarett, 2015-12-10 15:19:26]

On 2015/12/09 20:57:31, scroggo wrote:
> On 2015/12/09 19:51:04, msarett wrote:
> > In this implementation I added functions to SkAndroidCodec.
> > This is useful because I could then share these functions
> > with BitmapFactory.cpp in Android.
> > 
> > But I don't think these belong as part of the API. 
> 
> Why not (they are, but I think you mean non-static members?)? It seems like
you
> would not need to pass suggestedColorType or suggestedAlphaType (since the
> object already knows it).
> 

I was suggesting that we duplicate this code in
BitmapRegionDecoder and BitmapFactory rather than have it
in SkAndroidCodec.

But I like your suggestion.  I think this works well as
non-static member functions.

https://codereview.chromium.org/1513023002/diff/1/src/android/SkBitmapRegionC...
File src/android/SkBitmapRegionCanvas.cpp (right):

https://codereview.chromium.org/1513023002/diff/1/src/android/SkBitmapRegionC...
src/android/SkBitmapRegionCanvas.cpp:21: // FIXME: This class works well as a
performance/quality comparison for
On 2015/12/09 20:57:31, scroggo wrote:
> Maybe this should be in the header?

Done.

https://codereview.chromium.org/1513023002/diff/1/src/android/SkBitmapRegionC...
File src/android/SkBitmapRegionCodec.cpp (right):

https://codereview.chromium.org/1513023002/diff/1/src/android/SkBitmapRegionC...
src/android/SkBitmapRegionCodec.cpp:97: if (kGray_8_SkColorType == dstColorType)
{
On 2015/12/09 20:57:31, scroggo wrote:
> Comment explaining why?

Done.

[scroggo, 2015-12-10 15:40:55]

lgtm

[msarett, 2015-12-10 15:43:44]

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

[msarett, 2015-12-10 15:43:44]

Derek can you take a look at the API?

Adding these functions to SkAndroidCodec allows us to share them between
BitmapFactory and BitmapRegionDecoder.

[msarett, 2015-12-10 20:49:10]

PTAL

[msarett, 2015-12-10 20:50:25]

Hang on this is still buggy...

[djsollen, 2015-12-10 21:06:45]

https://codereview.chromium.org/1513023002/diff/80001/include/codec/SkAndroid...
File include/codec/SkAndroidCodec.h (right):

https://codereview.chromium.org/1513023002/diff/80001/include/codec/SkAndroid...
include/codec/SkAndroidCodec.h:61: SkColorType fixRequestedColorType(SkColorType
requestedColorType);
this seems like an odd name to me.  something like compute, find, or select seem
like a better fit than fixRequested.

[msarett, 2015-12-10 22:41:55]

I've changed the API to computeOutput*Type() and fixed computeOutputColorType()
to behave consistently with the expectations for BitmapFactory/BRD.

https://codereview.chromium.org/1513023002/diff/80001/include/codec/SkAndroid...
File include/codec/SkAndroidCodec.h (right):

https://codereview.chromium.org/1513023002/diff/80001/include/codec/SkAndroid...
include/codec/SkAndroidCodec.h:61: SkColorType fixRequestedColorType(SkColorType
requestedColorType);
On 2015/12/10 21:06:45, djsollen wrote:
> this seems like an odd name to me.  something like compute, find, or select
seem
> like a better fit than fixRequested.

Changing to computeOutput*Type()

[djsollen, 2015-12-11 13:49:26]

lgtm

api lgtm with a nit

https://codereview.chromium.org/1513023002/diff/100001/include/codec/SkAndroi...
File include/codec/SkAndroidCodec.h (right):

https://codereview.chromium.org/1513023002/diff/100001/include/codec/SkAndroi...
include/codec/SkAndroidCodec.h:56: *  @param requestedColorType Color type
requested by the client
When doxygen formats this comment this won't look quite right.  Each method
should have a description, which it looks like you put mostly in the @return
annotation.  I would just delete the @return and make those the main body of the
comment.

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

Patchset #5 (id:120001) has been deleted

[msarett, 2015-12-11 14:17:54]

https://codereview.chromium.org/1513023002/diff/100001/include/codec/SkAndroi...
File include/codec/SkAndroidCodec.h (right):

https://codereview.chromium.org/1513023002/diff/100001/include/codec/SkAndroi...
include/codec/SkAndroidCodec.h:56: *  @param requestedColorType Color type
requested by the client
On 2015/12/11 13:49:25, djsollen wrote:
> When doxygen formats this comment this won't look quite right.  Each method
> should have a description, which it looks like you put mostly in the @return
> annotation.  I would just delete the @return and make those the main body of
the
> comment.

Done.  Also changing the comment to reflect that we default to the color type
recommended by the codec.

[msarett, 2015-12-11 15:22:50]

The CQ bit was checked by msarett@google.com

[msarett, 2015-12-11 15:22:50]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com,
djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1513023002/#ps140001
(title: " Update comments")

[commit-bot: I haz the power, 2015-12-11 15:23:03]

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

[commit-bot: I haz the power, 2015-12-11 15:38:53]

Description was changed from

==========
Make BitmapRegionDecoder succeed on invalid requests

If the client requests a color type or alpha type that is
not supported, we should decode to the default/appropriate
color and alpha types to match existing behavior in Android.

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

to

==========
Make BitmapRegionDecoder succeed on invalid requests

If the client requests a color type or alpha type that is
not supported, we should decode to the default/appropriate
color and alpha types to match existing behavior in Android.

BUG=skia:

Committed:
https://skia.googlesource.com/skia/+/9a0e3467da2a61ec81f676c147c21c77de91fccf
==========

[commit-bot: I haz the power, 2015-12-11 15:38:54]

Committed patchset #5 (id:140001) as
https://skia.googlesource.com/skia/+/9a0e3467da2a61ec81f676c147c21c77de91fccf