Add a done task to ScreenRecorder::Stop()

remoting/host/screen_recorder.cc

 // Copyright (c) 2010 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 "remoting/host/screen_recorder.h"
 
 #include <algorithm>
 
 #include "base/logging.h"
 #include "base/scoped_ptr.h"
@@ skipping 28 matching lines @@
     MessageLoop* encode_loop,
     MessageLoop* network_loop,
     Capturer* capturer,
     Encoder* encoder)
     : capture_loop_(capture_loop),
       encode_loop_(encode_loop),
       network_loop_(network_loop),
       capturer_(capturer),
       encoder_(encoder),
       started_(false),
+      network_stopped_(false),
       recordings_(0),
       frame_skipped_(false),
       max_rate_(kDefaultCaptureRate) {
   DCHECK(capture_loop_);
   DCHECK(encode_loop_);
   DCHECK(network_loop_);
 }
 
 ScreenRecorder::~ScreenRecorder() {
   connections_.clear();
 }
 
 // Public methods --------------------------------------------------------------
 
 void ScreenRecorder::Start() {
   capture_loop_->PostTask(
       FROM_HERE, NewTracedMethod(this, &ScreenRecorder::DoStart));
 }
 
-void ScreenRecorder::Pause() {
+void ScreenRecorder::Stop(Task* done_task) {

[awong, 2011/01/20 20:14:00]

Can we fix this combine the all the Stop/DoStop, SetMaxRate/DoSetMaxRate
functions?

Can be a separate CL, but we should fix the convention.

[Alpha Left Google, 2011/01/20 22:48:47]

will do separately.

   capture_loop_->PostTask(
-      FROM_HERE, NewTracedMethod(this, &ScreenRecorder::DoPause));
+      FROM_HERE, NewTracedMethod(this, &ScreenRecorder::DoStop, done_task));
 }
 
 void ScreenRecorder::SetMaxRate(double rate) {
   capture_loop_->PostTask(
       FROM_HERE, NewTracedMethod(this, &ScreenRecorder::DoSetMaxRate, rate));
 }
 
 void ScreenRecorder::AddConnection(
     scoped_refptr<ConnectionToClient> connection) {
   ScopedTracer tracer("AddConnection");
@@ skipping 28 matching lines @@
 Encoder* ScreenRecorder::encoder() {
   DCHECK_EQ(encode_loop_, MessageLoop::current());
   DCHECK(encoder_.get());
   return encoder_.get();
 }
 
 // Capturer thread -------------------------------------------------------------
 
 void ScreenRecorder::DoStart() {
   DCHECK_EQ(capture_loop_, MessageLoop::current());
-  DCHECK(!started_);
 
   if (started_) {
     NOTREACHED() << "Record session already started.";
     return;
   }
 
   started_ = true;
   StartCaptureTimer();
 
   // Capture first frame immedately.
   DoCapture();
 }
 
-void ScreenRecorder::DoPause() {
+void ScreenRecorder::DoStop(Task* done_task) {
   DCHECK_EQ(capture_loop_, MessageLoop::current());
 
   if (!started_) {
     NOTREACHED() << "Record session not started.";
     return;
   }
 
   capture_timer_.Stop();
   started_ = 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) {
   DCHECK_EQ(capture_loop_, MessageLoop::current());
 
   // TODO(hclam): Should also check for small epsilon.
   DCHECK_GT(max_rate, 0.0) << "Rate is too small.";
 
   max_rate_ = max_rate;
 
@@ skipping 24 matching lines @@
 
   if (frame_skipped_) {
     frame_skipped_ = false;
     capture_timer_.Reset();
   }
 
   TraceContext::tracer()->PrintString("Capture Started");
 
   // 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(
       NewCallback(this, &ScreenRecorder::CaptureDoneCallback));
 }
 
 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 (!started_)

[awong, 2011/01/20 20:14:00]

Is this a normal condition?

If so, should we warn?

[Alpha Left Google, 2011/01/20 22:48:47]

You're right, this should be a DCHECK. The check in the next method is valid
though.

+    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 (!started_)
+    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.
   DoCapture();
 }
 
 // Network thread --------------------------------------------------------------
 
 void ScreenRecorder::DoSendVideoPacket(VideoPacket* packet) {
   DCHECK_EQ(network_loop_, MessageLoop::current());
 
   TraceContext::tracer()->PrintString("DoSendVideoPacket");
 
   bool last = (packet->flags() & VideoPacket::LAST_PARTITION) != 0;
 
+  if (network_stopped_) {

[awong, 2011/01/20 20:14:00]

This class's comment needs a description of the various states that it can be
in, and what that logically means.  We don't need to list out exactly which
functions are allowed, but having the states listed out would help

[Alpha Left Google, 2011/01/20 22:48:47]

Done.

+    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);
     }
 
     (*i)->video_stub()->ProcessVideoPacket(packet, done_task);
   }
 
   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(
     scoped_refptr<ConnectionToClient> connection) {
   DCHECK_EQ(network_loop_, MessageLoop::current());
 
   // TODO(hclam): Force a full frame for next encode.
   connections_.push_back(connection);
 }
 
 void ScreenRecorder::DoRemoveClient(
     scoped_refptr<ConnectionToClient> connection) {
   DCHECK_EQ(network_loop_, MessageLoop::current());
 
   // TODO(hclam): Is it correct to do to a scoped_refptr?
   ConnectionToClientList::iterator it =
       std::find(connections_.begin(), connections_.end(), connection);
   if (it != connections_.end()) {
     connections_.erase(it);
   }
 }
 
 void ScreenRecorder::DoRemoveAllClients() {
   DCHECK_EQ(network_loop_, MessageLoop::current());
 
   // Clear the list of connections.
   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(
     scoped_refptr<CaptureData> capture_data) {
   DCHECK_EQ(encode_loop_, MessageLoop::current());
   TraceContext::tracer()->PrintString("DoEncode called");
 
   // 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(
       FROM_HERE,
       NewTracedMethod(this, &ScreenRecorder::DoSendVideoPacket, packet));
 }
 
 }  // namespace remoting