|
|
Created:
7 years, 4 months ago by sugoi1 Modified:
7 years, 4 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionAdding 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
Patch Set 1 #Patch Set 2 : New serialization functions #
Total comments: 3
Patch Set 3 : ImageFilter only api with generalized local functions #Patch Set 4 : Oops, missing semi-colon added to header file #Patch Set 5 : Back to generic SkFlattenable version #
Messages
Total messages: 28 (0 generated)
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.
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.
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.
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 ?
Can you show some of the call-sites, and how they might use this?
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/
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
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).
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 ?
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).
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 ?
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.
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.
Here's the new API code. It causes an extra copy of the data, but completely hides SkOrderedReadBuffer/SkOrderedWriteBuffer from the API.
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()?
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.
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.
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.
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.
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.
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<>)
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 :)
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.
Here's the new version using SkFlattenable.
On 2013/08/08 11:30:51, sugoi1 wrote: > Here's the new version using SkFlattenable. lgtm
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/sugoi@chromium.org/22591002/33001
Message was sent while issue was closed.
Change committed as 10642 |