[mojo] Get rid of Skia type converters

[mojo] Get rid of Skia type converters

In order to ease the transition to Mojo IPC, we should reuse existing
ParamTraits whenever possible. We should also generally avoid doing
the extra TypeConverter copies that are common between mojom types
and native types.

This CL redefines the mojom skia::Bitmap as a native-only type
aliased to SkBitmap so that Mojo messages can carry pickled
SkBitmaps in their existing wire format.

The ImageDownloader service (the only one currently using skia::Bitmap)
is updated as well.

BUG=None

[Ken Rockot(use gerrit already), 2016-01-30 04:19:58]

rockot@chromium.org changed reviewers:
+ jam@chromium.org, reed@chromium.org

[Ken Rockot(use gerrit already), 2016-01-30 04:19:59]

reed@ could you please take a look at skia/? The gist of this change is that we
now support repurposing existing ParamTraits serialization for use in Mojo
messages, so we can avoid defining redundant types and doing extra type
conversion when we only care about C++ message consumers.

jam@ for content

[Ken Rockot(use gerrit already), 2016-01-30 04:24:12]

rockot@chromium.org changed reviewers:
+ tsepez@chromium.org

[Ken Rockot(use gerrit already), 2016-01-30 04:24:12]

And also +tsepez for ui/gfx/ipc

[reed1, 2016-01-30 13:23:51]

reed@google.com changed reviewers:
+ mtklein@chromium.org, reed@google.com

[reed1, 2016-01-30 13:23:52]

mtklein, could you look at the skia changes for mojo?

[Ken Rockot(use gerrit already), 2016-01-30 15:10:30]

The CQ bit was checked by rockot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-30 15:10:41]

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

[commit-bot: I haz the power, 2016-01-30 15:17:06]

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

[commit-bot: I haz the power, 2016-01-30 15:17:06]

Dry run: Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Ken Rockot(use gerrit already), 2016-01-30 15:43:15]

Patchset #6 (id:100001) has been deleted

[Ken Rockot(use gerrit already), 2016-01-30 15:47:19]

Patchset #6 (id:120001) has been deleted

[Ken Rockot(use gerrit already), 2016-02-01 17:49:07]

FYI: This patch ended up exploding in complexity a bit due to the widespread
problem of targets not properly specifying public GN dependencies. Also... GYP
support for mojo typemaps (ugh). Sorry about that. 

But most of the noise is build rule crap; the actual code changes are minimal.

[jam, 2016-02-01 17:53:48]

https://codereview.chromium.org/1648533002/diff/180001/mojo/public/tools/bind...
File mojo/public/tools/bindings/mojom.gni (right):

https://codereview.chromium.org/1648533002/diff/180001/mojo/public/tools/bind...
mojo/public/tools/bindings/mojom.gni:41: #       dependencies must be public by
design. Please use public_deps.
is there a bug on removing this? if not, please create one and reference it
here.

https://codereview.chromium.org/1648533002/diff/180001/ui/gfx/ipc/gfx_param_t...
File ui/gfx/ipc/gfx_param_traits.cc (right):

https://codereview.chromium.org/1648533002/diff/180001/ui/gfx/ipc/gfx_param_t...
ui/gfx/ipc/gfx_param_traits.cc:260: return sizeof(int32_t) * 2 +
sizeof(SkBitmap_Data) + p.getSize();
I'd expect this to match the Write method below, why the difference?

i.e. the "sizeof(SkBitmap_Data) + p.getSize();" parts correspond to the two
WriteData calls, but where is the "sizeof(int32_t)" calls coming from?

[Ken Rockot(use gerrit already), 2016-02-01 17:57:01]

WriteData also writes the size of the data first (which is a 32bit int). See
inline comment below:

https://codereview.chromium.org/1648533002/diff/180001/ui/gfx/ipc/gfx_param_t...
File ui/gfx/ipc/gfx_param_traits.cc (right):

https://codereview.chromium.org/1648533002/diff/180001/ui/gfx/ipc/gfx_param_t...
ui/gfx/ipc/gfx_param_traits.cc:259: size_t ParamTraits<SkBitmap>::GetSize(const
SkBitmap& p) {
NOTE to tsepez@: Defining a GetSize() trait is a prerequisite for using a
ParamTraits<T> as a serializer on top of Mojo, because we always preallocate
buffers for Mojo messages. I was thinking it might reduce errors to add a
base::PickleSizer type with an interface analogous to base::Pickle but which
just computes payload size, then we could pass that to GetSize instead of asking
developers to compute it 100% manually.

Alternatively we could make Pickle's interfaces virtual, have a new Pickle impl
just for computing size, and feed that directly to ParamTraits<T>::Write for a
first pass. I've been avoiding that approach because I didn't want to add vtable
lookups to such frequently called methods.

WDYT?

[Tom Sepez, 2016-02-01 18:31:58]

https://codereview.chromium.org/1648533002/diff/180001/ui/gfx/ipc/gfx_param_t...
File ui/gfx/ipc/gfx_param_traits.cc (right):

https://codereview.chromium.org/1648533002/diff/180001/ui/gfx/ipc/gfx_param_t...
ui/gfx/ipc/gfx_param_traits.cc:259: size_t ParamTraits<SkBitmap>::GetSize(const
SkBitmap& p) {
I prefer the first approach, I wouldn't be fond of adding virtual calls to
pickle.

[jam, 2016-02-01 18:39:27]

On 2016/02/01 17:57:01, Ken Rockot wrote:
> WriteData also writes the size of the data first (which is a 32bit int). See
> inline comment below:

ah, that's not obvious from reading the code. It seems like it would be easy for
someone to skip it.

This is the general problem of a param traits making assumptions about the size
of a serialized typed. Currently for ex, size_t is serialized as 64 bit always.
We might be moving towards doing that for all integers
(http://crbug.com/581409).

perhaps what we need is a set of Pickle::GetSize methods for various types, and
also GetDataSize. that way the knowledge of the implementation of pickle is only
in pickle.

> 
>
https://codereview.chromium.org/1648533002/diff/180001/ui/gfx/ipc/gfx_param_t...
> File ui/gfx/ipc/gfx_param_traits.cc (right):
> 
>
https://codereview.chromium.org/1648533002/diff/180001/ui/gfx/ipc/gfx_param_t...
> ui/gfx/ipc/gfx_param_traits.cc:259: size_t
ParamTraits<SkBitmap>::GetSize(const
> SkBitmap& p) {
> NOTE to tsepez@: Defining a GetSize() trait is a prerequisite for using a
> ParamTraits<T> as a serializer on top of Mojo, because we always preallocate
> buffers for Mojo messages. I was thinking it might reduce errors to add a
> base::PickleSizer type with an interface analogous to base::Pickle but which
> just computes payload size, then we could pass that to GetSize instead of
asking
> developers to compute it 100% manually.
> 
> Alternatively we could make Pickle's interfaces virtual, have a new Pickle
impl
> just for computing size, and feed that directly to ParamTraits<T>::Write for a
> first pass. I've been avoiding that approach because I didn't want to add
vtable
> lookups to such frequently called methods.
> 
> WDYT?

[jam, 2016-02-01 18:58:16]

just saw the other comment now. ok looks like both options work, either is fine
with me (i.e. your proposal or adding more methods to Picke)

On 2016/02/01 18:39:27, jam wrote:
> On 2016/02/01 17:57:01, Ken Rockot wrote:
> > WriteData also writes the size of the data first (which is a 32bit int). See
> > inline comment below:
> 
> ah, that's not obvious from reading the code. It seems like it would be easy
for
> someone to skip it.
> 
> This is the general problem of a param traits making assumptions about the
size
> of a serialized typed. Currently for ex, size_t is serialized as 64 bit
always.
> We might be moving towards doing that for all integers
> (http://crbug.com/581409).
> 
> perhaps what we need is a set of Pickle::GetSize methods for various types,
and
> also GetDataSize. that way the knowledge of the implementation of pickle is
only
> in pickle.
> 
> > 
> >
>
https://codereview.chromium.org/1648533002/diff/180001/ui/gfx/ipc/gfx_param_t...
> > File ui/gfx/ipc/gfx_param_traits.cc (right):
> > 
> >
>
https://codereview.chromium.org/1648533002/diff/180001/ui/gfx/ipc/gfx_param_t...
> > ui/gfx/ipc/gfx_param_traits.cc:259: size_t
> ParamTraits<SkBitmap>::GetSize(const
> > SkBitmap& p) {
> > NOTE to tsepez@: Defining a GetSize() trait is a prerequisite for using a
> > ParamTraits<T> as a serializer on top of Mojo, because we always preallocate
> > buffers for Mojo messages. I was thinking it might reduce errors to add a
> > base::PickleSizer type with an interface analogous to base::Pickle but which
> > just computes payload size, then we could pass that to GetSize instead of
> asking
> > developers to compute it 100% manually.
> > 
> > Alternatively we could make Pickle's interfaces virtual, have a new Pickle
> impl
> > just for computing size, and feed that directly to ParamTraits<T>::Write for
a
> > first pass. I've been avoiding that approach because I didn't want to add
> vtable
> > lookups to such frequently called methods.
> > 
> > WDYT?

[Ken Rockot(use gerrit already), 2016-02-01 18:58:38]

I'll work on a good general purpose solution for getting sizing right. Does it
seem reasonable to defer that momentarily so this change can be reviewed and
landed? It unblocks some other work which introduces another mojom dependency on
SkBitmap.

[jam, 2016-02-01 19:06:40]

On 2016/02/01 18:58:38, Ken Rockot wrote:
> I'll work on a good general purpose solution for getting sizing right. Does it
> seem reasonable to defer that momentarily so this change can be reviewed and
> landed? It unblocks some other work which introduces another mojom dependency
on
> SkBitmap.

I would prefer that we have the solution we want landed first, especially since
it doesn't seem like it should take more than a few days at most right?

Are there people whose work is blocked on this?

[Ken Rockot(use gerrit already), 2016-02-01 19:11:25]

On 2016/02/01 at 19:06:40, jam wrote:
> On 2016/02/01 18:58:38, Ken Rockot wrote:
> > I'll work on a good general purpose solution for getting sizing right. Does
it
> > seem reasonable to defer that momentarily so this change can be reviewed and
> > landed? It unblocks some other work which introduces another mojom
dependency on
> > SkBitmap.
> 
> I would prefer that we have the solution we want landed first, especially
since it doesn't seem like it should take more than a few days at most right?
> 
> Are there people whose work is blocked on this?

Fair enough, I'll wait. https://codereview.chromium.org/1607983002 is blocking
on that but it looks like there are other concerns with that CL as well.

I have a CL for base::PickleSizer already, including integration with IPC_STRUCT
macros. It's just not 100% ready yet and I have higher priorities than hacking
on that at the moment :)

[mtklein_C, 2016-02-01 20:21:07]

I'm confused why the transitional shim goes in this direction.  If we expect to
use Mojo more, wouldn't we want something to make types defined in a .mojom file
interoperate with legacy IPC, so that we have a sensible Mojo system after
removing old IPC?  Won't we have to re-write this .mojom file at some point to
kill off ParamTraits?

If the Skia team's looking to expand Mojo support to express more Skia concepts,
should we be writing ParamTraits and things, or is this an exception for bitmap
because it's used already by both systems?  We're interested in supporting Mojo,
but adopting this style would mean our .mojom files are only usable by Chrome.

[Ken Rockot(use gerrit already), 2016-02-01 20:32:06]

Yes, we'd have to rewrite the mojom eventually to kill off ParamTraits. The
long-term path would be:

  1. Do this.
  2. Start converting existing IPC cases to use the mojom interface with
naitve SkBitmap
  3. Convert the mojom back to a proper struct and implement
mojo::StructTraits<skia::Bitmap, SkBitmap>

My reasoning for not jumping directly to mojo::StructTraits is only that it
means maintaining two different sets of serialization traits.

I don't understand the requirement of using .mojom outside of Chrome. The
Mojo in the Chrome tree is very much specific to use within the Chrome
tree, i.e. any other consumers would still need dependencies on Chrome's
Mojo implementation. It was once an external dependency, but maintaining it
as such is no longer a goal.

Unless you just mean targets within the tree that don't (and can't/won't)
know about ParamTraits. Or if supporting Skia types in other languages (JS,
Java) is of interest. Either of those cases, particularly if they're
near-term goals, would be compelling reasons to avoid ParamTraits+Mojom for
Skia specifically IMHO.

On Mon, Feb 1, 2016 at 12:21 PM, <mtklein@chromium.org> wrote:

> I'm confused why the transitional shim goes in this direction.  If we expect
to
> use Mojo more, wouldn't we want something to make types defined in a .mojom
file
> interoperate with legacy IPC, so that we have a sensible Mojo system after
> removing old IPC?  Won't we have to re-write this .mojom file at some point
to
> kill off ParamTraits?
>
> If the Skia team's looking to expand Mojo support to express more Skia
concepts,
> should we be writing ParamTraits and things, or is this an exception for
bitmap
> because it's used already by both systems?  We're interested in supporting
Mojo,
> but adopting this style would mean our .mojom files are only usable by
Chrome.
> https://codereview.chromium.org/1648533002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mtklein, 2016-02-01 21:18:01]

On 2016/02/01 20:32:06, Ken Rockot wrote:
> Yes, we'd have to rewrite the mojom eventually to kill off ParamTraits. The
> long-term path would be:
> 
>   1. Do this.
>   2. Start converting existing IPC cases to use the mojom interface with
> naitve SkBitmap
>   3. Convert the mojom back to a proper struct and implement
> mojo::StructTraits<skia::Bitmap, SkBitmap>
Gotcha.

> My reasoning for not jumping directly to mojo::StructTraits is only that it
> means maintaining two different sets of serialization traits.
SGTM.

> I don't understand the requirement of using .mojom outside of Chrome. The
> Mojo in the Chrome tree is very much specific to use within the Chrome
> tree, i.e. any other consumers would still need dependencies on Chrome's
> Mojo implementation. It was once an external dependency, but maintaining it
> as such is no longer a goal.
> 
> Unless you just mean targets within the tree that don't (and can't/won't)
> know about ParamTraits. Or if supporting Skia types in other languages (JS,
> Java) is of interest. Either of those cases, particularly if they're
> near-term goals, would be compelling reasons to avoid ParamTraits+Mojom for
> Skia specifically IMHO.

We're thinking of reworking how we serialize and deserialize Skia data
structures generally (bitmaps, images, matrices, pictures, paints, paint
effects, image filters, etc.... there are many).  Today we've got a custom
binary format that's working but not necessarily the best fit for future use
cases.  One new design we're considering is to give Mojo a semi-preferred
status, in that we'd support Skia <-> Skia-team-provided Mojo structs, then ask
anyone who needs different serialization (e.g. protocol buffers) to work from
the Mojo structs.  This would be either hard or silly to do if Chrome's
representation of Skia objects is tied to the old IPC system.

[jam, 2016-02-01 22:14:34]

On 2016/02/01 21:18:01, mtklein wrote:
> On 2016/02/01 20:32:06, Ken Rockot wrote:
> > Yes, we'd have to rewrite the mojom eventually to kill off ParamTraits. The
> > long-term path would be:
> > 
> >   1. Do this.
> >   2. Start converting existing IPC cases to use the mojom interface with
> > naitve SkBitmap
> >   3. Convert the mojom back to a proper struct and implement
> > mojo::StructTraits<skia::Bitmap, SkBitmap>
> Gotcha.
> 
> > My reasoning for not jumping directly to mojo::StructTraits is only that it
> > means maintaining two different sets of serialization traits.
> SGTM.
> 
> > I don't understand the requirement of using .mojom outside of Chrome. The
> > Mojo in the Chrome tree is very much specific to use within the Chrome
> > tree, i.e. any other consumers would still need dependencies on Chrome's
> > Mojo implementation. It was once an external dependency, but maintaining it
> > as such is no longer a goal.
> > 
> > Unless you just mean targets within the tree that don't (and can't/won't)
> > know about ParamTraits. Or if supporting Skia types in other languages (JS,
> > Java) is of interest. Either of those cases, particularly if they're
> > near-term goals, would be compelling reasons to avoid ParamTraits+Mojom for
> > Skia specifically IMHO.
> 
> We're thinking of reworking how we serialize and deserialize Skia data
> structures generally (bitmaps, images, matrices, pictures, paints, paint
> effects, image filters, etc.... there are many).  Today we've got a custom
> binary format that's working but not necessarily the best fit for future use
> cases.  One new design we're considering is to give Mojo a semi-preferred
> status, in that we'd support Skia <-> Skia-team-provided Mojo structs, then
ask
> anyone who needs different serialization (e.g. protocol buffers) to work from
> the Mojo structs.  This would be either hard or silly to do if Chrome's
> representation of Skia objects is tied to the old IPC system.

what's the timeline for when this is needed? from your description, it sounds
like "not now", so can we punt on that until at least all of skia's types are
only sent over mojoms?

as Ken mentions, having two separate serialization is not good for a variety of
reasons. we are going to be in the chrome ipc-> mojo ipc transition for an
extended period. once that's complete, changing the traits will be an internal
change that's invisible to the developers.

[mtklein_C, 2016-02-02 14:50:09]

> what's the timeline for when this is needed? from your description, it sounds
like "not now", so can we punt on that until at least all of skia's types are
only sent over mojoms?
Don't really know yet.  We're trying to get ahead of "we needed this yesterday".
 We'd better just not worry about it for now... we'll keep hacking around with
Mojo in parallel to its actual use in Chrome, then if we get something good we
can talk about making sure there's only one best way.

> as Ken mentions, having two separate serialization is not good for a variety
of reasons. we are going to be in the chrome ipc-> mojo ipc transition for an
extended period. once that's complete, changing the traits will be an internal
change that's invisible to the developers.
SGTM

[yzshen1, 2016-02-05 17:48:25]

yzshen@chromium.org changed reviewers:
+ yzshen@chromium.org

[yzshen1, 2016-02-05 17:48:26]

https://codereview.chromium.org/1648533002/diff/180001/skia/public/interfaces...
File skia/public/interfaces/bitmap.mojom (right):

https://codereview.chromium.org/1648533002/diff/180001/skia/public/interfaces...
skia/public/interfaces/bitmap.mojom:8: [Native=True]
nit: please consider using [Native] (it equals to [Native=true] 't' in lower
case).

Currently the generator interprets True as a string and because it is not empty
it evaluates to true. That means [Native=False] achieves the same purpose.

We probably should tighten the rules. :)

[Ken Rockot(use gerrit already), 2016-02-05 17:49:02]

[Native] SGTM. thanks!

On Fri, Feb 5, 2016 at 9:48 AM, <yzshen@chromium.org> wrote:

>
https://codereview.chromium.org/1648533002/diff/180001/skia/public/interfaces...
> File skia/public/interfaces/bitmap.mojom (right):
>
https://codereview.chromium.org/1648533002/diff/180001/skia/public/interfaces...
> skia/public/interfaces/bitmap.mojom:8
<https://codereview.chromium.org/1648533002/diff/180001/skia/public/interfaces...>:
[Native=True]
> nit: please consider using [Native] (it equals to [Native=true] 't' in
> lower case).
>
> Currently the generator interprets True as a string and because it is
> not empty it evaluates to true. That means [Native=False] achieves the
> same purpose.
>
> We probably should tighten the rules. :)
> https://codereview.chromium.org/1648533002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Hadi, 2016-03-04 16:07:12]

I was wondering if there are any updates on the timeline of this CL?

I wanted to go back and work on the snapshot CL. If we're not going to have
native Skia types anytime soon, I can try to land the snapshot tools using the
current method and change it when we have native Skia.

[Hadi, 2016-03-04 16:07:41]

Description was changed from

==========
[mojo] Get rid of Skia type converters

In order to ease the transition to Mojo IPC, we should reuse existing
ParamTraits whenever possible. We should also generally avoid doing
the extra TypeConverter copies that are common between mojom types
and native types.

This CL redefines the mojom skia::Bitmap as a native-only type
aliased to SkBitmap so that Mojo messages can carry pickled
SkBitmaps in their existing wire format.

The ImageDownloader service (the only one currently using skia::Bitmap)
is updated as well.

BUG=None
==========

to

==========
[mojo] Get rid of Skia type converters

In order to ease the transition to Mojo IPC, we should reuse existing
ParamTraits whenever possible. We should also generally avoid doing
the extra TypeConverter copies that are common between mojom types
and native types.

This CL redefines the mojom skia::Bitmap as a native-only type
aliased to SkBitmap so that Mojo messages can carry pickled
SkBitmaps in their existing wire format.

The ImageDownloader service (the only one currently using skia::Bitmap)
is updated as well.

BUG=None
==========

[Ken Rockot(use gerrit already), 2016-03-04 16:10:01]

I may be able to land it some time next week. Sorry, I thought your CL was
blocking on other issues. If you'd like to move forward without native
types that SGTM.

On Fri, Mar 4, 2016 at 8:07 AM, <moshayedi@chromium.org> wrote:

> I was wondering if there are any updates on the timeline of this CL?
>
> I wanted to go back and work on the snapshot CL. If we're not going to have
> native Skia types anytime soon, I can try to land the snapshot tools using
> the
> current method and change it when we have native Skia.
>
>
> https://codereview.chromium.org/1648533002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Hadi, 2016-04-06 18:34:36]

What is the plan for this CL? Why did we close it?

[Ken Rockot(use gerrit already), 2016-04-12 14:39:47]

I closed it because I don't think we want to repurpose IPC::ParamTraits for
Skia types, which was the original plan. Work is under way to make mojo
StructTraits viable for Skia's types (among others), at which point we can
start chipping away at these type converters.

On Wed, Apr 6, 2016 at 11:34 AM, <moshayedi@chromium.org> wrote:

> What is the plan for this CL? Why did we close it?
>
> https://codereview.chromium.org/1648533002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.