cc: Adding BeginFrameTracker object and removing Now() from LTHI.

cc/scheduler/begin_frame_tracker.cc

+// 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.
+
+#include "cc/scheduler/begin_frame_tracker.h"
+
+namespace cc {
+
+BeginFrameTracker::BeginFrameTracker(const tracked_objects::Location& location)
+    : location_(location),
+      location_string_(location.ToString()),
+      current_updated_at_(),
+      current_args_(),
+      current_finished_at_(base::TimeTicks::FromInternalValue(-1)) {
+}
+
+BeginFrameTracker::~BeginFrameTracker() {
+}
+
+void BeginFrameTracker::Start(BeginFrameArgs new_args) {
+  // Trace the frame time being passed between BeginFrameTrackers.
+  TRACE_EVENT_FLOW_STEP0(
+      TRACE_DISABLED_BY_DEFAULT("cc.debug.scheduler.frames"), "BeginFrameArgs",
+      new_args.frame_time.ToInternalValue(), location_string_);
+
+  // Trace this specific begin frame tracker Start/Finish times.
+  TRACE_EVENT_ASYNC_BEGIN2(
+      TRACE_DISABLED_BY_DEFAULT("cc.debug.scheduler.frames"),
+      location_string_.c_str(), new_args.frame_time.ToInternalValue(),
+      "new args", new_args.AsValue(), "current args", current_args_.AsValue());
+
+  // Check the new BeginFrameArgs are valid and monotonically increasing.
+  DCHECK(new_args.IsValid());
+  DCHECK_LT(current_args_.frame_time, new_args.frame_time);
+
+  DCHECK(HasFinished())
+      << "Tried to start a new frame before finishing an existing frame.";
+  current_updated_at_ = base::TimeTicks::NowFromSystemTraceTime();
+  current_args_ = new_args;
+  current_finished_at_ = base::TimeTicks();
+
+  // TODO(mithro): Add UMA tracking of delta between current_updated_at_ time
+  // and the new_args.frame_time argument. This will give us how long after a
+  // BeginFrameArgs message was created before we started processing it.
+}
+
+const BeginFrameArgs& BeginFrameTracker::Get() const {
+  DCHECK(!HasFinished())
+      << "Tried to use BeginFrameArgs after marking the frame finished.";
+  DCHECK(current_args_.IsValid())
+      << "Tried to use BeginFrameArgs before starting a frame!";
+  return current_args_;
+}
+
+void BeginFrameTracker::Finish() {

[brianderson, 2014/12/18 02:01:38]

Or is this important for debug / frame timing purposes? Is it important for
anything else? If it's just for debug purposes, is this where it belongs?

[mithro-old, 2014/12/18 17:22:40]

The start/finish methods help control access to the BFA object which prevent
accidentally using the wrong BFA value. For example, if the LTHI thinks it's
finished with a BFA then nobody else should be trying to access it as it could
be updated by a new BeginFrame message at any time.

It also gives a very strong sanity check that we are only starting / finishing
in one location.

It's modelled on the current Update/Reset system which I actually think was
pretty good until people started working around it.

It also allows us to track the BFA usage. For example, did we use end up using
BFA for 40ms? If so that means we must have missed 2 other BFA objects.

All this information appears in tracing when you enable the traces below too.

+  DCHECK(!HasFinished()) << "Tried to finish an already finished frame";
+  current_finished_at_ = base::TimeTicks::NowFromSystemTraceTime();
+  TRACE_EVENT_ASYNC_END0(TRACE_DISABLED_BY_DEFAULT("cc.debug.scheduler.frames"),
+                         location_string_.c_str(),
+                         current_args_.frame_time.ToInternalValue());
+}
+
+const BeginFrameArgs& BeginFrameTracker::Last() const {
+  DCHECK(current_args_.IsValid())
+      << "Tried to use last BeginFrameArgs before starting a frame!";
+  DCHECK(HasFinished())
+      << "Tried to use last BeginFrameArgs before the frame is finished.";
+  return current_args_;

[brianderson, 2014/12/18 02:01:38]

Would it make sense for this to always return the last finished frame args,
regardless of whether a new one started or not? Seems a bit weird to track
Started/Finished state to determine whether this method can be called.

[mithro-old, 2014/12/18 17:22:40]

I want the caller to make a conscious decision about which BFA they want to use.
I don't want people just choose to use Last() because its easier and they are
feeling lazy.

It also provides a nice duality. When Get() is valid to call, Last() DCHECK
fails. When Last() is valid to call, Get() DCHECK fails.

Code which does the following;
 if (tracker.HasFinished())
  return tracker.Last();
 else
  return tracker.Get();

Should be treated with extreme suspicion because it is most certainly wrong in
some way. I want to make the HasFinished() method protected in the future but
can't do that right now.

+}
+
+base::TimeDelta BeginFrameTracker::Interval() const {
+  base::TimeDelta interval = current_args_.interval;
+  // Normal interval will be ~16ms, 200Hz (5ms) screens are the fastest
+  // easily available so anything less than that is likely an error.
+  if (interval < base::TimeDelta::FromMilliseconds(4)) {
+    interval = BeginFrameArgs::DefaultInterval();
+  }
+  return interval;
+}
+
+}  // namespace cc