Cleanup VideoFramePump.

remoting/host/screen_capturer_proxy.cc

+// Copyright 2015 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_capturer_proxy.h"
+
+#include "base/bind.h"
+#include "base/location.h"
+#include "base/single_thread_task_runner.h"
+#include "remoting/proto/control.pb.h"
+#include "third_party/webrtc/modules/desktop_capture/desktop_capturer.h"
+#include "third_party/webrtc/modules/desktop_capture/desktop_frame.h"
+#include "third_party/webrtc/modules/desktop_capture/desktop_region.h"
+
+namespace remoting {
+
+class ScreenCapturerProxy::Core : public webrtc::DesktopCapturer::Callback {

[Wez, 2015/02/11 01:05:07]

nit: You could make this class purely a DesktopCapturer::Callback
implementation, and have the ScreenCapturerProxy do:

PostTask(..., Bind(desktop_capturer_.get(), DesktopCapturer::Capture, ...))

directly - there's no real reason for the DesktopCapturer to be owned by the
ScreenCapturerProxy::Core; it just adds a bit more boilerplate to do so.
Instead, you could keep the Callback implementation purely responsible for
handling the capture completion.

[Sergey Ulanov, 2015/02/11 19:21:35]

the capturer and the callback would still need to be deleted on the capturer
thread, which means two DeleteSoon() calls in ~DesktopCapturerProxy(), so I
think it's cleaner to make the capturer owned by the Core. It's also more
consistent with other Proxy classes.

+ public:
+  Core(base::WeakPtr<ScreenCapturerProxy> proxy,
+       scoped_refptr<base::SingleThreadTaskRunner> network_task_runner,
+       scoped_refptr<base::SingleThreadTaskRunner> capture_task_runner,
+       scoped_ptr<webrtc::DesktopCapturer> capturer);
+  ~Core();
+
+  void Start();
+  void Capture();
+
+ private:
+  // webrtc::DesktopCapturer::Callback implementation.
+  webrtc::SharedMemory* CreateSharedMemory(size_t size) override;
+  void OnCaptureCompleted(webrtc::DesktopFrame* frame) override;
+
+

[Wez, 2015/02/11 01:05:07]

nit: Duplicate blank line?

[Sergey Ulanov, 2015/02/11 19:21:35]

Done.

+  base::WeakPtr<ScreenCapturerProxy> proxy_;
+  scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_;
+  scoped_refptr<base::SingleThreadTaskRunner> capture_task_runner_;
+  scoped_ptr<webrtc::DesktopCapturer> capturer_;
+
+  DISALLOW_COPY_AND_ASSIGN(Core);
+};
+
+ScreenCapturerProxy::Core::Core(
+    base::WeakPtr<ScreenCapturerProxy> proxy,
+    scoped_refptr<base::SingleThreadTaskRunner> network_task_runner,
+    scoped_refptr<base::SingleThreadTaskRunner> capture_task_runner,
+    scoped_ptr<webrtc::DesktopCapturer> capturer)
+    : proxy_(proxy),
+      network_task_runner_(network_task_runner),
+      capture_task_runner_(capture_task_runner),
+      capturer_(capturer.Pass()) {
+}
+
+void ScreenCapturerProxy::Core::Start() {
+  DCHECK(capture_task_runner_->BelongsToCurrentThread());
+
+  capturer_->Start(this);
+}
+
+ScreenCapturerProxy::Core::~Core() {

[Wez, 2015/02/11 01:05:07]

nit: This seems to be in the wrong order wrt Start()?

[Sergey Ulanov, 2015/02/11 19:21:35]

Done.

+  DCHECK(capture_task_runner_->BelongsToCurrentThread());
+}
+
+void ScreenCapturerProxy::Core::Capture() {
+  DCHECK(capture_task_runner_->BelongsToCurrentThread());
+
+  capturer_->Capture(webrtc::DesktopRegion());
+}
+
+webrtc::SharedMemory* ScreenCapturerProxy::Core::CreateSharedMemory(
+    size_t size) {
+  return nullptr;
+}
+
+void ScreenCapturerProxy::Core::OnCaptureCompleted(
+    webrtc::DesktopFrame* frame) {
+  DCHECK(capture_task_runner_->BelongsToCurrentThread());
+
+  network_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&ScreenCapturerProxy::OnFrameCaptured, proxy_,
+                            base::Passed(make_scoped_ptr(frame))));
+}

[Wez, 2015/02/11 01:05:07]

nit: Missing blank line.

[Sergey Ulanov, 2015/02/11 19:21:35]

Done.

+ScreenCapturerProxy::ScreenCapturerProxy(
+    scoped_refptr<base::SingleThreadTaskRunner> network_task_runner,
+    scoped_refptr<base::SingleThreadTaskRunner> capture_task_runner,
+    scoped_ptr<webrtc::DesktopCapturer> capturer)
+    : network_task_runner_(network_task_runner),
+      capture_task_runner_(capture_task_runner),
+      weak_factory_(this) {
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+
+  core_.reset(new Core(weak_factory_.GetWeakPtr(), network_task_runner,
+                       capture_task_runner, capturer.Pass()));
+}
+
+void ScreenCapturerProxy::Init(
+    const FrameCapturedCallback& frame_captured_callback) {
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+
+  frame_captured_callback_ = frame_captured_callback;
+
+  capture_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&Core::Start, base::Unretained(core_.get())));
+}
+
+ScreenCapturerProxy::~ScreenCapturerProxy() {
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+
+  capture_task_runner_->DeleteSoon(FROM_HERE, core_.release());
+}
+
+void ScreenCapturerProxy::Capture() {
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+
+  // Start() must be called before Capture().
+  DCHECK(!frame_captured_callback_.is_null());
+
+  capture_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&Core::Capture, base::Unretained(core_.get())));
+}
+
+void ScreenCapturerProxy::OnFrameCaptured(
+    scoped_ptr<webrtc::DesktopFrame> frame) {
+  DCHECK(network_task_runner_->BelongsToCurrentThread());
+
+  frame_captured_callback_.Run(frame.Pass());
+}
+
+}  // namespace remoting