Add BGRA as input format to SkColorSpaceXform

Add BGRA as input format to SkColorSpaceXform

This is immediately useful for webp and I think it's a fair guess
that BGRA src formats are not uncommon.

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

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

[msarett, 2016-09-22 13:10:36]

Description was changed from

==========
Add BGRA as input format to SkColorSpaceXform

This is immediately useful for webp and I think it's a fair guess
that BGRA src formats are not uncommon.

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

to

==========
Add BGRA as input format to SkColorSpaceXform

This is immediately useful for webp and I think it's a fair guess
that BGRA src formats are not uncommon.

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

[msarett, 2016-09-22 13:41:45]

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

[msarett, 2016-09-22 13:41:46]

https://codereview.chromium.org/2353363008/diff/40001/src/core/SkColorSpaceXf...
File src/core/SkColorSpaceXform.h (right):

https://codereview.chromium.org/2353363008/diff/40001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXform.h:15: class SkColorSpaceXform : SkNoncopyable {
Particularly interested in comments on this API.  Thinking about making it
public soon...

https://codereview.chromium.org/2353363008/diff/40001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXform.h:49: virtual void apply(void* dst, const uint32_t*
src, int len, ColorFormat dstColorFormat,
Might make this return a bool in a follow-up.

[mtklein, 2016-09-22 13:57:00]

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

https://codereview.chromium.org/2353363008/diff/40001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:497: static constexpr SkColorSpaceXform::ColorFormat
srcColorFormat =
Usually this sort of thing is fine spelled "const".  It's probably best to leave
both static and constexpr for where their specific meanings are required...
sometimes you accidentally end up creating something unintentional with them
(especially static).

https://codereview.chromium.org/2353363008/diff/40001/src/core/SkColorSpaceXf...
File src/core/SkColorSpaceXform.cpp (right):

https://codereview.chromium.org/2353363008/diff/40001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXform.cpp:624: #define SET_SHIFTS               \
Let's see if we can get close to what you want here without a macro...

    int kRShift, kGShift = 8, kBShift, kAShift = 24;
    set_rb_shifts(kOrder, &kRShift, &kBShift);

?

https://codereview.chromium.org/2353363008/diff/40001/src/core/SkColorSpaceXf...
File src/core/SkColorSpaceXform.h (right):

https://codereview.chromium.org/2353363008/diff/40001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXform.h:49: virtual void apply(void* dst, const uint32_t*
src, int len, ColorFormat dstColorFormat,
On 2016/09/22 13:41:45, msarett wrote:
> Might make this return a bool in a follow-up.

But which one??  true?  false?  so many choices.

No seriously, what's the bool mean?  Success?  Why would we want to fail?

[msarett, 2016-09-22 17:06:55]

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

https://codereview.chromium.org/2353363008/diff/40001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:497: static constexpr SkColorSpaceXform::ColorFormat
srcColorFormat =
On 2016/09/22 13:57:00, mtklein wrote:
> Usually this sort of thing is fine spelled "const".  It's probably best to
leave
> both static and constexpr for where their specific meanings are required...
> sometimes you accidentally end up creating something unintentional with them
> (especially static).

Done.

https://codereview.chromium.org/2353363008/diff/40001/src/core/SkColorSpaceXf...
File src/core/SkColorSpaceXform.cpp (right):

https://codereview.chromium.org/2353363008/diff/40001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXform.cpp:624: #define SET_SHIFTS               \
On 2016/09/22 13:57:00, mtklein wrote:
> Let's see if we can get close to what you want here without a macro...
> 
>     int kRShift, kGShift = 8, kBShift, kAShift = 24;
>     set_rb_shifts(kOrder, &kRShift, &kBShift);
> 
> ?

Done.

https://codereview.chromium.org/2353363008/diff/40001/src/core/SkColorSpaceXf...
File src/core/SkColorSpaceXform.h (right):

https://codereview.chromium.org/2353363008/diff/40001/src/core/SkColorSpaceXf...
src/core/SkColorSpaceXform.h:49: virtual void apply(void* dst, const uint32_t*
src, int len, ColorFormat dstColorFormat,
On 2016/09/22 13:57:00, mtklein wrote:
> On 2016/09/22 13:41:45, msarett wrote:
> > Might make this return a bool in a follow-up.
> 
> But which one??  true?  false?  so many choices.
> 
> No seriously, what's the bool mean?  Success?  Why would we want to fail?

We currently assert(false) if:
(1) srcColorFormat isn't supported.
(2) They request kUnknown_SkAlphaType
(3) They request F16 or F32 when the dstSpace is not linear.

Maybe it's fine to assert (or maybe we should return false)?

I think most clients would end up ignoring the return value (or asserting that
it's true).  Maybe that's a sign that this is fine as is?

[mtklein_C, 2016-09-22 18:07:12]

On 2016/09/22 at 17:06:55, msarett wrote:
> https://codereview.chromium.org/2353363008/diff/40001/src/codec/SkPngCodec.cpp
> File src/codec/SkPngCodec.cpp (right):
> 
>
https://codereview.chromium.org/2353363008/diff/40001/src/codec/SkPngCodec.cp...
> src/codec/SkPngCodec.cpp:497: static constexpr SkColorSpaceXform::ColorFormat
srcColorFormat =
> On 2016/09/22 13:57:00, mtklein wrote:
> > Usually this sort of thing is fine spelled "const".  It's probably best to
leave
> > both static and constexpr for where their specific meanings are required...
> > sometimes you accidentally end up creating something unintentional with them
> > (especially static).
> 
> Done.
> 
>
https://codereview.chromium.org/2353363008/diff/40001/src/core/SkColorSpaceXf...
> File src/core/SkColorSpaceXform.cpp (right):
> 
>
https://codereview.chromium.org/2353363008/diff/40001/src/core/SkColorSpaceXf...
> src/core/SkColorSpaceXform.cpp:624: #define SET_SHIFTS               \
> On 2016/09/22 13:57:00, mtklein wrote:
> > Let's see if we can get close to what you want here without a macro...
> > 
> >     int kRShift, kGShift = 8, kBShift, kAShift = 24;
> >     set_rb_shifts(kOrder, &kRShift, &kBShift);
> > 
> > ?
> 
> Done.
> 
>
https://codereview.chromium.org/2353363008/diff/40001/src/core/SkColorSpaceXf...
> File src/core/SkColorSpaceXform.h (right):
> 
>
https://codereview.chromium.org/2353363008/diff/40001/src/core/SkColorSpaceXf...
> src/core/SkColorSpaceXform.h:49: virtual void apply(void* dst, const uint32_t*
src, int len, ColorFormat dstColorFormat,
> On 2016/09/22 13:57:00, mtklein wrote:
> > On 2016/09/22 13:41:45, msarett wrote:
> > > Might make this return a bool in a follow-up.
> > 
> > But which one??  true?  false?  so many choices.
> > 
> > No seriously, what's the bool mean?  Success?  Why would we want to fail?
> 
> We currently assert(false) if:
> (1) srcColorFormat isn't supported.
> (2) They request kUnknown_SkAlphaType
> (3) They request F16 or F32 when the dstSpace is not linear.
> 
> Maybe it's fine to assert (or maybe we should return false)?
> 
> I think most clients would end up ignoring the return value (or asserting that
it's true).  Maybe that's a sign that this is fine as is?

Yeah, if most clients will ignore or assert the call succeeds, let's assert the
call succeeds internally.
If the assertion fails, the solution is to email you asking for support for the
transformation they wanted, right?  There's no other good way to do it?

[mtklein_C, 2016-09-22 18:08:26]

mtklein@chromium.org changed reviewers:
+ mtklein@chromium.org

[mtklein_C, 2016-09-22 18:08:27]

lgtm

[msarett, 2016-09-22 18:21:46]

On 2016/09/22 18:07:12, mtklein_C wrote:
> On 2016/09/22 at 17:06:55, msarett wrote:
> >
https://codereview.chromium.org/2353363008/diff/40001/src/codec/SkPngCodec.cpp
> > File src/codec/SkPngCodec.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2353363008/diff/40001/src/codec/SkPngCodec.cp...
> > src/codec/SkPngCodec.cpp:497: static constexpr
SkColorSpaceXform::ColorFormat
> srcColorFormat =
> > On 2016/09/22 13:57:00, mtklein wrote:
> > > Usually this sort of thing is fine spelled "const".  It's probably best to
> leave
> > > both static and constexpr for where their specific meanings are
required...
> > > sometimes you accidentally end up creating something unintentional with
them
> > > (especially static).
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2353363008/diff/40001/src/core/SkColorSpaceXf...
> > File src/core/SkColorSpaceXform.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2353363008/diff/40001/src/core/SkColorSpaceXf...
> > src/core/SkColorSpaceXform.cpp:624: #define SET_SHIFTS               \
> > On 2016/09/22 13:57:00, mtklein wrote:
> > > Let's see if we can get close to what you want here without a macro...
> > > 
> > >     int kRShift, kGShift = 8, kBShift, kAShift = 24;
> > >     set_rb_shifts(kOrder, &kRShift, &kBShift);
> > > 
> > > ?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2353363008/diff/40001/src/core/SkColorSpaceXf...
> > File src/core/SkColorSpaceXform.h (right):
> > 
> >
>
https://codereview.chromium.org/2353363008/diff/40001/src/core/SkColorSpaceXf...
> > src/core/SkColorSpaceXform.h:49: virtual void apply(void* dst, const
uint32_t*
> src, int len, ColorFormat dstColorFormat,
> > On 2016/09/22 13:57:00, mtklein wrote:
> > > On 2016/09/22 13:41:45, msarett wrote:
> > > > Might make this return a bool in a follow-up.
> > > 
> > > But which one??  true?  false?  so many choices.
> > > 
> > > No seriously, what's the bool mean?  Success?  Why would we want to fail?
> > 
> > We currently assert(false) if:
> > (1) srcColorFormat isn't supported.
> > (2) They request kUnknown_SkAlphaType
> > (3) They request F16 or F32 when the dstSpace is not linear.
> > 
> > Maybe it's fine to assert (or maybe we should return false)?
> > 
> > I think most clients would end up ignoring the return value (or asserting
that
> it's true).  Maybe that's a sign that this is fine as is?
> 
> Yeah, if most clients will ignore or assert the call succeeds, let's assert
the
> call succeeds internally.
> If the assertion fails, the solution is to email you asking for support for
the
> transformation they wanted, right?  There's no other good way to do it?

Yeah that seems fine.  If they hit an assert, they are probably using the API
wrong.

[msarett, 2016-09-22 18:21:54]

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

[commit-bot: I haz the power, 2016-09-22 18:22:03]

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-22 19:18:58]

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

[commit-bot: I haz the power, 2016-09-22 19:19:00]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-09-22 19:19:20]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2016-09-22 19:19:28]

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-22 19:37:09]

Description was changed from

==========
Add BGRA as input format to SkColorSpaceXform

This is immediately useful for webp and I think it's a fair guess
that BGRA src formats are not uncommon.

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

to

==========
Add BGRA as input format to SkColorSpaceXform

This is immediately useful for webp and I think it's a fair guess
that BGRA src formats are not uncommon.

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

Committed:
https://skia.googlesource.com/skia/+/cf7b877d62537672b67449bc96858cc1262be5f8
==========

[commit-bot: I haz the power, 2016-09-22 19:37:10]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/cf7b877d62537672b67449bc96858cc1262be5f8