Make skia.mojom.Bitmap use shared buffer

skia/public/interfaces/bitmap_skbitmap_struct_traits.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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:
       return kUnknown_SkColorType;
     case skia::mojom::ColorType::ALPHA_8:
       return kAlpha_8_SkColorType;
     case skia::mojom::ColorType::RGB_565:
       return kRGB_565_SkColorType;
     case skia::mojom::ColorType::ARGB_4444:
       return kARGB_4444_SkColorType;
@@ skipping 128 matching lines @@
   return b.height() < 0 ? 0 : static_cast<uint32_t>(b.height());
 }
 
 // static
 uint64_t StructTraits<skia::mojom::BitmapDataView, SkBitmap>::row_bytes(
     const SkBitmap& b) {
   return b.rowBytes();
 }
 
 // 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
 bool StructTraits<skia::mojom::BitmapDataView, SkBitmap>::Read(
     skia::mojom::BitmapDataView data,
     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())) {
+
+  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) {

[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;
   }
 
-  // If the image is empty, return success after setting the image info.
-  if (data.width() == 0 || data.height() == 0)
-    return true;
-
-  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