Option for SkCodec to treat dst as all zeroes.

Option for SkCodec to treat dst as all zeroes.

This recreates SkImageDecoder's feature to skip writing zeroes for
SkCodec.

Committed: https://skia.googlesource.com/skia/+/9552662e9fee5eb0ef435e52ab9db505d7ebe4ad

[scroggo, 2015-03-05 16:12:22]

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

[scroggo, 2015-03-05 16:12:23]

https://codereview.chromium.org/980903002/diff/1/include/codec/SkCodec.h
File include/codec/SkCodec.h (right):

https://codereview.chromium.org/980903002/diff/1/include/codec/SkCodec.h#newc...
include/codec/SkCodec.h:54: enum ZeroInitialized {
I decided to go with ZeroInitialized instead of SkipZeroes, since the decoder
may not skip zeroes. This seems more accurate as to what's going on.

That said, it may be that a caller happened to use zero initialized memory, but
there's very few zero pixels, so it's not worth skipping. In that case, they
could just not set it to kYes, even though it's technically not true.

https://codereview.chromium.org/980903002/diff/1/include/codec/SkCodec.h#newc...
include/codec/SkCodec.h:75: void setZeroInitialized(ZeroInitialized zero) {
fZeroInitialized = zero; }
It probably makes sense to include this as part of getPixels, although getPixels
already has a long parameter list, and I don't know that this is relevant for
other SkImageGenerators.

[scroggo, 2015-03-05 21:25:11]

PTAL

[reed1, 2015-03-05 22:20:06]

can we chat about this tomorrow?

[scroggo, 2015-03-05 22:30:55]

On 2015/03/05 22:20:06, reed1 wrote:
> can we chat about this tomorrow?

Sure. I just created meeting for the morning.

[scroggo, 2015-03-06 19:44:07]

As discussed in our meeting this morning, I moved ZeroInitialized to getPixels.
PTAL.

[djsollen, 2015-03-06 20:41:57]

https://codereview.chromium.org/980903002/diff/20001/include/core/SkImageGene...
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/980903002/diff/20001/include/core/SkImageGene...
include/core/SkImageGenerator.h:18: //#define
SK_SUPPORT_LEGACY_OPTIONLESS_GET_PIXELS
remove this

https://codereview.chromium.org/980903002/diff/20001/include/core/SkImageGene...
include/core/SkImageGenerator.h:146: ZeroInitialized fZeroInitialized;
I could go either way, but why not bool fZeroInitializedMemory?  What does the
enum get you?

[scroggo, 2015-03-06 20:54:25]

https://codereview.chromium.org/980903002/diff/20001/include/core/SkImageGene...
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/980903002/diff/20001/include/core/SkImageGene...
include/core/SkImageGenerator.h:18: //#define
SK_SUPPORT_LEGACY_OPTIONLESS_GET_PIXELS
On 2015/03/06 20:41:56, djsollen wrote:
> remove this

I don't think we have an official position, but typically we place this in the
header where it's meaningful.

https://codereview.chromium.org/980903002/diff/20001/include/core/SkImageGene...
include/core/SkImageGenerator.h:146: ZeroInitialized fZeroInitialized;
On 2015/03/06 20:41:57, djsollen wrote:
> I could go either way, but why not bool fZeroInitializedMemory? 

I'm fine with adding Memory to the name.

> What does the enum get you?

Clarity. When I look at a caller of SkSwizzler::Create, e.g. in SkPngCodec:

  SkSwizzler::CreateSwizzler(sc, colors, requestedInfo,dst, rowBytes, false)

it's not obvious what "false" means. However, if it's written as:

  SkSwizzler::CreateSwizzler(sc, colors, requestedInfo,dst, rowBytes,
                             kNo_ZeroInitialized)

it's obvious what the caller wanted (maybe it should be
kNot_ZeroInitialized[Memory]? and then kIs_ZeroInitialized[Memory] to match?).
It also means we don't have to remember whether the bool meant zero initialized
or the opposite.

This is another instance where nothing has been codified, but in a very
unscientific survey, reed@ likes this convention (enum instead of bool for API),
and others have supported it as well.

[djsollen, 2015-03-06 21:14:49]

lgtm

https://codereview.chromium.org/980903002/diff/20001/include/core/SkImageGene...
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/980903002/diff/20001/include/core/SkImageGene...
include/core/SkImageGenerator.h:146: ZeroInitialized fZeroInitialized;
The API as it stands is fine with me.

[scroggo, 2015-03-17 11:55:33]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-03-17 11:55:33]

The patchset sent to the CQ was uploaded after l-g-t-m from djsollen@google.com
Link to the patchset: https://codereview.chromium.org/980903002/#ps60001 (title:
"Update SkBmpCodec to use new ZeroInitialized API.")

[commit-bot: I haz the power, 2015-03-17 11:55:40]

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

[commit-bot: I haz the power, 2015-03-17 12:02:21]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/9552662e9fee5eb0ef435e52ab9db505d7ebe4ad