From ecc5d2071c160e730cc236a71b009803b0b1806e Mon Sep 17 00:00:00 2001 From: sunnyps Date: Mon, 5 Jan 2015 15:46:53 -0800 Subject: [PATCH] cc: Remove call to SetNeedsBeginFrames on losing output surface. Let the state machine be in charge of deciding when SetNeedsBeginFrames is called. This allows us to maintain the invariant that SetNeedsBeginFrames(false) is only called at the end of the frame i.e. inside the deadline or when output surface is lost. BUG=444430 Review URL: https://codereview.chromium.org/824303003 Cr-Commit-Position: refs/heads/master@{#310001} --- cc/scheduler/scheduler.cc | 28 +++++------------- cc/scheduler/scheduler.h | 2 +- cc/scheduler/scheduler_state_machine.cc | 38 ++++++++++++++++++------- cc/scheduler/scheduler_state_machine.h | 4 +++ cc/scheduler/scheduler_unittest.cc | 8 +++--- 5 files changed, 44 insertions(+), 36 deletions(-) diff --git a/cc/scheduler/scheduler.cc b/cc/scheduler/scheduler.cc index 69cd50761bee..68571278fe31 100644 --- a/cc/scheduler/scheduler.cc +++ b/cc/scheduler/scheduler.cc @@ -275,8 +275,6 @@ void Scheduler::DidLoseOutputSurface() { begin_retro_frame_args_.clear(); begin_retro_frame_task_.Cancel(); state_machine_.DidLoseOutputSurface(); - if (frame_source_->NeedsBeginFrames()) - frame_source_->SetNeedsBeginFrames(false); ProcessScheduledActions(); } @@ -313,41 +311,29 @@ void Scheduler::SetupNextBeginFrameIfNeeded() { if (!task_runner_.get()) return; - bool needs_begin_frame = state_machine_.BeginFrameNeeded(); - - bool at_end_of_deadline = - (state_machine_.begin_impl_frame_state() == - SchedulerStateMachine::BEGIN_IMPL_FRAME_STATE_INSIDE_DEADLINE); - - bool should_call_set_needs_begin_frame = - // Always request the BeginFrame immediately if it wasn't needed before. - (needs_begin_frame && !frame_source_->NeedsBeginFrames()) || - // Only stop requesting BeginFrames after a deadline. - (!needs_begin_frame && frame_source_->NeedsBeginFrames() && - at_end_of_deadline); - - if (should_call_set_needs_begin_frame) { - frame_source_->SetNeedsBeginFrames(needs_begin_frame); + if (state_machine_.ShouldSetNeedsBeginFrames( + frame_source_->NeedsBeginFrames())) { + frame_source_->SetNeedsBeginFrames(state_machine_.BeginFrameNeeded()); } - if (at_end_of_deadline) { + if (state_machine_.begin_impl_frame_state() == + SchedulerStateMachine::BEGIN_IMPL_FRAME_STATE_INSIDE_DEADLINE) { frame_source_->DidFinishFrame(begin_retro_frame_args_.size()); } PostBeginRetroFrameIfNeeded(); - SetupPollingMechanisms(needs_begin_frame); + SetupPollingMechanisms(); } // We may need to poll when we can't rely on BeginFrame to advance certain // state or to avoid deadlock. -void Scheduler::SetupPollingMechanisms(bool needs_begin_frame) { +void Scheduler::SetupPollingMechanisms() { bool needs_advance_commit_state_timer = false; // Setup PollForAnticipatedDrawTriggers if we need to monitor state but // aren't expecting any more BeginFrames. This should only be needed by // the synchronous compositor when BeginFrameNeeded is false. if (state_machine_.ShouldPollForAnticipatedDrawTriggers()) { DCHECK(!state_machine_.SupportsProactiveBeginFrame()); - DCHECK(!needs_begin_frame); if (poll_for_draw_triggers_task_.IsCancelled()) { poll_for_draw_triggers_task_.Reset(poll_for_draw_triggers_closure_); base::TimeDelta delay = begin_impl_frame_args_.IsValid() diff --git a/cc/scheduler/scheduler.h b/cc/scheduler/scheduler.h index 99a907a36d2d..577c87e3e818 100644 --- a/cc/scheduler/scheduler.h +++ b/cc/scheduler/scheduler.h @@ -223,7 +223,7 @@ class CC_EXPORT Scheduler : public BeginFrameObserverMixIn, void RescheduleBeginImplFrameDeadlineIfNeeded(); void SetupNextBeginFrameIfNeeded(); void PostBeginRetroFrameIfNeeded(); - void SetupPollingMechanisms(bool needs_begin_frame); + void SetupPollingMechanisms(); void DrawAndSwapIfPossible(); void ProcessScheduledActions(); bool CanCommitAndActivateBeforeDeadline() const; diff --git a/cc/scheduler/scheduler_state_machine.cc b/cc/scheduler/scheduler_state_machine.cc index ccf6b14e08e9..c3a4a1e272a5 100644 --- a/cc/scheduler/scheduler_state_machine.cc +++ b/cc/scheduler/scheduler_state_machine.cc @@ -696,6 +696,11 @@ bool SchedulerStateMachine::BeginFrameNeededForChildren() const { } bool SchedulerStateMachine::BeginFrameNeeded() const { + // We can't handle BeginFrames when output surface isn't initialized. + // TODO(brianderson): Support output surface creation inside a BeginFrame. + if (!HasInitializedOutputSurface()) + return false; + if (SupportsProactiveBeginFrame()) { return (BeginFrameNeededToAnimateOrDraw() || BeginFrameNeededForChildren() || @@ -712,6 +717,29 @@ bool SchedulerStateMachine::BeginFrameNeeded() const { return BeginFrameNeededToAnimateOrDraw(); } +bool SchedulerStateMachine::ShouldSetNeedsBeginFrames( + bool frame_source_needs_begin_frames) const { + bool needs_begin_frame = BeginFrameNeeded(); + + // Never call SetNeedsBeginFrames if the frame source has the right value. + if (needs_begin_frame == frame_source_needs_begin_frames) + return false; + + // Always request the BeginFrame immediately if it's needed. + if (needs_begin_frame) + return true; + + // Stop requesting BeginFrames after a deadline. + if (begin_impl_frame_state_ == BEGIN_IMPL_FRAME_STATE_INSIDE_DEADLINE) + return true; + + // Stop requesting BeginFrames immediately when output surface is lost. + if (!HasInitializedOutputSurface()) + return true; + + return false; +} + bool SchedulerStateMachine::ShouldPollForAnticipatedDrawTriggers() const { // ShouldPollForAnticipatedDrawTriggers is what we use in place of // ProactiveBeginFrameWanted when we are using the synchronous @@ -744,11 +772,6 @@ void SchedulerStateMachine::SetChildrenNeedBeginFrames( // These are the cases where we definitely (or almost definitely) have a // new frame to animate and/or draw and can draw. bool SchedulerStateMachine::BeginFrameNeededToAnimateOrDraw() const { - // The output surface is the provider of BeginImplFrames, so we are not going - // to get them even if we ask for them. - if (!HasInitializedOutputSurface()) - return false; - // The forced draw respects our normal draw scheduling, so we need to // request a BeginImplFrame for it. if (forced_redraw_state_ == FORCED_REDRAW_STATE_WAITING_FOR_DRAW) @@ -762,11 +785,6 @@ bool SchedulerStateMachine::BeginFrameNeededToAnimateOrDraw() const { // Proactively requesting the BeginImplFrame helps hide the round trip latency // of the SetNeedsBeginFrame request that has to go to the Browser. bool SchedulerStateMachine::ProactiveBeginFrameWanted() const { - // The output surface is the provider of BeginImplFrames, - // so we are not going to get them even if we ask for them. - if (!HasInitializedOutputSurface()) - return false; - // Do not be proactive when invisible. if (!visible_) return false; diff --git a/cc/scheduler/scheduler_state_machine.h b/cc/scheduler/scheduler_state_machine.h index b579998b5e1a..7509819c1d7f 100644 --- a/cc/scheduler/scheduler_state_machine.h +++ b/cc/scheduler/scheduler_state_machine.h @@ -124,6 +124,10 @@ class CC_EXPORT SchedulerStateMachine { // to make progress. bool BeginFrameNeeded() const; + // Indicates whether the scheduler should call + // SetNeedsBeginFrames(BeginFrameNeeded()) on the frame source. + bool ShouldSetNeedsBeginFrames(bool frame_source_needs_begin_frames) const; + // Indicates that we need to independently poll for new state and actions // because we can't expect a BeginImplFrame. This is mostly used to avoid // drawing repeat frames with the synchronous compositor without dropping diff --git a/cc/scheduler/scheduler_unittest.cc b/cc/scheduler/scheduler_unittest.cc index ea2b87445177..138420e4bd10 100644 --- a/cc/scheduler/scheduler_unittest.cc +++ b/cc/scheduler/scheduler_unittest.cc @@ -1827,8 +1827,8 @@ void DidLoseOutputSurfaceAfterReadyToCommit(bool impl_side_painting) { scheduler->DidLoseOutputSurface(); if (impl_side_painting) { // Sync tree should be forced to activate. - EXPECT_ACTION("SetNeedsBeginFrames(false)", client, 0, 2); - EXPECT_ACTION("ScheduledActionActivateSyncTree", client, 1, 2); + EXPECT_ACTION("ScheduledActionActivateSyncTree", client, 0, 2); + EXPECT_ACTION("SetNeedsBeginFrames(false)", client, 1, 2); } else { EXPECT_SINGLE_ACTION("SetNeedsBeginFrames(false)", client); } @@ -1919,8 +1919,8 @@ TEST(SchedulerTest, DidLoseOutputSurfaceAfterBeginRetroFramePosted) { client.Reset(); EXPECT_FALSE(scheduler->IsBeginRetroFrameArgsEmpty()); scheduler->DidLoseOutputSurface(); - EXPECT_ACTION("SetNeedsBeginFrames(false)", client, 0, 2); - EXPECT_ACTION("ScheduledActionBeginOutputSurfaceCreation", client, 1, 2); + EXPECT_ACTION("ScheduledActionBeginOutputSurfaceCreation", client, 0, 2); + EXPECT_ACTION("SetNeedsBeginFrames(false)", client, 1, 2); EXPECT_TRUE(scheduler->IsBeginRetroFrameArgsEmpty()); // Posted BeginRetroFrame is aborted.