Make skia.mojom.Bitmap use shared buffer

Make skia.mojom.Bitmap use shared buffer

- Merge gfx.mojom.SharedBufferSkBitmap into skia.mojom.Bitmap to
  transport pixel data via shared buffer. Shared buffer is created
  on the sender side by copying source pixel data. On the receiving
  end, the shared buffer is used to installPixels on the output
  SkBitmap. Only one copy happens for the process. The cost is that
  the shared buffer mapping has to be kept around for the lifetime
  of the output SkBitmap;
- Update test expectations of gfx.mojom.ImageSkia[Rep] because
  empty SkBitmap would end up as null on the other end (as
  a result of installPixels using shared buffer);

BUG=655874

[xiyuan, 2017-03-24 23:00:24]

xiyuan@chromium.org changed reviewers:
+ msw@chromium.org, rockot@chromium.org, tsepez@chromium.org

[xiyuan, 2017-03-24 23:00:25]

msw@, please review. This tickles me and I have to do it before the weekend. :)

rockot@, to make sure we use handle correctly.

tsepez@, for mojom and struct traits.

Thanks.

[xiyuan, 2017-03-24 23:00:32]

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

[commit-bot: I haz the power, 2017-03-24 23:01:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 23:07:22]

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

[commit-bot: I haz the power, 2017-03-24 23:07:23]

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

[xiyuan, 2017-03-24 23:20:34]

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

[commit-bot: I haz the power, 2017-03-24 23:21:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 23:34:00]

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

[commit-bot: I haz the power, 2017-03-24 23:34:01]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[msw, 2017-03-25 00:59:53]

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

https://codereview.chromium.org/2772113004/diff/20001/skia/public/interfaces/...
skia/public/interfaces/bitmap.mojom:45: handle<shared_buffer>?
pixel_data_buffer_handle;
Does it make sense to have our only SkBitmap transport work via shared memory?
Don't we then need to guarantee that all users keep their SkBitmaps alive for
the unknown duration needed by the remote mojom impl?

https://codereview.chromium.org/2772113004/diff/20001/skia/public/interfaces/...
File skia/public/interfaces/bitmap_skbitmap_struct_traits.cc (right):

https://codereview.chromium.org/2772113004/diff/20001/skia/public/interfaces/...
skia/public/interfaces/bitmap_skbitmap_struct_traits.cc:7: #include <string.h>
q: is this needed?

https://codereview.chromium.org/2772113004/diff/20001/skia/public/interfaces/...
skia/public/interfaces/bitmap_skbitmap_struct_traits.cc:188: if (data.width() ==
0 || data.height() == 0) {
nit: curlies not needed

https://codereview.chromium.org/2772113004/diff/20001/skia/public/interfaces/...
skia/public/interfaces/bitmap_skbitmap_struct_traits.cc:202: if
(!b->installPixels(info, mapping.get(), data.row_bytes(), nullptr,
nit: cache a raw pointer for mapping.get() above. The order in which arguments
are evaluated in C++ is undefined, so it's possible that the std::move(mapping)
might make this mapping.get() invalid.

https://codereview.chromium.org/2772113004/diff/20001/skia/public/interfaces/...
skia/public/interfaces/bitmap_skbitmap_struct_traits.cc:232:
(*pixel_data_buffer_handle)->Map(b.getSize());
nit: use |buffer_size|

https://codereview.chromium.org/2772113004/diff/20001/skia/public/interfaces/...
skia/public/interfaces/bitmap_skbitmap_struct_traits.cc:233:
memcpy(mapping.get(), b.getPixels(), buffer_size);
Do we need this copy? Can the buffer just point to b.getPixels()'s memory?
(I guess we's need to keep the pixels locked for the SkBitmap's lifetime)

https://codereview.chromium.org/2772113004/diff/20001/ui/gfx/image/mojo/image...
File ui/gfx/image/mojo/image_traits_unittest.cc (right):

https://codereview.chromium.org/2772113004/diff/20001/ui/gfx/image/mojo/image...
ui/gfx/image/mojo/image_traits_unittest.cc:97: EXPECT_TRUE(output.is_null());
Odd that this doesn't match empty_rep.is_null()...

[Ken Rockot(use gerrit already), 2017-03-26 17:52:50]

I doubt we really want this to be the default behavior for all SkBitmaps. Please
keep in mind that on Windows and OS X, allocating a shared buffer in a sandboxed
process requires that we do an internal sync IPC requesting the allocation from
a more privileged process. We could probably improve the  situation slightly by
having a dedicated thread in the broker process for handling those requests, but
still this is almost certainly more expensive than copying the pixel data an
extra time for some reasonably small bitmaps.

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

https://codereview.chromium.org/2772113004/diff/20001/skia/public/interfaces/...
skia/public/interfaces/bitmap.mojom:45: handle<shared_buffer>?
pixel_data_buffer_handle;
On 2017/03/25 at 00:59:52, msw wrote:
> Does it make sense to have our only SkBitmap transport work via shared memory?
Don't we then need to guarantee that all users keep their SkBitmaps alive for
the unknown duration needed by the remote mojom impl?

No, the shared buffer is allocated during serialization and will remain alive as
long as there are any handles referencing it. The bitmap data is copied into the
buffer at that point, and the handle sent to the receiver is enough to keep the
buffer alive.

[xiyuan, 2017-03-27 16:43:51]

On 2017/03/26 17:52:50, Ken Rockot wrote:
> I doubt we really want this to be the default behavior for all SkBitmaps.
Please
> keep in mind that on Windows and OS X, allocating a shared buffer in a
sandboxed
> process requires that we do an internal sync IPC requesting the allocation
from
> a more privileged process. We could probably improve the  situation slightly
by
> having a dedicated thread in the broker process for handling those requests,
but
> still this is almost certainly more expensive than copying the pixel data an
> extra time for some reasonably small bitmaps.
> 

Yep, did not realize that we were using this inside sandboxed process.
Abandoned.

[msw, 2017-03-27 17:01:07]

On 2017/03/27 16:43:51, xiyuan wrote:
> On 2017/03/26 17:52:50, Ken Rockot wrote:
> > I doubt we really want this to be the default behavior for all SkBitmaps.
> Please
> > keep in mind that on Windows and OS X, allocating a shared buffer in a
> sandboxed
> > process requires that we do an internal sync IPC requesting the allocation
> from
> > a more privileged process. We could probably improve the  situation slightly
> by
> > having a dedicated thread in the broker process for handling those requests,
> but
> > still this is almost certainly more expensive than copying the pixel data an
> > extra time for some reasonably small bitmaps.
> > 
> 
> Yep, did not realize that we were using this inside sandboxed process.
> Abandoned.

Does that mean we shouldn't use the shared buffer ImageSkia mojom for certain
things? Is it okay to use it for chrome<->ash shelf item and window icons, etc.?
Should we have a separate ImageSkia struct traits that copies pixels like
SkBitmaps?

[xiyuan, 2017-03-27 17:06:13]

On 2017/03/27 17:01:07, msw wrote:
> On 2017/03/27 16:43:51, xiyuan wrote:
> > On 2017/03/26 17:52:50, Ken Rockot wrote:
> > > I doubt we really want this to be the default behavior for all SkBitmaps.
> > Please
> > > keep in mind that on Windows and OS X, allocating a shared buffer in a
> > sandboxed
> > > process requires that we do an internal sync IPC requesting the allocation
> > from
> > > a more privileged process. We could probably improve the  situation
slightly
> > by
> > > having a dedicated thread in the broker process for handling those
requests,
> > but
> > > still this is almost certainly more expensive than copying the pixel data
an
> > > extra time for some reasonably small bitmaps.
> > > 
> > 
> > Yep, did not realize that we were using this inside sandboxed process.
> > Abandoned.
> 
> Does that mean we shouldn't use the shared buffer ImageSkia mojom for certain
> things? Is it okay to use it for chrome<->ash shelf item and window icons,
etc.?
> Should we have a separate ImageSkia struct traits that copies pixels like
> SkBitmaps?

The shared buffer creation code would not run inside a sandboxed process. It is
okay for chrome -> ash since chrome has the privilege. Not so sure about ash ->
chrome in the long run. It works now but it might break when we want to add some
restriction to ash.