Add SkBitmap StructTraits for skia::mojo::Bitmap.

skia/public/interfaces/bitmap_skbitmap_struct_traits.h

+// 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.
+
+#ifndef SKIA_PUBLIC_INTERFACES_BITMAP_SKBITMAP_STRUCT_TRAITS_H_
+#define SKIA_PUBLIC_INTERFACES_BITMAP_SKBITMAP_STRUCT_TRAITS_H_
+
+#include "mojo/public/cpp/bindings/array_traits.h"
+#include "third_party/skia/include/core/SkBitmap.h"

[yzshen1, 2016/05/26 22:44:13]

Please order them alphabetically.

[msw, 2016/05/27 19:34:11]

Done.

+#include "skia/public/interfaces/bitmap.mojom.h"
+
+namespace mojo {
+
+namespace {
+
+SkColorType MojoColorTypeToSk(skia::mojom::ColorType type) {

[yzshen1, 2016/05/26 22:44:13]

Can you move the impl code of this file into a .cc file?

(You can even move the StructTraits/ArrayTraits impl code because they are fully
specialized.)

[Tom Sepez, 2016/05/26 22:57:34]

So this is a header file and we risk instantiating copies of these in every file
that includes these.  I think these functions may have to be externs with the
impls in a .cpp file, or use the dreaded inline keyword.

[msw, 2016/05/27 19:34:11]

Moved impls to a cc file.

[msw, 2016/05/27 19:34:11]

I couldn't get forward decls working, but I split the definitions to a cc.

+  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;
+    case skia::mojom::ColorType::RGBA_8888:
+      return kRGBA_8888_SkColorType;
+    case skia::mojom::ColorType::BGRA_8888:
+      return kBGRA_8888_SkColorType;
+    case skia::mojom::ColorType::INDEX_8:
+      return kIndex_8_SkColorType;
+    case skia::mojom::ColorType::GRAY_8:
+      return kGray_8_SkColorType;
+    default:

[yzshen1, 2016/05/26 22:44:13]

Please consider removing "default" and also line 37.
That way if later a new value is added to skia::mojom::ColorType, the code won't
compile so that people will find out the mismatch.

(Same for the other conversion functions.)

[msw, 2016/05/27 19:34:11]

Done.

[msw, 2016/05/31 23:02:47]

I had to restore the post-switch return statements (now with NOTREACHED) to
resolve build errors for some linux and android trybots:
https://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy...
../../skia/public/interfaces/bitmap_skbitmap_struct_traits.cc: In function
'skia::mojom::ColorType mojo::{anonymous}::SkColorTypeToMojo(SkColorType)':
../../skia/public/interfaces/bitmap_skbitmap_struct_traits.cc:76:1: error:
control reaches end of non-void function [-Werror=return-type]

+      NOTREACHED();
+  }
+  return kUnknown_SkColorType;
+}
+
+SkAlphaType MojoAlphaTypeToSk(skia::mojom::AlphaType type) {
+  switch (type) {
+    case skia::mojom::AlphaType::UNKNOWN:
+      return kUnknown_SkAlphaType;
+    case skia::mojom::AlphaType::ALPHA_TYPE_OPAQUE:
+      return kOpaque_SkAlphaType;
+    case skia::mojom::AlphaType::PREMUL:
+      return kPremul_SkAlphaType;
+    case skia::mojom::AlphaType::UNPREMUL:
+      return kUnpremul_SkAlphaType;
+    default:
+      NOTREACHED();
+  }
+  return kUnknown_SkAlphaType;
+}
+
+SkColorProfileType MojoProfileTypeToSk(skia::mojom::ColorProfileType type) {
+  switch (type) {
+    case skia::mojom::ColorProfileType::LINEAR:
+      return kLinear_SkColorProfileType;
+    case skia::mojom::ColorProfileType::SRGB:
+      return kSRGB_SkColorProfileType;
+    default:
+      NOTREACHED();
+  }
+  return kLinear_SkColorProfileType;
+}
+
+skia::mojom::ColorType SkColorTypeToMojo(SkColorType type) {
+  switch (type) {
+    case kUnknown_SkColorType:
+      return skia::mojom::ColorType::UNKNOWN;
+    case kAlpha_8_SkColorType:
+      return skia::mojom::ColorType::ALPHA_8;
+    case kRGB_565_SkColorType:
+      return skia::mojom::ColorType::RGB_565;
+    case kARGB_4444_SkColorType:
+      return skia::mojom::ColorType::ARGB_4444;
+    case kRGBA_8888_SkColorType:
+      return skia::mojom::ColorType::RGBA_8888;
+    case kBGRA_8888_SkColorType:
+      return skia::mojom::ColorType::BGRA_8888;
+    case kIndex_8_SkColorType:
+      return skia::mojom::ColorType::INDEX_8;
+    case kGray_8_SkColorType:
+      return skia::mojom::ColorType::GRAY_8;
+    default:
+      NOTREACHED();
+  }
+  return skia::mojom::ColorType::UNKNOWN;
+}
+
+skia::mojom::AlphaType SkAlphaTypeToMojo(SkAlphaType type) {
+  switch (type) {
+    case kUnknown_SkAlphaType:
+      return skia::mojom::AlphaType::UNKNOWN;
+    case kOpaque_SkAlphaType:
+      return skia::mojom::AlphaType::ALPHA_TYPE_OPAQUE;
+    case kPremul_SkAlphaType:
+      return skia::mojom::AlphaType::PREMUL;
+    case kUnpremul_SkAlphaType:
+      return skia::mojom::AlphaType::UNPREMUL;
+    default:
+      NOTREACHED();
+  }
+  return skia::mojom::AlphaType::UNKNOWN;
+}
+
+skia::mojom::ColorProfileType SkProfileTypeToMojo(SkColorProfileType type) {
+  switch (type) {
+    case kLinear_SkColorProfileType:
+      return skia::mojom::ColorProfileType::LINEAR;
+    case kSRGB_SkColorProfileType:
+      return skia::mojom::ColorProfileType::SRGB;
+    default:
+      NOTREACHED();
+  }
+  return skia::mojom::ColorProfileType::LINEAR;
+}
+
+}  // namespace
+
+// A buffer used to read pixel data directly from BitmapDataView to SkBitmap.
+struct BitmapBuffer {
+  uint8_t* data = nullptr;
+  size_t size = 0;
+};
+
+// ArrayTraits needed for ReadPixelData use with BitmapBuffer.
+template <>
+struct ArrayTraits<BitmapBuffer> {
+  using Element = uint8_t;
+  static size_t GetSize(const BitmapBuffer& b) { return b.size; }
+  static uint8_t* GetData(BitmapBuffer& b) { return b.data; }
+  static const uint8_t* GetData(const BitmapBuffer& b) { return b.data; }
+  static uint8_t* GetAt(BitmapBuffer& b, size_t i) { return b.data + i; }
+  static const uint8_t* GetAt(const BitmapBuffer& b, size_t i) {
+    return b.data + i;
+  }
+  static void Resize(BitmapBuffer& b, size_t size) { CHECK_EQ(size, b.size); }
+};
+
+template <>
+struct StructTraits<skia::mojom::Bitmap, SkBitmap> {

[yzshen1, 2016/05/26 22:44:13]

SkBitmap supports null state; besides, we do have code that pass null nullable
Bitmap (please see ImageDecoder.DecodeImage()).

Therefore, you need to implement IsNull() and SetToNull(). Please see the
corresponding section in struct_traits.h for what they are used for. 

[msw, 2016/05/27 19:34:11]

Done.

+  static skia::mojom::ColorType color_type(const SkBitmap& r) {
+    return SkColorTypeToMojo(r.colorType());
+  }
+  static skia::mojom::AlphaType alpha_type(const SkBitmap& r) {
+    return SkAlphaTypeToMojo(r.alphaType());
+  }
+  static skia::mojom::ColorProfileType profile_type(const SkBitmap& r) {
+    return SkProfileTypeToMojo(r.profileType());
+  }
+  static uint32_t width(const SkBitmap& r) {
+    return r.width() < 0 ? 0 : static_cast<uint32_t>(r.width());

[yzshen1, 2016/05/26 22:44:13]

Out of curiosity: when will this be < 0?

[msw, 2016/05/27 19:34:11]

I don't know of any legitimate cases, this is just a guard for int->uint
conversion; should I add a DCHECK/CHECK, change the Bitmap mojo struct to use
signed int, or do something else?

+  }
+  static uint32_t height(const SkBitmap& r) {
+    return r.height() < 0 ? 0 : static_cast<uint32_t>(r.height());
+  }
+
+  static mojo::Array<uint8_t> pixel_data(const SkBitmap& r) {

[yzshen1, 2016/05/26 22:44:13]

Now that you have ArrayTraits<BitmapBuffer> defined, you can simply do:

// Return a BitmapBuffer value (instead of a reference) because it is cheap.
static BitmapBuffer pixel_data(const SkBitmap& r) {
}

That saves one extra copy.

[msw, 2016/05/27 19:34:11]

Done.

+    // NOTE: This code doesn't correctly serialize Index8 bitmaps.
+    const SkImageInfo& info = r.info();
+    size_t size = r.getSize();
+    size_t row_bytes = r.rowBytes();
+    mojo::Array<uint8_t> pixel_data = mojo::Array<uint8_t>::New(size);
+    if (!r.readPixels(info, &pixel_data[0], row_bytes, 0, 0))
+      return nullptr;
+    return pixel_data;
+  }
+
+  static bool Read(skia::mojom::BitmapDataView data, SkBitmap* out) {
+    // TODO: Ensure width and height are reasonable, eg. <= kMaxBitmapSize?

[yzshen1, 2016/05/26 22:44:13]

Should this be addressed in this CL?

[msw, 2016/05/27 19:34:11]

No, the converter didn't check, so it's fine to leave for followup.
(seems worth considering later, but I can remove the TODO if you want)

+    *out = SkBitmap();
+    if (!out->tryAllocPixels(SkImageInfo::Make(
+            data.width(), data.height(), MojoColorTypeToSk(data.color_type()),
+            MojoAlphaTypeToSk(data.alpha_type()),
+            MojoProfileTypeToSk(data.profile_type())))) {
+      return false;
+    }
+
+    if (static_cast<uint32_t>(out->width()) != data.width() ||
+        static_cast<uint32_t>(out->height()) != data.height() ||
+        !out->getPixels()) {
+      return false;
+    }
+
+    BitmapBuffer bitmap_buffer;
+    bitmap_buffer.data = static_cast<uint8_t*>(out->getPixels());
+    bitmap_buffer.size = out->getSize();
+    return data.ReadPixelData(&bitmap_buffer);
+  }
+};
+
+}  // namespace mojo
+
+#endif  // SKIA_PUBLIC_INTERFACES_BITMAP_SKBITMAP_STRUCT_TRAITS_H_