[WebRTC] Add ScreenCapturerDifferWrapper to share Differ across ScreenCapturers

webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h

Index: webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h
diff --git a/webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h b/webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h
new file mode 100644
index 0000000000000000000000000000000000000000..353f1515cb88c9eec5c5affb99bb510f16ca74e7
--- /dev/null
+++ b/webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h
@@ -0,0 +1,56 @@
+/*
+ *  Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#ifndef WEBRTC_MODULES_DESKTOP_CAPTURE_SCREEN_CAPTURER_DIFFER_WRAPPER_H_
+#define WEBRTC_MODULES_DESKTOP_CAPTURE_SCREEN_CAPTURER_DIFFER_WRAPPER_H_
+
+#include <memory>
+
+#include "webrtc/modules/desktop_capture/screen_capturer.h"
+#include "webrtc/modules/desktop_capture/shared_desktop_frame.h"
+
+namespace webrtc {
+
+class Differ;
+
+// A ScreenCapturer implementation to forward capture requests to the underlying
+// implementation, and help to calculate the dirty region.
+// This class expects the input ScreenCapturer uses SharedDesktopFrame to return
+// captured DesktopFrames.
+class ScreenCapturerDifferWrapper

[Jamie, 2016/08/03 23:52:26]

This class is not currently being used. Will there be a follow-up CL to delete
the diffing code from the platform-specific capturers and to use this wrapper
instead?

[Hzj_jie, 2016/08/04 02:28:07]

Exactly. Bug 633802 is opened to track all the works.

+    : public ScreenCapturer, public DesktopCapturer::Callback {

[Sergey Ulanov, 2016/08/03 23:19:11]

Potentially the same mechanism could work for window capturer as well. Currently
WindowCapturer and ScreenCapturer are different interfaces, but potentially we
remove them and move everything back to DesktopCapturer. The only difference is
GetScreenList() vs GetWindowList(). We could just add GetSourceList() and
SelectSource() in DesktopCapturer and then remove WindowCapturer and
ScreenCapturer. Let me know if you want to take on this task and we can discuss
you logistics of how it can be done without breaking chromium build.

[Sergey Ulanov, 2016/08/03 23:19:11]

git cl format

btw, you can get clang-format integrated in your text editor, depending on the
editor you use, see
https://chromium.googlesource.com/chromium/src/+/master/docs/clang_format.md .
Saves a lot time for me.

[Hzj_jie, 2016/08/04 02:28:07]

Done.

[Hzj_jie, 2016/08/04 02:28:07]

Sure. We can discuss it later.

[Hzj_jie, 2016/08/04 03:28:43]

And I also found,
1. Nobody, both ScreenCapturer and WindowCapturer, uses the input (const
DesktopRegion& region) parameter in Capture function.

2. The region parameter can be replaced by a DesktopRect.

3. Though on Linux and Mac, there are different implementations for
WindowCapturer and ScreenCapturer, indeed a WindowCapturer can be backed by a
ScreenCapturer. i.e. Capture the right screen, and select part of it. Or capture
a right rectangle.

So I thought we can have an set of DesktopCapturer implementations, with only
functions of current DesktopCapturer interface. (Only replace DesktopRegion to
DesktopRect) All the differ / extend to grid / directx / gdi / damage
notification / shared memory / double buffering, etc, should belong to
DesktopCapturer(s).

And ScreenCapturer(s) / WindowCapturer(s) just need to send a correct rectangle
to the DesktopCapturer(s) to capture a single window or screen. Then we can
eventually remove duplicate logic from ScreenCapturer(s) and WindowCapturer(s).

I do not see a potential performance loss on Linux and Windows by using a
ScreenCapturer to capture a single window. I cannot quite sure about Mac, though
it seems fine.

+ public:
+  // Creates a ScreenCapturerDifferWrapper with a ScreenCapturer implementation,
+  // and takes ownership.
+  explicit ScreenCapturerDifferWrapper(std::unique_ptr<ScreenCapturer> impl);

[Sergey Ulanov, 2016/08/03 23:19:12]

|impl| is not the best name. maybe call it base_capturer?

[Hzj_jie, 2016/08/04 02:28:07]

Done.

+  ~ScreenCapturerDifferWrapper() override;
+
+  void Start(DesktopCapturer::Callback* callback) override;

[Sergey Ulanov, 2016/08/03 23:19:11]

// ScreenCapturer interface.

[Hzj_jie, 2016/08/04 02:28:07]

Done.

+  void SetSharedMemoryFactory(
+      std::unique_ptr<SharedMemoryFactory> shared_memory_factory) override;
+  void Capture(const DesktopRegion& region) override;
+  bool GetScreenList(ScreenList* screens) override;
+  bool SelectScreen(ScreenId id) override;
+
+  // Catches callback from underlying implementation, uses Differ to update

[Sergey Ulanov, 2016/08/03 23:19:11]

I don't think you really need this comment here. It's obvious why you need to
implement DesktopCapturer::Callback interface.

[Hzj_jie, 2016/08/04 02:28:07]

Done.

+  // |frame|->updated_region(), and calls callback_.

[Sergey Ulanov, 2016/08/03 23:19:12]

Add
  // DesktopCapturer::Callback interface.

move this to the private section, to make it clear that this method is not
expected to be called directly.

[Hzj_jie, 2016/08/04 02:28:07]

Done.

+  void OnCaptureResult(Result result,
+                       std::unique_ptr<DesktopFrame> frame) override;
+
+ private:
+  std::unique_ptr<ScreenCapturer> impl_;

[Sergey Ulanov, 2016/08/03 23:19:11]

base_capturer_?

[Hzj_jie, 2016/08/04 02:28:07]

Done.

+  DesktopCapturer::Callback* callback_;
+  std::unique_ptr<SharedDesktopFrame> last_frame_;
+  std::unique_ptr<Differ> differ_;
+};
+
+}  // namespace webrtc
+
+#endif  // WEBRTC_MODULES_DESKTOP_CAPTURE_SCREEN_CAPTURER_DIFFER_WRAPPER_H_