SkTextBlob plumbing

SkTextBlob plumbing

Add SkTextBlob serialization + drawTextBlob() overrides.

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

[reed1, 2014-08-25 18:51:40]

Can the flattening/unflattening be written to purely use the Builder on
unflattening? This is a new pattern we are trying, to keep the format itself
"public" and less fragile.

[f(malita), 2014-08-25 18:57:49]

On 2014/08/25 18:51:40, reed1 wrote:
> Can the flattening/unflattening be written to purely use the Builder on
> unflattening? This is a new pattern we are trying, to keep the format itself
> "public" and less fragile.

Yes, that was actually in one of the local iterations :)

I switched because it seemed more elegant to write out the buffers at once (at
opposed to one slice per run), but it's probably better to use the builder. Let
me switch back.

[mtklein, 2014-08-25 18:59:07]

https://codereview.chromium.org/499413002/diff/1/samplecode/SampleApp.cpp
File samplecode/SampleApp.cpp (right):

https://codereview.chromium.org/499413002/diff/1/samplecode/SampleApp.cpp#new...
samplecode/SampleApp.cpp:1293: canvas =
fRecorder.DEPRECATED_beginRecording(9999, 9999, NULL, 0);
Can we make this work with vanilla beginRecording()?  By DEPRECATED I really
mean deprecated... going to make a trial flip over to new backend in Chromium
today.

[f(malita), 2014-08-25 19:00:26]

https://codereview.chromium.org/499413002/diff/1/samplecode/SampleApp.cpp
File samplecode/SampleApp.cpp (right):

https://codereview.chromium.org/499413002/diff/1/samplecode/SampleApp.cpp#new...
samplecode/SampleApp.cpp:1293: canvas =
fRecorder.DEPRECATED_beginRecording(9999, 9999, NULL, 0);
On 2014/08/25 18:59:07, mtklein wrote:
> Can we make this work with vanilla beginRecording()?  By DEPRECATED I really
> mean deprecated... going to make a trial flip over to new backend in Chromium
> today.

That was supposed to be a local/test change, leaked accidentally. Already
reverted.

[f(malita), 2014-08-25 19:30:59]

On 2014/08/25 18:57:49, Florin Malita wrote:
> On 2014/08/25 18:51:40, reed1 wrote:
> > Can the flattening/unflattening be written to purely use the Builder on
> > unflattening? This is a new pattern we are trying, to keep the format itself
> > "public" and less fragile.
> 
> Yes, that was actually in one of the local iterations :)
> 
> I switched because it seemed more elegant to write out the buffers at once (at
> opposed to one slice per run), but it's probably better to use the builder.
Let
> me switch back.

Done.

[mtklein, 2014-08-25 19:34:26]

lgtm

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

https://codereview.chromium.org/499413002/diff/20001/include/core/SkPicture.h...
include/core/SkPicture.h:245: // V34: Add SkTextBlob serialization.
Do you mind adding // V33: Serialize only public API of effects. ?

https://codereview.chromium.org/499413002/diff/20001/src/core/SkBBoxRecord.cpp
File src/core/SkBBoxRecord.cpp (right):

https://codereview.chromium.org/499413002/diff/20001/src/core/SkBBoxRecord.cp...
src/core/SkBBoxRecord.cpp:277: void SkBBoxRecord::onDrawTextBlob(const
SkTextBlob* blob, SkScalar x, SkScalar y,
FYI, this only works on old backend.

But don't worry!  I'm currently working on finishing up all the draw op bounds
for SkRecord (SkRecordDraw.cpp).  Will include you on that.

[robertphillips, 2014-08-25 21:32:06]

https://codereview.chromium.org/499413002/diff/40001/src/core/SkPictureData.cpp
File src/core/SkPictureData.cpp (right):

https://codereview.chromium.org/499413002/diff/40001/src/core/SkPictureData.c...
src/core/SkPictureData.cpp:80: 
templatize (sp) ?

https://codereview.chromium.org/499413002/diff/40001/src/core/SkPictureData.c...
src/core/SkPictureData.cpp:86: for (int i = 0; i < fTextBlobCount; ++i) {
Could do "= SkRef(blobs[i]);"

https://codereview.chromium.org/499413002/diff/40001/src/core/SkPictureData.c...
src/core/SkPictureData.cpp:534: // Delete the array
Don't we need an "fTextBlobRefs = NULL" after the SkDELETE_ARRAY ?

https://codereview.chromium.org/499413002/diff/40001/src/core/SkPictureRecord...
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/499413002/diff/40001/src/core/SkPictureRecord...
src/core/SkPictureRecord.cpp:1219: size_t initialOffset =
this->addDraw(DRAW_TEXT_BLOB, &size);
SkASSERT(initialOffset+getPaintOffset(DRAW_TEXT_BLOB, size) ==
fWriter.bytesWritten());

https://codereview.chromium.org/499413002/diff/40001/src/core/SkTextBlob.cpp
File src/core/SkTextBlob.cpp (right):

https://codereview.chromium.org/499413002/diff/40001/src/core/SkTextBlob.cpp#...
src/core/SkTextBlob.cpp:41: buffer.write32(runCount);
Shouldn't we bail here if runCount == 0 ?

https://codereview.chromium.org/499413002/diff/40001/src/core/SkTextBlob.cpp#...
src/core/SkTextBlob.cpp:49: buffer.write32(it.glyphCount());
Don't we bail here if glyphCount == 0 ?

https://codereview.chromium.org/499413002/diff/40001/src/utils/SkDumpCanvas.cpp
File src/utils/SkDumpCanvas.cpp (right):

https://codereview.chromium.org/499413002/diff/40001/src/utils/SkDumpCanvas.c...
src/utils/SkDumpCanvas.cpp:429: SkString str;
this-> ?

[f(malita), 2014-08-25 23:56:03]

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

https://codereview.chromium.org/499413002/diff/20001/include/core/SkPicture.h...
include/core/SkPicture.h:245: // V34: Add SkTextBlob serialization.
On 2014/08/25 19:34:26, mtklein wrote:
> Do you mind adding // V33: Serialize only public API of effects. ?

Done.

https://codereview.chromium.org/499413002/diff/40001/src/core/SkPictureData.cpp
File src/core/SkPictureData.cpp (right):

https://codereview.chromium.org/499413002/diff/40001/src/core/SkPictureData.c...
src/core/SkPictureData.cpp:80: 
On 2014/08/25 21:32:05, robertphillips wrote:
> templatize (sp) ?

Done.

https://codereview.chromium.org/499413002/diff/40001/src/core/SkPictureData.c...
src/core/SkPictureData.cpp:86: for (int i = 0; i < fTextBlobCount; ++i) {
On 2014/08/25 21:32:05, robertphillips wrote:
> Could do "= SkRef(blobs[i]);"

Done.

https://codereview.chromium.org/499413002/diff/40001/src/core/SkPictureData.c...
src/core/SkPictureData.cpp:534: // Delete the array
On 2014/08/25 21:32:05, robertphillips wrote:
> Don't we need an "fTextBlobRefs = NULL" after the SkDELETE_ARRAY ?

That was my thought too, but it doesn't seem needed for the picture handler
below so I'm guessing we guard everything on fTextBlobCount/fPictureCount. Can't
hurt to be explicit though -- done.

https://codereview.chromium.org/499413002/diff/40001/src/core/SkPictureRecord...
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/499413002/diff/40001/src/core/SkPictureRecord...
src/core/SkPictureRecord.cpp:1219: size_t initialOffset =
this->addDraw(DRAW_TEXT_BLOB, &size);
On 2014/08/25 21:32:05, robertphillips wrote:
> SkASSERT(initialOffset+getPaintOffset(DRAW_TEXT_BLOB, size) ==
> fWriter.bytesWritten());

Done.

https://codereview.chromium.org/499413002/diff/40001/src/core/SkTextBlob.cpp
File src/core/SkTextBlob.cpp (right):

https://codereview.chromium.org/499413002/diff/40001/src/core/SkTextBlob.cpp#...
src/core/SkTextBlob.cpp:41: buffer.write32(runCount);
On 2014/08/25 21:32:05, robertphillips wrote:
> Shouldn't we bail here if runCount == 0 ?

We could (and it would save us writing the bounds plus a paint constructor call)
but it's not really required: the run iterator below will skip gracefully in
that case.

For the sake of reduced branching, I'm inclined to leave as it -- unless you
feel otherwise. I think it also keeps the deserialization logic simpler if we
can always rely on runcount + bounds.

https://codereview.chromium.org/499413002/diff/40001/src/core/SkTextBlob.cpp#...
src/core/SkTextBlob.cpp:49: buffer.write32(it.glyphCount());
On 2014/08/25 21:32:05, robertphillips wrote:
> Don't we bail here if glyphCount == 0 ?

We shouldn't have any zero-length runs (asserting both in the builder and
above).

https://codereview.chromium.org/499413002/diff/40001/src/core/SkTextBlob.cpp#...
src/core/SkTextBlob.cpp:68: if (runCount <= 0) {
The earlier comment did point out this one though: if we allow serialization of
zero-run blobs, we should also allow their de-serialization (we need strict
inequality in this conditional). Done.

https://codereview.chromium.org/499413002/diff/40001/src/utils/SkDumpCanvas.cpp
File src/utils/SkDumpCanvas.cpp (right):

https://codereview.chromium.org/499413002/diff/40001/src/utils/SkDumpCanvas.c...
src/utils/SkDumpCanvas.cpp:429: SkString str;
On 2014/08/25 21:32:05, robertphillips wrote:
> this-> ?

can't do: toString is a standalone static function.

[robertphillips, 2014-08-26 13:13:14]

lgtm

[reed1, 2014-08-26 14:50:32]

lgtm

[f(malita), 2014-08-26 14:53:52]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-08-26 14:54:35]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/fmalita@chromium.org/499413002/60001

[commit-bot: I haz the power, 2014-08-26 14:56:50]

Committed patchset #4 (60001) as b7425173f96e93b090787e2386ba5f022b6c2869