[Chromoting] Improve DirectX capturer to support multiple outputs

webrtc/modules/desktop_capture/win/dxgi_context.h

+/*
+ *  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 MODULES_DESKTOP_CAPTURE_WIN_DXGI_CONTEXT_H_
+#define MODULES_DESKTOP_CAPTURE_WIN_DXGI_CONTEXT_H_
+
+#include <vector>
+
+#include "webrtc/modules/desktop_capture/desktop_region.h"
+
+namespace webrtc {
+
+// A context to store the status of a single consumer of
+// DxgiDuplicatorContainer. Consumers should create an instance of DxgiContext
+// and keep it through their lifetime, and send it as parameter to
+// DxgiDuplicatorContainer public functions. For details, please refer to
+// comments of DxgiDuplicatorContainer.
+class DxgiContext {
+ public:
+  // Unregister this DxgiContext instance from all Dxgi duplicators during
+  // destructing.
+  ~DxgiContext();
+
+ private:
+  friend class DxgiOutputDuplicator;
+  friend class DxgiAdapterDuplicator;
+  friend class DxgiDuplicatorContainer;

[Sergey Ulanov, 2016/07/28 22:15:32]

I still don't think think hiding content of this class is useful. Why not make
it a struct? There is no way for any code outside of
webrtc/modules/desktop_capture to get a context instance anyway.

[Hzj_jie, 2016/07/29 02:12:23]

Not really, indeed a consumer need to create a context instance and keep it
during the lifetime of itself. Say, ScreenCapturerWinDirectx is a consumer of
DxgiDuplicatorContainer. But ScreenCapturerWinDirectx should not change any
internal state of a DxgiContext. I would prefer to decouple Dxgi duplicators
with ScreenCapturerWinDirectx. Due to the restriction of Windows API,
DxgiDuplicatorContainer::Instance() is the only instance which can access Dxgi
duplication APIs. So it is not really reasonable to merge Dxgi duplicators with
ScreenCapturerWinDirectx.

[Sergey Ulanov, 2016/07/30 00:49:22]

In either case this usage of friend goes against style guide:
https://google.github.io/styleguide/cppguide.html#Friends

Do you have any examples when Dxgi classes will be useful outside of
ScreenCapturerWinDirectx? If not then I don't think there is a good reason to
treat Dxgi* classes as a component separate from the screen capturer itself. And
if you really want to make Dxgi* classes to have an API consumable outside of
desktop_capture module, then I think the API should be simplified. E.g. you
could replace Context class with an interface called CaptureSource, which would
then contain Duplicate() method and talk to the duplicators. And then
DxgiDuplicatorContainer should be responsible for allocation of instances of
that interface. But I don't think that this extra complexity is necessary. 

[Hzj_jie, 2016/07/31 22:04:42]

What I can image is a screenshot function. But no matter, I will move
DxgiContext to DxgiDuplicatorController::Context.

+
+  // A DxgiContext will have an exactly same |identity_| as
+  // DxgiDuplicatorContainer, to ensure it has been correctly setted up after
+  // each DxgiDuplicatorContainer::Initialize().
+  int identity_ = 0;
+
+  // Child DxgiContext belongs to this DxgiContext.
+  std::vector<DxgiContext> contexts_;

[Sergey Ulanov, 2016/07/28 22:15:32]

I still think it's wrong (it's confusing and error-prone) to use the same
Context class for all levels of duplicators hierarchy. identity_ is only used on
top level while, updated_region_ is only used for the leaves.
You can replace it with the following structs:

struct OutputContext {
  DesktopRegion updated_region;
}

struct AdapterContext {
  std::vector<AdapterContext> outputs;
}

struct Context {
 int identity;
 std::vector<AdapterContext> adapters;
}

Or something simpler like this:

struct Context {
 int identity;
 std::vector<std::vector<DesktopRegion>> updated_regions;
}

WDYT?

[Hzj_jie, 2016/07/29 02:12:23]

I have no strong opinion on this. But I still prefer to keep their internal
states accessible only by corresponding Dxgi duplicator.

+
+  // The updated region DxgiOutputDuplicator::DetectUpdatedRegion() output
+  // during last Duplicate() function call. It's a DesktopRegion translated by
+  // offset of each DxgiOutputDuplicator instance.
+  DesktopRegion updated_region_;
+};
+
+}  // namespace webrtc
+
+#endif  // MODULES_DESKTOP_CAPTURE_WIN_DXGI_CONTEXT_H_