Enable flattening/unflattening with custom unflatten procs

Enable flattening/unflattening with custom unflatten procs

Now flattenables are serialized using a string name, so that
flattenables do not necessarily need to be registered before
serialization.  They just need to override getTypeName().

Allows custom unflatten procs to be set on the SkReadBuffer.
This is optional if the flattenable is registered, but otherwise
must be called.

This was split off from:
https://codereview.chromium.org/1837913003/

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1858323002

Committed: https://skia.googlesource.com/skia/+/a3b3b238f507a6ec7f43febc6bf0bb17e04e770f

[msarett, 2016-04-05 17:55:36]

Description was changed from

==========
Enable flattening/unflattening with custom unflatten procs

Changes flattenable serialization to use the name string, so that
flattenables do not necessarily need to be registered before
serialization.  They just need to override getTypeName().

Allows custom unflatten procs to be set on the SkReadBuffer.
This is optional if the flattenable is registered, but otherwise
must be set.

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

to

==========
Enable flattening/unflattening with custom unflatten procs

Changes flattenable serialization to use the name string, so that
flattenables do not necessarily need to be registered before
serialization.  They just need to override getTypeName().

Allows custom unflatten procs to be set on the SkReadBuffer.
This is optional if the flattenable is registered, but otherwise
must be set.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[msarett, 2016-04-05 17:56:16]

Description was changed from

==========
Enable flattening/unflattening with custom unflatten procs

Changes flattenable serialization to use the name string, so that
flattenables do not necessarily need to be registered before
serialization.  They just need to override getTypeName().

Allows custom unflatten procs to be set on the SkReadBuffer.
This is optional if the flattenable is registered, but otherwise
must be set.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Enable flattening/unflattening with custom unflatten procs

Changes flattenable serialization to use the name string, so that
flattenables do not necessarily need to be registered before
serialization.  They just need to override getTypeName().

Allows custom unflatten procs to be set on the SkReadBuffer.
This is optional if the flattenable is registered, but otherwise
must be set.

This was split off from:
https://codereview.chromium.org/1837913003/
BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[msarett, 2016-04-05 17:56:25]

Description was changed from

==========
Enable flattening/unflattening with custom unflatten procs

Changes flattenable serialization to use the name string, so that
flattenables do not necessarily need to be registered before
serialization.  They just need to override getTypeName().

Allows custom unflatten procs to be set on the SkReadBuffer.
This is optional if the flattenable is registered, but otherwise
must be set.

This was split off from:
https://codereview.chromium.org/1837913003/
BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Enable flattening/unflattening with custom unflatten procs

Changes flattenable serialization to use the name string, so that
flattenables do not necessarily need to be registered before
serialization.  They just need to override getTypeName().

Allows custom unflatten procs to be set on the SkReadBuffer.
This is optional if the flattenable is registered, but otherwise
must be set.

This was split off from:
https://codereview.chromium.org/1837913003/

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[msarett, 2016-04-05 17:57:38]

Description was changed from

==========
Enable flattening/unflattening with custom unflatten procs

Changes flattenable serialization to use the name string, so that
flattenables do not necessarily need to be registered before
serialization.  They just need to override getTypeName().

Allows custom unflatten procs to be set on the SkReadBuffer.
This is optional if the flattenable is registered, but otherwise
must be set.

This was split off from:
https://codereview.chromium.org/1837913003/

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Enable flattening/unflattening with custom unflatten procs

Now flattenables are serialized using a string name, so that
flattenables do not necessarily need to be registered before
serialization.  They just need to override getTypeName().

Allows custom unflatten procs to be set on the SkReadBuffer.
This is optional if the flattenable is registered, but otherwise
must be called.

This was split off from:
https://codereview.chromium.org/1837913003/

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[msarett, 2016-04-05 18:00:44]

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

[msarett, 2016-04-05 19:08:52]

Patchset #2 (id:20001) has been deleted

[msarett, 2016-04-05 19:28:18]

Patchset #2 (id:40001) has been deleted

[reed1, 2016-04-06 11:37:12]

Moving getTypeName to be virtual seems a fine change.

Always forcing us to flatten strings instead of the option for function-ptr
feels like overkill, as it is slightly bulkier and slower. Why must we abandon
function-ptrs in the flatten data?

[mtklein, 2016-04-06 11:50:21]

On 2016/04/06 at 11:37:12, reed wrote:
> Moving getTypeName to be virtual seems a fine change.
> 
> Always forcing us to flatten strings instead of the option for function-ptr
feels like overkill, as it is slightly bulkier and slower. Why must we abandon
function-ptrs in the flatten data?

Am I thinking right that the only in-process use of flatten left is for creating
font descriptors?  Most (all?) other use cases of flattenable serialization are
going to leave the process and need to use something more stable than a function
pointer.

[reed1, 2016-04-06 11:53:50]

I guess that is the question : how often do we flatten but not serialize (if
serialize means leave the process)? And even in that case, is it ok to always
pay for the string-lookup, even for repeated names (e.g. gradients)?

[mtklein, 2016-04-06 12:06:13]

On 2016/04/06 at 11:53:50, reed wrote:
> I guess that is the question : how often do we flatten but not serialize (if
serialize means leave the process)? And even in that case, is it ok to always
pay for the string-lookup, even for repeated names (e.g. gradients)?

I think I'm going to get confused if we try to introduce a distinction between
"serialize" and "flatten".  flatten() always serializes, and sometimes the
format it uses is only suitable for in-process deserialization.

I'm not aware of anything using in-process serialization except SkPaint to fill
path effects, mask filters, and rasterizers into an SkDescriptor, and
SkScalerContext to reconstruct those effects (and SkGlyphCache in between
caching strikes based on the SkDescriptor).

Seems to me we could instead pass these three effects by sk_sp, checking to make
sure it doesn't affect SkGlyphCache strike lookup (too much).

[reed1, 2016-04-06 12:10:44]

I was just referring to the old distinction we had, where "serialize" implied
cross-process, and "flatten" didn't. Happy to not continue that wording
distinction.

1. If we're going to globally change how we flatten/serialize, then I'd like to
fix/change the font cache usage beforehand.

2. If we're going to use strings, have we tried deduping the strings themselves
(as it appears we did/used-to) ?

[msarett, 2016-04-11 14:55:23]

Any thoughts now that https://codereview.chromium.org/1863013003/ has landed?

I briefly discussed indexing strings to avoiding rewriting dups with Mike.  Not
sure if that belongs here or in a follow-up?

[reed1, 2016-04-11 15:07:48]

Since this CL will make our serialized size larger, I'd like to try also to
dictionaryize or otherwise try to reduce the size in this CL (partly because
this CL is already changing some of our heuristics of what to store/retrive).

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

https://codereview.chromium.org/1858323002/diff/80001/src/core/SkReadBuffer.c...
src/core/SkReadBuffer.cpp:354: // Check if a custom Factory has been specified
for this flattenable.
Is this order (checking custom first) important? Do we need to document that in
the header?

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

https://codereview.chromium.org/1858323002/diff/80001/src/core/SkReadBuffer.h...
src/core/SkReadBuffer.h:197: *  For an input flattenable (specified by name),
set a custom factory proc
Dox comments:

1. Does the name get copied, or must the caller keep the ptr alive for the life
of this readbuffer (or longer)?
2. Is there a way to remove a factory?
3. What happens if I specify the same name twice? (but possibly w/ different
factories)

nit: const char name[]

[msarett, 2016-04-19 17:56:19]

Patchset #5 (id:120001) has been deleted

[msarett, 2016-04-19 18:03:50]

PTAL.  We now build a dictionary on reads and writes to avoid writing each name
string more than once.

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

https://codereview.chromium.org/1858323002/diff/80001/src/core/SkReadBuffer.c...
src/core/SkReadBuffer.cpp:354: // Check if a custom Factory has been specified
for this flattenable.
On 2016/04/11 15:07:48, reed1 wrote:
> Is this order (checking custom first) important? Do we need to document that
in
> the header?

Yes I think we should check custom first.  I've this to the header.

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

https://codereview.chromium.org/1858323002/diff/80001/src/core/SkReadBuffer.h...
src/core/SkReadBuffer.h:197: *  For an input flattenable (specified by name),
set a custom factory proc
On 2016/04/11 15:07:48, reed1 wrote:
> Dox comments:
> 
> 1. Does the name get copied, or must the caller keep the ptr alive for the
life
> of this readbuffer (or longer)?

Done.

> 2. Is there a way to remove a factory?

I said no.  I don't think this would be hard to support, I just don't see a
reason to yet.

> 3. What happens if I specify the same name twice? (but possibly w/ different
> factories)
> 

Done.

> nit: const char name[]

Changed to const SkString&.

[mtklein, 2016-04-19 18:14:55]

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

https://codereview.chromium.org/1858323002/diff/140001/src/core/SkReadBuffer....
src/core/SkReadBuffer.cpp:362: factory = getCustomFactory(name);
this->

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

https://codereview.chromium.org/1858323002/diff/140001/src/core/SkReadBuffer....
src/core/SkReadBuffer.h:207: void setCustomFactory(const SkString& name,
SkFlattenable::Factory factory) {
Seems like calling setCustomFactory("...", nullptr) will actually effectively
unset the factory.

https://codereview.chromium.org/1858323002/diff/140001/src/core/SkValidatingR...
File src/core/SkValidatingReadBuffer.cpp (right):

https://codereview.chromium.org/1858323002/diff/140001/src/core/SkValidatingR...
src/core/SkValidatingReadBuffer.cpp:236: SkFlattenable::Factory factory =
getCustomFactory(name);
this->

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

https://codereview.chromium.org/1858323002/diff/140001/src/core/SkWriteBuffer...
src/core/SkWriteBuffer.cpp:305: if (indexPtr) {
I find it nice to scope these lookups a little tighter:

if (uint32_t* indexPtr = fFlattenableDict.find(key)) {
    this->write32(*indexPtr);
}

https://codereview.chromium.org/1858323002/diff/140001/src/core/SkWriteBuffer...
src/core/SkWriteBuffer.cpp:315: fNextIndex++;
Is fNextIndex redundant with fFlattenableDict.count()?

[msarett, 2016-04-19 18:55:52]

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

https://codereview.chromium.org/1858323002/diff/140001/src/core/SkReadBuffer....
src/core/SkReadBuffer.cpp:362: factory = getCustomFactory(name);
On 2016/04/19 18:14:55, mtklein wrote:
> this->

Done.

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

https://codereview.chromium.org/1858323002/diff/140001/src/core/SkReadBuffer....
src/core/SkReadBuffer.h:207: void setCustomFactory(const SkString& name,
SkFlattenable::Factory factory) {
On 2016/04/19 18:14:55, mtklein wrote:
> Seems like calling setCustomFactory("...", nullptr) will actually effectively
> unset the factory.

Oh yeah of course.  Will change the comment to reflect that.

https://codereview.chromium.org/1858323002/diff/140001/src/core/SkValidatingR...
File src/core/SkValidatingReadBuffer.cpp (right):

https://codereview.chromium.org/1858323002/diff/140001/src/core/SkValidatingR...
src/core/SkValidatingReadBuffer.cpp:236: SkFlattenable::Factory factory =
getCustomFactory(name);
On 2016/04/19 18:14:55, mtklein wrote:
> this->

Done.

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

https://codereview.chromium.org/1858323002/diff/140001/src/core/SkWriteBuffer...
src/core/SkWriteBuffer.cpp:305: if (indexPtr) {
On 2016/04/19 18:14:55, mtklein wrote:
> I find it nice to scope these lookups a little tighter:
> 
> if (uint32_t* indexPtr = fFlattenableDict.find(key)) {
>     this->write32(*indexPtr);
> }

Done.

https://codereview.chromium.org/1858323002/diff/140001/src/core/SkWriteBuffer...
src/core/SkWriteBuffer.cpp:315: fNextIndex++;
On 2016/04/19 18:14:55, mtklein wrote:
> Is fNextIndex redundant with fFlattenableDict.count()?

Hmm yeah... Nice!

[msarett, 2016-04-20 17:21:00]

Here's an implementation of the alternative we discussed: always send a string
or an index - never both.

I think overall it's cleaner, though it's a bit complicated to guarantee the
byte order of the index.

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

https://codereview.chromium.org/1858323002/diff/180001/src/core/SkWriteBuffer...
src/core/SkWriteBuffer.cpp:310: this->write32(*indexPtr << 8);
There are more ways to do this:
(1) I originally added a writeByte() call, but it turns out that fWriter will
write 32-bits even if I only tell it to write a byte.  It really wants things in
32-bit packages, and I didn't want to mess with that.
(2) It's also possible to force a big endian write and a big endian read.  That
would work fine as well.

[msarett, 2016-04-21 14:10:19]

Patchset #8 (id:200001) has been deleted

[msarett, 2016-04-21 14:18:04]

Patchset #8 (id:220001) has been deleted

[msarett, 2016-04-21 14:19:45]

PTAL.  I've combined with the isValidating() case in SkWriteBuffer, since the
new approach is also cross process.

Additionally, I've deleted the kValidation_Flag entirely from SkWriteBuffer.

[reed1, 2016-04-22 15:06:08]

lgtm w/ suggestions

https://codereview.chromium.org/1858323002/diff/260001/src/core/SkValidatingR...
File src/core/SkValidatingReadBuffer.cpp (right):

https://codereview.chromium.org/1858323002/diff/260001/src/core/SkValidatingR...
src/core/SkValidatingReadBuffer.cpp:230: uint8_t firstByte = this->peekByte();
Can we assert that there is no "factory" here (since in non-validating we check
that before reading the "name"?

If not, lets at least comment how/why this impl is different from the
non-validating one.

https://codereview.chromium.org/1858323002/diff/260001/src/core/SkValidatingR...
src/core/SkValidatingReadBuffer.cpp:235: SkString name;
Is it easy to share this section in a helper function (w/ non-validating
version)?

[msarett, 2016-04-22 18:54:09]

https://codereview.chromium.org/1858323002/diff/260001/src/core/SkValidatingR...
File src/core/SkValidatingReadBuffer.cpp (right):

https://codereview.chromium.org/1858323002/diff/260001/src/core/SkValidatingR...
src/core/SkValidatingReadBuffer.cpp:230: uint8_t firstByte = this->peekByte();
On 2016/04/22 15:06:08, reed1 wrote:
> Can we assert that there is no "factory" here (since in non-validating we
check
> that before reading the "name"?
> 
> If not, lets at least comment how/why this impl is different from the
> non-validating one.

I've made a protected getter for the factoryCount so that we can verify it is 0
here.

https://codereview.chromium.org/1858323002/diff/260001/src/core/SkValidatingR...
src/core/SkValidatingReadBuffer.cpp:235: SkString name;
On 2016/04/22 15:06:08, reed1 wrote:
> Is it easy to share this section in a helper function (w/ non-validating
> version)?

I refactored this, but it ultimately didn't work (failed tests).  The validating
read buffer needs to be able to check for errors after every read.

[msarett, 2016-04-22 18:54:54]

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

[commit-bot: I haz the power, 2016-04-22 18:54:58]

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

[reed1, 2016-04-22 18:56:50]

lgtm

[commit-bot: I haz the power, 2016-04-22 19:00:28]

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

[commit-bot: I haz the power, 2016-04-22 19:00:30]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[msarett, 2016-04-22 19:23:07]

This fails SkBitmapHeapTest because, under the new flattening strategy, there is
more than one entry in SkFlatDictionary when we flatten the same object more
than once.

Ex:
First entry: "name"
Second entry: index
Entries obviously don't match.

I think it's fine to delete the failed line of the test, since this test code is
the only user of SkFlatDictionary.  But this probably indicates that some
follow-up refactoring would be a good idea.

[msarett, 2016-04-22 19:33:00]

The CQ bit was checked by msarett@google.com

[msarett, 2016-04-22 19:33:01]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com
Link to the patchset: https://codereview.chromium.org/1858323002/#ps300001
(title: "Fix test")

[commit-bot: I haz the power, 2016-04-22 19:33:12]

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

[commit-bot: I haz the power, 2016-04-22 19:43:10]

Description was changed from

==========
Enable flattening/unflattening with custom unflatten procs

Now flattenables are serialized using a string name, so that
flattenables do not necessarily need to be registered before
serialization.  They just need to override getTypeName().

Allows custom unflatten procs to be set on the SkReadBuffer.
This is optional if the flattenable is registered, but otherwise
must be called.

This was split off from:
https://codereview.chromium.org/1837913003/

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Enable flattening/unflattening with custom unflatten procs

Now flattenables are serialized using a string name, so that
flattenables do not necessarily need to be registered before
serialization.  They just need to override getTypeName().

Allows custom unflatten procs to be set on the SkReadBuffer.
This is optional if the flattenable is registered, but otherwise
must be called.

This was split off from:
https://codereview.chromium.org/1837913003/

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/a3b3b238f507a6ec7f43febc6bf0bb17e04e770f
==========

[commit-bot: I haz the power, 2016-04-22 19:43:11]

Committed patchset #11 (id:300001) as
https://skia.googlesource.com/skia/+/a3b3b238f507a6ec7f43febc6bf0bb17e04e770f