[android] Make RWHVAndroid a BeginFrameObserver.

ui/android/window_android.cc

Index: ui/android/window_android.cc
diff --git a/ui/android/window_android.cc b/ui/android/window_android.cc
index cff32ba6f571def1c787cd219296027d86f1cbc8..8bd5abc6c4ae9a22383880d89e5a7b7d6bb4992b 100644
--- a/ui/android/window_android.cc
+++ b/ui/android/window_android.cc
@@ -4,12 +4,18 @@
 
 #include "ui/android/window_android.h"
 
+#include <stdint.h>
+#include <unordered_set>
+
 #include "base/android/context_utils.h"
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
 #include "base/android/jni_weak_ref.h"
 #include "base/android/scoped_java_ref.h"
+#include "base/auto_reset.h"
+#include "cc/output/begin_frame_args.h"
+#include "cc/scheduler/begin_frame_source.h"
 #include "jni/WindowAndroid_jni.h"
 #include "ui/android/window_android_compositor.h"
 #include "ui/android/window_android_observer.h"
@@ -21,8 +27,104 @@ using base::android::JavaParamRef;
 using base::android::JavaRef;
 using base::android::ScopedJavaLocalRef;
 
+class WindowAndroid::WindowBeginFrameSource : public cc::BeginFrameSource {
+ public:
+  explicit WindowBeginFrameSource(WindowAndroid* window)
+      : window_(window),
+        in_on_vsync_(false) {}
+  ~WindowBeginFrameSource() override {}
+
+  // cc::BeginFrameSource implementation.
+  void AddObserver(cc::BeginFrameObserver* obs) override;
+  void RemoveObserver(cc::BeginFrameObserver* obs) override;
+  void DidFinishFrame(cc::BeginFrameObserver* obs,
+                      size_t remaining_frames) override {}
+  bool IsThrottled() const override { return true; }
+
+  void OnVSync(base::TimeTicks frame_time,
+               base::TimeDelta vsync_period);
+
+ private:
+  WindowAndroid* const window_;
+  std::unordered_set<cc::BeginFrameObserver*> observers_;
+  cc::BeginFrameArgs last_begin_frame_args_;
+  bool in_on_vsync_;
+};
+
+void WindowAndroid::WindowBeginFrameSource::AddObserver(
+    cc::BeginFrameObserver* obs) {
+  DCHECK(obs);
+  DCHECK(observers_.find(obs) == observers_.end());

[boliu, 2016/12/12 21:41:48]

nit: use base::ContainsKey? more readable

[Eric Seckler, 2016/12/13 10:03:56]

Done.

+
+  observers_.insert(obs);
+  obs->OnBeginFrameSourcePausedChanged(false);
+  window_->SetNeedsBeginFrames(true);
+
+  // Send a MISSED BeginFrame if possible and necessary. If an observer is added

[boliu, 2016/12/12 21:41:48]

any reason this is needed?

[Eric Seckler, 2016/12/13 10:03:56]

I'm somewhat new to BeginFrames myself, so brianderson@ might correct me here.
Since this source also feeds the DisplayScheduler, I don't think we can get rid
of missed BeginFrames. Here an attempt at an explanation :)

One note on background of (missed) BeginFrames vs. vsyncs:
WindowAndroidObservers don't remove themselves when they aren't interested in
BeginFrames, while BeginFrameObservers do. Thus, with WindowAndroidObservers,
even if just one observer requests a vsync, all observers will be notified of it
through OnVSync(). With BeginFrameObservers, this is not the case: Only those
that are interested will receive it.

Some observers may awake during a vsync interval (i.e. decide that they need a
BeginFrame afterall) -- and for those, receiving a MISSED BeginFrame might be
advantageous.

In the android-chrome case, I think one such BeginFrameObserver is the
DisplayScheduler: It may go idle (remove itself as observer) when a BeginFrame
doesn't lead to a DrawAndSwap. Later, it will awake again (add itself as
observer) when damage to a surface is detected. It'll then receive a MISSED
BeginFrame for the currently active BeginFrame and (likely) DrawAndSwap the
surface damage during that BeginFrame's deadline. If it didn't receive the
missed BeginFrame, it'd have to wait until the next BeginFrame's deadline.

[boliu, 2016/12/13 18:07:47]

Sorry, typed that comment before I realized this code was moved from
CompositorImpl.. No justification needed :p

+  // during OnVSync(), it will get a NORMAL BeginFrame from OnVSync() instead.
+  if (!in_on_vsync_ && last_begin_frame_args_.IsValid()) {
+    cc::BeginFrameArgs last_args = obs->LastUsedBeginFrameArgs();
+    if (!last_args.IsValid() ||
+        last_args.frame_time < last_begin_frame_args_.frame_time) {
+      last_begin_frame_args_.type = cc::BeginFrameArgs::MISSED;
+      // TODO(crbug.com/602485): A deadline doesn't make too much sense
+      // for a missed BeginFrame (the intention rather is 'immediately'),
+      // but currently the retro frame logic is very strict in discarding
+      // BeginFrames.
+      last_begin_frame_args_.deadline =
+          base::TimeTicks::Now() + last_begin_frame_args_.interval;
+      obs->OnBeginFrame(last_begin_frame_args_);
+    }
+  }
+}
+
+void WindowAndroid::WindowBeginFrameSource::RemoveObserver(
+    cc::BeginFrameObserver* obs) {
+  DCHECK(obs);
+  DCHECK(observers_.find(obs) != observers_.end());

[boliu, 2016/12/12 21:41:48]

ditto ContainsKey

[Eric Seckler, 2016/12/13 10:03:56]

Done.

+
+  observers_.erase(obs);
+  if (observers_.empty())
+    window_->SetNeedsBeginFrames(false);
+}
+
+void WindowAndroid::WindowBeginFrameSource::OnVSync(
+    base::TimeTicks frame_time,
+    base::TimeDelta vsync_period) {
+  base::AutoReset<bool> auto_reset(&in_on_vsync_, true);
+
+  // frame time is in the past, so give the next vsync period as the deadline.
+  base::TimeTicks deadline = frame_time + vsync_period;
+  last_begin_frame_args_ =
+      cc::BeginFrameArgs::Create(BEGINFRAME_FROM_HERE, frame_time, deadline,
+                                 vsync_period, cc::BeginFrameArgs::NORMAL);
+  DCHECK(last_begin_frame_args_.IsValid());
+
+  // Support SynchronousCompositorBrowserFilter: Observers added during vsync
+  // distribution receive a NORMAL args in addition to the missed ones, after
+  // all current observers were notified. We also have to support observers
+  // removing themselves during OnBeginFrame, so we iterate on set copies until
+  // all observers were notified once.
+  std::unordered_set<cc::BeginFrameObserver*> notified_observers;
+  bool observer_was_notified = true;
+  while (observer_was_notified) {

[boliu, 2016/12/12 21:41:48]

any reason we don't keep using base::ObserverList and keep getting this behavior
for free?

[Eric Seckler, 2016/12/13 10:03:56]

For two (rather subjective) reasons:
1) This makes it explicit that SyncCompBrowserFilter is supported.
2) We get an immediate feedback about whether |observers_| is empty during
RemoveObserver() -- which we wouldn't get with base::ObserverList.

That said, I think we could also use base::ObserverList and solve
1) by adding a comment somewhere.
2) by checking after observer iteration in OnVSync() whether the list became
empty during the iteration.

WDYT?

[boliu, 2016/12/13 18:07:47]

Or you can just keep a count of the number of observers inserted. Then you can
remove this complexity. Can still keep a comment about the requirement for
AddObserver and iteration behavior.

I guess ObserverList is implemented as a sparse array, so getting the number of
things is actually expensive..

[Eric Seckler, 2016/12/14 15:36:57]

Sounds good, done.

+    observer_was_notified = false;
+    std::unordered_set<cc::BeginFrameObserver*> observers(observers_);
+    for (auto* obs : observers) {
+      if (notified_observers.find(obs) == notified_observers.end()) {
+        obs->OnBeginFrame(last_begin_frame_args_);
+        notified_observers.insert(obs);
+        observer_was_notified = true;
+      }
+    }
+  }
+}
+
 WindowAndroid::WindowAndroid(JNIEnv* env, jobject obj, int display_id)
-    : display_id_(display_id), compositor_(NULL) {
+    : display_id_(display_id),
+      compositor_(NULL),
+      begin_frame_source_(new WindowBeginFrameSource(this)),
+      needs_begin_frames_(false) {
   java_window_.Reset(env, obj);
 }
 
@@ -69,6 +171,10 @@ void WindowAndroid::RemoveObserver(WindowAndroidObserver* observer) {
   observer_list_.RemoveObserver(observer);
 }
 
+cc::BeginFrameSource* WindowAndroid::GetBeginFrameSource() {
+  return begin_frame_source_.get();
+}
+
 void WindowAndroid::AttachCompositor(WindowAndroidCompositor* compositor) {
   if (compositor_ && compositor != compositor_)
     DetachCompositor();
@@ -90,6 +196,15 @@ void WindowAndroid::RequestVSyncUpdate() {
   Java_WindowAndroid_requestVSyncUpdate(env, GetJavaObject());
 }
 
+void WindowAndroid::SetNeedsBeginFrames(bool needs_begin_frames) {
+  if (needs_begin_frames_ == needs_begin_frames)
+    return;
+
+  needs_begin_frames_ = needs_begin_frames;
+  if (needs_begin_frames_)
+    RequestVSyncUpdate();
+}
+
 void WindowAndroid::SetNeedsAnimate() {
   if (compositor_)
     compositor_->SetNeedsAnimate();
@@ -107,10 +222,9 @@ void WindowAndroid::OnVSync(JNIEnv* env,
   base::TimeTicks frame_time(base::TimeTicks::FromInternalValue(time_micros));
   base::TimeDelta vsync_period(
       base::TimeDelta::FromMicroseconds(period_micros));
-  for (WindowAndroidObserver& observer : observer_list_)
-    observer.OnVSync(frame_time, vsync_period);
-  if (compositor_)
-    compositor_->OnVSync(frame_time, vsync_period);
+  begin_frame_source_->OnVSync(frame_time, vsync_period);
+  if (needs_begin_frames_)
+    RequestVSyncUpdate();
 }
 
 void WindowAndroid::OnVisibilityChanged(JNIEnv* env,