Image Search: Move thumbnail JPEG encoding to the renderer.

chrome/renderer/chrome_render_frame_observer.cc

 // Copyright 2013 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 "chrome/renderer/chrome_render_frame_observer.h"
 
+#include <limits>
+#include <string>
+#include <vector>
+
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/common/prerender_messages.h"
 #include "chrome/common/print_messages.h"
 #include "chrome/common/render_messages.h"
 #include "chrome/renderer/prerender/prerender_helper.h"
 #include "chrome/renderer/printing/print_web_view_helper.h"
 #include "content/public/renderer/render_frame.h"
 #include "skia/ext/image_operations.h"
-#include "skia/ext/platform_canvas.h"
 #include "third_party/WebKit/public/platform/WebImage.h"
 #include "third_party/WebKit/public/web/WebElement.h"
 #include "third_party/WebKit/public/web/WebNode.h"
+#include "third_party/skia/include/core/SkBitmap.h"
+#include "ui/gfx/codec/jpeg_codec.h"
 
 using blink::WebElement;
 using blink::WebNode;
 
 namespace {
 
 // If the source image is null or occupies less area than
 // |thumbnail_min_area_pixels|, we return the image unmodified.  Otherwise, we
 // scale down the image so that the width and height do not exceed
 // |thumbnail_max_size_pixels|, preserving the original aspect ratio.
@@ skipping 81 matching lines @@
   WebNode context_node = render_frame()->GetContextMenuNode();
   SkBitmap thumbnail;
   gfx::Size original_size;
   if (!context_node.isNull() && context_node.isElementNode()) {
     blink::WebImage image = context_node.to<WebElement>().imageContents();
     original_size = image.size();
     thumbnail = Downscale(image,
                           thumbnail_min_area_pixels,
                           thumbnail_max_size_pixels);
   }
+
+  SkBitmap bitmap;
+  if (thumbnail.colorType() == kN32_SkColorType) {

[Peter Kasting, 2014/12/11 20:11:30]

Nit: {} unnecessary

[jbroman, 2014/12/12 00:08:57]

Done.

+    bitmap = thumbnail;
+  } else {
+    thumbnail.copyTo(&bitmap, kN32_SkColorType);
+  }
+
+  std::string thumbnail_data;
+  {
+    SkAutoLockPixels lock(bitmap);

[Peter Kasting, 2014/12/11 20:11:30]

Is it important that this lock be released before the Send() call below?  If
not, I'd avoid placing this in a new block scope.  If so, I'd add a comment as
to why.

[jbroman, 2014/12/12 00:08:57]

It's not important, no. There was just no reason for it to continue to be valid
during Send. Changed.

+    if (bitmap.getPixels() != nullptr &&

[Peter Kasting, 2014/12/11 20:11:30]

Nit: "!= nullptr" unnecessary

[jbroman, 2014/12/12 00:08:57]

Done.

I had some vague idea that Chromium style preferred this, but I can find no
supporting evidence. :D

+        static_cast<int>(bitmap.rowBytes()) > 0) {

[Peter Kasting, 2014/12/11 20:11:30]

Why is this cast to int necessary?

[jbroman, 2014/12/12 00:08:57]

The rowBytes argument to gfx::JPEGCodec::Encode is an int, but
SkBitmap::rowBytes returns a size_t.

Other call sites seem to use a cast in the call (but I don't remember whether
this is necessary with our warning flags on various platforms). I suppose I can
remove this check (it doesn't seem to be done elsewhere), though I'm not certain
whether it's actually impossible to have rowBytes between INT_MAX and SIZE_MAX.

[Peter Kasting, 2014/12/12 00:35:51]

I see.  You need some kind of cast here, then.  The question is whether it
should be a static_cast or a checked_cast.  A checked_cast will terminate
execution (basically CHECK(false)) if the source value doesn't fit in the
destination type.  We use that when we aren't sure the result will succeed, and
it might be a security issue if the cast were to pass the wrong value.

I think in this case a static_cast is OK, since I think it would just result in
the JPEG encoder not reading all the data, but you should make sure of my
reasoning.

[jbroman, 2014/12/12 01:10:46]

If this were to be negative after a cast, it would result in JPEGCodec reading
before the beginning of the data (it does computes a row's address as
&input[next_scanline * row_byte_width).

But Skia does say where SkBitmap::fRowBytes is set that "require[s] that
rowBytes fit in 31bits", so I suppose this assumption is reasonable (and we have
it elsewhere).

So I'm inclined to leave this as-is unless someone has another preference.

+      const int kDefaultQuality = 90;
+      std::vector<unsigned char> data;
+      if (gfx::JPEGCodec::Encode(
+              reinterpret_cast<unsigned char*>(bitmap.getAddr32(0, 0)),
+              gfx::JPEGCodec::FORMAT_SkBitmap, bitmap.width(), bitmap.height(),
+              static_cast<int>(bitmap.rowBytes()), kDefaultQuality, &data))
+        thumbnail_data = std::string(data.begin(), data.end());
+    }
+  }
+
   Send(new ChromeViewHostMsg_RequestThumbnailForContextNode_ACK(
-      routing_id(), thumbnail, original_size));
+      routing_id(), thumbnail_data, original_size));
 }
 
 void ChromeRenderFrameObserver::OnPrintNodeUnderContextMenu() {
   printing::PrintWebViewHelper* helper =
       printing::PrintWebViewHelper::Get(render_frame()->GetRenderView());
   if (helper)
     helper->PrintNode(render_frame()->GetContextMenuNode());
 }