Android:Allow a read-back request to wait for a first frame

content/browser/renderer_host/render_widget_host_view_android.cc

Index: content/browser/renderer_host/render_widget_host_view_android.cc
diff --git a/content/browser/renderer_host/render_widget_host_view_android.cc b/content/browser/renderer_host/render_widget_host_view_android.cc
index a66dffe37e5e3aaa810060cb715a44b876bd5439..3fedae522a94ea7e4d551106bbf9a981bf785a14 100644
--- a/content/browser/renderer_host/render_widget_host_view_android.cc
+++ b/content/browser/renderer_host/render_widget_host_view_android.cc
@@ -212,6 +212,7 @@ RenderWidgetHostViewAndroid::~RenderWidgetHostViewAndroid() {
   ImageTransportFactoryAndroid::RemoveObserver(this);
   SetContentViewCore(NULL);
   DCHECK(ack_callbacks_.empty());
+  DCHECK(readbacks_waiting_for_frame_.empty());
   if (resource_collection_.get())
     resource_collection_->SetClient(NULL);
 }
@@ -295,6 +296,19 @@ void RenderWidgetHostViewAndroid::SetBounds(const gfx::Rect& rect) {
   SetSize(rect.size());
 }
 
+void RenderWidgetHostViewAndroid::ProcessPendingReadbackRequests() {

[no sievers, 2014/07/28 17:07:06]

Ok, I see, so this doesn't actually do the readback. Then it should have a
better name too.
Something like Cancel/AbortPendingReadbacks or FailPendingReadbacks.

Or even better: How about getting rid of this method and letting
~ReadbackRequest handle this?
You can add something like ReadbackRequest::TakeCallback() to handle the case
where you actually do the readback and let GetScaledContentBitmap() handle
running the callback.

[sivag, 2014/08/01 10:10:20]

1. changed ProcessPendingReadbackRequests ->AbortPendingReadBackRequests
2. because the request queue is maintained in RWHVA  and we dont want to always
send the callbacks with false in destructor, some times it gets processed via
GetScaledContentBitmap(here copyFromcompositingsurface will return the
callback), except there is some contentt loss or we are hiding.

+  while (!readbacks_waiting_for_frame_.empty()) {
+    ReadbackRequest& readback_request = readbacks_waiting_for_frame_.front();
+    readback_request.GetResultCallback().Run(false, SkBitmap());
+    readbacks_waiting_for_frame_.pop();
+  }
+}
+
+void RenderWidgetHostViewAndroid::WaitForValidFrameArrival(
+    const ReadbackRequest& readback_request) {
+  readbacks_waiting_for_frame_.push(readback_request);

[no sievers, 2014/07/28 17:07:06]

nit: don't need a method for this one line of code.

[sivag, 2014/08/01 10:10:20]

Done.

+}
+
 void RenderWidgetHostViewAndroid::GetScaledContentBitmap(
     float scale,
     SkColorType color_type,
@@ -305,9 +319,12 @@ void RenderWidgetHostViewAndroid::GetScaledContentBitmap(
     return;
   }
   if (!IsSurfaceAvailableForCopy()) {
-    // TODO(Sikugu): allow a read-back request to wait for a first frame if it
-    // was invoked while no frame was received yet
-    result_callback.Run(false, SkBitmap());
+    // The view is visible, probably the frame has not yet arrived.
+    // Just add the ReadbackRequest to queue and wait for frame arrival
+    // to get this request processed.
+    WaitForValidFrameArrival(
+        ReadbackRequest(scale, color_type, src_subrect, result_callback));
+    // TODO(sikugu): Do we need to have a timeout for a readback here?

[no sievers, 2014/07/28 17:07:06]

I don't *think* we need a timeout. We know we are visible and if no frame
arrives, it should be a bug.
Except see comment in Hide().

[sivag, 2014/08/01 10:10:20]

Done.

     return;
   }
 
@@ -334,7 +351,7 @@ bool RenderWidgetHostViewAndroid::HasValidFrame() const {
 
   if (texture_size_in_layer_.IsEmpty())
     return false;
-
+  // This tell us whether a valid frame has arrived or not.
   if (!frame_evictor_->HasFrame())
     return false;
 
@@ -409,6 +426,9 @@ void RenderWidgetHostViewAndroid::Hide() {
     layer_->SetHideLayerAndSubtree(true);
 
   frame_evictor_->SetVisible(false);
+  // We are hiding a renderer and any readback requests pending on frame arrival
+  // need to be addressed here.
+  ProcessPendingReadbackRequests();

[no sievers, 2014/07/28 17:07:06]

It would be nice if we could allow deferring the readback if the surface was
locked.
But then I'm guessing it's not guaranteed that you will ever see a frame if you
quickly show and hide the view?

[no sievers, 2014/07/28 17:08:33]

Scrap that comment. We don't allow locking the surface if there is no frame.
So the app already has to deal with this and differentiate anyways.

[sivag, 2014/08/01 10:10:20]

Done.

[sivag, 2014/08/01 10:10:20]

Done.

   WasHidden();
 }
 
@@ -837,6 +857,7 @@ void RenderWidgetHostViewAndroid::DestroyDelegatedContent() {
   RemoveLayers();
   frame_provider_ = NULL;
   layer_ = NULL;
+  ProcessPendingReadbackRequests();

[no sievers, 2014/07/28 17:07:06]

Which case is this handling?
I'm not sure it's always right, or even if we should cancel the readbacks here
at all. Currently there are 4 call sites:

1) EvictDelegatedFrame(): Doesn't seem like this should happen. If you had a
frame you did the readback rightaway. If you don't have a frame, I think
EvictDelegatedFrame() should not get called.

2) OnLostResources(): not sure; it shouldn't stop drawing (or we have other
problems) and we can complete the readback when the new context generates the
first frame

3) SwapDelegatedFrame() line 877: I think it's same as 2). Should only happen
when a context was lost.

4) SwapDelegatedFrame() line 891: That's an interesting one. I don't know when
that happens. Same issue though, if we never get a new good frame very soon,
then something is wrong. (Unless we went hidden.)

[sivag, 2014/08/01 10:10:20]

If any of above functions gets triggered, they will call
DestroyDelegatedContent  where we abortpendingreadbacks.

Relevant DCHECKS added, to counter any unprocessed readbbacks.

 }
 
 void RenderWidgetHostViewAndroid::SwapDelegatedFrame(
@@ -959,6 +980,16 @@ void RenderWidgetHostViewAndroid::OnSwapCompositorFrame(
     uint32 output_surface_id,
     scoped_ptr<cc::CompositorFrame> frame) {
   InternalSwapCompositorFrame(output_surface_id, frame.Pass());
+  if (!readbacks_waiting_for_frame_.empty() && HasValidFrame()) {
+    while (!readbacks_waiting_for_frame_.empty()) {
+      ReadbackRequest& readback_request = readbacks_waiting_for_frame_.front();
+      GetScaledContentBitmap(readback_request.GetScale(),
+                             readback_request.GetColorFormat(),
+                             readback_request.GetCaptureRect(),
+                             readback_request.GetResultCallback());
+      readbacks_waiting_for_frame_.pop();
+    }

[no sievers, 2014/07/28 17:07:06]

Can you move this to the end of InternalSwapCompositorFrame()?

[sivag, 2014/08/01 10:10:20]

Done.

+  }
 }
 
 void RenderWidgetHostViewAndroid::RetainFrame(