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* task) {
   capture_loop_->PostTask(
-      FROM_HERE, NewTracedMethod(this, &ScreenRecorder::DoPause));
+      FROM_HERE, NewTracedMethod(this, &ScreenRecorder::DoStop, 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* 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, task));
+    return;
+  }
+
+  if (task) {

[Sergey Ulanov, 2011/01/20 19:14:17]

Call DoCompleteStop()?

[Alpha Left Google, 2011/01/20 19:32:08]

Done.

+    task->Run();
+    delete task;
+  }
+}
+
+void ScreenRecorder::DoCompleteStop(Task* task) {
+  DCHECK_EQ(capture_loop_, MessageLoop::current());
+
+  if (task) {
+    task->Run();
+    delete 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_)
+    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_) {
+    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.

[Sergey Ulanov, 2011/01/20 19:14:17]

I think we should just remove support for parallel connections

[Alpha Left Google, 2011/01/20 19:24:18]

I'd leave the decision later, not really related to this patch.

       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_)

[Sergey Ulanov, 2011/01/20 19:14:17]

Do we really need network_stopped_? It should be safe to post
DoFinishOneRecording task on capture_loop_, it doesn't do anything when started_
== false. If you remove network_stopped_, then we also don't need
DoStopOnNetworkThread, and the whole shutdown process becomes much simpler.

Is the problem that capture_loop_ may not exist at this point? Maybe we should
use MessageLoopProxy instead of MessageLoop?

[Alpha Left Google, 2011/01/20 19:24:18]

The problem that needs network_stopped_ here is that when we are shutting down
capture loop, network loop can receive a callback and needs to post back to the
capture loop and that causes race conditions in MessageLoop code.

I think we've gone through this argument before, the reason that we're not using
MessageLoopProxy was because we thought we can order destruction nicely and
doesn't need refcounting. Since we made that decision in the first place we
should try to make it happen.

+    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* 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, 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): Invalid the full screen if there is a new connection added.

[Sergey Ulanov, 2011/01/20 19:14:17]

Invalidate

   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* 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, task));
+}
+
+void ScreenRecorder::EncodedDataAvailableCallback(VideoPacket* packet) {
   DCHECK_EQ(encode_loop_, MessageLoop::current());
 
   network_loop_->PostTask(
       FROM_HERE,
       NewTracedMethod(this, &ScreenRecorder::DoSendVideoPacket, packet));
 }
 
 }  // namespace remoting