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..ddec71f461f55d5ed6fb0fe2de73485672bd3412 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_host.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();
@@ -42,8 +40,14 @@ SkData* BlobCacheImageMetadataProtoAsSkData(
 // See crbug.com/548434.
 class WebPImageEncoder : public SkPixelSerializer {
  public:
-  WebPImageEncoder() {}
-  ~WebPImageEncoder() override{};
+  explicit WebPImageEncoder(BlobChannelSenderHost* blob_channel)
+      : blob_channel_(blob_channel) {
+    DCHECK(blob_channel_);
+
+    WebPMemoryWriterInit(&writer_state_);
+  }
+
+  ~WebPImageEncoder() override { WebPMemoryWriterClear(&writer_state_); };
 
   bool onUseEncodedData(const void* data, size_t len) override {
     TRACE_EVENT1("blimp", "WebPImageEncoded::UsingEncodedData",
@@ -54,94 +58,96 @@ class WebPImageEncoder : public SkPixelSerializer {
 
   SkData* onEncode(const SkPixmap& pixmap) override {

[Wez, 2016/05/21 01:08:04]

This function has various error-handling (invalid dimensions, failure to
initialize WebP structures) - is it ever realistic for them to fail?

If not then should we change those to CHECKs?

[Kevin M, 2016/05/27 22:35:31]

Done.

     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();
 
+    // 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());
-    std::string blob_id_hex = BlobIdToString(blob_id);
+    if (!(blob_channel_->GetReplicationState(blob_id) &
+          BlobChannelSenderHost::ReplicationState::ENGINE)) {

[Wez, 2016/05/21 01:08:05]

nit: Would be more readable if you added a simple helper for the replication
state, or even just broke it into a struct with two bools!

[Kevin M, 2016/05/27 22:35:31]

Done.

+      DVLOG(2) << "Encoding image color_type=" << pixmap.colorType()

[nyquist, 2016/05/24 00:05:41]

This method mixes low-level bits and bytes with high-level concepts as
replication state. Could you see if there was a way for this method to be more
human readable at a high level? Maybe extracting out parts of the core pieces,
such as the WebP encoding itself?

As in, passing in the writer state to the encode function as a pointer and doing
the Put() on the outside?

[Kevin M, 2016/05/27 22:35:31]

How's this?

+               << ", alpha_type=" << pixmap.alphaType() << " " << pixmap.width()
+               << "x" << pixmap.height();
+
+      // Initialize an empty WebPConfig.

[Wez, 2016/05/21 01:08:04]

nit: This comment and the init one below don't reallu add anything.

[Kevin M, 2016/05/27 22:35:31]

Done.

+      WebPConfig config;
+      if (!WebPConfigInit(&config))
+        return nullptr;
+
+      // Initialize an empty WebPPicture.
+      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()))
+        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)
+        return nullptr;
+
+      DVLOG(2) << "Sending image " << BlobIdToString(blob_id)
+               << " (size = " << writer_state_.size << ").";
+
+      blob_channel_->Put(
+          blob_id, std::string(reinterpret_cast<const char*>(writer_state_.mem),
+                               writer_state_.size));
+    }
+
+    DCHECK(blob_channel_->GetReplicationState(blob_id) &
+           BlobChannelSenderHost::ReplicationState::ENGINE);
+
+    // Push the blob to the client, if the client doesn't already have it.
+    if (!(blob_channel_->GetReplicationState(blob_id) &
+          BlobChannelSenderHost::CLIENT)) {
+      blob_channel_->Push(blob_id);
+    }
 
-    // Create proto with all requires information.
+    // Construct a proto with the image metadata.
     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;
-    }
-
-    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();
+    DVLOG(3) << "Returning image ID " << BlobIdToString(blob_id);
     return sk_data;
   }
 
@@ -202,22 +208,23 @@ class WebPImageEncoder : public SkPixelSerializer {
       return WebPPictureImportRGBA(picture, pixels, row_stride);
     return WebPPictureImportBGRA(picture, pixels, row_stride);
   }
+
+  // Reusable output buffer for image encoding.
+  WebPMemoryWriter writer_state_;
+
+  BlobChannelSenderHost* blob_channel_;

[Wez, 2016/05/21 01:08:04]

nit: Brief comment to clarify purpose & ownership.

[Kevin M, 2016/05/27 22:35:31]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(WebPImageEncoder);
 };
 
 }  // namespace
 
-namespace engine {
-
 EngineImageSerializationProcessor::EngineImageSerializationProcessor(
-    mojom::BlobChannelPtr blob_channel)
+    std::unique_ptr<BlobChannelSenderHost> blob_channel)
     : blob_channel_(std::move(blob_channel)) {
   DCHECK(blob_channel_);
 
-  pixel_serializer_.reset(new WebPImageEncoder);
-
-  // Dummy BlobChannel command.
-  // TODO(nyquist): Remove this after integrating BlobChannel.
-  blob_channel_->Push("foo");
+  pixel_serializer_.reset(new WebPImageEncoder(blob_channel_.get()));

[Wez, 2016/05/21 01:08:05]

nit: Is there a reason why the EngineImageSerializationProcessor can't also
implement the PixelSerializer interface, rather than have two separate objects?

Does the EISP need to retain ownership of the blob_channel, or could it be
move()d, to make ownership/lifetime clearer?

[Kevin M, 2016/05/27 22:35:31]

Done.

 }
 
 EngineImageSerializationProcessor::~EngineImageSerializationProcessor() {}