Add SkEncodedInfo to report properties of encoded image data

Add SkEncodedInfo to report properties of encoded image data

All this does is build an SkEncodedInfo for each codec, and
then convert it to an SkImageInfo.

In future steps I intend to:
(1) Use SkEncodedInfo in place of SrcConfig in SkSwizzler.
(2) Support more conversions in SkSwizzler (non-native
BGRA/RGBA, 16-bit components, float, fixed point)
(3) Investigate optimizing conversions from encoded data
to linear color spaces.

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

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

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

[msarett, 2016-03-21 21:53:53]

Description was changed from

==========
It compiles

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

to

==========
It compiles

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

[msarett, 2016-03-21 22:00:03]

Patchset #1 (id:1) has been deleted

[msarett, 2016-03-21 22:23:40]

Patchset #1 (id:20001) has been deleted

[msarett, 2016-03-22 17:10:56]

Patchset #1 (id:40001) has been deleted

[msarett, 2016-03-22 17:42:12]

Description was changed from

==========
It compiles

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

to

==========
Add SkEncodedInfo to report properties of encoded image data

All this does is build an SkEncodedInfo for each codec, and
then convert it to an SkImageInfo.

In future steps I intend to:
(1) Use SkEncodedInfo in place of SrcConfig in SkSwizzler.
(2) Support more conversions in SkSwizzler (non-native
BGRA/RGBA, 16-bit components, float, fixed point)
(3) Investigate optimizing conversions from encoded data
to linear color spaces.

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

[msarett, 2016-03-22 17:42:21]

Patchset #1 (id:60001) has been deleted

[msarett, 2016-03-22 18:55:06]

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

[reed1, 2016-03-22 19:12:44]

I like the new struct.

No need to broadcast if the encoded form is table-based?

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

https://codereview.chromium.org/1820073002/diff/80001/include/codec/SkEncoded...
include/codec/SkEncodedInfo.h:78: SkImageInfo makeImageInfo() const {
why do we have this?

[msarett, 2016-03-22 20:59:07]

On 2016/03/22 19:12:44, reed1 wrote:
> I like the new struct.
> 
> No need to broadcast if the encoded form is table-based?
> 

We use kPalette_Color for this.

>
https://codereview.chromium.org/1820073002/diff/80001/include/codec/SkEncoded...
> File include/codec/SkEncodedInfo.h (right):
> 
>
https://codereview.chromium.org/1820073002/diff/80001/include/codec/SkEncoded...
> include/codec/SkEncodedInfo.h:78: SkImageInfo makeImageInfo() const {
> why do we have this?

We have tons of clients that call codec->getInfo(), so we each codec needs to be
able to report an SkImageInfo somehow.  This is useful for clients that just
want to decode with the "default" settings.

I thought it made sense to build an SkEncodedInfo for each codec and have a
function that converts the SkEncodedInfo to a "default" SkImageInfo.  Maybe this
function should be renamed?

[scroggo, 2016-03-23 14:48:50]

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

https://codereview.chromium.org/1820073002/diff/80001/include/codec/SkEncoded...
include/codec/SkEncodedInfo.h:16: enum Alpha {
Are these "Alpha" and "Color" (instead of "AlphaType", "ColorType") to
distinguish from SkAlphaType and SkColorType? "Type" seems to make sense here to
me.

https://codereview.chromium.org/1820073002/diff/80001/include/codec/SkEncoded...
include/codec/SkEncodedInfo.h:34: // PNG, WBMP
Isn't WBMP only Gray?

https://codereview.chromium.org/1820073002/diff/80001/include/codec/SkEncoded...
include/codec/SkEncodedInfo.h:47: kBGRX_Color,
X means we ignore the alpha value, right?

https://codereview.chromium.org/1820073002/diff/80001/include/codec/SkEncoded...
include/codec/SkEncodedInfo.h:57: kInvertedCMYK_Color,
Maybe a note here about how the spec disagrees with what is used in practice?
(Isn't that what we determined?)

https://codereview.chromium.org/1820073002/diff/80001/include/codec/SkEncoded...
include/codec/SkEncodedInfo.h:78: SkImageInfo makeImageInfo() const {
On 2016/03/22 19:12:44, reed1 wrote:
> why do we have this?

To implement SkCodec::getInfo(). Maybe we only need it for a transition, but I
can imagine that we want it to make it simple for the client to choose. This
gives them a good guess of a logical choice if they don't want to consider all
of the real possibilities.

https://codereview.chromium.org/1820073002/diff/80001/include/codec/SkEncoded...
include/codec/SkEncodedInfo.h:81: SkASSERT(kOpaque_Alpha == fAlpha);
Would it make sense to put all these asserts into the constructor? (I guess at
the moment, makeImageInfo() will always be called.)

https://codereview.chromium.org/1820073002/diff/80001/include/codec/SkEncoded...
include/codec/SkEncodedInfo.h:92: case kRGB_Color:
For RGB(A), should we instead return kRGBA_8888_SkColorType, and
kBGRA_8888_SkColorType for BGR(A)? Does that depend on us supporting the
non-native 32 bit swizzle?

https://codereview.chromium.org/1820073002/diff/80001/include/codec/SkEncoded...
include/codec/SkEncodedInfo.h:117: , fColor(kRGBA_Color)
The default constructor for SkImageInfo creates something that is invalid
(kUnknown_...) (although it *does* have a legitimate color profile type, which
does not have an Unknown option). Should this one be similar?

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

https://codereview.chromium.org/1820073002/diff/80001/src/codec/SkBmpCodec.cp...
src/codec/SkBmpCodec.cpp:426: uint8_t bitsPerComponent = SkTMin((uint16_t) 8,
bitsPerPixel);
It seems odd that you set bitsPerComponent to min(bitsPerPixel, 8). Might be
worth a comment? (e.g. <= 8 are palette; >8 are 8 bitsPerComponent)

https://codereview.chromium.org/1820073002/diff/80001/src/codec/SkBmpCodec.cp...
src/codec/SkBmpCodec.cpp:509: // Masked bmps are not a great fit for
SkEncodedInfo, since they have
Should SkEncodedInfo adapt to support masked bmps? I thought that bmp had some
cases that weren't covered, but I couldn't remember them. I guess that whatever
info would be specific to mask bmp would not make a client decide to treat it
differently than if we just choose types that are already there?

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

https://codereview.chromium.org/1820073002/diff/80001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:120: , fSrcInfo(info.makeImageInfo())
Alternatively, we could lazily create this when asked for. We would need to
change getInfo() to stop returning a reference, and how SkCodecImageGenerater
deals with it, so it would be a bigger change. If we do go that route, it should
be its own change, I think.

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

https://codereview.chromium.org/1820073002/diff/80001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:216: SkEncodedInfo info =
SkEncodedInfo::Make(size.width(), size.height(),
Should Make have an option to take an SkISize?

https://codereview.chromium.org/1820073002/diff/80001/src/codec/SkJpegDecoder...
File src/codec/SkJpegDecoderMgr.cpp (right):

https://codereview.chromium.org/1820073002/diff/80001/src/codec/SkJpegDecoder...
src/codec/SkJpegDecoderMgr.cpp:50: return SkEncodedInfo::kYUV_Color;
What types fall into the default case? Why not be explicit?

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

https://codereview.chromium.org/1820073002/diff/80001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:350: if (bitDepth < 8) {
I think eventually we'll want to just use the raw bit depth and use the
SkSwizzler. But for now, don't you want to the SkEncodedInfo to report a
bitDepth of 8, since png_set_packing will bump it up to 8?

Similarly, bitDepth may be 16, but we already called strip_16, so the
SkEncodedInfo should say 8, right?

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

https://codereview.chromium.org/1820073002/diff/80001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:67: SkEncodedInfo::Alpha alpha =
SkToBool(features.has_alpha) ? SkEncodedInfo::kUnpremul_Alpha :
How does alpha work for a YUV image? Is "unpremul" descriptive here? Or should
it have a separate value?

https://codereview.chromium.org/1820073002/diff/80001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:69: *info = SkEncodedInfo::Make(features.width,
features.height, color, alpha, 8);
Similarly, is 8 bits per component meaningful here?

[msarett, 2016-03-24 16:20:44]

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

https://codereview.chromium.org/1820073002/diff/80001/include/codec/SkEncoded...
include/codec/SkEncodedInfo.h:16: enum Alpha {
On 2016/03/23 14:48:50, scroggo wrote:
> Are these "Alpha" and "Color" (instead of "AlphaType", "ColorType") to
> distinguish from SkAlphaType and SkColorType? "Type" seems to make sense here
to
> me.

I agree that Type makes sense.

The reason I left it off is because I made these enums part of SkEncodedInfo.  I
thought that "SkEncodedInfo::Color" was clear without adding more wordiness. 
And also being different than SkColorType is nice.

I don't feel strongly though.  Happy to change.

https://codereview.chromium.org/1820073002/diff/80001/include/codec/SkEncoded...
include/codec/SkEncodedInfo.h:34: // PNG, WBMP
On 2016/03/23 14:48:50, scroggo wrote:
> Isn't WBMP only Gray?

Yes.  Making this more clear.

https://codereview.chromium.org/1820073002/diff/80001/include/codec/SkEncoded...
include/codec/SkEncodedInfo.h:47: kBGRX_Color,
On 2016/03/23 14:48:49, scroggo wrote:
> X means we ignore the alpha value, right?

Yes.  Alpha is ignored, image is always opaque.

https://codereview.chromium.org/1820073002/diff/80001/include/codec/SkEncoded...
include/codec/SkEncodedInfo.h:57: kInvertedCMYK_Color,
On 2016/03/23 14:48:50, scroggo wrote:
> Maybe a note here about how the spec disagrees with what is used in practice?
> (Isn't that what we determined?)

sgtm

Adding a comment.

https://codereview.chromium.org/1820073002/diff/80001/include/codec/SkEncoded...
include/codec/SkEncodedInfo.h:81: SkASSERT(kOpaque_Alpha == fAlpha);
On 2016/03/23 14:48:50, scroggo wrote:
> Would it make sense to put all these asserts into the constructor? (I guess at
> the moment, makeImageInfo() will always be called.)

Yes I think so.  Let's not depend on this being called.

https://codereview.chromium.org/1820073002/diff/80001/include/codec/SkEncoded...
include/codec/SkEncodedInfo.h:92: case kRGB_Color:
On 2016/03/23 14:48:49, scroggo wrote:
> For RGB(A), should we instead return kRGBA_8888_SkColorType, and
> kBGRA_8888_SkColorType for BGR(A)? Does that depend on us supporting the
> non-native 32 bit swizzle?

I think we should, but maybe not yet.  I think this change would break
SkImageGenerator (and maybe other places) because they just use the color type
that we recommend.

Without supporting the non-native swizzle, the decode will just fail.  If we
start supporting the non-native swizzle, then they'll get swapped RB colors.

https://codereview.chromium.org/1820073002/diff/80001/include/codec/SkEncoded...
include/codec/SkEncodedInfo.h:117: , fColor(kRGBA_Color)
On 2016/03/23 14:48:50, scroggo wrote:
> The default constructor for SkImageInfo creates something that is invalid
> (kUnknown_...) (although it *does* have a legitimate color profile type, which
> does not have an Unknown option). Should this one be similar?

Yeah I think so.  Adding kUnknown_Color and kUnknown_Alpha.

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

https://codereview.chromium.org/1820073002/diff/80001/src/codec/SkBmpCodec.cp...
src/codec/SkBmpCodec.cpp:426: uint8_t bitsPerComponent = SkTMin((uint16_t) 8,
bitsPerPixel);
On 2016/03/23 14:48:50, scroggo wrote:
> It seems odd that you set bitsPerComponent to min(bitsPerPixel, 8). Might be
> worth a comment? (e.g. <= 8 are palette; >8 are 8 bitsPerComponent)

Agreed.  I think maybe it's clearer to handle it in the switch statement.

https://codereview.chromium.org/1820073002/diff/80001/src/codec/SkBmpCodec.cp...
src/codec/SkBmpCodec.cpp:509: // Masked bmps are not a great fit for
SkEncodedInfo, since they have
On 2016/03/23 14:48:50, scroggo wrote:
> Should SkEncodedInfo adapt to support masked bmps? I thought that bmp had some
> cases that weren't covered, but I couldn't remember them. I guess that
whatever
> info would be specific to mask bmp would not make a client decide to treat it
> differently than if we just choose types that are already there?

I thought about 3 options:
(1) Create all of the modes we might see in masked bmps...

kGRB, kGBR, kRBG, kBRG (also other variations including A) with values of
bitsPerComponent for each component (since they don't have to be the same).

I think this is pretty clearly not worth it...it's a lot of extra complexity in
our struct without knowing if there are any clients who would use this info or
care.

Additionally, we would have to interpret the masks to figure all of this out.

(2) Add a kMask_Color to SkEncodedInfo

I almost did this...  I ended up not doing it because it's not a great fit with
the others.  Ex: we can't tell the number of components from the name, we still
have a problem with how to set bits per component.

Still, this might be worth it.  The client could recognize that this is a
"weird" bmp.  It could be useful to report the max/min bits per component.

(3) Include mask bmps with kBGR/kBGRA and handle them differently without
telling the client.

This seems easiest right now.  Especially because we don't really know what the
client wants or if they want anything.  I don't mind having bmp being a weird
special case.  We can always change this decision later.

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

https://codereview.chromium.org/1820073002/diff/80001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:120: , fSrcInfo(info.makeImageInfo())
On 2016/03/23 14:48:50, scroggo wrote:
> Alternatively, we could lazily create this when asked for. We would need to
> change getInfo() to stop returning a reference, and how SkCodecImageGenerater
> deals with it, so it would be a bigger change. If we do go that route, it
should
> be its own change, I think.

SGTM.

The reason that I did it this way is that, when I'm the client, I call getInfo()
over and over again (expecting it to be free).  I didn't want to force us to
create a new SkImageInfo on each call.

Another option would be to create an SkImageInfo on the first call and reuse it
on subsequent calls, but that may be unnecessarily complex.

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

https://codereview.chromium.org/1820073002/diff/80001/src/codec/SkGifCodec.cp...
src/codec/SkGifCodec.cpp:216: SkEncodedInfo info =
SkEncodedInfo::Make(size.width(), size.height(),
On 2016/03/23 14:48:50, scroggo wrote:
> Should Make have an option to take an SkISize?

Yes, why not!

https://codereview.chromium.org/1820073002/diff/80001/src/codec/SkJpegDecoder...
File src/codec/SkJpegDecoderMgr.cpp (right):

https://codereview.chromium.org/1820073002/diff/80001/src/codec/SkJpegDecoder...
src/codec/SkJpegDecoderMgr.cpp:50: return SkEncodedInfo::kYUV_Color;
On 2016/03/23 14:48:50, scroggo wrote:
> What types fall into the default case? Why not be explicit?

Other JPEG_COLOR_SPACES include RGBA, BGRA, 565 etc. but these should never the
be the encoded color_space.  They would indicate a bug in libjpeg-turbo.

The only other "valid" value would be JCS_UNKNOWN.  And we would get this if
there is a really strange number of components 2, 5, 6...

In this case the decode will fail because we cannot do color conversion (unless
we call jpeg_read_raw_data(), which we won't, because we only use this for YUV
images).

Still I should handle this more clearly and fail faster...

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

https://codereview.chromium.org/1820073002/diff/80001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:350: if (bitDepth < 8) {
On 2016/03/23 14:48:50, scroggo wrote:
> I think eventually we'll want to just use the raw bit depth and use the
> SkSwizzler. But for now, don't you want to the SkEncodedInfo to report a
> bitDepth of 8, since png_set_packing will bump it up to 8?
> 
> Similarly, bitDepth may be 16, but we already called strip_16, so the
> SkEncodedInfo should say 8, right?

Thanks I completely missed this...

Yes, let's report 8 for now, and we'll start saying 1/2/4/8/16 when we can
handle it.

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

https://codereview.chromium.org/1820073002/diff/80001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:67: SkEncodedInfo::Alpha alpha =
SkToBool(features.has_alpha) ? SkEncodedInfo::kUnpremul_Alpha :
On 2016/03/23 14:48:50, scroggo wrote:
> How does alpha work for a YUV image? Is "unpremul" descriptive here? Or should
> it have a separate value?

Thanks for questioning this.  I dug a little deeper into the webp format.  Turns
out kUnpremul and 8 bits per component are correct, but I've gotten some other
stuff wrong...

https://codereview.chromium.org/1820073002/diff/80001/src/codec/SkWebpCodec.c...
src/codec/SkWebpCodec.cpp:69: *info = SkEncodedInfo::Make(features.width,
features.height, color, alpha, 8);
On 2016/03/23 14:48:50, scroggo wrote:
> Similarly, is 8 bits per component meaningful here?

Acknowledged.

[msarett, 2016-03-24 16:21:40]

https://codereview.chromium.org/1820073002/diff/100001/src/codec/SkWebpCodec.cpp
File src/codec/SkWebpCodec.cpp (right):

https://codereview.chromium.org/1820073002/diff/100001/src/codec/SkWebpCodec....
src/codec/SkWebpCodec.cpp:88: // FIXME: Should we check the has_alpha flag here?
 It looks
Forgot to mention that I emailed webp-discuss about this.  AFAICT the spec is
unclear.

[scroggo, 2016-03-24 20:51:04]

lgtm

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

https://codereview.chromium.org/1820073002/diff/80001/src/codec/SkBmpCodec.cp...
src/codec/SkBmpCodec.cpp:509: // Masked bmps are not a great fit for
SkEncodedInfo, since they have
On 2016/03/24 16:20:44, msarett wrote:
> On 2016/03/23 14:48:50, scroggo wrote:
> > Should SkEncodedInfo adapt to support masked bmps? I thought that bmp had
some
> > cases that weren't covered, but I couldn't remember them. I guess that
> whatever
> > info would be specific to mask bmp would not make a client decide to treat
it
> > differently than if we just choose types that are already there?
> 
> I thought about 3 options:
> (1) Create all of the modes we might see in masked bmps...
> 
> kGRB, kGBR, kRBG, kBRG (also other variations including A) with values of
> bitsPerComponent for each component (since they don't have to be the same).
> 
> I think this is pretty clearly not worth it...it's a lot of extra complexity
in
> our struct without knowing if there are any clients who would use this info or
> care.
> 
> Additionally, we would have to interpret the masks to figure all of this out.
> 
> (2) Add a kMask_Color to SkEncodedInfo
> 
> I almost did this...  I ended up not doing it because it's not a great fit
with
> the others.  Ex: we can't tell the number of components from the name, we
still
> have a problem with how to set bits per component.
> 
> Still, this might be worth it.  The client could recognize that this is a
> "weird" bmp.  It could be useful to report the max/min bits per component.
> 
> (3) Include mask bmps with kBGR/kBGRA and handle them differently without
> telling the client.
> 
> This seems easiest right now.  Especially because we don't really know what
the
> client wants or if they want anything.  I don't mind having bmp being a weird
> special case.  We can always change this decision later.

I'm also fine with Mask BMP being a weird special case.

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

https://codereview.chromium.org/1820073002/diff/100001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:216: if (!decoderMgr->getEncodedColor(&color)) {
Alternatively, this could return kUnknown on failure.

https://codereview.chromium.org/1820073002/diff/100001/src/codec/SkWebpCodec.cpp
File src/codec/SkWebpCodec.cpp (right):

https://codereview.chromium.org/1820073002/diff/100001/src/codec/SkWebpCodec....
src/codec/SkWebpCodec.cpp:81: color = SkToBool(features.has_alpha) ?
SkEncodedInfo::kYUVA_Color :
nit: I would find this a little clearer as

if (has_alpha) {
  color = 
  alpha = 
} else {
  color = 
  alpha = 
}

[msarett, 2016-03-24 22:42:07]

Mike, any thoughts on the API?

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

https://codereview.chromium.org/1820073002/diff/100001/src/codec/SkJpegCodec....
src/codec/SkJpegCodec.cpp:216: if (!decoderMgr->getEncodedColor(&color)) {
On 2016/03/24 20:51:03, scroggo wrote:
> Alternatively, this could return kUnknown on failure.

You're right, I like that better.

https://codereview.chromium.org/1820073002/diff/100001/src/codec/SkWebpCodec.cpp
File src/codec/SkWebpCodec.cpp (right):

https://codereview.chromium.org/1820073002/diff/100001/src/codec/SkWebpCodec....
src/codec/SkWebpCodec.cpp:81: color = SkToBool(features.has_alpha) ?
SkEncodedInfo::kYUVA_Color :
On 2016/03/24 20:51:04, scroggo wrote:
> nit: I would find this a little clearer as
> 
> if (has_alpha) {
>   color = 
>   alpha = 
> } else {
>   color = 
>   alpha = 
> }

Done.

[msarett, 2016-04-06 18:59:46]

Mike, would you mind taking a look?

[reed1, 2016-04-07 15:08:01]

Do we need to also pack width/height in this extra info, or would the expanded
color/alpha info be sufficient?

https://codereview.chromium.org/1820073002/diff/120001/include/codec/SkEncode...
File include/codec/SkEncodedInfo.h (right):

https://codereview.chromium.org/1820073002/diff/120001/include/codec/SkEncode...
include/codec/SkEncodedInfo.h:20: // Each pixel is either fully opaque or fully
transparent.
Are there no native premul image formats (e.g. webp?)

[msarett, 2016-04-07 15:24:49]

"Do we need to also pack width/height in this extra info, or would the expanded
color/alpha info be sufficient?"

Under my current implementation, width/height are necessary
because NewFromStream creates an SkEncodedInfo and we use
that to build the SkImageInfo.

I think can rework this to pass width/height to the SkCodec
constructor and keep those fields out of SkEncodedInfo.

I kind of think width/height "fit" in SkEncodedInfo, but we
might as well keep it lighter since they are already in
SkImageInfo and therefore not necessary.

https://codereview.chromium.org/1820073002/diff/120001/include/codec/SkEncode...
File include/codec/SkEncodedInfo.h (right):

https://codereview.chromium.org/1820073002/diff/120001/include/codec/SkEncode...
include/codec/SkEncodedInfo.h:20: // Each pixel is either fully opaque or fully
transparent.
On 2016/04/07 15:08:01, reed1 wrote:
> Are there no native premul image formats (e.g. webp?)

As far as I know, nothing is kPremul.  We allow libwebp to premultiply for us,
but the image is encoded as kUnpremul.

[msarett, 2016-04-07 16:19:30]

I've uploaded a new Patch Set that removes width/height from SkEncodedInfo.  I
actually prefer the previous version that keeps it in the struct...I think just
because it's a pain to pass width/height to the constructor.

But I'm happy enough with either alternative.

[reed1, 2016-04-18 17:31:33]

api lgtm

[msarett, 2016-04-18 19:31:43]

The CQ bit was checked by msarett@google.com

[msarett, 2016-04-18 19:31:43]

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/1820073002/#ps140001
(title: "Remove width and height from SkEncodedInfo")

[commit-bot: I haz the power, 2016-04-18 19:31:52]

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

[commit-bot: I haz the power, 2016-04-18 19:33:04]

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

[commit-bot: I haz the power, 2016-04-18 19:33:06]

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...)
  Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
  Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)
  Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[msarett, 2016-04-18 21:17:42]

The CQ bit was checked by msarett@google.com

[msarett, 2016-04-18 21:17:43]

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/1820073002/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2016-04-18 21:17:55]

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

[commit-bot: I haz the power, 2016-04-18 21:19:04]

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

[commit-bot: I haz the power, 2016-04-18 21:19:06]

Try jobs failed on following builders:
  Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
  Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)
  Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[msarett, 2016-04-18 21:23:39]

Patchset #5 (id:160001) has been deleted

[msarett, 2016-04-18 21:23:48]

The CQ bit was checked by msarett@google.com

[msarett, 2016-04-18 21:23:49]

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/1820073002/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2016-04-18 21:23:54]

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

[commit-bot: I haz the power, 2016-04-18 21:26:04]

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

[commit-bot: I haz the power, 2016-04-18 21:26:05]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[msarett, 2016-04-18 23:09:36]

The CQ bit was checked by msarett@google.com

[msarett, 2016-04-18 23:09: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/1820073002/#ps200001
(title: "Bug fix + windows compile fix")

[commit-bot: I haz the power, 2016-04-18 23:09:42]

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

[commit-bot: I haz the power, 2016-04-18 23:20:05]

Description was changed from

==========
Add SkEncodedInfo to report properties of encoded image data

All this does is build an SkEncodedInfo for each codec, and
then convert it to an SkImageInfo.

In future steps I intend to:
(1) Use SkEncodedInfo in place of SrcConfig in SkSwizzler.
(2) Support more conversions in SkSwizzler (non-native
BGRA/RGBA, 16-bit components, float, fixed point)
(3) Investigate optimizing conversions from encoded data
to linear color spaces.

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

to

==========
Add SkEncodedInfo to report properties of encoded image data

All this does is build an SkEncodedInfo for each codec, and
then convert it to an SkImageInfo.

In future steps I intend to:
(1) Use SkEncodedInfo in place of SrcConfig in SkSwizzler.
(2) Support more conversions in SkSwizzler (non-native
BGRA/RGBA, 16-bit components, float, fixed point)
(3) Investigate optimizing conversions from encoded data
to linear color spaces.

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

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

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

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

[robertphillips, 2016-04-19 11:40:49]

A revert of this CL (patchset #6 id:200001) has been created in
https://codereview.chromium.org/1895383002/ by robertphillips@google.com.

The reason for reverting is: Lots of bots failing.

[msarett, 2016-04-19 13:27:16]

Description was changed from

==========
Add SkEncodedInfo to report properties of encoded image data

All this does is build an SkEncodedInfo for each codec, and
then convert it to an SkImageInfo.

In future steps I intend to:
(1) Use SkEncodedInfo in place of SrcConfig in SkSwizzler.
(2) Support more conversions in SkSwizzler (non-native
BGRA/RGBA, 16-bit components, float, fixed point)
(3) Investigate optimizing conversions from encoded data
to linear color spaces.

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

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

to

==========
Add SkEncodedInfo to report properties of encoded image data

All this does is build an SkEncodedInfo for each codec, and
then convert it to an SkImageInfo.

In future steps I intend to:
(1) Use SkEncodedInfo in place of SrcConfig in SkSwizzler.
(2) Support more conversions in SkSwizzler (non-native
BGRA/RGBA, 16-bit components, float, fixed point)
(3) Investigate optimizing conversions from encoded data
to linear color spaces.

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

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

[msarett, 2016-04-20 18:13:02]

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

[commit-bot: I haz the power, 2016-04-20 18:13:11]

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

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

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

[commit-bot: I haz the power, 2016-04-20 18:29:21]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-04-20 18:52:34]

The CQ bit was checked by msarett@google.com

[msarett, 2016-04-20 18:52:34]

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/1820073002/#ps220001
(title: "Order of param eval bug")

[commit-bot: I haz the power, 2016-04-20 18:52:41]

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

[commit-bot: I haz the power, 2016-04-20 18:53:37]

Description was changed from

==========
Add SkEncodedInfo to report properties of encoded image data

All this does is build an SkEncodedInfo for each codec, and
then convert it to an SkImageInfo.

In future steps I intend to:
(1) Use SkEncodedInfo in place of SrcConfig in SkSwizzler.
(2) Support more conversions in SkSwizzler (non-native
BGRA/RGBA, 16-bit components, float, fixed point)
(3) Investigate optimizing conversions from encoded data
to linear color spaces.

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

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

to

==========
Add SkEncodedInfo to report properties of encoded image data

All this does is build an SkEncodedInfo for each codec, and
then convert it to an SkImageInfo.

In future steps I intend to:
(1) Use SkEncodedInfo in place of SrcConfig in SkSwizzler.
(2) Support more conversions in SkSwizzler (non-native
BGRA/RGBA, 16-bit components, float, fixed point)
(3) Investigate optimizing conversions from encoded data
to linear color spaces.

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

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

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

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

Committed patchset #7 (id:220001) as
https://skia.googlesource.com/skia/+/c30c418f4eb75f365c7d7a32d5419b41ca780ba8