add pipecanvas

add pipecanvas

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

Committed: https://skia.googlesource.com/skia/+/54dc4878b02765efea39e68b218df1e4bfff4b88

[reed1, 2016-08-03 21:17:32]

Description was changed from

==========
add pipecanvas

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

to

==========
add pipecanvas

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

[reed1, 2016-08-04 03:33:17]

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

[commit-bot: I haz the power, 2016-08-04 03:33: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-08-04 03:35:52]

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

[commit-bot: I haz the power, 2016-08-04 03:35:52]

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

[reed1, 2016-08-04 03:37:02]

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

[commit-bot: I haz the power, 2016-08-04 03:37:16]

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-04 03:41:03]

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

[commit-bot: I haz the power, 2016-08-04 03:41:04]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-GCC-x86_64-Release-GN-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[reed1, 2016-08-04 12:13:55]

reed@google.com changed reviewers:
+ mtklein@google.com

[reed1, 2016-08-04 12:40:01]

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

[commit-bot: I haz the power, 2016-08-04 12:40:12]

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-04 12:43:33]

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

[commit-bot: I haz the power, 2016-08-04 12:43:34]

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...)

[mtklein, 2016-08-04 12:50:38]

https://codereview.chromium.org/2201323003/diff/80001/include/core/SkWriteBuf...
File include/core/SkWriteBuffer.h (right):

https://codereview.chromium.org/2201323003/diff/80001/include/core/SkWriteBuf...
include/core/SkWriteBuffer.h:34: virtual void write(const void*, size_t) {
sk_throw(); }
The other implementation of SkWriteBuffer isn't writing bytes, so I don't think
it'll ever be able to be implemented.

Why don't we just keep it non-virtual on SkBinaryWriteBuffer?

https://codereview.chromium.org/2201323003/diff/80001/include/core/SkWriteBuf...
include/core/SkWriteBuffer.h:67: void setDedupers(SkDeduper* images, SkDeduper*
typefaces) {
We can probably just put these on SkBinaryWriteBuffer too?

https://codereview.chromium.org/2201323003/diff/80001/src/pipe/SkPipeCanvas.cpp
File src/pipe/SkPipeCanvas.cpp (right):

https://codereview.chromium.org/2201323003/diff/80001/src/pipe/SkPipeCanvas.c...
src/pipe/SkPipeCanvas.cpp:31: char tmp[4];
Or,

uint32_t tmp = 0;
memcpy(&tmp, src, len);
writer->write(&tmp, 4);

https://codereview.chromium.org/2201323003/diff/80001/src/pipe/SkPipeCanvas.c...
src/pipe/SkPipeCanvas.cpp:46: ScopedWriter(SkPipeCanvas*) :
SkBinaryWriteBuffer(0) {}
This probably doesn't need to inherit from SkBinaryWriteBuffer?  Is that just so
we don't have to retype the write() methods, or are we passing it somewhere that
needs to be an SkWriteBuffer?

I take it the idea is that at some point we'll call setDedupers() and in the
destructor we'll flush to fStream?

https://codereview.chromium.org/2201323003/diff/80001/src/pipe/SkPipeCanvas.c...
src/pipe/SkPipeCanvas.cpp:61: bits |= (paint.getTextScaleX() !=
kTextScaleX_Default   ? kTextSize_NonDef : 0);
I bet scale and skew stay default with a non-default size pretty often.

https://codereview.chromium.org/2201323003/diff/80001/src/pipe/SkPipeCanvas.c...
src/pipe/SkPipeCanvas.cpp:112: #define CHECK_WRITE_SCALAR(bit, code)   \
Might help readability to pass nondef in too.

https://codereview.chromium.org/2201323003/diff/80001/src/pipe/SkPipeCanvas.c...
src/pipe/SkPipeCanvas.cpp:177: this->writeVerb(kSave_Verb);
We should probably funnel all verbs through the same way? 
write32(pack_verb(...)) seems fine to me... where is writeVerb() anyway?

https://codereview.chromium.org/2201323003/diff/80001/src/pipe/SkPipeCanvas.c...
src/pipe/SkPipeCanvas.cpp:232: fStream->write32(pack_verb(kClipRect_Verb,
((unsigned)op << 1) | edgeStyle));
static inline pack_clip(SkRegion::Op, ClipEdgeStyle = kHard)  ?

https://codereview.chromium.org/2201323003/diff/80001/src/pipe/SkPipeFormat.h
File src/pipe/SkPipeFormat.h (right):

https://codereview.chromium.org/2201323003/diff/80001/src/pipe/SkPipeFormat.h...
src/pipe/SkPipeFormat.h:13: enum Verb {
Should probably namespace all these symbols somehow, e.g. namespace SkPipe

https://codereview.chromium.org/2201323003/diff/80001/src/pipe/SkPipeFormat.h...
src/pipe/SkPipeFormat.h:13: enum Verb {
: uint8_t

?

[reed1, 2016-08-04 13:18:41]

https://codereview.chromium.org/2201323003/diff/80001/include/core/SkWriteBuf...
File include/core/SkWriteBuffer.h (right):

https://codereview.chromium.org/2201323003/diff/80001/include/core/SkWriteBuf...
include/core/SkWriteBuffer.h:34: virtual void write(const void*, size_t) {
sk_throw(); }
On 2016/08/04 12:50:37, mtklein wrote:
> The other implementation of SkWriteBuffer isn't writing bytes, so I don't
think
> it'll ever be able to be implemented.
> 
> Why don't we just keep it non-virtual on SkBinaryWriteBuffer?

Makes sense. I hadn't internalized that I was going to require that my writer be
a subclass of BinaryWriter, but of course it will be.

https://codereview.chromium.org/2201323003/diff/80001/src/pipe/SkPipeCanvas.cpp
File src/pipe/SkPipeCanvas.cpp (right):

https://codereview.chromium.org/2201323003/diff/80001/src/pipe/SkPipeCanvas.c...
src/pipe/SkPipeCanvas.cpp:31: char tmp[4];
On 2016/08/04 12:50:37, mtklein wrote:
> Or,
> 
> uint32_t tmp = 0;
> memcpy(&tmp, src, len);
> writer->write(&tmp, 4);

Nice. Considered that, but got temporarily confused about endianness (which
doesn't matter)

https://codereview.chromium.org/2201323003/diff/80001/src/pipe/SkPipeCanvas.c...
src/pipe/SkPipeCanvas.cpp:46: ScopedWriter(SkPipeCanvas*) :
SkBinaryWriteBuffer(0) {}
On 2016/08/04 12:50:37, mtklein wrote:
> This probably doesn't need to inherit from SkBinaryWriteBuffer?  Is that just
so
> we don't have to retype the write() methods, or are we passing it somewhere
that
> needs to be an SkWriteBuffer?
> 
> I take it the idea is that at some point we'll call setDedupers() and in the
> destructor we'll flush to fStream?

Correct, just wanted a quickie so that syntactically it would build.

https://codereview.chromium.org/2201323003/diff/80001/src/pipe/SkPipeCanvas.c...
src/pipe/SkPipeCanvas.cpp:61: bits |= (paint.getTextScaleX() !=
kTextScaleX_Default   ? kTextSize_NonDef : 0);
On 2016/08/04 12:50:37, mtklein wrote:
> I bet scale and skew stay default with a non-default size pretty often.

That's a typo, fixed.

https://codereview.chromium.org/2201323003/diff/80001/src/pipe/SkPipeCanvas.c...
src/pipe/SkPipeCanvas.cpp:112: #define CHECK_WRITE_SCALAR(bit, code)   \
On 2016/08/04 12:50:37, mtklein wrote:
> Might help readability to pass nondef in too.

... and writer? I was sort of modeling other macro helpers that have implicit
parameters, but I'll add those explicitly

https://codereview.chromium.org/2201323003/diff/80001/src/pipe/SkPipeCanvas.c...
src/pipe/SkPipeCanvas.cpp:177: this->writeVerb(kSave_Verb);
On 2016/08/04 12:50:37, mtklein wrote:
> We should probably funnel all verbs through the same way? 
> write32(pack_verb(...)) seems fine to me... where is writeVerb() anyway?

Done

https://codereview.chromium.org/2201323003/diff/80001/src/pipe/SkPipeCanvas.c...
src/pipe/SkPipeCanvas.cpp:232: fStream->write32(pack_verb(kClipRect_Verb,
((unsigned)op << 1) | edgeStyle));
On 2016/08/04 12:50:37, mtklein wrote:
> static inline pack_clip(SkRegion::Op, ClipEdgeStyle = kHard)  ?

Yea, there are lots of helpers like this I will write (e.g. clip, text, image)
that have custom "extra" packings.

https://codereview.chromium.org/2201323003/diff/80001/src/pipe/SkPipeFormat.h
File src/pipe/SkPipeFormat.h (right):

https://codereview.chromium.org/2201323003/diff/80001/src/pipe/SkPipeFormat.h...
src/pipe/SkPipeFormat.h:13: enum Verb {
On 2016/08/04 12:50:38, mtklein wrote:
> Should probably namespace all these symbols somehow, e.g. namespace SkPipe

Definitely, just going fast at the moment

[reed1, 2016-08-04 13:18:51]

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

[commit-bot: I haz the power, 2016-08-04 13:19:04]

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-04 13:22:45]

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

[commit-bot: I haz the power, 2016-08-04 13:22:45]

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...)
  Build-Ubuntu-GCC-x86_64-Release-GN-Trybot on master.client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[reed1, 2016-08-04 13:28:45]

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

[commit-bot: I haz the power, 2016-08-04 13:28:51]

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-04 13:59:15]

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

[commit-bot: I haz the power, 2016-08-04 13:59:16]

Dry run: This issue passed the CQ dry run.

[mtklein, 2016-08-04 15:13:23]

all sounds good

https://codereview.chromium.org/2201323003/diff/80001/src/pipe/SkPipeCanvas.cpp
File src/pipe/SkPipeCanvas.cpp (right):

https://codereview.chromium.org/2201323003/diff/80001/src/pipe/SkPipeCanvas.c...
src/pipe/SkPipeCanvas.cpp:112: #define CHECK_WRITE_SCALAR(bit, code)   \
On 2016/08/04 13:18:41, reed1 wrote:
> On 2016/08/04 12:50:37, mtklein wrote:
> > Might help readability to pass nondef in too.
> 
> ... and writer? I was sort of modeling other macro helpers that have implicit
> parameters, but I'll add those explicitly

The rule of thumb I try to stick to is that implicit parameters must outlive the
macro in textual scope, so it's always clear what those params are.  I might not
have done so well...

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

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

[commit-bot: I haz the power, 2016-08-05 02:44:38]

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-05 02:50:07]

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

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

Dry run: Try jobs failed on following builders:
  Build-Win-MSVC-x86_64-Debug-Trybot on master.client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[reed1, 2016-08-05 11:20:09]

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

[commit-bot: I haz the power, 2016-08-05 11:20:13]

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-05 11:36:55]

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

[commit-bot: I haz the power, 2016-08-05 11:36:55]

Dry run: This issue passed the CQ dry run.

[reed1, 2016-08-10 21:35:35]

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

[commit-bot: I haz the power, 2016-08-10 21:35:47]

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-10 22:03:04]

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

[commit-bot: I haz the power, 2016-08-10 22:03:05]

Dry run: This issue passed the CQ dry run.

[reed1, 2016-08-17 17:10:08]

Not finished, but would appreciate a round of reviewing, to be sure the general
direction/pattern makes sense.

+ nearly all drawing calls are supported
- no custom callbacks yet
- no budgeting yet
- not optimzed yet (need to look at some real data-sets)

[reed1, 2016-08-17 17:15:21]

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

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

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-17 17:15:28]

The CQ bit was unchecked by reed@google.com

[reed1, 2016-08-17 17:19:34]

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

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

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-17 17:49:26]

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

[commit-bot: I haz the power, 2016-08-17 17:49:27]

Dry run: This issue passed the CQ dry run.

[mtklein, 2016-08-26 18:43:24]

Anything significant left beyond typefaces and setMatrix() ?

https://codereview.chromium.org/2201323003/diff/420001/src/core/SkReadBuffer.h
File src/core/SkReadBuffer.h (right):

https://codereview.chromium.org/2201323003/diff/420001/src/core/SkReadBuffer....
src/core/SkReadBuffer.h:222: sk_sp<SkImage> inflateImage();
Is this implemented?  Is it different than readImage()?

https://codereview.chromium.org/2201323003/diff/420001/src/pipe/SkPipeCanvas.cpp
File src/pipe/SkPipeCanvas.cpp (right):

https://codereview.chromium.org/2201323003/diff/420001/src/pipe/SkPipeCanvas....
src/pipe/SkPipeCanvas.cpp:279: // Can we eliminate this?
Ah ha, was wondering how you handled this.

If we switch this to willSetMatrix(), can't we translate set into concat via
invert?

https://codereview.chromium.org/2201323003/diff/420001/src/pipe/SkPipeFormat.h
File src/pipe/SkPipeFormat.h (right):

https://codereview.chromium.org/2201323003/diff/420001/src/pipe/SkPipeFormat....
src/pipe/SkPipeFormat.h:16: enum class SkPipeVerb {
may want to : uint8_t this to make it explicit?

https://codereview.chromium.org/2201323003/diff/420001/src/pipe/SkPipeReader.cpp
File src/pipe/SkPipeReader.cpp (right):

https://codereview.chromium.org/2201323003/diff/420001/src/pipe/SkPipeReader....
src/pipe/SkPipeReader.cpp:167: if (nondef & kTypeface_NonDef) {
:|

[reed1, 2016-08-27 14:14:55]

https://codereview.chromium.org/2201323003/diff/420001/src/core/SkReadBuffer.h
File src/core/SkReadBuffer.h (right):

https://codereview.chromium.org/2201323003/diff/420001/src/core/SkReadBuffer....
src/core/SkReadBuffer.h:222: sk_sp<SkImage> inflateImage();
On 2016/08/26 18:43:23, mtklein wrote:
> Is this implemented?  Is it different than readImage()?

Gone. Was not implemented or called.

https://codereview.chromium.org/2201323003/diff/420001/src/pipe/SkPipeFormat.h
File src/pipe/SkPipeFormat.h (right):

https://codereview.chromium.org/2201323003/diff/420001/src/pipe/SkPipeFormat....
src/pipe/SkPipeFormat.h:16: enum class SkPipeVerb {
On 2016/08/26 18:43:23, mtklein wrote:
> may want to : uint8_t this to make it explicit?

Done.

[reed1, 2016-08-31 10:21:30]

reed@google.com changed reviewers:
+ vollick@chromium.org

[reed1, 2016-09-10 17:59:06]

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

[commit-bot: I haz the power, 2016-09-10 17:59:14]

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

[reed1, 2016-09-10 18:00:36]

ptal

given a blacklist derived from serialize-8888, this appears to pass dm

> dm --config pipe-8888

Plenty of tweaking to do around budgeting and optimization (size of transport),
but I think its ready for an initial landing.

[commit-bot: I haz the power, 2016-09-10 18:01:32]

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

[commit-bot: I haz the power, 2016-09-10 18:01:34]

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-09-10 18:07:49]

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

[commit-bot: I haz the power, 2016-09-10 18:07:53]

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-09-10 18:33:50]

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

[commit-bot: I haz the power, 2016-09-10 18:33:51]

Dry run: This issue passed the CQ dry run.

[reed1, 2016-09-12 19:20:06]

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

[commit-bot: I haz the power, 2016-09-12 19:20:11]

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-09-12 19:25:00]

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

[commit-bot: I haz the power, 2016-09-12 19:25:01]

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-09-12 19:31:57]

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

[commit-bot: I haz the power, 2016-09-12 19:32:09]

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

[mtklein_C, 2016-09-12 19:47:54]

mtklein@chromium.org changed reviewers:
+ mtklein@chromium.org

[mtklein_C, 2016-09-12 19:47:59]

lgtm

I've got a bunch little nits here, and sort of got tired while reading, but I'm
on board to land and iterate.

https://codereview.chromium.org/2201323003/diff/640001/dm/DMSrcSink.h
File dm/DMSrcSink.h (right):

https://codereview.chromium.org/2201323003/diff/640001/dm/DMSrcSink.h#newcode341
dm/DMSrcSink.h:341: const char* fileExtension() const override { return "skp"; }
are you intentionally punning .skp?

https://codereview.chromium.org/2201323003/diff/640001/include/private/SkRefS...
File include/private/SkRefSet.h (right):

https://codereview.chromium.org/2201323003/diff/640001/include/private/SkRefS...
include/private/SkRefSet.h:31: }
The particular invariants of this class (values must start non-null, but can be
later reset to null) seem SkPipeReader-specific.  I think you might want to move
this into SkPipeReader.cpp?

https://codereview.chromium.org/2201323003/diff/640001/samplecode/SampleApp.cpp
File samplecode/SampleApp.cpp (right):

https://codereview.chromium.org/2201323003/diff/640001/samplecode/SampleApp.c...
samplecode/SampleApp.cpp:560: return sk_sp<SkTypeface>(tf);
// with the ref from tf->ref() above in serialize().

https://codereview.chromium.org/2201323003/diff/640001/samplecode/SampleApp.c...
samplecode/SampleApp.cpp:1407: delete fPipeStream;
Probably better as an std::unique_ptr?

https://codereview.chromium.org/2201323003/diff/640001/src/core/SkPipe.h
File src/core/SkPipe.h (right):

https://codereview.chromium.org/2201323003/diff/640001/src/core/SkPipe.h#newc...
src/core/SkPipe.h:25: void setTypefaceSerializer(SkTypefaceSerializer*);
Ownership transfer here is unclear.  This reads like it takes no ownership, but
in SampleApp we call setTypefaceSerializer(new ...) directly.  Might wanna pass
this by std::unique_ptr?

https://codereview.chromium.org/2201323003/diff/640001/src/core/SkPipe.h#newc...
src/core/SkPipe.h:37: Impl* fImpl;
The usual idiom these days is

private:
  class Impl;
  std::unique_ptr<Impl> fImpl;

This'll work as long as your destructor is defined in the same translation unit
as Impl is defined (as is true here for both your types.)

https://codereview.chromium.org/2201323003/diff/640001/src/core/SkPipe.h#newc...
src/core/SkPipe.h:45: void setTypefaceDeserializer(SkTypefaceDeserializer*);
Ditto.

https://codereview.chromium.org/2201323003/diff/640001/src/core/SkPipe.h#newc...
src/core/SkPipe.h:58: class SkTypefaceSerializer {
Another option you might want to consider for this sort of thing is

using SkTypefaceSerializer   = std::function<sk_sp<SkData> (SkTypeface*)>;
using SkTypefaceDeserializer = std::function<SkTypeface* (const void*, size_t)>;

https://codereview.chromium.org/2201323003/diff/640001/src/core/SkReadBuffer.cpp
File src/core/SkReadBuffer.cpp (right):

https://codereview.chromium.org/2201323003/diff/640001/src/core/SkReadBuffer....
src/core/SkReadBuffer.cpp:308: return
sk_ref_sp(fInflator->getTypeface(this->read32()));
No chance for null here?

https://codereview.chromium.org/2201323003/diff/640001/src/core/SkWriteBuffer...
File src/core/SkWriteBuffer.cpp (right):

https://codereview.chromium.org/2201323003/diff/640001/src/core/SkWriteBuffer...
src/core/SkWriteBuffer.cpp:195: if (this->newWriteImage(image)) {
Doesn't really seem like newWriteImage() and newWriteTypeface() need to be their
own methods.  It seems clearer to read the inlined version.

if (fDeduper) {
    this->write32(...);
    return;
}

https://codereview.chromium.org/2201323003/diff/640001/src/pipe/SkPipeCanvas.cpp
File src/pipe/SkPipeCanvas.cpp (right):

https://codereview.chromium.org/2201323003/diff/640001/src/pipe/SkPipeCanvas....
src/pipe/SkPipeCanvas.cpp:1030: void SkPipeSerializer::write(SkImage*,
SkWStream* stream) {
Probably shouldn't leave this in half-implemented.

https://codereview.chromium.org/2201323003/diff/640001/src/pipe/SkPipeFormat.h
File src/pipe/SkPipeFormat.h (right):

https://codereview.chromium.org/2201323003/diff/640001/src/pipe/SkPipeFormat....
src/pipe/SkPipeFormat.h:139: kHasMatrix_DrawPictureExtra = 1 << 21,
Are these mean to fit in the 3 user bits above?  Worth noting that explicitly?

https://codereview.chromium.org/2201323003/diff/640001/src/pipe/SkPipeFormat....
src/pipe/SkPipeFormat.h:182: kCount_DrawImageLatticeMask     = 0xFF,     //
sentinel for 32bits follow,
Let's use 0?

https://codereview.chromium.org/2201323003/diff/640001/src/pipe/SkPipeReader.cpp
File src/pipe/SkPipeReader.cpp (right):

https://codereview.chromium.org/2201323003/diff/640001/src/pipe/SkPipeReader....
src/pipe/SkPipeReader.cpp:160: packedFlags >>= 2;
// Currently unused.

https://codereview.chromium.org/2201323003/diff/640001/src/pipe/SkPipeReader....
src/pipe/SkPipeReader.cpp:163: paint.setHinting((SkPaint::Hinting)(packedFlags &
2));              packedFlags >>= 2;
Is &2 a typo?

https://codereview.chromium.org/2201323003/diff/640001/src/pipe/SkPipeReader....
src/pipe/SkPipeReader.cpp:533: if (packedVerb & 1) {
Can you take a pass through replacing & N with & kHasPaint_BlahBlahMask ?

https://codereview.chromium.org/2201323003/diff/640001/src/pipe/SkPipeReader....
src/pipe/SkPipeReader.cpp:655: static sk_sp<SkImage> make_from_skiacodc(const
void* encoded, size_t encodedSize) {
We might not want to further overload the term codec... I don't think skia_raw
would confuse anyone too much.

https://codereview.chromium.org/2201323003/diff/640001/src/pipe/SkPipeReader....
src/pipe/SkPipeReader.cpp:727: // zero-index means we are "forgetting" that
cache entry
We might want to call forget "undefine".  Forget sounds accidental.

https://codereview.chromium.org/2201323003/diff/640001/src/pipe/SkPipeReader....
src/pipe/SkPipeReader.cpp:826: HANDLER(endPicture),
Might wanna just { nullptr, "endPicture" } // Handled specially.  ?

https://codereview.chromium.org/2201323003/diff/640001/src/pipe/SkPipeReader....
src/pipe/SkPipeReader.cpp:854: uint32_t header;
What's the value of these 4-byte headers?  Would anything go wrong if we just
didn't write those four bytes (and obviously, didn't try to read them)?

[mtklein_C, 2016-09-12 19:48:01]

lgtm

I've got a bunch little nits here, and sort of got tired while reading, but I'm
on board to land and iterate.

[commit-bot: I haz the power, 2016-09-12 20:11:40]

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

[commit-bot: I haz the power, 2016-09-12 20:11:41]

Dry run: This issue passed the CQ dry run.

[reed1, 2016-09-12 21:35:24]

https://codereview.chromium.org/2201323003/diff/640001/dm/DMSrcSink.h
File dm/DMSrcSink.h (right):

https://codereview.chromium.org/2201323003/diff/640001/dm/DMSrcSink.h#newcode341
dm/DMSrcSink.h:341: const char* fileExtension() const override { return "skp"; }
On 2016/09/12 19:47:58, mtklein_C wrote:
> are you intentionally punning .skp?

Oops, copy-paste error.

https://codereview.chromium.org/2201323003/diff/640001/include/private/SkRefS...
File include/private/SkRefSet.h (right):

https://codereview.chromium.org/2201323003/diff/640001/include/private/SkRefS...
include/private/SkRefSet.h:31: }
On 2016/09/12 19:47:58, mtklein_C wrote:
> The particular invariants of this class (values must start non-null, but can
be
> later reset to null) seem SkPipeReader-specific.  I think you might want to
move
> this into SkPipeReader.cpp?

Done.

https://codereview.chromium.org/2201323003/diff/640001/samplecode/SampleApp.cpp
File samplecode/SampleApp.cpp (right):

https://codereview.chromium.org/2201323003/diff/640001/samplecode/SampleApp.c...
samplecode/SampleApp.cpp:1407: delete fPipeStream;
On 2016/09/12 19:47:58, mtklein_C wrote:
> Probably better as an std::unique_ptr?

Done.

https://codereview.chromium.org/2201323003/diff/640001/src/core/SkPipe.h
File src/core/SkPipe.h (right):

https://codereview.chromium.org/2201323003/diff/640001/src/core/SkPipe.h#newc...
src/core/SkPipe.h:25: void setTypefaceSerializer(SkTypefaceSerializer*);
On 2016/09/12 19:47:58, mtklein_C wrote:
> Ownership transfer here is unclear.  This reads like it takes no ownership,
but
> in SampleApp we call setTypefaceSerializer(new ...) directly.  Might wanna
pass
> this by std::unique_ptr?

updated dox

https://codereview.chromium.org/2201323003/diff/640001/src/core/SkPipe.h#newc...
src/core/SkPipe.h:58: class SkTypefaceSerializer {
On 2016/09/12 19:47:58, mtklein_C wrote:
> Another option you might want to consider for this sort of thing is
> 
> using SkTypefaceSerializer   = std::function<sk_sp<SkData> (SkTypeface*)>;
> using SkTypefaceDeserializer = std::function<SkTypeface* (const void*,
size_t)>;

Just keeping my options open as we think more about these. Picture serialization
(the public api) might end up wanting the same sort of object(s), and they may
want more than one method (don't know yet). But I see that if these stay
singe-method, then std::function would be clean.

https://codereview.chromium.org/2201323003/diff/640001/src/core/SkReadBuffer.cpp
File src/core/SkReadBuffer.cpp (right):

https://codereview.chromium.org/2201323003/diff/640001/src/core/SkReadBuffer....
src/core/SkReadBuffer.cpp:308: return
sk_ref_sp(fInflator->getTypeface(this->read32()));
On 2016/09/12 19:47:58, mtklein_C wrote:
> No chance for null here?

sk_ref_sp handles null.

https://codereview.chromium.org/2201323003/diff/640001/src/core/SkWriteBuffer...
File src/core/SkWriteBuffer.cpp (right):

https://codereview.chromium.org/2201323003/diff/640001/src/core/SkWriteBuffer...
src/core/SkWriteBuffer.cpp:195: if (this->newWriteImage(image)) {
On 2016/09/12 19:47:58, mtklein_C wrote:
> Doesn't really seem like newWriteImage() and newWriteTypeface() need to be
their
> own methods.  It seems clearer to read the inlined version.
> 
> if (fDeduper) {
>     this->write32(...);
>     return;
> }

Done.

https://codereview.chromium.org/2201323003/diff/640001/src/pipe/SkPipeReader.cpp
File src/pipe/SkPipeReader.cpp (right):

https://codereview.chromium.org/2201323003/diff/640001/src/pipe/SkPipeReader....
src/pipe/SkPipeReader.cpp:160: packedFlags >>= 2;
On 2016/09/12 19:47:59, mtklein_C wrote:
> // Currently unused.

Done.

https://codereview.chromium.org/2201323003/diff/640001/src/pipe/SkPipeReader....
src/pipe/SkPipeReader.cpp:163: paint.setHinting((SkPaint::Hinting)(packedFlags &
2));              packedFlags >>= 2;
On 2016/09/12 19:47:59, mtklein_C wrote:
> Is &2 a typo?

Done.

https://codereview.chromium.org/2201323003/diff/640001/src/pipe/SkPipeReader....
src/pipe/SkPipeReader.cpp:533: if (packedVerb & 1) {
On 2016/09/12 19:47:59, mtklein_C wrote:
> Can you take a pass through replacing & N with & kHasPaint_BlahBlahMask ?

Will do. I stared using constants later in the process, so you can see which
methods are early-birds

https://codereview.chromium.org/2201323003/diff/640001/src/pipe/SkPipeReader....
src/pipe/SkPipeReader.cpp:655: static sk_sp<SkImage> make_from_skiacodc(const
void* encoded, size_t encodedSize) {
On 2016/09/12 19:47:59, mtklein_C wrote:
> We might not want to further overload the term codec... I don't think skia_raw
> would confuse anyone too much.

Done.

https://codereview.chromium.org/2201323003/diff/640001/src/pipe/SkPipeReader....
src/pipe/SkPipeReader.cpp:727: // zero-index means we are "forgetting" that
cache entry
On 2016/09/12 19:47:59, mtklein_C wrote:
> We might want to call forget "undefine".  Forget sounds accidental.

Done.

https://codereview.chromium.org/2201323003/diff/640001/src/pipe/SkPipeReader....
src/pipe/SkPipeReader.cpp:826: HANDLER(endPicture),
On 2016/09/12 19:47:59, mtklein_C wrote:
> Might wanna just { nullptr, "endPicture" } // Handled specially.  ?

Done.

[reed1, 2016-09-12 21:35:37]

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

[commit-bot: I haz the power, 2016-09-12 21:35:46]

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-09-12 22:14:51]

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

[commit-bot: I haz the power, 2016-09-12 22:14:52]

Dry run: This issue passed the CQ dry run.

[reed1, 2016-09-13 14:23:44]

https://codereview.chromium.org/2201323003/diff/640001/src/pipe/SkPipeCanvas.cpp
File src/pipe/SkPipeCanvas.cpp (right):

https://codereview.chromium.org/2201323003/diff/640001/src/pipe/SkPipeCanvas....
src/pipe/SkPipeCanvas.cpp:1030: void SkPipeSerializer::write(SkImage*,
SkWStream* stream) {
On 2016/09/12 19:47:58, mtklein_C wrote:
> Probably shouldn't leave this in half-implemented.

Done.

https://codereview.chromium.org/2201323003/diff/640001/src/pipe/SkPipeFormat.h
File src/pipe/SkPipeFormat.h (right):

https://codereview.chromium.org/2201323003/diff/640001/src/pipe/SkPipeFormat....
src/pipe/SkPipeFormat.h:139: kHasMatrix_DrawPictureExtra = 1 << 21,
On 2016/09/12 19:47:59, mtklein_C wrote:
> Are these mean to fit in the 3 user bits above?  Worth noting that explicitly?

Done.

[reed1, 2016-09-13 14:23:54]

The CQ bit was checked by reed@google.com

[reed1, 2016-09-13 14:23:55]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@chromium.org
Link to the patchset: https://codereview.chromium.org/2201323003/#ps720001
(title: "add test for writeImage")

[commit-bot: I haz the power, 2016-09-13 14:24:05]

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-09-13 15:09:47]

Description was changed from

==========
add pipecanvas

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

to

==========
add pipecanvas

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

Committed:
https://skia.googlesource.com/skia/+/54dc4878b02765efea39e68b218df1e4bfff4b88
==========

[commit-bot: I haz the power, 2016-09-13 15:09:49]

Committed patchset #37 (id:720001) as
https://skia.googlesource.com/skia/+/54dc4878b02765efea39e68b218df1e4bfff4b88

[hal.canary, 2016-09-13 15:52:52]

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

[hal.canary, 2016-09-13 15:52:54]

https://codereview.chromium.org/2201323003/diff/720001/src/core/SkPipe.h
File src/core/SkPipe.h (right):

https://codereview.chromium.org/2201323003/diff/720001/src/core/SkPipe.h#newc...
src/core/SkPipe.h:25: // Ownership is not transferred, so caller must ceep the
serializer alive
s/ceep/keep/