Refactoring the way begin frame sources inside scheduler work.

cc/scheduler/frame_source.h

+// 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 <set>
+#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/compositor/compositor_vsync_manager.h"
+
+namespace cc {
+
+// (Pure) Interface for observing BeginFrame messages from BeginFrameSource
+// objects.
+class CC_EXPORT BeginFrameObserver {

[simonhong, 2014/09/23 04:39:09]

How about using BeginFrameSourceObserver instead of BeginFrameObserver?
I think this can make more readable.
(But, not a strong opinion.)

+ public:
+  // The |frame_info| given to OnBeginFrame is guaranteed to have
+  // |frame_info|.IsValid()==true and have |frame_info|.frame_time field be
+  // strictly greater than the previous call.
+  //
+  // Side effects: This function can (and most of the time *will*) change the
+  // return value of the LastBeginFrameArgs method. See the documentation on
+  // that method for more information.
+  virtual void OnBeginFrame(const BeginFrameArgs& frame_info) = 0;

[Sami, 2014/09/19 13:44:22]

Please call this |args| or |begin_frame_args| since that's done elsewhere in the
codebase.

[brianderson, 2014/09/23 01:31:06]

+1

[mithro-old, 2014/09/23 12:43:55]

Acknowledged.

[mithro-old, 2014/09/23 12:43:55]

I original had |args| but a co-worker pointed out that it could be easily
confused with "all the arguments to this function" (hence the change to
frame_info name).

I've changed it back to |args| now.

+
+  // Some frame sources *may* provide the last missed begin frame when
+  // SetNeedsBeginFrames is set true (such as the SyntheticBeginFrameSource).
+  //
+  // **Don't** depend on this function ever being called.
+  virtual void OnMissedBeginFrame(const BeginFrameArgs& frame_info) = 0;
+
+  // Returns the last BeginFrameArgs used by the observer. This method's return
+  // value is affected by the OnBeginFrame method!
+  //
+  //  - Before the first call of OnBeginFrame, this method should return a
+  //    BeginFramArgs on which IsValid() returns false.

[Sami, 2014/09/19 13:44:22]

typo: BeginFrameArgs

[mithro-old, 2014/09/23 12:43:54]

Done.

+  //
+  //  - If the |frame_info| passed to OnBeginFrame is (or *will be*) used, then
+  //    this method's return value should become the |frame_info| given to
+  //    OnBeginFrame.
+  //
+  //    This should occur *by the time* the OnBeginFrame method returns to it's
+  //    caller.
+  //
+  //  - If the |frame_info| passed to OnBeginFrame is dropped, then this
+  //    method's return value should *not* change.
+  //
+  // These requirements are design to allow chaining and nesting of

[brianderson, 2014/09/23 01:31:06]

design -> designed

[mithro-old, 2014/09/23 12:43:54]

Done.

+  // BeginFrameObservers which filter the incoming BeginFrame messages while
+  // preventing "double dropping" and other bad side effects.
+  virtual const BeginFrameArgs LastBeginFrameArgs() const = 0;

[Sami, 2014/09/19 13:44:22]

Is it so that the return value from LastBeginFrameArgs will only ever change
inside calls to OnBeginFrame? If so, how about we give OnBeginFrame a return
enum or an out parameter that conveys the same information? Otherwise I think
this should be called something like BeginFrameSourceClient to highlight it's
not a simpler observer.

Also, LastUsedBeginFrameArgs() might make the relationship more obvious.

[mithro-old, 2014/09/23 12:43:55]

Yes, that is correct - LastUsedBeginFrameArgs should only ever change inside the
OnBeginFrame method.
I'm not sure I fully understand but I gave this idea a go, does it look okay?
The patch original called it a BeginFrameSink, but I changed it to
BeginFrameObserver to match the AddObserver/RemoveObserver methods.
Using this name now.

[brianderson, 2014/09/24 06:03:05]

Sami, were you suggesting the enums should replace LastUsedBeginFrameArgs()?
There seems to be a lot of overlap between the two. I like the
LastUsedBeginFrameArgs() since the Observer/Client is the one who really decides
that and is going to store a copy of it anyway; no need to store it twice.

[Sami, 2014/09/24 11:12:53]

Yes, let's have one or the other but not both because that is redundant. If
Brian's happy with just having LastUsedBeginFrameArgs() then I am too :)

[mithro-old, 2014/09/24 17:14:54]

Enum is removed in favour of LastUsedBeginFrameArgs now.

[mithro-old, 2014/09/24 17:14:54]

Acknowledged.

+
+  // Tracing support
+  virtual void AsValueInto(base::debug::TracedValue* dict) const = 0;
+};
+
+// Simple implementation of a BeginFrameObserver with some convenience
+// functionality.
+//
+// Basic observers should;
+//  - Implement the OnBeginFrameImpl function.
+//  - Recommended (but not required) to call
+//    BeginFrameObserverImpl::OnValueInto in their overridden OnValueInto
+//    function.
+//
+// Advance observers may also wish to override the OnMissedBeginFrame method.
+class CC_EXPORT BeginFrameObserverImpl : public BeginFrameObserver {
+ public:
+  // BeginFrameObserver
+
+  // Traces |args| and DCHECK |args| satisfies pre-conditions then calls
+  // OnBeginFrameImpl and updates the last_begin_frame_args_ value on true.
+  virtual void OnBeginFrame(const BeginFrameArgs& frame_info) OVERRIDE;
+
+  // Forwards call to the OnBeginFrame callback.
+  virtual void OnMissedBeginFrame(const BeginFrameArgs& frame_info) OVERRIDE;
+
+  virtual const BeginFrameArgs LastBeginFrameArgs() const OVERRIDE;
+
+  // Outputs last_begin_frame_args_
+  virtual void AsValueInto(base::debug::TracedValue* dict) const OVERRIDE;
+
+ protected:
+  // Subclasses should override this method!
+  // Return true if the given argument is (or will be) used.
+  virtual bool OnBeginFrameImpl(const BeginFrameArgs& frame_info) = 0;
+
+  BeginFrameArgs last_begin_frame_args_;
+};
+
+// Interface for a class which produces BeginFrame calls to a
+// BeginFrameObserver.
+//
+// BeginFrame calls *normally* occur just after a vsync interrupt when input
+// processing has been finished and provide information about the time values
+// of the vsync times. *However*, these values can be heavily modified or even
+// plain made up (when no vsync signal is available or vsync throttling is
+// turned off). See the BeginFrameObserver for information about the guarantees
+// all BeginFrameSources *must* provide.
+//
+// Base classes should:

[simonhong, 2014/09/23 04:39:09]

Sub-classes instead of Base classes?

[mithro-old, 2014/09/23 12:43:55]

Done.

+//  - Use the CallOn(Missed)BeginFrame method to call to the observer(s).
+//  - Implement the pure virtual methods.
+//  - Recommend (but not required) to call BeginFrameSource::AsValueInto in
+//    their own AsValueInto implementation.
+class CC_EXPORT BeginFrameSource {

[brianderson, 2014/09/23 01:31:06]

Do you want to land this version of BeginFrameSource first and then make it pure
virtual when we make the "OutputSurface" implement BeginFrameSource?

[mithro-old, 2014/09/23 12:43:55]

I think we are leaning towards abandoning this idea now. See further discussion
at http://crbug.com/416760

+ public:
+  virtual ~BeginFrameSource() {}
+
+  // SetNeedsBeginFrames is the on/off "switch" for the BeginFrameSource. When
+  // set to false no more BeginFrame messages should be sent to observer.
+  virtual bool NeedsBeginFrames() const = 0;
+  virtual void SetNeedsBeginFrames(bool needs_begin_frames) = 0;
+
+  // DidFinishFrame provides back pressure to a frame source about frame
+  // processing (rather than toggling SetNeedsBeginFrames every frame). It is
+  // used by systems like the BackToBackFrameSource to make sure only one frame
+  // is pending at a time.
+  virtual void DidFinishFrame(size_t remaining_frames) = 0;
+
+  // Add/Remove and observer from the source.
+  // *At the moment* only a single observer can be added to the source, however
+  // in the future this may be extended to allow multiple observers.
+  // If making this change, please use base::ObserverList to do so.
+  void AddObserver(BeginFrameObserver* obs);
+  void RemoveObserver(BeginFrameObserver* obs);
+
+  // Tracing support - Recommend (but not required) to call this implementation
+  // in any override.
+  virtual void AsValueInto(base::debug::TracedValue* dict) const;
+
+ protected:
+  BeginFrameSource();
+
+  // These methods should be used by subclasses to make the call to the
+  // observers.
+  void CallOnBeginFrame(const BeginFrameArgs& frame_info);
+  void CallOnMissedBeginFrame(const BeginFrameArgs& frame_info);
+
+  BeginFrameObserver* observer_;
+
+ private:
+  bool inside_as_value_into_;
+};
+
+// A frame source which calls BeginFrame (at the next possible time) as soon as
+// remaining frames reaches zero.
+class CC_EXPORT BackToBackBeginFrameSource : public BeginFrameSource {
+ public:
+  static scoped_ptr<BackToBackBeginFrameSource> Create(
+      base::SingleThreadTaskRunner* task_runner);
+  virtual ~BackToBackBeginFrameSource();
+
+  // BeginFrameSource
+  virtual bool NeedsBeginFrames() const OVERRIDE;
+  virtual void SetNeedsBeginFrames(bool needs_begin_frames) OVERRIDE;
+  virtual void DidFinishFrame(size_t remaining_frames) OVERRIDE;
+
+  // Tracing
+  virtual void AsValueInto(base::debug::TracedValue* dict) const OVERRIDE;
+
+ protected:
+  explicit BackToBackBeginFrameSource(
+      base::SingleThreadTaskRunner* task_runner);
+  virtual base::TimeTicks Now();  // Now overridable for testing
+
+  base::WeakPtrFactory<BackToBackBeginFrameSource> weak_factory_;
+  base::SingleThreadTaskRunner* task_runner_;
+
+  bool needs_begin_frames_;
+  bool send_begin_frame_posted_;
+
+  void ScheduleBeginFrame();
+  void BeginFrame();
+};
+
+// A frame source which is locked to an external parameters provides from a
+// vsync source and generates BeginFrameArgs for it.
+class CC_EXPORT SyntheticBeginFrameSource
+    : public BeginFrameSource,
+      public ui::CompositorVSyncManager::Observer,

[brianderson, 2014/09/23 01:31:06]

BrowserCompositorOutputSurface implements ui::CompositorVSyncManager::Observer.
We should be able to translate that to
OutputSurfaceClient::CommitVSyncParameters and thread it to the
SyntheticBeginFrameSource without depending on
ui::CompositorVSyncManager::Observer here.

Then, after the unified BeginFrame patch, I think we can get rid of
ui::CompositorVSyncManager::Observer since it becomes an unnecessary middle man
between BrowserCompositorOutputSurface::OnUpdateVSyncParametersFromGpu and
OutputSurfaceClient::CommitVSyncParameters.

[mithro-old, 2014/09/23 12:43:55]

I'm not sure I understand what you are saying correctly. I've created the new
VSyncParamaterObserver interface in cc/output/vsync_parameter_observer.h file.

+      public TimeSourceClient {
+ public:
+  static scoped_ptr<SyntheticBeginFrameSource> Create(
+      base::SingleThreadTaskRunner* task_runner,
+      base::TimeTicks initial_vsync_timebase,
+      base::TimeDelta initial_vsync_interval);
+  virtual ~SyntheticBeginFrameSource();
+
+  // BeginFrameSource
+  virtual bool NeedsBeginFrames() const OVERRIDE;
+  virtual void SetNeedsBeginFrames(bool needs_begin_frames) OVERRIDE;
+  virtual void DidFinishFrame(size_t remaining_frames) OVERRIDE{};
+
+  // Tracing
+  virtual void AsValueInto(base::debug::TracedValue* dict) const OVERRIDE;
+
+  // ui::CompositorVSyncManager::Observer

[simonhong, 2014/09/23 04:39:09]

If use use ui::CompositorVSyncManager::Observer for code re-use and want to use
observer pattern here, I think new observer class should be better than re-using
that. That observer class is used in the ui compositor layer.

Or,calling directly like primary_frame_source_->UpdateVSyncParameters() also
looks fine.

[mithro-old, 2014/09/23 12:43:55]

See my response to Brian's comment.

+  virtual void OnUpdateVSyncParameters(
+      base::TimeTicks new_vsync_timebase,
+      base::TimeDelta new_vsync_interval) OVERRIDE;
+
+  // TimeSourceClient
+  virtual void OnTimerTick() OVERRIDE;
+
+ protected:
+  explicit SyntheticBeginFrameSource(
+      scoped_refptr<DelayBasedTimeSource> time_source);
+
+  void SendBeginFrameFromTick(base::TimeTicks frame_time);
+
+  scoped_refptr<DelayBasedTimeSource> time_source_;
+};
+
+// A "virtual" frame source which lets you switch between multiple other frame
+// sources while making sure the BeginFrameArgs stays increasing (possibly
+// enforcing minimum boundry between BeginFrameArgs messages).
+class CC_EXPORT BeginFrameSourceMultiplexer : public BeginFrameSource,
+                                              public BeginFrameObserver {
+ public:
+  static scoped_ptr<BeginFrameSourceMultiplexer> Create();
+  virtual ~BeginFrameSourceMultiplexer();
+
+  void SetMinimumInterval(base::TimeDelta new_minimum_interval);
+
+  void AddSource(BeginFrameSource* new_source);
+  void RemoveSource(BeginFrameSource* existing_source);
+  void SetActiveSource(BeginFrameSource* new_source);
+  const BeginFrameSource* ActiveSource();
+
+  // BeginFrameObserver
+  // The mux is an BeginFrameObserver as it needs to proxy the OnBeginFrame
+  // calls to preserve the monotonicity of the BeginFrameArgs when switching
+  // sources.
+  virtual void OnBeginFrame(const BeginFrameArgs& frame_info) OVERRIDE;
+  virtual void OnMissedBeginFrame(const BeginFrameArgs& frame_info) OVERRIDE;
+  virtual const BeginFrameArgs LastBeginFrameArgs() const OVERRIDE;
+
+  // BeginFrameSource
+  virtual bool NeedsBeginFrames() const OVERRIDE;
+  virtual void SetNeedsBeginFrames(bool needs_begin_frames) OVERRIDE;
+  virtual void DidFinishFrame(size_t remaining_frames) OVERRIDE;
+
+  // Tracing
+  virtual void AsValueInto(base::debug::TracedValue* dict) const OVERRIDE;
+
+ protected:
+  BeginFrameSourceMultiplexer();
+  explicit BeginFrameSourceMultiplexer(base::TimeDelta minimum_interval);
+
+  bool HasSource(BeginFrameSource* source);
+  bool IsIncreasing(const BeginFrameArgs& frame_info);
+
+  base::TimeDelta minimum_interval_;
+
+  BeginFrameSource* active_source_;
+  std::set<BeginFrameSource*> source_list_;

[brianderson, 2014/09/23 01:31:06]

The frame sources should outlive the multiplexer, so I think we are good with
raw pointers here.

You only need weak pointers when you are afraid you might try to access the
pointer after it's been deallocated and it's okay to turn any calls on the weak
pointer into no-ops if they occur after deallocation. Usually this happens when
you post a task and then deallocate a dependency before the task actually
executes.

[mithro-old, 2014/09/23 12:43:54]

Okay. 

+};
+
+}  // namespace cc
+
+#endif  // CC_SCHEDULER_FRAME_SOURCE_H_