Checking for valid colorType, alphaType, colorSpace in SkCodec

Checking for valid colorType, alphaType, colorSpace in SkCodec

* Refactor to share code between SkPngCodec and SkWebpCodec
* Didn't end up sharing with SkJpegCodec but did refactor
  that code a bit
* Disallow conversions to F16 with non-linear color spaces
* Fail to decode if we fail to create a SkColorSpaceXform
  (should be an assert soon).  We used to fallback on a
  legacy decode if we failed to create the transform.
* A bunch of name changes

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2319293003

Committed: https://skia.googlesource.com/skia/+/7a9900d6d34e437bb24beb5524a1f6488ae138c9
Committed: https://skia.googlesource.com/skia/+/2ecc35ffa5feb56cd088478589ba1e48382b2e1f

[msarett, 2016-09-08 14:51:48]

Description was changed from

==========
Refactor conversion_possible() in SkCodec

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

to

==========
Refactor conversion_possible() in SkCodec

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2319293003
==========

[msarett, 2016-09-08 14:54:13]

Patchset #2 (id:20001) has been deleted

[msarett, 2016-09-08 14:54:20]

Patchset #1 (id:1) has been deleted

[msarett, 2016-09-08 15:49:39]

Patchset #1 (id:40001) has been deleted

[msarett, 2016-09-08 15:53:56]

Description was changed from

==========
Refactor conversion_possible() in SkCodec

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2319293003
==========

to

==========
Checking for valid colorType, alphaType, colorSpace in SkCodec
* Refactor to share code between SkPngCodec and SkWebpCodec
* Disallow conversions to F16 with non-linear color spaces
* Fail to decode if we fail to create color xform
  (should be an assert soon).  We used to fallback on a
  legacy decode if we failed to create the transform.
* A bunch of name changes

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2319293003
==========

[msarett, 2016-09-08 15:54:09]

Description was changed from

==========
Checking for valid colorType, alphaType, colorSpace in SkCodec
* Refactor to share code between SkPngCodec and SkWebpCodec
* Disallow conversions to F16 with non-linear color spaces
* Fail to decode if we fail to create color xform
  (should be an assert soon).  We used to fallback on a
  legacy decode if we failed to create the transform.
* A bunch of name changes

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2319293003
==========

to

==========
Checking for valid colorType, alphaType, colorSpace in SkCodec

* Refactor to share code between SkPngCodec and SkWebpCodec
* Disallow conversions to F16 with non-linear color spaces
* Fail to decode if we fail to create color xform
  (should be an assert soon).  We used to fallback on a
  legacy decode if we failed to create the transform.
* A bunch of name changes

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2319293003
==========

[msarett, 2016-09-08 15:55:48]

Description was changed from

==========
Checking for valid colorType, alphaType, colorSpace in SkCodec

* Refactor to share code between SkPngCodec and SkWebpCodec
* Disallow conversions to F16 with non-linear color spaces
* Fail to decode if we fail to create color xform
  (should be an assert soon).  We used to fallback on a
  legacy decode if we failed to create the transform.
* A bunch of name changes

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2319293003
==========

to

==========
Checking for valid colorType, alphaType, colorSpace in SkCodec

* Refactor to share code between SkPngCodec and SkWebpCodec
* Didn't end up sharing with SkJpegCodec but did refactor
  that code a bit
* Disallow conversions to F16 with non-linear color spaces
* Fail to decode if we fail to create color xform
  (should be an assert soon).  We used to fallback on a
  legacy decode if we failed to create the transform.
* A bunch of name changes

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2319293003
==========

[msarett, 2016-09-08 15:56:25]

Description was changed from

==========
Checking for valid colorType, alphaType, colorSpace in SkCodec

* Refactor to share code between SkPngCodec and SkWebpCodec
* Didn't end up sharing with SkJpegCodec but did refactor
  that code a bit
* Disallow conversions to F16 with non-linear color spaces
* Fail to decode if we fail to create color xform
  (should be an assert soon).  We used to fallback on a
  legacy decode if we failed to create the transform.
* A bunch of name changes

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2319293003
==========

to

==========
Checking for valid colorType, alphaType, colorSpace in SkCodec

* Refactor to share code between SkPngCodec and SkWebpCodec
* Didn't end up sharing with SkJpegCodec but did refactor
  that code a bit
* Disallow conversions to F16 with non-linear color spaces
* Fail to decode if we fail to create a SkColorSpaceXform
  (should be an assert soon).  We used to fallback on a
  legacy decode if we failed to create the transform.
* A bunch of name changes

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2319293003
==========

[msarett, 2016-09-08 15:58:42]

Patchset #1 (id:60001) has been deleted

[msarett, 2016-09-08 16:01:58]

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

[msarett, 2016-09-08 16:01:58]

https://codereview.chromium.org/2319293003/diff/80001/src/codec/SkJpegCodec.cpp
File src/codec/SkJpegCodec.cpp (right):

https://codereview.chromium.org/2319293003/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:641: // TODO: Can we assert this instead?
This can be an assert when Brian's CL lands:
https://codereview.chromium.org/2323003002/

[scroggo, 2016-09-08 16:33:11]

lgtm

https://codereview.chromium.org/2319293003/diff/80001/src/android/SkBitmapReg...
File src/android/SkBitmapRegionCodec.cpp (right):

https://codereview.chromium.org/2319293003/diff/80001/src/android/SkBitmapReg...
src/android/SkBitmapRegionCodec.cpp:139: sk_sp<SkColorSpace> colorSpace =
nullptr;
This looks to be unused

https://codereview.chromium.org/2319293003/diff/80001/src/codec/SkCodecPriv.h
File src/codec/SkCodecPriv.h (right):

https://codereview.chromium.org/2319293003/diff/80001/src/codec/SkCodecPriv.h...
src/codec/SkCodecPriv.h:112: * This is not synonymous with decoding in "legacy
mode".  Codecs should be able
I vote we use a different name. Conversion possible ignore color space? (With
underscores) Maybe too lengthy,...

https://codereview.chromium.org/2319293003/diff/80001/src/codec/SkJpegCodec.cpp
File src/codec/SkJpegCodec.cpp (right):

https://codereview.chromium.org/2319293003/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:668: bool needsColorXform = needs_color_xform(dstInfo,
this->getInfo());
Both callers check this first and otherwise don't use the boolean. Why not fold
it into the method?

[msarett, 2016-09-08 16:49:45]

Also went ahead and added SkASSERTs, Brian's change is about to land.

https://codereview.chromium.org/2319293003/diff/80001/src/android/SkBitmapReg...
File src/android/SkBitmapRegionCodec.cpp (right):

https://codereview.chromium.org/2319293003/diff/80001/src/android/SkBitmapReg...
src/android/SkBitmapRegionCodec.cpp:139: sk_sp<SkColorSpace> colorSpace =
nullptr;
On 2016/09/08 16:33:11, scroggo wrote:
> This looks to be unused

Ahh thanks.

https://codereview.chromium.org/2319293003/diff/80001/src/codec/SkCodecPriv.h
File src/codec/SkCodecPriv.h (right):

https://codereview.chromium.org/2319293003/diff/80001/src/codec/SkCodecPriv.h...
src/codec/SkCodecPriv.h:112: * This is not synonymous with decoding in "legacy
mode".  Codecs should be able
On 2016/09/08 16:33:11, scroggo wrote:
> I vote we use a different name. Conversion possible ignore color space? (With
> underscores) Maybe too lengthy,...

Works for me - also removed this part of the comment.

https://codereview.chromium.org/2319293003/diff/80001/src/codec/SkJpegCodec.cpp
File src/codec/SkJpegCodec.cpp (right):

https://codereview.chromium.org/2319293003/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:668: bool needsColorXform = needs_color_xform(dstInfo,
this->getInfo());
On 2016/09/08 16:33:11, scroggo wrote:
> Both callers check this first and otherwise don't use the boolean. Why not
fold
> it into the method?

Yes, sgtm.

[msarett, 2016-09-08 16:51:24]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 16:51:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 17:06:07]

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

[commit-bot: I haz the power, 2016-09-08 17:06:08]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-09-08 17:13:04]

The CQ bit was checked by msarett@google.com

[msarett, 2016-09-08 17:13:04]

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/2319293003/#ps100001
(title: "Response to comments")

[commit-bot: I haz the power, 2016-09-08 17:13:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 17:14:06]

Description was changed from

==========
Checking for valid colorType, alphaType, colorSpace in SkCodec

* Refactor to share code between SkPngCodec and SkWebpCodec
* Didn't end up sharing with SkJpegCodec but did refactor
  that code a bit
* Disallow conversions to F16 with non-linear color spaces
* Fail to decode if we fail to create a SkColorSpaceXform
  (should be an assert soon).  We used to fallback on a
  legacy decode if we failed to create the transform.
* A bunch of name changes

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2319293003
==========

to

==========
Checking for valid colorType, alphaType, colorSpace in SkCodec

* Refactor to share code between SkPngCodec and SkWebpCodec
* Didn't end up sharing with SkJpegCodec but did refactor
  that code a bit
* Disallow conversions to F16 with non-linear color spaces
* Fail to decode if we fail to create a SkColorSpaceXform
  (should be an assert soon).  We used to fallback on a
  legacy decode if we failed to create the transform.
* A bunch of name changes

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2319293003

Committed:
https://skia.googlesource.com/skia/+/7a9900d6d34e437bb24beb5524a1f6488ae138c9
==========

[commit-bot: I haz the power, 2016-09-08 17:14:07]

Committed patchset #2 (id:100001) as
https://skia.googlesource.com/skia/+/7a9900d6d34e437bb24beb5524a1f6488ae138c9

[msarett, 2016-09-08 17:54:14]

A revert of this CL (patchset #2 id:100001) has been created in
https://codereview.chromium.org/2328663002/ by msarett@google.com.

The reason for reverting is: Broken perf bots.

[msarett, 2016-09-08 18:24:53]

Description was changed from

==========
Checking for valid colorType, alphaType, colorSpace in SkCodec

* Refactor to share code between SkPngCodec and SkWebpCodec
* Didn't end up sharing with SkJpegCodec but did refactor
  that code a bit
* Disallow conversions to F16 with non-linear color spaces
* Fail to decode if we fail to create a SkColorSpaceXform
  (should be an assert soon).  We used to fallback on a
  legacy decode if we failed to create the transform.
* A bunch of name changes

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2319293003

Committed:
https://skia.googlesource.com/skia/+/7a9900d6d34e437bb24beb5524a1f6488ae138c9
==========

to

==========
Checking for valid colorType, alphaType, colorSpace in SkCodec

* Refactor to share code between SkPngCodec and SkWebpCodec
* Didn't end up sharing with SkJpegCodec but did refactor
  that code a bit
* Disallow conversions to F16 with non-linear color spaces
* Fail to decode if we fail to create a SkColorSpaceXform
  (should be an assert soon).  We used to fallback on a
  legacy decode if we failed to create the transform.
* A bunch of name changes

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2319293003

Committed:
https://skia.googlesource.com/skia/+/7a9900d6d34e437bb24beb5524a1f6488ae138c9
==========

[msarett, 2016-09-08 18:26:30]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 18:26:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 18:53:40]

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

[commit-bot: I haz the power, 2016-09-08 18:53:41]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-09-08 18:53:50]

The CQ bit was checked by msarett@google.com

[msarett, 2016-09-08 18:53:51]

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/2319293003/#ps120001
(title: "Fix nanobench")

[commit-bot: I haz the power, 2016-09-08 18:54:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 18:55:18]

Description was changed from

==========
Checking for valid colorType, alphaType, colorSpace in SkCodec

* Refactor to share code between SkPngCodec and SkWebpCodec
* Didn't end up sharing with SkJpegCodec but did refactor
  that code a bit
* Disallow conversions to F16 with non-linear color spaces
* Fail to decode if we fail to create a SkColorSpaceXform
  (should be an assert soon).  We used to fallback on a
  legacy decode if we failed to create the transform.
* A bunch of name changes

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2319293003

Committed:
https://skia.googlesource.com/skia/+/7a9900d6d34e437bb24beb5524a1f6488ae138c9
==========

to

==========
Checking for valid colorType, alphaType, colorSpace in SkCodec

* Refactor to share code between SkPngCodec and SkWebpCodec
* Didn't end up sharing with SkJpegCodec but did refactor
  that code a bit
* Disallow conversions to F16 with non-linear color spaces
* Fail to decode if we fail to create a SkColorSpaceXform
  (should be an assert soon).  We used to fallback on a
  legacy decode if we failed to create the transform.
* A bunch of name changes

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2319293003

Committed:
https://skia.googlesource.com/skia/+/7a9900d6d34e437bb24beb5524a1f6488ae138c9
Committed:
https://skia.googlesource.com/skia/+/2ecc35ffa5feb56cd088478589ba1e48382b2e1f
==========

[commit-bot: I haz the power, 2016-09-08 18:55:19]

Committed patchset #3 (id:120001) as
https://skia.googlesource.com/skia/+/2ecc35ffa5feb56cd088478589ba1e48382b2e1f