Creating PNG images from web content to be used by OverviewMode navigation

athena/content/content_proxy.cc

Index: athena/content/content_proxy.cc
diff --git a/athena/content/content_proxy.cc b/athena/content/content_proxy.cc
index a63aa35ed08ddf20da32a8e2ab0e3595d071db28..7614a7c9e078219eb4b21b384924c9f78feeb2e2 100644
--- a/athena/content/content_proxy.cc
+++ b/athena/content/content_proxy.cc
@@ -6,72 +6,104 @@
 
 #include "athena/activity/public/activity.h"
 #include "athena/activity/public/activity_view_model.h"
+#include "base/bind.h"
+#include "base/threading/worker_pool.h"
+#include "content/public/browser/render_view_host.h"
+#include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/web_contents.h"
 #include "ui/aura/window.h"
-#include "ui/views/background.h"
+#include "ui/gfx/codec/png_codec.h"
+#include "ui/gfx/geometry/rect.h"
+#include "ui/gfx/image/image_png_rep.h"
 #include "ui/views/controls/webview/webview.h"
 #include "ui/views/widget/widget.h"
 
 namespace athena {
 
+// Encodes an A8 SkBitmap to grayscale PNG in a worker thread.
+class ProxyImageData : public base::RefCountedThreadSafe<ProxyImageData> {

[oshima, 2014/09/22 16:10:28]

If this is to be used by overview mode, it will be decoded again.
Why this is encoded where? Is it enough to encode when saving to secondary
storage?

[Mr4D (OOO till 08-26), 2014/09/23 19:07:34]

No. The problem is that every activity will have this object and the required
resources to store the uncompressed images will mount.

As such we keep the compressed image here. One step (not yet done though) is to
save the resulting image on disc to re-create upon session restore.

+ public:
+  ProxyImageData() {
+  }
+
+  void EncodeImage(const SkBitmap& bitmap, base::Closure callback) {
+    if (!base::WorkerPool::PostTaskAndReply(FROM_HERE,
+            base::Bind(&ProxyImageData::EncodeOnWorker,
+                       this,
+                       bitmap),
+            callback,
+            true)) {
+      callback.Run();

[oshima, 2014/09/22 16:10:28]

should this be DCHECK? Does it make sense to run callback when the task wasn't
posed?

[Mr4D (OOO till 08-26), 2014/09/23 19:07:33]

Adding a DCHECK is possibly a good idea to see a failure early. However - it
does the "correct" thing in case of a failure and the result will be an empty
bitmap.

[oshima, 2014/09/24 00:40:00]

Acknowledged.

+    }
+  }
+
+  scoped_refptr<base::RefCountedBytes> data() const { return data_; }
+
+ private:
+  friend class base::RefCountedThreadSafe<ProxyImageData>;
+  virtual ~ProxyImageData() {
+  }
+
+  void EncodeOnWorker(const SkBitmap& bitmap) {
+    DCHECK_EQ(bitmap.colorType(), kAlpha_8_SkColorType);
+    // Encode the A8 bitmap to grayscale PNG treating alpha as color intensity.
+    std::vector<unsigned char> data;
+    if (gfx::PNGCodec::EncodeA8SkBitmap(bitmap, &data))
+      data_ = new base::RefCountedBytes(data);
+  }
+
+  scoped_refptr<base::RefCountedBytes> data_;
+
+  DISALLOW_COPY_AND_ASSIGN(ProxyImageData);
+};

[sadrul, 2014/09/22 16:13:32]

Any chance this can be consolidated with
NavigationEntryScreenshotManager::ScreenshotData?

[Mr4D (OOO till 08-26), 2014/09/23 19:07:34]

I looked into that at the beginning, but there are several special cases of
handling that I thought it would not be worth while to pursue it (not even
mentioning data structures involved (e.g. NavigationEntryImpl, ScreenshotData)).

If you feel however strongly I will create a more general read-back API (which
is possibly dangerous since it might get called more frequently then we want it
to get called). 

+
 ContentProxy::ContentProxy(views::WebView* web_view, Activity* activity)
     : web_view_(web_view),
-      window_(activity->GetWindow()),
-      background_color_(
-          activity->GetActivityViewModel()->GetRepresentativeColor()),
-      content_loaded_(true) {
+      content_visible_(true),
+      content_loaded_(true),
+      proxy_content_to_image_factory_(this) {
+  // Note: The content will be hidden once the image got created.
   CreateProxyContent();
-  HideOriginalContent();
 }
 
 ContentProxy::~ContentProxy() {
   // If we still have a connection to the original Activity, we make it visible
   // again.
   ShowOriginalContent();
-  // At this point we should either have no view - or the view should not be
-  // shown by its parent anymore.
-  DCHECK(!proxy_content_.get() || !proxy_content_->parent());
-  proxy_content_.reset();
 }
 
 void ContentProxy::ContentWillUnload() {
   content_loaded_ = false;
 }
 
-gfx::ImageSkia ContentProxy::GetContentImage() {
-  // For the time being we keep this here and return an empty image.
-  return image_;
+scoped_ptr<gfx::Image> ContentProxy::GetContentImage() {
+  // While we compress to PNG, we use the original read back.
+  if (image_.IsEmpty())

[oshima, 2014/09/22 16:10:28]

shouldn't this be !image_.IsEmpty(), or
maybe checking png_data is better/ (if both are empty, it will return empty
image)

[Mr4D (OOO till 08-26), 2014/09/23 19:07:34]

Done.

+    return scoped_ptr<gfx::Image>(new gfx::Image(image_));
+
+  // Otherwise we convert the PNG.
+  std::vector<gfx::ImagePNGRep> image_reps;
+  image_reps.push_back(gfx::ImagePNGRep(png_data_, 2.0f));

[oshima, 2014/09/22 16:10:28]

why this is using 2x scale factor?

[Mr4D (OOO till 08-26), 2014/09/23 19:07:33]

I wasn't able to test this yet, but according to the comment it states "and the
scale factor which it was intended for". I also looked at the code and the scale
factor was not used for the decode. We are half size and I know that many image
formats have an indication on the size which helps upon reconstruction. I was
under the impression that this is it (without being able to test this yet). But
I have set it to 1.0 and I will revisit when we have something running. In the
end we want to load it into a layer and we transform simple the layer.

[oshima, 2014/09/24 00:40:00]

The thumbnail should be "unscaled" image (that is, it should simply scaled to
the given pixel size)

The scale should be 0.0f which should indicates that it's unscaled. Let me know
if it didn't work.

[Mr4D (OOO till 08-26), 2014/09/24 16:18:25]

Done.

I tested and the result seems to work. Note: I find a scale factor of 0 rather
odd + I checked unit tests and other places and have seen 1.0 everywhere but not
0.0, but it works ;).

+  return scoped_ptr<gfx::Image>(new gfx::Image(image_reps));
 }
 
 void ContentProxy::EvictContent() {
-  HideProxyContent();
-  CreateSolidProxyContent();
-  ShowProxyContent();
+  image_ = gfx::Image();
+  png_data_->Release();
 }
 
-void ContentProxy::Reparent(aura::Window* new_parent_window) {
-  if (new_parent_window == window_)
-    return;
-
+void ContentProxy::ContentGetsDestroyed() {
   // Since we are breaking now the connection to the old content, we make the
   // content visible again before we continue.
   // Note: Since the owning window is invisible, it does not matter that we
   // make the web content visible if the window gets destroyed shortly after.
   ShowOriginalContent();
 
-  // Transfer the |proxy_content_| to the passed window.
-  window_ = new_parent_window;
   web_view_ = NULL;
-
-  // Move the view to the new window and show it there.
-  HideOriginalContent();
 }
 
 void ContentProxy::ShowOriginalContent() {
-  // Hide our content.
-  HideProxyContent();
-
-  if (web_view_) {
+  if (web_view_ && !content_visible_) {

[oshima, 2014/09/22 16:10:28]

don't you have to change content_visible_? 

Rather than having another flag, is it possible to use the web_view's visibility
instead?

[Mr4D (OOO till 08-26), 2014/09/23 19:07:34]

Yes, it's better to set it.

I didn't want to use the web_view's visibility since someone else might change
that as well and we have changed more things than only the visibility of the
web_view.

     // Show the original |web_view_| again.
     web_view_->SetFastResize(false);
     // If the content is loaded, we ask it to relayout itself since the
@@ -85,7 +117,7 @@ void ContentProxy::ShowOriginalContent() {
 }
 
 void ContentProxy::HideOriginalContent() {
-  if (web_view_) {
+  if (web_view_ && content_visible_) {
     // Hide the |web_view_|.
     // TODO(skuhne): We might consider removing the view from the window while
     // it's hidden - it should work the same way as show/hide and does not have
@@ -94,39 +126,55 @@ void ContentProxy::HideOriginalContent() {
     web_view_->SetVisible(false);
     // Don't allow the content to get resized with window size changes.
     web_view_->SetFastResize(true);

[oshima, 2014/09/22 18:26:05]

We may not want to resize at all when hidden? It may become the soruce of junk
when screen rotates.

[Mr4D (OOO till 08-26), 2014/09/23 19:07:33]

An excellent idea. I was thinking of maybe doing what was suggested in line 122.
There is a long reasoning for this which I can explain later. I added more to
the comment in 122.

+    content_visible_ = false;
   }
-
-  // Show our replacement content.
-  ShowProxyContent();
 }
 
 void ContentProxy::CreateProxyContent() {
-  // For the time being we create only a solid color here.
-  // TODO(skuhne): Replace with copying the drawn content into |proxy_content_|
-  // instead.
-  CreateSolidProxyContent();
-}
+  // Unit tests might not have a |web_view_|.
+  if (!web_view_)
+    return;
 
-void ContentProxy::CreateSolidProxyContent() {
-  proxy_content_.reset(new views::View());
-  proxy_content_->set_owned_by_client();
-  proxy_content_->set_background(
-      views::Background::CreateSolidBackground(background_color_));
+  content::RenderViewHost* host =
+      web_view_->GetWebContents()->GetRenderViewHost();
+  DCHECK(host && host->GetView());
+  gfx::Size source = host->GetView()->GetViewBounds().size();
+  gfx::Size target = gfx::Size(source.width() / 2, source.height() / 2);
+  host->CopyFromBackingStore(
+      gfx::Rect(),
+      target,
+      base::Bind(&ContentProxy::ContentReadBackAsImage,
+                 proxy_content_to_image_factory_.GetWeakPtr()),

[sadrul, 2014/09/22 16:13:32]

InvalidateWeakPtrs() first, so that we ignore an earlier call to getting the
screenshot that is still ongoing. Alternatively, early out from here is there
are live weak-ptrs, which indicates that we are in the process of encoding an
earlier version of the content, and so we don't want to do this again. (I would
go for the first option)

[Mr4D (OOO till 08-26), 2014/09/23 19:07:34]

This function gets only called once through the constructor. There will be no
second call (ever - makes simply no sense). However I see your point and added a
DCHECK and variable to make sure that it does not happen in a code refactor.

Note: This object gets only created when an Activity becomes invisible and it
gets destroyed when it becomes visible again. 

+      kAlpha_8_SkColorType);
 }
 
-void ContentProxy::ShowProxyContent() {
-  views::Widget* widget = views::Widget::GetWidgetForNativeWindow(window_);
-  DCHECK(widget);
-  views::View* client_view = widget->client_view();
-  // Place the view in front of all others.
-  client_view->AddChildView(proxy_content_.get());
-  proxy_content_->SetSize(client_view->bounds().size());
+void ContentProxy::ContentReadBackAsImage(bool success,
+                                          const SkBitmap& bitmap) {
+  // Now we can hide the content.
+  HideOriginalContent();

[sadrul, 2014/09/22 16:13:32]

Should we hide only if the PNG encoding works? (i.e. in
OnContentImageEncodeComplete() instead)

[Mr4D (OOO till 08-26), 2014/09/23 19:07:34]

I chose this location since I think its the best location because...

If something goes wrong (or did already go wrong with the read back) we end up
with a blank page which will be visible immediately and makes sure that we do
not hold on to resources unnecessarily. (I am afraid that our current way of
turning windows invisible might cause read back problems). Furthermore I think
that the PNG compress can only fail when we are running out of memory - and by
turning the view invisible rather sooner then later we can avoid that situation
;).

+
+  if (!success || bitmap.empty() || bitmap.isNull())
+    return;
+
+  // While we are encoding the image, we keep the current image as reference
+  // to have something for the overview mode to grab. Once we have the encoded
+  // PNG, we will get rid of this.
+  image_ = gfx::Image::CreateFrom1xBitmap(bitmap);
+
+  scoped_refptr<ProxyImageData> png_image = new ProxyImageData();
+  png_image->EncodeImage(
+      bitmap,
+      base::Bind(&ContentProxy::OnContentImageEncodeComplete,
+                 proxy_content_to_image_factory_.GetWeakPtr(),
+                 png_image));
 }
 
-void ContentProxy::HideProxyContent() {
-  views::Widget* widget = views::Widget::GetWidgetForNativeWindow(window_);
-  views::View* client_view = widget->client_view();
-  client_view->RemoveChildView(proxy_content_.get());
+void ContentProxy::OnContentImageEncodeComplete(
+    scoped_refptr<ProxyImageData> image) {
+  png_data_ = image->data();
+
+  // From now on we decode the image as needed to save memory.
+  image_ = gfx::Image();

[sadrul, 2014/09/22 16:13:33]

Call image_ something else, to clarify that it is the un-encoded image (e.g.
raw_image_).

[Mr4D (OOO till 08-26), 2014/09/23 19:07:33]

Done.

 }
 
 }  // namespace athena