Clean up remoting::VideoScheduler implementation:

- Reduce thread-switching in VideoScheduler::Stop(). 
- Simplify VideoScheduler API and clean up implementation. 
- Tidy up ClientSessionTest test base. 
- Rename SessionConfig::GetDefault() to ForTest(). 

This was originally reviewed as crrev.com/11229023.

BUG=104543


Review URL: https://chromiumcodereview.appspot.com/11273051

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@164409 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 225 insertions, 363 deletions

diff --git a/remoting/host/chromoting_host_unittest.cc b/remoting/host/chromoting_host_unittest.cc
index 36164913..cf7aa24 100644
--- a/remoting/host/chromoting_host_unittest.cc
+++ b/remoting/host/chromoting_host_unittest.cc
@@ -169,13 +169,13 @@ class ChromotingHostTest : public testing::Test {
     session2_ = new MockSession();
     session_unowned1_.reset(new MockSession());
     session_unowned2_.reset(new MockSession());
-    session_config1_ = SessionConfig::GetDefault();
+    session_config1_ = SessionConfig::ForTest();
     session_jid1_ = "user@domain/rest-of-jid";
-    session_config2_ = SessionConfig::GetDefault();
+    session_config2_ = SessionConfig::ForTest();
     session_jid2_ = "user2@domain/rest-of-jid";
-    session_unowned_config1_ = SessionConfig::GetDefault();
+    session_unowned_config1_ = SessionConfig::ForTest();
     session_unowned_jid1_ = "user3@doman/rest-of-jid";
-    session_unowned_config2_ = SessionConfig::GetDefault();
+    session_unowned_config2_ = SessionConfig::ForTest();
     session_unowned_jid2_ = "user4@doman/rest-of-jid";
 
     EXPECT_CALL(*session1_, jid())
diff --git a/remoting/host/client_session.cc b/remoting/host/client_session.cc
index df72e7f..9575cc4 100644
--- a/remoting/host/client_session.cc
+++ b/remoting/host/client_session.cc
@@ -130,14 +130,17 @@ void ClientSession::OnConnectionChannelsConnected(
   scoped_ptr<VideoEncoder> video_encoder =
       CreateVideoEncoder(connection_->session()->config());
 
-  // Create a VideoScheduler to capture frames and feed them to the encoder.
+  // Create a VideoScheduler to pump frames from the capturer to the client.
   video_scheduler_ = new VideoScheduler(capture_task_runner_,
                                         encode_task_runner_,
                                         network_task_runner_,
                                         desktop_environment_->video_capturer(),
-                                        video_encoder.Pass());
+                                        video_encoder.Pass(),
+                                        connection_->client_stub(),
+                                        connection_->video_stub());
   ++active_recorders_;
 
+  // Create an AudioScheduler if audio is enabled, to pump audio samples.
   if (connection_->session()->config().is_audio_enabled()) {
     scoped_ptr<AudioEncoder> audio_encoder =
         CreateAudioEncoder(connection_->session()->config());
@@ -150,11 +153,10 @@ void ClientSession::OnConnectionChannelsConnected(
     ++active_recorders_;
   }
 
-  // Start the session.
-  video_scheduler_->AddConnection(connection_.get());
-  video_scheduler_->Start();
+  // Let the desktop environment notify us of local clipboard changes.
   desktop_environment_->Start(CreateClipboardProxy());
 
+  // Notify the event handler that all our channels are now connected.
   event_handler_->OnSessionChannelsConnected(this);
 }
 
@@ -221,7 +223,6 @@ void ClientSession::Stop(const base::Closure& done_task) {
   }
 
   if (video_scheduler_.get()) {
-    video_scheduler_->RemoveConnection(connection_.get());
     video_scheduler_->Stop(base::Bind(&ClientSession::OnRecorderStopped, this));
     video_scheduler_ = NULL;
   }
diff --git a/remoting/host/client_session_unittest.cc b/remoting/host/client_session_unittest.cc
index e9f1f4b..bc7c9c3 100644
--- a/remoting/host/client_session_unittest.cc
+++ b/remoting/host/client_session_unittest.cc
@@ -14,12 +14,12 @@
 
 namespace remoting {
 
-using protocol::MockClipboardStub;
 using protocol::MockConnectionToClient;
-using protocol::MockConnectionToClientEventHandler;
+using protocol::MockClientStub;
 using protocol::MockHostStub;
 using protocol::MockInputStub;
 using protocol::MockSession;
+using protocol::MockVideoStub;
 using protocol::SessionConfig;
 
 using testing::_;
@@ -40,19 +40,6 @@ class ClientSessionTest : public testing::Test {
         base::Bind(&ClientSessionTest::QuitMainMessageLoop,
                    base::Unretained(this)));
 
-    EXPECT_CALL(context_, ui_task_runner())
-        .Times(AnyNumber())
-        .WillRepeatedly(Return(ui_task_runner_.get()));
-    EXPECT_CALL(context_, capture_task_runner())
-        .Times(AnyNumber())
-        .WillRepeatedly(Return(ui_task_runner_.get()));
-    EXPECT_CALL(context_, encode_task_runner())
-        .Times(AnyNumber())
-        .WillRepeatedly(Return(ui_task_runner_.get()));
-    EXPECT_CALL(context_, network_task_runner())
-        .Times(AnyNumber())
-        .WillRepeatedly(Return(ui_task_runner_.get()));
-
     client_jid_ = "user@domain/rest-of-jid";
 
     desktop_environment_factory_.reset(new MockDesktopEnvironmentFactory());
@@ -64,23 +51,31 @@ class ClientSessionTest : public testing::Test {
     // Set up a large default screen size that won't affect most tests.
     screen_size_.set(1000, 1000);
 
-    session_config_ = SessionConfig::GetDefault();
+    session_config_ = SessionConfig::ForTest();
 
+    // Mock protocol::Session APIs called directly by ClientSession.
     protocol::MockSession* session = new MockSession();
     EXPECT_CALL(*session, config()).WillRepeatedly(ReturnRef(session_config_));
     EXPECT_CALL(*session, jid()).WillRepeatedly(ReturnRef(client_jid_));
     EXPECT_CALL(*session, SetEventHandler(_));
-    EXPECT_CALL(*session, Close());
-    scoped_ptr<protocol::ConnectionToClient> connection(
-        new protocol::ConnectionToClient(session));
+
+    // Mock protocol::ConnectionToClient APIs called directly by ClientSession.
+    // HostStub is not touched by ClientSession, so we can safely pass NULL.
+    scoped_ptr<MockConnectionToClient> connection(
+        new MockConnectionToClient(session, NULL));
+    EXPECT_CALL(*connection, session()).WillRepeatedly(Return(session));
+    EXPECT_CALL(*connection, client_stub())
+        .WillRepeatedly(Return(&client_stub_));
+    EXPECT_CALL(*connection, video_stub()).WillRepeatedly(Return(&video_stub_));
+    EXPECT_CALL(*connection, Disconnect());
     connection_ = connection.get();
 
     client_session_ = new ClientSession(
         &session_event_handler_,
-        context_.capture_task_runner(),
-        context_.encode_task_runner(),
-        context_.network_task_runner(),
-        connection.Pass(),
+        ui_task_runner_, // Capture thread.
+        ui_task_runner_, // Encode thread.
+        ui_task_runner_, // Network thread.
+        connection.PassAs<protocol::ConnectionToClient>(),
         desktop_environment_factory_.get(),
         base::TimeDelta());
   }
@@ -131,19 +126,34 @@ class ClientSessionTest : public testing::Test {
     client_session_ = NULL;
   }
 
+  // Message loop passed to |client_session_| to perform all functions on.
   MessageLoop message_loop_;
   scoped_refptr<AutoThreadTaskRunner> ui_task_runner_;
-  MockChromotingHostContext context_;
+
+  // ClientSession instance under test.
+  scoped_refptr<ClientSession> client_session_;
+
+  // ClientSession::EventHandler mock for use in tests.
+  MockClientSessionEventHandler session_event_handler_;
+
+  // Screen size that the fake VideoFrameCapturer should report.
   SkISize screen_size_;
+
+  // Storage for values to be returned by the protocol::Session mock.
+  SessionConfig session_config_;
   std::string client_jid_;
-  MockHostStub host_stub_;
+
+  // Stubs returned to |client_session_| components by |connection_|.
+  MockClientStub client_stub_;
+  MockVideoStub video_stub_;
+
+  // DesktopEnvironment owns |event_executor_|, but input injection tests need
+  // to express expectations on it.
   MockEventExecutor* event_executor_;
-  MockClientSessionEventHandler session_event_handler_;
-  scoped_refptr<ClientSession> client_session_;
-  SessionConfig session_config_;
 
   // ClientSession owns |connection_| but tests need it to inject fake events.
-  protocol::ConnectionToClient* connection_;
+  MockConnectionToClient* connection_;
+
   scoped_ptr<MockDesktopEnvironmentFactory> desktop_environment_factory_;
 };
 
diff --git a/remoting/host/video_frame_capturer.h b/remoting/host/video_frame_capturer.h
index 57f3a86..e2db2b9 100644
--- a/remoting/host/video_frame_capturer.h
+++ b/remoting/host/video_frame_capturer.h
@@ -18,16 +18,15 @@ class CursorShapeInfo;
 
 class CaptureData;
 
-// A class to perform the task of capturing the image of a window.
-// The capture action is asynchronous to allow maximum throughput.
+// Class used to capture video frames asynchronously.
 //
-// The full capture process is as follows:
+// The full capture sequence is as follows:
 //
 // (1) Start
 //     This is when pre-capture steps are executed, such as flagging the
 //     display to prevent it from sleeping during a session.
 //
-// (2) InvalidateRects
+// (2) InvalidateRegion
 //     This is an optional step where regions of the screen are marked as
 //     invalid. Some platforms (Windows, for now) won't use this and will
 //     instead calculate the diff-regions later (in step (2). Other
@@ -35,7 +34,7 @@ class CaptureData;
 //     screen. Some limited rect-merging (e.g., to eliminate exact
 //     duplicates) may be done here.
 //
-// (3) CaptureInvalidRects
+// (3) CaptureInvalidRegion
 //     This is where the bits for the invalid rects are packaged up and sent
 //     to the encoder.
 //     A screen capture is performed if needed. For example, Windows requires
diff --git a/remoting/host/video_scheduler.cc b/remoting/host/video_scheduler.cc
index 2031ae1..e4c349f 100644
--- a/remoting/host/video_scheduler.cc
+++ b/remoting/host/video_scheduler.cc
@@ -20,84 +20,55 @@
 #include "remoting/proto/internal.pb.h"
 #include "remoting/proto/video.pb.h"
 #include "remoting/protocol/client_stub.h"
-#include "remoting/protocol/connection_to_client.h"
 #include "remoting/protocol/message_decoder.h"
+#include "remoting/protocol/video_stub.h"
 #include "remoting/protocol/util.h"
 
-using remoting::protocol::ConnectionToClient;
-
 namespace remoting {
 
-// Maximum number of frames that can be processed similtaneously.
+// Maximum number of frames that can be processed simultaneously.
 // TODO(hclam): Move this value to CaptureScheduler.
-static const int kMaxRecordings = 2;
+static const int kMaxPendingCaptures = 2;
 
 VideoScheduler::VideoScheduler(
     scoped_refptr<base::SingleThreadTaskRunner> capture_task_runner,
     scoped_refptr<base::SingleThreadTaskRunner> encode_task_runner,
     scoped_refptr<base::SingleThreadTaskRunner> network_task_runner,
     VideoFrameCapturer* capturer,
-    scoped_ptr<VideoEncoder> encoder)
+    scoped_ptr<VideoEncoder> encoder,
+    protocol::ClientStub* client_stub,
+    protocol::VideoStub* video_stub)
     : capture_task_runner_(capture_task_runner),
       encode_task_runner_(encode_task_runner),
       network_task_runner_(network_task_runner),
       capturer_(capturer),
       encoder_(encoder.Pass()),
-      network_stopped_(false),
-      encoder_stopped_(false),
-      max_recordings_(kMaxRecordings),
-      recordings_(0),
-      frame_skipped_(false),
+      cursor_stub_(client_stub),
+      video_stub_(video_stub),
+      pending_captures_(0),
+      did_skip_frame_(false),
       sequence_number_(0) {
-  DCHECK(capture_task_runner_);
-  DCHECK(encode_task_runner_);
-  DCHECK(network_task_runner_);
-}
-
-// Public methods --------------------------------------------------------------
-
-void VideoScheduler::Start() {
-  capture_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&VideoScheduler::DoStart, this));
-}
-
-void VideoScheduler::Stop(const base::Closure& done_task) {
-  if (!capture_task_runner_->BelongsToCurrentThread()) {
-    capture_task_runner_->PostTask(FROM_HERE, base::Bind(
-        &VideoScheduler::Stop, this, done_task));
-    return;
-  }
-
-  DCHECK(!done_task.is_null());
-
-  capturer()->Stop();
-  capture_timer_.reset();
-
-  network_task_runner_->PostTask(FROM_HERE, base::Bind(
-      &VideoScheduler::DoStopOnNetworkThread, this, done_task));
-}
-
-void VideoScheduler::AddConnection(ConnectionToClient* connection) {
   DCHECK(network_task_runner_->BelongsToCurrentThread());
-  connections_.push_back(connection);
+  DCHECK(capturer_);
+  DCHECK(cursor_stub_);
+  DCHECK(video_stub_);
 
   capture_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&VideoScheduler::DoInvalidateFullScreen, this));
+      FROM_HERE, base::Bind(&VideoScheduler::StartOnCaptureThread, this));
 }
 
-void VideoScheduler::RemoveConnection(ConnectionToClient* connection) {
+// Public methods --------------------------------------------------------------
+
+void VideoScheduler::Stop(const base::Closure& done_task) {
   DCHECK(network_task_runner_->BelongsToCurrentThread());
+  DCHECK(!done_task.is_null());
 
-  ConnectionToClientList::iterator it =
-      std::find(connections_.begin(), connections_.end(), connection);
-  if (it != connections_.end()) {
-    connections_.erase(it);
-  }
-}
+  // Clear stubs to prevent further updates reaching the client.
+  cursor_stub_ = NULL;
+  video_stub_ = NULL;
 
-void VideoScheduler::RemoveAllConnections() {
-  DCHECK(network_task_runner_->BelongsToCurrentThread());
-  connections_.clear();
+  capture_task_runner_->PostTask(FROM_HERE,
+      base::Bind(&VideoScheduler::StopOnCaptureThread, this, done_task));
 }
 
 void VideoScheduler::UpdateSequenceNumber(int64 sequence_number) {
@@ -117,78 +88,72 @@ void VideoScheduler::UpdateSequenceNumber(int64 sequence_number) {
 VideoScheduler::~VideoScheduler() {
 }
 
-VideoFrameCapturer* VideoScheduler::capturer() {
-  DCHECK(capture_task_runner_->BelongsToCurrentThread());
-  DCHECK(capturer_);
-  return capturer_;
-}
-
-VideoEncoder* VideoScheduler::encoder() {
-  DCHECK(encode_task_runner_->BelongsToCurrentThread());
-  DCHECK(encoder_.get());
-  return encoder_.get();
-}
-
-bool VideoScheduler::is_recording() {
-  DCHECK(capture_task_runner_->BelongsToCurrentThread());
-  return capture_timer_.get() != NULL;
-}
-
 // Capturer thread -------------------------------------------------------------
 
-void VideoScheduler::DoStart() {
+void VideoScheduler::StartOnCaptureThread() {
   DCHECK(capture_task_runner_->BelongsToCurrentThread());
 
-  if (is_recording()) {
-    NOTREACHED() << "Record session already started.";
-    return;
-  }
-
-  capturer()->Start(
+  // Start the capturer and let it notify us of cursor shape changes.
+  capturer_->Start(
       base::Bind(&VideoScheduler::CursorShapeChangedCallback, this));
 
   capture_timer_.reset(new base::OneShotTimer<VideoScheduler>());
 
   // Capture first frame immedately.
-  DoCapture();
+  CaptureNextFrame();
+}
+
+void VideoScheduler::StopOnCaptureThread(const base::Closure& done_task) {
+  DCHECK(capture_task_runner_->BelongsToCurrentThread());
+
+  // Stop |capturer_| and clear it to prevent pending tasks from using it.
+  capturer_->Stop();
+  capturer_ = NULL;
+
+  // |capture_timer_| must be destroyed on the thread on which it is used.
+  capture_timer_.reset();
+
+  // Activity on the encode thread will stop implicitly as a result of
+  // captures having stopped.
+  network_task_runner_->PostTask(FROM_HERE, done_task);
 }
 
-void VideoScheduler::StartCaptureTimer() {
+void VideoScheduler::ScheduleNextCapture() {
   DCHECK(capture_task_runner_->BelongsToCurrentThread());
 
   capture_timer_->Start(FROM_HERE,
                         scheduler_.NextCaptureDelay(),
                         this,
-                        &VideoScheduler::DoCapture);
+                        &VideoScheduler::CaptureNextFrame);
 }
 
-void VideoScheduler::DoCapture() {
+void VideoScheduler::CaptureNextFrame() {
   DCHECK(capture_task_runner_->BelongsToCurrentThread());
+
+  // If |capturer_| is NULL then we're in the process of stopping.
+  if (!capturer_)
+    return;
+
   // Make sure we have at most two oustanding recordings. We can simply return
   // if we can't make a capture now, the next capture will be started by the
   // end of an encode operation.
-  if (recordings_ >= max_recordings_ || !is_recording()) {
-    frame_skipped_ = true;
+  if (pending_captures_ >= kMaxPendingCaptures) {
+    did_skip_frame_ = true;
     return;
   }
 
-  if (frame_skipped_)
-    frame_skipped_ = false;
+  did_skip_frame_ = false;
 
   // At this point we are going to perform one capture so save the current time.
-  ++recordings_;
-  DCHECK_LE(recordings_, max_recordings_);
+  pending_captures_++;
+  DCHECK_LE(pending_captures_, kMaxPendingCaptures);
 
   // Before doing a capture schedule for the next one.
-  capture_timer_->Stop();
-  capture_timer_->Start(FROM_HERE,
-                        scheduler_.NextCaptureDelay(),
-                        this,
-                        &VideoScheduler::DoCapture);
+  ScheduleNextCapture();
 
   // And finally perform one capture.
   capture_start_time_ = base::Time::Now();
-  capturer()->CaptureInvalidRegion(
+  capturer_->CaptureInvalidRegion(
       base::Bind(&VideoScheduler::CaptureDoneCallback, this));
 }
 
@@ -196,9 +161,6 @@ void VideoScheduler::CaptureDoneCallback(
     scoped_refptr<CaptureData> capture_data) {
   DCHECK(capture_task_runner_->BelongsToCurrentThread());
 
-  if (!is_recording())
-    return;
-
   if (capture_data) {
     base::TimeDelta capture_time = base::Time::Now() - capture_start_time_;
     int capture_time_ms =
@@ -215,147 +177,92 @@ void VideoScheduler::CaptureDoneCallback(
   }
 
   encode_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&VideoScheduler::DoEncode, this, capture_data));
+      FROM_HERE, base::Bind(&VideoScheduler::EncodeFrame, this, capture_data));
 }
 
 void VideoScheduler::CursorShapeChangedCallback(
     scoped_ptr<protocol::CursorShapeInfo> cursor_shape) {
   DCHECK(capture_task_runner_->BelongsToCurrentThread());
 
-  if (!is_recording())
-    return;
-
   network_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&VideoScheduler::DoSendCursorShape, this,
-                            base::Passed(cursor_shape.Pass())));
+      FROM_HERE, base::Bind(&VideoScheduler::SendCursorShape, this,
+                            base::Passed(&cursor_shape)));
 }
 
-void VideoScheduler::DoFinishOneRecording() {
+void VideoScheduler::FrameCaptureCompleted() {
   DCHECK(capture_task_runner_->BelongsToCurrentThread());
 
-  if (!is_recording())
-    return;
-
-  // Decrement the number of recording in process since we have completed
-  // one cycle.
-  --recordings_;
-  DCHECK_GE(recordings_, 0);
+  // Decrement the pending capture count.
+  pending_captures_--;
+  DCHECK_GE(pending_captures_, 0);
 
-  // Try to do a capture again only if |frame_skipped_| is set to true by
-  // capture timer.
-  if (frame_skipped_)
-    DoCapture();
-}
-
-void VideoScheduler::DoInvalidateFullScreen() {
-  DCHECK(capture_task_runner_->BelongsToCurrentThread());
-
-  SkRegion region;
-  region.op(SkIRect::MakeSize(capturer_->size_most_recent()),
-            SkRegion::kUnion_Op);
-  capturer_->InvalidateRegion(region);
+  // If we've skipped a frame capture because too we had too many captures
+  // pending then schedule one now.
+  if (did_skip_frame_)
+    CaptureNextFrame();
 }
 
 // Network thread --------------------------------------------------------------
 
-void VideoScheduler::DoSendVideoPacket(scoped_ptr<VideoPacket> packet) {
+void VideoScheduler::SendVideoPacket(scoped_ptr<VideoPacket> packet) {
   DCHECK(network_task_runner_->BelongsToCurrentThread());
 
-  if (network_stopped_ || connections_.empty())
+  if (!video_stub_)
     return;
 
   base::Closure callback;
   if ((packet->flags() & VideoPacket::LAST_PARTITION) != 0)
     callback = base::Bind(&VideoScheduler::VideoFrameSentCallback, this);
 
-  // TODO(sergeyu): Currently we send the data only to the first
-  // connection. Send it to all connections if necessary.
-  connections_.front()->video_stub()->ProcessVideoPacket(
-      packet.Pass(), callback);
+  video_stub_->ProcessVideoPacket(packet.Pass(), callback);
 }
 
 void VideoScheduler::VideoFrameSentCallback() {
   DCHECK(network_task_runner_->BelongsToCurrentThread());
 
-  if (network_stopped_)
+  if (!video_stub_)
     return;
 
   capture_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&VideoScheduler::DoFinishOneRecording, this));
-}
-
-void VideoScheduler::DoStopOnNetworkThread(const base::Closure& done_task) {
-  DCHECK(network_task_runner_->BelongsToCurrentThread());
-
-  // There could be tasks on the network thread when this method is being
-  // executed. By setting the flag we'll not post anymore tasks from network
-  // thread.
-  //
-  // After that a task is posted on encode thread to continue the stop
-  // sequence.
-  network_stopped_ = true;
-
-  encode_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&VideoScheduler::DoStopOnEncodeThread,
-                            this, done_task));
+      FROM_HERE, base::Bind(&VideoScheduler::FrameCaptureCompleted, this));
 }
 
-void VideoScheduler::DoSendCursorShape(
+void VideoScheduler::SendCursorShape(
     scoped_ptr<protocol::CursorShapeInfo> cursor_shape) {
   DCHECK(network_task_runner_->BelongsToCurrentThread());
 
-  if (network_stopped_ || connections_.empty())
+  if (!cursor_stub_)
     return;
 
-  // TODO(sergeyu): Currently we send the data only to the first
-  // connection. Send it to all connections if necessary.
-  connections_.front()->client_stub()->SetCursorShape(*cursor_shape);
+  cursor_stub_->SetCursorShape(*cursor_shape);
 }
 
 // Encoder thread --------------------------------------------------------------
 
-void VideoScheduler::DoEncode(
+void VideoScheduler::EncodeFrame(
     scoped_refptr<CaptureData> capture_data) {
   DCHECK(encode_task_runner_->BelongsToCurrentThread());
 
-  if (encoder_stopped_)
-    return;
-
-  // Early out if there's nothing to encode.
+  // If there is nothing to encode then send an empty keep-alive packet.
   if (!capture_data || capture_data->dirty_region().isEmpty()) {
-    // Send an empty video packet to keep network active.
     scoped_ptr<VideoPacket> packet(new VideoPacket());
     packet->set_flags(VideoPacket::LAST_PARTITION);
     network_task_runner_->PostTask(
-        FROM_HERE, base::Bind(&VideoScheduler::DoSendVideoPacket,
-                              this, base::Passed(packet.Pass())));
+        FROM_HERE, base::Bind(&VideoScheduler::SendVideoPacket, this,
+                              base::Passed(&packet)));
     return;
   }
 
   encode_start_time_ = base::Time::Now();
-  encoder()->Encode(
+  encoder_->Encode(
       capture_data, false,
       base::Bind(&VideoScheduler::EncodedDataAvailableCallback, this));
 }
 
-void VideoScheduler::DoStopOnEncodeThread(const base::Closure& done_task) {
-  DCHECK(encode_task_runner_->BelongsToCurrentThread());
-
-  encoder_stopped_ = true;
-
-  // When this method is being executed there are no more tasks on encode thread
-  // for this object. We can then post a task to capture thread to finish the
-  // stop sequence.
-  capture_task_runner_->PostTask(FROM_HERE, done_task);
-}
-
 void VideoScheduler::EncodedDataAvailableCallback(
     scoped_ptr<VideoPacket> packet) {
   DCHECK(encode_task_runner_->BelongsToCurrentThread());
 
-  if (encoder_stopped_)
-    return;
-
   bool last = (packet->flags() & VideoPacket::LAST_PACKET) != 0;
   if (last) {
     base::TimeDelta encode_time = base::Time::Now() - encode_start_time_;
@@ -366,8 +273,8 @@ void VideoScheduler::EncodedDataAvailableCallback(
   }
 
   network_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&VideoScheduler::DoSendVideoPacket, this,
-                            base::Passed(packet.Pass())));
+      FROM_HERE, base::Bind(&VideoScheduler::SendVideoPacket, this,
+                            base::Passed(&packet)));
 }
 
 }  // namespace remoting
diff --git a/remoting/host/video_scheduler.h b/remoting/host/video_scheduler.h
index 5296253..b89c881 100644
--- a/remoting/host/video_scheduler.h
+++ b/remoting/host/video_scheduler.h
@@ -27,20 +27,20 @@ class CaptureData;
 class VideoFrameCapturer;
 
 namespace protocol {
-class ConnectionToClient;
+class ClientStub;
 class CursorShapeInfo;
+class VideoStub;
 }  // namespace protocol
 
-// A class for controlling and coordinate VideoFrameCapturer, Encoder
-// and NetworkChannel in a record session.
+// Class responsible for scheduling frame captures from a VideoFrameCapturer,
+// delivering them to a VideoEncoder to encode, and finally passing the encoded
+// video packets to the specified VideoStub to send on the network.
 //
 // THREADING
 //
-// This class works on three threads, namely capture, encode and network
-// thread. The main function of this class is to coordinate and schedule
-// capture, encode and transmission of data on different threads.
-//
-// The following is an example of timeline for operations scheduled.
+// This class is supplied TaskRunners to use for capture, encode and network
+// operations.  Capture, encode and network transmission tasks are interleaved
+// as illustrated below:
 //
 // |       CAPTURE       ENCODE     NETWORK
 // |    .............
@@ -63,133 +63,108 @@ class CursorShapeInfo;
 // | Time
 // v
 //
-// VideoScheduler has the following responsibilities:
-// 1. Make sure capture and encode occurs no more frequently than |rate|.
-// 2. Make sure there is at most one outstanding capture not being encoded.
-// 3. Distribute tasks on three threads on a timely fashion to minimize latency.
-//
-// This class has the following state variables:
-// |capture_timer_| - If this is set to NULL there should be no activity on
-//                    the capture thread by this object.
-// |network_stopped_| - This state is to prevent activity on the network thread
-//                      if set to false.
+// VideoScheduler would ideally schedule captures so as to saturate the slowest
+// of the capture, encode and network processes.  However, it also needs to
+// rate-limit captures to avoid overloading the host system, either by consuming
+// too much CPU, or hogging the host's graphics subsystem.
+
 class VideoScheduler : public base::RefCountedThreadSafe<VideoScheduler> {
  public:
-  // Construct a VideoScheduler. Message loops and threads are provided.
-  // This object does not own capturer but owns encoder.
+  // Creates a VideoScheduler running capture, encode and network tasks on the
+  // supplied TaskRunners.  Video and cursor shape updates will be pumped to
+  // |video_stub| and |client_stub|, which must remain valid until Stop() is
+  // called. |capturer| is used to capture frames and must remain valid until
+  // the |done_task| supplied to Stop() is executed.
   VideoScheduler(
       scoped_refptr<base::SingleThreadTaskRunner> capture_task_runner,
       scoped_refptr<base::SingleThreadTaskRunner> encode_task_runner,
       scoped_refptr<base::SingleThreadTaskRunner> network_task_runner,
       VideoFrameCapturer* capturer,
-      scoped_ptr<VideoEncoder> encoder);
-
-  // Start recording.
-  void Start();
+      scoped_ptr<VideoEncoder> encoder,
+      protocol::ClientStub* client_stub,
+      protocol::VideoStub* video_stub);
 
-  // Stop the recording session. |done_task| is executed when recording is fully
-  // stopped. This object cannot be used again after |task| is executed.
+  // Stop scheduling frame captures.  |done_task| is executed on the network
+  // thread when capturing has stopped.  This object cannot be re-used once
+  // it has been stopped.
   void Stop(const base::Closure& done_task);
 
-  // Add a connection to this recording session.
-  void AddConnection(protocol::ConnectionToClient* connection);
-
-  // Remove a connection from receiving screen updates.
-  void RemoveConnection(protocol::ConnectionToClient* connection);
-
-  // Remove all connections.
-  void RemoveAllConnections();
-
-  // Update the sequence number for tracing performance.
+  // Updates the sequence number embedded in VideoPackets.
+  // Sequence numbers are used for performance measurements.
   void UpdateSequenceNumber(int64 sequence_number);
 
  private:
   friend class base::RefCountedThreadSafe<VideoScheduler>;
   virtual ~VideoScheduler();
 
-  // Getters for capturer and encoder.
-  VideoFrameCapturer* capturer();
-  VideoEncoder* encoder();
+  // Capturer thread ----------------------------------------------------------
 
-  bool is_recording();
+  // Starts the capturer on the capture thread.
+  void StartOnCaptureThread();
 
-  // Capturer thread ----------------------------------------------------------
+  // Stops scheduling frame captures on the capture thread, and posts
+  // |done_task| to the network thread when done.
+  void StopOnCaptureThread(const base::Closure& done_task);
 
-  void DoStart();
-  void DoSetMaxRate(double max_rate);
+  // Schedules the next call to CaptureNextFrame.
+  void ScheduleNextCapture();
 
-  // Hepler method to schedule next capture using the current rate.
-  void StartCaptureTimer();
+  // Starts the next frame capture, unless there are already too many pending.
+  void CaptureNextFrame();
 
-  void DoCapture();
+  // Called when a frame capture completes.
   void CaptureDoneCallback(scoped_refptr<CaptureData> capture_data);
+
+  // Called when the cursor shape changes.
   void CursorShapeChangedCallback(
       scoped_ptr<protocol::CursorShapeInfo> cursor_data);
-  void DoFinishOneRecording();
-  void DoInvalidateFullScreen();
-
-  // Network thread -----------------------------------------------------------
 
-  void DoSendVideoPacket(scoped_ptr<VideoPacket> packet);
+  // Called when a frame capture has been encoded & sent to the client.
+  void FrameCaptureCompleted();
 
-  void DoSendInit(scoped_refptr<protocol::ConnectionToClient> connection,
-                  int width, int height);
+  // Network thread -----------------------------------------------------------
 
-  // Signal network thread to cease activities.
-  void DoStopOnNetworkThread(const base::Closure& done_task);
+  // Send |packet| to the client, unless we are in the process of stopping.
+  void SendVideoPacket(scoped_ptr<VideoPacket> packet);
 
-  // Callback for VideoStub::ProcessVideoPacket() that is used for
-  // each last packet in a frame.
+  // Callback passed to |video_stub_| for the last packet in each frame, to
+  // rate-limit frame captures to network throughput.
   void VideoFrameSentCallback();
 
   // Send updated cursor shape to client.
-  void DoSendCursorShape(scoped_ptr<protocol::CursorShapeInfo> cursor_shape);
+  void SendCursorShape(scoped_ptr<protocol::CursorShapeInfo> cursor_shape);
 
   // Encoder thread -----------------------------------------------------------
 
-  void DoEncode(scoped_refptr<CaptureData> capture_data);
-
-  // Perform stop operations on encode thread.
-  void DoStopOnEncodeThread(const base::Closure& done_task);
+  // Encode a frame, passing generated VideoPackets to SendVideoPacket().
+  void EncodeFrame(scoped_refptr<CaptureData> capture_data);
 
   void EncodedDataAvailableCallback(scoped_ptr<VideoPacket> packet);
-  void SendVideoPacket(VideoPacket* packet);
 
   // Task runners used by this class.
   scoped_refptr<base::SingleThreadTaskRunner> capture_task_runner_;
   scoped_refptr<base::SingleThreadTaskRunner> encode_task_runner_;
   scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_;
 
-  // Reference to the capturer. This member is always accessed on the capture
-  // thread.
+  // Used to capture frames. Always accessed on the capture thread.
   VideoFrameCapturer* capturer_;
 
-  // Reference to the encoder. This member is always accessed on the encode
-  // thread.
+  // Used to encode captured frames. Always accessed on the encode thread.
   scoped_ptr<VideoEncoder> encoder_;
 
-  // A list of clients connected to this hosts.
-  // This member is always accessed on the network thread.
-  typedef std::vector<protocol::ConnectionToClient*> ConnectionToClientList;
-  ConnectionToClientList connections_;
+  // Interfaces through which video frames and cursor shapes are passed to the
+  // client. These members are always accessed on the network thread.
+  protocol::ClientStub* cursor_stub_;
+  protocol::VideoStub* video_stub_;
 
-  // Timer that calls DoCapture. Set to NULL when not recording.
+  // Timer used to schedule CaptureNextFrame().
   scoped_ptr<base::OneShotTimer<VideoScheduler> > capture_timer_;
 
-  // Per-thread flags that are set when the VideoScheduler is
-  // stopped. They must be used on the corresponding threads only.
-  bool network_stopped_;
-  bool encoder_stopped_;
-
-  // Maximum simultaneous recordings allowed.
-  int max_recordings_;
-
   // Count the number of recordings (i.e. capture or encode) happening.
-  int recordings_;
+  int pending_captures_;
 
-  // Set to true if we've skipped last capture because there are too
-  // many pending frames.
-  int frame_skipped_;
+  // True if the previous scheduled capture was skipped.
+  int did_skip_frame_;
 
   // Time when capture is started.
   base::Time capture_start_time_;
diff --git a/remoting/host/video_scheduler_unittest.cc b/remoting/host/video_scheduler_unittest.cc
index abaea18..54b572c 100644
--- a/remoting/host/video_scheduler_unittest.cc
+++ b/remoting/host/video_scheduler_unittest.cc
@@ -6,6 +6,7 @@
 
 #include "base/bind.h"
 #include "base/message_loop.h"
+#include "base/run_loop.h"
 #include "remoting/base/capture_data.h"
 #include "remoting/codec/video_encoder.h"
 #include "remoting/host/host_mock_objects.h"
@@ -14,10 +15,7 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-using ::remoting::protocol::MockConnectionToClient;
-using ::remoting::protocol::MockConnectionToClientEventHandler;
-using ::remoting::protocol::MockHostStub;
-using ::remoting::protocol::MockSession;
+using ::remoting::protocol::MockClientStub;
 using ::remoting::protocol::MockVideoStub;
 
 using ::testing::_;
@@ -52,13 +50,8 @@ ACTION(FinishSend) {
   arg1.Run();
 }
 
-// Helper method to quit the main message loop.
-void QuitMessageLoop(MessageLoop* message_loop) {
-  message_loop->PostTask(FROM_HERE, MessageLoop::QuitClosure());
-}
-
 ACTION_P2(StopVideoScheduler, scheduler, task) {
-  scheduler->Stop(task);
+  scheduler->get()->Stop(task);
 }
 
 }  // namespace
@@ -93,40 +86,29 @@ class VideoSchedulerTest : public testing::Test {
   }
 
   virtual void SetUp() OVERRIDE {
-    // VideoFrameCapturer and VideoEncoder are owned by VideoScheduler.
     encoder_ = new MockVideoEncoder();
-
-    session_ = new MockSession();
-    EXPECT_CALL(*session_, SetEventHandler(_));
-    EXPECT_CALL(*session_, Close())
-        .Times(AnyNumber());
-    connection_.reset(new MockConnectionToClient(session_, &host_stub_));
-    connection_->SetEventHandler(&handler_);
-
-    scheduler_ = new VideoScheduler(
-        message_loop_.message_loop_proxy(), message_loop_.message_loop_proxy(),
-        message_loop_.message_loop_proxy(), &capturer_,
-        scoped_ptr<remoting::VideoEncoder>(encoder_));
   }
 
-  virtual void TearDown() OVERRIDE {
-    connection_.reset();
-    // Run message loop before destroying because protocol::Session is
-    // destroyed asynchronously.
-    message_loop_.RunAllPending();
+  void StartVideoScheduler() {
+    scheduler_ = new VideoScheduler(
+        message_loop_.message_loop_proxy(),
+        message_loop_.message_loop_proxy(),
+        message_loop_.message_loop_proxy(),
+        &capturer_,
+        scoped_ptr<VideoEncoder>(encoder_),
+        &client_stub_,
+        &video_stub_);
   }
 
  protected:
   MessageLoop message_loop_;
   scoped_refptr<VideoScheduler> scheduler_;
 
-  MockConnectionToClientEventHandler handler_;
-  MockHostStub host_stub_;
-  MockSession* session_;  // Owned by |connection_|.
-  scoped_ptr<MockConnectionToClient> connection_;
+  MockClientStub client_stub_;
+  MockVideoStub video_stub_;
+  MockVideoFrameCapturer capturer_;
 
   // The following mock objects are owned by VideoScheduler.
-  MockVideoFrameCapturer capturer_;
   MockVideoEncoder* encoder_;
 
  private:
@@ -150,11 +132,12 @@ TEST_F(VideoSchedulerTest, StartAndStop) {
   SkISize size(SkISize::Make(kWidth, kHeight));
   scoped_refptr<CaptureData> data(new CaptureData(planes, size, kFormat));
 
+  // Create a RunLoop through which to drive |message_loop_|.
+  base::RunLoop run_loop;
+
   EXPECT_CALL(capturer_, size_most_recent())
       .WillRepeatedly(ReturnRef(size));
 
-  EXPECT_CALL(capturer_, InvalidateRegion(_));
-
   // First the capturer is called.
   Expectation capturer_capture = EXPECT_CALL(capturer_, CaptureInvalidRegion(_))
       .After(capturer_start)
@@ -164,38 +147,24 @@ TEST_F(VideoSchedulerTest, StartAndStop) {
   EXPECT_CALL(*encoder_, Encode(data, false, _))
       .WillRepeatedly(FinishEncode());
 
-  MockVideoStub video_stub;
-  EXPECT_CALL(*connection_, video_stub())
-      .WillRepeatedly(Return(&video_stub));
-
   // By default delete the arguments when ProcessVideoPacket is received.
-  EXPECT_CALL(video_stub, ProcessVideoPacketPtr(_, _))
+  EXPECT_CALL(video_stub_, ProcessVideoPacketPtr(_, _))
       .WillRepeatedly(FinishSend());
 
   // For the first time when ProcessVideoPacket is received we stop the
   // VideoScheduler.
-  EXPECT_CALL(video_stub, ProcessVideoPacketPtr(_, _))
+  EXPECT_CALL(video_stub_, ProcessVideoPacketPtr(_, _))
       .WillOnce(DoAll(
           FinishSend(),
-          StopVideoScheduler(scheduler_,
-                             base::Bind(&QuitMessageLoop, &message_loop_))))
+          StopVideoScheduler(&scheduler_, run_loop.QuitClosure())))
       .RetiresOnSaturation();
 
   EXPECT_CALL(capturer_, Stop())
       .After(capturer_capture);
 
-  // Add the mock client connection to the session.
-  scheduler_->AddConnection(connection_.get());
-
-  // Start capturing.
-  scheduler_->Start();
-  message_loop_.Run();
-}
-
-TEST_F(VideoSchedulerTest, StopWithoutStart) {
-  EXPECT_CALL(capturer_, Stop());
-  scheduler_->Stop(base::Bind(&QuitMessageLoop, &message_loop_));
-  message_loop_.Run();
+  // Start video frame capture.
+  StartVideoScheduler();
+  run_loop.Run();
 }
 
 }  // namespace remoting
diff --git a/remoting/protocol/fake_session.cc b/remoting/protocol/fake_session.cc
index e3e7da1..ad8d18fe 100644
--- a/remoting/protocol/fake_session.cc
+++ b/remoting/protocol/fake_session.cc
@@ -283,7 +283,7 @@ bool FakeUdpSocket::SetSendBufferSize(int32 size) {
 FakeSession::FakeSession()
     : event_handler_(NULL),
       candidate_config_(CandidateSessionConfig::CreateDefault()),
-      config_(SessionConfig::GetDefault()),
+      config_(SessionConfig::ForTest()),
       message_loop_(MessageLoop::current()),
       async_creation_(false),
       jid_(kTestJid),
diff --git a/remoting/protocol/jingle_session_unittest.cc b/remoting/protocol/jingle_session_unittest.cc
index 623de44..809dd65 100644
--- a/remoting/protocol/jingle_session_unittest.cc
+++ b/remoting/protocol/jingle_session_unittest.cc
@@ -97,7 +97,7 @@ class JingleSessionTest : public testing::Test {
     host_session_.reset(session);
     host_session_->SetEventHandler(&host_session_event_handler_);
 
-    session->set_config(SessionConfig::GetDefault());
+    session->set_config(SessionConfig::ForTest());
   }
 
   void OnClientChannelCreated(scoped_ptr<net::StreamSocket> socket) {
diff --git a/remoting/protocol/session_config.cc b/remoting/protocol/session_config.cc
index f825302..1f05a83 100644
--- a/remoting/protocol/session_config.cc
+++ b/remoting/protocol/session_config.cc
@@ -39,7 +39,7 @@ SessionConfig::SessionConfig() {
 }
 
 // static
-SessionConfig SessionConfig::GetDefault() {
+SessionConfig SessionConfig::ForTest() {
   SessionConfig result;
   result.set_control_config(ChannelConfig(ChannelConfig::TRANSPORT_STREAM,
                                           kDefaultStreamVersion,
diff --git a/remoting/protocol/session_config.h b/remoting/protocol/session_config.h
index e4e0574..bca9800 100644
--- a/remoting/protocol/session_config.h
+++ b/remoting/protocol/session_config.h
@@ -82,7 +82,8 @@ class SessionConfig {
     return audio_config_.transport != ChannelConfig::TRANSPORT_NONE;
   }
 
-  static SessionConfig GetDefault();
+  // Returns a suitable session configuration for use in tests.
+  static SessionConfig ForTest();
 
  private:
   ChannelConfig control_config_;