Run CodecSrc DM.

Run CodecSrc  DM.

Rather than making SkCodec an option instead of SkImageDecoder,
create a separate CodecSrc. This allows us to compare the two.

For both CodecSrc and ImageSrc, do not decode to a gpu backend.

BUG=skia:3475
Committed: https://skia.googlesource.com/skia/+/9b77ddde08efe702256355a333cf31ade8f15bb0

[scroggo, 2015-03-05 22:34:57]

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

[scroggo, 2015-03-05 22:34:57]

Mike, any thoughts on the best way to integrate this into DM?

[mtklein, 2015-03-06 00:25:25]

https://codereview.chromium.org/978823002/diff/80001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/978823002/diff/80001/dm/DM.cpp#newcode154
dm/DM.cpp:154: static const CodecSrc::SkipZeroes gSkips[] = {
Why don't we wait to land these options until the CL that supports other
options?  Seems like we're adding a lot of boilerplate before it's needed.

https://codereview.chromium.org/978823002/diff/80001/dm/DM.cpp#newcode177
dm/DM.cpp:177: if (!SkColorTypeValidateAlphaType(ct, at, &canonical) ||
canonical != at) {
Don't we know ahead of time which pairs will pass this test?  The list won't
change very often, right?

struct { SkColorType ct; SkAlphaType at; } targets[] = {
  { kN32_SkColorType,     kPremul_SkAlphaType },
  { kN32_SkColorType,     kOpaque_SkAlphaType },
  { kAlpha_8_SkColorType, kPremul_SkAlphaType },
  { kRGB_565_SkColorType, kOpaque_SkAlphaType },
};
for (int i = 0; i < SK_ARRAY_COUNT(targets); i++) {
    push_src("codec", new CodecSrc(path, targets[i].ct, targets[i].at));
}

https://codereview.chromium.org/978823002/diff/80001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/978823002/diff/80001/dm/DMSrcSink.cpp#newcode47
dm/DMSrcSink.cpp:47: Error CodecSrc::draw(SkCanvas* canvas) const {
Can we use non-fatal errors here to have this CodecSrc restrict itself to its
targets of choice, rather than having to have DM.cpp do it?  E.g.

SkImageInfo info;
if (!canvas->peekPixels(&info, NULL) || info.colorType() != kN32_SkColorType) {
    return Error::Nonfatal("unsupported decode target");
}

Then just append the lot of CodecSrcs to the main source list?

[scroggo, 2015-03-06 20:40:27]

https://codereview.chromium.org/978823002/diff/80001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/978823002/diff/80001/dm/DM.cpp#newcode154
dm/DM.cpp:154: static const CodecSrc::SkipZeroes gSkips[] = {
On 2015/03/06 00:25:25, mtklein wrote:
> Why don't we wait to land these options until the CL that supports other
> options?  Seems like we're adding a lot of boilerplate before it's needed.

Sure! I wasn't ready to land this anyway, I just wanted to run the larger plan
by you.

https://codereview.chromium.org/978823002/diff/80001/dm/DM.cpp#newcode177
dm/DM.cpp:177: if (!SkColorTypeValidateAlphaType(ct, at, &canonical) ||
canonical != at) {
On 2015/03/06 00:25:25, mtklein wrote:
> Don't we know ahead of time which pairs will pass this test?  The list won't
> change very often, right?
> 
> struct { SkColorType ct; SkAlphaType at; } targets[] = {
>   { kN32_SkColorType,     kPremul_SkAlphaType },
>   { kN32_SkColorType,     kOpaque_SkAlphaType },
>   { kAlpha_8_SkColorType, kPremul_SkAlphaType },
>   { kRGB_565_SkColorType, kOpaque_SkAlphaType },
> };
> for (int i = 0; i < SK_ARRAY_COUNT(targets); i++) {
>     push_src("codec", new CodecSrc(path, targets[i].ct, targets[i].at));
> }

Yes, that will work.

https://codereview.chromium.org/978823002/diff/80001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/978823002/diff/80001/dm/DMSrcSink.cpp#newcode47
dm/DMSrcSink.cpp:47: Error CodecSrc::draw(SkCanvas* canvas) const {
On 2015/03/06 00:25:25, mtklein wrote:
> Can we use non-fatal errors here to have this CodecSrc restrict itself to its
> targets of choice, rather than having to have DM.cpp do it?  E.g.
> 
> SkImageInfo info;
> if (!canvas->peekPixels(&info, NULL) || info.colorType() != kN32_SkColorType)
{
>     return Error::Nonfatal("unsupported decode target");
> }
> 
> Then just append the lot of CodecSrcs to the main source list?

That could work. Although I'm still trying to wrap my head around making each
flavor of CodecSrc have the same name, but not overwrite each other. I was
thinking maybe I wanted a different kind of Sink (or via), but maybe I should
just call push_src with a different tag? That could make the Corpus dropdown in
Gold get much bigger, but maybe that's okay?

[scroggo, 2015-03-06 20:43:45]

https://codereview.chromium.org/978823002/diff/80001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/978823002/diff/80001/dm/DMSrcSink.cpp#newcode47
dm/DMSrcSink.cpp:47: Error CodecSrc::draw(SkCanvas* canvas) const {
On 2015/03/06 20:40:27, scroggo wrote:
> On 2015/03/06 00:25:25, mtklein wrote:
> > Can we use non-fatal errors here to have this CodecSrc restrict itself to
its
> > targets of choice, rather than having to have DM.cpp do it?  E.g.
> > 
> > SkImageInfo info;
> > if (!canvas->peekPixels(&info, NULL) || info.colorType() !=
kN32_SkColorType)
> {
> >     return Error::Nonfatal("unsupported decode target");
> > }
> > 
> > Then just append the lot of CodecSrcs to the main source list?
> 
> That could work. Although I'm still trying to wrap my head around making each
> flavor of CodecSrc have the same name, but not overwrite each other. I was
> thinking maybe I wanted a different kind of Sink (or via), but maybe I should
> just call push_src with a different tag? That could make the Corpus dropdown
in
> Gold get much bigger, but maybe that's okay?

My other thought is that the config you choose will then determine whether you
test the codecs. That's probably okay, since we probably have the following
non-overlapping sets of use cases:

bots: run everything
most skia developers: run exactly what they want (i.e. not codec)
matt and I, only when changing codec: run codec

[mtklein, 2015-03-06 21:01:00]

> That could work. Although I'm still trying to wrap my head around making each
> flavor of CodecSrc have the same name, but not overwrite each other. I was
> thinking maybe I wanted a different kind of Sink (or via), but maybe I should
> just call push_src with a different tag? That could make the Corpus dropdown
in
> Gold get much bigger, but maybe that's okay?

What do you mean, overwrite each other?  (src, sink, name) has to be unique, but
lesser sharing is fine.  Things with the same name will be compared in Gold. 
We've gone back and forth about whether that should be same (name, source), but
we're on name now and I think that'll stick.

[mtklein, 2015-03-06 21:01:33]

> My other thought is that the config you choose will then determine whether you
> test the codecs. That's probably okay, since we probably have the following
> non-overlapping sets of use cases:
> 
> bots: run everything
> most skia developers: run exactly what they want (i.e. not codec)
> matt and I, only when changing codec: run codec

Yep.

[scroggo, 2015-03-06 21:30:12]

On 2015/03/06 21:01:00, mtklein wrote:
> > That could work. Although I'm still trying to wrap my head around making
each
> > flavor of CodecSrc have the same name, but not overwrite each other. I was
> > thinking maybe I wanted a different kind of Sink (or via), but maybe I
should
> > just call push_src with a different tag? That could make the Corpus dropdown
> in
> > Gold get much bigger, but maybe that's okay?
> 
> What do you mean, overwrite each other?  (src, sink, name) has to be unique,
but
> lesser sharing is fine.  Things with the same name will be compared in Gold. 
> We've gone back and forth about whether that should be same (name, source),
but
> we're on name now and I think that'll stick.

Let's say I do the following decodes:

CodecSrc decode to 565
CodecSrc decode to 8888
CodecSrc decode to Alpha8

I need some way to distinguish the three output images. If they have the same
name (not shown in this CL), they all try to write to
<writeDir>/8888/codec/<image-name>, and one of them wins (that's the overwrite).

So I gave them different names. As a result, in Gold, IIUC, I won't be able to
compare these three images. So I'm trying to figure out the alternative to using
different names, bringing back the original problem. I think using different
tags solves this. It just means Corpus will now have:

gm
codec_8888
codec_565
codec_alpha8
image (will eventually fall off)
skp
subset


codec_* will expand once we do more things (skip zeroes, etc)

[mtklein, 2015-03-06 22:29:45]

On 2015/03/06 21:30:12, scroggo wrote:
> On 2015/03/06 21:01:00, mtklein wrote:
> > > That could work. Although I'm still trying to wrap my head around making
> each
> > > flavor of CodecSrc have the same name, but not overwrite each other. I was
> > > thinking maybe I wanted a different kind of Sink (or via), but maybe I
> should
> > > just call push_src with a different tag? That could make the Corpus
dropdown
> > in
> > > Gold get much bigger, but maybe that's okay?
> > 
> > What do you mean, overwrite each other?  (src, sink, name) has to be unique,
> but
> > lesser sharing is fine.  Things with the same name will be compared in Gold.

> > We've gone back and forth about whether that should be same (name, source),
> but
> > we're on name now and I think that'll stick.
> 
> Let's say I do the following decodes:
> 
> CodecSrc decode to 565
> CodecSrc decode to 8888
> CodecSrc decode to Alpha8
> 
> I need some way to distinguish the three output images. If they have the same
> name (not shown in this CL), they all try to write to
> <writeDir>/8888/codec/<image-name>, and one of them wins (that's the
overwrite).

What if we go back to the straightforward idea where we have them decode against
raster sinks with 565, 8888, and A8 color types?  (And make the other src types
like SKPs and GMs skip non-fatal out on A8 backends.)  They'll all then be
distinct by the config, but have the same name so they compare together in Gold.

[scroggo, 2015-03-11 13:44:00]

On 2015/03/06 22:29:45, mtklein wrote:
> On 2015/03/06 21:30:12, scroggo wrote:
> > On 2015/03/06 21:01:00, mtklein wrote:
> > > > That could work. Although I'm still trying to wrap my head around making
> > each
> > > > flavor of CodecSrc have the same name, but not overwrite each other. I
was
> > > > thinking maybe I wanted a different kind of Sink (or via), but maybe I
> > should
> > > > just call push_src with a different tag? That could make the Corpus
> dropdown
> > > in
> > > > Gold get much bigger, but maybe that's okay?
> > > 
> > > What do you mean, overwrite each other?  (src, sink, name) has to be
unique,
> > but
> > > lesser sharing is fine.  Things with the same name will be compared in
Gold.
> 
> > > We've gone back and forth about whether that should be same (name,
source),
> > but
> > > we're on name now and I think that'll stick.
> > 
> > Let's say I do the following decodes:
> > 
> > CodecSrc decode to 565
> > CodecSrc decode to 8888
> > CodecSrc decode to Alpha8
> > 
> > I need some way to distinguish the three output images. If they have the
same
> > name (not shown in this CL), they all try to write to
> > <writeDir>/8888/codec/<image-name>, and one of them wins (that's the
> overwrite).
> 
> What if we go back to the straightforward idea where we have them decode
against
> raster sinks with 565, 8888, and A8 color types?  (And make the other src
types
> like SKPs and GMs skip non-fatal out on A8 backends.)  They'll all then be
> distinct by the config, but have the same name so they compare together in
Gold.

We'd need to change what the raster sink does. We cannot draw to an A8 SkBitmap.
A Sink has an SkBitmap, so we could decode directly to that, but not without
changing the relationship between Sink and Src (where Sink calls
Src::draw(SkCanvas*)).

This also doesn't solve the problem of testing the other modes (e.g. skip
zeroes, etc) - which doesn't mean it's wrong; only that it might be nice to have
a solution to both.

[mtklein, 2015-03-11 22:44:34]

> We'd need to change what the raster sink does. We cannot draw to an A8
SkBitmap.
> A Sink has an SkBitmap, so we could decode directly to that, but not without
> changing the relationship between Sink and Src (where Sink calls
> Src::draw(SkCanvas*)).

What am I not getting about A8?  I just added this to DM.cpp as line 233:
   SINK("a8",   RasterSink, kAlpha_8_SkColorType);
then compiled and ran out/Debug/dm dm --src image --config a8 -w bad.  Looking
in bad/a8/image, I see a bunch of images decoded in A8 (mostly uninterestingly),
including the naturally-A8 mandrill .ktx, which is the one we care about right?

> This also doesn't solve the problem of testing the other modes (e.g. skip
> zeroes, etc) - which doesn't mean it's wrong; only that it might be nice to
have
> a solution to both.

This is true.  Gonna have to think about this bit.  What if we have the CodecSrc
actually sniff the pixels to decide if it can do a skip-zeros run or not?  Then,
we run pipes like CodecSrc->8888 (transparent) and CodecSrc->ViaClearWhite->8888
(where ViaClearWhite just clears to white before drawing the normal Src).

[reed1, 2015-03-13 13:08:27]

On 2015/03/11 22:44:34, mtklein wrote:
> > We'd need to change what the raster sink does. We cannot draw to an A8
> SkBitmap.
> > A Sink has an SkBitmap, so we could decode directly to that, but not without
> > changing the relationship between Sink and Src (where Sink calls
> > Src::draw(SkCanvas*)).
> 
> What am I not getting about A8?  I just added this to DM.cpp as line 233:
>    SINK("a8",   RasterSink, kAlpha_8_SkColorType);
> then compiled and ran out/Debug/dm dm --src image --config a8 -w bad.  Looking
> in bad/a8/image, I see a bunch of images decoded in A8 (mostly
uninterestingly),
> including the naturally-A8 mandrill .ktx, which is the one we care about
right?
> 
> > This also doesn't solve the problem of testing the other modes (e.g. skip
> > zeroes, etc) - which doesn't mean it's wrong; only that it might be nice to
> have
> > a solution to both.
> 
> This is true.  Gonna have to think about this bit.  What if we have the
CodecSrc
> actually sniff the pixels to decide if it can do a skip-zeros run or not? 
Then,
> we run pipes like CodecSrc->8888 (transparent) and
CodecSrc->ViaClearWhite->8888
> (where ViaClearWhite just clears to white before drawing the normal Src).

We can draw into 8888, 565, and A8. Cannot draw into 4444.

[scroggo, 2015-03-13 21:34:33]

On 2015/03/06 22:29:45, mtklein wrote:
> What if we go back to the straightforward idea where we have them decode
against
> raster sinks with 565, 8888, and A8 color types?  (And make the other src
types
> like SKPs and GMs skip non-fatal out on A8 backends.)  They'll all then be
> distinct by the config, but have the same name so they compare together in
Gold.

Done, with the exception of A8, which I'll leave to a later CL when I have A8
working for SkCodec.

On 2015/03/13 13:08:27, reed1 wrote:
> We can draw into 8888, 565, and A8. Cannot draw into 4444.

My mistake. We'll still run into the config problem if we decide to support
Index8, but perhaps we'll cross that bridge when we get there. (My understanding
is that the Android graphics guys don't like Index8, and I know they copy to
8888 before texture upload, but if we took it away, they may notice the extra
RAM used.)

On 2015/03/11 22:44:34, mtklein wrote:
> in bad/a8/image, I see a bunch of images decoded in A8 (mostly
uninterestingly),
> including the naturally-A8 mandrill .ktx, which is the one we care about
right?

I'm not aware about the A8 mandrill, but I have a test image somewhere, it's
just not checked in or tested currently :(

> This is true.  Gonna have to think about this bit.  What if we have the
CodecSrc
> actually sniff the pixels to decide if it can do a skip-zeros run or not? 
Then,
> we run pipes like CodecSrc->8888 (transparent) and
CodecSrc->ViaClearWhite->8888
> (where ViaClearWhite just clears to white before drawing the normal Src).

That's an interesting idea. I'll include that as a separate CL once we land the
CL that supports the feature (or along with it).

https://codereview.chromium.org/978823002/diff/80001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/978823002/diff/80001/dm/DM.cpp#newcode177
dm/DM.cpp:177: if (!SkColorTypeValidateAlphaType(ct, at, &canonical) ||
canonical != at) {
On 2015/03/06 20:40:26, scroggo wrote:
> On 2015/03/06 00:25:25, mtklein wrote:
> > Don't we know ahead of time which pairs will pass this test?  The list won't
> > change very often, right?
> > 
> > struct { SkColorType ct; SkAlphaType at; } targets[] = {
> >   { kN32_SkColorType,     kPremul_SkAlphaType },
> >   { kN32_SkColorType,     kOpaque_SkAlphaType },
> >   { kAlpha_8_SkColorType, kPremul_SkAlphaType },
> >   { kRGB_565_SkColorType, kOpaque_SkAlphaType },
> > };
> > for (int i = 0; i < SK_ARRAY_COUNT(targets); i++) {
> >     push_src("codec", new CodecSrc(path, targets[i].ct, targets[i].at));
> > }
> 
> Yes, that will work. 

Leaving out for now, since we're using the config of the Sink, and we don't yet
have a choice between premul and unpremul (and I'm not sure it's necessary to
compare the same image decoded as opaque vs unpremul, since it should just be
one or the other).

https://codereview.chromium.org/978823002/diff/80001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/978823002/diff/80001/dm/DMSrcSink.cpp#newcode47
dm/DMSrcSink.cpp:47: Error CodecSrc::draw(SkCanvas* canvas) const {
On 2015/03/06 00:25:25, mtklein wrote:
> Can we use non-fatal errors here to have this CodecSrc restrict itself to its
> targets of choice, rather than having to have DM.cpp do it?  E.g.
> 
> SkImageInfo info;
> if (!canvas->peekPixels(&info, NULL) || info.colorType() != kN32_SkColorType)
{
>     return Error::Nonfatal("unsupported decode target");
> }
> 
> Then just append the lot of CodecSrcs to the main source list?

Done.

[mtklein, 2015-03-17 20:24:15]

Some suggestions, but LGTM as it stands.

https://codereview.chromium.org/978823002/diff/160001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/978823002/diff/160001/dm/DM.cpp#newcode173
dm/DM.cpp:173: push_src("codec", new CodecSrc(flag));     // Decode with
SkCodec.
Want to check the extension here too?

https://codereview.chromium.org/978823002/diff/160001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/978823002/diff/160001/dm/DMSrcSink.cpp#newcode46
dm/DMSrcSink.cpp:46: return Error::Nonfatal("No need to test decoding to
non-raster backend.");
// TODO: rethink with JPGs  ?

https://codereview.chromium.org/978823002/diff/160001/dm/DMSrcSink.cpp#newcode76
dm/DMSrcSink.cpp:76: SkAutoLockPixels alp(bitmap);
This seems fine, but it's not needed right?  SkBitmaps we back with cpu-memory
are always locked.

https://codereview.chromium.org/978823002/diff/160001/dm/DMSrcSink.cpp#newcode79
dm/DMSrcSink.cpp:79: switch (result) {
Might consider fusing into

switch (codec->getPixels(decodeInfo, bitmap.getPixels(), bitmap.rowBytes())) {
    ...
}

[mtklein, 2015-03-17 20:24:39]

https://codereview.chromium.org/978823002/diff/160001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/978823002/diff/160001/dm/DMSrcSink.cpp#newcode16
dm/DMSrcSink.cpp:16: DEFINE_bool(codec, false, "Use SkCodec instead of
SkImageDecoder");
Forgot: remove this?

[scroggo, 2015-03-19 12:57:55]

https://codereview.chromium.org/978823002/diff/160001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/978823002/diff/160001/dm/DM.cpp#newcode173
dm/DM.cpp:173: push_src("codec", new CodecSrc(flag));     // Decode with
SkCodec.
On 2015/03/17 20:24:15, mtklein wrote:
> Want to check the extension here too?

I thought about it, but this case is different:
We already don't check to make sure it has an image extension, unlike when we're
reading directories.
This isn't tested by the bots - it's only for local testing if someone wants to
test an individual file. If they try to decode a file that's not an image (or at
least doesn't have an extension we support), I think that's okay.

https://codereview.chromium.org/978823002/diff/160001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/978823002/diff/160001/dm/DMSrcSink.cpp#newcode16
dm/DMSrcSink.cpp:16: DEFINE_bool(codec, false, "Use SkCodec instead of
SkImageDecoder");
On 2015/03/17 20:24:39, mtklein wrote:
> Forgot: remove this?

Good catch. I lost track of that when moving between branches...

https://codereview.chromium.org/978823002/diff/160001/dm/DMSrcSink.cpp#newcode46
dm/DMSrcSink.cpp:46: return Error::Nonfatal("No need to test decoding to
non-raster backend.");
On 2015/03/17 20:24:15, mtklein wrote:
> // TODO: rethink with JPGs  ?

Done.

https://codereview.chromium.org/978823002/diff/160001/dm/DMSrcSink.cpp#newcode76
dm/DMSrcSink.cpp:76: SkAutoLockPixels alp(bitmap);
On 2015/03/17 20:24:15, mtklein wrote:
> This seems fine, but it's not needed right?  SkBitmaps we back with cpu-memory
> are always locked.

The implementation of tryAllocPixels does lock automatically, but AFAICT, we
make no guarantee that it's the case.

https://codereview.chromium.org/978823002/diff/160001/dm/DMSrcSink.cpp#newcode79
dm/DMSrcSink.cpp:79: switch (result) {
On 2015/03/17 20:24:15, mtklein wrote:
> Might consider fusing into
> 
> switch (codec->getPixels(decodeInfo, bitmap.getPixels(), bitmap.rowBytes())) {
>     ...
> }

Done.

https://codereview.chromium.org/978823002/diff/160001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:115: return Error::Nonfatal("No need to test decoding to
non-raster backend.");
I've also added a TODO to ImageSrc to use deferred decoding.

[scroggo, 2015-03-19 12:58:00]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-03-19 12:58:00]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com
Link to the patchset: https://codereview.chromium.org/978823002/#ps200001
(title: "Respond to comments.")

[commit-bot: I haz the power, 2015-03-19 12:58:09]

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

[commit-bot: I haz the power, 2015-03-19 13:03:43]

Committed patchset #11 (id:200001) as
https://skia.googlesource.com/skia/+/9b77ddde08efe702256355a333cf31ade8f15bb0

[reed1, 2015-03-19 16:50:42]

On 2015/03/19 13:03:43, I haz the power (commit-bot) wrote:
> Committed patchset #11 (id:200001) as
> https://skia.googlesource.com/skia/+/9b77ddde08efe702256355a333cf31ade8f15bb0

We just need to update the dox (I will) : we can definitely rely on
allocPixels/tryAllocPixels etc. to returned locked pixels.