Add a done task to ScreenRecorder::Stop()

ScreenRecorder::Stop() need a done task to indicate it is actually paused.
This is needed so that we can shutdown threads safely.

BUG=69997
TEST=remoting_unittests --gtest_filter=ScreenRecorder.*

Review URL: http://codereview.chromium.org/6282006

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

remoting/host/chromoting_host.cc

@@ -121,9 +121,9 @@ void ChromotingHost::Shutdown() {
     state_ = kStopped;
   }
 
-  // Tell the session to pause and then disconnect all clients.
+  // Tell the session to stop and then disconnect all clients.
   if (recorder_.get()) {
-    recorder_->Pause();
+    recorder_->Stop(NULL);
     recorder_->RemoveAllConnections();
   }
 
@@ -182,11 +182,11 @@ void ChromotingHost::OnClientConnected(ConnectionToClient* connection) {
 void ChromotingHost::OnClientDisconnected(ConnectionToClient* connection) {
   DCHECK_EQ(context_->main_message_loop(), MessageLoop::current());
 
-  // Remove the connection from the session manager and pause the session.
-  // TODO(hclam): Pause only if the last connection disconnected.
+  // Remove the connection from the session manager and stop the session.
+  // TODO(hclam): Stop only if the last connection disconnected.
   if (recorder_.get()) {
     recorder_->RemoveConnection(connection);
-    recorder_->Pause();
+    recorder_->Stop(NULL);
   }
 
   // Close the connection to connection just to be safe.

remoting/host/screen_recorder.cc

@@ -45,7 +45,8 @@ ScreenRecorder::ScreenRecorder(
       network_loop_(network_loop),
       capturer_(capturer),
       encoder_(encoder),
-      started_(false),
+      is_recording_(false),
+      network_stopped_(false),
       recordings_(0),
       frame_skipped_(false),
       max_rate_(kDefaultCaptureRate) {
@@ -55,7 +56,6 @@ ScreenRecorder::ScreenRecorder(
 }
 
 ScreenRecorder::~ScreenRecorder() {
-  connections_.clear();
 }
 
 // Public methods --------------------------------------------------------------
@@ -65,9 +65,9 @@ void ScreenRecorder::Start() {
       FROM_HERE, NewTracedMethod(this, &ScreenRecorder::DoStart));
 }
 
-void ScreenRecorder::Pause() {
+void ScreenRecorder::Stop(Task* done_task) {
   capture_loop_->PostTask(
-      FROM_HERE, NewTracedMethod(this, &ScreenRecorder::DoPause));
+      FROM_HERE, NewTracedMethod(this, &ScreenRecorder::DoStop, done_task));
 }
 
 void ScreenRecorder::SetMaxRate(double rate) {
@@ -116,30 +116,49 @@ Encoder* ScreenRecorder::encoder() {
 
 void ScreenRecorder::DoStart() {
   DCHECK_EQ(capture_loop_, MessageLoop::current());
-  DCHECK(!started_);
 
-  if (started_) {
+  if (is_recording_) {
     NOTREACHED() << "Record session already started.";
     return;
   }
 
-  started_ = true;
+  is_recording_ = true;
   StartCaptureTimer();
 
   // Capture first frame immedately.
   DoCapture();
 }
 
-void ScreenRecorder::DoPause() {
+void ScreenRecorder::DoStop(Task* done_task) {
   DCHECK_EQ(capture_loop_, MessageLoop::current());
 
-  if (!started_) {
+  if (!is_recording_) {
     NOTREACHED() << "Record session not started.";
     return;
   }
 
   capture_timer_.Stop();
-  started_ = false;
+  is_recording_ = false;
+
+  DCHECK_GE(recordings_, 0);
+  if (recordings_) {
+     network_loop_->PostTask(
+        FROM_HERE,
+        NewTracedMethod(this,
+                        &ScreenRecorder::DoStopOnNetworkThread, done_task));
+    return;
+  }
+
+  DoCompleteStop(done_task);
+}
+
+void ScreenRecorder::DoCompleteStop(Task* done_task) {
+  DCHECK_EQ(capture_loop_, MessageLoop::current());
+
+  if (done_task) {
+    done_task->Run();
+    delete done_task;
+  }
 }
 
 void ScreenRecorder::DoSetMaxRate(double max_rate) {
@@ -151,7 +170,7 @@ void ScreenRecorder::DoSetMaxRate(double max_rate) {
   max_rate_ = max_rate;
 
   // Restart the timer with the new rate.
-  if (started_) {
+  if (is_recording_) {
     capture_timer_.Stop();
     StartCaptureTimer();
   }
@@ -170,7 +189,7 @@ void ScreenRecorder::DoCapture() {
   // 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_ >= kMaxRecordings || !started_) {
+  if (recordings_ >= kMaxRecordings || !is_recording_) {
     frame_skipped_ = true;
     return;
   }
@@ -184,6 +203,7 @@ void ScreenRecorder::DoCapture() {
 
   // At this point we are going to perform one capture so save the current time.
   ++recordings_;
+  DCHECK_LE(recordings_, kMaxRecordings);
 
   // And finally perform one capture.
   capturer()->CaptureInvalidRects(
@@ -192,21 +212,27 @@ void ScreenRecorder::DoCapture() {
 
 void ScreenRecorder::CaptureDoneCallback(
     scoped_refptr<CaptureData> capture_data) {
-  // TODO(hclam): There is a bug if the capturer doesn't produce any dirty
-  // rects.
   DCHECK_EQ(capture_loop_, MessageLoop::current());
+
+  if (!is_recording_)
+    return;
+
   TraceContext::tracer()->PrintString("Capture Done");
   encode_loop_->PostTask(
       FROM_HERE,
       NewTracedMethod(this, &ScreenRecorder::DoEncode, capture_data));
 }
 
-void ScreenRecorder::DoFinishSend() {
+void ScreenRecorder::DoFinishOneRecording() {
   DCHECK_EQ(capture_loop_, MessageLoop::current());
 
+  if (!is_recording_)
+    return;
+
   // Decrement the number of recording in process since we have completed
   // one cycle.
   --recordings_;
+  DCHECK_GE(recordings_, 0);
 
   // Try to do a capture again. Note that the following method may do nothing
   // if it is too early to perform a capture.
@@ -222,14 +248,23 @@ void ScreenRecorder::DoSendVideoPacket(VideoPacket* packet) {
 
   bool last = (packet->flags() & VideoPacket::LAST_PARTITION) != 0;
 
+  if (network_stopped_) {
+    delete packet;
+    return;
+  }
+
   for (ConnectionToClientList::const_iterator i = connections_.begin();
        i < connections_.end(); ++i) {
     Task* done_task = NULL;
 
-    // Call OnFrameSent() only for the last packet in the first connection.
+    // Call FrameSentCallback() only for the last packet in the first
+    // connection.
     if (last && i == connections_.begin()) {
-      done_task = NewTracedMethod(this, &ScreenRecorder::OnFrameSent, packet);
+      done_task = NewTracedMethod(this, &ScreenRecorder::FrameSentCallback,
+                                  packet);
     } else {
+      // TODO(hclam): Fix this code since it causes multiple deletion if there's
+      // more than one connection.
       done_task = new DeleteTask<VideoPacket>(packet);
     }
 
@@ -239,10 +274,14 @@ void ScreenRecorder::DoSendVideoPacket(VideoPacket* packet) {
   TraceContext::tracer()->PrintString("DoSendVideoPacket done");
 }
 
-void ScreenRecorder::OnFrameSent(VideoPacket* packet) {
+void ScreenRecorder::FrameSentCallback(VideoPacket* packet) {
   delete packet;
+
+  if (network_stopped_)
+    return;
+
   capture_loop_->PostTask(
-      FROM_HERE, NewTracedMethod(this, &ScreenRecorder::DoFinishSend));
+      FROM_HERE, NewTracedMethod(this, &ScreenRecorder::DoFinishOneRecording));
 }
 
 void ScreenRecorder::DoAddConnection(
@@ -272,6 +311,22 @@ void ScreenRecorder::DoRemoveAllClients() {
   connections_.clear();
 }
 
+void ScreenRecorder::DoStopOnNetworkThread(Task* done_task) {
+  DCHECK_EQ(network_loop_, MessageLoop::current());
+
+  // 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_loop_->PostTask(
+      FROM_HERE,
+      NewTracedMethod(this, &ScreenRecorder::DoStopOnEncodeThread, done_task));
+}
+
 // Encoder thread --------------------------------------------------------------
 
 void ScreenRecorder::DoEncode(
@@ -282,19 +337,31 @@ void ScreenRecorder::DoEncode(
   // Early out if there's nothing to encode.
   if (!capture_data->dirty_rects().size()) {
     capture_loop_->PostTask(
-        FROM_HERE, NewTracedMethod(this, &ScreenRecorder::DoFinishSend));
+        FROM_HERE,
+        NewTracedMethod(this, &ScreenRecorder::DoFinishOneRecording));
     return;
   }
 
-  // TODO(hclam): Enable |force_refresh| if a new connection was
-  // added.
+  // TODO(hclam): Invalidate the full screen if there is a new connection added.
   TraceContext::tracer()->PrintString("Encode start");
-  encoder()->Encode(capture_data, false,
-                   NewCallback(this, &ScreenRecorder::EncodeDataAvailableTask));
+  encoder()->Encode(
+      capture_data, false,
+      NewCallback(this, &ScreenRecorder::EncodedDataAvailableCallback));
   TraceContext::tracer()->PrintString("Encode Done");
 }
 
-void ScreenRecorder::EncodeDataAvailableTask(VideoPacket* packet) {
+void ScreenRecorder::DoStopOnEncodeThread(Task* done_task) {
+  DCHECK_EQ(encode_loop_, MessageLoop::current());
+
+  // 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_loop_->PostTask(
+      FROM_HERE,
+      NewTracedMethod(this, &ScreenRecorder::DoCompleteStop, done_task));
+}
+
+void ScreenRecorder::EncodedDataAvailableCallback(VideoPacket* packet) {
   DCHECK_EQ(encode_loop_, MessageLoop::current());
 
   network_loop_->PostTask(

remoting/host/screen_recorder.h

@@ -60,6 +60,12 @@ class CaptureData;
 // 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:
+// |is_recording_| - If this is set to false 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.
 class ScreenRecorder : public base::RefCountedThreadSafe<ScreenRecorder> {
  public:
 
@@ -76,8 +82,9 @@ class ScreenRecorder : public base::RefCountedThreadSafe<ScreenRecorder> {
   // Start recording.
   void Start();
 
-  // Pause the recording session.
-  void Pause();
+  // Stop the recording session. |done_task| is executed when recording is fully
+  // stopped. This object cannot be used again after |task| is executed.
+  void Stop(Task* done_task);
 
   // Set the maximum capture rate. This is denoted by number of updates
   // in one second. The actual system may run in a slower rate than the maximum
@@ -103,7 +110,8 @@ class ScreenRecorder : public base::RefCountedThreadSafe<ScreenRecorder> {
   // Capturer thread ----------------------------------------------------------
 
   void DoStart();
-  void DoPause();
+  void DoStop(Task* done_task);
+  void DoCompleteStop(Task* done_task);
 
   void DoSetMaxRate(double max_rate);
 
@@ -112,7 +120,7 @@ class ScreenRecorder : public base::RefCountedThreadSafe<ScreenRecorder> {
 
   void DoCapture();
   void CaptureDoneCallback(scoped_refptr<CaptureData> capture_data);
-  void DoFinishSend();
+  void DoFinishOneRecording();
 
   // Network thread -----------------------------------------------------------
 
@@ -127,16 +135,22 @@ class ScreenRecorder : public base::RefCountedThreadSafe<ScreenRecorder> {
   void DoRemoveClient(scoped_refptr<protocol::ConnectionToClient> connection);
   void DoRemoveAllClients();
 
+  // Signal network thread to cease activities.
+  void DoStopOnNetworkThread(Task* done_task);
+
   // Callback for the last packet in one update. Deletes |packet| and
   // schedules next screen capture.
-  void OnFrameSent(VideoPacket* packet);
+  void FrameSentCallback(VideoPacket* packet);
 
   // Encoder thread -----------------------------------------------------------
 
   void DoEncode(scoped_refptr<CaptureData> capture_data);
 
-  // EncodeDataAvailableTask takes ownership of |packet|.
-  void EncodeDataAvailableTask(VideoPacket* packet);
+  // Perform stop operations on encode thread.
+  void DoStopOnEncodeThread(Task* done_task);
+
+  // EncodedDataAvailableCallback takes ownership of |packet|.
+  void EncodedDataAvailableCallback(VideoPacket* packet);
   void SendVideoPacket(VideoPacket* packet);
 
   // Message loops used by this class.
@@ -153,15 +167,18 @@ class ScreenRecorder : public base::RefCountedThreadSafe<ScreenRecorder> {
   scoped_ptr<Encoder> encoder_;
 
   // A list of clients connected to this hosts.
-  // This member is always accessed on the NETWORK thread.
-  // TODO(hclam): Have to scoped_refptr the clients since they have a shorter
-  // lifetime than this object.
+  // This member is always accessed on the network thread.
   typedef std::vector<scoped_refptr<protocol::ConnectionToClient> >
       ConnectionToClientList;
   ConnectionToClientList connections_;
 
-  // The following members are accessed on the capture thread.
-  bool started_;
+  // Flag that indicates recording has been started. This variable should only
+  // be used on the capture thread.
+  bool is_recording_;
+
+  // Flag that indicates network is being stopped. This variable should only
+  // be used on the network thread.
+  bool network_stopped_;
 
   // Timer that calls DoCapture.
   base::RepeatingTimer<ScreenRecorder> capture_timer_;

remoting/host/screen_recorder_unittest.cc

@@ -17,12 +17,52 @@ using ::remoting::protocol::MockVideoStub;
 
 using ::testing::_;
 using ::testing::AtLeast;
+using ::testing::DeleteArg;
+using ::testing::DoAll;
+using ::testing::InSequence;
+using ::testing::InvokeWithoutArgs;
 using ::testing::NotNull;
 using ::testing::Return;
 using ::testing::SaveArg;
 
 namespace remoting {
 
+namespace {
+
+ACTION_P2(RunCallback, rects, data) {
+  InvalidRects& dirty_rects = data->mutable_dirty_rects();
+  InvalidRects temp_rects;
+  std::set_union(dirty_rects.begin(), dirty_rects.end(),
+                 rects.begin(), rects.end(),
+                 std::inserter(temp_rects, temp_rects.begin()));
+  dirty_rects.swap(temp_rects);
+  arg0->Run(data);
+  delete arg0;
+}
+
+ACTION(FinishEncode) {
+  scoped_ptr<VideoPacket> packet(new VideoPacket());
+  packet->set_flags(VideoPacket::LAST_PACKET | VideoPacket::LAST_PARTITION);
+  arg2->Run(packet.release());
+  delete arg2;
+}
+
+ACTION(FinishSend) {
+  arg1->Run();
+  delete arg1;
+}
+
+// Helper method to quit the main message loop.
+void QuitMessageLoop(MessageLoop* message_loop) {
+  message_loop->PostTask(FROM_HERE, new MessageLoop::QuitTask());
+}
+
+ACTION_P2(StopScreenRecorder, recorder, task) {
+  recorder->Stop(task);
+}
+
+}  // namespace
+
 static const int kWidth = 640;
 static const int kHeight = 480;
 static const media::VideoFrame::Format kFormat = media::VideoFrame::RGB32;
@@ -56,22 +96,8 @@ class ScreenRecorderTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(ScreenRecorderTest);
 };
 
-ACTION_P2(RunCallback, rects, data) {
-  InvalidRects& dirty_rects = data->mutable_dirty_rects();
-  InvalidRects temp_rects;
-  std::set_union(dirty_rects.begin(), dirty_rects.end(),
-                 rects.begin(), rects.end(),
-                 std::inserter(temp_rects, temp_rects.begin()));
-  dirty_rects.swap(temp_rects);
-  arg0->Run(data);
-  delete arg0;
-}
-
-ACTION_P(FinishEncode, msg) {
-  arg2->Run(msg);
-  delete arg2;
-}
-
+// This test mocks capturer, encoder and network layer to operate one recording
+// cycle.
 TEST_F(ScreenRecorderTest, OneRecordCycle) {
   InvalidRects update_rects;
   update_rects.insert(gfx::Rect(0, 0, 10, 10));
@@ -82,49 +108,94 @@ TEST_F(ScreenRecorderTest, OneRecordCycle) {
   }
   scoped_refptr<CaptureData> data(new CaptureData(planes, kWidth,
                                                   kHeight, kFormat));
-  // Set the recording rate to very low to avoid capture twice.
-  record_->SetMaxRate(0.01);
-
-  // Add the mock client connection to the session.
   EXPECT_CALL(*capturer_, width()).WillRepeatedly(Return(kWidth));
   EXPECT_CALL(*capturer_, height()).WillRepeatedly(Return(kHeight));
-  record_->AddConnection(connection_);
 
   // First the capturer is called.
   EXPECT_CALL(*capturer_, CaptureInvalidRects(NotNull()))
       .WillOnce(RunCallback(update_rects, data));
 
   // Expect the encoder be called.
-  VideoPacket* packet = new VideoPacket();
   EXPECT_CALL(*encoder_, Encode(data, false, NotNull()))
-      .WillOnce(FinishEncode(packet));
+      .WillOnce(FinishEncode());
 
   MockVideoStub video_stub;
   EXPECT_CALL(*connection_, video_stub())
       .WillRepeatedly(Return(&video_stub));
 
-  Task* done_task = NULL;
-
   // Expect the client be notified.
   EXPECT_CALL(video_stub, ProcessVideoPacket(_, _))
       .Times(1)
-      .WillOnce(SaveArg<1>(&done_task));
+      .WillOnce(DoAll(DeleteArg<0>(), DeleteArg<1>()));
   EXPECT_CALL(video_stub, GetPendingPackets())
       .Times(AtLeast(0))
       .WillRepeatedly(Return(0));
 
+  // Set the recording rate to very low to avoid capture twice.
+  record_->SetMaxRate(0.01);
+
+  // Add the mock client connection to the session.
+  record_->AddConnection(connection_);
+
   // Start the recording.
   record_->Start();
 
   // Make sure all tasks are completed.
   message_loop_.RunAllPending();
+}
+
+// This test mocks capturer, encoder and network layer to simulate one recording
+// cycle. When the first encoded packet is submitted to the network
+// ScreenRecorder is instructed to come to a complete stop. We expect the stop
+// sequence to be executed successfully.
+TEST_F(ScreenRecorderTest, StartAndStop) {
+  InvalidRects update_rects;
+  update_rects.insert(gfx::Rect(0, 0, 10, 10));
+  DataPlanes planes;
+  for (int i = 0; i < DataPlanes::kPlaneCount; ++i) {
+    planes.data[i] = reinterpret_cast<uint8*>(i);
+    planes.strides[i] = kWidth * 4;
+  }
+  scoped_refptr<CaptureData> data(new CaptureData(planes, kWidth,
+                                                  kHeight, kFormat));
+  EXPECT_CALL(*capturer_, width()).WillRepeatedly(Return(kWidth));
+  EXPECT_CALL(*capturer_, height()).WillRepeatedly(Return(kHeight));
+
+  // First the capturer is called.
+  EXPECT_CALL(*capturer_, CaptureInvalidRects(NotNull()))
+      .WillRepeatedly(RunCallback(update_rects, data));
+
+  // Expect the encoder be called.
+  EXPECT_CALL(*encoder_, Encode(data, false, NotNull()))
+      .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, ProcessVideoPacket(_, _))
+      .WillRepeatedly(FinishSend());
+
+  // For the first time when ProcessVideoPacket is received we stop the
+  // ScreenRecorder.
+  EXPECT_CALL(video_stub, ProcessVideoPacket(_, _))
+      .WillOnce(DoAll(
+          FinishSend(),
+          StopScreenRecorder(record_,
+                             NewRunnableFunction(&QuitMessageLoop,
+                                                 &message_loop_))))
+      .RetiresOnSaturation();
 
-  done_task->Run();
-  delete done_task;
+  // Add the mock client connection to the session.
+  record_->AddConnection(connection_);
+
+  // Start the recording.
+  record_->Start();
+  message_loop_.Run();
 }
 
 // TODO(hclam): Add test for double buffering.
 // TODO(hclam): Add test for multiple captures.
-// TODO(hclam): Add test for interruption.
 
 }  // namespace remoting