cc: Adding a BeginFrameObserverMultiplexer.

cc: Adding a BeginFrameObserverMultiplexer.

This class allows a BeginFrameSource to send to multiple BeginFrameObservers at
the same time. It also provides callback functionality to monitor if there are
any BeginFrameObservers in the multiplexer. This is needed by the
UnifiedBeginFrame project.

This patch was based on Simon Hong's work at
https://codereview.chromium.org/845393002/ - "cc: Create
ProxyBeginFrameSource".

BUG=448140
R=brianderson@chromium.org,simonhong@chromium.org
TEST=cc_unittests

[mithro-old, 2015-03-24 04:41:37]

Hi Simon / Brian,

This is an alternative implementation of the ProxyBeginFrame stuff that Simon
was working on. I figured it would be easier to actually write this up then try
and describe it.

I also have an alternative patch which makes all BeginFrameSource objects able
to send to multiple observers. Will also upload that soon.

Tim 'mithro' Ansell

[danakj, 2015-03-24 17:19:52]

FWIW I liked that simon's CL didn't have complicated inheritence hierarchies, if
this was meant to replace that.

[simonhong, 2015-03-25 02:03:23]

I like the BeginFrameObserverMultiplxer.
It describes really well what it will do.

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

https://codereview.chromium.org/1022483003/diff/20001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:68:
BeginFrameObserverMultiplexer::~BeginFrameObserverMultiplexer() {
How about checking empty() of observer list?

https://codereview.chromium.org/1022483003/diff/20001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:78: void
BeginFrameObserverMultiplexer::AddObserver(BeginFrameObserver* obs) {
As you can see in my previous landed cl
(https://codereview.chromium.org/1000503002/),
BeginFrameObserverProxy sends missed_begin_frame_args_ immediately when new
observer is added.
Each observer determines whether or not to send it to RenderWidget.

https://codereview.chromium.org/1022483003/diff/20001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:100: return
observer_list_.might_have_observers();
Also in my previous cl, there is some suggestion.
"might" word can give confusion.
So, I replaced this with plane std::list.
We can use it here because we don't modify observer list during observer method
is called.

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

https://codereview.chromium.org/1022483003/diff/20001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:92: class CC_EXPORT
BeginFrameObserverMultiplexer : public BeginFrameObserverMixIn {
As dana said, how about subclassing BeginFrameObserver directly?
I think OnBeginFrameMixInDelegate() can be replaced with
BeginFrameObserver::OnBeginFrame().

[mithro-old, 2015-03-25 02:34:47]

Thanks for your review Simon.

The alternative solution is to make all BFS objects capable of sending to
multiple observers. You can find an implementation of that at
https://codereview.chromium.org/1026233002/

I'm unsure which is a better solution. What do you think?

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

https://codereview.chromium.org/1022483003/diff/20001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:68:
BeginFrameObserverMultiplexer::~BeginFrameObserverMultiplexer() {
On 2015/03/25 02:03:21, simonhong wrote:
> How about checking empty() of observer list?

Do we (and should we) require a user to remove all observers before removing the
multiplexer?

https://codereview.chromium.org/1022483003/diff/20001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:78: void
BeginFrameObserverMultiplexer::AddObserver(BeginFrameObserver* obs) {
On 2015/03/25 02:03:21, simonhong wrote:
> As you can see in my previous landed cl
> (https://codereview.chromium.org/1000503002/),
> BeginFrameObserverProxy sends missed_begin_frame_args_ immediately when new
> observer is added.
> Each observer determines whether or not to send it to RenderWidget.

I'm not sure what you are saying here? This code also sends the last recorded
begin frame args as a missed begin frame args on observer addition?

The BeginFrameObserver contract is that any new frame_time sent is strictly
greater than the frame_time returned by an observers LastUsedBeginFrameArgs
method.

https://codereview.chromium.org/1022483003/diff/20001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:100: return
observer_list_.might_have_observers();
On 2015/03/25 02:03:21, simonhong wrote:
> Also in my previous cl, there is some suggestion.
> "might" word can give confusion.

Looking at the ObserverList implementation, the reason it is "might" is because
if an observer is removed while iterating over the list this method won't return
false until the Compact() method is called (which happens at the end of
iteration).

Guess I should add some type of comment here?

> So, I replaced this with plane std::list.
> We can use it here because we don't modify observer list during observer
method
> is called.

A std::list (in fact pretty much all std::xxx structures) is not safe to modify
while being iterated over, something that is important for the idea of
Observers. This is why the ObserverList type exists.

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

https://codereview.chromium.org/1022483003/diff/20001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.h:92: class CC_EXPORT
BeginFrameObserverMultiplexer : public BeginFrameObserverMixIn {
On 2015/03/25 02:03:22, simonhong wrote:
> As dana said, how about subclassing BeginFrameObserver directly?
> I think OnBeginFrameMixInDelegate() can be replaced with
> BeginFrameObserver::OnBeginFrame().

Implementing BeginFrameObserver means duplicating the code inside the
BeginFrameObserverMixIn. Having two places which implement identical logic
substantially increases the risk of them getting out of sync leading to subtle
bugs which are hard to debug.

Using inheritance here should makes sure you don't forget to maintain the
BeginFrameObserver contract by not updating the LastBeginFrameArgs output.

An alternative to using inheritance here would be to make the code in
BFSOberverMixIn some type of "helper library" but we would need to be vigilant
in making sure it always used.

[brianderson, 2015-04-07 03:24:51]

Didn't get to look at this patch in detail yet, but I think we should use this
approach (pending inheritance decision), as opposed to
https://codereview.chromium.org/1026233002/.

The class that is aware of all the observers is going to have to become much
more well integrated with the DisplayScheduler in order to 1) influence the
DisplayScheduler's deadline and 2) send the observers additional information
they should use to detect and recover from high latency modes.

We should probably stay away from adding that complexity to all
BeginFrameSources as the other approach does.