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
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..770fcd8348016a087ebd1b9e8f43647196c069e8 100644
--- a/cc/scheduler/frame_source.h
+++ b/cc/scheduler/frame_source.h
@@ -0,0 +1,220 @@
+// 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>

[brianderson, 2014/06/17 06:24:43]

Is this only needed for the TypeString? Can we just use const char * instead to
avoid pulling in this dependency?

+
+#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 BeginFrameSink {
+ public:
+  virtual void BeginFrame(const BeginFrameArgs& args) = 0;
+};
+
+class CC_EXPORT BeginFrameSource {
+ public:
+  virtual void SetBeginFrameSink(BeginFrameSink* sink) = 0;
+  virtual BeginFrameSink* GetBeginFrameSink() const = 0;

[brianderson, 2014/06/17 06:24:43]

Is GetBeginFrameSink needed for anything? Can we get rid of it?

+
+  virtual void SetGenerateFrames(bool generate_frames) = 0;

[brianderson, 2014/06/17 06:24:42]

Good idea using the 's' to signify a level request. I think we should rename
this to SetNeedsBeginFrames to be more consistent with the rest of cc's codebase
though.

+  virtual bool IsGeneratingFrames() const = 0;
+  // This method allows a more grandular frame sink to provide backpressure to
+  // a frame source rather then toggling SetGenerateFrames. It is used by
+  // systems like the BackToBackFrameSource to make sure only one frame is
+  // pending at a time.
+  virtual void PendingFrames(size_t count) = 0;

[brianderson, 2014/06/17 06:24:42]

I think the unthrottled BeginFrame source is really making the attempt to
generalize all sources too difficult by forcing BeginFrameSources that shouldn't
have to care about the unthrottled case to worry about it.

Maybe we should go back to an edge triggered SetNeedsBeginFrame (without the
's'). Then, in the throttled case, the Scheduler could call
SetNeedsBeginFrame(true) within a BeginFrame to request a stream of BeginFrames.
In the unthrottled case, the Scheduler can call SetNeedsBeginFrame(true) only in
the deadline. This would mirror the behavior of SetNeedsCommit more closely and
localize the behavioral differences to the Scheduler rather than leaking it to
all BeginFrameSources.

+
+  virtual void SetTimeBaseAndInterval(base::TimeTicks timebase,
+                                      base::TimeDelta interval) = 0;
+  virtual base::TimeTicks TimeBase() const = 0;
+  virtual base::TimeDelta Interval() const = 0;

[brianderson, 2014/06/17 06:24:42]

Not all BeginFrameSources necessarily need a TimeBase and interval. Can these
methods be moved into only the sources that need it.

Maybe have a separate pure virtual class called TimeBaseAndIntervalSink and have
a member variable of that type in the Scheduler assigned to the
SyntheticBeginFrameSource or BackToBackBeginFrameSource when they are active?

+
+  virtual scoped_ptr<base::Value> BeginFrameSourceAsValue() const = 0;
+
+  virtual ~BeginFrameSource() {}
+};
+
+class CC_EXPORT BaseBeginFrameSource : public BeginFrameSource {

[brianderson, 2014/06/17 06:24:42]

This BaseBeginFrameSource makes understanding the logic a bit convoluted. If it
really helps classes within this file, that's maybe ok; but I'd really like to
prevent either the LayerTreeHostImpl or OutputSurfaces from inheriting it.

I'm hoping that reduceing the API surface of BeginFrameSource will make reduce
the need for BaseBeginFrameSource.

+ public:
+  virtual void SetBeginFrameSink(BeginFrameSink* sink) OVERRIDE;
+  virtual BeginFrameSink* GetBeginFrameSink() const OVERRIDE;
+
+  virtual void SetGenerateFrames(bool generate_frames) OVERRIDE;
+  virtual bool IsGeneratingFrames() const OVERRIDE;
+  virtual void PendingFrames(size_t count) OVERRIDE;
+
+  virtual void SetTimeBaseAndInterval(base::TimeTicks timebase,
+                                      base::TimeDelta interval) OVERRIDE;
+  virtual base::TimeTicks TimeBase() const OVERRIDE;
+  virtual base::TimeDelta Interval() const OVERRIDE;
+
+  virtual scoped_ptr<base::Value> BeginFrameSourceAsValue() const OVERRIDE;
+
+  virtual ~BaseBeginFrameSource() {}
+
+ protected:
+  BeginFrameSink* frame_sink_;
+  bool generate_frames_;
+  base::TimeTicks timebase_;
+  base::TimeDelta interval_;

[simonhong, 2014/06/12 13:47:39]

Variable declaration should be followed by Method declaration.

+
+  explicit BaseBeginFrameSource(BeginFrameSink* sink);
+
+  virtual std::string TypeString() const = 0;
+  virtual void ExtraAsValue(base::DictionaryValue* state) const {}
+  virtual void OnGenerateChange(bool generate_frames) {}
+  virtual void OnPendingFrames(size_t count) {}
+  virtual void OnTimeBaseAndIntervalChange(const base::TimeTicks timebase,
+                                           const base::TimeDelta interval) {}
+};
+
+/**
+ * A frame source which proxies to / from a frame source we don't take
+ * ownership of.
+ */

[simonhong, 2014/06/12 13:47:39]

How about using // instead of /* */?
Style guide says both are ok but be consistent.
All of other sources in cc/ uses // style.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Commen...

+class CC_EXPORT ProxyBeginFrameSource : public BaseBeginFrameSource,

[brianderson, 2014/06/17 06:24:43]

I understand the desire to have the DualBeginFrameSource own its child
BeginFrameSoures. Do any other sources need to own child sources that we want to
prevent them from actually owning?

I think it would be easier to prevent DualBeginFrameSource from owning its
children and have the Scheduler take ownership of the sources it creates. We'd
just need to be extra careful about destruction order.

+                                        public BeginFrameSink {
+ public:
+  // TODO(mithro): This should probably be a weakptr to the frame source
+  // rather then a raw pointer.
+  ProxyBeginFrameSource(BeginFrameSink* sink, BeginFrameSource* source);
+
+  virtual void BeginFrame(const BeginFrameArgs& args) OVERRIDE;
+
+ protected:
+  BeginFrameSource* source_;
+
+  // BaseBeginFrameSource
+  virtual std::string TypeString() const OVERRIDE;
+  virtual void ExtraAsValue(base::DictionaryValue* state) const OVERRIDE;
+  virtual void OnGenerateChange(bool generate_frames) OVERRIDE;
+  virtual void OnPendingFrames(size_t count) OVERRIDE;
+  virtual void OnTimeBaseAndIntervalChange(
+      const base::TimeTicks timebase,
+      const base::TimeDelta interval) OVERRIDE;
+};
+
+/**
+ * A frame source which throttles down another begin frame source.
+ */
+class CC_EXPORT ThrottledBeginFrameSource : public ProxyBeginFrameSource {
+ public:
+  ThrottledBeginFrameSource(BeginFrameSink* sink,
+                            BeginFrameSource* source,
+                            base::TimeDelta interval);
+
+  // BeginFrameSink
+  virtual void BeginFrame(const BeginFrameArgs& args) OVERRIDE;
+
+ private:
+  BeginFrameArgs last_frame_args_;
+
+  // BaseBeginFrameSource
+  virtual std::string TypeString() const OVERRIDE;
+  virtual void ExtraAsValue(base::DictionaryValue* state) const OVERRIDE;
+};
+
+/**
+ * A frame source which sends a BeginFrame as soon as pending frames reaches
+ * zero.
+ */
+class CC_EXPORT BackToBackBeginFrameSource : public BaseBeginFrameSource {
+ public:
+  BackToBackBeginFrameSource(BeginFrameSink* sink,
+                             base::SingleThreadTaskRunner* task_runner);
+  virtual ~BackToBackBeginFrameSource();
+
+  virtual void SendBeginFrameArgs();
+
+ protected:
+  base::WeakPtrFactory<BackToBackBeginFrameSource> weak_factory_;
+  base::SingleThreadTaskRunner* task_runner_;
+  bool send_begin_frame_posted_;
+
+  void ScheduleSendBeginFrameArgs();
+
+  // BaseBeginFrameSource
+  virtual std::string TypeString() const OVERRIDE;
+  virtual void ExtraAsValue(base::DictionaryValue* state) const OVERRIDE;
+  virtual void OnGenerateChange(bool generate_frames) OVERRIDE;
+  virtual void OnPendingFrames(size_t count) OVERRIDE;
+};
+
+/**
+ * A frame source which is locked an external vsync source and generates
+ * BeginFrameArgs for it.
+ */
+class CC_EXPORT SyntheticBeginFrameSource : public BaseBeginFrameSource,
+                                            public TimeSourceClient {
+ public:
+  SyntheticBeginFrameSource(BeginFrameSink* sink,
+                            base::SingleThreadTaskRunner* task_runner,
+                            base::TimeDelta interval);
+  virtual ~SyntheticBeginFrameSource();
+
+  virtual void OnTimerTick() OVERRIDE;
+
+ protected:
+  base::SingleThreadTaskRunner* task_runner_;
+  scoped_refptr<DelayBasedTimeSource> time_source_;
+
+  BeginFrameArgs CreateBeginFrameArgs(base::TimeTicks frame_time);
+
+  // BaseBeginFrameSource
+  virtual std::string TypeString() const OVERRIDE;
+  virtual void ExtraAsValue(base::DictionaryValue* state) const OVERRIDE;
+  virtual void OnGenerateChange(bool generate_frames) OVERRIDE;
+  virtual void OnTimeBaseAndIntervalChange(
+      const base::TimeTicks timebase,
+      const base::TimeDelta interval) OVERRIDE;
+};
+
+/**
+ * A virtual frame source which lets you switch between two other frame sources
+ * (making sure the BeginFrameArgs stays monotonic).
+ */
+class CC_EXPORT DualBeginFrameSource : public BaseBeginFrameSource,

[brianderson, 2014/06/17 06:24:43]

Do we need this class anymore since the background ticking change isn't part of
this patch?

+                                       public BeginFrameSink {
+ public:
+  DualBeginFrameSource(BeginFrameSink* sink,
+                       scoped_ptr<BeginFrameSource> source_primary,
+                       scoped_ptr<BeginFrameSource> source_secondary);

[simonhong, 2014/06/12 13:47:39]

How about foreground_source and background_source?

+  virtual ~DualBeginFrameSource();
+
+  // BeginFrameSink
+  virtual void BeginFrame(const BeginFrameArgs& args) OVERRIDE;
+
+  // ---------------------------------------------------------------------
+
+  void SwitchSource(const BeginFrameSource* new_source);
+  const BeginFrameSource* SourceForeground() const;
+  const BeginFrameSource* SourceBackground() const;

[simonhong, 2014/06/12 13:47:39]

How about ForegroundSource() and BackgroundSource()?

+
+ private:
+  BeginFrameArgs last_frame_args_;
+  BeginFrameSource* active_source_;
+  scoped_ptr<BeginFrameSource> source_foreground_;
+  scoped_ptr<BeginFrameSource> source_background_;

[simonhong, 2014/06/12 13:47:39]

How about foreground_source_ and background_source_?

+
+  // BaseBeginFrameSource
+  virtual std::string TypeString() const OVERRIDE;
+  virtual void ExtraAsValue(base::DictionaryValue* state) const OVERRIDE;
+  virtual void OnGenerateChange(bool generate_frames) OVERRIDE;
+  virtual void OnPendingFrames(size_t count) OVERRIDE;
+  virtual void OnTimeBaseAndIntervalChange(
+      const base::TimeTicks timebase,
+      const base::TimeDelta interval) OVERRIDE;
+};
+
+}  // namespace cc
+
+#endif  // CC_SCHEDULER_FRAME_SOURCE_H_