Simplify ScreenCapturer interface.

remoting/host/video_scheduler.cc

 // Copyright (c) 2012 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/video_scheduler.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 160 matching lines @@
   capture_timer_.reset(new base::OneShotTimer<VideoScheduler>());
 
   // Capture first frame immedately.
   CaptureNextFrame();
 }
 
 void VideoScheduler::StopOnCaptureThread() {
   DCHECK(capture_task_runner_->BelongsToCurrentThread());
 
   // Stop |capturer_| and clear it to prevent pending tasks from using it.
-  capturer_->Stop();
+  capturer_.reset();

[alexeypa (please no reviews), 2013/04/22 16:23:19]

|capturer_| can only be deleted only after the encoding thread stopped reading
the frames. Note that |capturer_| is passed to StopOnEncodeThread() and then to
StopOnNetworkThread().


This could be avoided if we move frames allocation (and frame queue management)
out of the capturer to the caller. The capturer can still provide code for
allocating frames (as a separate class most likely) but the caller would decide
when to allocate them. CaptureFrame() would check if the frame is compatible
with the current resolution and return an error if it is not.

This is out of scope for this CL I guess but it is something to consider.

[Sergey Ulanov, 2013/04/22 18:18:56]

Oh, right. Do we need to call Stop() here at all?

With the refactoring I'm working on I'm going to fix this problem: it will be
safe to delete capturer while the frames still being processed (will upload it
for review today).
As I mentioned in previous response I think it's the captured who should be
allocating frames - only it has all details needed to allocated it properly.

[alexeypa (please no reviews), 2013/04/22 18:36:37]

Yes. It clears |delegate_| preventing the capturer from using 
It is true that frame allocation is capturer specific. What I don't like about
it is that the capturer has to manage the queue of frames. One issue that you
mentioned elsewhere - "what if InvalidateRegion() is called in between
CaptureFrame() and OnCaptureCompleted()?" is directly related to the frame queue
management. We "solve" it by making the capturer single-threaded. We, probably,
should express this arrangement in the capturer interface. 

 
   // |capture_timer_| must be destroyed on the thread on which it is used.
   capture_timer_.reset();
 
   // Schedule deletion of |capturer_| once the encode thread is no longer
   // processing capture data. See http://crbug.com/163641. This also clears
   // |capturer_| pointer to prevent pending tasks from using it.
   // TODO(wez): Make it safe to tear down capturer while buffers remain, and
   // remove this work-around.
   encode_task_runner_->PostTask(
@@ skipping 136 matching lines @@
 
   // This is posted by StopOnCaptureThread, so we know that by the time we
   // process it there are no more encode tasks queued. Pass |capturer_| for
   // deletion on the thread that created it.
   network_task_runner_->PostTask(
       FROM_HERE, base::Bind(&VideoScheduler::StopOnNetworkThread, this,
                             base::Passed(&capturer)));
 }
 
 }  // namespace remoting