Make skia.mojom.Bitmap use shared buffer

skia/public/interfaces/bitmap_skbitmap_struct_traits.cc

Index: skia/public/interfaces/bitmap_skbitmap_struct_traits.cc
diff --git a/skia/public/interfaces/bitmap_skbitmap_struct_traits.cc b/skia/public/interfaces/bitmap_skbitmap_struct_traits.cc
index 35f18be16bce02aed0f2ae89599bcf7901599e83..1bea044a8cde0eee8887ee6432ec21351d4e1d35 100644
--- a/skia/public/interfaces/bitmap_skbitmap_struct_traits.cc
+++ b/skia/public/interfaces/bitmap_skbitmap_struct_traits.cc
@@ -4,10 +4,18 @@
 
 #include "skia/public/interfaces/bitmap_skbitmap_struct_traits.h"
 
+#include <string.h>

[msw, 2017/03/25 00:59:52]

q: is this needed?

+
+#include <utility>
+
 namespace mojo {
 
 namespace {
 
+void ReleaseScopedSharedBufferMapping(void* addr, void* context) {
+  delete static_cast<mojo::ScopedSharedBufferMapping*>(context);
+}
+
 SkColorType MojoColorTypeToSk(skia::mojom::ColorType type) {
   switch (type) {
     case skia::mojom::ColorType::UNKNOWN:
@@ -156,9 +164,12 @@ uint64_t StructTraits<skia::mojom::BitmapDataView, SkBitmap>::row_bytes(
 }
 
 // static
-BitmapBuffer StructTraits<skia::mojom::BitmapDataView, SkBitmap>::pixel_data(
-    const SkBitmap& b) {
-  return {b.getSize(), b.getSize(), static_cast<uint8_t*>(b.getPixels())};
+mojo::ScopedSharedBufferHandle
+StructTraits<skia::mojom::BitmapDataView, SkBitmap>::pixel_data_buffer_handle(
+    const SkBitmap& b,
+    void* context) {
+  return (*(static_cast<mojo::ScopedSharedBufferHandle*>(context)))
+      ->Clone(mojo::SharedBufferHandle::AccessMode::READ_ONLY);
 }
 
 // static
@@ -167,51 +178,67 @@ bool StructTraits<skia::mojom::BitmapDataView, SkBitmap>::Read(
     SkBitmap* b) {
   // TODO: Ensure width and height are reasonable, eg. <= kMaxBitmapSize?
   *b = SkBitmap();
-  if (!b->tryAllocPixels(
-          SkImageInfo::Make(data.width(), data.height(),
-                            MojoColorTypeToSk(data.color_type()),
-                            MojoAlphaTypeToSk(data.alpha_type()),
-                            MojoProfileTypeToSk(data.profile_type())),
-          data.row_bytes())) {
-    return false;
-  }
+
+  const SkImageInfo info = SkImageInfo::Make(
+      data.width(), data.height(), MojoColorTypeToSk(data.color_type()),
+      MojoAlphaTypeToSk(data.alpha_type()),
+      MojoProfileTypeToSk(data.profile_type()));
 
   // If the image is empty, return success after setting the image info.
-  if (data.width() == 0 || data.height() == 0)
-    return true;
+  if (data.width() == 0 || data.height() == 0) {

[msw, 2017/03/25 00:59:52]

nit: curlies not needed

+    return b->setInfo(info, data.row_bytes());
+  }
+
+  mojo::ScopedSharedBufferHandle pixel_data_buffer_handle =
+      data.TakePixelDataBufferHandle();
+  if (!pixel_data_buffer_handle.is_valid())
+    return false;
+
+  const size_t data_byte_size = data.height() * data.row_bytes();
+  auto mapping = pixel_data_buffer_handle->Map(data_byte_size);
+  if (!mapping)
+    return false;
+
+  if (!b->installPixels(info, mapping.get(), data.row_bytes(), nullptr,

[msw, 2017/03/25 00:59:52]

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.

+                        &ReleaseScopedSharedBufferMapping,
+                        new ScopedSharedBufferMapping(std::move(mapping)))) {
+    return false;
+  }
 
-  SkAutoPixmapUnlock pixmap;
-  mojo::ArrayDataView<uint8_t> data_view;
-  data.GetPixelDataDataView(&data_view);
   if (static_cast<uint32_t>(b->width()) != data.width() ||
       static_cast<uint32_t>(b->height()) != data.height() ||
       static_cast<uint64_t>(b->rowBytes()) != data.row_bytes() ||
-      b->getSize() != data_view.size() || !b->requestLock(&pixmap) ||
-      !b->readyToDraw()) {
+      b->getSize() != data_byte_size) {
     return false;
   }
 
-  BitmapBuffer bitmap_buffer = {0, b->getSize(),
-                                static_cast<uint8_t*>(b->getPixels())};
-  if (!data.ReadPixelData(&bitmap_buffer) || bitmap_buffer.size != b->getSize())
-    return false;
-
   b->notifyPixelsChanged();
+  b->setImmutable();  // Pixels are on a read-only shared buffer.
   return true;
 }
 
 // static
 void* StructTraits<skia::mojom::BitmapDataView, SkBitmap>::SetUpContext(
     const SkBitmap& b) {
-  b.lockPixels();
-  return nullptr;
+  // Shared buffer is not a good way for huge images. Consider alternatives.
+  DCHECK_LT(b.width() * b.height(), 4000 * 4000);
+
+  // Use a context to create a shared buffer for pixels only once.
+  SkAutoLockPixels lock_pixels(b);
+  const size_t buffer_size = b.getSize();
+  auto* pixel_data_buffer_handle = new mojo::ScopedSharedBufferHandle(
+      mojo::SharedBufferHandle::Create(buffer_size));
+  mojo::ScopedSharedBufferMapping mapping =
+      (*pixel_data_buffer_handle)->Map(b.getSize());

[msw, 2017/03/25 00:59:52]

nit: use |buffer_size|

+  memcpy(mapping.get(), b.getPixels(), buffer_size);

[msw, 2017/03/25 00:59:52]

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)

+  return pixel_data_buffer_handle;
 }
 
 // static
 void StructTraits<skia::mojom::BitmapDataView, SkBitmap>::TearDownContext(
     const SkBitmap& b,
     void* context) {
-  b.unlockPixels();
+  delete static_cast<mojo::ScopedSharedBufferHandle*>(context);
 }
 
 }  // namespace mojo