ui: Rework CompositorResizeLock interaction with DelegatedFrameHost

content/browser/renderer_host/delegated_frame_host.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 "content/browser/renderer_host/delegated_frame_host.h"
 
 #include <algorithm>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 33 matching lines @@
 
 DelegatedFrameHost::DelegatedFrameHost(const cc::FrameSinkId& frame_sink_id,
                                        DelegatedFrameHostClient* client)
     : frame_sink_id_(frame_sink_id),
       client_(client),
       compositor_(nullptr),
       tick_clock_(new base::DefaultTickClock()),
       skipped_frames_(false),
       background_color_(SK_ColorRED),
       current_scale_factor_(1.f),
-      can_lock_compositor_(YES_CAN_LOCK),
       delegated_frame_evictor_(new DelegatedFrameEvictor(this)) {
   ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
   factory->GetContextFactory()->AddObserver(this);
   factory->GetContextFactoryPrivate()->GetSurfaceManager()->RegisterFrameSinkId(
       frame_sink_id_);
   CreateCompositorFrameSinkSupport();
 }
 
 void DelegatedFrameHost::WasShown(const ui::LatencyInfo& latency_info) {
   delegated_frame_evictor_->SetVisible(true);
@@ skipping 11 matching lines @@
 bool DelegatedFrameHost::HasSavedFrame() {
   return delegated_frame_evictor_->HasFrame();
 }
 
 void DelegatedFrameHost::WasHidden() {
   delegated_frame_evictor_->SetVisible(false);
   released_front_lock_ = NULL;
 }
 
 void DelegatedFrameHost::MaybeCreateResizeLock() {
-  if (!ShouldCreateResizeLock())
+  DCHECK(!resize_lock_);
+
+  if (!compositor_)
     return;
-  DCHECK(compositor_);
 
-  bool defer_compositor_lock =
-      can_lock_compositor_ == NO_PENDING_RENDERER_FRAME ||
-      can_lock_compositor_ == NO_PENDING_COMMIT;
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kDisableResizeLock))
+    return;
 
-  if (can_lock_compositor_ == YES_CAN_LOCK)
-    can_lock_compositor_ = YES_DID_LOCK;
+  if (!client_->DelegatedFrameCanCreateResizeLock())
+    return;
+
+  gfx::Size desired_size = client_->DelegatedFrameHostDesiredSizeInDIP();
+  if (desired_size.IsEmpty())
+    return;
+  if (desired_size == current_frame_size_in_dip_)
+    return;
 
   resize_lock_ = client_->DelegatedFrameHostCreateResizeLock();
-  if (!defer_compositor_lock) {
-    bool locked = resize_lock_->Lock();
-    DCHECK(locked);
-  }
-}
-
-bool DelegatedFrameHost::ShouldCreateResizeLock() {
-  static const bool is_disabled =
-      base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kDisableResizeLock);
-  if (is_disabled)
-    return false;
-
-  if (!client_->DelegatedFrameCanCreateResizeLock())
-    return false;
-
-  if (resize_lock_)
-    return false;
-
-  gfx::Size desired_size = client_->DelegatedFrameHostDesiredSizeInDIP();
-  if (desired_size == current_frame_size_in_dip_ || desired_size.IsEmpty())
-    return false;
-
-  if (!compositor_)
-    return false;
-
-  return true;
+  bool locked = resize_lock_->Lock();
+  DCHECK(locked);
 }
 
 void DelegatedFrameHost::CopyFromCompositingSurface(
     const gfx::Rect& src_subrect,
     const gfx::Size& output_size,
     const ReadbackRequestCallback& callback,
     const SkColorType preferred_color_type) {
   // Only ARGB888 and RGB565 supported as of now.
   bool format_support = ((preferred_color_type == kAlpha_8_SkColorType) ||
                          (preferred_color_type == kRGB_565_SkColorType) ||
@@ skipping 111 matching lines @@
     // confirming newer sequence numbers.
     modified_ack.has_damage = false;
     modified_ack.latest_confirmed_sequence_number =
         latest_confirmed_begin_frame_sequence_number_;
   }
 
   support_->BeginFrameDidNotSwap(modified_ack);
 }
 
 bool DelegatedFrameHost::ShouldSkipFrame(gfx::Size size_in_dip) const {
-  // Should skip a frame only when another frame from the renderer is guaranteed
-  // to replace it. Otherwise may cause hangs when the renderer is waiting for
-  // the completion of latency infos (such as when taking a Snapshot.)
-  if (can_lock_compositor_ == NO_PENDING_RENDERER_FRAME ||
-      can_lock_compositor_ == NO_PENDING_COMMIT || !resize_lock_.get())
+  if (!resize_lock_)
     return false;
-
   return size_in_dip != resize_lock_->expected_size();
 }
 
 void DelegatedFrameHost::WasResized() {
   if (client_->DelegatedFrameHostDesiredSizeInDIP() !=
           current_frame_size_in_dip_ &&
       !client_->DelegatedFrameHostIsVisible())
     EvictDelegatedFrame();
-  MaybeCreateResizeLock();
+  if (!resize_lock_)
+    MaybeCreateResizeLock();
   UpdateGutters();
 }
 
 SkColor DelegatedFrameHost::GetGutterColor() const {
   // In fullscreen mode resizing is uncommon, so it makes more sense to
   // make the initial switch to fullscreen mode look better by using black as
   // the gutter color.
   return client_->DelegatedFrameHostGetGutterColor(background_color_);
 }
 
@@ skipping 199 matching lines @@
   current_frame_size_in_dip_ = frame_size_in_dip;
   CheckResizeLock();
 
   UpdateGutters();
 
   if (!damage_rect_in_dip.IsEmpty()) {
     client_->DelegatedFrameHostGetLayer()->OnDelegatedFrameDamage(
         damage_rect_in_dip);
   }
 
-  if (compositor_)
-    can_lock_compositor_ = NO_PENDING_COMMIT;
-
   if (has_frame_) {
     delegated_frame_evictor_->SwappedFrame(
         client_->DelegatedFrameHostIsVisible());
   }
   // Note: the frame may have been evicted immediately.
 
   DidFinishFrame(ack);
 }
 
 void DelegatedFrameHost::ClearDelegatedFrame() {
@@ skipping 199 matching lines @@
       video_frame->stride(media::VideoFrame::kVPlane),
       video_frame->data(media::VideoFrame::kVPlane), region_in_frame.origin(),
       finished_callback);
   media::LetterboxYUV(video_frame.get(), region_in_frame);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // DelegatedFrameHost, ui::CompositorObserver implementation:
 
 void DelegatedFrameHost::OnCompositingDidCommit(ui::Compositor* compositor) {
-  if (can_lock_compositor_ == NO_PENDING_COMMIT) {
-    can_lock_compositor_ = YES_CAN_LOCK;
-    if (resize_lock_ && resize_lock_->Lock())
-      can_lock_compositor_ = YES_DID_LOCK;
-  }
+  // If |create_resize_lock_after_commit_| then we should have popped the old
+  // lock already.
+  DCHECK(!resize_lock_ || !create_resize_lock_after_commit_);
+
   if (resize_lock_ &&
       resize_lock_->expected_size() == current_frame_size_in_dip_) {
     resize_lock_.reset();
-    client_->DelegatedFrameHostResizeLockWasReleased();
-    // We may have had a resize while we had the lock (e.g. if the lock expired,
-    // or if the UI still gave us some resizes), so make sure we grab a new lock
-    // if necessary.
+    // We had a lock but the UI may have resized in the meantime.
+    create_resize_lock_after_commit_ = true;
+  }
+
+  if (create_resize_lock_after_commit_) {
+    create_resize_lock_after_commit_ = false;
     MaybeCreateResizeLock();
   }
 }
 
 void DelegatedFrameHost::OnCompositingStarted(ui::Compositor* compositor,
                                               base::TimeTicks start_time) {
   last_draw_ended_ = start_time;
 }
 
 void DelegatedFrameHost::OnCompositingLockStateChanged(
     ui::Compositor* compositor) {
-  // A compositor lock that is part of a resize lock timed out. We
-  // should display a renderer frame.
-  if (!compositor->IsLocked() && can_lock_compositor_ == YES_DID_LOCK) {
-    can_lock_compositor_ = NO_PENDING_RENDERER_FRAME;
+  if (resize_lock_ && resize_lock_->timed_out()) {
+    // A compositor lock that is part of a resize lock timed out. We allow
+    // the UI to produce a frame before locking it again, so we don't lock here.
+    // We release the |resize_lock_| though to allow any other resizes that are
+    // desired at the same time since we're allowing the UI to make a frame
+    // which will gutter anyways.

[piman, 2017/03/31 00:31:55]

Ok, here I think I know why we wanted the NO_PENDING_RENDERER_FRAME state (with
create_resize_lock_after_commit_ = true being like the NO_PENDING_COMMIT state).
If we take a lock again at the next commit, if it happens before we got a frame
from the renderer, we won't grab the update (because ShouldSkipFrame will be
true) - so basically during the resize of a slow renderer, we may be stuck with
the first frame that we had, and not change until we stop resizing.

Now, I think that when we're in the ShouldSkipFrame case, we should still take
the renderer frame. I don't think there's a reason not to anymore, with
delegated rendering (it was important in the old days). That would fix it. wdyt?

[danakj, 2017/03/31 19:34:42]

Ah okay, so maybe allow_one_renderer_frame_ = true. But it's not tied to the UI
compositor importantly, as it used to be.

+    resize_lock_.reset();
+    create_resize_lock_after_commit_ = true;
   }
 }
 
 void DelegatedFrameHost::OnCompositingShuttingDown(ui::Compositor* compositor) {
   DCHECK_EQ(compositor, compositor_);
   ResetCompositor();
   DCHECK(!compositor_);
 }
 
 void DelegatedFrameHost::OnUpdateVSyncParameters(base::TimeTicks timebase,
@@ skipping 37 matching lines @@
   DCHECK(!vsync_manager_.get());
   vsync_manager_ = compositor_->vsync_manager();
   vsync_manager_->AddObserver(this);
 
   compositor_->AddFrameSink(frame_sink_id_);
 }
 
 void DelegatedFrameHost::ResetCompositor() {
   if (!compositor_)
     return;
-  if (resize_lock_) {
-    resize_lock_.reset();
-    client_->DelegatedFrameHostResizeLockWasReleased();
-  }
+  resize_lock_.reset();
   if (compositor_->HasObserver(this))
     compositor_->RemoveObserver(this);
   if (vsync_manager_) {
     vsync_manager_->RemoveObserver(this);
     vsync_manager_ = nullptr;
   }
 
   compositor_->RemoveFrameSink(frame_sink_id_);
   compositor_ = nullptr;
 }
@@ skipping 53 matching lines @@
   }
 
   if (!skipped_frames_) {
     latest_confirmed_begin_frame_source_id_ = ack.source_id;
     latest_confirmed_begin_frame_sequence_number_ =
         ack.latest_confirmed_sequence_number;
   }
 }
 
 }  // namespace content