Ensure blob typeface information survives SkGPipe serialization.

Ensure blob typeface information survives SkGPipe serialization.

When flattening text blobs to the temp buffer, we need to collect
typeface info and ship it across the pipe explicitly.

R=reed@google.com,robertphillips@google.com,mtklein@google.com
BUG=412445
Committed: https://skia.googlesource.com/skia/+/acb882c239c0cfea738fe63ed9de48ddc2ddc76a

[jbroman, 2014-09-12 15:35:35]

wrong bug #?

[f(malita), 2014-09-12 15:37:44]

On 2014/09/12 15:35:35, jbroman wrote:
> wrong bug #?

yes, fixed.

[mtklein, 2014-09-12 15:46:34]

Why are text blobs different from the other text draw ops?

Add a regression test of some sort?

[f(malita), 2014-09-12 15:53:13]

On 2014/09/12 15:46:34, mtklein wrote:
> Why are text blobs different from the other text draw ops?

They have per-run paints/typeface info.

Other draw ops have a single paint which is pushed onto their SkGPipeState, but
a blob is opaque and can encapsulate an arbitrary number of paints/typefaces =>
blobs handle their own serialization into an SkWriteBuffer, but if the buffer
doesn't have a typeface recorder then that info is just lost.

> Add a regression test of some sort?

Yes, I should add one.

[mtklein, 2014-09-12 15:58:40]

On 2014/09/12 15:53:13, Florin Malita wrote:
> On 2014/09/12 15:46:34, mtklein wrote:
> > Why are text blobs different from the other text draw ops?
> 
> They have per-run paints/typeface info.
> 
> Other draw ops have a single paint which is pushed onto their SkGPipeState,
but
> a blob is opaque and can encapsulate an arbitrary number of paints/typefaces
=>
> blobs handle their own serialization into an SkWriteBuffer, but if the buffer
> doesn't have a typeface recorder then that info is just lost.

Gotcha.  Think you could tack a comment to this effect into, say,
GPipeWrite.cpp?

[mtklein, 2014-09-12 16:07:17]

code lgtm

https://codereview.chromium.org/563783003/diff/20001/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.chromium.org/563783003/diff/20001/src/pipe/SkGPipeWrite.cp...
src/pipe/SkGPipeWrite.cpp:955: SkTypeface** array = (SkTypeface**)storage.get();
Out of curiosity, why this cast?  Doesn't storage.get() return an SkTypeface** ?

https://codereview.chromium.org/563783003/diff/20001/src/pipe/SkGPipeWrite.cp...
src/pipe/SkGPipeWrite.cpp:958: size_t size = 2 * sizeof(uint32_t) + 2 *
sizeof(SkScalar) + blobBuffer.bytesWritten();
Can you add some comments explaining what the different parts of size are?

// text byte count + typeface count + x + y + all the text
size_t size = ...
// plus an index (cross-process) or pointer (in-process) for each typeface
size += ...

[reed1, 2014-09-12 16:24:57]

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

[f(malita), 2014-09-12 22:20:49]

While putting together a test, I discovered that cross-process is borked:
getTypefaceID injects a kDef_Typeface_DrawOp in the stream the first when a new
typeface is seen => we can't serialize the blob atomically :(

May need to do one run at a time and teach the pipe state machine about it. Back
to the drawing board...

https://codereview.chromium.org/563783003/diff/20001/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.chromium.org/563783003/diff/20001/src/pipe/SkGPipeWrite.cp...
src/pipe/SkGPipeWrite.cpp:955: SkTypeface** array = (SkTypeface**)storage.get();
On 2014/09/12 16:07:17, mtklein wrote:
> Out of curiosity, why this cast?  Doesn't storage.get() return an SkTypeface**
?

Indeed it does. Removed.

https://codereview.chromium.org/563783003/diff/20001/src/pipe/SkGPipeWrite.cp...
src/pipe/SkGPipeWrite.cpp:958: size_t size = 2 * sizeof(uint32_t) + 2 *
sizeof(SkScalar) + blobBuffer.bytesWritten();
On 2014/09/12 16:07:17, mtklein wrote:
> Can you add some comments explaining what the different parts of size are?
> 
> // text byte count + typeface count + x + y + all the text
> size_t size = ...
> // plus an index (cross-process) or pointer (in-process) for each typeface
> size += ...

Done.

[f(malita), 2014-09-12 22:26:49]

On 2014/09/12 22:20:49, Florin Malita wrote:
> While putting together a test, I discovered that cross-process is borked:
> getTypefaceID injects a kDef_Typeface_DrawOp in the stream the first when a
new
> typeface is seen => we can't serialize the blob atomically :(
> 
> May need to do one run at a time and teach the pipe state machine about it.
Back
> to the drawing board...

On second thought, it may not be that bad: hoisting the id lookup before we
start writing the blob may suffice. Checking.

[f(malita), 2014-09-12 22:59:29]

Updated, PTAL.

[mtklein, 2014-09-12 23:09:29]

https://codereview.chromium.org/563783003/diff/40001/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.chromium.org/563783003/diff/40001/src/pipe/SkGPipeWrite.cp...
src/pipe/SkGPipeWrite.cpp:969: typefaceIDs[i] = this->getTypefaceID(array[i]);
I can't help but think that if onDrawTextBlob() looked like this
{
  isCrossProcess(fFlags) ? this->blobCrossProcess(blob,x,y,p)
                         : this->blobInProcess(blob,x,y,p);
}

we'd be much less inclined to do weird things with memory and casts that require
explanatory comments.

[reed1, 2014-09-15 14:13:40]

https://codereview.chromium.org/563783003/diff/40001/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.chromium.org/563783003/diff/40001/src/pipe/SkGPipeWrite.cp...
src/pipe/SkGPipeWrite.cpp:969: typefaceIDs[i] = this->getTypefaceID(array[i]);
On 2014/09/12 23:09:29, mtklein wrote:
> I can't help but think that if onDrawTextBlob() looked like this
> {
>   isCrossProcess(fFlags) ? this->blobCrossProcess(blob,x,y,p)
>                          : this->blobInProcess(blob,x,y,p);
> }
> 
> we'd be much less inclined to do weird things with memory and casts that
require
> explanatory comments.

Hmm, seems like most of this function is common w/ or w/o xprocess. What if we
just had an either/or set of helpers to build the typeface list?

[robertphillips, 2014-09-15 18:13:03]

https://codereview.chromium.org/563783003/diff/40001/src/pipe/SkGPipeRead.cpp
File src/pipe/SkGPipeRead.cpp (right):

https://codereview.chromium.org/563783003/diff/40001/src/pipe/SkGPipeRead.cpp...
src/pipe/SkGPipeRead.cpp:196: 
Should/can this be a "const SkTypeface*" ?

https://codereview.chromium.org/563783003/diff/40001/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.chromium.org/563783003/diff/40001/src/pipe/SkGPipeWrite.cp...
src/pipe/SkGPipeWrite.cpp:38: 
is_cross_process ?

[f(malita), 2014-09-15 21:51:19]

https://codereview.chromium.org/563783003/diff/40001/src/pipe/SkGPipeRead.cpp
File src/pipe/SkGPipeRead.cpp (right):

https://codereview.chromium.org/563783003/diff/40001/src/pipe/SkGPipeRead.cpp...
src/pipe/SkGPipeRead.cpp:196: 
On 2014/09/15 18:13:03, robertphillips wrote:
> Should/can this be a "const SkTypeface*" ?

It seems that it should be, but the current clients really want a non-const
pointer so not sure how intrusive the cleanup may be. We can look at it in a
followup.

https://codereview.chromium.org/563783003/diff/40001/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.chromium.org/563783003/diff/40001/src/pipe/SkGPipeWrite.cp...
src/pipe/SkGPipeWrite.cpp:38: 
On 2014/09/15 18:13:03, robertphillips wrote:
> is_cross_process ?

Done.

https://codereview.chromium.org/563783003/diff/40001/src/pipe/SkGPipeWrite.cp...
src/pipe/SkGPipeWrite.cpp:969: typefaceIDs[i] = this->getTypefaceID(array[i]);
On 2014/09/15 14:13:39, reed1 wrote:
> On 2014/09/12 23:09:29, mtklein wrote:
> > I can't help but think that if onDrawTextBlob() looked like this
> > {
> >   isCrossProcess(fFlags) ? this->blobCrossProcess(blob,x,y,p)
> >                          : this->blobInProcess(blob,x,y,p);
> > }
> > 
> > we'd be much less inclined to do weird things with memory and casts that
> require
> > explanatory comments.
> 
> Hmm, seems like most of this function is common w/ or w/o xprocess. What if we
> just had an either/or set of helpers to build the typeface list?

Added a couple of helpers to munge the typeface buffer, does it look nicer?

[mtklein, 2014-09-15 22:13:14]

https://codereview.chromium.org/563783003/diff/60001/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.chromium.org/563783003/diff/60001/src/pipe/SkGPipeWrite.cp...
src/pipe/SkGPipeWrite.cpp:955: typefaceSet.copyToArray((SkRefCnt**)buffer);
Is there any hope for writing typefaceSet.copyIDsToArray(uint32_t*), so we never
have to do the two passes and memory punning?

[f(malita), 2014-09-15 23:46:26]

On 2014/09/15 22:13:14, mtklein wrote:
> https://codereview.chromium.org/563783003/diff/60001/src/pipe/SkGPipeWrite.cpp
> File src/pipe/SkGPipeWrite.cpp (right):
> 
>
https://codereview.chromium.org/563783003/diff/60001/src/pipe/SkGPipeWrite.cp...
> src/pipe/SkGPipeWrite.cpp:955: typefaceSet.copyToArray((SkRefCnt**)buffer);
> Is there any hope for writing typefaceSet.copyIDsToArray(uint32_t*), so we
never
> have to do the two passes and memory punning?

Unfortunately typefaceSet is a lower-level data structure (an opaque SkRefCntSet
collection) which has no notion of what its members are -- so adding a
full-blown ID converter is not feasible.

But I think a simple set iterator would also solve the problem nicely. I'll look
into adding one (unless it was omitted on purpose to not constrain the
implementation) and see how it plays out.

[mtklein, 2014-09-16 15:47:04]

On 2014/09/15 23:46:26, Florin Malita wrote:
> On 2014/09/15 22:13:14, mtklein wrote:
> >
https://codereview.chromium.org/563783003/diff/60001/src/pipe/SkGPipeWrite.cpp
> > File src/pipe/SkGPipeWrite.cpp (right):
> > 
> >
>
https://codereview.chromium.org/563783003/diff/60001/src/pipe/SkGPipeWrite.cp...
> > src/pipe/SkGPipeWrite.cpp:955: typefaceSet.copyToArray((SkRefCnt**)buffer);
> > Is there any hope for writing typefaceSet.copyIDsToArray(uint32_t*), so we
> never
> > have to do the two passes and memory punning?
> 
> Unfortunately typefaceSet is a lower-level data structure (an opaque
SkRefCntSet
> collection) which has no notion of what its members are -- so adding a
> full-blown ID converter is not feasible.
> 
> But I think a simple set iterator would also solve the problem nicely. I'll
look
> into adding one (unless it was omitted on purpose to not constrain the
> implementation) and see how it plays out.

SGTM.  I think it's fair to say that SkRefCntSet has few users outside of pipe
and the old picture backend.  I think you can even specialize it into
SkTypefaceSet without breaking anyone.

[f(malita), 2014-09-16 23:14:32]

On 2014/09/16 15:47:04, mtklein wrote:
> On 2014/09/15 23:46:26, Florin Malita wrote:
> > On 2014/09/15 22:13:14, mtklein wrote:
> > >
> https://codereview.chromium.org/563783003/diff/60001/src/pipe/SkGPipeWrite.cpp
> > > File src/pipe/SkGPipeWrite.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/563783003/diff/60001/src/pipe/SkGPipeWrite.cp...
> > > src/pipe/SkGPipeWrite.cpp:955:
typefaceSet.copyToArray((SkRefCnt**)buffer);
> > > Is there any hope for writing typefaceSet.copyIDsToArray(uint32_t*), so we
> > never
> > > have to do the two passes and memory punning?
> > 
> > Unfortunately typefaceSet is a lower-level data structure (an opaque
> SkRefCntSet
> > collection) which has no notion of what its members are -- so adding a
> > full-blown ID converter is not feasible.
> > 
> > But I think a simple set iterator would also solve the problem nicely. I'll
> look
> > into adding one (unless it was omitted on purpose to not constrain the
> > implementation) and see how it plays out.
> 
> SGTM.  I think it's fair to say that SkRefCntSet has few users outside of pipe
> and the old picture backend.  I think you can even specialize it into
> SkTypefaceSet without breaking anyone.

I have a more ambitious refactor CL
(https://codereview.chromium.org/574773003/), but I think it's too intrusive at
this point -- maybe we can revisit later. Added a plain SkRefCntSet iterator
instead, PTAL.

[mtklein, 2014-09-16 23:41:12]

lgtm

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

https://codereview.chromium.org/563783003/diff/80001/src/pipe/SkGPipeWrite.cp...
src/pipe/SkGPipeWrite.cpp:947: TypefaceBuffer& buffer) {
pass by TypefaceBuffer* ?

[f(malita), 2014-09-17 00:49:36]

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

https://codereview.chromium.org/563783003/diff/80001/src/pipe/SkGPipeWrite.cp...
src/pipe/SkGPipeWrite.cpp:947: TypefaceBuffer& buffer) {
On 2014/09/16 23:41:12, mtklein wrote:
> pass by TypefaceBuffer* ?

Done.

[f(malita), 2014-09-17 00:49:46]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-09-17 00:50:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/563783003/100001

[commit-bot: I haz the power, 2014-09-17 00:58:38]

Committed patchset #6 (id:100001) as acb882c239c0cfea738fe63ed9de48ddc2ddc76a