DelegatedFrameHost should not create an ExternalBeginFrameSource

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 41 matching lines @@
       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();
-  begin_frame_source_ = base::MakeUnique<cc::ExternalBeginFrameSource>(this);
-  client_->SetBeginFrameSource(begin_frame_source_.get());
 }
 
 void DelegatedFrameHost::WasShown(const ui::LatencyInfo& latency_info) {
   delegated_frame_evictor_->SetVisible(true);
 
   if (!has_frame_ && !released_front_lock_.get()) {
     if (compositor_)
       released_front_lock_ = compositor_->GetCompositorLock();
   }
 
@@ skipping 150 matching lines @@
     RenderWidgetHostViewBase* target_view,
     gfx::Point* transformed_point) {
   if (!has_frame_)
     return false;
 
   return target_view->TransformPointToLocalCoordSpace(
       point, cc::SurfaceId(frame_sink_id_, local_surface_id_),
       transformed_point);
 }
 
+void DelegatedFrameHost::SetNeedsBeginFrames(bool needs_begin_frames) {
+  needs_begin_frame_ = needs_begin_frames;
+  support_->SetNeedsBeginFrame(needs_begin_frames);
+}
+
+void DelegatedFrameHost::BeginFrameDidNotSwap(const cc::BeginFrameAck& ack) {
+  if (ack.source_id != latest_confirmed_begin_frame_source_id_) {

[Eric Seckler, 2017/03/27 13:05:03]

The code that was previously OnDidFinishFrame() still needs to be executed after
SwapDelegatedFrame, so that
latest_confirmed_begin_frame_source_id_/sequence_number_ are updated.

You'll probably need to keep OnDidFinishFrame (feel free to rename), which you
call from here and after SwapDelegatedFrame.

[Saman Sami, 2017/03/27 17:10:23]

Done.

+    // Source changed, we don't know our freshness anymore.
+    latest_confirmed_begin_frame_sequence_number_ =
+        cc::BeginFrameArgs::kInvalidFrameNumber;
+  }
+
+  cc::BeginFrameAck modified_ack = ack;
+  if (skipped_frames_) {
+    // If we skipped the last frame(s), we didn't incorporate the last
+    // CompositorFrame's damage, so need to wait for the next one before
+    // confirming newer sequence numbers.
+    modified_ack.has_damage = false;
+    modified_ack.latest_confirmed_sequence_number =
+        latest_confirmed_begin_frame_sequence_number_;
+  } else {
+    latest_confirmed_begin_frame_source_id_ = modified_ack.source_id;
+    latest_confirmed_begin_frame_sequence_number_ =
+        modified_ack.latest_confirmed_sequence_number;
+  }
+
+  // If there was damage, the unmodified ack was sent with the CompositorFrame.
+  DCHECK(!modified_ack.has_damage);
+  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())
     return false;
 
   return size_in_dip != resize_lock_->expected_size();
 }
@@ skipping 153 matching lines @@
     cc::ReturnedResourceArray resources;
     cc::TransferableResource::ReturnResources(frame.resource_list, &resources);
 
     skipped_latency_info_list_.insert(skipped_latency_info_list_.end(),
                                       frame.metadata.latency_info.begin(),
                                       frame.metadata.latency_info.end());
 
     client_->DelegatedFrameHostSendReclaimCompositorResources(
         compositor_frame_sink_id, true /* is_swap_ack*/, resources);
     skipped_frames_ = true;
+    BeginFrameDidNotSwap(frame.metadata.begin_frame_ack);
     return;
   }
 
   if (skipped_frames_) {
     skipped_frames_ = false;
     damage_rect = gfx::Rect(frame_size);
     damage_rect_in_dip = gfx::Rect(frame_size_in_dip);
 
     // Give the same damage rect to the compositor.
     cc::RenderPass* root_pass = frame.render_pass_list.back().get();
@@ skipping 88 matching lines @@
   // Frame subscribers are only interested in changes to the target surface, so
   // do not attempt capture if |damage_rect| is empty.  This prevents the draws
   // of parent surfaces from triggering extra frame captures, which can affect
   // smoothness.
   if (id != local_surface_id_ || damage_rect.IsEmpty())
     return;
   AttemptFrameSubscriberCapture(damage_rect);
 }
 
 void DelegatedFrameHost::OnBeginFrame(const cc::BeginFrameArgs& args) {
-  begin_frame_source_->OnBeginFrame(args);
+  client_->OnBeginFrame(args);
 }
 
 void DelegatedFrameHost::EvictDelegatedFrame() {
   if (!has_frame_)
     return;
   client_->DelegatedFrameHostGetLayer()->SetShowSolidColorContent();
   support_->EvictFrame();
   has_frame_ = false;
   delegated_frame_evictor_->DiscardedFrame();
   UpdateGutters();
@@ skipping 223 matching lines @@
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // DelegatedFrameHost, private:
 
 DelegatedFrameHost::~DelegatedFrameHost() {
   DCHECK(!compositor_);
   ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
   factory->GetContextFactory()->RemoveObserver(this);
 
-  begin_frame_source_.reset();
   ResetCompositorFrameSinkSupport();
 
   factory->GetContextFactoryPrivate()
       ->GetSurfaceManager()
       ->InvalidateFrameSinkId(frame_sink_id_);
 
   DCHECK(!vsync_manager_.get());
 }
 
 void DelegatedFrameHost::SetCompositor(ui::Compositor* compositor) {
@@ skipping 45 matching lines @@
   } else {
     request_copy_of_output_callback_for_testing_.Run(std::move(request));
   }
 }
 
 void DelegatedFrameHost::UnlockResources() {
   DCHECK(local_surface_id_.is_valid());
   delegated_frame_evictor_->UnlockFrame();
 }
 
-void DelegatedFrameHost::OnNeedsBeginFrames(bool needs_begin_frames) {
-  needs_begin_frame_ = needs_begin_frames;
-  support_->SetNeedsBeginFrame(needs_begin_frames);
-}
-
-void DelegatedFrameHost::OnDidFinishFrame(const cc::BeginFrameAck& ack) {
-  if (ack.source_id != latest_confirmed_begin_frame_source_id_) {
-    // Source changed, we don't know our freshness anymore.
-    latest_confirmed_begin_frame_sequence_number_ =
-        cc::BeginFrameArgs::kInvalidFrameNumber;
-  }
-
-  cc::BeginFrameAck modified_ack = ack;
-  if (skipped_frames_) {
-    // If we skipped the last frame(s), we didn't incorporate the last
-    // CompositorFrame's damage, so need to wait for the next one before
-    // confirming newer sequence numbers.
-    modified_ack.has_damage = false;
-    modified_ack.latest_confirmed_sequence_number =
-        latest_confirmed_begin_frame_sequence_number_;
-  } else {
-    latest_confirmed_begin_frame_source_id_ = modified_ack.source_id;
-    latest_confirmed_begin_frame_sequence_number_ =
-        modified_ack.latest_confirmed_sequence_number;
-  }
-
-  // If there was damage, the unmodified ack was sent with the CompositorFrame.
-  if (!modified_ack.has_damage)
-    support_->BeginFrameDidNotSwap(modified_ack);
-}
-
 void DelegatedFrameHost::CreateCompositorFrameSinkSupport() {
   DCHECK(!support_);
   ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
   support_ = base::MakeUnique<cc::CompositorFrameSinkSupport>(
       this, factory->GetContextFactoryPrivate()->GetSurfaceManager(),
       frame_sink_id_, false /* is_root */,
       false /* handles_frame_sink_id_invalidation */,
       true /* needs_sync_points */);
   if (compositor_)
     compositor_->AddFrameSink(frame_sink_id_);
   if (needs_begin_frame_)
     support_->SetNeedsBeginFrame(true);
 }
 
 void DelegatedFrameHost::ResetCompositorFrameSinkSupport() {
   if (!support_)
     return;
   if (compositor_)
     compositor_->RemoveFrameSink(frame_sink_id_);
   support_.reset();
 }
 
 }  // namespace content