Adding 2 functions to the Skia API

Adding 2 functions to the Skia API

I need wrappers for both SkOrderedReadBuffer and SkOrderedWriteBuffer inside Chromium in order to do the IPC serialization of Skia SkImageFilter objects.

See https://codereview.chromium.org/21271002/

BUG=

Committed: http://code.google.com/p/skia/source/detail?r=10642

[djsollen, 2013-08-07 15:07:47]

We shouldn't have SK_API headers in our src directory.  If this is going to be
exposed it needs to be promoted to a public include, but Mike needs to weigh in
on that.

[sugoi1, 2013-08-07 15:09:37]

On 2013/08/07 15:07:47, djsollen wrote:
> We shouldn't have SK_API headers in our src directory.  If this is going to be
> exposed it needs to be promoted to a public include, but Mike needs to weigh
in
> on that.

Ok, I can move the files if Mike thinks it's acceptable to do so.

[reed1, 2013-08-07 15:29:38]

Do you need access to all of the particulars of orderedwritebuffer, or can you
write a (mostly) opaque wrapper/factory that does this, and just expose that?

My goal is to enable your use-case, w/o exposing the API details of that
internal class, which might make it much harder to evolve it internally.

[sugoi1, 2013-08-07 15:46:32]

On 2013/08/07 15:29:38, reed1 wrote:
> Do you need access to all of the particulars of orderedwritebuffer, or can you
> write a (mostly) opaque wrapper/factory that does this, and just expose that?
> 
> My goal is to enable your use-case, w/o exposing the API details of that
> internal class, which might make it much harder to evolve it internally.

That should be doable. For now (apart from constructors, obviously), I use :
- SkOrderedWriteBuffer::setFlags()
- SkOrderedWriteBuffer::writeFlattenable()
- SkOrderedWriteBuffer::writeToMemory()
- SkOrderedReadBuffer::readFlattenableT<SkImageFilter>()

If I made 2 wrapper classes, I could combine the first 2 into a single call (or
leave them as 2 separate functions), but I'd still need to wrap the last 2 as 2
separate individual functions in the wrapper classes. So, it could be something
like :

class SK_API SkOrderedWriteBufferWrapper {
  // some constructors
  ...
  void writeFlattenableWithFlags(...);
  OR
  void writeFlattenable(...);
  void setFlags(...);
  ...
  void writeToMemory(...);
private:
  SkOrderedWriteBuffer fWriter;
}

class SK_API SkOrderedReadBufferWrapper {
  // some constructors
  ...
  template <typename T> T* readFlattenableT();
private:
  SkOrderedReadBuffer fReader;
}

Is there a naming convention for wrapper classes ?

[reed1, 2013-08-07 15:47:52]

Can you show some of the call-sites, and how they might use this?

[sugoi1, 2013-08-07 15:49:22]

On 2013/08/07 15:47:52, reed1 wrote:
> Can you show some of the call-sites, and how they might use this?

You can look at the patch mentioned in the description :
https://codereview.chromium.org/21271002/

[sugoi1, 2013-08-07 15:50:02]

On 2013/08/07 15:49:22, sugoi1 wrote:
> On 2013/08/07 15:47:52, reed1 wrote:
> > Can you show some of the call-sites, and how they might use this?
> 
> You can look at the patch mentioned in the description :
> https://codereview.chromium.org/21271002/

In cc_messages.cc : https://codereview.chromium.org/21271002/patch/28001/29003

[reed1, 2013-08-07 17:07:23]

Thanks for the links.

I wonder if we can make it even simpler, given the call pattern in
https://codereview.chromium.org/21271002/patch/28001/29003

Here's one idea:

SkData* SkSerializeFlattenable(SkFlattenable*);
SkFlattenable* SkDeserializeFlattenable(const void* buffer, size_t len);

You could use the SkData to copy into the Message. Since you always (I think)
want cross-process semantics, we can just back that into the name (serialize).

[sugoi1, 2013-08-07 17:24:06]

On 2013/08/07 17:07:23, reed1 wrote:
> Thanks for the links.
> 
> I wonder if we can make it even simpler, given the call pattern in
> https://codereview.chromium.org/21271002/patch/28001/29003
> 
> Here's one idea:
> 
> SkData* SkSerializeFlattenable(SkFlattenable*);
> SkFlattenable* SkDeserializeFlattenable(const void* buffer, size_t len);
> 
> You could use the SkData to copy into the Message. Since you always (I think)
> want cross-process semantics, we can just back that into the name (serialize).

I'm not sure I fully understand the SkData part. Right now, the
SkOrderedWriteBuffer object flattens directly to the Message's pre-allocated
buffer (by using BeginWriteData). If I use SkData, won't that force me to do an
extra copy of that data into some other memory that I'd put into an SkData
object ?

[reed1, 2013-08-07 17:41:03]

On 2013/08/07 17:24:06, sugoi1 wrote:
> On 2013/08/07 17:07:23, reed1 wrote:
> > Thanks for the links.
> > 
> > I wonder if we can make it even simpler, given the call pattern in
> > https://codereview.chromium.org/21271002/patch/28001/29003
> > 
> > Here's one idea:
> > 
> > SkData* SkSerializeFlattenable(SkFlattenable*);
> > SkFlattenable* SkDeserializeFlattenable(const void* buffer, size_t len);
> > 
> > You could use the SkData to copy into the Message. Since you always (I
think)
> > want cross-process semantics, we can just back that into the name
(serialize).
> 
> I'm not sure I fully understand the SkData part. Right now, the
> SkOrderedWriteBuffer object flattens directly to the Message's pre-allocated
> buffer (by using BeginWriteData). If I use SkData, won't that force me to do
an
> extra copy of that data into some other memory that I'd put into an SkData
> object ?

The orderwritebuffer is 2-pass, so it is already buffering the write internally,
and then you copy that out into your Message. SkData is just skia's way to
return blobs of read-only data (w/ sharing to avoid extra copies).

[sugoi1, 2013-08-07 17:59:20]

On 2013/08/07 17:41:03, reed1 wrote:
> The orderwritebuffer is 2-pass, so it is already buffering the write
internally,
> and then you copy that out into your Message. SkData is just skia's way to
> return blobs of read-only data (w/ sharing to avoid extra copies).

Right, by we'd get 2 copies instead of 1. So to avoid the 2nd copy, I need the
SkData object to point to SkOrderedWriteBuffer's internal data. I don't think
SkOrderedWriteBuffer's internal data is guaranteed to be contiguous memory. It
looks like it's represented by blocks of memory (looking at
SkWriter32::flatten()). To get this to work, I'd need to allocate memory of the
total size of the flattened objects, use that memory pointer in SkData, copy
that data into Message, then release the allocated memory.

Am I missing a shortcut here that could allow me to prevent the 2nd copy ?

[reed1, 2013-08-07 18:04:08]

On 2013/08/07 17:59:20, sugoi1 wrote:
> On 2013/08/07 17:41:03, reed1 wrote:
> > The orderwritebuffer is 2-pass, so it is already buffering the write
> internally,
> > and then you copy that out into your Message. SkData is just skia's way to
> > return blobs of read-only data (w/ sharing to avoid extra copies).
> 
> Right, by we'd get 2 copies instead of 1. So to avoid the 2nd copy, I need the
> SkData object to point to SkOrderedWriteBuffer's internal data. I don't think
> SkOrderedWriteBuffer's internal data is guaranteed to be contiguous memory. It
> looks like it's represented by blocks of memory (looking at
> SkWriter32::flatten()). To get this to work, I'd need to allocate memory of
the
> total size of the flattened objects, use that memory pointer in SkData, copy
> that data into Message, then release the allocated memory.
> 
> Am I missing a shortcut here that could allow me to prevent the 2nd copy ?

True, given the current internal impl of SkOrderWriteBuffer, it will have to
make a copy to return the data. That could change in the future, but today it is
as you say.

Can we live with that? This proposal completely isolates all internal decisions
and APIs about flattening from the client, which is a big plus. If this extra
copy actually shows up as a problem, I'd be the first to revisit this.

[sugoi1, 2013-08-07 18:09:01]

On 2013/08/07 18:04:08, reed1 wrote:
> On 2013/08/07 17:59:20, sugoi1 wrote:
> > On 2013/08/07 17:41:03, reed1 wrote:
> > > The orderwritebuffer is 2-pass, so it is already buffering the write
> > internally,
> > > and then you copy that out into your Message. SkData is just skia's way to
> > > return blobs of read-only data (w/ sharing to avoid extra copies).
> > 
> > Right, by we'd get 2 copies instead of 1. So to avoid the 2nd copy, I need
the
> > SkData object to point to SkOrderedWriteBuffer's internal data. I don't
think
> > SkOrderedWriteBuffer's internal data is guaranteed to be contiguous memory.
It
> > looks like it's represented by blocks of memory (looking at
> > SkWriter32::flatten()). To get this to work, I'd need to allocate memory of
> the
> > total size of the flattened objects, use that memory pointer in SkData, copy
> > that data into Message, then release the allocated memory.
> > 
> > Am I missing a shortcut here that could allow me to prevent the 2nd copy ?
> 
> True, given the current internal impl of SkOrderWriteBuffer, it will have to
> make a copy to return the data. That could change in the future, but today it
is
> as you say.
> 
> Can we live with that? This proposal completely isolates all internal
decisions
> and APIs about flattening from the client, which is a big plus. If this extra
> copy actually shows up as a problem, I'd be the first to revisit this.

I can give it a try. Flattened SkImageFilter objects are probably fairly small,
and the extra copy may not end up taking a significant amount of time in the
overall IPC communication. I'll start working on it.

[sugoi1, 2013-08-07 19:21:31]

Here's the new API code. It causes an extra copy of the data, but completely
hides SkOrderedReadBuffer/SkOrderedWriteBuffer from the API.

[reed1, 2013-08-07 19:32:56]

https://codereview.chromium.org/22591002/diff/17001/include/core/SkFlattenabl...
File include/core/SkFlattenableSerialization.h (right):

https://codereview.chromium.org/22591002/diff/17001/include/core/SkFlattenabl...
include/core/SkFlattenableSerialization.h:17: SK_API SkData*
SkSerializeFlattenable(SkFlattenable*);
Lets keep these in parallel. Why not change the first to be
SkSerializeImageFilter()?

[scroggo, 2013-08-07 19:34:33]

https://codereview.chromium.org/22591002/diff/17001/include/core/SkFlattenabl...
File include/core/SkFlattenableSerialization.h (right):

https://codereview.chromium.org/22591002/diff/17001/include/core/SkFlattenabl...
include/core/SkFlattenableSerialization.h:17: SK_API SkData*
SkSerializeFlattenable(SkFlattenable*);
Please add some comments.

https://codereview.chromium.org/22591002/diff/17001/include/core/SkFlattenabl...
include/core/SkFlattenableSerialization.h:18: SK_API SkImageFilter*
SkDeserializeImageFilter(const void* data, size_t size)
Will/might there be a need to serialize other objects? Would it make sense to
make this generic like readFlattenableT?

Related: it seems odd to me that you can flatten any SkFlattenable object, but
you can only deserialize SkImageFilters.

[sugoi1, 2013-08-07 19:35:57]

On 2013/08/07 19:32:56, reed1 wrote:
>
https://codereview.chromium.org/22591002/diff/17001/include/core/SkFlattenabl...
> File include/core/SkFlattenableSerialization.h (right):
> 
>
https://codereview.chromium.org/22591002/diff/17001/include/core/SkFlattenabl...
> include/core/SkFlattenableSerialization.h:17: SK_API SkData*
> SkSerializeFlattenable(SkFlattenable*);
> Lets keep these in parallel. Why not change the first to be
> SkSerializeImageFilter()?

I kept different names in case anyone adds another case, so that they would only
have to implement the deserialization, since the serialization code will be the
same for all SkFlattenable objects. But I can change the name/signature, no
problem.

[sugoi1, 2013-08-07 19:41:04]

On 2013/08/07 19:34:33, scroggo wrote:
>
https://codereview.chromium.org/22591002/diff/17001/include/core/SkFlattenabl...
> File include/core/SkFlattenableSerialization.h (right):
> 
>
https://codereview.chromium.org/22591002/diff/17001/include/core/SkFlattenabl...
> include/core/SkFlattenableSerialization.h:17: SK_API SkData*
> SkSerializeFlattenable(SkFlattenable*);
> Please add some comments.
> 
>
https://codereview.chromium.org/22591002/diff/17001/include/core/SkFlattenabl...
> include/core/SkFlattenableSerialization.h:18: SK_API SkImageFilter*
> SkDeserializeImageFilter(const void* data, size_t size)
> Will/might there be a need to serialize other objects? Would it make sense to
> make this generic like readFlattenableT?
> 
> Related: it seems odd to me that you can flatten any SkFlattenable object, but
> you can only deserialize SkImageFilters.

To deserialize, you need a specific function (or template function with
instantiations for each possible case) because you need to know what kind of
data you are deserializing. I'll change the API so that both sides are strictly
for SkImageFilter.

[sugoi1, 2013-08-07 19:56:24]

I changed the API so that it's SkImageFilter only, but added utility functions
to make it easy to add more SkFlattenable derived classes to the API later on.

[reed1, 2013-08-07 20:27:00]

The templated version of readFlattenable is just a convenience wrapper around

SkFlattenable* readFlattenable()

The reader does *not* know what the subclass is, and it just always works.

[reed1, 2013-08-07 20:28:53]

I think either of these are safe and acceptable:

SerializeImageFilter
DeserializeImageFilter

or

SerializeFlattenable
DeserializeFlattenable

If you want to add a tempate helper that performs the cast in your new header,
that is fine, but is only syntactic sugar (like the sugar we have in
readFlattenableT<>)

[sugoi1, 2013-08-08 11:28:08]

On 2013/08/07 20:27:00, reed1 wrote:
> The templated version of readFlattenable is just a convenience wrapper around
> 
> SkFlattenable* readFlattenable()
> 
> The reader does *not* know what the subclass is, and it just always works.

Oh, right, I don't how I missed that :)

[sugoi1, 2013-08-08 11:28:57]

On 2013/08/07 20:28:53, reed1 wrote:
> I think either of these are safe and acceptable:
> 
> SerializeImageFilter
> DeserializeImageFilter
> 
> or
> 
> SerializeFlattenable
> DeserializeFlattenable
> 
> If you want to add a tempate helper that performs the cast in your new header,
> that is fine, but is only syntactic sugar (like the sugar we have in
> readFlattenableT<>)

I'll do the generic Flattenable version. Seems more future proof.

[sugoi1, 2013-08-08 11:30:51]

Here's the new version using SkFlattenable.

[scroggo, 2013-08-08 18:13:23]

On 2013/08/08 11:30:51, sugoi1 wrote:
> Here's the new version using SkFlattenable.

lgtm

[commit-bot: I haz the power, 2013-08-08 18:22:16]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/sugoi@chromium.org/22591002/33001

[commit-bot: I haz the power, 2013-08-08 19:20:08]

Change committed as 10642