Remove uses of SkImageDecoder from gms

Remove uses of SkImageDecoder from gms

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

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

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

[msarett, 2016-03-11 20:50:07]

Description was changed from

==========
Remove uses of SkImageDecoder from gms

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

to

==========
Remove uses of SkImageDecoder from gms

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

[msarett, 2016-03-11 20:53:09]

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

[msarett, 2016-03-11 20:53:10]

https://codereview.chromium.org/1791583002/diff/1/gm/cmykjpeg.cpp
File gm/cmykjpeg.cpp (left):

https://codereview.chromium.org/1791583002/diff/1/gm/cmykjpeg.cpp#oldcode2
gm/cmykjpeg.cpp:2: * Copyright 2012 Google Inc.
We test tons of cmyk jpegs through dm/gold.  I think this test is unnecessary.

https://codereview.chromium.org/1791583002/diff/1/gm/copyTo4444.cpp
File gm/copyTo4444.cpp (left):

https://codereview.chromium.org/1791583002/diff/1/gm/copyTo4444.cpp#oldcode2
gm/copyTo4444.cpp:2: * Copyright 2013 Google Inc.
Pretty sure we don't support drawing into 4444?

[scroggo, 2016-03-17 13:05:48]

I suggested migrating some gms over to GetResourceAsBitmap, but I'm fine with
those as is (maybe it's better anyway to go all the way to SkImage, but I didn't
look that closely at those gms).

My only hesitation to give 1gtm is that we might want to keep the 4444 copy
test, unless we're sure that no one takes advantage of copying to 4444 anymore.

https://codereview.chromium.org/1791583002/diff/1/gm/cmykjpeg.cpp
File gm/cmykjpeg.cpp (left):

https://codereview.chromium.org/1791583002/diff/1/gm/cmykjpeg.cpp#oldcode2
gm/cmykjpeg.cpp:2: * Copyright 2012 Google Inc.
On 2016/03/11 20:53:09, msarett wrote:
> We test tons of cmyk jpegs through dm/gold.  I think this test is unnecessary.

Agreed

https://codereview.chromium.org/1791583002/diff/1/gm/copyTo4444.cpp
File gm/copyTo4444.cpp (left):

https://codereview.chromium.org/1791583002/diff/1/gm/copyTo4444.cpp#oldcode2
gm/copyTo4444.cpp:2: * Copyright 2013 Google Inc.
On 2016/03/11 20:53:09, msarett wrote:
> Pretty sure we don't support drawing into 4444?

We don't support *drawing* into 4444, but we do support *copying* into 4444,
which this tests.

Copying into 4444 was added for a particular use case in Chrome
(crbug.com/245774). As far as I know they are still using it, but I have not
kept up with it.

https://codereview.chromium.org/1791583002/diff/1/gm/downsamplebitmap.cpp
File gm/downsamplebitmap.cpp (right):

https://codereview.chromium.org/1791583002/diff/1/gm/downsamplebitmap.cpp#new...
gm/downsamplebitmap.cpp:178: if (!image || !image->asLegacyBitmap(&fBM,
SkImage::kRW_LegacyBitmapMode)) {
Could you simplify this by using GetResourceAsBitmap in Resources.h?

https://codereview.chromium.org/1791583002/diff/1/gm/filterbitmap.cpp
File gm/filterbitmap.cpp (right):

https://codereview.chromium.org/1791583002/diff/1/gm/filterbitmap.cpp#newcode203
gm/filterbitmap.cpp:203: if (!image || !image->asLegacyBitmap(&fBM,
SkImage::kRW_LegacyBitmapMode)) {
GetResourceAsBitmap? Same for a few more files?

[msarett, 2016-03-17 13:53:38]

+1 for GetResourceAsBitmap().  Makes everything look nicer.

https://codereview.chromium.org/1791583002/diff/1/gm/copyTo4444.cpp
File gm/copyTo4444.cpp (left):

https://codereview.chromium.org/1791583002/diff/1/gm/copyTo4444.cpp#oldcode2
gm/copyTo4444.cpp:2: * Copyright 2013 Google Inc.
On 2016/03/17 13:05:48, scroggo wrote:
> On 2016/03/11 20:53:09, msarett wrote:
> > Pretty sure we don't support drawing into 4444?
> 
> We don't support *drawing* into 4444, but we do support *copying* into 4444,
> which this tests.
> 
> Copying into 4444 was added for a particular use case in Chrome
> (crbug.com/245774). As far as I know they are still using it, but I have not
> kept up with it.

Gotcha adding this back in.

https://codereview.chromium.org/1791583002/diff/1/gm/downsamplebitmap.cpp
File gm/downsamplebitmap.cpp (right):

https://codereview.chromium.org/1791583002/diff/1/gm/downsamplebitmap.cpp#new...
gm/downsamplebitmap.cpp:178: if (!image || !image->asLegacyBitmap(&fBM,
SkImage::kRW_LegacyBitmapMode)) {
On 2016/03/17 13:05:48, scroggo wrote:
> Could you simplify this by using GetResourceAsBitmap in Resources.h?

Done.

https://codereview.chromium.org/1791583002/diff/1/gm/filterbitmap.cpp
File gm/filterbitmap.cpp (right):

https://codereview.chromium.org/1791583002/diff/1/gm/filterbitmap.cpp#newcode203
gm/filterbitmap.cpp:203: if (!image || !image->asLegacyBitmap(&fBM,
SkImage::kRW_LegacyBitmapMode)) {
On 2016/03/17 13:05:48, scroggo wrote:
> GetResourceAsBitmap? Same for a few more files?

Done.

[scroggo, 2016-03-17 14:00:11]

lgtm

[msarett, 2016-03-17 14:03:59]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2016-03-17 14:04:05]

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

[commit-bot: I haz the power, 2016-03-17 14:19:43]

Description was changed from

==========
Remove uses of SkImageDecoder from gms

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

to

==========
Remove uses of SkImageDecoder from gms

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/+/d427266a58af7e6ea6c12c9cd56ade3e179a0c04
==========

[commit-bot: I haz the power, 2016-03-17 14:19:44]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/d427266a58af7e6ea6c12c9cd56ade3e179a0c04

[bungeman-skia, 2016-03-17 17:38:12]

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

The reason for reverting is: Suspected of causing assertion failures in
unrelated tests on Windows..

[bungeman-skia, 2016-03-17 17:49:39]

On 2016/03/17 17:38:12, bungeman1 wrote:
> A revert of this CL (patchset #2 id:20001) has been created in
> https://codereview.chromium.org/1806383002/ by mailto:bungeman@google.com.
> 
> The reason for reverting is: Suspected of causing assertion failures in
> unrelated tests on Windows..

Confirmed that this is causing the issue (on a local Win7). The error reported
was

Failures:
	c:\0\build\slave\workdir\build\skia\tests\cacheddecodingpixelreftest.cpp:111	encoded.get()
!= nullptr
	c:\0\build\slave\workdir\build\skia\tests\pdfdocumenttest.cpp:154	count_bytes(bm,
true) < count_bytes(bm, false)

[msarett, 2016-03-17 21:03:01]

Description was changed from

==========
Remove uses of SkImageDecoder from gms

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/+/d427266a58af7e6ea6c12c9cd56ade3e179a0c04
==========

to

==========
Remove uses of SkImageDecoder from gms

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/+/d427266a58af7e6ea6c12c9cd56ade3e179a0c04
==========

[msarett, 2016-03-18 13:29:34]

Patchset #4 (id:60001) has been deleted

[msarett, 2016-03-18 14:27:40]

Patchset #4 (id:80001) has been deleted

[msarett, 2016-03-18 16:34:25]

Patchset #5 (id:120001) has been deleted

[msarett, 2016-03-18 16:34:32]

Patchset #4 (id:100001) has been deleted

[msarett, 2016-03-18 16:41:32]

Fixing a bug in the WIC encoder will fix this.

It looks like we don't really have control over the order in which our encoders
register when they link.  For some reason, my change caused the WIC encoder to
be tried before the other encoders (exposing this bug).

I actually think we've run into this before - we just reverted and never figured
out the problem.

I don't really feel strongly about the order in which our encoders are used. 
But if we do care, we'll want to revisit this.

https://codereview.chromium.org/1791583002/diff/140001/src/ports/SkImageDecod...
File src/ports/SkImageDecoder_WIC.cpp (left):

https://codereview.chromium.org/1791583002/diff/140001/src/ports/SkImageDecod...
src/ports/SkImageDecoder_WIC.cpp:413: if (SUCCEEDED(hr)) {
This code block causes the WIC encoder to fail for all Jpeg encodes.

Since Jpegs are opaque, it indicates that it supports BGR (not BGRA).  None of
this really matters, since we just want the encoded data (which is YUV anyway).

[msarett, 2016-03-18 16:42:02]

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

[commit-bot: I haz the power, 2016-03-18 16:42:10]

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

[commit-bot: I haz the power, 2016-03-18 16:51:20]

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

[commit-bot: I haz the power, 2016-03-18 16:51:21]

Dry run: This issue passed the CQ dry run.

[scroggo, 2016-03-18 17:23:12]

On 2016/03/18 16:41:32, msarett wrote:
> Fixing a bug in the WIC encoder will fix this.
> 
> It looks like we don't really have control over the order in which our
encoders
> register when they link.  For some reason, my change caused the WIC encoder to
> be tried before the other encoders (exposing this bug).

Let me see if I understand. Prior to your change, our encoder that uses libjpeg
linked before WIC, so the libjpeg version gets used. Somehow your change
switched the order, so that now the WIC encoder, which always had a bug, gets
used.

That seems strange. For the decoders, I think the order depended on the order of
compilation, which you did not appear to change here. (I also thought we did not
build the libjpeg version on windows.)

> 
> I actually think we've run into this before - we just reverted and never
figured
> out the problem.
> 
> I don't really feel strongly about the order in which our encoders are used. 
> But if we do care, we'll want to revisit this.

I don't know how much our encoders (particularly WIC) are being used. But I
would think that like with decoding, some clients will always want the platform
version. So I think we need to make them choose, rather than specifying an
order.

https://codereview.chromium.org/1791583002/diff/140001/src/ports/SkImageDecod...
File src/ports/SkImageDecoder_WIC.cpp (right):

https://codereview.chromium.org/1791583002/diff/140001/src/ports/SkImageDecod...
src/ports/SkImageDecoder_WIC.cpp:438: /*
This will disable the encoders on Windows. Is that your intention? It seems odd
that you changed onEncode if it was just going to be disabled here.

[msarett, 2016-03-18 17:25:03]

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

[commit-bot: I haz the power, 2016-03-18 17:25:16]

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

[msarett, 2016-03-18 17:30:56]

On 2016/03/18 17:23:12, scroggo wrote:
> On 2016/03/18 16:41:32, msarett wrote:
> > Fixing a bug in the WIC encoder will fix this.
> > 
> > It looks like we don't really have control over the order in which our
> encoders
> > register when they link.  For some reason, my change caused the WIC encoder
to
> > be tried before the other encoders (exposing this bug).
> 
> Let me see if I understand. Prior to your change, our encoder that uses
libjpeg
> linked before WIC, so the libjpeg version gets used. Somehow your change
> switched the order, so that now the WIC encoder, which always had a bug, gets
> used.
> 
> That seems strange. For the decoders, I think the order depended on the order
of
> compilation, which you did not appear to change here. (I also thought we did
not
> build the libjpeg version on windows.)
> 

I thought the same thing, based on comments in images.gyp.  I spoke to Mike
about
this briefly, and he suggested that it really is unpredictable.  If we want to
enforce the order, we can't trust the linker.

> > 
> > I actually think we've run into this before - we just reverted and never
> figured
> > out the problem.
> > 
> > I don't really feel strongly about the order in which our encoders are used.

> > But if we do care, we'll want to revisit this.
> 
> I don't know how much our encoders (particularly WIC) are being used. But I
> would think that like with decoding, some clients will always want the
platform
> version. So I think we need to make them choose, rather than specifying an
> order.
> 

I agree.  I think a follow-up change should force clients to explicitly choose
WIC
in order to use it.

>
https://codereview.chromium.org/1791583002/diff/140001/src/ports/SkImageDecod...
> File src/ports/SkImageDecoder_WIC.cpp (right):
> 
>
https://codereview.chromium.org/1791583002/diff/140001/src/ports/SkImageDecod...
> src/ports/SkImageDecoder_WIC.cpp:438: /*
> This will disable the encoders on Windows. Is that your intention? It seems
odd
> that you changed onEncode if it was just going to be disabled here.

This was a mistake...  The trybots didn't like it much either.

[scroggo, 2016-03-18 17:35:54]

patch set 5 lgtm

[commit-bot: I haz the power, 2016-03-18 17:36:31]

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

[commit-bot: I haz the power, 2016-03-18 17:36:33]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-03-18 19:12:18]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2016-03-18 19:12:30]

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

[commit-bot: I haz the power, 2016-03-18 19:13:50]

Description was changed from

==========
Remove uses of SkImageDecoder from gms

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/+/d427266a58af7e6ea6c12c9cd56ade3e179a0c04
==========

to

==========
Remove uses of SkImageDecoder from gms

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/+/d427266a58af7e6ea6c12c9cd56ade3e179a0c04

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

[commit-bot: I haz the power, 2016-03-18 19:13:51]

Committed patchset #5 (id:160001) as
https://skia.googlesource.com/skia/+/e820dfebbc652bfc4a653f07a29f5fe371eb019b

[bungeman-skia, 2016-03-19 21:23:04]

On 2016/03/18 19:13:51, commit-bot: I haz the power wrote:
> Committed patchset #5 (id:160001) as
> https://skia.googlesource.com/skia/+/e820dfebbc652bfc4a653f07a29f5fe371eb019b

For some reason this appears to be affecting gms... see gold for examples.

[msarett, 2016-03-21 13:06:58]

Thanks for the heads up.

The issue is that SkCodec does not support astc.  I've marked these as negative.