Improve timing of placeholder image on Chrome resume

Currently, when Chrome is foregrounded, Android OS shows a placeholder
image of the last screen rendered. It does this until Chrome fires the
surfaceRedrawAsync callback. Chrome currently does this as soon as it
produces its first frame after foregrounding.

Unfortunately, Chrome doesn't wait for renderer content before
producing a frame, so the first frame may include a blank renderer.
This is especially common on low-end devices which produce frames
more slowly. In these cases, we see the placeholder, then a blank
renderer, then a populated renderer, which produces a flickering
effect.

This CL ports the ui/compositor's compositor lock to the android
CompositorImpl. We take this lock when a renderer first connects to the
browser, and release it when the renderer produces a frame. This
prevents the browser from producing a frame until it has renderer
content (or a failsafe timeout has been reached).

Note that the VrCompositor used to provide similar functionality by
calling DeferComimits direclty. This code wasn't working as intended
and after discussion with the VR team has been removed.

Bug: 792120
Change-Id: I011428bfb75e7455aae00426258ea05daf89fb38
Reviewed-on: https://chromium-review.googlesource.com/832959
Commit-Queue: Eric Karl <ericrk@chromium.org>
Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
Reviewed-by: ccameron <ccameron@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527398}

chrome/browser/android/vr_shell/vr_compositor.cc

@@ -56,10 +56,6 @@ void VrCompositor::SetWindowBounds(gfx::Size size) {
   compositor_->SetWindowBounds(size);
 }
 
-void VrCompositor::SetDeferCommits(bool defer_commits) {
-  compositor_->SetDeferCommits(defer_commits);
-}
-
 void VrCompositor::SurfaceChanged(jobject surface) {
   compositor_->SetSurface(surface);
 }

chrome/browser/android/vr_shell/vr_compositor.h

@@ -36,7 +36,6 @@ class VrCompositor : public content::CompositorClient {
 
   void SurfaceDestroyed();
   void SetWindowBounds(gfx::Size size);
-  void SetDeferCommits(bool defer_commits);
   void SurfaceChanged(jobject surface);
   void SetLayer(content::WebContents* web_contents);
 

chrome/browser/android/vr_shell/vr_shell.cc

@@ -148,10 +148,6 @@ VrShell::VrShell(JNIEnv* env,
   g_instance = this;
   j_vr_shell_.Reset(env, obj);
 
-  // Defer applying commits to the renderer until we know the desired
-  // content resolution and DPR.
-  compositor_->SetDeferCommits(true);
-
   gl_thread_ = base::MakeUnique<VrGLThread>(
       weak_ptr_factory_.GetWeakPtr(), main_thread_task_runner_, gvr_api,
       ui_initial_state, reprojected_rendering_, HasDaydreamSupport(env));
@@ -776,7 +772,6 @@ void VrShell::OnContentScreenBoundsChanged(const gfx::SizeF& bounds) {
   JNIEnv* env = base::android::AttachCurrentThread();
   Java_VrShellImpl_setContentCssSize(env, j_vr_shell_, window_size.width(),
                                      window_size.height(), dpr);
-  compositor_->SetDeferCommits(false);
 }
 
 void VrShell::SetVoiceSearchActive(bool active) {

content/browser/BUILD.gn

@@ -2114,6 +2114,7 @@ jumbo_source_set("browser") {
       "//media/capture/content/android",
       "//media/capture/video/android",
       "//ui/android",
+      "//ui/compositor",
     ]
     defines += [
       "APPCACHE_USE_SIMPLE_CACHE",

content/browser/android/overscroll_controller_android_unittest.cc

@@ -33,6 +33,7 @@ namespace {
 
 class MockCompositor : public WindowAndroidCompositor {
  public:
+  virtual ~MockCompositor() {}
   void AttachLayerForReadback(scoped_refptr<cc::Layer>) override {}
   void RequestCopyOfOutputOnRootLayer(
       std::unique_ptr<viz::CopyOutputRequest>) override {}
@@ -41,6 +42,11 @@ class MockCompositor : public WindowAndroidCompositor {
   MOCK_METHOD0(GetFrameSinkId, viz::FrameSinkId());
   void AddChildFrameSink(const viz::FrameSinkId& frame_sink_id) override {}
   void RemoveChildFrameSink(const viz::FrameSinkId& frame_sink_id) override {}
+  std::unique_ptr<ui::CompositorLock> GetCompositorLock(
+      ui::CompositorLockClient* client,
+      base::TimeDelta timeout) override {
+    return nullptr;
+  }
 };
 
 class MockGlowClient : public OverscrollGlowClient {

content/browser/renderer_host/compositor_impl_android.cc

@@ -484,6 +484,7 @@ CompositorImpl::CompositorImpl(CompositorClient* client,
       needs_animate_(false),
       pending_frames_(0U),
       layer_tree_frame_sink_request_pending_(false),
+      lock_manager_(base::ThreadTaskRunnerHandle::Get(), this),
       weak_factory_(this) {
   GetHostFrameSinkManager()->RegisterFrameSinkId(frame_sink_id_, this);
 #if DCHECK_IS_ON()
@@ -648,10 +649,6 @@ void CompositorImpl::SetWindowBounds(const gfx::Size& size) {
   root_window_->GetLayer()->SetBounds(size);
 }
 
-void CompositorImpl::SetDeferCommits(bool defer_commits) {
-  host_->SetDeferCommits(defer_commits);
-}
-
 void CompositorImpl::SetRequiresAlphaChannel(bool flag) {
   requires_alpha_channel_ = flag;
 }
@@ -961,4 +958,15 @@ bool CompositorImpl::HavePendingReadbacks() {
   return !readback_layer_tree_->children().empty();
 }
 
+std::unique_ptr<ui::CompositorLock> CompositorImpl::GetCompositorLock(
+    ui::CompositorLockClient* client,
+    base::TimeDelta timeout) {
+  return lock_manager_.GetCompositorLock(client, timeout);
+}
+
+void CompositorImpl::OnCompositorLockStateChanged(bool locked) {
+  if (host_)
+    host_->SetDeferCommits(locked);
+}
+
 }  // namespace content

content/browser/renderer_host/compositor_impl_android.h

@@ -28,6 +28,7 @@
 #include "ui/android/resources/resource_manager_impl.h"
 #include "ui/android/resources/ui_resource_provider.h"
 #include "ui/android/window_android_compositor.h"
+#include "ui/compositor/compositor_lock.h"
 #include "ui/display/display_observer.h"
 
 struct ANativeWindow;
@@ -57,6 +58,7 @@ class CONTENT_EXPORT CompositorImpl
     : public Compositor,
       public cc::LayerTreeHostClient,
       public cc::LayerTreeHostSingleThreadClient,
+      public ui::CompositorLockManagerClient,
       public ui::UIResourceProvider,
       public ui::WindowAndroidCompositor,
       public viz::HostFrameSinkClient,
@@ -82,7 +84,6 @@ class CONTENT_EXPORT CompositorImpl
   void SetSurface(jobject surface) override;
   void SetBackgroundColor(int color) override;
   void SetWindowBounds(const gfx::Size& size) override;
-  void SetDeferCommits(bool defer_commits) override;
   void SetRequiresAlphaChannel(bool flag) override;
   void SetNeedsComposite() override;
   ui::UIResourceProvider& GetUIResourceProvider() override;
@@ -124,6 +125,9 @@ class CONTENT_EXPORT CompositorImpl
   viz::FrameSinkId GetFrameSinkId() override;
   void AddChildFrameSink(const viz::FrameSinkId& frame_sink_id) override;
   void RemoveChildFrameSink(const viz::FrameSinkId& frame_sink_id) override;
+  std::unique_ptr<ui::CompositorLock> GetCompositorLock(
+      ui::CompositorLockClient* client,
+      base::TimeDelta timeout) override;
 
   // viz::HostFrameSinkClient implementation.
   void OnFirstSurfaceActivation(const viz::SurfaceInfo& surface_info) override;
@@ -133,6 +137,9 @@ class CONTENT_EXPORT CompositorImpl
   void OnDisplayMetricsChanged(const display::Display& display,
                                uint32_t changed_metrics) override;
 
+  // ui::CompositorLockManagerClient implementation.
+  void OnCompositorLockStateChanged(bool locked) override;
+
   void SetVisible(bool visible);
   void CreateLayerTreeHost();
 
@@ -195,6 +202,7 @@ class CONTENT_EXPORT CompositorImpl
   bool has_layer_tree_frame_sink_ = false;
   std::unordered_set<viz::FrameSinkId, viz::FrameSinkIdHash>
       pending_child_frame_sink_ids_;
+  ui::CompositorLockManager lock_manager_;
   base::WeakPtrFactory<CompositorImpl> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(CompositorImpl);

content/public/browser/android/compositor.h

@@ -65,9 +65,6 @@ class CONTENT_EXPORT Compositor {
   // Set the output surface bounds.
   virtual void SetWindowBounds(const gfx::Size& size) = 0;
 
-  // Defer commits on the layer tree host.
-  virtual void SetDeferCommits(bool defer_commits) = 0;
-
   // Set the output surface which the compositor renders into.
   virtual void SetSurface(jobject surface) = 0;
 

ui/android/BUILD.gn

@@ -65,6 +65,7 @@ component("android") {
     "//skia",
     "//third_party/WebKit/public:blink_headers",
     "//ui/base",
+    "//ui/compositor",
     "//ui/display",
     "//ui/events",
     "//ui/events/devices",

ui/android/DEPS

@@ -16,6 +16,7 @@ include_rules = [
   "+third_party/WebKit/public/platform/WebCursorInfo.h",
   "+third_party/skia",
   "+ui/base",
+  "+ui/compositor/compositor_lock.h",
   "+ui/display",
   "+ui/events",
   "+ui/gfx",

ui/android/delegated_frame_host_android.cc

@@ -104,6 +104,10 @@ void DelegatedFrameHostAndroid::SubmitCompositorFrame(
   } else {
     support_->SubmitCompositorFrame(local_surface_id, std::move(frame));
   }
+
+  if (compositor_attach_until_frame_lock_) {
+    compositor_attach_until_frame_lock_.reset();
+  }
 }
 
 void DelegatedFrameHostAndroid::DidNotProduceFrame(
@@ -166,6 +170,12 @@ void DelegatedFrameHostAndroid::AttachToCompositor(
     WindowAndroidCompositor* compositor) {
   if (registered_parent_compositor_)
     DetachFromCompositor();
+  // Take the compositor lock, preventing frames from being displayed until
+  // we've produced content. Set a 5 second timeout to prevent locking up the
+  // browser in cases where the renderer hangs or another factor prevents a
+  // frame from being produced.
+  compositor_attach_until_frame_lock_ =
+      compositor->GetCompositorLock(this, base::TimeDelta::FromSeconds(5));
   compositor->AddChildFrameSink(frame_sink_id_);
   client_->SetBeginFrameSource(&begin_frame_source_);
   registered_parent_compositor_ = compositor;
@@ -174,6 +184,8 @@ void DelegatedFrameHostAndroid::AttachToCompositor(
 void DelegatedFrameHostAndroid::DetachFromCompositor() {
   if (!registered_parent_compositor_)
     return;
+  if (compositor_attach_until_frame_lock_)
+    compositor_attach_until_frame_lock_.reset();
   client_->SetBeginFrameSource(nullptr);
   support_->SetNeedsBeginFrame(false);
   registered_parent_compositor_->RemoveChildFrameSink(frame_sink_id_);
@@ -222,6 +234,8 @@ void DelegatedFrameHostAndroid::OnFrameTokenChanged(uint32_t frame_token) {
   client_->OnFrameTokenChanged(frame_token);
 }
 
+void DelegatedFrameHostAndroid::CompositorLockTimedOut() {}
+
 void DelegatedFrameHostAndroid::CreateNewCompositorFrameSinkSupport() {
   constexpr bool is_root = false;
   constexpr bool needs_sync_points = true;

ui/android/delegated_frame_host_android.h

@@ -14,6 +14,7 @@
 #include "components/viz/service/frame_sinks/compositor_frame_sink_support.h"
 #include "services/viz/public/interfaces/compositing/compositor_frame_sink.mojom.h"
 #include "ui/android/ui_android_export.h"
+#include "ui/compositor/compositor_lock.h"
 
 namespace cc {
 class SurfaceLayer;
@@ -33,7 +34,8 @@ class WindowAndroidCompositor;
 class UI_ANDROID_EXPORT DelegatedFrameHostAndroid
     : public viz::mojom::CompositorFrameSinkClient,
       public viz::ExternalBeginFrameSourceClient,
-      public viz::HostFrameSinkClient {
+      public viz::HostFrameSinkClient,
+      public ui::CompositorLockClient {
  public:
   class Client {
    public:
@@ -102,6 +104,9 @@ class UI_ANDROID_EXPORT DelegatedFrameHostAndroid
   void OnFirstSurfaceActivation(const viz::SurfaceInfo& surface_info) override;
   void OnFrameTokenChanged(uint32_t frame_token) override;
 
+  // ui::CompositorLockClient implementation.
+  void CompositorLockTimedOut() override;
+
   void CreateNewCompositorFrameSinkSupport();
 
   const viz::FrameSinkId frame_sink_id_;
@@ -121,6 +126,11 @@ class UI_ANDROID_EXPORT DelegatedFrameHostAndroid
 
   scoped_refptr<cc::SurfaceLayer> content_layer_;
 
+  // A lock that is held from the point at which we attach to the compositor to
+  // the point at which we submit our first frame to the compositor. This
+  // ensures that the compositor doesn't swap without a frame available.
+  std::unique_ptr<ui::CompositorLock> compositor_attach_until_frame_lock_;
+
   DISALLOW_COPY_AND_ASSIGN(DelegatedFrameHostAndroid);
 };
 

ui/android/window_android_compositor.h

@@ -10,6 +10,7 @@
 #include "components/viz/common/frame_sinks/copy_output_request.h"
 #include "components/viz/common/surfaces/frame_sink_id.h"
 #include "ui/android/ui_android_export.h"
+#include "ui/compositor/compositor_lock.h"
 
 namespace cc {
 class Layer;
@@ -22,8 +23,6 @@ class ResourceManager;
 // Android interface for compositor-related tasks.
 class UI_ANDROID_EXPORT WindowAndroidCompositor {
  public:
-  virtual ~WindowAndroidCompositor() {}
-
   virtual void AttachLayerForReadback(scoped_refptr<cc::Layer> layer) = 0;
   virtual void RequestCopyOfOutputOnRootLayer(
       std::unique_ptr<viz::CopyOutputRequest> request) = 0;
@@ -32,6 +31,9 @@ class UI_ANDROID_EXPORT WindowAndroidCompositor {
   virtual viz::FrameSinkId GetFrameSinkId() = 0;
   virtual void AddChildFrameSink(const viz::FrameSinkId& frame_sink_id) = 0;
   virtual void RemoveChildFrameSink(const viz::FrameSinkId& frame_sink_id) = 0;
+  virtual std::unique_ptr<CompositorLock> GetCompositorLock(
+      CompositorLockClient* client,
+      base::TimeDelta timeout) = 0;
 };
 
 }  // namespace ui