Make image_decoder a Leaky LazyInstance

chrome/browser/image_decoder.cc

 // Copyright (c) 2012 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/browser/image_decoder.h"
 
 #include "base/bind.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/common/chrome_utility_messages.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/utility_process_host.h"
 
 using content::BrowserThread;
 using content::UtilityProcessHost;
 
-ImageDecoder::ImageDecoder(Delegate* delegate,
-                           const std::string& image_data,
-                           ImageCodec image_codec)
-    : delegate_(delegate),
-      image_data_(image_data.begin(), image_data.end()),
-      image_codec_(image_codec),
-      task_runner_(NULL),
-      shrink_to_fit_(false) {
+
+// static, Leaky to allow access from any thread.
+base::LazyInstance<ImageDecoder>::Leaky image_decoder_instance_ =
+    LAZY_INSTANCE_INITIALIZER;
+
+// static
+ImageDecoder* ImageDecoder::GetInstance() {
+  return image_decoder_instance_.Pointer();
 }
 
-ImageDecoder::ImageDecoder(Delegate* delegate,
-                           const std::vector<char>& image_data,
-                           ImageCodec image_codec)
-    : delegate_(delegate),
-      image_data_(image_data.begin(), image_data.end()),
-      image_codec_(image_codec),
-      task_runner_(NULL),
-      shrink_to_fit_(false) {
+ImageDecoder::ImageDecoder() {
+  image_decoder_impl_ = new ImageDecoderImpl();

[dcheng, 2015/02/27 16:47:17]

It might be better to put this in the initializer. Then you mark the member
const as well?

[dcheng, 2015/02/27 16:47:17]

It might be better to put this in the initializer. Then you can mark the member
const as well.

[Theresa, 2015/03/04 03:10:06]

Done.

[Theresa, 2015/03/04 03:10:06]

Done.

 }
 
-ImageDecoder::~ImageDecoder() {}
-
-void ImageDecoder::Start(scoped_refptr<base::SequencedTaskRunner> task_runner) {
-  task_runner_ = task_runner;
-  BrowserThread::PostTask(
-     BrowserThread::IO, FROM_HERE,
-     base::Bind(&ImageDecoder::DecodeImageInSandbox, this, image_data_));
+ImageDecoder::~ImageDecoder() {
 }
 
-bool ImageDecoder::OnMessageReceived(const IPC::Message& message) {
+void ImageDecoder::Start(Delegate* delegate,
+                         const std::string& image_data,
+                         ImageCodec image_codec,
+                         scoped_refptr<base::SequencedTaskRunner> task_runner,
+                         bool shrink_to_fit) {
+  image_decoder_impl_->Start(delegate, image_data, image_codec,
+                             task_runner, shrink_to_fit);
+}
+
+// ImageDecoder::ImageDecoderImpl Methods
+
+ImageDecoder::ImageDecoderImpl::ImageDecoderImpl(){

[dcheng, 2015/02/27 16:47:17]

clang-format should catch these random missing spaces, though it still mangles
IPC message map macros, so you'll need to be a bit careful.

[Theresa, 2015/03/04 03:10:06]

Done.

+}
+
+ImageDecoder::ImageDecoderImpl::~ImageDecoderImpl(){
+}
+
+void ImageDecoder::ImageDecoderImpl::Start(
+    Delegate* delegate,
+    const std::string& image_data,
+    ImageCodec image_codec,
+    scoped_refptr<base::SequencedTaskRunner> task_runner,
+    bool shrink_to_fit) {
+  // If the task runner is null, we can't post tasks back to it when the
+  // image is decoded. Notify the delegate if possible and return.
+  if (!task_runner) {

[dcheng, 2015/02/27 16:47:17]

This seems like a programming error that we should be DCHECK()ing on, rather
than trying to handle it gracefully somehow.

[Theresa, 2015/03/04 03:10:06]

Done.

+    if (delegate) {

[dcheng, 2015/02/27 16:47:17]

IMO, |delegate| should probably be required too.

[Theresa, 2015/03/04 03:10:06]

Done.

+      delegate->OnDecodeImageFailed();
+    }
+    return;
+  }
+
+  if (!batch_mode_started_) {
+    task_runner_at_start_ = task_runner;
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(
+            &ImageDecoder::ImageDecoderImpl::StartBatchMode,
+            this,
+            delegate,
+            image_data,
+            image_codec,
+            shrink_to_fit));
+    return;
+  }
+
+  last_request_ = time(0);
+  delegate_map_[delegate_id_counter_] = delegate;

[dcheng, 2015/02/27 16:47:17]

I'd recommend using IDMap here. Also, it's probably better to just maintain one
map than two parallel maps.

[Theresa, 2015/03/04 03:10:06]

I changed it to one map instead of two parallel maps. IDMap can't be accessed
from multiple threads (it can be created on one thread and accessed from a
different, consistent thread post-construction, but not multiple threads
post-construction), so the DCHECK's in that class for CalledOnValidThread()
fail.

+  task_runner_map_[delegate_id_counter_] = task_runner;
+
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(
+          &ImageDecoder::ImageDecoderImpl::DecodeImageInSandbox,
+          this,
+          delegate,
+          std::vector<unsigned char>(image_data.begin(), image_data.end()),
+          image_codec,
+          shrink_to_fit,
+          delegate_id_counter_));
+
+  ++delegate_id_counter_;
+}
+
+void ImageDecoder::ImageDecoderImpl::StartBatchMode(
+    Delegate* delegate,
+    const std::string& image_data,
+    ImageCodec image_codec,
+    bool shrink_to_fit) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  utility_process_host_ = UtilityProcessHost::Create(
+      this, base::MessageLoopProxy::current().get())->AsWeakPtr();
+  utility_process_host_->StartBatchMode();
+  batch_mode_started_ = true;
+  PostDelayedStopBatchModeTask();
+  Start(delegate, image_data, image_codec, task_runner_at_start_,
+        shrink_to_fit);
+}
+
+void ImageDecoder::ImageDecoderImpl::PostDelayedStopBatchModeTask() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  BrowserThread::PostDelayedTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&ImageDecoder::ImageDecoderImpl::StopBatchMode,
+                 this),
+      base::TimeDelta::FromSeconds(batch_mode_timeout_seconds_));
+}
+
+void ImageDecoder::ImageDecoderImpl::StopBatchMode() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  if ((time(0) - last_request_) < batch_mode_timeout_seconds_) {

[dcheng, 2015/02/27 16:47:17]

I think we can use base::DelayTimer here to simplify the code, instead of having
a lot of our own tracking logic.

[Theresa, 2015/03/04 03:10:06]

The timer api's aren't thread safe, so they won't work here.

[Theresa, 2015/03/05 16:53:05]

I rethought this and spent some time playing around with it last night. I can
work around the non-thread-safe nature by putting the DelayTimer creation and
Reset calls in StartBatchMode and DecodeImageInSandbox. I have a patchset
stashed that does this, but there are some crashes I need to debug. One thing I
don't like about using the DelayTimer is in the case I'm testing, the bookmarks
grid view, if you scroll past hundreds of bookmarks, hundreds of requests are
made to decode images and Reset is called on the timer hundreds of time causing
a bunch of churn. In the current approach, a delayed task is posted at most once
every 5 seconds (rather than reset hundreds of times), so I'd like to stick with
the current approach. If that's not okay, I can debug the crashes and change it
to a delay timer.

For the IDMap (other comments above), there is in theory a similar workaround
where I could insert and remove items solely from the methods that run on the IO
thread, but I'd have to work around the issue of scoped ptrs not being Bindable
because the task_runners would have to be passed to DecodeImageInSandbox.

+    PostDelayedStopBatchModeTask();
+    return;
+  }
+
+  if (utility_process_host_.get()) {
+    utility_process_host_->EndBatchMode();
+    utility_process_host_.reset();
+  }
+  batch_mode_started_ = false;
+}
+
+bool ImageDecoder::ImageDecoderImpl::OnMessageReceived(
+    const IPC::Message& message) {
   bool handled = true;
-  IPC_BEGIN_MESSAGE_MAP(ImageDecoder, message)
+  IPC_BEGIN_MESSAGE_MAP(ImageDecoder::ImageDecoderImpl, message)
     IPC_MESSAGE_HANDLER(ChromeUtilityHostMsg_DecodeImage_Succeeded,
                         OnDecodeImageSucceeded)
     IPC_MESSAGE_HANDLER(ChromeUtilityHostMsg_DecodeImage_Failed,
                         OnDecodeImageFailed)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
 }
 
-void ImageDecoder::OnDecodeImageSucceeded(const SkBitmap& decoded_image) {
-  DCHECK(task_runner_->RunsTasksOnCurrentThread());
-  if (delegate_)
-    delegate_->OnImageDecoded(this, decoded_image);
+void ImageDecoder::ImageDecoderImpl::OnDecodeImageSucceeded(
+    const SkBitmap& decoded_image,
+    int id) {
+  if (delegate_map_.find(id) != delegate_map_.end()
+      && task_runner_map_.find(id) != task_runner_map_.end()) {
+    task_runner_map_[id]->PostTask(
+        FROM_HERE,
+        base::Bind(
+            &ImageDecoder::ImageDecoderImpl::NotifyDelegateDecodeSucceeded,
+            this,
+            decoded_image,
+            delegate_map_[id]));
+
+    delegate_map_.erase(id);
+    task_runner_map_.erase(id);
+  }
 }
 
-void ImageDecoder::OnDecodeImageFailed() {
-  DCHECK(task_runner_->RunsTasksOnCurrentThread());
-  if (delegate_)
-    delegate_->OnDecodeImageFailed(this);
+void ImageDecoder::ImageDecoderImpl::OnDecodeImageFailed(int id) {
+  if (delegate_map_.find(id) != delegate_map_.end()
+      && task_runner_map_.find(id) != task_runner_map_.end()) {
+    task_runner_map_[id]->PostTask(
+        FROM_HERE,
+        base::Bind(
+            &ImageDecoder::ImageDecoderImpl::NotifyDelegateDecodeFailed,
+            this,
+            delegate_map_[id]));
+
+    delegate_map_.erase(id);
+    task_runner_map_.erase(id);
+  }
 }
 
-void ImageDecoder::DecodeImageInSandbox(
-    const std::vector<unsigned char>& image_data) {
+void ImageDecoder::ImageDecoderImpl::NotifyDelegateDecodeSucceeded(
+    const SkBitmap& decoded_image, Delegate* delegate) {
+    delegate->OnImageDecoded(decoded_image);
+}
+
+void ImageDecoder::ImageDecoderImpl::NotifyDelegateDecodeFailed(
+    Delegate* delegate) {
+  delegate->OnDecodeImageFailed();
+}
+
+void ImageDecoder::ImageDecoderImpl::DecodeImageInSandbox(
+    Delegate* delegate,
+    const std::vector<unsigned char>& image_data,
+    ImageCodec image_codec,
+    bool shrink_to_fit,
+    int delegate_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  UtilityProcessHost* utility_process_host;
-  utility_process_host = UtilityProcessHost::Create(this, task_runner_.get());
-  if (image_codec_ == ROBUST_JPEG_CODEC) {
-    utility_process_host->Send(
-        new ChromeUtilityMsg_RobustJPEGDecodeImage(image_data));
+  if (image_codec == ROBUST_JPEG_CODEC) {
+    utility_process_host_->Send(
+        new ChromeUtilityMsg_RobustJPEGDecodeImage(image_data,
+                                                   delegate_id));
   } else {
-    utility_process_host->Send(
-        new ChromeUtilityMsg_DecodeImage(image_data, shrink_to_fit_));
+    utility_process_host_->Send(
+        new ChromeUtilityMsg_DecodeImage(image_data, shrink_to_fit,
+                                         delegate_id));
   }
 }