Always return ImageShader, even from SkShader::MakeBitmapShader

Always return ImageShader, even from SkShader::MakeBitmapShader

Lessons learned

1. ImageShader (correctly) always compresses (typically via PNG) during serialization. This has the surprise results of
- if the image was marked opaque, but has some non-opaque pixels (i.e. bug in blitter or caller), then compressing may "fix" those pixels, making the deserialized version draw differently. bug filed.
- 565 compressess/decompresses to 8888 (at least on Mac), which draws differently (esp. under some filters). bug filed.

2. BitmapShader did not enforce a copy for mutable bitmaps, but ImageShader does (since it creates an Image). Thus the former would see subsequent changes to the pixels after shader creation, while the latter does not, hence the change to the BlitRow test to avoid this modify-after-create pattern. I sure hope this prev. behavior was a bug/undefined-behavior, since this CL changes that.

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

Committed: https://skia.googlesource.com/skia/+/320a40d7733979703bdf675c31108255e011e34e

[reed1, 2016-07-29 22:25:57]

Description was changed from

==========
return SkImageShader from create-shader-from-bitmap

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

to

==========
return SkImageShader from create-shader-from-bitmap

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

[reed1, 2016-07-29 22:38:42]

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

[commit-bot: I haz the power, 2016-07-29 22:38:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[reed1, 2016-07-29 22:39:25]

I *think* I need to keep some of bitmapprocshader around, so we have a
CreateProc to deserialize older pictures. Don't think there's any other need.

[reed1, 2016-07-29 22:39:46]

reed@google.com changed reviewers:
+ fmalita@chromium.org, herb@google.com, mtklein@google.com

[reed1, 2016-07-29 22:40:28]

motivated by earlier CLs (and future ones) where we unify around always
recording/serializing images, not bitmaps.

[commit-bot: I haz the power, 2016-07-29 22:46:55]

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

[commit-bot: I haz the power, 2016-07-29 22:46:56]

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

[reed1, 2016-07-31 19:10:15]

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

[commit-bot: I haz the power, 2016-07-31 19:10:22]

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-07-31 19:18:52]

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

[commit-bot: I haz the power, 2016-07-31 19:18:53]

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

[reed1, 2016-08-01 19:20:24]

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

[commit-bot: I haz the power, 2016-08-01 19:20:26]

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-08-01 19:31:47]

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

[commit-bot: I haz the power, 2016-08-01 19:31:48]

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

[reed1, 2016-08-01 23:55:10]

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

[commit-bot: I haz the power, 2016-08-01 23:55:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[reed1, 2016-08-01 23:57:25]

ptal

[commit-bot: I haz the power, 2016-08-02 00:05:12]

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

[commit-bot: I haz the power, 2016-08-02 00:05:13]

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

[reed1, 2016-08-02 01:43:53]

Description was changed from

==========
return SkImageShader from create-shader-from-bitmap

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

to

==========
return SkImageShader from create-shader-from-bitmap

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

[reed1, 2016-08-02 01:44:29]

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

[commit-bot: I haz the power, 2016-08-02 01:44:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[reed1, 2016-08-02 01:55:09]

Description was changed from

==========
return SkImageShader from create-shader-from-bitmap

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

to

==========
return SkImageShader from create-shader-from-bitmap

Lessons learned switching imageshader

1. ImageShader (correctly) always compresses (typically via PNG) during
serialization. This has the surprise results of
- if the image was marked opaque, but has some non-opaque pixels (i.e. bug in
blitter or caller), then compressing may "fix" those pixels, making the
deserialized version draw differently. bug filed.
- 565 compressess/decompresses to 8888 (at least on Mac), which draws
differently (esp. under some filters). bug filed.

2. BitmapShader did not enforce a copy for mutable bitmaps, but ImageShader does
(since it creates an Image). Thus the former would see subsequent changes to the
pixels after shader creation, while the latter does not, hence the change to the
BlitRow test to avoid this modify-after-create pattern. I sure hope this prev.
behavior was a bug/undefined-behavior, since this CL changes that.

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

[reed1, 2016-08-02 01:56:10]

Description was changed from

==========
return SkImageShader from create-shader-from-bitmap

Lessons learned switching imageshader

1. ImageShader (correctly) always compresses (typically via PNG) during
serialization. This has the surprise results of
- if the image was marked opaque, but has some non-opaque pixels (i.e. bug in
blitter or caller), then compressing may "fix" those pixels, making the
deserialized version draw differently. bug filed.
- 565 compressess/decompresses to 8888 (at least on Mac), which draws
differently (esp. under some filters). bug filed.

2. BitmapShader did not enforce a copy for mutable bitmaps, but ImageShader does
(since it creates an Image). Thus the former would see subsequent changes to the
pixels after shader creation, while the latter does not, hence the change to the
BlitRow test to avoid this modify-after-create pattern. I sure hope this prev.
behavior was a bug/undefined-behavior, since this CL changes that.

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

to

==========
Always return ImageShader, even from SkShader::MakeBitmapShader

Lessons learned

1. ImageShader (correctly) always compresses (typically via PNG) during
serialization. This has the surprise results of
- if the image was marked opaque, but has some non-opaque pixels (i.e. bug in
blitter or caller), then compressing may "fix" those pixels, making the
deserialized version draw differently. bug filed.
- 565 compressess/decompresses to 8888 (at least on Mac), which draws
differently (esp. under some filters). bug filed.

2. BitmapShader did not enforce a copy for mutable bitmaps, but ImageShader does
(since it creates an Image). Thus the former would see subsequent changes to the
pixels after shader creation, while the latter does not, hence the change to the
BlitRow test to avoid this modify-after-create pattern. I sure hope this prev.
behavior was a bug/undefined-behavior, since this CL changes that.

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

[reed1, 2016-08-02 02:02:05]

The CQ bit was unchecked by reed@google.com

[reed1, 2016-08-02 02:02:09]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2016-08-02 02:02:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[f(malita), 2016-08-02 13:10:28]

Cool!  LGTM

https://codereview.chromium.org/2195893002/diff/150001/src/core/SkBitmapProcS...
File src/core/SkBitmapProcShader.h (right):

https://codereview.chromium.org/2195893002/diff/150001/src/core/SkBitmapProcS...
src/core/SkBitmapProcShader.h:16: class SkBitmapProcLegacyShader : public
SkShader {
This is now just a collection of static private helpers, accessed by friend
SkImageShader.

Move everything to SkImageShader impl?

https://codereview.chromium.org/2195893002/diff/150001/src/image/SkImageShader.h
File src/image/SkImageShader.h (right):

https://codereview.chromium.org/2195893002/diff/150001/src/image/SkImageShade...
src/image/SkImageShader.h:29: SkImageShader(const SkImage*, TileMode tx,
TileMode ty, const SkMatrix* localMatrix);
Is there any magic registrar class we can befriend to avoid exposing the ctor?

[commit-bot: I haz the power, 2016-08-02 13:12:10]

Description was changed from

==========
Always return ImageShader, even from SkShader::MakeBitmapShader

Lessons learned

1. ImageShader (correctly) always compresses (typically via PNG) during
serialization. This has the surprise results of
- if the image was marked opaque, but has some non-opaque pixels (i.e. bug in
blitter or caller), then compressing may "fix" those pixels, making the
deserialized version draw differently. bug filed.
- 565 compressess/decompresses to 8888 (at least on Mac), which draws
differently (esp. under some filters). bug filed.

2. BitmapShader did not enforce a copy for mutable bitmaps, but ImageShader does
(since it creates an Image). Thus the former would see subsequent changes to the
pixels after shader creation, while the latter does not, hence the change to the
BlitRow test to avoid this modify-after-create pattern. I sure hope this prev.
behavior was a bug/undefined-behavior, since this CL changes that.

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

to

==========
Always return ImageShader, even from SkShader::MakeBitmapShader

Lessons learned

1. ImageShader (correctly) always compresses (typically via PNG) during
serialization. This has the surprise results of
- if the image was marked opaque, but has some non-opaque pixels (i.e. bug in
blitter or caller), then compressing may "fix" those pixels, making the
deserialized version draw differently. bug filed.
- 565 compressess/decompresses to 8888 (at least on Mac), which draws
differently (esp. under some filters). bug filed.

2. BitmapShader did not enforce a copy for mutable bitmaps, but ImageShader does
(since it creates an Image). Thus the former would see subsequent changes to the
pixels after shader creation, while the latter does not, hence the change to the
BlitRow test to avoid this modify-after-create pattern. I sure hope this prev.
behavior was a bug/undefined-behavior, since this CL changes that.

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

Committed:
https://skia.googlesource.com/skia/+/320a40d7733979703bdf675c31108255e011e34e
==========

[commit-bot: I haz the power, 2016-08-02 13:12:11]

Committed patchset #9 (id:150001) as
https://skia.googlesource.com/skia/+/320a40d7733979703bdf675c31108255e011e34e

[f(malita), 2016-08-02 13:14:35]

On 2016/08/02 13:12:11, commit-bot: I haz the power wrote:
> Committed patchset #9 (id:150001) as
> https://skia.googlesource.com/skia/+/320a40d7733979703bdf675c31108255e011e34e

Ha, I didn't notice the CQ was armed... coffeeeee

[scroggo, 2016-09-08 13:42:23]

scroggo@google.com changed reviewers:
+ scroggo@google.com

[scroggo, 2016-09-08 13:42:24]

> 2. BitmapShader did not enforce a copy for mutable bitmaps, but ImageShader
does (since it creates an Image). Thus the former would see subsequent changes
to the pixels after shader creation, while the latter does not, hence the change
to the BlitRow test to avoid this modify-after-create pattern. I sure hope this
prev. behavior was a bug/undefined-behavior, since this CL changes that.


"this prev. behavior" = "see subsequent changes to the pixels after shader
creation"?

We had a bug tracking that : about.com/3157. There was some concern at the time
about fixing it cheaply.

https://codereview.chromium.org/2195893002/diff/150001/src/core/SkBitmapProcS...
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.chromium.org/2195893002/diff/150001/src/core/SkBitmapProcS...
src/core/SkBitmapProcShader.cpp:207: return s;
This is effectively the same as the old code, right? Either way, we don't need
both.