Android: Browser-side scheduling latency tweaks

content/browser/renderer_host/compositor_impl_android.cc

 // Copyright (c) 2012 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 "content/browser/renderer_host/compositor_impl_android.h"
 
 #include <android/bitmap.h>
 #include <android/native_window_jni.h>
 #include <stdint.h>
 #include <utility>
@@ skipping 151 matching lines @@
   ExternalBeginFrameSource(CompositorImpl* compositor)
       : compositor_(compositor) {
     compositor_->AddObserver(this);
   }
 
   ~ExternalBeginFrameSource() override {
     compositor_->RemoveObserver(this);
   }
 
   // cc::BeginFrameSourceBase implementation:
+  void AddObserver(cc::BeginFrameObserver* obs) override {
+    cc::BeginFrameSourceBase::AddObserver(obs);
+    DCHECK(needs_begin_frames());
+    // Send a MISSED begin frame if necessary.
+    if (last_begin_frame_args_.IsValid()) {

[enne (OOO), 2016/04/18 21:36:01]

style nit: I'd early out here instead of nesting all these conditions.

[no sievers, 2016/04/22 19:53:11]

Done.

+      cc::BeginFrameArgs last_args = obs->LastUsedBeginFrameArgs();
+      if (!last_args.IsValid() ||
+          (last_begin_frame_args_.frame_time > last_args.frame_time)) {
+        last_begin_frame_args_.type = cc::BeginFrameArgs::MISSED;

[no sievers, 2016/04/13 00:23:53]

Btw not using MISSED here breaks the world and I'm wondering if it's because of
the reentrency into OnBeginFrame() from AddObserver().

+        // We know that the scheduler wants to draw, so the deadline
+        // doesn't matter much. But pass a timestamp that won't expire
+        // soon, so we don't get discarded in BeginRetroFrame(), see
+        // crbug.com/602485.
+        last_begin_frame_args_.deadline = base::TimeTicks::Now() +
+            base::TimeDelta::FromMilliseconds(16);

[enne (OOO), 2016/04/18 21:36:01]

With https://codereview.chromium.org/1887243002, is this still needed?

If it is, maybe use the BeginFrameArgs::interval instead of a hardcoded number?

[no sievers, 2016/04/22 19:53:11]

I made it a TODO and reworded the comment, since I think Sunny will be able to
remove the (a bit nonsensical) deadline here it when consolidating the missed
frame logic in BFSBase.
Done.

+        obs->OnBeginFrame(last_begin_frame_args_);
+      }
+    }
+  }
+
   void OnNeedsBeginFramesChanged(bool needs_begin_frames) override {
+    TRACE_EVENT1("compositor", "OnNeedsBeginFramesChanged",
+                 "needs_begin_frames", needs_begin_frames);
     compositor_->OnNeedsBeginFramesChange(needs_begin_frames);
   }
 
   // CompositorImpl::VSyncObserver implementation:
   void OnVSync(base::TimeTicks frame_time,
                base::TimeDelta vsync_period) override {
-    CallOnBeginFrame(cc::BeginFrameArgs::Create(
-        BEGINFRAME_FROM_HERE, frame_time, base::TimeTicks::Now(), vsync_period,
-        cc::BeginFrameArgs::NORMAL));
+    base::TimeTicks deadline = frame_time + 2 * vsync_period / 3;

[enne (OOO), 2016/04/18 21:36:01]

Where does this come from?

[no sievers, 2016/04/22 19:53:11]

Removed. It's not needed as long as the scheduler doesn't somehow immediately
send a BeginMainFrame as long as nothing changed, which seems reasonable
behavior.
see https://bugs.chromium.org/p/chromium/issues/detail?id=603331#c1

+    last_begin_frame_args_ =
+        cc::BeginFrameArgs::Create(BEGINFRAME_FROM_HERE, frame_time, deadline,
+                                   vsync_period, cc::BeginFrameArgs::NORMAL);
+    CallOnBeginFrame(last_begin_frame_args_);
   }
 
  private:
   CompositorImpl* compositor_;
+  cc::BeginFrameArgs last_begin_frame_args_;
 };
 
 static bool g_initialized = false;
 
 base::LazyInstance<cc::SurfaceManager> g_surface_manager =
     LAZY_INSTANCE_INITIALIZER;
 
 int g_surface_id_namespace = 0;
 
 class SingleThreadTaskGraphRunner : public cc::SingleThreadTaskGraphRunner {
@@ skipping 203 matching lines @@
 
 void CompositorImpl::SetHasTransparentBackground(bool flag) {
   has_transparent_background_ = flag;
   if (host_)
     host_->set_has_transparent_background(flag);
 }
 
 void CompositorImpl::SetNeedsComposite() {
   if (!host_->visible())
     return;
+  TRACE_EVENT0("compositor", "Compositor::SetNeedsComposite");
   host_->SetNeedsAnimate();
 }
 
 static scoped_ptr<WebGraphicsContext3DCommandBufferImpl>
 CreateGpuProcessViewContext(
     const scoped_refptr<gpu::GpuChannelHost>& gpu_channel_host,
     const gpu::gles2::ContextCreationAttribHelper& attributes,
     int surface_id) {
   GURL url("chrome://gpu/Compositor::createContext3D");
   static const size_t kBytesPerPixel = 4;
@@ skipping 159 matching lines @@
 void CompositorImpl::AddObserver(VSyncObserver* observer) {
   observer_list_.AddObserver(observer);
 }
 
 void CompositorImpl::RemoveObserver(VSyncObserver* observer) {
   observer_list_.RemoveObserver(observer);
 }
 
 cc::UIResourceId CompositorImpl::CreateUIResource(
     cc::UIResourceClient* client) {
+  TRACE_EVENT0("compositor", "CompositorImpl::CreateUIResource");
   return host_->CreateUIResource(client);
 }
 
 void CompositorImpl::DeleteUIResource(cc::UIResourceId resource_id) {
+  TRACE_EVENT0("compositor", "CompositorImpl::DeleteUIResource");
   host_->DeleteUIResource(resource_id);
 }
 
 bool CompositorImpl::SupportsETC1NonPowerOfTwo() const {
   return gpu_capabilities_.texture_format_etc1_npot;
 }
 
 void CompositorImpl::DidPostSwapBuffers() {
   TRACE_EVENT0("compositor", "CompositorImpl::DidPostSwapBuffers");
   pending_swapbuffers_++;
@@ skipping 40 matching lines @@
   needs_begin_frames_ = needs_begin_frames;
   if (needs_begin_frames_)
     root_window_->RequestVSyncUpdate();
 }
 
 void CompositorImpl::SetNeedsAnimate() {
   needs_animate_ = true;
   if (!host_->visible())
     return;
 
+  TRACE_EVENT0("compositor", "Compositor::SetNeedsAnimate");
   host_->SetNeedsAnimate();
 }
 
 }  // namespace content