Remote Assistance on Chrome OS Part I - Aura Desktop Capturer

remoting/host/chromeos/aura_desktop_capturer.cc

Index: remoting/host/chromeos/aura_desktop_capturer.cc
diff --git a/remoting/host/chromeos/aura_desktop_capturer.cc b/remoting/host/chromeos/aura_desktop_capturer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..6bf6a048db291d4653df8e9636b6dd596ae4a39b
--- /dev/null
+++ b/remoting/host/chromeos/aura_desktop_capturer.cc
@@ -0,0 +1,105 @@
+// Copyright 2014 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/chromeos/aura_desktop_capturer.h"
+
+#include "ash/shell.h"
+#include "base/bind.h"
+#include "cc/output/copy_output_request.h"
+#include "cc/output/copy_output_result.h"
+#include "third_party/skia/include/core/SkBitmap.h"
+#include "third_party/webrtc/modules/desktop_capture/desktop_frame.h"
+#include "ui/aura/window_tree_host.h"
+
+namespace remoting {
+
+// DesktopFrame implementation used by screen capture on ChromeOS.
+// Frame data is stored in a SkBitmap.
+class SkiaBitmapDestkopFrame : public webrtc::DesktopFrame {

[Wez, 2014/09/10 01:41:10]

typo: Destkop->Desktop

[Wez, 2014/09/10 01:41:10]

This class, and the implementations of its methods, should be in an anonymous
namespace.

[kelvinp, 2014/09/10 22:33:58]

Done.

[kelvinp, 2014/09/10 22:33:58]

Done.

+ public:
+  virtual ~SkiaBitmapDestkopFrame();
+  static SkiaBitmapDestkopFrame* Create(SkBitmap* bitmap);

[Wez, 2014/09/10 01:41:10]

nit: static methods should appear before ctor/dtor.

[Wez, 2014/09/10 01:41:10]

scoped_ptr<SkBitmap>, not SkBitmap*

[kelvinp, 2014/09/10 22:33:58]

Done.

[kelvinp, 2014/09/10 22:33:58]

Done.

+
+ private:
+  SkiaBitmapDestkopFrame(webrtc::DesktopSize size,
+                         int stride,
+                         uint8_t* data,
+                         SkBitmap* bitmap);
+
+  scoped_ptr<SkBitmap> bitmap_;
+
+  DISALLOW_COPY_AND_ASSIGN(SkiaBitmapDestkopFrame);
+};
+
+AuraDesktopCapturer::AuraDesktopCapturer()
+    : callback_(NULL), desktop_window_(NULL), weak_factory_(this) {
+}
+
+AuraDesktopCapturer::~AuraDesktopCapturer() {
+}
+
+void AuraDesktopCapturer::Start(webrtc::DesktopCapturer::Callback* callback) {
+  callback_ = callback;
+}
+
+void AuraDesktopCapturer::Capture(const webrtc::DesktopRegion&) {
+  scoped_ptr<cc::CopyOutputRequest> request =
+      cc::CopyOutputRequest::CreateBitmapRequest(base::Bind(
+          &AuraDesktopCapturer::OnFrameCaptured, weak_factory_.GetWeakPtr()));

[Wez, 2014/09/10 01:41:10]

nit: I think this'll read better if you wrap before base::Bind and then again
before weak_factory, even though it adds an extra line.

[kelvinp, 2014/09/10 22:33:58]

Done.

+
+  int width = desktop_window_->bounds().width();
+  int height = desktop_window_->bounds().height();
+  gfx::Rect window_rect = gfx::Rect(width, height);
+
+  request->set_area(window_rect);
+  DCHECK(desktop_window_);

[Wez, 2014/09/10 01:41:10]

No need to DCHECK this here - you're about to deref it, which will go boom
anyway if it's NULL. You should DCHECK it at the point where it is set.

[kelvinp, 2014/09/10 22:33:58]

Done.

+  desktop_window_->layer()->RequestCopyOfOutput(request.Pass());
+}
+
+void AuraDesktopCapturer::OnFrameCaptured(
+    scoped_ptr<cc::CopyOutputResult> result) {
+  DCHECK(result->HasBitmap());

[Wez, 2014/09/10 01:41:10]

Is there a situation in which it might not have a bitmap, in a real system?

If so then consider handling that case by returning a NULL frame to the caller.

[kelvinp, 2014/09/10 22:33:58]

No, that should never happen.

+
+  scoped_ptr<SkBitmap> bitmap = result->TakeBitmap();
+
+  scoped_ptr<webrtc::DesktopFrame> frame(
+      SkiaBitmapDestkopFrame::Create(bitmap.release()));
+
+  callback_->OnCaptureCompleted(frame.release());
+}
+
+SkiaBitmapDestkopFrame::SkiaBitmapDestkopFrame(webrtc::DesktopSize size,
+                                               int stride,
+                                               uint8_t* data,
+                                               SkBitmap* bitmap)
+    : DesktopFrame(size, stride, data, NULL), bitmap_(bitmap) {
+}
+
+SkiaBitmapDestkopFrame::~SkiaBitmapDestkopFrame() {
+}
+
+// static
+SkiaBitmapDestkopFrame* SkiaBitmapDestkopFrame::Create(SkBitmap* bitmap) {
+  DCHECK(bitmap);
+
+  int width = bitmap->width();
+  int height = bitmap->height();
+
+  webrtc::DesktopSize size(width, height);
+
+  DCHECK(bitmap->bytesPerPixel() == 4)

[Wez, 2014/09/10 01:41:10]

DCHECK_EQ(4, bitmap->bytesPerPixel())

Is there a way that this might not be true in real systems? If so then we should
error out instead of DCHECKing; what will the rest of the function do if
bytesPerPixel != 4?

[kelvinp, 2014/09/10 22:33:58]

No, the current implementation of both the gl renderer and the software renderer
has a 32-bit pixel format

+      << "DesktopFrame objects always hold RGBA data.";
+
+  uint8_t* bitmap_data = reinterpret_cast<uint8_t*>(bitmap->getPixels());
+
+  SkiaBitmapDestkopFrame* result =
+      new SkiaBitmapDestkopFrame(size, bitmap->rowBytes(), bitmap_data, bitmap);
+
+  const webrtc::DesktopRect& rect = webrtc::DesktopRect::MakeWH(width, height);
+  result->mutable_updated_region()->SetRect(rect);

[Wez, 2014/09/10 01:41:10]

nit: This makes more logical sense to do in OnFrameCaptured, immediately before
returning the frame.

[kelvinp, 2014/09/10 22:33:58]

Done.

+
+  return result;
+}
+
+}  // namespace remoting