Implement service-side surface synchronization

Implement service-side surface synchronization

This CL introduces the notion of a pending and active CompositorFrame 
along with surface dependency tracking and resolution.

An active CompositorFrame is a frame that is a candidate for display. A
CompositorFrame is active if all its surface dependencies are active, 
or the display compositor explicitly activated it after some set 
deadline.

A pending CompositorFrame is a frame that has unresolved dependencies: 
it refers to surface IDs that either do not yet exist in the display 
compositor or don't have an active frame.

Surface::QueueFrame determines whether a frame refers to valid surface 
IDs or not. If a CompositorFrame has unresolved dependencies then it's
marked as pending and the SurfaceDependencyTracker is asked to track
the resolution of its dependencies. SurfaceDependencyTracker will call
back into the Surface to report when a dependency has been activated.

For the time being (until SurfaceReference work is complete), the first 
CompositorFrame submitted with unresolved dependencies will trigger a 
global deadline. The deadline will be cleared if all dependencies are
resolved within the alloted time window. If dependencies are not 
resolved before the deadline, then SurfaceDependencyTracker will 
forcibly activate currently pending frames.

BUG=672962
TBR=reveman@chromium.org for trivial exo change.
TBR=tsepez@chromium.org for minor ipc change.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2676373004
Cr-Commit-Position: refs/heads/master@{#449182}
Committed: https://chromium.googlesource.com/chromium/src/+/cb618a172eafc9c9d902ff7257a8e72d8bb764d7

[Fady Samuel, 2017-02-06 22:47:57]

Description was changed from

==========
Implement service-side surface synchronization

BUG=
==========

to

==========
Implement service-side surface synchronization

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2017-02-07 17:15:27]

Description was changed from

==========
Implement service-side surface synchronization

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Implement service-side surface synchronization

This CL introduces the notion of a pending and active CompositorFrame 
along with surface dependency tracking and resolution.

An active CompositorFrame is a frame that is a candidate for display. A
CompositorFrame is active if all its surface dependencies are active, 
or the display compositor explicitly activated it after some set 
deadline.

A pending CompositorFrame is a frame that has unresolved dependencies: it refers
to surface IDs that either do not yet exist in the display 
compositor or don't have an active frame.

Surface::QueueFrame determines whether a frame refers to valid surface 
IDs or not. If a CompositorFrame has unresolved dependencies then its
marked as pending and the SurfaceDependencyTracker is asked to track
the resolution of its dependencies. SurfaceDependencyTracker will call
back into the Surface to report when a dependency has been activated.

BUG=672962

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2017-02-07 17:15:48]

Description was changed from

==========
Implement service-side surface synchronization

This CL introduces the notion of a pending and active CompositorFrame 
along with surface dependency tracking and resolution.

An active CompositorFrame is a frame that is a candidate for display. A
CompositorFrame is active if all its surface dependencies are active, 
or the display compositor explicitly activated it after some set 
deadline.

A pending CompositorFrame is a frame that has unresolved dependencies: it refers
to surface IDs that either do not yet exist in the display 
compositor or don't have an active frame.

Surface::QueueFrame determines whether a frame refers to valid surface 
IDs or not. If a CompositorFrame has unresolved dependencies then its
marked as pending and the SurfaceDependencyTracker is asked to track
the resolution of its dependencies. SurfaceDependencyTracker will call
back into the Surface to report when a dependency has been activated.

BUG=672962

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Implement service-side surface synchronization

This CL introduces the notion of a pending and active CompositorFrame 
along with surface dependency tracking and resolution.

An active CompositorFrame is a frame that is a candidate for display. A
CompositorFrame is active if all its surface dependencies are active, 
or the display compositor explicitly activated it after some set 
deadline.

A pending CompositorFrame is a frame that has unresolved dependencies: 
it refers to surface IDs that either do not yet exist in the display 
compositor or don't have an active frame.

Surface::QueueFrame determines whether a frame refers to valid surface 
IDs or not. If a CompositorFrame has unresolved dependencies then its
marked as pending and the SurfaceDependencyTracker is asked to track
the resolution of its dependencies. SurfaceDependencyTracker will call
back into the Surface to report when a dependency has been activated.

BUG=672962

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2017-02-07 17:18:21]

Description was changed from

==========
Implement service-side surface synchronization

This CL introduces the notion of a pending and active CompositorFrame 
along with surface dependency tracking and resolution.

An active CompositorFrame is a frame that is a candidate for display. A
CompositorFrame is active if all its surface dependencies are active, 
or the display compositor explicitly activated it after some set 
deadline.

A pending CompositorFrame is a frame that has unresolved dependencies: 
it refers to surface IDs that either do not yet exist in the display 
compositor or don't have an active frame.

Surface::QueueFrame determines whether a frame refers to valid surface 
IDs or not. If a CompositorFrame has unresolved dependencies then its
marked as pending and the SurfaceDependencyTracker is asked to track
the resolution of its dependencies. SurfaceDependencyTracker will call
back into the Surface to report when a dependency has been activated.

BUG=672962

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Implement service-side surface synchronization

This CL introduces the notion of a pending and active CompositorFrame 
along with surface dependency tracking and resolution.

An active CompositorFrame is a frame that is a candidate for display. A
CompositorFrame is active if all its surface dependencies are active, 
or the display compositor explicitly activated it after some set 
deadline.

A pending CompositorFrame is a frame that has unresolved dependencies: 
it refers to surface IDs that either do not yet exist in the display 
compositor or don't have an active frame.

Surface::QueueFrame determines whether a frame refers to valid surface 
IDs or not. If a CompositorFrame has unresolved dependencies then it's
marked as pending and the SurfaceDependencyTracker is asked to track
the resolution of its dependencies. SurfaceDependencyTracker will call
back into the Surface to report when a dependency has been activated.

For the time being (until SurfaceReference work is complete), the first
CompositorFrame submitted with unresolved dependencies will trigger a global
deadline. The deadline will be cleared if all dependencies are
resolved within the alloted time window. If dependencies are not 
resolved before the deadline, then SurfaceDependencyTracker will 
forcibly activate currently pending frames.

BUG=672962

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2017-02-07 17:18:42]

Description was changed from

==========
Implement service-side surface synchronization

This CL introduces the notion of a pending and active CompositorFrame 
along with surface dependency tracking and resolution.

An active CompositorFrame is a frame that is a candidate for display. A
CompositorFrame is active if all its surface dependencies are active, 
or the display compositor explicitly activated it after some set 
deadline.

A pending CompositorFrame is a frame that has unresolved dependencies: 
it refers to surface IDs that either do not yet exist in the display 
compositor or don't have an active frame.

Surface::QueueFrame determines whether a frame refers to valid surface 
IDs or not. If a CompositorFrame has unresolved dependencies then it's
marked as pending and the SurfaceDependencyTracker is asked to track
the resolution of its dependencies. SurfaceDependencyTracker will call
back into the Surface to report when a dependency has been activated.

For the time being (until SurfaceReference work is complete), the first
CompositorFrame submitted with unresolved dependencies will trigger a global
deadline. The deadline will be cleared if all dependencies are
resolved within the alloted time window. If dependencies are not 
resolved before the deadline, then SurfaceDependencyTracker will 
forcibly activate currently pending frames.

BUG=672962

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Implement service-side surface synchronization

This CL introduces the notion of a pending and active CompositorFrame 
along with surface dependency tracking and resolution.

An active CompositorFrame is a frame that is a candidate for display. A
CompositorFrame is active if all its surface dependencies are active, 
or the display compositor explicitly activated it after some set 
deadline.

A pending CompositorFrame is a frame that has unresolved dependencies: 
it refers to surface IDs that either do not yet exist in the display 
compositor or don't have an active frame.

Surface::QueueFrame determines whether a frame refers to valid surface 
IDs or not. If a CompositorFrame has unresolved dependencies then it's
marked as pending and the SurfaceDependencyTracker is asked to track
the resolution of its dependencies. SurfaceDependencyTracker will call
back into the Surface to report when a dependency has been activated.

For the time being (until SurfaceReference work is complete), the first 
CompositorFrame submitted with unresolved dependencies will trigger a 
global deadline. The deadline will be cleared if all dependencies are
resolved within the alloted time window. If dependencies are not 
resolved before the deadline, then SurfaceDependencyTracker will 
forcibly activate currently pending frames.

BUG=672962

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2017-02-07 17:19:45]

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

[Fady Samuel, 2017-02-07 17:19:46]

+danakj@ for review
+rjkroege@ FYI [happy to get comments too :-)]

[Fady Samuel, 2017-02-07 17:21:53]

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

[Fady Samuel, 2017-02-07 17:21:54]

+kylechar@ FYI

[danakj, 2017-02-07 19:44:04]

danakj@chromium.org changed reviewers:
+ vmpstr@chromium.org
- danakj@chromium.org

[danakj, 2017-02-07 19:44:04]

=> vmpstr

[Fady Samuel, 2017-02-07 20:41:53]

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

[commit-bot: I haz the power, 2017-02-07 20:42:32]

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, 2017-02-07 20:56:13]

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

[commit-bot: I haz the power, 2017-02-07 20:56:14]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Fady Samuel, 2017-02-07 21:29:40]

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

[commit-bot: I haz the power, 2017-02-07 21:30:51]

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, 2017-02-07 21:45:36]

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

[commit-bot: I haz the power, 2017-02-07 21:45:37]

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

[Fady Samuel, 2017-02-07 22:34:59]

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

[commit-bot: I haz the power, 2017-02-07 22:36:39]

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

[vmpstr, 2017-02-07 22:46:20]

sorry, missed a few patchsets, so some comments might not apply?

https://codereview.chromium.org/2676373004/diff/80001/cc/ipc/compositor_frame...
File cc/ipc/compositor_frame_metadata.mojom (right):

https://codereview.chromium.org/2676373004/diff/80001/cc/ipc/compositor_frame...
cc/ipc/compositor_frame_metadata.mojom:33: bool respect_deadline;
As an aside, should there some ordering to these variables with respect to
types? I'm not sure how mojo structs work, but in C++ for example (on my
machine)

struct {
 bool b1;
 int i;
 bool b2;
};

is 12 bytes, where as

struct {
 int i;
 bool b1;
 bool b2;
};

is 8 bytes. (padding ftl).

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/pending_fra...
File cc/surfaces/pending_frame_observer.h (right):

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/pending_fra...
cc/surfaces/pending_frame_observer.h:1: // Copyright 2016 The Chromium Authors.
All rights reserved.
nit: 2017

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/pending_fra...
cc/surfaces/pending_frame_observer.h:19: virtual void OnSurfaceChanged(
Is this OnSurfaceDependenciesChanged or can surface itself change without
dependencies changing? If it's the former, I think OnSurfaceDependenciesChanged
is a better name

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:55: void Surface::QueueFrame(CompositorFrame frame, const
DrawCallback& callback) {
Can you extract the code between lines 60 and 96 to a helper function, something
like:

UpdateBlockedSurfaces(previous_pending_frame, frame);

? I think that might read better

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:74: SurfaceDependencies added_dependencies;
move this to right before the second for loop please, so it's clear which loop
populates what

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:77: for (const SurfaceId& surface_id :
blocked_on_surfaces_) {
If you use flat_sets which are ordered, then I think you could use
std::set_difference here, but tbh I think this is fine.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:127: if (active_frame_ &&
!active_frame_->render_pass_list.empty()) {
minor nit: while here, I think this would read better as

if (!active_frame_ || active_frame_->render_pass_list.empty()) {
  copy_request->SendEmptyResult();
  return;
}

...

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:160: ActivatePendingFrame();
Are we guaranteed to have a pending frame at this point?

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:188: void Surface::ActivateFrame(CompositorFrame frame) {
Can you add a comment at the top of the function to just outline what it takes
to activate a frame

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:197: if (active_frame_)
In what situations can we not have an active_frame_ here?

Also, you can combine this if with the next one

if (active_frame) {
  ...
  if (!active_frame->render_pass_list_empty())
    ...
}

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:219: if (active_frame_) {
nit:

if (!active_frame_)
  return;

...

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.h
File cc/surfaces/surface.h (right):

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.h#n...
cc/surfaces/surface.h:60: void ReportSurfaceIdAvailable(const SurfaceId&
surface_id);
Notify?

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.h#n...
cc/surfaces/surface.h:79: // Returns the currently pending frame.
Can you add a similar comment here as above, "must check HasPendingFrame" etc

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.h#n...
cc/surfaces/surface.h:137: SurfaceDependencies blocked_on_surfaces_;
nit: "blocking_surfaces_"?

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.h#n...
cc/surfaces/surface.h:145: using PendingSurfaceSet =
std::unordered_set<Surface*>;
What's the expected size of these things? flat sets are landing soon, so I
wonder if they could be used here.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
File cc/surfaces/surface_dependency_tracker.cc (right):

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
nit: 2017

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:14: static constexpr uint32_t
MAX_BEGIN_FRAME_COUNT = 4;
nit: no need for static or no need for namespace {}
nit: kMaxBeginFrameCount

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:31: if
(!pending_surface->HasPendingFrame())
From reading the header comment, it seems that the surface wants to be informed
of something. Is that a strong requirement? If so I think you need to call
something on it before returning :P

Either that or this should be a DCHECK?

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:36: // Referenced surface IDs that
aren't currently known to the surface manager
I guess I'm misunderstanding a bit, but is

"known to the surface manager" imply both surface is in there _and_ it has an
active frame? Specifically, if it _doesn't_ have an active frame, does that mean
the surface is not known?

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:58: // scope deadlines to surface
subtrees. We cannot do that until
Can you file a bug for this and reference it here, please?

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:70: if (has_deadline_) {
Instead of adding/removing |this| from the observers, do you think it's better
to just always observe but early out if it has no deadline?

Also, how is it possible that we're in this function and has_deadline_ is false?

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:84: if
(!pending_surface->HasPendingFrame())
Comment please

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:94: if (it !=
blocking_surfaces_.end()) {
if (it == blocking_surface_.end())
  continue;
...

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:113: // TODO(fsamuel): We should
consider removing this surface as a dependency on
File a bug for this please and reference here (I know this is just an
exploratory TODO, but it's easier to track progress on crbug)

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:133:
blocking_surfaces_[surface_id].erase(pending_surface);
Do a find and use the iterator for both erase and checking if empty and erasing
(less lookups)

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
File cc/surfaces/surface_dependency_tracker.h (right):

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.h:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
nit: 2017

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.h:76: BeginFrameSource*
begin_frame_source_;
should this be a const ptr as well?

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.h:78: // Whether or not a frame observer
has been added.
nit: Indicates whether...

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_fac...
File cc/surfaces/surface_factory.cc (right):

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_fac...
cc/surfaces/surface_factory.cc:119: const CompositorFrame& frame =
surface->GetActiveFrame();
DCHECK that it hasactiveframe

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_fac...
cc/surfaces/surface_factory.cc:120: // CompositorFrames may not be populated
with a RenderPass in unit tests.
nit: this reads like unittests are not allowed to populate it with a render
pass... maybe s/may/might/? *shrug*

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_fac...
cc/surfaces/surface_factory.cc:129: surface->RemoveObserver(this);
Is there a doc describing who observes whom and for how long and why? This patch
looks to be adding a few more places where we observe or not observe, and I
think having a central doc with all of the relationships might be good for
posterity.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_id_...
File cc/surfaces/surface_id_allocator.cc (right):

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_id_...
cc/surfaces/surface_id_allocator.cc:19: const LocalSurfaceId&
SurfaceIdAllocator::GenerateId() {
Why this change?

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_man...
File cc/surfaces/surface_manager.cc (right):

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_man...
cc/surfaces/surface_manager.cc:161: // if (surface_map_.count(child_id) == 0) {
<_<

[commit-bot: I haz the power, 2017-02-07 23:11:42]

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

[commit-bot: I haz the power, 2017-02-07 23:11:44]

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_...)

[Fady Samuel, 2017-02-08 00:52:36]

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

[Fady Samuel, 2017-02-08 00:52:50]

Thank you for the thorough comments, Vlad! PTAL!

https://codereview.chromium.org/2676373004/diff/80001/cc/ipc/compositor_frame...
File cc/ipc/compositor_frame_metadata.mojom (right):

https://codereview.chromium.org/2676373004/diff/80001/cc/ipc/compositor_frame...
cc/ipc/compositor_frame_metadata.mojom:33: bool respect_deadline;
On 2017/02/07 22:46:19, vmpstr wrote:
> As an aside, should there some ordering to these variables with respect to
> types? I'm not sure how mojo structs work, but in C++ for example (on my
> machine)
> 
> struct {
>  bool b1;
>  int i;
>  bool b2;
> };
> 
> is 12 bytes, where as
> 
> struct {
>  int i;
>  bool b1;
>  bool b2;
> };
> 
> is 8 bytes. (padding ftl).
> 

Yes, we should probably move all bools to the bottom, but I won't do it in this
CL to confuse matters.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/pending_fra...
File cc/surfaces/pending_frame_observer.h (right):

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/pending_fra...
cc/surfaces/pending_frame_observer.h:1: // Copyright 2016 The Chromium Authors.
All rights reserved.
On 2017/02/07 22:46:19, vmpstr wrote:
> nit: 2017

Done.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/pending_fra...
cc/surfaces/pending_frame_observer.h:19: virtual void OnSurfaceChanged(
On 2017/02/07 22:46:19, vmpstr wrote:
> Is this OnSurfaceDependenciesChanged or can surface itself change without
> dependencies changing? If it's the former, I think
OnSurfaceDependenciesChanged
> is a better name

Done.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:55: void Surface::QueueFrame(CompositorFrame frame, const
DrawCallback& callback) {
On 2017/02/07 22:46:19, vmpstr wrote:
> Can you extract the code between lines 60 and 96 to a helper function,
something
> like:
> 
> UpdateBlockedSurfaces(previous_pending_frame, frame);
> 
> ? I think that might read better

Done.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:74: SurfaceDependencies added_dependencies;
On 2017/02/07 22:46:19, vmpstr wrote:
> move this to right before the second for loop please, so it's clear which loop
> populates what

Done.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:77: for (const SurfaceId& surface_id :
blocked_on_surfaces_) {
On 2017/02/07 22:46:19, vmpstr wrote:
> If you use flat_sets which are ordered, then I think you could use
> std::set_difference here, but tbh I think this is fine.

I'll leave it as is for now.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:127: if (active_frame_ &&
!active_frame_->render_pass_list.empty()) {
On 2017/02/07 22:46:19, vmpstr wrote:
> minor nit: while here, I think this would read better as
> 
> if (!active_frame_ || active_frame_->render_pass_list.empty()) {
>   copy_request->SendEmptyResult();
>   return;
> }
> 
> ...

Done.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:160: ActivatePendingFrame();
On 2017/02/07 22:46:19, vmpstr wrote:
> Are we guaranteed to have a pending frame at this point?

We got this far, which means we had a blocker (and thus a pending frame), that
blocker was cleared, and now we can activate the pending frame so I think we are
guaranteed to have a pending frame at this point. I have a DCHECK in
ActivatePendingFrame.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:188: void Surface::ActivateFrame(CompositorFrame frame) {
On 2017/02/07 22:46:19, vmpstr wrote:
> Can you add a comment at the top of the function to just outline what it takes
> to activate a frame

Done.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:197: if (active_frame_)
On 2017/02/07 22:46:19, vmpstr wrote:
> In what situations can we not have an active_frame_ here?
> 
> Also, you can combine this if with the next one
> 
> if (active_frame) {
>   ...
>   if (!active_frame->render_pass_list_empty())
>     ...
> }

Heh, We just set the active_frame_ so we should have an active_frame_. I think
this is cruft that carried over from old code.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.cc#...
cc/surfaces/surface.cc:219: if (active_frame_) {
On 2017/02/07 22:46:19, vmpstr wrote:
> nit:
> 
> if (!active_frame_)
>   return;
> 
> ...

Done.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.h
File cc/surfaces/surface.h (right):

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.h#n...
cc/surfaces/surface.h:60: void ReportSurfaceIdAvailable(const SurfaceId&
surface_id);
On 2017/02/07 22:46:19, vmpstr wrote:
> Notify?

Done.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.h#n...
cc/surfaces/surface.h:79: // Returns the currently pending frame.
On 2017/02/07 22:46:19, vmpstr wrote:
> Can you add a similar comment here as above, "must check HasPendingFrame" etc

Done.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.h#n...
cc/surfaces/surface.h:137: SurfaceDependencies blocked_on_surfaces_;
On 2017/02/07 22:46:19, vmpstr wrote:
> nit: "blocking_surfaces_"?

Done.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface.h#n...
cc/surfaces/surface.h:145: using PendingSurfaceSet =
std::unordered_set<Surface*>;
On 2017/02/07 22:46:19, vmpstr wrote:
> What's the expected size of these things? flat sets are landing soon, so I
> wonder if they could be used here.

Added a TODO.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
File cc/surfaces/surface_dependency_tracker.cc (right):

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
On 2017/02/07 22:46:19, vmpstr wrote:
> nit: 2017

Done.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:14: static constexpr uint32_t
MAX_BEGIN_FRAME_COUNT = 4;
On 2017/02/07 22:46:19, vmpstr wrote:
> nit: no need for static or no need for namespace {}
> nit: kMaxBeginFrameCount

Done.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:31: if
(!pending_surface->HasPendingFrame())
On 2017/02/07 22:46:20, vmpstr wrote:
> From reading the header comment, it seems that the surface wants to be
informed
> of something. Is that a strong requirement? If so I think you need to call
> something on it before returning :P
> 
> Either that or this should be a DCHECK?

Yes, this should be a DCHECK. Also renamed instances of pending_surface to
surface because pending_surface sounds weird here because the surface may have
an active frame too.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:36: // Referenced surface IDs that
aren't currently known to the surface manager
On 2017/02/07 22:46:19, vmpstr wrote:
> I guess I'm misunderstanding a bit, but is
> 
> "known to the surface manager" imply both surface is in there _and_ it has an
> active frame? Specifically, if it _doesn't_ have an active frame, does that
mean
> the surface is not known?

Yes, I've added an extra bit to clarify. Thanks!

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:58: // scope deadlines to surface
subtrees. We cannot do that until
On 2017/02/07 22:46:19, vmpstr wrote:
> Can you file a bug for this and reference it here, please?

Done.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:70: if (has_deadline_) {
On 2017/02/07 22:46:19, vmpstr wrote:
> Instead of adding/removing |this| from the observers, do you think it's better
> to just always observe but early out if it has no deadline?
> 
> Also, how is it possible that we're in this function and has_deadline_ is
false?

I agree, that's easier. Also, eww, this looks like cruft from my prototype.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:84: if
(!pending_surface->HasPendingFrame())
On 2017/02/07 22:46:20, vmpstr wrote:
> Comment please

Done.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:94: if (it !=
blocking_surfaces_.end()) {
On 2017/02/07 22:46:19, vmpstr wrote:
> if (it == blocking_surface_.end())
>   continue;
> ...

Done.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:113: // TODO(fsamuel): We should
consider removing this surface as a dependency on
On 2017/02/07 22:46:19, vmpstr wrote:
> File a bug for this please and reference here (I know this is just an
> exploratory TODO, but it's easier to track progress on crbug)

Done.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.cc:133:
blocking_surfaces_[surface_id].erase(pending_surface);
On 2017/02/07 22:46:19, vmpstr wrote:
> Do a find and use the iterator for both erase and checking if empty and
erasing
> (less lookups)

Done.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
File cc/surfaces/surface_dependency_tracker.h (right):

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.h:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
On 2017/02/07 22:46:20, vmpstr wrote:
> nit: 2017

Done.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.h:76: BeginFrameSource*
begin_frame_source_;
On 2017/02/07 22:46:20, vmpstr wrote:
> should this be a const ptr as well?

Done.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_dep...
cc/surfaces/surface_dependency_tracker.h:78: // Whether or not a frame observer
has been added.
On 2017/02/07 22:46:20, vmpstr wrote:
> nit: Indicates whether...

Done.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_fac...
File cc/surfaces/surface_factory.cc (right):

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_fac...
cc/surfaces/surface_factory.cc:119: const CompositorFrame& frame =
surface->GetActiveFrame();
On 2017/02/07 22:46:20, vmpstr wrote:
> DCHECK that it hasactiveframe

Done.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_fac...
cc/surfaces/surface_factory.cc:120: // CompositorFrames may not be populated
with a RenderPass in unit tests.
On 2017/02/07 22:46:20, vmpstr wrote:
> nit: this reads like unittests are not allowed to populate it with a render
> pass... maybe s/may/might/? *shrug*

Done.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_fac...
cc/surfaces/surface_factory.cc:129: surface->RemoveObserver(this);
On 2017/02/07 22:46:20, vmpstr wrote:
> Is there a doc describing who observes whom and for how long and why? This
patch
> looks to be adding a few more places where we observe or not observe, and I
> think having a central doc with all of the relationships might be good for
> posterity.

SurfaceFactory is going away soon, but SurfaceDependencyTracker observes
surfaces too. I think that's about it. I'll write a quick summary too, but I
fear it'll go out of date very quickly.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_id_...
File cc/surfaces/surface_id_allocator.cc (right):

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_id_...
cc/surfaces/surface_id_allocator.cc:19: const LocalSurfaceId&
SurfaceIdAllocator::GenerateId() {
On 2017/02/07 22:46:20, vmpstr wrote:
> Why this change?

It's not necessary for now. I've reverted it.

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_man...
File cc/surfaces/surface_manager.cc (right):

https://codereview.chromium.org/2676373004/diff/80001/cc/surfaces/surface_man...
cc/surfaces/surface_manager.cc:161: // if (surface_map_.count(child_id) == 0) {
On 2017/02/07 22:46:20, vmpstr wrote:
> <_<

Heh fixed in a later patchset. Sorry about that.

[commit-bot: I haz the power, 2017-02-08 00:53:44]

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

[Fady Samuel, 2017-02-08 01:18:31]

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

[commit-bot: I haz the power, 2017-02-08 01:18:58]

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, 2017-02-08 02:08:00]

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

[commit-bot: I haz the power, 2017-02-08 02:08:01]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Fady Samuel, 2017-02-08 02:56:16]

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

[commit-bot: I haz the power, 2017-02-08 02:57:06]

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

[Fady Samuel, 2017-02-08 04:14:53]

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

[commit-bot: I haz the power, 2017-02-08 04:15:08]

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, 2017-02-08 05:59:40]

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

[commit-bot: I haz the power, 2017-02-08 05:59:41]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2017-02-08 16:22:24]

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

[commit-bot: I haz the power, 2017-02-08 16:22:47]

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

[Fady Samuel, 2017-02-08 16:30:41]

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

[commit-bot: I haz the power, 2017-02-08 16:31:45]

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, 2017-02-08 18:38:34]

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

[commit-bot: I haz the power, 2017-02-08 18:38:35]

Dry run: This issue passed the CQ dry run.

[vmpstr, 2017-02-08 19:25:40]

https://codereview.chromium.org/2676373004/diff/250001/cc/output/compositor_f...
File cc/output/compositor_frame_metadata.h (right):

https://codereview.chromium.org/2676373004/diff/250001/cc/output/compositor_f...
cc/output/compositor_frame_metadata.h:83: bool can_activate_without_dependencies
= true;
Based on the comment, I think the variable name is ambiguous

"can activate without dependencies" could also mean if there are no
dependencies, this can activate. I don't mind the name, but if there's something
better, then I'd prefer that :P

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/pending_fr...
File cc/surfaces/pending_frame_observer.h (right):

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/pending_fr...
cc/surfaces/pending_frame_observer.h:34: virtual ~PendingFrameObserver() {}
= default?

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:138:
!pending_frame_->metadata.can_activate_without_dependencies)
nit: braces pls

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:148: DCHECK(!!pending_frame_);
just DCHECK(pending_frame_) is good I think? Or do you need !! for optional
stuff?

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:190: if (!factory_->manager()->dependency_tracker()) {
Does this also imply that there are no observers_? If so can you DCHECK that. If
not, then do we need to call OnSurfaceDependenciesChanged here?

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_de...
File cc/surfaces/surface_dependency_tracker.cc (right):

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_de...
cc/surfaces/surface_dependency_tracker.cc:36:
pending_frame.metadata.can_activate_without_dependencies;
oh, maybe "can_activate_before_dependencies" :D

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_de...
cc/surfaces/surface_dependency_tracker.cc:42: Surface* surface_dep =
surface_manager_->GetSurfaceForId(surface_id);
s/surface_dep/dependency/

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_de...
cc/surfaces/surface_dependency_tracker.cc:44:
blocking_surfaces_[surface_id].insert(surface);
this to me says if the dependency isn't satisfied, then then given surface is a
blocking surface. Should this be "blocked" surface? Or am I misunderstanding
this?

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_de...
cc/surfaces/surface_dependency_tracker.cc:52: if (needs_begin_frame) {
When can RequestSurfaceResolution be called? My worry here is that while you're
waiting for kMaxBeginFrameCount begin frames, if we call this function once a
frame, then you would always reset begin_frame_count and never reach your count.

Maybe this if should also say && !has_deadline_? If so, this could also use a
unittest probably.

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_de...
cc/surfaces/surface_dependency_tracker.cc:78: begin_frame_count_ = 0;
You might hate the idea, but having a bool and an int that is only valid when
the bool is true is two variables that need to be updated together (for example
you clear begin frame count both here and in the above function).

Would it be better to have something like
base::Optional<int> deadline_begin_frame_count_; where if it's ==
base::nullopt_t then it's equivalent to "has_deadline_ == false"?

I don't mind what you have either, so up to you.

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_un...
File cc/surfaces/surface_unittest.cc (right):

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_un...
cc/surfaces/surface_unittest.cc:115: TEST(SurfaceTest,
DisplayCompositorLocking2) {
Can you name the tests something more descriptive? something like

DisplayCompositorLockingBlockedChain

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_un...
cc/surfaces/surface_unittest.cc:185: TEST(SurfaceTest,
DisplayCompositorLocking3) {
DisplayCompositorLockingTwoBlockedOnOne

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_un...
cc/surfaces/surface_unittest.cc:254: TEST(SurfaceTest,
DisplayCompositorLocking4) {
DisplayCompositorLockingDeadlineHits

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_un...
cc/surfaces/surface_unittest.cc:309: begin_frame_source->TestOnBeginFrame(args);
Can you make this into a for loop that checks the expect falses (above) for each
on begin frame until the last one?

Also, can you add a test where we create(?) more surfaces between each begin
frame? (I mentioned this in the other file)

[rjkroege, 2017-02-08 21:52:30]

https://codereview.chromium.org/2676373004/diff/250001/cc/output/compositor_f...
File cc/output/compositor_frame_metadata.h (right):

https://codereview.chromium.org/2676373004/diff/250001/cc/output/compositor_f...
cc/output/compositor_frame_metadata.h:83: bool can_activate_without_dependencies
= true;
how about activation_can_ignore_missing_dependencies

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/pending_fr...
File cc/surfaces/pending_frame_observer.h (right):

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/pending_fr...
cc/surfaces/pending_frame_observer.h:18: class CC_SURFACES_EXPORT
PendingFrameObserver {
Add class comment

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_de...
File cc/surfaces/surface_dependency_tracker.h (right):

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_de...
cc/surfaces/surface_dependency_tracker.h:20: // dependent surface ID to a set of
Surfaces that have CompositorFrames
from a dependent

[Fady Samuel, 2017-02-08 23:39:58]

PTAL Vlad! :D

https://codereview.chromium.org/2676373004/diff/250001/cc/output/compositor_f...
File cc/output/compositor_frame_metadata.h (right):

https://codereview.chromium.org/2676373004/diff/250001/cc/output/compositor_f...
cc/output/compositor_frame_metadata.h:83: bool can_activate_without_dependencies
= true;
On 2017/02/08 19:25:39, vmpstr wrote:
> Based on the comment, I think the variable name is ambiguous
> 
> "can activate without dependencies" could also mean if there are no
> dependencies, this can activate. I don't mind the name, but if there's
something
> better, then I'd prefer that :P

Changed to can_activate_before_dependencies.

https://codereview.chromium.org/2676373004/diff/250001/cc/output/compositor_f...
cc/output/compositor_frame_metadata.h:83: bool can_activate_without_dependencies
= true;
On 2017/02/08 21:52:29, rjkroege wrote:
> how about activation_can_ignore_missing_dependencies

I've changed to can_activate_before_dependencies...more or less says the same
thing.

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/pending_fr...
File cc/surfaces/pending_frame_observer.h (right):

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/pending_fr...
cc/surfaces/pending_frame_observer.h:18: class CC_SURFACES_EXPORT
PendingFrameObserver {
On 2017/02/08 21:52:29, rjkroege wrote:
> Add class comment

Done.

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/pending_fr...
cc/surfaces/pending_frame_observer.h:34: virtual ~PendingFrameObserver() {}
On 2017/02/08 19:25:39, vmpstr wrote:
> = default?

Done.

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:138:
!pending_frame_->metadata.can_activate_without_dependencies)
On 2017/02/08 19:25:39, vmpstr wrote:
> nit: braces pls

Done.

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:148: DCHECK(!!pending_frame_);
On 2017/02/08 19:25:39, vmpstr wrote:
> just DCHECK(pending_frame_) is good I think? Or do you need !! for optional
> stuff?

Done.

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:190: if (!factory_->manager()->dependency_tracker()) {
On 2017/02/08 19:25:39, vmpstr wrote:
> Does this also imply that there are no observers_? If so can you DCHECK that.
If
> not, then do we need to call OnSurfaceDependenciesChanged here?

Only SurfaceDependencyTracker cares about SurfaceDependenciesChanged. If we
don't have a SurfaceDependencyTracker then we always activate. SurfaceFactory
still observes OnSurfaceActivated though.

I'm not making changes as a result.

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_de...
File cc/surfaces/surface_dependency_tracker.cc (right):

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_de...
cc/surfaces/surface_dependency_tracker.cc:36:
pending_frame.metadata.can_activate_without_dependencies;
On 2017/02/08 19:25:40, vmpstr wrote:
> oh, maybe "can_activate_before_dependencies" :D

Done.

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_de...
cc/surfaces/surface_dependency_tracker.cc:42: Surface* surface_dep =
surface_manager_->GetSurfaceForId(surface_id);
On 2017/02/08 19:25:39, vmpstr wrote:
> s/surface_dep/dependency/

Done.

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_de...
cc/surfaces/surface_dependency_tracker.cc:44:
blocking_surfaces_[surface_id].insert(surface);
On 2017/02/08 19:25:40, vmpstr wrote:
> this to me says if the dependency isn't satisfied, then then given surface is
a
> blocking surface. Should this be "blocked" surface? Or am I misunderstanding
> this?

If the dependency isn't satisfied, then it blocks |surface|. Yes, I think
|blocked_surfaces| is probably a better name, now that you mention it. Added a
comment on the member variable.

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_de...
cc/surfaces/surface_dependency_tracker.cc:52: if (needs_begin_frame) {
On 2017/02/08 19:25:40, vmpstr wrote:
> When can RequestSurfaceResolution be called? My worry here is that while
you're
> waiting for kMaxBeginFrameCount begin frames, if we call this function once a
> frame, then you would always reset begin_frame_count and never reach your
count.
> 
> Maybe this if should also say && !has_deadline_? If so, this could also use a
> unittest probably.

Done.

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_de...
cc/surfaces/surface_dependency_tracker.cc:78: begin_frame_count_ = 0;
On 2017/02/08 19:25:39, vmpstr wrote:
> You might hate the idea, but having a bool and an int that is only valid when
> the bool is true is two variables that need to be updated together (for
example
> you clear begin frame count both here and in the above function).
> 
> Would it be better to have something like
> base::Optional<int> deadline_begin_frame_count_; where if it's ==
> base::nullopt_t then it's equivalent to "has_deadline_ == false"?
> 
> I don't mind what you have either, so up to you.

added base::Optional<uint32_t> frames_since_deadline_set_;

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_de...
File cc/surfaces/surface_dependency_tracker.h (right):

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_de...
cc/surfaces/surface_dependency_tracker.h:20: // dependent surface ID to a set of
Surfaces that have CompositorFrames
On 2017/02/08 21:52:29, rjkroege wrote:
> from a dependent

Done.

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_un...
File cc/surfaces/surface_unittest.cc (right):

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_un...
cc/surfaces/surface_unittest.cc:115: TEST(SurfaceTest,
DisplayCompositorLocking2) {
On 2017/02/08 19:25:40, vmpstr wrote:
> Can you name the tests something more descriptive? something like
> 
> DisplayCompositorLockingBlockedChain

Done.

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_un...
cc/surfaces/surface_unittest.cc:185: TEST(SurfaceTest,
DisplayCompositorLocking3) {
On 2017/02/08 19:25:40, vmpstr wrote:
> DisplayCompositorLockingTwoBlockedOnOne

Done.

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_un...
cc/surfaces/surface_unittest.cc:254: TEST(SurfaceTest,
DisplayCompositorLocking4) {
On 2017/02/08 19:25:40, vmpstr wrote:
> DisplayCompositorLockingDeadlineHits

Done.

https://codereview.chromium.org/2676373004/diff/250001/cc/surfaces/surface_un...
cc/surfaces/surface_unittest.cc:309: begin_frame_source->TestOnBeginFrame(args);
On 2017/02/08 19:25:40, vmpstr wrote:
> Can you make this into a for loop that checks the expect falses (above) for
each
> on begin frame until the last one?
> 
> Also, can you add a test where we create(?) more surfaces between each begin
> frame? (I mentioned this in the other file)

Done.

[Fady Samuel, 2017-02-08 23:46:42]

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

[commit-bot: I haz the power, 2017-02-08 23:47:22]

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

[vmpstr, 2017-02-08 23:47:46]

lgtm

https://codereview.chromium.org/2676373004/diff/280001/cc/surfaces/surface_un...
File cc/surfaces/surface_unittest.cc (right):

https://codereview.chromium.org/2676373004/diff/280001/cc/surfaces/surface_un...
cc/surfaces/surface_unittest.cc:48: TEST(SurfaceTest, DisplayCompositorLocking1)
{
nit: DisplayCompositorLockingOneBlockedOnTwo

[commit-bot: I haz the power, 2017-02-08 23:51:51]

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

[commit-bot: I haz the power, 2017-02-08 23:51:54]

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

[Fady Samuel, 2017-02-09 00:06:00]

fsamuel@chromium.org changed reviewers:
+ reveman@chromium.org, tsepez@chromium.org

[Fady Samuel, 2017-02-09 00:06:01]

+tsepez@ for cc/ipc
+reveman@ for exo (TBR'ing).

[Fady Samuel, 2017-02-09 00:06:22]

Description was changed from

==========
Implement service-side surface synchronization

This CL introduces the notion of a pending and active CompositorFrame 
along with surface dependency tracking and resolution.

An active CompositorFrame is a frame that is a candidate for display. A
CompositorFrame is active if all its surface dependencies are active, 
or the display compositor explicitly activated it after some set 
deadline.

A pending CompositorFrame is a frame that has unresolved dependencies: 
it refers to surface IDs that either do not yet exist in the display 
compositor or don't have an active frame.

Surface::QueueFrame determines whether a frame refers to valid surface 
IDs or not. If a CompositorFrame has unresolved dependencies then it's
marked as pending and the SurfaceDependencyTracker is asked to track
the resolution of its dependencies. SurfaceDependencyTracker will call
back into the Surface to report when a dependency has been activated.

For the time being (until SurfaceReference work is complete), the first 
CompositorFrame submitted with unresolved dependencies will trigger a 
global deadline. The deadline will be cleared if all dependencies are
resolved within the alloted time window. If dependencies are not 
resolved before the deadline, then SurfaceDependencyTracker will 
forcibly activate currently pending frames.

BUG=672962

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Implement service-side surface synchronization

This CL introduces the notion of a pending and active CompositorFrame 
along with surface dependency tracking and resolution.

An active CompositorFrame is a frame that is a candidate for display. A
CompositorFrame is active if all its surface dependencies are active, 
or the display compositor explicitly activated it after some set 
deadline.

A pending CompositorFrame is a frame that has unresolved dependencies: 
it refers to surface IDs that either do not yet exist in the display 
compositor or don't have an active frame.

Surface::QueueFrame determines whether a frame refers to valid surface 
IDs or not. If a CompositorFrame has unresolved dependencies then it's
marked as pending and the SurfaceDependencyTracker is asked to track
the resolution of its dependencies. SurfaceDependencyTracker will call
back into the Surface to report when a dependency has been activated.

For the time being (until SurfaceReference work is complete), the first 
CompositorFrame submitted with unresolved dependencies will trigger a 
global deadline. The deadline will be cleared if all dependencies are
resolved within the alloted time window. If dependencies are not 
resolved before the deadline, then SurfaceDependencyTracker will 
forcibly activate currently pending frames.

BUG=672962
TBR=reveman@chromium.org for trivial exo change.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2017-02-09 00:06:30]

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

[commit-bot: I haz the power, 2017-02-09 00:07:00]

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

[Fady Samuel, 2017-02-09 00:09:19]

Description was changed from

==========
Implement service-side surface synchronization

This CL introduces the notion of a pending and active CompositorFrame 
along with surface dependency tracking and resolution.

An active CompositorFrame is a frame that is a candidate for display. A
CompositorFrame is active if all its surface dependencies are active, 
or the display compositor explicitly activated it after some set 
deadline.

A pending CompositorFrame is a frame that has unresolved dependencies: 
it refers to surface IDs that either do not yet exist in the display 
compositor or don't have an active frame.

Surface::QueueFrame determines whether a frame refers to valid surface 
IDs or not. If a CompositorFrame has unresolved dependencies then it's
marked as pending and the SurfaceDependencyTracker is asked to track
the resolution of its dependencies. SurfaceDependencyTracker will call
back into the Surface to report when a dependency has been activated.

For the time being (until SurfaceReference work is complete), the first 
CompositorFrame submitted with unresolved dependencies will trigger a 
global deadline. The deadline will be cleared if all dependencies are
resolved within the alloted time window. If dependencies are not 
resolved before the deadline, then SurfaceDependencyTracker will 
forcibly activate currently pending frames.

BUG=672962
TBR=reveman@chromium.org for trivial exo change.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Implement service-side surface synchronization

This CL introduces the notion of a pending and active CompositorFrame 
along with surface dependency tracking and resolution.

An active CompositorFrame is a frame that is a candidate for display. A
CompositorFrame is active if all its surface dependencies are active, 
or the display compositor explicitly activated it after some set 
deadline.

A pending CompositorFrame is a frame that has unresolved dependencies: 
it refers to surface IDs that either do not yet exist in the display 
compositor or don't have an active frame.

Surface::QueueFrame determines whether a frame refers to valid surface 
IDs or not. If a CompositorFrame has unresolved dependencies then it's
marked as pending and the SurfaceDependencyTracker is asked to track
the resolution of its dependencies. SurfaceDependencyTracker will call
back into the Surface to report when a dependency has been activated.

For the time being (until SurfaceReference work is complete), the first 
CompositorFrame submitted with unresolved dependencies will trigger a 
global deadline. The deadline will be cleared if all dependencies are
resolved within the alloted time window. If dependencies are not 
resolved before the deadline, then SurfaceDependencyTracker will 
forcibly activate currently pending frames.

BUG=672962
TBR=reveman@chromium.org for trivial exo change.
TBR=tsepez@chromium.org for minor ipc change.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[reveman, 2017-02-09 00:41:10]

components/exo lgtm

[commit-bot: I haz the power, 2017-02-09 01:29:05]

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

[commit-bot: I haz the power, 2017-02-09 01:29:06]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2017-02-09 01:30:49]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2017-02-09 01:30:49]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/2676373004/#ps300001
(title: "Better unit test name")

[commit-bot: I haz the power, 2017-02-09 01:31:30]

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, 2017-02-09 01:38:39]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1486603849061930,
"parent_rev": "632e302014aaccf6482edb896c409221e0a9b6cc", "commit_rev":
"cb618a172eafc9c9d902ff7257a8e72d8bb764d7"}

[commit-bot: I haz the power, 2017-02-09 01:39:16]

Description was changed from

==========
Implement service-side surface synchronization

This CL introduces the notion of a pending and active CompositorFrame 
along with surface dependency tracking and resolution.

An active CompositorFrame is a frame that is a candidate for display. A
CompositorFrame is active if all its surface dependencies are active, 
or the display compositor explicitly activated it after some set 
deadline.

A pending CompositorFrame is a frame that has unresolved dependencies: 
it refers to surface IDs that either do not yet exist in the display 
compositor or don't have an active frame.

Surface::QueueFrame determines whether a frame refers to valid surface 
IDs or not. If a CompositorFrame has unresolved dependencies then it's
marked as pending and the SurfaceDependencyTracker is asked to track
the resolution of its dependencies. SurfaceDependencyTracker will call
back into the Surface to report when a dependency has been activated.

For the time being (until SurfaceReference work is complete), the first 
CompositorFrame submitted with unresolved dependencies will trigger a 
global deadline. The deadline will be cleared if all dependencies are
resolved within the alloted time window. If dependencies are not 
resolved before the deadline, then SurfaceDependencyTracker will 
forcibly activate currently pending frames.

BUG=672962
TBR=reveman@chromium.org for trivial exo change.
TBR=tsepez@chromium.org for minor ipc change.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Implement service-side surface synchronization

This CL introduces the notion of a pending and active CompositorFrame 
along with surface dependency tracking and resolution.

An active CompositorFrame is a frame that is a candidate for display. A
CompositorFrame is active if all its surface dependencies are active, 
or the display compositor explicitly activated it after some set 
deadline.

A pending CompositorFrame is a frame that has unresolved dependencies: 
it refers to surface IDs that either do not yet exist in the display 
compositor or don't have an active frame.

Surface::QueueFrame determines whether a frame refers to valid surface 
IDs or not. If a CompositorFrame has unresolved dependencies then it's
marked as pending and the SurfaceDependencyTracker is asked to track
the resolution of its dependencies. SurfaceDependencyTracker will call
back into the Surface to report when a dependency has been activated.

For the time being (until SurfaceReference work is complete), the first 
CompositorFrame submitted with unresolved dependencies will trigger a 
global deadline. The deadline will be cleared if all dependencies are
resolved within the alloted time window. If dependencies are not 
resolved before the deadline, then SurfaceDependencyTracker will 
forcibly activate currently pending frames.

BUG=672962
TBR=reveman@chromium.org for trivial exo change.
TBR=tsepez@chromium.org for minor ipc change.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2676373004
Cr-Commit-Position: refs/heads/master@{#449182}
Committed:
https://chromium.googlesource.com/chromium/src/+/cb618a172eafc9c9d902ff7257a8...
==========

[commit-bot: I haz the power, 2017-02-09 01:39:17]

Committed patchset #17 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/cb618a172eafc9c9d902ff7257a8...

[kylechar, 2017-02-09 15:50:32]

Didn't notice it was submitted before looking it over. Here are a couple of
comment/include issues I noticed if you're modifying the files again in a
subsequent CL.

https://codereview.chromium.org/2676373004/diff/300001/cc/surfaces/surface.cc
File cc/surfaces/surface.cc (right):

https://codereview.chromium.org/2676373004/diff/300001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:15: #include "cc/surfaces/pending_frame_observer.h"
Duplicate include.

https://codereview.chromium.org/2676373004/diff/300001/cc/surfaces/surface.cc...
cc/surfaces/surface.cc:196: base::flat_set<SurfaceId> new_blocking_surfaces;
Why mix SurfaceDependencies and base::flat_set<SurfaceId>?

https://codereview.chromium.org/2676373004/diff/300001/cc/surfaces/surface.h
File cc/surfaces/surface.h (right):

https://codereview.chromium.org/2676373004/diff/300001/cc/surfaces/surface.h#...
cc/surfaces/surface.h:115: void ActivatePendingFrame();
What does ActivePendingFrame() do exactly? Can you leave a comment.

https://codereview.chromium.org/2676373004/diff/300001/cc/surfaces/surface_de...
File cc/surfaces/surface_dependency_tracker.cc (right):

https://codereview.chromium.org/2676373004/diff/300001/cc/surfaces/surface_de...
cc/surfaces/surface_dependency_tracker.cc:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
2017

https://codereview.chromium.org/2676373004/diff/300001/cc/surfaces/surface_de...
cc/surfaces/surface_dependency_tracker.cc:7: #include "cc/surfaces/surface.h"
Duplicate include.

https://codereview.chromium.org/2676373004/diff/300001/cc/surfaces/surface_de...
cc/surfaces/surface_dependency_tracker.cc:15: }
}  // namespace

https://codereview.chromium.org/2676373004/diff/300001/cc/surfaces/surface_de...
File cc/surfaces/surface_dependency_tracker.h (right):

https://codereview.chromium.org/2676373004/diff/300001/cc/surfaces/surface_de...
cc/surfaces/surface_dependency_tracker.h:8: #include
"cc/scheduler/begin_frame_source.h"
#include "base/macros.h"