lacros: Stabilize SkBitmap mojom traits

We need a version-skew stable mojo transport for UI images. The skia
bitmap serializers have been stable for some time, so tidy them up
and mark them as [Stable].

* Transport color spaces explicitly as a 7-float transfer function
  and 9-float transform matrix to XYZ D50. SkColorSpace::serialize()
  depends on the layout of internal Skia structs that might change.
* Rename ColorType INDEX_8 as DEPRECATED, since it doesn't exist any
  more in skia.
* Don't try to deserialize images with unreasonably large height/width
  (addresses a TODO)
* CHECK-fail before sending a bitmap with negative height/width, rather
  than sending an empty one
* Mark skia mojoms as [Stable]
* Mark mojo_base.mojom.BigBuffer as [Stable], since it's a dependency

See go/lacros-image-transport for background.

Bug: 1116652
Test: skia_unittests
Change-Id: I229cc405901a607ae613c410d7df5b92f03caf54
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2368484Reviewed-by: Mike Klein <mtklein@google.com>
Reviewed-by: Greg Kerr <kerrnel@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803234}

mojo/public/mojom/base/big_buffer.mojom

@@ -4,6 +4,7 @@
 
 module mojo_base.mojom;
 
+[Stable]
 struct BigBufferSharedMemoryRegion {
   handle<shared_buffer> buffer_handle;
   uint32 size;
@@ -15,6 +16,7 @@ struct BigBufferSharedMemoryRegion {
 // by the sender and the contents of the buffer would be too large to inline,
 // this will instead take on the value of |invalid_buffer=true| indicating that
 // the buffer does not represent any contents intended by the sender.
+[Stable]
 union BigBuffer {
   array<uint8> bytes;
   BigBufferSharedMemoryRegion shared_memory;

skia/public/mojom/BUILD.gn

@@ -23,6 +23,7 @@ component("shared_typemap_traits") {
     "//base",
     "//mojo/public/cpp/base:shared_typemap_traits",
     "//skia",
+    "//skia:skcms",
     "//skia/public/mojom:mojom_shared",
   ]
 }

skia/public/mojom/bitmap.mojom

@@ -8,6 +8,7 @@ module skia.mojom;
 import "mojo/public/mojom/base/big_buffer.mojom";
 import "skia/public/mojom/image_info.mojom";
 
+[Stable]
 struct Bitmap {
   ImageInfo image_info;
   uint64 row_bytes;

skia/public/mojom/bitmap_skbitmap_mojom_traits.cc

@@ -5,6 +5,14 @@
 #include "skia/public/mojom/bitmap_skbitmap_mojom_traits.h"
 
 namespace mojo {
+namespace {
+
+// Maximum reasonable width and height. We don't try to deserialize bitmaps
+// bigger than these dimensions. Arbitrarily chosen.
+constexpr int kMaxWidth = 32 * 1024;
+constexpr int kMaxHeight = 32 * 1024;
+
+}  // namespace
 
 // static
 bool StructTraits<skia::mojom::BitmapDataView, SkBitmap>::IsNull(
@@ -41,11 +49,14 @@ mojo_base::BigBufferView StructTraits<skia::mojom::BitmapDataView,
 bool StructTraits<skia::mojom::BitmapDataView, SkBitmap>::Read(
     skia::mojom::BitmapDataView data,
     SkBitmap* b) {
-  // TODO: Ensure width and height are reasonable, eg. <= kMaxBitmapSize?
   SkImageInfo image_info;
   if (!data.ReadImageInfo(&image_info))
     return false;
 
+  // Ensure width and height are reasonable.
+  if (image_info.width() > kMaxWidth || image_info.height() > kMaxHeight)
+    return false;
+
   *b = SkBitmap();
   if (!b->tryAllocPixels(image_info, data.row_bytes())) {
     return false;
@@ -108,11 +119,14 @@ StructTraits<skia::mojom::InlineBitmapDataView, SkBitmap>::pixel_data(
 bool StructTraits<skia::mojom::InlineBitmapDataView, SkBitmap>::Read(
     skia::mojom::InlineBitmapDataView data,
     SkBitmap* b) {
-  // TODO: Ensure width and height are reasonable, eg. <= kMaxBitmapSize?
   SkImageInfo image_info;
   if (!data.ReadImageInfo(&image_info))
     return false;
 
+  // Ensure width and height are reasonable.
+  if (image_info.width() > kMaxWidth || image_info.height() > kMaxHeight)
+    return false;
+
   *b = SkBitmap();
   if (!b->tryAllocPixels(image_info, data.row_bytes()))
     return false;

skia/public/mojom/image_info.mojom

@@ -4,7 +4,9 @@
 
 module skia.mojom;
 
-// Mirror of SkColorType.
+// Mirror of SkColorType. Intentionally not marked [Extensible] as we want
+// unsupported image formats to fail to deserialize.
+[Stable]
 enum ColorType {
   UNKNOWN,
   ALPHA_8,
@@ -12,11 +14,13 @@ enum ColorType {
   ARGB_4444,
   RGBA_8888,
   BGRA_8888,
-  INDEX_8,
+  DEPRECATED_INDEX_8,
   GRAY_8,
 };
 
-// Mirror of SkAlphaType.
+// Mirror of SkAlphaType. Intentionally not marked [Extensible] as we want
+// unsupported image formats to fail to deserialize.
+[Stable]
 enum AlphaType {
   UNKNOWN,
   ALPHA_TYPE_OPAQUE,
@@ -24,10 +28,25 @@ enum AlphaType {
   UNPREMUL,
 };
 
+// Mirror of SkImageInfo.
+[Stable]
 struct ImageInfo {
   ColorType color_type;
   AlphaType alpha_type;
-  array<uint8> serialized_color_space;  // Empty means "null" SkColorSpace.
   uint32 width;
   uint32 height;
+
+  // Color transfer function mapping encoded values to linear values,
+  // represented by this 7-parameter piecewise function:
+  //   linear = sign(encoded) *  (c*|encoded| + f)       , 0 <= |encoded| < d
+  //          = sign(encoded) * ((a*|encoded| + b)^g + e), d <= |encoded|
+  // (A simple gamma transfer function sets g to gamma and a to 1.)
+  // See SkColorSpace and skcms_TransferFunction. Null if the image has no
+  // explicit color space. Parameters are serialized as: g, a, b, c, d, e, f.
+  array<float, 7>? color_transfer_function;
+
+  // Color transformation matrix to convert colors to XYZ D50, represented as
+  // a row-major 3x3 matrix. See SkColorSpace::MakeRGB(). Null if the image has
+  // no explicit color space.
+  array<float, 9>? color_to_xyz_matrix;
 };

skia/public/mojom/image_info_mojom_traits.cc

@@ -4,6 +4,11 @@
 
 #include "skia/public/mojom/image_info_mojom_traits.h"
 
+#include "base/numerics/safe_conversions.h"
+#include "base/optional.h"
+#include "mojo/public/cpp/bindings/array_data_view.h"
+#include "third_party/skia/include/third_party/skcms/skcms.h"
+
 namespace mojo {
 
 namespace {
@@ -24,7 +29,7 @@ SkColorType MojoColorTypeToSk(skia::mojom::ColorType type) {
       return kBGRA_8888_SkColorType;
     case skia::mojom::ColorType::GRAY_8:
       return kGray_8_SkColorType;
-    case skia::mojom::ColorType::INDEX_8:
+    case skia::mojom::ColorType::DEPRECATED_INDEX_8:
       // no longer supported
       break;
   }
@@ -103,55 +108,79 @@ StructTraits<skia::mojom::ImageInfoDataView, SkImageInfo>::alpha_type(
 }
 
 // static
-std::vector<uint8_t>
-StructTraits<skia::mojom::ImageInfoDataView,
-             SkImageInfo>::serialized_color_space(const SkImageInfo& info) {
-  std::vector<uint8_t> serialized_color_space;
-  if (auto* sk_color_space = info.colorSpace()) {
-    serialized_color_space.resize(sk_color_space->writeToMemory(nullptr));
-    // Assumption 1: Since a "null" SkColorSpace is represented as an empty byte
-    // array, the serialization of a non-null SkColorSpace should produce at
-    // least one byte.
-    CHECK_GT(serialized_color_space.size(), 0u);
-    // Assumption 2: Serialized data should be reasonably small, since
-    // SkImageInfo should efficiently pass through mojo message pipes. As of
-    // this writing, the max would be 80 bytes. However, that could change in
-    // the future. So, set an upper-bound of 1 KB here.
-    CHECK_LE(serialized_color_space.size(), 1024u);
-    sk_color_space->writeToMemory(serialized_color_space.data());
-  } else {
-    // Represent the "null" color space as an empty byte vector.
-  }
-  return serialized_color_space;
+uint32_t StructTraits<skia::mojom::ImageInfoDataView, SkImageInfo>::width(
+    const SkImageInfo& info) {
+  // Negative width images are invalid.
+  return base::checked_cast<uint32_t>(info.width());
 }
 
 // static
-uint32_t StructTraits<skia::mojom::ImageInfoDataView, SkImageInfo>::width(
+uint32_t StructTraits<skia::mojom::ImageInfoDataView, SkImageInfo>::height(
     const SkImageInfo& info) {
-  return info.width() < 0 ? 0 : static_cast<uint32_t>(info.width());
+  // Negative height images are invalid.
+  return base::checked_cast<uint32_t>(info.height());
 }
 
 // static
-uint32_t StructTraits<skia::mojom::ImageInfoDataView, SkImageInfo>::height(
+base::Optional<std::vector<float>>
+StructTraits<skia::mojom::ImageInfoDataView,
+             SkImageInfo>::color_transfer_function(const SkImageInfo& info) {
+  SkColorSpace* color_space = info.colorSpace();
+  if (!color_space)
+    return base::nullopt;
+  skcms_TransferFunction fn;
+  color_space->transferFn(&fn);
+  return std::vector<float>({fn.g, fn.a, fn.b, fn.c, fn.d, fn.e, fn.f});
+}
+
+// static
+base::Optional<std::vector<float>>
+StructTraits<skia::mojom::ImageInfoDataView, SkImageInfo>::color_to_xyz_matrix(
     const SkImageInfo& info) {
-  return info.height() < 0 ? 0 : static_cast<uint32_t>(info.height());
+  SkColorSpace* color_space = info.colorSpace();
+  if (!color_space)
+    return base::nullopt;
+  skcms_Matrix3x3 to_xyz_matrix;
+  CHECK(color_space->toXYZD50(&to_xyz_matrix));
+
+  // C-style arrays-of-arrays are tightly packed, so directly copy into vector.
+  static_assert(sizeof(to_xyz_matrix.vals) == sizeof(float) * 9,
+                "matrix must be 3x3 floats");
+  float* values = &to_xyz_matrix.vals[0][0];
+  return std::vector<float>(values, values + 9);
 }
 
 // static
 bool StructTraits<skia::mojom::ImageInfoDataView, SkImageInfo>::Read(
     skia::mojom::ImageInfoDataView data,
     SkImageInfo* info) {
-  mojo::ArrayDataView<uint8_t> serialized_color_space;
-  data.GetSerializedColorSpaceDataView(&serialized_color_space);
+  mojo::ArrayDataView<float> color_transfer_function;
+  data.GetColorTransferFunctionDataView(&color_transfer_function);
+  mojo::ArrayDataView<float> color_to_xyz_matrix;
+  data.GetColorToXyzMatrixDataView(&color_to_xyz_matrix);
+
+  // Sender must supply both color space fields or neither. This approach is
+  // simpler than having an optional ColorSpace mojo struct, due to BUILD.gn
+  // complexity with blink variants.
+  CHECK_EQ(color_transfer_function.is_null(), color_to_xyz_matrix.is_null());
+
   sk_sp<SkColorSpace> sk_color_space;
-  if (serialized_color_space.size() != 0u) {
-    sk_color_space = SkColorSpace::Deserialize(serialized_color_space.data(),
-                                               serialized_color_space.size());
-    // Deserialize() returns nullptr on invalid input.
-    if (!sk_color_space)
-      return false;
-  } else {
-    // Empty byte array is interpreted as "null."
+  if (!color_transfer_function.is_null() && !color_to_xyz_matrix.is_null()) {
+    const float* data = color_transfer_function.data();
+    skcms_TransferFunction transfer_function;
+    CHECK_EQ(7u, color_transfer_function.size());
+    transfer_function.g = data[0];
+    transfer_function.a = data[1];
+    transfer_function.b = data[2];
+    transfer_function.c = data[3];
+    transfer_function.d = data[4];
+    transfer_function.e = data[5];
+    transfer_function.f = data[6];
+
+    skcms_Matrix3x3 to_xyz_matrix;
+    CHECK_EQ(9u, color_to_xyz_matrix.size());
+    memcpy(to_xyz_matrix.vals, color_to_xyz_matrix.data(), 9 * sizeof(float));
+    sk_color_space = SkColorSpace::MakeRGB(transfer_function, to_xyz_matrix);
   }
 
   *info = SkImageInfo::Make(

skia/public/mojom/image_info_mojom_traits.h

@@ -8,6 +8,7 @@
 #include <vector>
 
 #include "base/component_export.h"
+#include "base/optional.h"
 #include "skia/public/mojom/image_info.mojom-shared.h"
 #include "third_party/skia/include/core/SkImageInfo.h"
 
@@ -18,9 +19,13 @@ struct COMPONENT_EXPORT(SKIA_SHARED_TRAITS)
     StructTraits<skia::mojom::ImageInfoDataView, SkImageInfo> {
   static skia::mojom::ColorType color_type(const SkImageInfo& info);
   static skia::mojom::AlphaType alpha_type(const SkImageInfo& info);
-  static std::vector<uint8_t> serialized_color_space(const SkImageInfo& info);
   static uint32_t width(const SkImageInfo& info);
   static uint32_t height(const SkImageInfo& info);
+  static base::Optional<std::vector<float>> color_transfer_function(
+      const SkImageInfo& info);
+  static base::Optional<std::vector<float>> color_to_xyz_matrix(
+      const SkImageInfo& info);
+
   static bool Read(skia::mojom::ImageInfoDataView data, SkImageInfo* info);
 };
 

skia/public/mojom/skcolor.mojom

@@ -5,6 +5,7 @@
 module skia.mojom;
 
 // Mirror of SkColor
+[Stable]
 struct SkColor {
     uint32 value;
 };

skia/public/mojom/test/mojom_traits_unittest.cc

@@ -8,11 +8,13 @@
 #include "skia/public/mojom/test/traits_test_service.mojom.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkColorFilter.h"
+#include "third_party/skia/include/core/SkColorSpace.h"
 #include "third_party/skia/include/core/SkImageInfo.h"
 #include "third_party/skia/include/core/SkString.h"
 #include "third_party/skia/include/effects/SkBlurImageFilter.h"
 #include "third_party/skia/include/effects/SkColorFilterImageFilter.h"
 #include "third_party/skia/include/effects/SkDropShadowImageFilter.h"
+#include "third_party/skia/include/third_party/skcms/skcms.h"
 #include "ui/gfx/skia_util.h"
 
 namespace skia {
@@ -73,6 +75,21 @@ TEST_F(StructTraitsTest, ImageInfo) {
   EXPECT_EQ(another_input_with_null_color_space, output);
 }
 
+TEST_F(StructTraitsTest, ImageInfoCustomColorSpace) {
+  skcms_TransferFunction transfer{0.1f, 0.2f, 0.3f, 0.4f, 0.5f, 0.6f, 0.7f};
+  skcms_Matrix3x3 gamut{
+      .vals = {{0.1f, 0.2f, 0.3f}, {0.4f, 0.5f, 0.6f}, {0.7f, 0.8f, 0.9f}}};
+  sk_sp<SkColorSpace> color_space = SkColorSpace::MakeRGB(transfer, gamut);
+  SkImageInfo input =
+      SkImageInfo::Make(12, 34, SkColorType::kRGBA_8888_SkColorType,
+                        kUnpremul_SkAlphaType, color_space);
+  mojo::Remote<mojom::TraitsTestService> remote = GetTraitsTestRemote();
+  SkImageInfo output;
+  remote->EchoImageInfo(input, &output);
+  EXPECT_TRUE(output.colorSpace());
+  EXPECT_EQ(input, output);
+}
+
 TEST_F(StructTraitsTest, Bitmap) {
   SkBitmap input;
   input.allocPixels(SkImageInfo::MakeN32Premul(
@@ -89,6 +106,30 @@ TEST_F(StructTraitsTest, Bitmap) {
   EXPECT_TRUE(gfx::BitmapsAreEqual(input, output));
 }
 
+TEST_F(StructTraitsTest, BitmapTooWideToSerialize) {
+  SkBitmap input;
+  constexpr int kTooWide = 32 * 1024 + 1;
+  input.allocPixels(
+      SkImageInfo::MakeN32(kTooWide, 1, SkAlphaType::kUnpremul_SkAlphaType));
+  input.eraseColor(SK_ColorYELLOW);
+  mojo::Remote<mojom::TraitsTestService> remote = GetTraitsTestRemote();
+  SkBitmap output;
+  remote->EchoBitmap(input, &output);
+  EXPECT_TRUE(output.isNull());
+}
+
+TEST_F(StructTraitsTest, BitmapTooTallToSerialize) {
+  SkBitmap input;
+  constexpr int kTooTall = 32 * 1024 + 1;
+  input.allocPixels(
+      SkImageInfo::MakeN32(1, kTooTall, SkAlphaType::kUnpremul_SkAlphaType));
+  input.eraseColor(SK_ColorYELLOW);
+  mojo::Remote<mojom::TraitsTestService> remote = GetTraitsTestRemote();
+  SkBitmap output;
+  remote->EchoBitmap(input, &output);
+  EXPECT_TRUE(output.isNull());
+}
+
 TEST_F(StructTraitsTest, BitmapWithExtraRowBytes) {
   SkBitmap input;
   // Ensure traits work with bitmaps containing additional bytes between rows.