Incorporate BlobChannel into Blimp image encode/decode pipeline.

blimp/engine/renderer/engine_image_serialization_processor.cc

Index: blimp/engine/renderer/engine_image_serialization_processor.cc
diff --git a/blimp/engine/renderer/engine_image_serialization_processor.cc b/blimp/engine/renderer/engine_image_serialization_processor.cc
index cc792dd582f788ac95a345de96280a897a7bf324..d715bd42ea1284b4a932dc8eae3a37df5192b455 100644
--- a/blimp/engine/renderer/engine_image_serialization_processor.cc
+++ b/blimp/engine/renderer/engine_image_serialization_processor.cc
@@ -15,6 +15,7 @@
 #include "base/trace_event/trace_event.h"
 #include "blimp/common/blob_cache/id_util.h"
 #include "blimp/common/proto/blob_cache.pb.h"
+#include "blimp/engine/renderer/blob_channel_sender_proxy.h"
 #include "content/public/renderer/render_frame.h"
 #include "third_party/libwebp/webp/encode.h"
 #include "third_party/skia/include/core/SkData.h"
@@ -23,12 +24,9 @@
 #include "third_party/skia/include/core/SkUnPreMultiply.h"
 
 namespace blimp {
+namespace engine {
 namespace {
 
-// TODO(nyquist): Add support for changing this from the client.
-static base::LazyInstance<std::set<BlobId>> g_client_cache_contents =
-    LAZY_INSTANCE_INITIALIZER;
-
 SkData* BlobCacheImageMetadataProtoAsSkData(
     const BlobCacheImageMetadata& proto) {
   int signed_size = proto.ByteSize();
@@ -38,148 +36,30 @@ SkData* BlobCacheImageMetadataProtoAsSkData(
   return SkData::NewWithCopy(serialized.data(), serialized.size());
 }
 
-// TODO(nyquist): Make sure encoder does not serialize images more than once.
-// See crbug.com/548434.
-class WebPImageEncoder : public SkPixelSerializer {
- public:
-  WebPImageEncoder() {}
-  ~WebPImageEncoder() override{};
-
-  bool onUseEncodedData(const void* data, size_t len) override {
-    TRACE_EVENT1("blimp", "WebPImageEncoded::UsingEncodedData",
-                 "OriginalImageSize", len);
-    // Encode all images regardless of their format, including WebP images.
-    return false;
-  }
-
-  SkData* onEncode(const SkPixmap& pixmap) override {
-    TRACE_EVENT0("blimp", "WebImageEncoder::onEncode");
-    // Initialize an empty WebPConfig.
-    WebPConfig config;
-    if (!WebPConfigInit(&config))
-      return nullptr;
-
-    // Initialize an empty WebPPicture.
-    WebPPicture picture;
-    if (!WebPPictureInit(&picture))
-      return nullptr;
-
-    // Ensure width and height are valid dimensions.
-    if (!pixmap.width() || pixmap.width() > WEBP_MAX_DIMENSION)
-      return nullptr;
-    picture.width = pixmap.width();
-    if (!pixmap.height() || pixmap.height() > WEBP_MAX_DIMENSION)
-      return nullptr;
-    picture.height = pixmap.height();
-
-    const BlobId blob_id = CalculateBlobId(pixmap.addr(), pixmap.getSafeSize());
-    std::string blob_id_hex = BlobIdToString(blob_id);
-
-    // Create proto with all requires information.
-    BlobCacheImageMetadata proto;
-    proto.set_id(blob_id);
-    proto.set_width(pixmap.width());
-    proto.set_height(pixmap.height());
-
-    if (g_client_cache_contents.Get().find(blob_id) !=
-        g_client_cache_contents.Get().end()) {
-      // Found image in client cache, so skip sending decoded payload.
-      SkData* sk_data = BlobCacheImageMetadataProtoAsSkData(proto);
-      TRACE_EVENT1("blimp", "WebPImageEncoder::onEncode ImageFoundInCache",
-                   "EncodedImageSize", sk_data->size());
-      DVLOG(2) << "Sending cached: " << blob_id_hex
-               << " size = " << sk_data->size();
-      return sk_data;
+// For each pixel, un-premultiplies the alpha-channel for each of the RGB
+// channels. As an example, for a channel value that before multiplication was
+// 255, and after applying an alpha of 128, the premultiplied pixel would be
+// 128. The un-premultiply step uses the alpha-channel to get back to 255. The
+// alpha channel is kept unchanged.
+void UnPremultiply(const unsigned char* in_pixels,
+                   unsigned char* out_pixels,
+                   size_t pixel_count) {
+  const SkUnPreMultiply::Scale* table = SkUnPreMultiply::GetScaleTable();
+  for (; pixel_count-- > 0; in_pixels += 4) {
+    unsigned char alpha = in_pixels[3];
+    if (alpha == 255) {  // Full opacity, just blindly copy.
+      *out_pixels++ = in_pixels[0];
+      *out_pixels++ = in_pixels[1];
+      *out_pixels++ = in_pixels[2];
+      *out_pixels++ = alpha;
+    } else {
+      SkUnPreMultiply::Scale scale = table[alpha];
+      *out_pixels++ = SkUnPreMultiply::ApplyScale(scale, in_pixels[0]);
+      *out_pixels++ = SkUnPreMultiply::ApplyScale(scale, in_pixels[1]);
+      *out_pixels++ = SkUnPreMultiply::ApplyScale(scale, in_pixels[2]);
+      *out_pixels++ = alpha;
     }
-
-    DVLOG(2) << "Encoding image color_type=" << pixmap.colorType()
-             << ", alpha_type=" << pixmap.alphaType() << " " << pixmap.width()
-             << "x" << pixmap.height();
-
-    // Import picture from raw pixels.
-    auto pixel_chars = static_cast<const unsigned char*>(pixmap.addr());
-    if (!PlatformPictureImport(pixel_chars, &picture, pixmap.alphaType()))
-      return nullptr;
-
-    // Create a buffer for where to store the output data.
-    std::vector<unsigned char> encoded_data;
-    picture.custom_ptr = &encoded_data;
-
-    // Use our own WebPWriterFunction implementation.
-    picture.writer = &WebPImageEncoder::WriteOutput;
-
-    // Setup the configuration for the output WebP picture. This is currently
-    // the same as the default configuration for WebP, but since any change in
-    // the WebP defaults would invalidate all caches they are hard coded.
-    config.lossless = 0;
-    config.quality = 75.0;  // between 0 (smallest file) and 100 (biggest).
-
-    // TODO(nyquist): Move image encoding to a different thread when
-    // asynchronous loading of images is possible. The encode work currently
-    // blocks the render thread so we are dropping the method down to 0.
-    // crbug.com/603643.
-    config.method = 0;  // quality/speed trade-off (0=fast, 6=slower-better).
-
-    TRACE_EVENT_BEGIN0("blimp", "WebPImageEncoder::onEncode WebPEncode");
-    // Encode the picture using the given configuration.
-    bool success = WebPEncode(&config, &picture);
-    TRACE_EVENT_END1("blimp", "WebPImageEncoder::onEncode WebPEncode",
-                     "EncodedImageSize", encoded_data.size());
-
-    // Release the memory allocated by WebPPictureImport*(). This does not free
-    // the memory used by the picture object itself.
-    WebPPictureFree(&picture);
-
-    if (!success)
-      return nullptr;
-
-    // Did not find item in cache, so add it to client cache representation
-    // and send full item.
-    g_client_cache_contents.Get().insert(blob_id);
-    proto.set_payload(&encoded_data.front(), encoded_data.size());
-
-    // Copy proto into SkData.
-    SkData* sk_data = BlobCacheImageMetadataProtoAsSkData(proto);
-    DVLOG(2) << "Sending image: " << blob_id_hex
-             << " size = " << sk_data->size();
-    return sk_data;
   }
-
- private:
-  // WebPWriterFunction implementation.
-  static int WriteOutput(const uint8_t* data,
-                         size_t size,
-                         const WebPPicture* const picture) {
-    std::vector<unsigned char>* dest =
-        static_cast<std::vector<unsigned char>*>(picture->custom_ptr);
-    dest->insert(dest->end(), data, data + size);
-    return 1;
-  }
-
-  // For each pixel, un-premultiplies the alpha-channel for each of the RGB
-  // channels. As an example, for a channel value that before multiplication was
-  // 255, and after applying an alpha of 128, the premultiplied pixel would be
-  // 128. The un-premultiply step uses the alpha-channel to get back to 255. The
-  // alpha channel is kept unchanged.
-  void UnPremultiply(const unsigned char* in_pixels,
-                     unsigned char* out_pixels,
-                     size_t pixel_count) {
-    const SkUnPreMultiply::Scale* table = SkUnPreMultiply::GetScaleTable();
-    for (; pixel_count-- > 0; in_pixels += 4) {
-      unsigned char alpha = in_pixels[3];
-      if (alpha == 255) {  // Full opacity, just blindly copy.
-        *out_pixels++ = in_pixels[0];
-        *out_pixels++ = in_pixels[1];
-        *out_pixels++ = in_pixels[2];
-        *out_pixels++ = alpha;
-      } else {
-        SkUnPreMultiply::Scale scale = table[alpha];
-        *out_pixels++ = SkUnPreMultiply::ApplyScale(scale, in_pixels[0]);
-        *out_pixels++ = SkUnPreMultiply::ApplyScale(scale, in_pixels[1]);
-        *out_pixels++ = SkUnPreMultiply::ApplyScale(scale, in_pixels[2]);
-        *out_pixels++ = alpha;
-      }
-    }
   }
 
   bool PlatformPictureImport(const unsigned char* pixels,
@@ -202,28 +82,22 @@ class WebPImageEncoder : public SkPixelSerializer {
       return WebPPictureImportRGBA(picture, pixels, row_stride);
     return WebPPictureImportBGRA(picture, pixels, row_stride);
   }
-};
-
-}  // namespace
-
-namespace engine {
 
-EngineImageSerializationProcessor::EngineImageSerializationProcessor(
-    mojom::BlobChannelPtr blob_channel)
-    : blob_channel_(std::move(blob_channel)) {
-  DCHECK(blob_channel_);
+  }  // namespace
 
-  pixel_serializer_.reset(new WebPImageEncoder);
-
-  // Dummy BlobChannel command.
-  // TODO(nyquist): Remove this after integrating BlobChannel.
-  blob_channel_->Push("foo");
+  EngineImageSerializationProcessor::EngineImageSerializationProcessor(
+      std::unique_ptr<BlobChannelSenderProxy> blob_channel)
+      : blob_channel_(std::move(blob_channel)) {
+    DCHECK(blob_channel_);
+    WebPMemoryWriterInit(&writer_state_);
 }
 
-EngineImageSerializationProcessor::~EngineImageSerializationProcessor() {}
+EngineImageSerializationProcessor::~EngineImageSerializationProcessor() {
+  WebPMemoryWriterClear(&writer_state_);
+}
 
 SkPixelSerializer* EngineImageSerializationProcessor::GetPixelSerializer() {
-  return pixel_serializer_.get();
+  return this;
 }
 
 SkPicture::InstallPixelRefProc
@@ -231,5 +105,108 @@ EngineImageSerializationProcessor::GetPixelDeserializer() {
   return nullptr;
 }
 
+bool EngineImageSerializationProcessor::onUseEncodedData(const void* data,
+                                                         size_t len) {
+  TRACE_EVENT1("blimp", "WebPImageEncoded::UsingEncodedData",
+               "OriginalImageSize", len);
+  // Encode all images regardless of their format, including WebP images.
+  return false;
+}
+
+SkData* EngineImageSerializationProcessor::onEncode(const SkPixmap& pixmap) {
+  TRACE_EVENT0("blimp", "WebImageEncoder::onEncode");
+
+  // Ensure width and height are valid dimensions.
+  if (!pixmap.width() || pixmap.width() > WEBP_MAX_DIMENSION)
+    return nullptr;
+  if (!pixmap.height() || pixmap.height() > WEBP_MAX_DIMENSION)
+    return nullptr;

[Wez, 2016/06/07 01:18:31]

Can these ever fail in reality, or would it indicate a caller error? i.e. is the
|pixmap| generated directly from content-supplied fields, or by Blink code that
we can assume creates valid data? If the latter then consider [D]CHECKing here.

[Kevin M, 2016/06/08 00:21:09]

Done.

[Wez, 2016/06/10 22:29:54]

I'd recommend using DCHECK_GT(width, 0) here, rather than DCHECK(width), for
clarity - SkPixmap::width() is an |int|, so the existing code wasn't strictly
correct.

+
+  // Send the image to the client via the BlobChannel if we know that the
+  // client doesn't have it.
+  const BlobId blob_id = CalculateBlobId(pixmap.addr(), pixmap.getSafeSize());
+  if (!blob_channel_->IsInEngineCache(blob_id)) {
+    blob_channel_->PutBlob(blob_id, EncodeImageAsBlob(pixmap));
+    DCHECK(blob_channel_->IsInEngineCache(blob_id));
+  }
+
+  // Push the blob to the client, if the client doesn't already have it.
+  if (!blob_channel_->IsInClientCache(blob_id)) {
+    blob_channel_->DeliverBlob(blob_id);
+  }
+
+  // Construct a proto with the image metadata.
+  BlobCacheImageMetadata proto;
+  proto.set_id(blob_id);
+  proto.set_width(pixmap.width());
+  proto.set_height(pixmap.height());
+
+  SkData* sk_data = BlobCacheImageMetadataProtoAsSkData(proto);

[Wez, 2016/06/07 01:18:31]

Under what circumstances can this call actually fail?

[Kevin M, 2016/06/08 00:21:09]

I don't think it can so I'll remove the DCHECK, unless +nyquist@chromium.org can
talk me out of it :)

[Wez, 2016/06/10 22:29:54]

Acknowledged.

[Kevin M, 2016/06/10 23:51:50]

Done.

+  DCHECK(sk_data);
+  DVLOG(3) << "Returning image ID " << BlobIdToString(blob_id);
+  return sk_data;
+}
+
+scoped_refptr<BlobData> EngineImageSerializationProcessor::EncodeImageAsBlob(
+    const SkPixmap& pixmap) {
+  DVLOG(2) << "Encoding image color_type=" << pixmap.colorType()
+           << ", alpha_type=" << pixmap.alphaType() << " " << pixmap.width()
+           << "x" << pixmap.height();
+
+  WebPConfig config;
+  if (!WebPConfigInit(&config))
+    return nullptr;
+
+  WebPPicture picture;
+  if (!WebPPictureInit(&picture))
+    return nullptr;
+  picture.width = pixmap.width();
+  picture.height = pixmap.height();
+
+  // Import picture from raw pixels.
+  auto pixel_chars = static_cast<const unsigned char*>(pixmap.addr());
+  if (!PlatformPictureImport(pixel_chars, &picture, pixmap.alphaType())) {
+    LOG(FATAL) << "PlatformPictureImport() failed.";

[Wez, 2016/06/07 01:18:31]

Don't mix CHECK() and equivalents in with error-returns like this, except in the
special-case that we're dealing with data received from a potentially-malicious
source.

In this case we presumably expect that WebPConfigInit, WebPPictureInit and
PlatformPictureImport will all always succeed - only the latter appears to
actually depend on the supplied pixmap, so it's actually the only one I could
conceivably see failing at run-time due to anything other than a coding error.

So I think these should all be CHECK()s?

[Kevin M, 2016/06/08 00:21:09]

Done.

+    return nullptr;
+  }
+
+  // Set up the writer parameters.
+  writer_state_.size = 0;
+  picture.custom_ptr = &writer_state_;
+  picture.writer = &WebPMemoryWrite;
+
+  // Setup the configuration for the output WebP picture. This is currently
+  // the same as the default configuration for WebP, but since any change in
+  // the WebP defaults would invalidate all caches they are hard coded.
+  config.lossless = 0;
+  config.quality = 75.0;  // between 0 (smallest file) and 100 (biggest).
+
+  // TODO(nyquist): Move image encoding to a different thread when
+  // asynchronous loading of images is possible. The encode work currently
+  // blocks the render thread so we are dropping the method down to 0.
+  // crbug.com/603643.
+  config.method = 0;  // quality/speed trade-off (0=fast, 6=slower-better).
+
+  TRACE_EVENT_BEGIN0("blimp", "WebPImageEncoder::onEncode WebPEncode");
+  // Encode the picture using the given configuration.
+  bool success = WebPEncode(&config, &picture);
+  TRACE_EVENT_END1("blimp", "WebPImageEncoder::onEncode WebPEncode",
+                   "EncodedImageSize", static_cast<int>(writer_state_.size));
+
+  // Release the memory allocated by WebPPictureImport*(). This does not
+  // free the memory used by the picture object itself.
+  WebPPictureFree(&picture);
+
+  if (!success) {
+    LOG(FATAL) << "WebPPictureFree() failed.";

[Wez, 2016/06/07 01:18:31]

Looks like this means WebPEncode() failed, not WebPPictureFree()?

[Kevin M, 2016/06/08 00:21:09]

Done.

+    return nullptr;
+  }
+
+  scoped_refptr<BlobData> blob_data(new BlobData);
+  blob_data->data.assign(reinterpret_cast<const char*>(writer_state_.mem),
+                         writer_state_.size);
+  return blob_data;
+}
+
 }  // namespace engine
 }  // namespace blimp