Use SkEncodedInfo in place of SkSwizzler::SrcConfig

Use SkEncodedInfo in place of SkSwizzler::SrcConfig

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1911613002

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

[msarett, 2016-04-20 21:21:18]

Description was changed from

==========
stuff


so far so good

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

to

==========
stuff


so far so good

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[msarett, 2016-04-20 21:21:50]

Description was changed from

==========
stuff


so far so good

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Use SkEncodedInfo in place of SkSwizzler::SrcConfig

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[msarett, 2016-04-20 21:30:56]

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

[msarett, 2016-04-20 21:30:57]

This used to be part of another CL (https://codereview.chromium.org/1835083002/)
that enabled non-platform (BGRA/RGBA) swizzles.  But that one got out of hand. 
So here is part 1.

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

https://codereview.chromium.org/1911613002/diff/1/src/codec/SkJpegCodec.cpp#n...
src/codec/SkJpegCodec.cpp:513: // libjpeg-turbo may have already performed color
conversion.  In this
This is a bit strange - since we let libjpeg-turbo do format conversion for us,
we need to pretend the "encoded info" is whatever we have already converted to.

Even worse, since we don't have a 565 encoded info, I'm using kUnknown to
indicate that conversion has already been done...

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

https://codereview.chromium.org/1911613002/diff/1/src/codec/SkSwizzler.cpp#ol...
src/codec/SkSwizzler.cpp:43: static void sample4(void* dst, const uint8_t* src,
int width, int bpp, int deltaSrc, int offset,
This opt will go back in, in the next CL.  For now, we use rgba->n32 and
bgra->n32.

[reed1, 2016-04-21 14:15:00]

https://codereview.chromium.org/1911613002/diff/1/include/codec/SkEncodedInfo.h
File include/codec/SkEncodedInfo.h (right):

https://codereview.chromium.org/1911613002/diff/1/include/codec/SkEncodedInfo...
include/codec/SkEncodedInfo.h:165: uint8_t bitsPerPixel() const {
Why do we offer bitsPer and bytesPer, when bits is always a multiple of 8?

[msarett, 2016-04-21 14:36:44]

Removed bytesPerPixel() from the public API.

https://codereview.chromium.org/1911613002/diff/1/include/codec/SkEncodedInfo.h
File include/codec/SkEncodedInfo.h (right):

https://codereview.chromium.org/1911613002/diff/1/include/codec/SkEncodedInfo...
include/codec/SkEncodedInfo.h:165: uint8_t bitsPerPixel() const {
On 2016/04/21 14:15:00, reed1 wrote:
> Why do we offer bitsPer and bytesPer, when bits is always a multiple of 8?

It's not always a multiple.

Though crashing through SkASSERT when clients ask for bytesPerPixel is a strange
behavior.

I've rewritten to only provide bitsPerPixel() and fixed the clients.

[scroggo, 2016-04-21 15:09:11]

lgtm

I like how this consolidates a lot of code into the swizzler!

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

https://codereview.chromium.org/1911613002/diff/1/src/codec/SkJpegCodec.cpp#n...
src/codec/SkJpegCodec.cpp:513: // libjpeg-turbo may have already performed color
conversion.  In this
On 2016/04/20 21:30:56, msarett wrote:
> This is a bit strange - since we let libjpeg-turbo do format conversion for
us,
> we need to pretend the "encoded info" is whatever we have already converted
to.
> 
> Even worse, since we don't have a 565 encoded info, I'm using kUnknown to
> indicate that conversion has already been done...

Instead of pretending kUnknown means something we actually know about, why not
have a separate Color? Maybe something like kPreswizzled? It's a little awkward,
since of course it's not preswizzled how we'll ask for it as encoded, but I
think it's better than abusing kUnknown.

A further TODO might be to investigate skipping the swizzler if it's
kPreswizzled, and decoding directly to the output memory. It does have some
complications/things to consider though:

- we may not know if we're going to sample until later
- introducing more branches in decoding may hurt performance
- if it does hurt performance, we may need to use separate (but similar
code-paths)
- how much of the time do people decode JPEG to 565?

https://codereview.chromium.org/1911613002/diff/20001/include/codec/SkEncoded...
File include/codec/SkEncodedInfo.h (left):

https://codereview.chromium.org/1911613002/diff/20001/include/codec/SkEncoded...
include/codec/SkEncodedInfo.h:106: SkASSERT(kOpaque_Alpha != alpha);
I suppose these are removed because e.g. a PNG might claim to have alpha but not
use it?

And did you find YUV images with alpha?

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

https://codereview.chromium.org/1911613002/diff/20001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:536: encodedInfo =
this->getEncodedInfo().makeColor(SkEncodedInfo::kUnknown_Color);
If you don't add a new color type for this, I think there should be a comment
explaining why you're using kUnknown here.

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

https://codereview.chromium.org/1911613002/diff/20001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:517: // supporting outputs from libpng that are less
than 8-bits per component.
Do you have a gut feeling whether that will be beneficial? I know you've done
some scraping to determine frequencies of some properties. Do you know if that
is a common case? Do you have an idea how much faster we could go by not forcing
libpng to convert first?

https://codereview.chromium.org/1911613002/diff/20001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1911613002/diff/20001/src/codec/SkSwizzler.cp...
src/codec/SkSwizzler.cpp:666: bool premultiply = (SkEncodedInfo::kOpaque_Alpha
!= encodedInfo.alpha()) &&
nit: could be const?

https://codereview.chromium.org/1911613002/diff/20001/src/codec/SkSwizzler.cp...
src/codec/SkSwizzler.cpp:739: // We assumes that the color table is
premultiplied and swizzled
assume*

[msarett, 2016-04-21 16:03:41]

I think adding kPreSwizzled makes things a lot cleaner.

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

https://codereview.chromium.org/1911613002/diff/1/src/codec/SkJpegCodec.cpp#n...
src/codec/SkJpegCodec.cpp:513: // libjpeg-turbo may have already performed color
conversion.  In this
On 2016/04/21 15:09:11, scroggo wrote:
> On 2016/04/20 21:30:56, msarett wrote:
> > This is a bit strange - since we let libjpeg-turbo do format conversion for
> us,
> > we need to pretend the "encoded info" is whatever we have already converted
> to.
> > 
> > Even worse, since we don't have a 565 encoded info, I'm using kUnknown to
> > indicate that conversion has already been done...
> 
> Instead of pretending kUnknown means something we actually know about, why not
> have a separate Color? Maybe something like kPreswizzled? It's a little
awkward,
> since of course it's not preswizzled how we'll ask for it as encoded, but I
> think it's better than abusing kUnknown.
> 

Yes I think your idea is much better than abusing kUnknown.  I've made this
change.

> A further TODO might be to investigate skipping the swizzler if it's
> kPreswizzled, and decoding directly to the output memory. It does have some
> complications/things to consider though:
> 

Agreed!  We always do skip the swizzler unless we are sampling or subsetting
(where the subsets don't align perfectly with jpeg blocks).

> - we may not know if we're going to sample until later
> - introducing more branches in decoding may hurt performance
> - if it does hurt performance, we may need to use separate (but similar
> code-paths)
> - how much of the time do people decode JPEG to 565?

I *think* 565 is rare.  Would be in favor of phasing that out.

https://codereview.chromium.org/1911613002/diff/20001/include/codec/SkEncoded...
File include/codec/SkEncodedInfo.h (left):

https://codereview.chromium.org/1911613002/diff/20001/include/codec/SkEncoded...
include/codec/SkEncodedInfo.h:106: SkASSERT(kOpaque_Alpha != alpha);
On 2016/04/21 15:09:11, scroggo wrote:
> I suppose these are removed because e.g. a PNG might claim to have alpha but
not
> use it?

Sorry should have commented on this.  These are removed because I used them
internally for jpegs that have already swizzled to RGBA/BGRA (and these are
opaque).

I can add these back in now that we are using kPreSwizzled.

> 
> And did you find YUV images with alpha?

Webp supports YUVA (has YUV planes and an alpha channel).

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

https://codereview.chromium.org/1911613002/diff/20001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:536: encodedInfo =
this->getEncodedInfo().makeColor(SkEncodedInfo::kUnknown_Color);
On 2016/04/21 15:09:11, scroggo wrote:
> If you don't add a new color type for this, I think there should be a comment
> explaining why you're using kUnknown here.

Using kPreSwizzled.

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

https://codereview.chromium.org/1911613002/diff/20001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:517: // supporting outputs from libpng that are less
than 8-bits per component.
On 2016/04/21 15:09:11, scroggo wrote:
> Do you have a gut feeling whether that will be beneficial? I know you've done
> some scraping to determine frequencies of some properties. Do you know if that
> is a common case? Do you have an idea how much faster we could go by not
forcing
> libpng to convert first?

I haven't measured either, but my gut feeling is "not very common" and "not too
much faster".  I think that's why it keeps sliding down on the priorities
list...

https://codereview.chromium.org/1911613002/diff/20001/src/codec/SkSwizzler.cpp
File src/codec/SkSwizzler.cpp (right):

https://codereview.chromium.org/1911613002/diff/20001/src/codec/SkSwizzler.cp...
src/codec/SkSwizzler.cpp:666: bool premultiply = (SkEncodedInfo::kOpaque_Alpha
!= encodedInfo.alpha()) &&
On 2016/04/21 15:09:11, scroggo wrote:
> nit: could be const?

Done.

https://codereview.chromium.org/1911613002/diff/20001/src/codec/SkSwizzler.cp...
src/codec/SkSwizzler.cpp:739: // We assumes that the color table is
premultiplied and swizzled
On 2016/04/21 15:09:11, scroggo wrote:
> assume*

Done.

[scroggo, 2016-04-21 16:12:17]

LGTM

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

https://codereview.chromium.org/1911613002/diff/20001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:517: // supporting outputs from libpng that are less
than 8-bits per component.
On 2016/04/21 16:03:41, msarett wrote:
> On 2016/04/21 15:09:11, scroggo wrote:
> > Do you have a gut feeling whether that will be beneficial? I know you've
done
> > some scraping to determine frequencies of some properties. Do you know if
that
> > is a common case? Do you have an idea how much faster we could go by not
> forcing
> > libpng to convert first?
> 
> I haven't measured either, but my gut feeling is "not very common" and "not
too
> much faster".  I think that's why it keeps sliding down on the priorities
> list...

Haha, good :)

[reed1, 2016-04-22 15:08:35]

https://codereview.chromium.org/1911613002/diff/60001/include/codec/SkEncoded...
File include/codec/SkEncodedInfo.h (right):

https://codereview.chromium.org/1911613002/diff/60001/include/codec/SkEncoded...
include/codec/SkEncodedInfo.h:80: static SkEncodedInfo Make(Color color, Alpha
alpha, int bitsPerComponent) {
Do we need to allow the public to create EncodedInfos with kPreSwizzled or
kUnknown ? That seems weird.

[msarett, 2016-04-22 16:13:26]

https://codereview.chromium.org/1911613002/diff/60001/include/codec/SkEncoded...
File include/codec/SkEncodedInfo.h (right):

https://codereview.chromium.org/1911613002/diff/60001/include/codec/SkEncoded...
include/codec/SkEncodedInfo.h:80: static SkEncodedInfo Make(Color color, Alpha
alpha, int bitsPerComponent) {
On 2016/04/22 15:08:35, reed1 wrote:
> Do we need to allow the public to create EncodedInfos with kPreSwizzled or
> kUnknown ? That seems weird.

Agreed that is weird.

We already disallow kUnknown (except in the default constructor case).

I have now disallowed kPreSwizzled by making SkJpegCodec a friend class and
adding a private setColor() function.

[reed1, 2016-04-22 18:17:01]

lgtm

[msarett, 2016-04-22 18:19:58]

The CQ bit was checked by msarett@google.com

[msarett, 2016-04-22 18:19:58]

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/1911613002/#ps80001
(title: "Create private setColor")

[commit-bot: I haz the power, 2016-04-22 18:20:06]

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

[commit-bot: I haz the power, 2016-04-22 18:29:07]

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

[commit-bot: I haz the power, 2016-04-22 18:29:08]

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, 2016-04-22 18:31:22]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2016-04-22 18:31:27]

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

[commit-bot: I haz the power, 2016-04-22 18:36:59]

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

[commit-bot: I haz the power, 2016-04-22 18:37:00]

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, 2016-04-22 20:07:01]

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

[commit-bot: I haz the power, 2016-04-22 20:07:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1911613002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1911613002/100001

[commit-bot: I haz the power, 2016-04-22 20:17:06]

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

[commit-bot: I haz the power, 2016-04-22 20:17:07]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-04-22 20:17:36]

The CQ bit was checked by msarett@google.com

[msarett, 2016-04-22 20:17:36]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com,
reed@google.com
Link to the patchset: https://codereview.chromium.org/1911613002/#ps100001
(title: "Fix bmp bug")

[commit-bot: I haz the power, 2016-04-22 20:17:41]

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

[commit-bot: I haz the power, 2016-04-22 20:18:39]

Description was changed from

==========
Use SkEncodedInfo in place of SkSwizzler::SrcConfig

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Use SkEncodedInfo in place of SkSwizzler::SrcConfig

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

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

[commit-bot: I haz the power, 2016-04-22 20:18:40]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/a45a668fa57eb968e24f379eceb2e56324e2cca2