Refactoring the way begin frame sources inside scheduler work.

cc/scheduler/frame_source.h

Index: cc/scheduler/frame_source.h
diff --git a/cc/scheduler/frame_source.h b/cc/scheduler/frame_source.h
new file mode 100644
index 0000000000000000000000000000000000000000..59978455fd6e736674d07d78011c0c739c3324a5
--- /dev/null
+++ b/cc/scheduler/frame_source.h
@@ -0,0 +1,202 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CC_SCHEDULER_FRAME_SOURCE_H_
+#define CC_SCHEDULER_FRAME_SOURCE_H_
+
+#include <string>
+
+#include "base/debug/trace_event.h"
+#include "base/logging.h"
+#include "cc/output/begin_frame_args.h"
+#include "cc/scheduler/delay_based_time_source.h"
+#include "ui/gfx/frame_time.h"
+
+namespace cc {
+
+class CC_EXPORT FrameSink {

[brianderson, 2014/05/07 17:20:16]

Please rename this BeginFrameSink so we have consistent naming and avoid
overloading the word Frame.

[mithro-old, 2014/05/07 23:42:28]

Done.

+ public:
+  virtual void BeginFrame(const BeginFrameArgs& args) = 0;
+};
+
+class CC_EXPORT FrameSource {

[brianderson, 2014/05/07 17:20:16]

Please rename this BeginFrameSource.

[mithro-old, 2014/05/07 23:42:28]

Done.

+ public:
+  virtual void SetNeedsBeginFrame(bool needs_begin_frame) = 0;
+  virtual void SetSink(FrameSink* sink) = 0;
+  virtual void SetTimeBaseAndInterval(base::TimeTicks timebase,
+                                      base::TimeDelta interval) = 0;
+
+  virtual scoped_ptr<base::Value> FrameSourceAsValue() const = 0;
+  virtual inline std::string FrameSourceType() const = 0;

[brianderson, 2014/05/07 17:20:16]

Is this for testing purposes? Can it be removed in the final version of this
patch?

[mithro-old, 2014/05/07 22:02:32]

Having which frame source is in use as part of the scheduler debug state would
be pretty useful.

+  virtual ~FrameSource() {}
+};
+
+class CC_EXPORT BaseFrameSource : public FrameSource {

[brianderson, 2014/05/07 17:20:16]

I need to think about this a bit. But can you provide reasoning for why
implementation inheritance is better than a composition solution?

[mithro-old, 2014/05/07 23:42:28]

The code BaseFrameSource saves is a relatively small (but not zero). Feels like
an inheritance type situation but have no other reasoning.

I've fixed the method order and a little more documentation to make the
inheritance easier to understand.

+ public:
+  virtual void SetNeedsBeginFrame(bool needs_begin_frame) = 0;
+  virtual void SetSink(FrameSink* sink) OVERRIDE;
+  virtual void SetTimeBaseAndInterval(base::TimeTicks timebase,
+                                      base::TimeDelta interval) OVERRIDE;
+  virtual scoped_ptr<base::Value> FrameSourceAsValue() const OVERRIDE;
+
+  virtual inline std::string FrameSourceType() const = 0;
+
+  virtual ~BaseFrameSource() {}
+
+ protected:
+  base::TimeDelta interval_;
+
+  scoped_ptr<base::DictionaryValue> BaseFrameSourceAsValue() const;
+  virtual void SendBeginFrame(const BeginFrameArgs& args);
+
+  explicit BaseFrameSource(FrameSink* sink);
+
+ private:
+  FrameSink* frame_sink_;
+};
+
+/**
+ * A frame source which proxies to / from a frame source we don't take
+ * ownership of.
+ */
+class CC_EXPORT ProxyFrameSource : public BaseFrameSource, public FrameSink {

[brianderson, 2014/05/07 17:20:16]

This is getting a bit confusing for me to follow. Is there any way we can do
without the ProxyFrameSource?

[mithro-old, 2014/05/07 22:02:32]

The sole reason that ProxyFrameSource exists at the moment is because
ThrottleFrameSource/DualFrameSource both take ownership of the frame source that
they are given (via the scope_ptr). Most of the time I think we *do* want these
sources to own the object but it doesn't work with LayerTreeHost as that is
owned via a different route.

[brianderson, 2014/05/08 00:55:58]

I feel like there must be a solution to this problem that requires less
boilerplate and is more straight forward.

For the classes that we might want to have take ownership, could we make the
constructors take in raw pointers and then have separate methods to transfer
ownership? Or have additional constructors that take in raw pointers?

[mithro-old, 2014/05/09 02:45:58]

This seems like mostly a style question? 

I guess we could do the following, but we probably should then have constructors
for all the combinations (O, O) (U, U) (O, U) (U, O)...
----------------------------------------------------------
class DualFrameSource {
  FrameSource* source_foreground_;
  scoped_ptr<FrameSource> source_foreground_owner_;
  FrameSource* source_background_;
  scoped_ptr<FrameSource> source_background_owner_;

  DualFrameSource(FrameSource* foreground, FrameSource* background)
    : source_foreground_(foreground), 
      source_background_(background)
   {}

  DualFrameSource(scoped_ptr<FrameSource> foreground, scoped_ptr<FrameSource>
background)
    : source_foreground_(foreground.get()),
source_foreground_owner_(foreground),
      source_background_(background.get()),
source_background_owner_(background), 
   {}
}
----------------------------------------------------------

+ public:
+  // FIXME(mithro): This should probably be a weakptr to the frame source
+  // rather then a raw pointer.
+  ProxyFrameSource(FrameSink* sink, FrameSource* source);
+
+  // FrameSource
+  virtual void SetNeedsBeginFrame(bool needs_begin_frame) OVERRIDE;
+  virtual scoped_ptr<base::Value> FrameSourceAsValue() const OVERRIDE;
+  virtual std::string FrameSourceType() const OVERRIDE;
+  virtual void SetTimeBaseAndInterval(base::TimeTicks timebase,
+                                      base::TimeDelta interval) OVERRIDE;
+
+  // FrameSink
+  virtual void BeginFrame(const BeginFrameArgs& args) OVERRIDE;
+
+ private:
+  FrameSource* source_;
+};
+
+/**
+ * A frame source which throttles down another begin frame source.
+ */
+class CC_EXPORT ThrottledFrameSource : public BaseFrameSource,

[brianderson, 2014/05/07 17:20:16]

I was thinking about this more and think a wrapper based solution like this for
throttling might be too restrictive. For example, when running the compositor at
30 fps, we will want to run the main thread at 30 fps but maybe at an offset
phase to reduce latency. Hiding BeginFrames from the Scheduler would prevent it
from being able to control the phase in a meaningful way.

Let's add throttling related logic for later.

[mithro-old, 2014/05/07 23:42:28]

I'm going to leave the ThrottledFrameSource here for the moment. Will remove it
once the unit tests all pass if we don't end up using it. (It is also useful at
the moment to compare to dual frame sources as they are both cases where you are
wrapping a frame source).

+                                       public FrameSink {
+ public:
+  ThrottledFrameSource(FrameSink* sink,
+                       scoped_ptr<FrameSource> source,
+                       base::TimeDelta interval);
+  virtual ~ThrottledFrameSource();
+
+  // FrameSource
+  virtual void SetNeedsBeginFrame(bool needs_begin_frame) OVERRIDE;
+  virtual scoped_ptr<base::Value> FrameSourceAsValue() const OVERRIDE;
+  virtual std::string FrameSourceType() const OVERRIDE;
+  virtual void SetTimeBaseAndInterval(base::TimeTicks timebase,
+                                      base::TimeDelta interval) OVERRIDE;
+
+  // FrameSink
+  virtual void BeginFrame(const BeginFrameArgs& args) OVERRIDE;
+
+ private:
+  BeginFrameArgs last_frame_args_;
+  scoped_ptr<FrameSource> source_;
+};
+
+/**
+ * Frame source which needs a task runner object.
+ */
+class CC_EXPORT TaskRunnerFrameSource : public BaseFrameSource {

[brianderson, 2014/05/07 17:20:16]

Does this extra level of inheritance buy us much?

[mithro-old, 2014/05/07 23:42:28]

Not really - I thought there would be more shared code for posting tasks /
creating BeginFrameArgs but it hasn't turned out that way yet.

I can remove it if you want?

[brianderson, 2014/05/08 00:55:58]

Please remove it unless you find there is a significant benefit.

[mithro-old, 2014/05/09 02:45:58]

Done.

+ public:
+  TaskRunnerFrameSource(FrameSink* sink,
+                        base::SingleThreadTaskRunner* task_runner);
+
+ protected:
+  base::SingleThreadTaskRunner* task_runner_;
+};
+
+/**
+ * A frame source which sends a BeginFrame as soon as SetNeedsBeginFrame is
+ * requested.
+ */
+class CC_EXPORT BackToBackFrameSource : public TaskRunnerFrameSource {
+ public:
+  BackToBackFrameSource(FrameSink* sink,
+                        base::SingleThreadTaskRunner* task_runner);
+
+  // FrameSource
+  virtual void SetNeedsBeginFrame(bool needs_begin_frame) OVERRIDE;
+  virtual std::string FrameSourceType() const OVERRIDE;
+
+  // ---------------------------------------------------------------------
+  void PostBeginFrame();
+};
+
+/**
+ * A frame source which is locked an external vsync source and generates
+ * BeginFrameArgs for it.
+ */
+class CC_EXPORT SyntheticFrameSource : public TaskRunnerFrameSource,
+                                       public TimeSourceClient {
+ public:
+  SyntheticFrameSource(FrameSink* sink,
+                       base::SingleThreadTaskRunner* task_runner,
+                       base::TimeDelta interval);
+  virtual ~SyntheticFrameSource();
+
+  // FrameSource
+  virtual void SetNeedsBeginFrame(bool needs_begin_frame) OVERRIDE;
+  virtual void SetTimeBaseAndInterval(base::TimeTicks timebase,
+                                      base::TimeDelta interval) OVERRIDE;
+  virtual scoped_ptr<base::Value> FrameSourceAsValue() const OVERRIDE;
+  virtual std::string FrameSourceType() const OVERRIDE;
+
+  // ---------------------------------------------------------------------
+
+  virtual void OnTimerTick() OVERRIDE;
+
+ private:
+  scoped_refptr<TimeSource> time_source_;
+
+  BeginFrameArgs CreateSyntheticBeginFrameArgs(base::TimeTicks frame_time);
+};
+
+/**
+ * A virtual frame source which lets you switch between two other frame sources
+ * (making sure the BeginFrameArgs stays monotonic).
+ */
+class CC_EXPORT DualFrameSource : public BaseFrameSource, public FrameSink {

[brianderson, 2014/05/07 17:20:16]

Tim and I talked offline comparing this solution for background ticking and a
solution that pulses SetNeedsBeginFrame in the background from a single
BeginFrameSource instead.

A summary of the tradeoffs:

DualFrameSource:
* Although not a common occurrence, a background tick that arrives right before
we become visible will never delay us from drawing until the next BeginFrame.
* The Browser could eventually control the background FrameSource and coordinate
multiple Renderers so they are more power efficient.

Pulse SetNeedsBeginFrame:
* Doesn't need logic to avoid non monotonic timestamps.
* Retroactively drawing when a background tick arrives right before we become
visible might be complicated.

I wonder if a third solution might be to request foreground or background
BeginFrames from the FrameSource directly.

===================

In any case, I'd be happy with landing the DualFrameSource solution first to
unblock animation work - it reflects the system we have today pretty well.

If we really want to improve the corner cases when becoming visible and improve
power, we can revisit how background ticking is implemented.

[mithro-old, 2014/05/07 23:42:28]

We are currently 
Good summary.

+ public:
+  DualFrameSource(FrameSink* sink,
+                  scoped_ptr<FrameSource> source_primary,
+                  scoped_ptr<FrameSource> source_secondary);
+  virtual ~DualFrameSource();
+
+  // FrameSource

[brianderson, 2014/05/07 17:20:16]

I think DualFrameSource needs to override SetTimebaseAndInterval.

[mithro-old, 2014/05/07 22:02:32]

Yes it does.

At the moment I think we want to forward SetTimebaseAndInterval to the primary
source only?

[brianderson, 2014/05/08 00:55:58]

To the primary source only sounds good.

[mithro-old, 2014/05/09 02:45:58]

Done.

How do we set / change the interval for the background source then?

+  virtual void SetNeedsBeginFrame(bool needs_begin_frame) OVERRIDE;
+  virtual scoped_ptr<base::Value> FrameSourceAsValue() const OVERRIDE;
+  virtual std::string FrameSourceType() const OVERRIDE;
+
+  // FrameSink
+  virtual void BeginFrame(const BeginFrameArgs& args) OVERRIDE;
+
+  // ---------------------------------------------------------------------
+
+  void SwitchSource(const FrameSource* new_source);
+  const FrameSource* SourcePrimary() const;
+  const FrameSource* SourceSecondary() const;

[brianderson, 2014/05/07 17:20:16]

Should we rename these to Foreground and Background? We can generalize in the
future if we need to, but I don't see us needing to.

[mithro-old, 2014/05/07 23:42:28]

Done.

+
+ private:
+  bool needs_begin_frame_;
+  BeginFrameArgs last_frame_args_;
+  FrameSource* active_source_;
+  scoped_ptr<FrameSource> source_primary_;
+  scoped_ptr<FrameSource> source_secondary_;
+};
+
+}  // namespace cc
+
+#endif  // CC_SCHEDULER_FRAME_SOURCE_H_