sync compositor: Add output_surface_id

sync compositor: Add output_surface_id plumbing

The output_surface_id is unique for a particular renderer compositor
instance. It is used to avoid returning resources from lost context to
compositor with new context.

Plumb output_surface_id from SynchronousOutputSurface all the way
to android_webview::HardwareRenderer. To avoid having to send down
a map<id, resource list>, a simpler guarantee is to assume the
output_surface_id is increasing, so only need to return resources
of with highest output surface id and drop resources other resources.

BUG=592744
Committed: https://crrev.com/f4e57485631dc4c3beaa87de398975cab87fdc32
Cr-Commit-Position: refs/heads/master@{#382636}

[boliu, 2016-03-08 03:34:31]

Description was changed from

==========
sync compositor: Add output_surface_id

WIP

movable frame

BUG=592744
==========

to

==========
sync compositor: Add output_surface_id plumbing

The output_surface_id is unique for a particular renderer compositor
instance. It is used to avoid returning resources from lost context to
compositor with new context.

Plumb output_surface_id from SynchronousOutputSurface all the way
to android_webview::HardwareRenderer. To avoid having to send down
a map<id, resource list>, a simpler guarantee is that when the output
surface id changes, drop old resources and/or guarantee resources with
old id are returned before resources with new id.

BUG=592744
==========

[boliu, 2016-03-08 03:46:56]

boliu@chromium.org changed reviewers:
+ hush@chromium.org

[boliu, 2016-03-08 03:46:56]

hush for first pass

[hush (inactive), 2016-03-08 23:55:50]

https://codereview.chromium.org/1769913003/diff/60001/android_webview/browser...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/1769913003/diff/60001/android_webview/browser...
android_webview/browser/hardware_renderer.cc:257: uint32_t output_surface_id) {
A bit weird that we have both unsigned int and uint32_t here. I think we should
use uint32_t for compositor_id too. The length of unsigned int is platform
dependent.

https://codereview.chromium.org/1769913003/diff/60001/android_webview/browser...
File android_webview/browser/test/rendering_test.cc (right):

https://codereview.chromium.org/1769913003/diff/60001/android_webview/browser...
android_webview/browser/test/rendering_test.cc:80:
compositor_->SetHardwareFrame(1u, std::move(compositor_frame));
can you write a rendering test for context loss case?

https://codereview.chromium.org/1769913003/diff/60001/content/browser/android...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1769913003/diff/60001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:43:
current_output_surface_id_(0u),
Not sure where we get the correct value of current_output_surface_id_. Is 0 a
guaranteed invalid id?
Also it is only set during ReturnResources. And we keep using 0u as surface id
in PopulateCommonParams until resources are returned?

We should assign the current_output_surface_id_ during DrawHW at least.

https://codereview.chromium.org/1769913003/diff/60001/content/common/android/...
File content/common/android/sync_compositor_messages.h (right):

https://codereview.chromium.org/1769913003/diff/60001/content/common/android/...
content/common/android/sync_compositor_messages.h:162: uint32_t /*
output_surface_id */,
where is the renderer change that writes output surface id to the reply message?

[hush (inactive), 2016-03-08 23:56:56]

https://codereview.chromium.org/1769913003/diff/60001/content/common/android/...
File content/common/android/sync_compositor_messages.h (right):

https://codereview.chromium.org/1769913003/diff/60001/content/common/android/...
content/common/android/sync_compositor_messages.h:162: uint32_t /*
output_surface_id */,
On 2016/03/08 23:55:49, hush wrote:
> where is the renderer change that writes output surface id to the reply
message?

nevermind that. Just saw it.

[boliu, 2016-03-09 00:29:59]

no new patch set

https://codereview.chromium.org/1769913003/diff/60001/android_webview/browser...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/1769913003/diff/60001/android_webview/browser...
android_webview/browser/hardware_renderer.cc:257: uint32_t output_surface_id) {
On 2016/03/08 23:55:49, hush wrote:
> A bit weird that we have both unsigned int and uint32_t here. I think we
should
> use uint32_t for compositor_id too. The length of unsigned int is platform
> dependent. 

Compositor_id doesn't leave the browser process, so doesn't really need to be
platform independent.

But can change it to uint32_t first in separate CL to remove weirdness.

https://codereview.chromium.org/1769913003/diff/60001/android_webview/browser...
File android_webview/browser/test/rendering_test.cc (right):

https://codereview.chromium.org/1769913003/diff/60001/android_webview/browser...
android_webview/browser/test/rendering_test.cc:80:
compositor_->SetHardwareFrame(1u, std::move(compositor_frame));
On 2016/03/08 23:55:49, hush wrote:
> can you write a rendering test for context loss case?

Tests are coming.. Need to ensure that

1) no old resources are returned *after new resources*
2) all new resources are returned

https://codereview.chromium.org/1769913003/diff/60001/content/browser/android...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1769913003/diff/60001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:43:
current_output_surface_id_(0u),
On 2016/03/08 23:55:49, hush wrote:
> Not sure where we get the correct value of current_output_surface_id_. Is 0 a
> guaranteed invalid id?

Yes

> Also it is only set during ReturnResources. And we keep using 0u as surface id
> in PopulateCommonParams until resources are returned?

Yeah. The resource list is guaranteed to be empty before the first
ReturnResources. So having an invalid output surface id doesn't really matter.

> 
> We should assign the current_output_surface_id_ during DrawHW at least.

no! the output_surface_id should refers to the resources that are in the
returned resource array, not what's in the current/new frame

hmm, maybe I should rename this var or add a comment

[hush (inactive), 2016-03-09 02:36:17]

https://codereview.chromium.org/1769913003/diff/60001/content/browser/android...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1769913003/diff/60001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:43:
current_output_surface_id_(0u),
On 2016/03/09 00:29:59, boliu wrote:
> On 2016/03/08 23:55:49, hush wrote:
> > Not sure where we get the correct value of current_output_surface_id_. Is 0
a
> > guaranteed invalid id?
> 
> Yes
> 
> > Also it is only set during ReturnResources. And we keep using 0u as surface
id
> > in PopulateCommonParams until resources are returned?
> 
> Yeah. The resource list is guaranteed to be empty before the first
> ReturnResources. So having an invalid output surface id doesn't really matter.
> 
> > 
> > We should assign the current_output_surface_id_ during DrawHW at least.
> 
> no! the output_surface_id should refers to the resources that are in the
> returned resource array, not what's in the current/new frame
> 
> hmm, maybe I should rename this var or add a comment

yeah. Renaming the variable name to indicate this would be good.

https://codereview.chromium.org/1769913003/diff/60001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1769913003/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:767: uint32_t output_surface_id =
next_output_surface_id_++;
We can probably pull this line between 737 and 738.
Also, can we use ++next_output_surface_id_ instead? This way the valid id will
start with 1.

[boliu, 2016-03-09 02:39:44]

https://codereview.chromium.org/1769913003/diff/60001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1769913003/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:767: uint32_t output_surface_id =
next_output_surface_id_++;
On 2016/03/09 02:36:17, hush wrote:
> We can probably pull this line between 737 and 738.

That can leave gaps if context creation fails. Not sure if that's ok or not.

> Also, can we use ++next_output_surface_id_ instead? This way the valid id will
> start with 1.

Hmm... so 0 is a valid id, and there is no invalid id. I think this still works
out ok?

Don't want to be changing chrome behavior if I don't have to.

[hush (inactive), 2016-03-09 20:21:49]

https://codereview.chromium.org/1769913003/diff/60001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1769913003/diff/60001/content/renderer/render...
content/renderer/render_widget.cc:767: uint32_t output_surface_id =
next_output_surface_id_++;
On 2016/03/09 02:39:44, boliu wrote:
> On 2016/03/09 02:36:17, hush wrote:
> > We can probably pull this line between 737 and 738.
> 
> That can leave gaps if context creation fails. Not sure if that's ok or not.
> 
> > Also, can we use ++next_output_surface_id_ instead? This way the valid id
will
> > start with 1.
> 
> Hmm... so 0 is a valid id, and there is no invalid id. I think this still
works
> out ok?
> 
> Don't want to be changing chrome behavior if I don't have to.

Okay. Let's not change chrome behavior.

[boliu, 2016-03-10 15:37:49]

unit test up

[hush (inactive), 2016-03-10 22:34:20]

https://codereview.chromium.org/1769913003/diff/120001/android_webview/browse...
File android_webview/browser/browser_view_renderer_unittest.cc (right):

https://codereview.chromium.org/1769913003/diff/120001/android_webview/browse...
android_webview/browser/browser_view_renderer_unittest.cc:257: // Make sure
resources for the last output surface are returned.
why don't you also test that the resources belong to the first output surface
are returned as well?

[boliu, 2016-03-10 22:49:30]

https://codereview.chromium.org/1769913003/diff/120001/android_webview/browse...
File android_webview/browser/browser_view_renderer_unittest.cc (right):

https://codereview.chromium.org/1769913003/diff/120001/android_webview/browse...
android_webview/browser/browser_view_renderer_unittest.cc:257: // Make sure
resources for the last output surface are returned.
On 2016/03/10 22:34:20, hush wrote:
> why don't you also test that the resources belong to the first output surface
> are returned as well?

because we intentionally drop them at places, eg:
https://codereview.chromium.org/1769913003/diff/120001/android_webview/browse...

[hush (inactive), 2016-03-10 23:01:45]

ok. android_webview lgtm

[boliu, 2016-03-11 22:24:32]

Description was changed from

==========
sync compositor: Add output_surface_id plumbing

The output_surface_id is unique for a particular renderer compositor
instance. It is used to avoid returning resources from lost context to
compositor with new context.

Plumb output_surface_id from SynchronousOutputSurface all the way
to android_webview::HardwareRenderer. To avoid having to send down
a map<id, resource list>, a simpler guarantee is that when the output
surface id changes, drop old resources and/or guarantee resources with
old id are returned before resources with new id.

BUG=592744
==========

to

==========
sync compositor: Add output_surface_id plumbing

The output_surface_id is unique for a particular renderer compositor
instance. It is used to avoid returning resources from lost context to
compositor with new context.

Plumb output_surface_id from SynchronousOutputSurface all the way
to android_webview::HardwareRenderer. To avoid having to send down
a map<id, resource list>, a simpler guarantee is that when the output
surface id changes, drop old resources and guarantee resources with
old id are returned before resources with new id.

BUG=592744
==========

[boliu, 2016-03-11 22:25:16]

boliu@chromium.org changed reviewers:
+ sievers@chromium.org

[boliu, 2016-03-11 22:25:17]

+sievers for all of content

Fwiw follow up for actually handling context loss here:
https://codereview.chromium.org/1773873002/

[hush (inactive), 2016-03-12 01:28:10]

compilation error here:
../../android_webview/browser/shared_renderer_state.h:37:3: error:
[chromium-style] Complex class/struct needs an explicit out-of-line destructor.
  struct ReturnedResources {
  ^
1 error generated.

[boliu, 2016-03-12 01:28:55]

On 2016/03/12 01:28:10, hush wrote:
> compilation error here:
> ../../android_webview/browser/shared_renderer_state.h:37:3: error:
> [chromium-style] Complex class/struct needs an explicit out-of-line
destructor.
>   struct ReturnedResources {
>   ^
> 1 error generated.

Yeah yeah. Turns out gn doesn't turn on some clang plugins or something..

[boliu, 2016-03-14 16:59:56]

I thought of a case where android_webview code returns frame resources out of
order, causing resource leaks:

UI: produce OS_id 0 frame 0
RT: OS_id 0 frame 0 passed all the way into HardwareRenderer
UI: context loss, produce OS_id 1 frame 0, but no kModeDraw, so this frame is
stuck in SharedRendererState
UI: produce OS_id 1 frame 1, frame id 0 resources are returned directly
RT: OS_id 1 frame 1 passed all the way into SharedRendererState
UI: OS_id 0 frame 0 resources returned asynchronously

At this point, the OS_id are mixed, ie returned resources for OS 1, then 0,
which will make content code think that the current output surface id is 0. And
if timing is correct, the returned resources for OS 1 will be dropped.

So this sequence depends on a chromium corner case (context loss) happening the
same time as an android framework corner case (no kModeSync), and only really
leads to a leak. Wonder if I should actually spend much time fixing this..

[boliu, 2016-03-14 18:04:44]

Description was changed from

==========
sync compositor: Add output_surface_id plumbing

The output_surface_id is unique for a particular renderer compositor
instance. It is used to avoid returning resources from lost context to
compositor with new context.

Plumb output_surface_id from SynchronousOutputSurface all the way
to android_webview::HardwareRenderer. To avoid having to send down
a map<id, resource list>, a simpler guarantee is that when the output
surface id changes, drop old resources and guarantee resources with
old id are returned before resources with new id.

BUG=592744
==========

to

==========
sync compositor: Add output_surface_id plumbing

The output_surface_id is unique for a particular renderer compositor
instance. It is used to avoid returning resources from lost context to
compositor with new context.

Plumb output_surface_id from SynchronousOutputSurface all the way
to android_webview::HardwareRenderer. To avoid having to send down
a map<id, resource list>, a simpler guarantee is to assume the
output_surface_id is increasing, so only need to return resources
of with highest output surface id and drop resources other resources. 

BUG=592744
==========

[boliu, 2016-03-14 18:06:37]

On 2016/03/14 16:59:56, boliu wrote:
> I thought of a case where android_webview code returns frame resources out of
> order, causing resource leaks:
> 
> UI: produce OS_id 0 frame 0
> RT: OS_id 0 frame 0 passed all the way into HardwareRenderer
> UI: context loss, produce OS_id 1 frame 0, but no kModeDraw, so this frame is
> stuck in SharedRendererState
> UI: produce OS_id 1 frame 1, frame id 0 resources are returned directly
> RT: OS_id 1 frame 1 passed all the way into SharedRendererState
> UI: OS_id 0 frame 0 resources returned asynchronously
> 
> At this point, the OS_id are mixed, ie returned resources for OS 1, then 0,
> which will make content code think that the current output surface id is 0.
And
> if timing is correct, the returned resources for OS 1 will be dropped.
> 
> So this sequence depends on a chromium corner case (context loss) happening
the
> same time as an android framework corner case (no kModeSync), and only really
> leads to a leak. Wonder if I should actually spend much time fixing this..

Decided to fix that by assuming the output surface id is increasing, and thus
can distinguish old resources from new resources

[hush (inactive), 2016-03-14 21:29:53]

https://codereview.chromium.org/1769913003/diff/220001/content/browser/androi...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1769913003/diff/220001/content/browser/androi...
content/browser/android/synchronous_compositor_host.cc:277: 0x80000000) {
what does this mean? (especially with the constaint 0x80000000)
Is it the same as "output_surface_id <
output_surface_id_for_returned_resources_"?

[hush (inactive), 2016-03-14 21:35:07]

https://codereview.chromium.org/1769913003/diff/220001/content/browser/androi...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1769913003/diff/220001/content/browser/androi...
content/browser/android/synchronous_compositor_host.cc:277: 0x80000000) {
On 2016/03/14 21:29:53, hush wrote:
> what does this mean? (especially with the constaint 0x80000000)
> Is it the same as "output_surface_id <
> output_surface_id_for_returned_resources_"?

s/constraint/constant

[boliu, 2016-03-14 22:50:49]

https://codereview.chromium.org/1769913003/diff/220001/content/browser/androi...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1769913003/diff/220001/content/browser/androi...
content/browser/android/synchronous_compositor_host.cc:277: 0x80000000) {
On 2016/03/14 21:35:07, hush wrote:
> On 2016/03/14 21:29:53, hush wrote:
> > what does this mean? (especially with the constaint 0x80000000)
> > Is it the same as "output_surface_id <
> > output_surface_id_for_returned_resources_"?

0x80000000 is the "midpoint" for uint32_t.

You can think of this statement as (output_surface_id_for_returned_resources_ >=
output_surface_id) except this statement correctly supports wrapping.

But as the failing unit test shows, it's not supposed to cover the == case.
Fixed that by moving the check into the if statement below.

> 
> s/constraint/constant

[hush (inactive), 2016-03-15 00:05:59]

https://codereview.chromium.org/1769913003/diff/240001/android_webview/browse...
File android_webview/browser/shared_renderer_state.cc (right):

https://codereview.chromium.org/1769913003/diff/240001/android_webview/browse...
android_webview/browser/shared_renderer_state.cc:229: if
((returned_resources.output_surface_id - output_surface_id) <
i guess you can also do:
if (output_surface_id - returned_sources.output_surface_id > 0x80000000)

But anyway, a comment here would be great. The effort to address wrapping case
is also just best effort, I think.

[no sievers, 2016-03-18 20:03:13]

https://codereview.chromium.org/1769913003/diff/260001/content/browser/androi...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1769913003/diff/260001/content/browser/androi...
content/browser/android/synchronous_compositor_host.cc:45:
output_surface_id_for_returned_resources_(0u),
This looks like it might be potentially brittle combined with the logic in
ReturnResources() below if somebody went and changed the first id from the
renderer to be nonzero.

https://codereview.chromium.org/1769913003/diff/260001/content/browser/androi...
content/browser/android/synchronous_compositor_host.cc:282:
output_surface_id_for_returned_resources_ = output_surface_id;
The logic in the if-block here is a bit confusing.

Isn't it easier to just track the output surface id for the last frame we got,
and whenever the id we are trying to return to mismatches here then drop the
resources?

And for the synchronous swap-ack the renderer already deals with it.

https://codereview.chromium.org/1769913003/diff/260001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1769913003/diff/260001/content/renderer/rende...
content/renderer/render_widget.cc:769: uint32_t output_surface_id =
next_output_surface_id_++;
you forgot to remove this line.

[boliu, 2016-03-19 00:18:16]

hush: a_w code should be reviewed again. I removed the 0x80000000 thing from
SharedRendererState as well, and added the guarantee that HardwareRenderer
return resources in output_surface_id order. That unit test came in really handy
for catching mistakes here.

https://codereview.chromium.org/1769913003/diff/260001/content/browser/androi...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1769913003/diff/260001/content/browser/androi...
content/browser/android/synchronous_compositor_host.cc:45:
output_surface_id_for_returned_resources_(0u),
On 2016/03/18 20:03:13, sievers wrote:
> This looks like it might be potentially brittle combined with the logic in
> ReturnResources() below if somebody went and changed the first id from the
> renderer to be nonzero.

Doesn't matter anymore, that the id is set in draw. :)

https://codereview.chromium.org/1769913003/diff/260001/content/browser/androi...
content/browser/android/synchronous_compositor_host.cc:282:
output_surface_id_for_returned_resources_ = output_surface_id;
On 2016/03/18 20:03:13, sievers wrote:
> The logic in the if-block here is a bit confusing.
> 
> Isn't it easier to just track the output surface id for the last frame we got,
> and whenever the id we are trying to return to mismatches here then drop the
> resources?
> 
> And for the synchronous swap-ack the renderer already deals with it.

Hmm, yeah, that works. hush actually suggested that earlier in the review,
before I realized the out of order problem.

Just need to be careful super careful not to label the returned resources with
the wrong id.

https://codereview.chromium.org/1769913003/diff/260001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1769913003/diff/260001/content/renderer/rende...
content/renderer/render_widget.cc:769: uint32_t output_surface_id =
next_output_surface_id_++;
On 2016/03/18 20:03:13, sievers wrote:
> you forgot to remove this line.

Actually the one added above is guarded by !use_software. I moved the new one to
immediately before returns instead.

[no sievers, 2016-03-21 19:56:12]

https://codereview.chromium.org/1769913003/diff/300001/android_webview/browse...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/1769913003/diff/300001/android_webview/browse...
android_webview/browser/hardware_renderer.cc:232:
last_submitted_output_surface_id_);
Regarding using |last_submitted_output_surface_id_| is that really true that
we'll never commit but not draw, or skip a frame or so?

https://codereview.chromium.org/1769913003/diff/300001/content/browser/androi...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1769913003/diff/300001/content/browser/androi...
content/browser/android/synchronous_compositor_host.cc:280: if
(output_surface_id_from_last_draw_ != output_surface_id)
nit: maybe worth a comment saying that in that case the renderer's output
surface has changed in the meantime

[hush (inactive), 2016-03-21 20:43:56]

lgtm

https://codereview.chromium.org/1769913003/diff/300001/android_webview/browse...
File android_webview/browser/hardware_renderer.h (right):

https://codereview.chromium.org/1769913003/diff/300001/android_webview/browse...
android_webview/browser/hardware_renderer.h:54: void
ReturnResourcesToCompositor(const cc::ReturnedResourceArray& resources,
while you are at it, can you turn this function to private? It is only used
internally by this class.

[boliu, 2016-03-21 21:35:49]

https://codereview.chromium.org/1769913003/diff/300001/android_webview/browse...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/1769913003/diff/300001/android_webview/browse...
android_webview/browser/hardware_renderer.cc:232:
last_submitted_output_surface_id_);
On 2016/03/21 19:56:12, sievers wrote:
> Regarding using |last_submitted_output_surface_id_| is that really true that
> we'll never commit but not draw, or skip a frame or so?

It totally is *not* true. CommitFrame can get called twice, and both times
committing new frames, but there is no DrawGL (ie submit) in between them.

That's why this is the submitted id rather than committed id.

Let's say:
Commit id 0 frame 0
Submit id 0 frame 0
Commit id 1 frame 0
Commit id 1 frame 1

At this point, "id 1 frame 0" will be returned from ReturnResourcesInChildFrame.

Submit id 1 frame 1

At this point, "id 0 frame 0" will be dropped here.

https://codereview.chromium.org/1769913003/diff/300001/android_webview/browse...
File android_webview/browser/hardware_renderer.h (right):

https://codereview.chromium.org/1769913003/diff/300001/android_webview/browse...
android_webview/browser/hardware_renderer.h:54: void
ReturnResourcesToCompositor(const cc::ReturnedResourceArray& resources,
On 2016/03/21 20:43:55, hush wrote:
> while you are at it, can you turn this function to private? It is only used
> internally by this class.

Already private?

https://codereview.chromium.org/1769913003/diff/300001/content/browser/androi...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1769913003/diff/300001/content/browser/androi...
content/browser/android/synchronous_compositor_host.cc:280: if
(output_surface_id_from_last_draw_ != output_surface_id)
On 2016/03/21 19:56:12, sievers wrote:
> nit: maybe worth a comment saying that in that case the renderer's output
> surface has changed in the meantime

Done.

[no sievers, 2016-03-21 21:59:55]

https://codereview.chromium.org/1769913003/diff/300001/android_webview/browse...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/1769913003/diff/300001/android_webview/browse...
android_webview/browser/hardware_renderer.cc:232:
last_submitted_output_surface_id_);
On 2016/03/21 21:35:49, boliu wrote:
> On 2016/03/21 19:56:12, sievers wrote:
> > Regarding using |last_submitted_output_surface_id_| is that really true that
> > we'll never commit but not draw, or skip a frame or so?
> 
> It totally is *not* true. CommitFrame can get called twice, and both times
> committing new frames, but there is no DrawGL (ie submit) in between them.
> 
> That's why this is the submitted id rather than committed id.
> 
> Let's say:
> Commit id 0 frame 0
> Submit id 0 frame 0
> Commit id 1 frame 0
> Commit id 1 frame 1
> 
> At this point, "id 1 frame 0" will be returned from
ReturnResourcesInChildFrame.
> 
> Submit id 1 frame 1
> 
> At this point, "id 0 frame 0" will be dropped here.

Ah right, makes sense. But it's a bit hard to follow. Just wondering - would it
make things easier to follow if SharedRendererState would track the surface id
for which it's currently collecting resources?

[boliu, 2016-03-21 22:10:30]

https://codereview.chromium.org/1769913003/diff/300001/android_webview/browse...
File android_webview/browser/hardware_renderer.cc (right):

https://codereview.chromium.org/1769913003/diff/300001/android_webview/browse...
android_webview/browser/hardware_renderer.cc:232:
last_submitted_output_surface_id_);
On 2016/03/21 21:59:54, sievers wrote:
> On 2016/03/21 21:35:49, boliu wrote:
> > On 2016/03/21 19:56:12, sievers wrote:
> > > Regarding using |last_submitted_output_surface_id_| is that really true
that
> > > we'll never commit but not draw, or skip a frame or so?
> > 
> > It totally is *not* true. CommitFrame can get called twice, and both times
> > committing new frames, but there is no DrawGL (ie submit) in between them.
> > 
> > That's why this is the submitted id rather than committed id.
> > 
> > Let's say:
> > Commit id 0 frame 0
> > Submit id 0 frame 0
> > Commit id 1 frame 0
> > Commit id 1 frame 1
> > 
> > At this point, "id 1 frame 0" will be returned from
> ReturnResourcesInChildFrame.
> > 
> > Submit id 1 frame 1
> > 
> > At this point, "id 0 frame 0" will be dropped here.
> 
> Ah right, makes sense. But it's a bit hard to follow. Just wondering - would
it
> make things easier to follow if SharedRendererState would track the surface id
> for which it's currently collecting resources?

Oh I totally tried that first. It's more complex this doing it here imo. So
options I considered:

1) SRS keep a map of maps:
compositor_id -> output_surface_id -> resources
too complex

2) SRS keep a map of compositor_id -> struct {output_surface_id, resources}
This doesn't work because SwapReturnedResourcesOnUI swaps the whole map, so the
output_surface_id info is lost. And fixing that is complex.

3) Add parallel compositor_id -> current output_surface_id map.
Then SRS will need to pay attention to compositor lifetimes, which is ugly.

[no sievers, 2016-03-21 22:18:29]

On 2016/03/21 22:10:30, boliu wrote:
>
https://codereview.chromium.org/1769913003/diff/300001/android_webview/browse...
> File android_webview/browser/hardware_renderer.cc (right):
> 
>
https://codereview.chromium.org/1769913003/diff/300001/android_webview/browse...
> android_webview/browser/hardware_renderer.cc:232:
> last_submitted_output_surface_id_);
> On 2016/03/21 21:59:54, sievers wrote:
> > On 2016/03/21 21:35:49, boliu wrote:
> > > On 2016/03/21 19:56:12, sievers wrote:
> > > > Regarding using |last_submitted_output_surface_id_| is that really true
> that
> > > > we'll never commit but not draw, or skip a frame or so?
> > > 
> > > It totally is *not* true. CommitFrame can get called twice, and both times
> > > committing new frames, but there is no DrawGL (ie submit) in between them.
> > > 
> > > That's why this is the submitted id rather than committed id.
> > > 
> > > Let's say:
> > > Commit id 0 frame 0
> > > Submit id 0 frame 0
> > > Commit id 1 frame 0
> > > Commit id 1 frame 1
> > > 
> > > At this point, "id 1 frame 0" will be returned from
> > ReturnResourcesInChildFrame.
> > > 
> > > Submit id 1 frame 1
> > > 
> > > At this point, "id 0 frame 0" will be dropped here.
> > 
> > Ah right, makes sense. But it's a bit hard to follow. Just wondering - would
> it
> > make things easier to follow if SharedRendererState would track the surface
id
> > for which it's currently collecting resources?
> 
> Oh I totally tried that first. It's more complex this doing it here imo. So
> options I considered:
> 
> 1) SRS keep a map of maps:
> compositor_id -> output_surface_id -> resources
> too complex
> 
> 2) SRS keep a map of compositor_id -> struct {output_surface_id, resources}
> This doesn't work because SwapReturnedResourcesOnUI swaps the whole map, so
the
> output_surface_id info is lost. And fixing that is complex.
> 
> 3) Add parallel compositor_id -> current output_surface_id map.
> Then SRS will need to pay attention to compositor lifetimes, which is ugly.

fair enough. lgtm

[boliu, 2016-03-21 22:43:46]

The CQ bit was checked by boliu@chromium.org

[boliu, 2016-03-21 22:43:47]

The patchset sent to the CQ was uploaded after l-g-t-m from hush@chromium.org
Link to the patchset: https://codereview.chromium.org/1769913003/#ps320001
(title: "comment")

[commit-bot: I haz the power, 2016-03-21 22:44:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1769913003/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1769913003/320001

[commit-bot: I haz the power, 2016-03-21 22:55:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-21 22:55:12]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[boliu, 2016-03-21 22:56:46]

boliu@chromium.org changed reviewers:
+ dcheng@chromium.org

[boliu, 2016-03-21 22:56:47]

+dcheng for content/common/android/sync_compositor_messages.*

[dcheng, 2016-03-22 07:22:37]

https://codereview.chromium.org/1769913003/diff/320001/content/public/browser...
File content/public/browser/android/synchronous_compositor.cc (right):

https://codereview.chromium.org/1769913003/diff/320001/content/public/browser...
content/public/browser/android/synchronous_compositor.cc:21:
SynchronousCompositor::Frame&& rhs) {
Frame&& rhs should work, for consistency with the move constructor.

https://codereview.chromium.org/1769913003/diff/320001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/1769913003/diff/320001/content/renderer/andro...
content/renderer/android/synchronous_compositor_proxy.cc:243: // Not using
WriteParams because cc::CompositorFrame is not copy-able.
I don't think WriteParams requires that the input be copyable, does it?

[boliu, 2016-03-22 15:25:50]

https://codereview.chromium.org/1769913003/diff/320001/content/public/browser...
File content/public/browser/android/synchronous_compositor.cc (right):

https://codereview.chromium.org/1769913003/diff/320001/content/public/browser...
content/public/browser/android/synchronous_compositor.cc:21:
SynchronousCompositor::Frame&& rhs) {
On 2016/03/22 07:22:37, dcheng wrote:
> Frame&& rhs should work, for consistency with the move constructor.

Done.

https://codereview.chromium.org/1769913003/diff/320001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/1769913003/diff/320001/content/renderer/andro...
content/renderer/android/synchronous_compositor_proxy.cc:243: // Not using
WriteParams because cc::CompositorFrame is not copy-able.
On 2016/03/22 07:22:37, dcheng wrote:
> I don't think WriteParams requires that the input be copyable, does it?

Switched to WriteReplyParams.

Honestly don't remember why I didn't choose this at the time.. I think it was
before ipc switched to variadic templates, and I didn't figure out how to
construct a tuple containing cc::CompositorFrame at the time.

[dcheng, 2016-03-22 17:27:21]

lgtm

https://codereview.chromium.org/1769913003/diff/340001/android_webview/browse...
File android_webview/browser/browser_view_renderer_unittest.cc (right):

https://codereview.chromium.org/1769913003/diff/340001/android_webview/browse...
android_webview/browser/browser_view_renderer_unittest.cc:210: for (auto& info :
infos) {
Nit: const auto&

https://codereview.chromium.org/1769913003/diff/340001/android_webview/browse...
android_webview/browser/browser_view_renderer_unittest.cc:260: for (auto&
resources : returned_resources_array) {
Nit: I think const auto& should work here and 263 as well.

https://codereview.chromium.org/1769913003/diff/340001/android_webview/browse...
File android_webview/browser/test/rendering_test.cc (right):

https://codereview.chromium.org/1769913003/diff/340001/android_webview/browse...
android_webview/browser/test/rendering_test.cc:83: DCHECK(compositor_.get());
Nit: no .get()

[boliu, 2016-03-22 18:11:22]

https://codereview.chromium.org/1769913003/diff/340001/android_webview/browse...
File android_webview/browser/browser_view_renderer_unittest.cc (right):

https://codereview.chromium.org/1769913003/diff/340001/android_webview/browse...
android_webview/browser/browser_view_renderer_unittest.cc:210: for (auto& info :
infos) {
On 2016/03/22 17:27:21, dcheng wrote:
> Nit: const auto&

Done.

https://codereview.chromium.org/1769913003/diff/340001/android_webview/browse...
android_webview/browser/browser_view_renderer_unittest.cc:260: for (auto&
resources : returned_resources_array) {
On 2016/03/22 17:27:21, dcheng wrote:
> Nit: I think const auto& should work here and 263 as well.

Done

https://codereview.chromium.org/1769913003/diff/340001/android_webview/browse...
File android_webview/browser/test/rendering_test.cc (right):

https://codereview.chromium.org/1769913003/diff/340001/android_webview/browse...
android_webview/browser/test/rendering_test.cc:83: DCHECK(compositor_.get());
On 2016/03/22 17:27:21, dcheng wrote:
> Nit: no .get()

Done.

[boliu, 2016-03-22 18:11:30]

The CQ bit was checked by boliu@chromium.org

[boliu, 2016-03-22 18:11:30]

The patchset sent to the CQ was uploaded after l-g-t-m from hush@chromium.org,
sievers@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1769913003/#ps360001
(title: "fix nit in test")

[commit-bot: I haz the power, 2016-03-22 18:11:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1769913003/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1769913003/360001

[commit-bot: I haz the power, 2016-03-22 19:33:26]

Description was changed from

==========
sync compositor: Add output_surface_id plumbing

The output_surface_id is unique for a particular renderer compositor
instance. It is used to avoid returning resources from lost context to
compositor with new context.

Plumb output_surface_id from SynchronousOutputSurface all the way
to android_webview::HardwareRenderer. To avoid having to send down
a map<id, resource list>, a simpler guarantee is to assume the
output_surface_id is increasing, so only need to return resources
of with highest output surface id and drop resources other resources. 

BUG=592744
==========

to

==========
sync compositor: Add output_surface_id plumbing

The output_surface_id is unique for a particular renderer compositor
instance. It is used to avoid returning resources from lost context to
compositor with new context.

Plumb output_surface_id from SynchronousOutputSurface all the way
to android_webview::HardwareRenderer. To avoid having to send down
a map<id, resource list>, a simpler guarantee is to assume the
output_surface_id is increasing, so only need to return resources
of with highest output surface id and drop resources other resources. 

BUG=592744
==========

[commit-bot: I haz the power, 2016-03-22 19:33:27]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2016-03-22 19:34:25]

Description was changed from

==========
sync compositor: Add output_surface_id plumbing

The output_surface_id is unique for a particular renderer compositor
instance. It is used to avoid returning resources from lost context to
compositor with new context.

Plumb output_surface_id from SynchronousOutputSurface all the way
to android_webview::HardwareRenderer. To avoid having to send down
a map<id, resource list>, a simpler guarantee is to assume the
output_surface_id is increasing, so only need to return resources
of with highest output surface id and drop resources other resources. 

BUG=592744
==========

to

==========
sync compositor: Add output_surface_id plumbing

The output_surface_id is unique for a particular renderer compositor
instance. It is used to avoid returning resources from lost context to
compositor with new context.

Plumb output_surface_id from SynchronousOutputSurface all the way
to android_webview::HardwareRenderer. To avoid having to send down
a map<id, resource list>, a simpler guarantee is to assume the
output_surface_id is increasing, so only need to return resources
of with highest output surface id and drop resources other resources.

BUG=592744

Committed: https://crrev.com/f4e57485631dc4c3beaa87de398975cab87fdc32
Cr-Commit-Position: refs/heads/master@{#382636}
==========

[commit-bot: I haz the power, 2016-03-22 19:34:26]

Patchset 19 (id:??) landed as
https://crrev.com/f4e57485631dc4c3beaa87de398975cab87fdc32
Cr-Commit-Position: refs/heads/master@{#382636}