Mus+Ash: Use standard cc mechanism for surface lifetime.

Mus+Ash: Use standard cc mechanism for surface lifetime.

Rendering of the topmost frame happens in two phases. First the frame
is generated and submitted, and a later date it is actually drawn.
During the time the frame is generated and drawn we can't destroy the
surface, otherwise when drawn you get an empty surface.

To address this previously, mus-ws implemented its own lifetime management
system. cc already has a mechanism called SurfaceSequence (which is being
updated in parallel). However, in order to manage surface lifetime correctly
the face of surface ID propagation to clients, we need to track surfaces
using cc.

BUG=647852
Committed: https://crrev.com/d469420cbb2b76ae2311ecfbfbccff74422878f0
Cr-Commit-Position: refs/heads/master@{#424010}

[Fady Samuel, 2016-10-06 23:44:25]

fsamuel@chromium.org changed reviewers:
+ sky@chromium.org

[Fady Samuel, 2016-10-07 00:22:49]

fsamuel@chromium.org changed reviewers:
+ rjkroege@chromium.org

[Fady Samuel, 2016-10-07 00:22:50]

+rjkroege@ PTAL

[Fady Samuel, 2016-10-07 00:24:55]

Ooops, please ignore this for now. There's a memory leak.

[Fady Samuel, 2016-10-07 14:11:55]

PTAL Scott! Thanks

[Fady Samuel, 2016-10-07 14:24:55]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 14:25:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 15:32:28]

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

[commit-bot: I haz the power, 2016-10-07 15:32:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2016-10-07 16:28:04]

Looks like you still have issues (based on try output)?

[Fady Samuel, 2016-10-07 16:35:28]

On 2016/10/07 16:28:04, sky wrote:
> Looks like you still have issues (based on try output)?

Fixed. It looks like FrameGenerator is generating SurfaceDrawQuads for
ServerWindowSurfaces that haven't received CompositorFrames. This test operates
under the assumption that this is OK.

Since this code is all going to change significantly soon anyway, I decided to
continue to behave as before so that the test doesn't need to change. This is
unrelated to the rest of the patch though.

It mostly stays as is.

[Fady Samuel, 2016-10-07 16:42:02]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 16:42:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sky, 2016-10-07 16:56:15]

LGTM - but wait for Rob to review FrameGenerator as I'm not familiar with that
code.

[Fady Samuel, 2016-10-07 17:34:31]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 17:34:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rjkroege, 2016-10-07 19:34:50]

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:257:
surface->AddDestructionDependency(dependency.sequence);
I think that this logic could be refactored in the future

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:276:
AddOrUpdateSurfaceReference(window_surface);
What is going on with this recursion? Is this ever reached? It's not clear to me
if it will always stop.

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator.h (right):

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.h:76: void Draw();
there's something not right about this comment? Or the code. You told me that
there was no need to submit new compositor frames from FrameGenerator unless the
wm changes?

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.h:96: // LocalFrameId then release that reference
first.
the arg is surface. not window_surface? Something needs to be corrected here?

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.h:97: void
AddOrUpdateSurfaceReference(ServerWindowSurface* surface);

can you say why/when we should call this?

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/server_w...
File services/ui/ws/server_window_surface.h (right):

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/server_w...
services/ui/ws/server_window_surface.h:53: const cc::FrameSinkId&
frame_sink_id() const { return frame_sink_id_; }
would be awesome to remind people what a FrameSinkId is.

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/server_w...
services/ui/ws/server_window_surface.h:60: ServerWindow* window();
if I remember correctly, we need windows that don't have surfaces. How does that
affect this code?

[commit-bot: I haz the power, 2016-10-07 19:36:20]

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

[commit-bot: I haz the power, 2016-10-07 19:36:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Fady Samuel, 2016-10-07 20:31:33]

PTAL Rob!

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:257:
surface->AddDestructionDependency(dependency.sequence);
On 2016/10/07 19:34:50, rjkroege wrote:
> I think that this logic could be refactored in the future

This is going away once kylechar@ finishes his surface refcounting work.

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:276:
AddOrUpdateSurfaceReference(window_surface);
On 2016/10/07 19:34:50, rjkroege wrote:
> What is going on with this recursion? Is this ever reached? It's not clear to
me
> if it will always stop.

It will. The reason we got here is because we already had a stale dependency. We
released the reference in the line above so it has been cleared from the
dependencies_ map and so we'll enter the if statement above.

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator.h (right):

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.h:76: void Draw();
On 2016/10/07 19:34:50, rjkroege wrote:
> there's something not right about this comment? Or the code. You told me that
> there was no need to submit new compositor frames from FrameGenerator unless
the
> wm changes?

We're not there yet. That's once all my code and Sadrul's code lands.

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.h:96: // LocalFrameId then release that reference
first.
On 2016/10/07 19:34:50, rjkroege wrote:
> the arg is surface. not window_surface? Something needs to be corrected here?

Done.

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.h:97: void
AddOrUpdateSurfaceReference(ServerWindowSurface* surface);
On 2016/10/07 19:34:50, rjkroege wrote:
> 
> can you say why/when we should call this?

Done.

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/server_w...
File services/ui/ws/server_window_surface.h (right):

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/server_w...
services/ui/ws/server_window_surface.h:53: const cc::FrameSinkId&
frame_sink_id() const { return frame_sink_id_; }
On 2016/10/07 19:34:50, rjkroege wrote:
> would be awesome to remind people what a FrameSinkId is.

Done.

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/server_w...
services/ui/ws/server_window_surface.h:60: ServerWindow* window();
On 2016/10/07 19:34:50, rjkroege wrote:
> if I remember correctly, we need windows that don't have surfaces. How does
that
> affect this code?

A surface has a window but a window may not have a surface. This is the
relationship here.

[Fady Samuel, 2016-10-07 20:31:44]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 20:32:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rjkroege, 2016-10-07 20:48:13]

Thanks for adding the comments. Very nice. Do that more please. :-)

lgtm with nits.

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:257:
surface->AddDestructionDependency(dependency.sequence);
On 2016/10/07 20:31:33, Fady Samuel wrote:
> On 2016/10/07 19:34:50, rjkroege wrote:
> > I think that this logic could be refactored in the future
> 
> This is going away once kylechar@ finishes his surface refcounting work.

can you add a TODO before landing?

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:276:
AddOrUpdateSurfaceReference(window_surface);
On 2016/10/07 20:31:33, Fady Samuel wrote:
> On 2016/10/07 19:34:50, rjkroege wrote:
> > What is going on with this recursion? Is this ever reached? It's not clear
to
> me
> > if it will always stop.
> 
> It will. The reason we got here is because we already had a stale dependency.
We
> released the reference in the line above so it has been cleared from the
> dependencies_ map and so we'll enter the if statement above.

Please insert this something like this as a comment before landing.

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator.h (right):

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.h:76: void Draw();
On 2016/10/07 20:31:33, Fady Samuel wrote:
> On 2016/10/07 19:34:50, rjkroege wrote:
> > there's something not right about this comment? Or the code. You told me
that
> > there was no need to submit new compositor frames from FrameGenerator unless
> the
> > wm changes?
> 
> We're not there yet. That's once all my code and Sadrul's code lands.

please update the TODO for the brave polliwog world

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/server_w...
File services/ui/ws/server_window_surface.h (right):

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/server_w...
services/ui/ws/server_window_surface.h:60: ServerWindow* window();
On 2016/10/07 20:31:33, Fady Samuel wrote:
> On 2016/10/07 19:34:50, rjkroege wrote:
> > if I remember correctly, we need windows that don't have surfaces. How does
> that
> > affect this code?
> 
> A surface has a window but a window may not have a surface. This is the
> relationship here.

was an off-topic comment. sorry

https://codereview.chromium.org/2400723003/diff/100001/services/ui/ws/frame_g...
File services/ui/ws/frame_generator.h (right):

https://codereview.chromium.org/2400723003/diff/100001/services/ui/ws/frame_g...
services/ui/ws/frame_generator.h:97: // ServerWindowSurrface as FrameGenerator
walks the window tree to generate a
nit: spelling

[Fady Samuel, 2016-10-07 21:04:33]

Thanks Rob! CQ'ing!

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/frame_ge...
File services/ui/ws/frame_generator.cc (right):

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:257:
surface->AddDestructionDependency(dependency.sequence);
On 2016/10/07 20:48:13, rjkroege wrote:
> On 2016/10/07 20:31:33, Fady Samuel wrote:
> > On 2016/10/07 19:34:50, rjkroege wrote:
> > > I think that this logic could be refactored in the future
> > 
> > This is going away once kylechar@ finishes his surface refcounting work.
> 
> can you add a TODO before landing?

Done.

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/frame_ge...
services/ui/ws/frame_generator.cc:276:
AddOrUpdateSurfaceReference(window_surface);
On 2016/10/07 20:48:13, rjkroege wrote:
> On 2016/10/07 20:31:33, Fady Samuel wrote:
> > On 2016/10/07 19:34:50, rjkroege wrote:
> > > What is going on with this recursion? Is this ever reached? It's not clear
> to
> > me
> > > if it will always stop.
> > 
> > It will. The reason we got here is because we already had a stale
dependency.
> We
> > released the reference in the line above so it has been cleared from the
> > dependencies_ map and so we'll enter the if statement above.
> 
> Please insert this something like this as a comment before landing.

Done.

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/server_w...
File services/ui/ws/server_window_surface.h (right):

https://codereview.chromium.org/2400723003/diff/80001/services/ui/ws/server_w...
services/ui/ws/server_window_surface.h:60: ServerWindow* window();
On 2016/10/07 20:48:13, rjkroege wrote:
> On 2016/10/07 20:31:33, Fady Samuel wrote:
> > On 2016/10/07 19:34:50, rjkroege wrote:
> > > if I remember correctly, we need windows that don't have surfaces. How
does
> > that
> > > affect this code?
> > 
> > A surface has a window but a window may not have a surface. This is the
> > relationship here.
> 
> was an off-topic comment. sorry

Acknowledged.

https://codereview.chromium.org/2400723003/diff/100001/services/ui/ws/frame_g...
File services/ui/ws/frame_generator.h (right):

https://codereview.chromium.org/2400723003/diff/100001/services/ui/ws/frame_g...
services/ui/ws/frame_generator.h:97: // ServerWindowSurrface as FrameGenerator
walks the window tree to generate a
On 2016/10/07 20:48:13, rjkroege wrote:
> nit: spelling

Done.

[Fady Samuel, 2016-10-07 21:04:38]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-10-07 21:04:38]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
rjkroege@chromium.org
Link to the patchset: https://codereview.chromium.org/2400723003/#ps120001
(title: "Addressed Rob's comments")

[commit-bot: I haz the power, 2016-10-07 21:05:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Fady Samuel, 2016-10-07 22:43:35]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-10-07 22:43:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
rjkroege@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2400723003/#ps140001
(title: "Only AddObserver once")

[commit-bot: I haz the power, 2016-10-07 22:44:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 23:31:06]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-10-07 23:34:50]

Description was changed from

==========
Mus+Ash: Use standard cc mechanism for surface lifetime.

Rendering of the topmost frame happens in two phases. First the frame
is generated and submitted, and a later date it is actually drawn.
During the time the frame is generated and drawn we can't destroy the
surface, otherwise when drawn you get an empty surface.

To address this previously, mus-ws implemented its own lifetime management
system. cc already has a mechanism called SurfaceSequence (which is being
updated in parallel). However, in order to manage surface lifetime correctly
the face of surface ID propagation to clients, we need to track surfaces
using cc.

BUG=647852
==========

to

==========
Mus+Ash: Use standard cc mechanism for surface lifetime.

Rendering of the topmost frame happens in two phases. First the frame
is generated and submitted, and a later date it is actually drawn.
During the time the frame is generated and drawn we can't destroy the
surface, otherwise when drawn you get an empty surface.

To address this previously, mus-ws implemented its own lifetime management
system. cc already has a mechanism called SurfaceSequence (which is being
updated in parallel). However, in order to manage surface lifetime correctly
the face of surface ID propagation to clients, we need to track surfaces
using cc.

BUG=647852

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

[commit-bot: I haz the power, 2016-10-07 23:34:51]

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