Make image_decoder a Leaky LazyInstance

chrome/browser/image_decoder.cc

Index: chrome/browser/image_decoder.cc
diff --git a/chrome/browser/image_decoder.cc b/chrome/browser/image_decoder.cc
index 2f9e83b7ab8953f41eb2d5a4bfc091e8f7e81ce7..f4b10479159f9439637bc86fe0d02d1b835b2fc3 100644
--- a/chrome/browser/image_decoder.cc
+++ b/chrome/browser/image_decoder.cc
@@ -13,38 +13,137 @@
 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) {
-}
-
-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() {}
-
-void ImageDecoder::Start(scoped_refptr<base::SequencedTaskRunner> task_runner) {
-  task_runner_ = task_runner;
+// static, Leaky to allow access from any thread.
+base::LazyInstance<ImageDecoder>::Leaky image_decoder_instance_ =

[dcheng, 2015/03/23 11:52:09]

You /could/ make this a base::LazyInstance<scoped_refptr<ImageDecoder>>.

It's a little horrible, but it gets rid of the boilerplate class and is not
without precedent.

Yes.
Ick.

However, the downside is this results requires two checks per call to Start() /
Cancel(): the implicit one by LazyInstance<T>, and then the check by Start() /
Cancel() to set the ImageDecoder in the scoped_refptr if it doesn't already
exist. In general, it looks like people only use this pattern when they need to
pass the constructor some arguments.


... instead of that, maybe we could do this instead:
base::LazyInstance<ImageDecoder>::Leaky image_decoder_instance = ...

And then in ImageDecoder()'s constructor, we can leak a ref so we guarantee it's
never deleted.

[Theresa, 2015/03/23 17:33:10]

I'm not sure that a reference needs to be leaked. The LazyInstance documentation
says "When the instance is constructed it is registered with AtExitManager. The
destructor will be called on program exit." ImageDecoder isn't refcounted atm,
and its destructor is private, so its destructor shouldn't be getting called any
time except at program exit.

[dcheng, 2015/03/24 05:18:18]

To clarify, I'm proposing that we eliminate the ImageDecoder / ImageDecoderImpl
wrapping and just lift the ImageDecoderImpl implementation into the parent
class.

Then, to make sure that the ImageDecoder isn't accidentally destroyed, we need
to leak a ref in ImageDecoder's ctor, since refcounted objects start with a
refcount of 0.

[Theresa, 2015/03/24 23:51:42]

Done. Thank you for the clarification :) I should have picked up on that.

To "leak" a reference, I just called AddRef() in the constructor. Is that the
appropriate way to make sure it's not accidentally destroyed or is there a
different pattern we use?

[dcheng, 2015/03/25 14:10:09]

Yep, that's the best way.

+    LAZY_INSTANCE_INITIALIZER;
+
+// static
+ImageDecoder* ImageDecoder::GetInstance() {
+  return image_decoder_instance_.Pointer();
+}
+
+ImageDecoder::ImageDecoder() : image_decoder_impl_(new ImageDecoderImpl()) {
+}
+
+ImageDecoder::~ImageDecoder() {
+}
+
+ImageDecoder::ImageRequest::ImageRequest(
+    const scoped_refptr<base::SequencedTaskRunner>& t)
+    : t(t) {
+}
+
+ImageDecoder::ImageRequest::~ImageRequest() {
+  ImageDecoder::Cancel(this);
+}
+
+void ImageDecoder::Start(ImageRequest* image_request,
+                         const std::string& image_data,
+                         ImageCodec image_codec,
+                         bool shrink_to_fit) {
+  DCHECK(image_request);
+  DCHECK(image_request->task_runner());
   BrowserThread::PostTask(
-     BrowserThread::IO, FROM_HERE,
-     base::Bind(&ImageDecoder::DecodeImageInSandbox, this, image_data_));
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(
+          &ImageDecoder::ImageDecoderImpl::DecodeImageInSandbox,
+          GetInstance()->image_decoder_impl_, image_request,
+          std::vector<unsigned char>(image_data.begin(), image_data.end()),
+          image_codec, shrink_to_fit));
 }
 
-bool ImageDecoder::OnMessageReceived(const IPC::Message& message) {
+void ImageDecoder::Cancel(ImageRequest* image_request) {
+  DCHECK(image_request);
+  GetInstance()->image_decoder_impl_->Cancel(image_request);
+}
+
+// ImageDecoder::ImageDecoderImpl Methods
+
+ImageDecoder::ImageDecoderImpl::ImageDecoderImpl()
+    : image_request_id_counter_(0),
+      last_request_(base::TimeTicks::Now()),
+      batch_mode_started_(false) {
+}
+
+ImageDecoder::ImageDecoderImpl::~ImageDecoderImpl() {
+}
+
+void ImageDecoder::ImageDecoderImpl::DecodeImageInSandbox(
+    ImageRequest* image_request,
+    const std::vector<unsigned char>& image_data,
+    ImageCodec image_codec,
+    bool shrink_to_fit) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  if (!batch_mode_started_) {

[dcheng, 2015/03/23 11:52:09]

Can we just check that utility_process_host_ is non null here? This should also
handle the case if the utility process crashes for some reason.

[Theresa, 2015/03/23 17:33:10]

Done.

+    StartBatchMode();
+  }
+
+  last_request_ = base::TimeTicks::Now();
+  base::AutoLock lock(map_lock_);
+  image_request_id_map_.insert(
+      std::pair<int, ImageRequest*>(image_request_id_counter_, image_request));

[dcheng, 2015/03/23 11:52:09]

Use std::make_pair so you don't explicitly need to name the types.

[Theresa, 2015/03/23 17:33:10]

Done.

+
+  switch (image_codec) {
+    case ROBUST_JPEG_CODEC:
+      utility_process_host_->Send(new ChromeUtilityMsg_RobustJPEGDecodeImage(
+          image_data, image_request_id_counter_));
+      break;
+    case DEFAULT_CODEC:
+      utility_process_host_->Send(new ChromeUtilityMsg_DecodeImage(
+          image_data, shrink_to_fit, image_request_id_counter_));
+      break;
+    default:

[dcheng, 2015/03/23 11:52:09]

You should be OK omitting this; adding a new enum that's not handled will
generate a compile warning/error that way.

[Theresa, 2015/03/23 17:33:10]

Done.

+      NOTREACHED();
+  }
+
+  ++image_request_id_counter_;
+}
+
+void ImageDecoder::ImageDecoderImpl::Cancel(ImageRequest* image_request) {
+  base::AutoLock lock(map_lock_);
+  std::vector<int> canceled_requests;
+
+  for (RequestMap::iterator it = image_request_id_map_.begin();

[dcheng, 2015/03/23 11:52:09]

Use a foreach here to save some typing as well.

Alternatively, you can remove the need for two loops by doing this:

for (auto it = image_request_id_map_.begin(); it != image_request_id_map_.end();
) {
  if (it->second == image_request) {
    image_request_id_map_.erase(it++);
  } else {
    ++it;
  }
}

Also, ++it is generally preferred to it++ if the behavior of the post-increment
version is not required. It can be slightly more efficient, since it++ requires
a copy by nature (though I think that can be often optimized away).

[Theresa, 2015/03/23 17:33:10]

Done.

+       it != image_request_id_map_.end(); it++) {
+    if (it->second == image_request) {
+      canceled_requests.push_back(it->first);
+    }
+  }
+
+  for (auto id : canceled_requests) {
+    image_request_id_map_.erase(id);
+  }
+}
+
+void ImageDecoder::ImageDecoderImpl::StartBatchMode() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  utility_process_host_ =
+      UtilityProcessHost::Create(this, base::MessageLoopProxy::current().get())
+          ->AsWeakPtr();
+  utility_process_host_->StartBatchMode();
+  batch_mode_started_ = true;
+  batch_mode_timer_.Start(FROM_HERE, kBatchModeTimeout, this,
+                          &ImageDecoder::ImageDecoderImpl::StopBatchMode);
+}
+
+void ImageDecoder::ImageDecoderImpl::StopBatchMode() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  if ((base::TimeTicks::Now() - last_request_) < kBatchModeTimeout) {
+    return;
+  }
+
+  if (utility_process_host_) {
+    utility_process_host_->EndBatchMode();
+    utility_process_host_.reset();
+  }
+  batch_mode_started_ = false;
+  batch_mode_timer_.Stop();
+}
+
+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,
@@ -54,28 +153,30 @@ bool ImageDecoder::OnMessageReceived(const IPC::Message& message) {
   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) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  base::AutoLock lock(map_lock_);
+  if (image_request_id_map_.find(id) != image_request_id_map_.end()) {

[dcheng, 2015/03/23 11:52:09]

It's a pretty common idiom to do this:
auto it = image_request_id_map_.find(id);
if (it != image_request_id_map_.end()) {
  ImageRequest* image_request = it->second;
  ...
  image_request_id_map_.erase(it);
}

[Theresa, 2015/03/23 17:33:10]

Done.

+    ImageRequest* image_request = image_request_id_map_.find(id)->second;
+    image_request->task_runner()->PostTask(
+        FROM_HERE, base::Bind(&ImageRequest::OnImageDecoded,
+                              base::Unretained(image_request), decoded_image));
 
-void ImageDecoder::OnDecodeImageFailed() {
-  DCHECK(task_runner_->RunsTasksOnCurrentThread());
-  if (delegate_)
-    delegate_->OnDecodeImageFailed(this);
+    image_request_id_map_.erase(id);
+  }
 }
 
-void ImageDecoder::DecodeImageInSandbox(
-    const std::vector<unsigned char>& image_data) {
+void ImageDecoder::ImageDecoderImpl::OnDecodeImageFailed(int 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));
-  } else {
-    utility_process_host->Send(
-        new ChromeUtilityMsg_DecodeImage(image_data, shrink_to_fit_));
+  base::AutoLock lock(map_lock_);
+  if (image_request_id_map_.find(id) != image_request_id_map_.end()) {
+    ImageRequest* image_request = image_request_id_map_.find(id)->second;
+    image_request->task_runner()->PostTask(
+        FROM_HERE, base::Bind(&ImageRequest::OnDecodeImageFailed,
+                              base::Unretained(image_request)));
+
+    image_request_id_map_.erase(id);
   }
 }