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

Allow a read-back request to wait for a first frame
if it was invoked while no frame was received yet.
This patch takes care of adding any readback request 
to a queue and ensure that this gets processed, once 
the frame arrives.This also takes care of discarding
all the readback requests if the delegated layer is 
destroyed or view is hidden, before a frame arrives.

BUG=376769
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290802

[sivag, 2014-07-25 10:57:44]

ptal..

[no sievers, 2014-07-28 17:07:07]

https://codereview.chromium.org/415773007/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/415773007/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:299: void
RenderWidgetHostViewAndroid::ProcessPendingReadbackRequests() {
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.

https://codereview.chromium.org/415773007/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:309:
readbacks_waiting_for_frame_.push(readback_request);
nit: don't need a method for this one line of code.

https://codereview.chromium.org/415773007/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:327: //
TODO(sikugu): Do we need to have a timeout for a readback here?
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().

https://codereview.chromium.org/415773007/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:431:
ProcessPendingReadbackRequests();
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?

https://codereview.chromium.org/415773007/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:860:
ProcessPendingReadbackRequests();
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.)

https://codereview.chromium.org/415773007/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:991: }
Can you move this to the end of InternalSwapCompositorFrame()?

https://codereview.chromium.org/415773007/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.h (right):

https://codereview.chromium.org/415773007/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.h:75: gfx::Rect
GetCaptureRect() { return src_subrect_; }
nit: return a reference for the rect

https://codereview.chromium.org/415773007/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.h:345: // Add the
ReadbackRequest to the queue and wait for frame arraival.
not: typo in 'arraival'

https://codereview.chromium.org/415773007/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.h:346: void
WaitForValidFrameArrival(const ReadbackRequest& readback_request);
nit: How about a more specific name? Something like 'DeferReadback' or so.

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

https://codereview.chromium.org/415773007/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/415773007/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:431:
ProcessPendingReadbackRequests();
On 2014/07/28 17:07:06, sievers wrote:
> 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?

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]

ptal..

https://codereview.chromium.org/415773007/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/415773007/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:299: void
RenderWidgetHostViewAndroid::ProcessPendingReadbackRequests() {
On 2014/07/28 17:07:06, sievers wrote:
> 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.

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.

https://codereview.chromium.org/415773007/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:309:
readbacks_waiting_for_frame_.push(readback_request);
On 2014/07/28 17:07:06, sievers wrote:
> nit: don't need a method for this one line of code.

Done.

https://codereview.chromium.org/415773007/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:327: //
TODO(sikugu): Do we need to have a timeout for a readback here?
On 2014/07/28 17:07:06, sievers wrote:
> 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().

Done.

https://codereview.chromium.org/415773007/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:431:
ProcessPendingReadbackRequests();
On 2014/07/28 17:07:06, sievers wrote:
> 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?

Done.

https://codereview.chromium.org/415773007/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:431:
ProcessPendingReadbackRequests();
On 2014/07/28 17:07:06, sievers wrote:
> 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?

Done.

https://codereview.chromium.org/415773007/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:860:
ProcessPendingReadbackRequests();
On 2014/07/28 17:07:06, sievers wrote:
> 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.)

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

Relevant DCHECKS added, to counter any unprocessed readbbacks.

https://codereview.chromium.org/415773007/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_android.cc:991: }
On 2014/07/28 17:07:06, sievers wrote:
> Can you move this to the end of InternalSwapCompositorFrame()?

Done.

[sivag, 2014-08-19 07:56:05]

@sievers , ptal

[no sievers, 2014-08-19 17:47:08]

lgtm

https://codereview.chromium.org/415773007/diff/140001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/415773007/diff/140001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:441: // need to
be addressed here.
nit: I'd say that we don't know if we will ever get a frame if we are hiding the
renderer, so we need to cancel all requests.

https://codereview.chromium.org/415773007/diff/140001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1004: if
(!readbacks_waiting_for_frame_.empty() && HasValidFrame()) {
nit: Isn't HasValidFrame() always true at this location?

https://codereview.chromium.org/415773007/diff/140001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.h (right):

https://codereview.chromium.org/415773007/diff/140001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.h:71: gfx::Rect
GetCaptureRect() { return src_subrect_; }
nit: const gfx::Rect&

[sivag, 2014-08-20 07:10:54]

https://codereview.chromium.org/415773007/diff/140001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/415773007/diff/140001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:441: // need to
be addressed here.
On 2014/08/19 17:47:07, sievers wrote:
> nit: I'd say that we don't know if we will ever get a frame if we are hiding
the
> renderer, so we need to cancel all requests.

Done.

https://codereview.chromium.org/415773007/diff/140001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:1004: if
(!readbacks_waiting_for_frame_.empty() && HasValidFrame()) {
On 2014/08/19 17:47:07, sievers wrote:
> nit: Isn't HasValidFrame() always true at this location?

Done.

https://codereview.chromium.org/415773007/diff/140001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.h (right):

https://codereview.chromium.org/415773007/diff/140001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.h:71: gfx::Rect
GetCaptureRect() { return src_subrect_; }
On 2014/08/19 17:47:08, sievers wrote:
> nit: const gfx::Rect&

Done.

[sivag, 2014-08-20 08:55:45]

The CQ bit was checked by siva.gunturi@samsung.com

[commit-bot: I haz the power, 2014-08-20 08:56:57]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/siva.gunturi@samsung.com/415773007/160001

[commit-bot: I haz the power, 2014-08-20 10:10:18]

Committed patchset #9 (160001) as 290802