Implement screen capturer for MediaStream API.

media/video/capture/screen/screen_capture_device.cc

Index: media/video/capture/screen/screen_capture_device.cc
diff --git a/media/video/capture/screen/screen_capture_device.cc b/media/video/capture/screen/screen_capture_device.cc
new file mode 100644
index 0000000000000000000000000000000000000000..1cbfe196038963b24696053063f1c80e6b303aea
--- /dev/null
+++ b/media/video/capture/screen/screen_capture_device.cc
@@ -0,0 +1,371 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.

[Wez, 2013/01/31 00:15:29]

Drop (c), change to 2013.

[Sergey Ulanov, 2013/01/31 02:05:43]

same as in the previous file.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "media/video/capture/screen/screen_capture_device.h"
+
+#include "base/bind.h"
+#include "base/location.h"
+#include "base/logging.h"
+#include "base/sequenced_task_runner.h"
+#include "base/synchronization/lock.h"
+#include "media/video/capture/screen/mouse_cursor_shape.h"
+#include "media/video/capture/screen/screen_capture_data.h"
+#include "third_party/skia/include/core/SkCanvas.h"
+#include "third_party/skia/include/core/SkDevice.h"
+
+namespace media {
+
+namespace {
+const int kBytesPerPixel = 4;
+}  // namespace

[Wez, 2013/01/31 00:15:29]

nit: No need for // namespace comment if it's a one-liner like this, as far as
I'm aware.

[Sergey Ulanov, 2013/01/31 02:05:43]

It's better to have it for consistency.

+
+class ScreenCaptureDevice::Core
+    : public base::RefCountedThreadSafe<Core>,
+      public ScreenCapturer::Delegate {
+ public:
+  Core(scoped_refptr<base::SequencedTaskRunner> task_runner);

[Wez, 2013/01/31 00:15:29]

explicit

[Sergey Ulanov, 2013/01/31 02:05:43]

Done.

+
+  // Helper used in tests to supply a fake capturer.
+  void set_test_frame_capturer(

[Wez, 2013/01/31 00:15:29]

See comment on public interface re naming.

[Sergey Ulanov, 2013/01/31 02:05:43]

Done.

+      scoped_ptr<ScreenCapturer> capturer) {
+    video_frame_capturer_ = capturer.Pass();
+  }
+
+  void Allocate(int width, int height,

[Wez, 2013/01/31 00:15:29]

nit: Add comment, e.g. "Implementation of VideoCaptureDevice interface."

[Sergey Ulanov, 2013/01/31 02:05:43]

This is not implementation of the interface. Core doesn't implement
VideoCaptureDevice.

[Wez, 2013/02/01 00:44:34]

No, but these are the methods that actually implement that interface for
ScreenCaptureDevice.  You could equally describe them as the implementation of
ScreenCapturedDevice's public API.

+                int frame_rate,
+                EventHandler* event_handler);
+  void Start();
+  void Stop();
+  void DeAllocate();
+
+  // ScreenCapturer::Delegate interface. Called by |video_frame_capturer_| on
+  // the |task_runner_|.
+  virtual void OnCaptureCompleted(
+      scoped_refptr<ScreenCaptureData> capture_data) OVERRIDE;
+  virtual void OnCursorShapeChanged(
+      scoped_ptr<MouseCursorShape> cursor_shape) OVERRIDE;
+
+ private:
+  friend class base::RefCountedThreadSafe<Core>;
+  virtual ~Core();
+
+  // Helper methods that run on the |task_runner_|. Posted from the
+  // corresponding public methods.
+  void DoAllocate(int frame_rate);
+  void DoStart();
+  void DoStop();
+  void DoDeAllocate();
+
+  // Method that is scheduled on |task_runner_| to be called on regular interval
+  // to capture the screen.
+  void OnCaptureTimer();

[Wez, 2013/01/31 00:15:29]

nit: CaptureFrame() would be a better name for this, and the comment might be
"Captures the next frame. Called on |capture_task_runner_| at regular
intervals."

[Sergey Ulanov, 2013/01/31 02:05:43]

IMHO OnCaptureTimer() is better because it makes it clear that this method is
used to post a timer tasks, but not called directly.

+
+  // Captures a single frame.
+  void DoCapture();

[Wez, 2013/01/31 00:15:29]

nit: I'd prefer that DoFoo() only be used where there is already a public method
Foo() that the function is part of the implementation of; in this case
CaptureFrame() would be fine - looking at the implementation, I'm not sure why
OnCaptureTimer() and DoCapture() are separate?

[Sergey Ulanov, 2013/01/31 02:05:43]

Because OnCaptureTimer() also schedules next capture task. DoCapture() just
captures a frame without posting a new task. I believe that Do prefix is useful
here to make it clear that it's the method that actually captures next frame,
and not OnCaptureTimer(). Also, we often use Do prefix in remoting code when
there is no public method with corresponding name. E.g. see DoRead() in
remoting::MessageReader. media code also uses Do in similar cases.

+
+  // Task runner used for screen capturing operations.
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;

[Wez, 2013/01/31 00:15:29]

nit: capture_task_runner_ would be clearer, then.

[Sergey Ulanov, 2013/01/31 02:05:43]

I don't think it's necessary because there is only one task runner used by this
class and the class has "Capture" in the name.

+
+  // |event_handler_lock_| must be locked whenever |event_handler_| is used.
+  // It's necessary because DeAllocate() needs to reset it on the calling thread
+  // to ensure that the event handler is not called once DeAllocate() returns.
+  base::Lock event_handler_lock_;
+  EventHandler* event_handler_;
+
+  // Frame rate specified in Allocate().

[Wez, 2013/01/31 00:15:29]

nit: in -> to

[Sergey Ulanov, 2013/01/31 02:05:43]

Done.

+  int frame_rate_;
+
+  scoped_ptr<ScreenCapturer> video_frame_capturer_;

[Wez, 2013/01/31 00:15:29]

nit: Please add a comment, e.g. "The underlying ScreenCapturer instance used to
capture frames".

[Sergey Ulanov, 2013/01/31 02:05:43]

Done.

+
+  // After Allocate() is called we need to call OnFrameInfo() method of the
+  // |event_handler_| to specify the size of the frames this capturer will
+  // produce. In order to get screen size from |video_frame_capturer_| we need
+  // to capture at least one frame. Once screen size is known it's stored in
+  // |frame_size_|.
+  bool waiting_frame_size_;

[Wez, 2013/01/31 00:15:29]

nit: waiting_for_frame_size_ or !have_frame_size_

[Sergey Ulanov, 2013/01/31 02:05:43]

Done.

+  SkISize frame_size_;
+  SkBitmap resized_bitmap_;
+
+  // Set to true between DoStart() and DoStop().

[Wez, 2013/01/31 00:15:29]

nit: I don't think you need "Set to" for this or the following comments.

[Wez, 2013/01/31 00:15:29]

Please make clear that we can't just test |event_handler_| because this is
checked on the capture thread, not the caller one.

[Sergey Ulanov, 2013/01/31 02:05:43]

Done.

[Sergey Ulanov, 2013/01/31 02:05:43]

Done.

+  bool started_;
+
+  // Set to true when we have delayed OnCaptureTimer() task posted on
+  // |task_runner_|.
+  bool capture_task_posted_;
+
+  // Set to true when waiting for |video_frame_capturer_| to capture current
+  // frame.
+  bool capture_in_progress_;
+
+  DISALLOW_COPY_AND_ASSIGN(Core);
+};
+
+ScreenCaptureDevice::Core::Core(
+    scoped_refptr<base::SequencedTaskRunner> task_runner)
+    : task_runner_(task_runner),
+      event_handler_(NULL),
+      waiting_frame_size_(false),
+      started_(false),
+      capture_task_posted_(false),
+      capture_in_progress_(false) {
+}
+
+ScreenCaptureDevice::Core::~Core() {
+}
+
+void ScreenCaptureDevice::Core::Allocate(int width, int height,
+                                    int frame_rate,

[Wez, 2013/01/31 00:15:29]

Indentation.

[Sergey Ulanov, 2013/01/31 02:05:43]

Done.

+                                    EventHandler* event_handler) {
+  DCHECK_GT(width, 0);
+  DCHECK_GT(height, 0);
+  DCHECK_GT(frame_rate, 0);
+
+  {
+    base::AutoLock auto_lock(event_handler_lock_);
+    event_handler_ = event_handler;
+  }
+
+  task_runner_->PostTask(
+      FROM_HERE, base::Bind(&Core::DoAllocate, this, frame_rate));
+}
+
+void ScreenCaptureDevice::Core::Start() {
+  task_runner_->PostTask(
+      FROM_HERE, base::Bind(&Core::DoStart, this));
+}
+
+void ScreenCaptureDevice::Core::Stop() {
+  task_runner_->PostTask(
+      FROM_HERE, base::Bind(&Core::DoStop, this));
+}
+
+void ScreenCaptureDevice::Core::DeAllocate() {
+  {
+    base::AutoLock auto_lock(event_handler_lock_);
+    event_handler_ = NULL;
+  }
+  task_runner_->PostTask(FROM_HERE, base::Bind(&Core::DoDeAllocate, this));
+}
+
+void ScreenCaptureDevice::Core::OnCaptureCompleted(
+    scoped_refptr<ScreenCaptureData> capture_data) {
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
+  DCHECK(capture_in_progress_);
+  DCHECK(!capture_data->size().isEmpty());
+  capture_in_progress_ = false;

[Wez, 2013/01/31 00:15:29]

nit: Leave a blank line between the DCHECKs and the code.

[Sergey Ulanov, 2013/01/31 02:05:43]

Done.

+
+

[Wez, 2013/01/31 00:15:29]

nit: Spurious blank line.

[Sergey Ulanov, 2013/01/31 02:05:43]

Done.

+  if (waiting_frame_size_) {
+    frame_size_ = capture_data->size();
+    waiting_frame_size_ = false;
+
+    // Inform the EventHandler of the video frame dimensions and format.
+    VideoCaptureCapability caps;
+    caps.width = frame_size_.width();
+    caps.height = frame_size_.height();
+    caps.frame_rate = frame_rate_;
+    caps.color = VideoCaptureCapability::kARGB;
+    caps.expected_capture_delay =
+        base::Time::kMillisecondsPerSecond / frame_rate_;
+    caps.interlaced = false;
+
+    base::AutoLock auto_lock(event_handler_lock_);
+    if (event_handler_)
+      event_handler_->OnFrameInfo(caps);
+  }
+
+  if (!started_)
+    return;

[Wez, 2013/01/31 00:15:29]

Move this before |waiting_frame_size_| check?

[Sergey Ulanov, 2013/01/31 02:05:43]

It's in the right place. We want to call OnFrameInfo() even if Start() hasn't
been called.

+  size_t buffer_size = frame_size_.width() * frame_size_.height() *
+      ScreenCaptureData::kBytesPerPixel;
+
+  if (capture_data->size() == frame_size_) {
+    // If the captured frame matches the requested size, we don't need to
+    // resize it.
+    resized_bitmap_.reset();
+
+    base::AutoLock auto_lock(event_handler_lock_);
+    if (event_handler_) {
+      event_handler_->OnIncomingCapturedFrame(
+          capture_data->data(), buffer_size, base::Time::Now());
+    }
+    return;
+  }
+
+  // In case screen size has changed we need to resize the image. Resized image

[Wez, 2013/01/31 00:15:29]

Scaling to cope with changes in resolution after startup feels wrong (it'd be a
different story if we were supply a frame the size the caller wants, but we're
not).  How does the Tab Capture API support tab resize?

[Sergey Ulanov, 2013/01/31 02:05:43]

Tab capture scales the screen too:
https://code.google.com/p/chromium/source/search?q=CalculateFittedSize&origq=...

+  // is stored to |resized_bitmap_|. Only regions of the screen that are
+  // changing are copied.
+
+  SkRegion dirty_region = capture_data->dirty_region();
+
+  if (resized_bitmap_.width() != frame_size_.width() ||
+      resized_bitmap_.height() != frame_size_.height()) {
+    resized_bitmap_.setConfig(SkBitmap::kARGB_8888_Config,
+                              frame_size_.width(), frame_size_.height());
+    resized_bitmap_.setIsOpaque(true);
+    resized_bitmap_.allocPixels();
+    dirty_region.setRect(SkIRect::MakeSize(frame_size_));
+  }
+
+  float scale_x = static_cast<float>(frame_size_.width()) /
+      capture_data->size().width();
+  float scale_y = static_cast<float>(frame_size_.height()) /
+      capture_data->size().height();
+  float scale;
+  float x, y;
+  // Center the image in case aspect ratio is different.
+  if (scale_x > scale_y) {
+    scale = scale_y;
+    x = (scale_x - scale_y) / scale * frame_size_.width() / 2.0;
+    y = 0.f;
+  } else {
+    scale = scale_x;
+    x = 0.f;
+    y = (scale_y - scale_x) / scale * frame_size_.height() / 2.0;
+  }
+
+  // Create skia device and canvas that draw to |resized_bitmap_|.
+  SkDevice device(resized_bitmap_);
+  SkCanvas canvas(&device);
+  canvas.scale(scale, scale);
+
+  int source_stride = capture_data->stride();
+  for (SkRegion::Iterator i(dirty_region); !i.done(); i.next()) {
+    SkBitmap source_bitmap;
+    source_bitmap.setConfig(SkBitmap::kARGB_8888_Config,
+                            i.rect().width(), i.rect().height(),
+                            source_stride);
+    source_bitmap.setIsOpaque(true);
+    source_bitmap.setPixels(
+        capture_data->data() + i.rect().top() * source_stride +
+        i.rect().left() * ScreenCaptureData::kBytesPerPixel);
+    canvas.drawBitmap(source_bitmap, i.rect().left() + x / scale,
+                      i.rect().top() + y / scale, NULL);
+  }
+
+  base::AutoLock auto_lock(event_handler_lock_);
+  if (event_handler_) {
+    event_handler_->OnIncomingCapturedFrame(
+        reinterpret_cast<uint8*>(resized_bitmap_.getPixels()), buffer_size,
+        base::Time::Now());
+  }
+}
+
+void ScreenCaptureDevice::Core::OnCursorShapeChanged(
+    scoped_ptr<MouseCursorShape> cursor_shape) {
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());

[Wez, 2013/01/31 00:15:29]

Please add a TODO (and file a bug) for storing the cursor shape and merging it
into each captured frame - it's what remote-assistance-style apps will expect,
though we may need an option for the app to request capture without the cursor.

[Sergey Ulanov, 2013/01/31 02:05:43]

Filed crbug.com/173265

+}
+
+void ScreenCaptureDevice::Core::DoAllocate(int frame_rate) {
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
+
+  frame_rate_ = frame_rate;
+
+  // Create and start frame capturer.
+  if (!video_frame_capturer_)
+    video_frame_capturer_ = ScreenCapturer::Create();
+  if (video_frame_capturer_)
+    video_frame_capturer_->Start(this);

[Wez, 2013/01/31 00:15:29]

Why are we not Start()ing and Stop()ing in DoStart()/DoStop()?  Staying started
when the consumer isn't actually interested in the data may be expensive, so we
shouldn't do so unless it's necessary to provide the desired semantics.

[Sergey Ulanov, 2013/01/31 02:05:43]

because we need to capture first frame to get screen size and invoke
OnFrameInfo(). There is a comment about it two lines below.

[Wez, 2013/02/01 00:44:34]

So there is.  Can we post a delayed task to Stop() the underlying capturer if
the wrapper hasn't been Start()ed after, say, a second?

[Wez, 2013/02/01 00:44:34]

Good point!  Can we post a task here to Stop the underlying capturer after, say,
a second if the ScreenCaptureDevice isn't Start()ed by then, and then have
Start() and Stop() call through the underlying capturer?

[Sergey Ulanov, 2013/02/01 02:02:26]

We could, but it won't make any difference. Stop() doesn't really do anything.
Screen capturer don't actually do anything between Start() and Stop() unless
CaptureFrame() is called. We should just remove these methods from
ScreenCapturer

[Wez, 2013/02/02 00:19:56]

SGTM.

+
+  // Capture first frame, so that we can call OnFrameInfo() callback.
+  waiting_frame_size_ = true;
+  DoCapture();
+}
+
+void ScreenCaptureDevice::Core::DoStart() {
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
+  started_ = true;
+  if (!capture_task_posted_) {
+    capture_task_posted_ = true;
+    task_runner_->PostDelayedTask(
+        FROM_HERE, base::Bind(&Core::OnCaptureTimer, this),
+        base::TimeDelta::FromSeconds(1) / frame_rate_);

[Wez, 2013/01/31 00:15:29]

Why post a task here rather than triggering a capture immediately; posting just
means we have a delay before the first frame.

[Sergey Ulanov, 2013/01/31 02:05:43]

Done.

+  }
+}
+
+void ScreenCaptureDevice::Core::DoStop() {
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
+  started_ = false;
+  resized_bitmap_.reset();
+}
+
+void ScreenCaptureDevice::Core::DoDeAllocate() {
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
+  DoStop();
+  if (video_frame_capturer_) {
+    video_frame_capturer_->Stop();
+    video_frame_capturer_.reset();
+  }
+  waiting_frame_size_ = false;
+}
+
+void ScreenCaptureDevice::Core::OnCaptureTimer() {
+  DCHECK(capture_task_posted_);
+  capture_task_posted_ = false;
+
+  if (started_) {
+    // Schedule a task for the next frame.
+    capture_task_posted_ = true;
+    task_runner_->PostDelayedTask(
+        FROM_HERE, base::Bind(&Core::OnCaptureTimer, this),
+        base::TimeDelta::FromSeconds(1) / frame_rate_);
+  } else if (!waiting_frame_size_) {

[Wez, 2013/01/31 00:15:29]

If you always have the frame size populated by a synchronously-initiated capture
operation then you can't ever get into a state in which the timer goes off and
the frame size isn't yet populated, can you?

[Sergey Ulanov, 2013/01/31 02:05:43]

Done.

+    return;
+  }
+
+  DoCapture();
+}
+
+void ScreenCaptureDevice::Core::DoCapture() {
+  DCHECK(!capture_in_progress_);

[Wez, 2013/01/31 00:15:29]

DCHECK(started_ || waiting_frame_size_)?

[Sergey Ulanov, 2013/01/31 02:05:43]

I don't think it's necessary to check it here.

+
+  capture_in_progress_ = true;
+  video_frame_capturer_->CaptureFrame();
+
+  // Assume that ScreenCapturer always calls OnCaptureCompleted()
+  // callback before it returns.
+  //
+  // TODO(sergeyu): Fix ScreenCapturer to return video frame
+  // synchronously instead of using Delegate interface.
+  DCHECK(!capture_in_progress_);
+}
+
+ScreenCaptureDevice::ScreenCaptureDevice(
+    scoped_refptr<base::SequencedTaskRunner> task_runner)
+    : core_(new Core(task_runner)) {
+  name_.device_name = "Screen";
+}
+
+ScreenCaptureDevice::~ScreenCaptureDevice() {
+  DeAllocate();
+}
+
+void ScreenCaptureDevice::set_test_screen_capturer(
+  scoped_ptr<ScreenCapturer> capturer) {
+  core_->set_test_frame_capturer(capturer.Pass());
+}
+
+void ScreenCaptureDevice::Allocate(int width, int height,
+                              int frame_rate,
+                              EventHandler* event_handler) {
+  core_->Allocate(width, height, frame_rate, event_handler);
+}
+
+void ScreenCaptureDevice::Start() {
+  core_->Start();
+}
+
+void ScreenCaptureDevice::Stop() {
+  core_->Stop();
+}
+
+void ScreenCaptureDevice::DeAllocate() {
+  core_->DeAllocate();
+}
+
+const VideoCaptureDevice::Name& ScreenCaptureDevice::device_name() {
+  return name_;
+}
+
+}  // namespace media