[cc] Track observer status in ExternalBeginFrameSource.

[cc] Track observer status in ExternalBeginFrameSource.

This introduces a BeginFrameObserverAckTracker, which tracks which
active observers have finished the current BeginFrame, and hooks it up
to ExternalBeginFrameSource.

BUG=646774, 401331
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

[Eric Seckler, 2017-02-09 23:30:49]

Description was changed from

==========
[cc] Track observer status in ExternalBeginFrameSource.

This introduces a BeginFrameObserverAckTracker, which tracks which
active observers have finished the current BeginFrame, and hooks it up
to ExternalBeginFrameSource.

BUG=646774, 401331
==========

to

==========
[cc] Track observer status in ExternalBeginFrameSource.

This introduces a BeginFrameObserverAckTracker, which tracks which
active observers have finished the current BeginFrame, and hooks it up
to ExternalBeginFrameSource.

BUG=646774, 401331
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Eric Seckler, 2017-02-09 23:31:10]

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

[commit-bot: I haz the power, 2017-02-09 23:31:39]

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-09 23:57:07]

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

[commit-bot: I haz the power, 2017-02-09 23:57:10]

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

[Eric Seckler, 2017-02-10 00:20:49]

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

[commit-bot: I haz the power, 2017-02-10 00:21:40]

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

[Eric Seckler, 2017-02-10 00:26:54]

eseckler@chromium.org changed reviewers:
+ brianderson@chromium.org, skyostil@chromium.org

[Eric Seckler, 2017-02-10 00:26:55]

Sami + Brian, PTAL :)

Sorry for adding yet another patch to the pile, Brian!

[commit-bot: I haz the power, 2017-02-10 00:46:40]

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

[commit-bot: I haz the power, 2017-02-10 00:46:41]

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

[Eric Seckler, 2017-02-10 00:52:10]

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

[commit-bot: I haz the power, 2017-02-10 00:52:36]

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

[Eric Seckler, 2017-02-10 00:52:49]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-02-10 01:15:49]

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

[commit-bot: I haz the power, 2017-02-10 01:15:50]

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

[Eric Seckler, 2017-02-10 01:26:06]

Patchset #2 (id:40001) has been deleted

[Eric Seckler, 2017-02-10 01:26:11]

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

[commit-bot: I haz the power, 2017-02-10 01:26:38]

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-10 03:34:05]

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

[commit-bot: I haz the power, 2017-02-10 03:34:06]

Dry run: This issue passed the CQ dry run.

[Sami, 2017-02-10 14:14:28]

lgtm

https://codereview.chromium.org/2691453002/diff/60001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/2691453002/diff/60001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:266: std::set<BeginFrameObserver*> observers_;
nit: unordered_set for both of these?
edit: Ah, I guess this is required by base::STLIncludes. Nevermind then.

[brianderson, 2017-02-15 20:03:33]

brianderson@chromium.org changed reviewers:
+ enne@chromium.org

[brianderson, 2017-02-15 20:03:34]

+enne

[brianderson, 2017-02-15 20:21:36]

https://codereview.chromium.org/2691453002/diff/60001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/2691453002/diff/60001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:266: std::set<BeginFrameObserver*> observers_;
On 2017/02/10 14:14:28, Sami wrote:
> nit: unordered_set for both of these?
> edit: Ah, I guess this is required by base::STLIncludes. Nevermind then.

Probably would be good to add a comment explaining why order matters.

https://codereview.chromium.org/2691453002/diff/60001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:310: BeginFrameObserverAckTracker
ack_tracker_;
Are there any instances of the ExternalBeginFrameSource where this is useful?

Seems like it would be more useful to add it to the BeginFrameSource(s) used by
cc::Display.

[brianderson, 2017-02-15 20:50:48]

https://codereview.chromium.org/2691453002/diff/60001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/2691453002/diff/60001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:264: if
(obs->LastUsedBeginFrameArgs().sequence_number != current_sequence_number_)
Why is this != instead of ==?

https://codereview.chromium.org/2691453002/diff/60001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:325: return
base::STLIncludes(finished_observers_, observers_);
It looks like STLIncludes depends on std::includes which takes generic
InputIterators where it can't assume std::distance will be constant time, so
it's unlikely std::include can do an efficient early out based on the sizes of
the two containers.

It might help to start this method with:
if (finished_obervers.size() < observers.size())
  return false;

Also, if we can guarantee that we only get OnObserverFinishedFrame from
observers that are currently observing, we might be able to just return
(finished_observers.size == observers.size()). Any idea if that's the case given
the current state of the code? I assume not given the request+response is async.

[enne (OOO), 2017-02-15 23:12:25]

(Apologies for length.  If I'm entirely off base and misunderstanding, please
let me know!)

I'm a little confused on the design here.  It seems a little overly generic for
ack tracking on external BFS.  (What if all of them don't produce frames and are
just listening?)  I would have expected the ack tracking to be more at the top
level than at the BFS level.

Here's my understanding of begin frames, ignoring mus and just talking about the
aura case.  GPTF owns a synthetic BFS (which soon might alternately be an
external BFS for Windows).  This gets handed to SurfaceManager, which hands it
off to SurfaceFactoryClients.  For things like non-oopif renderers,
RenderWidgetHostViewAura observes this top level BFS and then forwards begin
frame events via IPCs to the renderer, which turns those ipcs into a
CompositorExternalBeginFrameSource (which wraps an ExternalBeginFrameSource). 
cc::Scheduler in the renderer observes this CompositorExternalBeginFrameSource. 
ui::Compositor and the DisplayScheduler both observe the top level BFS given to
the Display from the GPTF.

With that understanding, it seems like we don't really need complex ack tracking
on the renderer ExternalBeginFrameSource since cc::Scheduler is the only
observer that counts (and is maybe the only observer ever?)  It also seems like
the top level browser BFS is also not always an external BFS either, so it's a
bit awkward to have this logic tied to external BFS.  It seems like this would
miss ui::Compositor, for example.  I think this is maybe what brianderson is
getting at with his comments.

To be honest, it feels like we have two competing systems here: surfaces and
begin frames.  Begin frames are the way to say "yes please produce a frame" and
surfaces are the "ok here is your frame" response.  And acks are additional
information to say "yes I handled your begin frame" as well.   I think
BeginFrameAck the structure is good, and provides information that the top level
needs for scheduling and that headless needs for control.  I'm just less clear
on what the plumbing should be to send them back.  If delivered separately
through BFS, it seems like there's somewhat of a potential race (or an ordering
dependency in the renderer, at least) if the acks get used in the
DisplayScheduler to kick a frame but then the surface hasn't actually been
delivered yet.  Is returning the ack a better fit for the CompositorFrameSink
interface, since that's where the surface is also returned?

+fsamuel for surface-y thoughts

https://codereview.chromium.org/2691453002/diff/60001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/2691453002/diff/60001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:266: std::set<BeginFrameObserver*> observers_;
On 2017/02/15 at 20:21:36, brianderson wrote:
> On 2017/02/10 14:14:28, Sami wrote:
> > nit: unordered_set for both of these?
> > edit: Ah, I guess this is required by base::STLIncludes. Nevermind then.
> 
> Probably would be good to add a comment explaining why order matters.

Maybe flat_set instead? (And SmallMap instead of unordered_map?) How many
observers do you expect to have?

[Eric Seckler, 2017-02-16 00:04:55]

The general design stems from https://codereview.chromium.org/2527283003/.

The tracker introduced in this patch is used both for ExternalBFS and
(eventually) within DisplayScheduler to track which observers have completed a
BeginFrame. My goal is to replace the DisplayScheduler's heuristics about child
surface damages with this sort of ack tracking - ideally on all platforms.

It is correct that CompositorExternalBFS in the renderer only has a single
observer, which is the cc::Scheduler. So all this fuss with the tracker may well
be over-engineered for that particular ExternalBFS :)

What makes things more complicated are other places that ExternalBFSs are used
(mus / exo). The common pattern is that a BeginFrameObserver consumes a
BeginFrame, passes it on to a CompositorFrameSink elsewhere, where it feeds into
an ExternalBFS, which in turn feeds a number of CompositorFrame producers, not
necessarily just one (I assume - Fady?). The patch [1], which was recently
submitted but rolled back, is the latest such example.

To decide when to acknowledge a BeginFrame from within such a
CompositorFrameSink, it needs to track when all observers of its ExternalBFS
have acked. For this purpose, it seemed to make sense to use the same
ack-tracking mechanism in DisplayScheduler and the CompositorFrameSinks'
ExternalBFSs.

[1] https://codereview.chromium.org/2612083002

On 2017/02/15 23:12:25, enne wrote:
> (Apologies for length.  If I'm entirely off base and misunderstanding, please
> let me know!)
> 
> I'm a little confused on the design here.  It seems a little overly generic
for
> ack tracking on external BFS.  (What if all of them don't produce frames and
are
> just listening?)  I would have expected the ack tracking to be more at the top
> level than at the BFS level.
> 
> Here's my understanding of begin frames, ignoring mus and just talking about
the
> aura case.  GPTF owns a synthetic BFS (which soon might alternately be an
> external BFS for Windows).  This gets handed to SurfaceManager, which hands it
> off to SurfaceFactoryClients.  For things like non-oopif renderers,
> RenderWidgetHostViewAura observes this top level BFS and then forwards begin
> frame events via IPCs to the renderer, which turns those ipcs into a
> CompositorExternalBeginFrameSource (which wraps an ExternalBeginFrameSource). 
> cc::Scheduler in the renderer observes this
CompositorExternalBeginFrameSource. 
> ui::Compositor and the DisplayScheduler both observe the top level BFS given
to
> the Display from the GPTF.
> 
> With that understanding, it seems like we don't really need complex ack
tracking
> on the renderer ExternalBeginFrameSource since cc::Scheduler is the only
> observer that counts (and is maybe the only observer ever?)  It also seems
like
> the top level browser BFS is also not always an external BFS either, so it's a
> bit awkward to have this logic tied to external BFS.  It seems like this would
> miss ui::Compositor, for example.  I think this is maybe what brianderson is
> getting at with his comments.
> 
> To be honest, it feels like we have two competing systems here: surfaces and
> begin frames.  Begin frames are the way to say "yes please produce a frame"
and
> surfaces are the "ok here is your frame" response.  And acks are additional
> information to say "yes I handled your begin frame" as well.   I think
> BeginFrameAck the structure is good, and provides information that the top
level
> needs for scheduling and that headless needs for control.  I'm just less clear
> on what the plumbing should be to send them back.  If delivered separately
> through BFS, it seems like there's somewhat of a potential race (or an
ordering
> dependency in the renderer, at least) if the acks get used in the
> DisplayScheduler to kick a frame but then the surface hasn't actually been
> delivered yet.  Is returning the ack a better fit for the CompositorFrameSink
> interface, since that's where the surface is also returned?

In my prototype patch (see above), I deliver the BeginFrameAcks together with
the CompositorFrame to avoid such races - acknowledgement always happens after
submission of the CompositorFrame.

For cases when no new CompositorFrame was produced for a BeginFrame, we need to
deliver the ack by itself instead, via a separate IPC/mojo message where
necessary.

[brianderson, 2017-02-16 00:12:02]

> To be honest, it feels like we have two competing systems here: surfaces and
begin frames.

We discussed this with Fady while he was here for BlinkOn. Here's a summary of
what I think the plan is currently:

Long term, it makes sense for headless to use the Surface ID synchronization
mechanisms intended for resize - but that is still in the design phase.

Near term, headless will rely on BeginFrameAcks for synchronization since that
is faster to implement and will also be useful for scheduling every frame that
doesn't have a resize.

Currently, Surface ID based synchronization plans to force child surfaces to
draw even if there is no damage; whereas BeginFrameAcks can be sent even if
there is no damage.

Long term, Surface ID synchronization and BeginFrameAcks will co-exist. The
former being used when synchronization is required and the later for best-effort
synchronization / latency management. Surface synchronization will be set up by
the window manager and resolved by the display compositor; whereas
BeginFrameAcks will be set up *and* resolved by the display compositor. Once MUS
is enabled, that means the set up for Surface synchronization and BeginFrameAcks
will be in different processes.

The two definitely need to coordinate with each other to avoid racing.
Coordinating across processes with MUS will be a bit of a headache, but should
be easier to do today where everything is on the same thread. I think Eric plans
to piggy back the BeginFrameAck with the compositor frame when there is damage.

[enne (OOO), 2017-02-16 01:49:33]

Thanks.  Sorry I missed previous discussions and am out of the loop.

I think acks for synchronization are good.  I just didn't understand how
external BFS was the right place to integrate those acks.  Will other begin
frame source types grow this same logic?

[Eric Seckler, 2017-02-16 01:58:51]

On 2017/02/16 01:49:33, enne wrote:
> Thanks.  Sorry I missed previous discussions and am out of the loop.
Np, guess it makes sense to start looping you back in :)

> I think acks for synchronization are good.  I just didn't understand how
> external BFS was the right place to integrate those acks.  Will other begin
> frame source types grow this same logic?

Other BFSs aren't used "behind" observers, so don't need the ack-tracking logic.
ExternalBFS are an exception, since they receive BeginFrames that were issued by
a different BFS.

The only other place we'll need to track them is within the
Display/DisplayScheduler - In my prototype, I'm doing this via wrapping the BFS
provided to the Display and tracking inside that wrapper (see
display_begin_frame_source.h in https://codereview.chromium.org/2527283003/).

[enne (OOO), 2017-02-16 18:51:18]

Ok, so then my understanding is you're handling the top level BFS that the
Display is using separately in the display, no matter what type the BFS is, and
then any other "forwarding" BFS must be external and so you'll track that inside
the external BFS class and catch the rest of them.

I guess I am surprised to hear that a single CompositorFrameSink would have
multiple frame producers that would need to be tracked separately.   Maybe
fsamuel can explain?

[Fady Samuel, 2017-02-16 18:56:15]

On 2017/02/16 18:51:18, enne wrote:
> Ok, so then my understanding is you're handling the top level BFS that the
> Display is using separately in the display, no matter what type the BFS is,
and
> then any other "forwarding" BFS must be external and so you'll track that
inside
> the external BFS class and catch the rest of them.
> 
> I guess I am surprised to hear that a single CompositorFrameSink would have
> multiple frame producers that would need to be tracked separately.   Maybe
> fsamuel can explain?

I'm not really following what you mean?

The reason we're introducing ExternalBeginFrameSources all over is
CompositorFrameSink is getting split into two, a service-side object and a
client-side object. The client side will have an ExternalBeginFrameSource.

[enne (OOO), 2017-02-16 19:03:10]

On 2017/02/16 at 18:56:15, fsamuel wrote:
> On 2017/02/16 18:51:18, enne wrote:
> > Ok, so then my understanding is you're handling the top level BFS that the
> > Display is using separately in the display, no matter what type the BFS is,
and
> > then any other "forwarding" BFS must be external and so you'll track that
inside
> > the external BFS class and catch the rest of them.
> > 
> > I guess I am surprised to hear that a single CompositorFrameSink would have
> > multiple frame producers that would need to be tracked separately.   Maybe
> > fsamuel can explain?
> 
> I'm not really following what you mean?
> 
> The reason we're introducing ExternalBeginFrameSources all over is
CompositorFrameSink is getting split into two, a service-side object and a
client-side object. The client side will have an ExternalBeginFrameSource.

This patch has tracking in ExternalBeginFrameSource on the client side to track
multiple acks.  I'm trying to understand the use case where a client-side
CompositorFrameSink would expect to see multiple callers to
SubmitCompositorFrame from a single begin frame.  Alternatively, where a
client-side ExternalBeginFrameSource would have multiple observers.

[Fady Samuel, 2017-02-16 19:05:18]

On 2017/02/16 19:03:10, enne wrote:
> On 2017/02/16 at 18:56:15, fsamuel wrote:
> > On 2017/02/16 18:51:18, enne wrote:
> > > Ok, so then my understanding is you're handling the top level BFS that the
> > > Display is using separately in the display, no matter what type the BFS
is,
> and
> > > then any other "forwarding" BFS must be external and so you'll track that
> inside
> > > the external BFS class and catch the rest of them.
> > > 
> > > I guess I am surprised to hear that a single CompositorFrameSink would
have
> > > multiple frame producers that would need to be tracked separately.   Maybe
> > > fsamuel can explain?
> > 
> > I'm not really following what you mean?
> > 
> > The reason we're introducing ExternalBeginFrameSources all over is
> CompositorFrameSink is getting split into two, a service-side object and a
> client-side object. The client side will have an ExternalBeginFrameSource.
> 
> This patch has tracking in ExternalBeginFrameSource on the client side to
track
> multiple acks.  I'm trying to understand the use case where a client-side
> CompositorFrameSink would expect to see multiple callers to
> SubmitCompositorFrame from a single begin frame.  Alternatively, where a
> client-side ExternalBeginFrameSource would have multiple observers.

Ohh I definitely wouldn't expect multiple observers.

[Eric Seckler, 2017-02-16 21:15:02]

On 2017/02/16 19:05:18, Fady Samuel wrote:
> On 2017/02/16 19:03:10, enne wrote:
> > On 2017/02/16 at 18:56:15, fsamuel wrote:
> > > On 2017/02/16 18:51:18, enne wrote:
> > > > Ok, so then my understanding is you're handling the top level BFS that
the
> > > > Display is using separately in the display, no matter what type the BFS
> is,
> > and
> > > > then any other "forwarding" BFS must be external and so you'll track
that
> > inside
> > > > the external BFS class and catch the rest of them.
> > > > 
> > > > I guess I am surprised to hear that a single CompositorFrameSink would
> have
> > > > multiple frame producers that would need to be tracked separately.  
Maybe
> > > > fsamuel can explain?
> > > 
> > > I'm not really following what you mean?
> > > 
> > > The reason we're introducing ExternalBeginFrameSources all over is
> > CompositorFrameSink is getting split into two, a service-side object and a
> > client-side object. The client side will have an ExternalBeginFrameSource.
> > 
> > This patch has tracking in ExternalBeginFrameSource on the client side to
> track
> > multiple acks.  I'm trying to understand the use case where a client-side
> > CompositorFrameSink would expect to see multiple callers to
> > SubmitCompositorFrame from a single begin frame.  Alternatively, where a
> > client-side ExternalBeginFrameSource would have multiple observers.
> 
> Ohh I definitely wouldn't expect multiple observers.

OK, this means we could simplify tracking in ExternalBFS given that there's
always at most a single observer? Would it be fine to restrict ExternalBFS to
never be used with more than one observer?

[enne (OOO), 2017-02-16 21:23:27]

What about: an external begin frame source can have as many observers as it
wants, but at most one is allowed to call did finish frame (or at most one call
to did finish frame is allowed per frame?)

I don't know that I have a real use case for multiple observers, but in general
begin frame sources can have as many observers as they want, so this lets there
be special behavior without breaking that abstraction.

[Eric Seckler, 2017-02-16 21:58:45]

On 2017/02/16 21:23:27, enne wrote:
> What about: an external begin frame source can have as many observers as it
> wants, but at most one is allowed to call did finish frame (or at most one
call
> to did finish frame is allowed per frame?)
> 
> I don't know that I have a real use case for multiple observers, but in
general
> begin frame sources can have as many observers as they want, so this lets
there
> be special behavior without breaking that abstraction.

Hm, I was hoping to make it a requirement for any BeginFrameObserver to call
DidFinishFrame for every BeginFrame (we need this for all observers of the
Display's BFS). Feels odd to have a non-explicit exception for some specific
ones.

[Eric Seckler, 2017-02-16 22:05:19]

On 2017/02/16 21:58:45, Eric Seckler wrote:
> On 2017/02/16 21:23:27, enne wrote:
> > What about: an external begin frame source can have as many observers as it
> > wants, but at most one is allowed to call did finish frame (or at most one
> call
> > to did finish frame is allowed per frame?)
> > 
> > I don't know that I have a real use case for multiple observers, but in
> general
> > begin frame sources can have as many observers as they want, so this lets
> there
> > be special behavior without breaking that abstraction.
> 
> Hm, I was hoping to make it a requirement for any BeginFrameObserver to call
> DidFinishFrame for every BeginFrame (we need this for all observers of the
> Display's BFS). Feels odd to have a non-explicit exception for some specific
> ones.

Btw, there's at least one case where we feed an ExternalBFS into a cc::Display.
Not sure if we're limited to a single BFObserver/CompositorFrame producer in
that case?
That's here:
https://cs.chromium.org/chromium/src/content/browser/compositor/gpu_process_t...

[enne (OOO), 2017-02-16 22:12:10]

In which class is the ack tracking going to live? BeginFrameSource?
DisplayBeginFrameSource?

I could see saying something like of these two designs:
(1) BeginFrameSource can have any number of observers.  DisplayBeginFrameSource
has the ack tracking.  All observers of DisplayBeginFrameSource must
DidFinishFrame.  Exactly one observer of ExternalBeginFrameSource must
DidFinishFrame.
(2) BeginFrameSource has the ack tracking.  BeginFrameSource can have any number
of observers, but all observers must DidFinishFrame.

It sounds like you're saying #2?

(Also, what does the ack tracking mean for back-to-back begin frame source where
it just keeps sending begin frames at the top level without waiting? Is
DisplayBeginFrameSource going to throttle these?)

[Eric Seckler, 2017-02-16 22:25:03]

On 2017/02/16 22:12:10, enne wrote:
> In which class is the ack tracking going to live? BeginFrameSource?
> DisplayBeginFrameSource?
> 
> I could see saying something like of these two designs:
> (1) BeginFrameSource can have any number of observers. 
DisplayBeginFrameSource
> has the ack tracking.  All observers of DisplayBeginFrameSource must
> DidFinishFrame.  Exactly one observer of ExternalBeginFrameSource must
> DidFinishFrame.
> (2) BeginFrameSource has the ack tracking.  BeginFrameSource can have any
number
> of observers, but all observers must DidFinishFrame.
> 
> It sounds like you're saying #2?
> 
> (Also, what does the ack tracking mean for back-to-back begin frame source
where
> it just keeps sending begin frames at the top level without waiting? Is
> DisplayBeginFrameSource going to throttle these?)

I was thinking more of #3: Every observer must call DidFinishFrame, so that a
source can track its observers if desired. It just turns out that the
DisplayScheduler has a use for tracking the Acks to decide when to trigger an
early deadline. And some BFObservers need to track when they receive acks from
their "child" observers behind an ExternalBFS, so that they themselves can call
DidFinishFrame.

For headless/DevTools, we'll actually want to track on a custom BeginFrameSource
for the DisplayScheduler's DidFinishFrame.

[enne (OOO), 2017-02-16 22:34:40]

On 2017/02/16 at 22:25:03, eseckler wrote:

> I was thinking more of #3: Every observer must call DidFinishFrame, so that a
source can track its observers if desired. It just turns out that the
DisplayScheduler has a use for tracking the Acks to decide when to trigger an
early deadline. And some BFObservers need to track when they receive acks from
their "child" observers behind an ExternalBFS, so that they themselves can call
DidFinishFrame.

Ok, so DisplayBeginFrameSource will use the ack tracker and not generic
BeginFrameSource.  I guess there's no way to simplify external begin frame
source's ack tracking other than saying an external begin frame source can only
have one observer.  I guess that's fine with me if you want to simplify the
external tracking and add comments and DCHECKs around it so that it's obvious
there can be only one observer, and we can make it more complicated in the
future if a use case occurs.

[Eric Seckler, 2017-02-16 22:45:04]

On 2017/02/16 22:34:40, enne wrote:
> On 2017/02/16 at 22:25:03, eseckler wrote:
> 
> > I was thinking more of #3: Every observer must call DidFinishFrame, so that
a
> source can track its observers if desired. It just turns out that the
> DisplayScheduler has a use for tracking the Acks to decide when to trigger an
> early deadline. And some BFObservers need to track when they receive acks from
> their "child" observers behind an ExternalBFS, so that they themselves can
call
> DidFinishFrame.
> 
> Ok, so DisplayBeginFrameSource will use the ack tracker and not generic
> BeginFrameSource. 
Yup, the DisplayBFS is essentially just a wrapper that uses the ack tracker.

> I guess there's no way to simplify external begin frame
> source's ack tracking other than saying an external begin frame source can
only
> have one observer.  I guess that's fine with me if you want to simplify the
> external tracking and add comments and DCHECKs around it so that it's obvious
> there can be only one observer, and we can make it more complicated in the
> future if a use case occurs.
OK, I'm happy to do this if we are confident that below ExternalBFS only has one
observer behind the cc::Display.
https://cs.chromium.org/chromium/src/content/browser/compositor/gpu_process_t...

I'm not sure that's the case though, since DisplayScheduler is a BFO on its own,
and there'll be at least one CompositorFrame producer feeding into the Display
(?)

[Eric Seckler, 2017-02-16 23:43:52]

On 2017/02/16 22:45:04, Eric Seckler wrote:
> On 2017/02/16 22:34:40, enne wrote:
> > On 2017/02/16 at 22:25:03, eseckler wrote:
> > 
> > > I was thinking more of #3: Every observer must call DidFinishFrame, so
that
> a
> > source can track its observers if desired. It just turns out that the
> > DisplayScheduler has a use for tracking the Acks to decide when to trigger
an
> > early deadline. And some BFObservers need to track when they receive acks
from
> > their "child" observers behind an ExternalBFS, so that they themselves can
> call
> > DidFinishFrame.
> > 
> > Ok, so DisplayBeginFrameSource will use the ack tracker and not generic
> > BeginFrameSource. 
> Yup, the DisplayBFS is essentially just a wrapper that uses the ack tracker.
> 
> > I guess there's no way to simplify external begin frame
> > source's ack tracking other than saying an external begin frame source can
> only
> > have one observer.  I guess that's fine with me if you want to simplify the
> > external tracking and add comments and DCHECKs around it so that it's
obvious
> > there can be only one observer, and we can make it more complicated in the
> > future if a use case occurs.
> OK, I'm happy to do this if we are confident that below ExternalBFS only has
one
> observer behind the cc::Display.
>
https://cs.chromium.org/chromium/src/content/browser/compositor/gpu_process_t...
> 
> I'm not sure that's the case though, since DisplayScheduler is a BFO on its
own,
> and there'll be at least one CompositorFrame producer feeding into the Display
> (?)

Chatted to Fady about this. Turns out that this ExternalBFS currently drives the
browser compositor in Mus, including all its RenderWidgetHostViews etc, so
there's definitely more than one observer here at the moment.

Eventually (2017 goal), the browser compositor will disappear. When that
happens, this ExternalBFS will be gone, too. But until then, we'll have to deal
with it :)

Given this, do we want to keep the ack tracker in ExternalBFS as in this patch?
Alternatively, do we want to wrap the ExternalBFS in Mus' browser compositor to
add the ack tracker there?

[enne (OOO), 2017-02-17 00:05:06]

Haha, ok.  We've come full circle.  Ack tracker in external bfs is good.  Sorry
for the noise.  *<_<*

[enne (OOO), 2017-02-17 18:01:28]

Meant to also say lgtm, with my nit about flat_set many comments ago.

[Eric Seckler, 2017-02-17 19:10:01]

Thanks :)

https://codereview.chromium.org/2691453002/diff/60001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/2691453002/diff/60001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:264: if
(obs->LastUsedBeginFrameArgs().sequence_number != current_sequence_number_)
On 2017/02/15 20:50:47, brianderson wrote:
> Why is this != instead of ==?

This checks if the observer actually uses the BeginFrame. If it doesn't,
LastUsedBeginFrameArgs will be an older BeginFrame. In that case, the observer
has already finished the current BeginFrame, because it didn't use it.

https://codereview.chromium.org/2691453002/diff/60001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:325: return
base::STLIncludes(finished_observers_, observers_);
On 2017/02/15 20:50:48, brianderson wrote:
> It looks like STLIncludes depends on std::includes which takes generic
> InputIterators where it can't assume std::distance will be constant time, so
> it's unlikely std::include can do an efficient early out based on the sizes of
> the two containers.
> 
> It might help to start this method with:
> if (finished_obervers.size() < observers.size())
>   return false;
> 
> Also, if we can guarantee that we only get OnObserverFinishedFrame from
> observers that are currently observing, we might be able to just return
> (finished_observers.size == observers.size()). Any idea if that's the case
given
> the current state of the code? I assume not given the request+response is
async.

Added the early out. I don't think we can guarantee that we never receive an ack
from an already-inactive BFO, given that some of these ACKs can come
asynchronously from across process boundaries.

https://codereview.chromium.org/2691453002/diff/60001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/2691453002/diff/60001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:266: std::set<BeginFrameObserver*> observers_;
On 2017/02/15 23:12:25, enne wrote:
> On 2017/02/15 at 20:21:36, brianderson wrote:
> > On 2017/02/10 14:14:28, Sami wrote:
> > > nit: unordered_set for both of these?
> > > edit: Ah, I guess this is required by base::STLIncludes. Nevermind then.
> > 
> > Probably would be good to add a comment explaining why order matters.
> 
> Maybe flat_set instead? (And SmallMap instead of unordered_map?) How many
> observers do you expect to have?

All of these collections are likely very small. Done.

[Eric Seckler, 2017-02-17 19:10:08]

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

[commit-bot: I haz the power, 2017-02-17 19:10:51]

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

[Eric Seckler, 2017-02-17 19:12:33]

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

[Eric Seckler, 2017-02-17 19:12:34]

Fady, can you have a look at ui/?
+reveman for components/exo.

[Fady Samuel, 2017-02-17 19:19:44]

lgtm

[commit-bot: I haz the power, 2017-02-17 20:44:34]

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

[commit-bot: I haz the power, 2017-02-17 20:44:35]

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

[brianderson, 2017-02-17 20:49:54]

https://codereview.chromium.org/2691453002/diff/60001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/2691453002/diff/60001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:264: if
(obs->LastUsedBeginFrameArgs().sequence_number != current_sequence_number_)
On 2017/02/17 19:10:01, Eric Seckler wrote:
> On 2017/02/15 20:50:47, brianderson wrote:
> > Why is this != instead of ==?
> 
> This checks if the observer actually uses the BeginFrame. If it doesn't,
> LastUsedBeginFrameArgs will be an older BeginFrame. In that case, the observer
> has already finished the current BeginFrame, because it didn't use it.

Ah, I see. Instead of doing this, can we require Observers to *always* call
OnObserverFinishedFrame in response to a BeginFrame - even when it drops a
BeginFrame?

https://codereview.chromium.org/2691453002/diff/80001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/2691453002/diff/80001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:319: finished_observers_.insert(obs);
Do we ever get here where finished_observers_ doesn't already have the obs in
it?

Can this be a DCHECK instead?

It would be nice if the only place finished_observers_.insert is called is in
OnObserverFinishedFrame. It would make me feel more confident that the Observers
are behaving correctly.

[brianderson, 2017-02-17 21:26:10]

https://codereview.chromium.org/2691453002/diff/80001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source_unittest.cc (right):

https://codereview.chromium.org/2691453002/diff/80001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source_unittest.cc:620:
std::unique_ptr<TestBeginFrameConsumer> obs2_;
Do these need to be unique_ptrs?

https://codereview.chromium.org/2691453002/diff/80001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source_unittest.cc:872:
std::unique_ptr<MockBeginFrameObserver> obs_;
Do these need to be unique_ptrs?

[Eric Seckler, 2017-02-17 22:47:06]

https://codereview.chromium.org/2691453002/diff/80001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/2691453002/diff/80001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:319: finished_observers_.insert(obs);
On 2017/02/17 20:49:54, brianderson wrote:
> Do we ever get here where finished_observers_ doesn't already have the obs in
> it?
> 
> Can this be a DCHECK instead?
> 
> It would be nice if the only place finished_observers_.insert is called is in
> OnObserverFinishedFrame. It would make me feel more confident that the
Observers
> are behaving correctly.

Yes, we can get here without it being in finished_obs, because we remove from
finished_obs in OnObserverRemoved. I guess we can get rid of the remove in
OnObserverRemoved if you prefer?

[Eric Seckler, 2017-02-17 22:49:35]

https://codereview.chromium.org/2691453002/diff/60001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/2691453002/diff/60001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:264: if
(obs->LastUsedBeginFrameArgs().sequence_number != current_sequence_number_)
On 2017/02/17 20:49:54, brianderson wrote:
> On 2017/02/17 19:10:01, Eric Seckler wrote:
> > On 2017/02/15 20:50:47, brianderson wrote:
> > > Why is this != instead of ==?
> > 
> > This checks if the observer actually uses the BeginFrame. If it doesn't,
> > LastUsedBeginFrameArgs will be an older BeginFrame. In that case, the
observer
> > has already finished the current BeginFrame, because it didn't use it.
> 
> Ah, I see. Instead of doing this, can we require Observers to *always* call
> OnObserverFinishedFrame in response to a BeginFrame - even when it drops a
> BeginFrame?

I think I'll make those changes and send out another patch for that next week :)

[Eric Seckler, 2017-02-21 12:09:04]

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

[commit-bot: I haz the power, 2017-02-21 12:09:11]

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

[Eric Seckler, 2017-02-21 12:10:57]

Brian, PTAL. Thanks!

https://codereview.chromium.org/2691453002/diff/60001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/2691453002/diff/60001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:264: if
(obs->LastUsedBeginFrameArgs().sequence_number != current_sequence_number_)
On 2017/02/17 22:49:35, Eric Seckler wrote:
> On 2017/02/17 20:49:54, brianderson wrote:
> > On 2017/02/17 19:10:01, Eric Seckler wrote:
> > > On 2017/02/15 20:50:47, brianderson wrote:
> > > > Why is this != instead of ==?
> > > 
> > > This checks if the observer actually uses the BeginFrame. If it doesn't,
> > > LastUsedBeginFrameArgs will be an older BeginFrame. In that case, the
> observer
> > > has already finished the current BeginFrame, because it didn't use it.
> > 
> > Ah, I see. Instead of doing this, can we require Observers to *always* call
> > OnObserverFinishedFrame in response to a BeginFrame - even when it drops a
> > BeginFrame?
> 
> I think I'll make those changes and send out another patch for that next week
:)

Turns out it doesn't require much changes at this point, since aside from
Scheduler/DisplayScheduler, no observers call DidFinishFrame currently. When I
add those calls later, I'll make sure to also add them where they drop a
BeginFrame. Added a call in cc::Scheduler. The DisplayScheduler doesn't seem to
ever drop a BeginFrame this way.

Also updated the tests to match.

https://codereview.chromium.org/2691453002/diff/80001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/2691453002/diff/80001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:319: finished_observers_.insert(obs);
On 2017/02/17 22:47:05, Eric Seckler wrote:
> On 2017/02/17 20:49:54, brianderson wrote:
> > Do we ever get here where finished_observers_ doesn't already have the obs
in
> > it?
> > 
> > Can this be a DCHECK instead?
> > 
> > It would be nice if the only place finished_observers_.insert is called is
in
> > OnObserverFinishedFrame. It would make me feel more confident that the
> Observers
> > are behaving correctly.
> 
> Yes, we can get here without it being in finished_obs, because we remove from
> finished_obs in OnObserverRemoved. I guess we can get rid of the remove in
> OnObserverRemoved if you prefer?

Actually, in theory the observer could have been attached to a different
ExternalBFS before and received the same BeginFrame from that ExternalBFS. Don't
think we can assume that that's never the case, given what we learned about the
recent DCHECK failures in crbug.com/690127.

https://codereview.chromium.org/2691453002/diff/80001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source_unittest.cc (right):

https://codereview.chromium.org/2691453002/diff/80001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source_unittest.cc:620:
std::unique_ptr<TestBeginFrameConsumer> obs2_;
On 2017/02/17 21:26:09, brianderson wrote:
> Do these need to be unique_ptrs?

Since I now no longer need to recreate the TestBeginFrameConsumers (don't have
any state anymore), I got rid of their unique_ptrs.

For the tracker, I'm sticking with unique_ptr to avoid copy-assign, which might
incur a copy, and to match the style of the other test harnesses in this file.
As long as you don't have an objection :)

https://codereview.chromium.org/2691453002/diff/80001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source_unittest.cc:872:
std::unique_ptr<MockBeginFrameObserver> obs_;
On 2017/02/17 21:26:09, brianderson wrote:
> Do these need to be unique_ptrs?

At the very least, obs_ and client_ should stay one. They should be reset during
TearDown() (which I also failed to do :)).

I'm going to leave source_ as one as well, since all other test harnesses also
use it as a unique_ptr - and I'd rather like that to be consistent within the
file.

[commit-bot: I haz the power, 2017-02-21 13:05:41]

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

[commit-bot: I haz the power, 2017-02-21 13:05:42]

Dry run: This issue passed the CQ dry run.

[brianderson, 2017-02-21 22:28:03]

lgtm

[reveman, 2017-02-21 22:40:05]

components/exo lgtm % nits

https://codereview.chromium.org/2691453002/diff/100001/components/exo/composi...
File components/exo/compositor_frame_sink_holder.cc (right):

https://codereview.chromium.org/2691453002/diff/100001/components/exo/composi...
components/exo/compositor_frame_sink_holder.cc:61: // TODO(eseckler): Hook up
exo::Surface to the ExternalBFS.
nit: s/exo::Surface/|surface_|/ and s/ExternalBFS/ExternalBeginFrameSource/

https://codereview.chromium.org/2691453002/diff/100001/components/exo/composi...
File components/exo/compositor_frame_sink_holder.h (right):

https://codereview.chromium.org/2691453002/diff/100001/components/exo/composi...
components/exo/compositor_frame_sink_holder.h:64: // Overridden from
cc::ExternalBeginFrameSouceClient:
can you s/cc::ExternalBeginFrameSouceClient/cc::ExternalBeginFrameSourceClient/
while here?

[Eric Seckler, 2017-02-22 09:12:35]

Thanks! :)

https://codereview.chromium.org/2691453002/diff/100001/components/exo/composi...
File components/exo/compositor_frame_sink_holder.cc (right):

https://codereview.chromium.org/2691453002/diff/100001/components/exo/composi...
components/exo/compositor_frame_sink_holder.cc:61: // TODO(eseckler): Hook up
exo::Surface to the ExternalBFS.
On 2017/02/21 22:40:05, reveman wrote:
> nit: s/exo::Surface/|surface_|/ and s/ExternalBFS/ExternalBeginFrameSource/

Done.

https://codereview.chromium.org/2691453002/diff/100001/components/exo/composi...
File components/exo/compositor_frame_sink_holder.h (right):

https://codereview.chromium.org/2691453002/diff/100001/components/exo/composi...
components/exo/compositor_frame_sink_holder.h:64: // Overridden from
cc::ExternalBeginFrameSouceClient:
On 2017/02/21 22:40:05, reveman wrote:
> can you
s/cc::ExternalBeginFrameSouceClient/cc::ExternalBeginFrameSourceClient/
> while here?

Done.

[Eric Seckler, 2017-02-22 09:12:49]

The CQ bit was checked by eseckler@chromium.org

[Eric Seckler, 2017-02-22 09:12:49]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org,
skyostil@chromium.org, fsamuel@chromium.org, reveman@chromium.org,
brianderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2691453002/#ps120001
(title: "Fix comments.")

[commit-bot: I haz the power, 2017-02-22 09:13:32]

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-22 11:25:13]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487754769236110,
"parent_rev": "bd8c80c437ceaa8427186bbbbd3040ae17d6d13d", "commit_rev":
"fa5fec25bef17c2b0855201a3b0e1dfa848eb50e"}

[commit-bot: I haz the power, 2017-02-22 11:26:05]

Description was changed from

==========
[cc] Track observer status in ExternalBeginFrameSource.

This introduces a BeginFrameObserverAckTracker, which tracks which
active observers have finished the current BeginFrame, and hooks it up
to ExternalBeginFrameSource.

BUG=646774, 401331
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[cc] Track observer status in ExternalBeginFrameSource.

This introduces a BeginFrameObserverAckTracker, which tracks which
active observers have finished the current BeginFrame, and hooks it up
to ExternalBeginFrameSource.

BUG=646774, 401331
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

[commit-bot: I haz the power, 2017-02-22 11:26:07]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/fa5fec25bef17c2b0855201a3b0e...