Add a SkPixelSerializer SkImage encode variant

Add a SkPixelSerializer SkImage encode variant

R=reed@google.com
BUG=skia:4285
Committed: https://skia.googlesource.com/skia/+/2be7125f3274a4b661f50e992fc0eb3c14c24f05

[f(malita), 2015-09-02 12:29:44]

The CQ bit was checked by fmalita@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-02 12:29:48]

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

[commit-bot: I haz the power, 2015-09-02 12:35:26]

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

[commit-bot: I haz the power, 2015-09-02 12:35:26]

Dry run: This issue passed the CQ dry run.

[f(malita), 2015-09-02 12:46:33]

The CQ bit was checked by fmalita@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-02 12:46:43]

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

[commit-bot: I haz the power, 2015-09-02 12:52:16]

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

[commit-bot: I haz the power, 2015-09-02 12:52:16]

Dry run: This issue passed the CQ dry run.

[reed1, 2015-09-02 13:59:58]

lgtm w/ comment requests

https://codereview.chromium.org/1310633006/diff/20001/include/core/SkImage.h
File include/core/SkImage.h (right):

https://codereview.chromium.org/1310633006/diff/20001/include/core/SkImage.h#...
include/core/SkImage.h:228: *  Same as the above, but using the
SkPixelSerializer.
// Can the serialize be null?
// What happens if the serializer returns false?

[f(malita), 2015-09-02 15:18:25]

The CQ bit was checked by fmalita@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-02 15:18:29]

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

[commit-bot: I haz the power, 2015-09-02 15:24:11]

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

[commit-bot: I haz the power, 2015-09-02 15:24:12]

Dry run: This issue passed the CQ dry run.

[f(malita), 2015-09-03 01:32:03]

The CQ bit was checked by fmalita@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-03 01:32:11]

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

[f(malita), 2015-09-03 01:32:56]

Updated per conversation, PTAL.

[commit-bot: I haz the power, 2015-09-03 01:37:16]

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

[commit-bot: I haz the power, 2015-09-03 01:37:17]

Dry run: This issue passed the CQ dry run.

[reed1, 2015-09-03 02:36:13]

lgtm

https://codereview.chromium.org/1310633006/diff/60001/include/core/SkImage.h
File include/core/SkImage.h (right):

https://codereview.chromium.org/1310633006/diff/60001/include/core/SkImage.h#...
include/core/SkImage.h:220: *  the caller must manage (i.e. call unref() when
they are done).
// Note: this will attempt to encode the image's pixels in the specified format,
even if the image returns a data from refEncoded(). That data will be ignored.

https://codereview.chromium.org/1310633006/diff/60001/include/core/SkImage.h#...
include/core/SkImage.h:231: *  If the serializer is NULL, a default PNG encoder
(with quality == 100) will be used.
How about this as a way to organize this wackiness:

- This attempts to return the image as an encoded (compressed) data.
- If the image already has itself in that form (i.e. refEncoded() returns a
data), we return that.
- Else we try to encode the image into PNG.

However, if the caller passes a pixel-serializer, then it can change this
behavior:
- if the image returns data from refEncoded(), that is passed to the serializer,
which can reject it (e.g. that format may not be supported by the intended
recipient).
- if we need to encode/compress the raw pixels, the serializer may perform that,
and return the data in any format it chooses.

Actually I think mine is also confusing. Anyway, I do want us to be explicit
about the contract (e.g. order of what we check when), but wording it is clearly
hard.

If there is a serializer, we completely defer to it. If it fails, we will fail
(return null).

If there is no

[f(malita), 2015-09-03 14:10:27]

https://codereview.chromium.org/1310633006/diff/60001/include/core/SkImage.h
File include/core/SkImage.h (right):

https://codereview.chromium.org/1310633006/diff/60001/include/core/SkImage.h#...
include/core/SkImage.h:220: *  the caller must manage (i.e. call unref() when
they are done).
On 2015/09/03 02:36:13, reed1 wrote:
> // Note: this will attempt to encode the image's pixels in the specified
format,
> even if the image returns a data from refEncoded(). That data will be ignored.

Done.

https://codereview.chromium.org/1310633006/diff/60001/include/core/SkImage.h#...
include/core/SkImage.h:231: *  If the serializer is NULL, a default PNG encoder
(with quality == 100) will be used.
On 2015/09/03 02:36:13, reed1 wrote:
> How about this as a way to organize this wackiness:
> 
> - This attempts to return the image as an encoded (compressed) data.
> - If the image already has itself in that form (i.e. refEncoded() returns a
> data), we return that.
> - Else we try to encode the image into PNG.
> 
> However, if the caller passes a pixel-serializer, then it can change this
> behavior:
> - if the image returns data from refEncoded(), that is passed to the
serializer,
> which can reject it (e.g. that format may not be supported by the intended
> recipient).
> - if we need to encode/compress the raw pixels, the serializer may perform
that,
> and return the data in any format it chooses.
> 
> Actually I think mine is also confusing. Anyway, I do want us to be explicit
> about the contract (e.g. order of what we check when), but wording it is
clearly
> hard.
> 
> If there is a serializer, we completely defer to it. If it fails, we will fail
> (return null).
> 
> If there is no 

(this looks truncated, did you have more text which didn't post?)

Yeah, describing the behavior seems tricky.  Always a bad sign :)

How about we focus on the serializer-present flow, and then describe the
default-serializer (instead of a separate serializer-missing flow)?

Updated, but don't feel strongly and at this point I'll just past whatever you
prefer.

[reed1, 2015-09-03 14:11:44]

I like this new version much better (than my suggestion). LGTM

[f(malita), 2015-09-03 14:12:24]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2015-09-03 14:12:36]

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

[commit-bot: I haz the power, 2015-09-03 14:17:29]

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