Large wallpaper decoding in utility process

chrome/utility/chrome_content_utility_client.cc

Index: chrome/utility/chrome_content_utility_client.cc
diff --git a/chrome/utility/chrome_content_utility_client.cc b/chrome/utility/chrome_content_utility_client.cc
index e2f9c45a1a7294673391088a78b1179a96c304e7..497b67896f1756bd2667a11b07befabf49f2216c 100644
--- a/chrome/utility/chrome_content_utility_client.cc
+++ b/chrome/utility/chrome_content_utility_client.cc
@@ -18,6 +18,8 @@
 #include "content/public/utility/utility_thread.h"
 #include "courgette/courgette.h"
 #include "courgette/third_party/bsdiff.h"
+#include "ipc/ipc_channel.h"
+#include "skia/ext/image_operations.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/zlib/google/zip.h"
 #include "ui/gfx/codec/jpeg_codec.h"
@@ -172,12 +174,43 @@ void ChromeContentUtilityClient::PreSandboxStartup() {
 #endif  // ENABLE_MDNS
 }
 
+// Compute size of ChromeUtilityHostMsg_DecodeImage_Succeeded message with
+// image argument, and compare with maximum IPC message size
+size_t IsMessageTooBig(const SkBitmap& image, int width, int height) {
+  // Slightly different image size formula from SkImageInfo::getSafeSize64()
+  int64_t msg_size = sk_64_mul(height, width * image.bytesPerPixel());
+  msg_size += sizeof(ChromeUtilityHostMsg_DecodeImage_Succeeded);
+  return msg_size >= static_cast<int64_t>(IPC::Channel::kMaximumMessageSize);
+}
+
 // static
-void ChromeContentUtilityClient::DecodeImage(
-    const std::vector<unsigned char>& encoded_data) {
-  const SkBitmap& decoded_image = content::DecodeImage(&encoded_data[0],
+SkBitmap ChromeContentUtilityClient::DecodeImage(
+    const std::vector<unsigned char>& encoded_data, bool shrink_to_fit) {
+  SkBitmap decoded_image = content::DecodeImage(&encoded_data[0],
                                                        gfx::Size(),
                                                        encoded_data.size());

[bshe, 2014/08/29 19:56:26]

nit: indent

[Greg Levin, 2014/09/04 16:39:19]

Done.

+
+  // If decoded image is too large for IPC message, shrink it by halves

[Tom Sepez, 2014/08/29 19:47:13]

looks like using SkBitmap::computeSize64() would avoid these calculations. And
if IsMessageTooBig took a size_t (or an int64_t to be consistent with Sk's
unfortunate use of signed types), it could avoid multiplying, too ...

[bshe, 2014/08/29 19:56:26]

Ideally, I think we should shrink to best fit. Not just shrink by halves.
This can avoid the over shrunk problem when the image is just slightly over the
limit. Do you mind to leave a TODO.

[Greg Levin, 2014/09/04 16:39:18]

Rewrote size checking logic, hopefully addressed all concerns here

[Greg Levin, 2014/09/04 16:39:19]

Discussed off-line, decided shrinking by halves was okay (right?).  Added
comment to code.

+  int width = decoded_image.width();
+  int height = decoded_image.height();
+  if (shrink_to_fit && IsMessageTooBig(decoded_image, width, height)) {
+    do {

[Tom Sepez, 2014/08/29 19:47:13]

A while loop may get rid of having to call IsMessageTooBig from two places in
this function.

[Greg Levin, 2014/09/04 16:39:18]

Done.

+      width = width/2;

[Tom Sepez, 2014/08/29 19:47:13]

... having done the above, then in this loop, you'd divide
SkBitmap::computeSize64() by 4 on each pass (since you're havling in each
dimension), keeping track of iterations ...

[Greg Levin, 2014/09/04 16:39:18]

Done.

+      height = height/2;

[bshe, 2014/08/29 19:56:26]

nit: width /= 2 or width = width / 2;. same for height.

[Greg Levin, 2014/09/04 16:39:18]

Replaced division with bit shifts

+    } while (IsMessageTooBig(decoded_image, width, height));
+    decoded_image = skia::ImageOperations::Resize(

[Tom Sepez, 2014/08/29 19:47:13]

Lastly, you can extract and divide width, height appropriately.

[Greg Levin, 2014/09/04 16:39:19]

Done.

+        decoded_image, skia::ImageOperations::RESIZE_LANCZOS3,
+        width, height);
+  }
+
+  return decoded_image;
+}
+
+// static
+void ChromeContentUtilityClient::DecodeImageAndSend(
+    const std::vector<unsigned char>& encoded_data, bool shrink_to_fit){
+  SkBitmap decoded_image = DecodeImage(encoded_data, shrink_to_fit);
+

[bshe, 2014/08/29 19:56:26]

Add an error message for over IPC limit case would be nice.

[Greg Levin, 2014/09/04 16:39:19]

Improved error handling and LOG'ed error

   if (decoded_image.empty()) {
     Send(new ChromeUtilityHostMsg_DecodeImage_Failed());
   } else {
@@ -204,8 +237,8 @@ void ChromeContentUtilityClient::OnUnpackWebResource(
 }
 
 void ChromeContentUtilityClient::OnDecodeImage(
-    const std::vector<unsigned char>& encoded_data) {
-  DecodeImage(encoded_data);
+    const std::vector<unsigned char>& encoded_data, bool shrink_to_fit) {
+  DecodeImageAndSend(encoded_data, shrink_to_fit);
 }
 
 #if defined(OS_CHROMEOS)