[DesktopCapturer] FallbackDesktopCapturerWrapper and its tests

webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.cc

+/*
+ *  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.
+ */
+
+#include "webrtc/modules/desktop_capture/fallback_desktop_capturer_wrapper.h"
+
+#include <utility>
+
+#include "webrtc/base/checks.h"
+
+namespace webrtc {
+
+FallbackDesktopCapturerWrapper::FallbackDesktopCapturerWrapper(
+    std::unique_ptr<DesktopCapturer> main_capturer,
+    std::unique_ptr<DesktopCapturer> secondary_capturer)
+    : main_capturer_(std::move(main_capturer)),
+      secondary_capturer_(std::move(secondary_capturer)) {
+  RTC_DCHECK(main_capturer_);
+  RTC_DCHECK(secondary_capturer_);
+}
+
+FallbackDesktopCapturerWrapper::~FallbackDesktopCapturerWrapper() = default;
+
+void FallbackDesktopCapturerWrapper::Start(
+    DesktopCapturer::Callback* callback) {
+  main_capturer_->Start(this);
+  secondary_capturer_->Start(callback);
+  callback_ = callback;
+}
+
+void FallbackDesktopCapturerWrapper::SetSharedMemoryFactory(
+    std::unique_ptr<SharedMemoryFactory> shared_memory_factory) {
+  if (shared_memory_factory) {
+    shared_memory_factory_.reset(new SharedMemoryFactoryWrapper(
+        std::move(shared_memory_factory)));
+    main_capturer_->SetSharedMemoryFactory(shared_memory_factory_->Wrap());
+    secondary_capturer_->SetSharedMemoryFactory(shared_memory_factory_->Wrap());

[Sergey Ulanov, 2017/02/15 23:08:43]

There is a potential threading issue that comes with using the same factory for
multiple capturers. Capturers are allowed to use the factory on a separate
thread. See the comments for SetSharedMemoryFactory() in desktop_capturer.h . It
shouldn't be a problem for the current use-case as all capturers in WebRTC call
SharedMemoryFactory on the same thread on which Capture is called (it would be a
problem with remoting::DesktopCaptureProxy). But it's worth calling out this
limitation in the comments in the header. Also please add DCHECKs in
SharedMemoryFactoryWrapper() to verify it never used on multiple threads.

[Hzj_jie, 2017/02/16 01:57:07]

Done.

+  } else {
+    shared_memory_factory_.reset();
+    main_capturer_->SetSharedMemoryFactory(
+        std::unique_ptr<SharedMemoryFactory>());
+    secondary_capturer_->SetSharedMemoryFactory(
+        std::unique_ptr<SharedMemoryFactory>());
+  }
+}
+
+void FallbackDesktopCapturerWrapper::CaptureFrame() {
+  RTC_DCHECK(callback_);
+  if (main_capturer_permanent_error_) {
+    secondary_capturer_->CaptureFrame();
+  } else {
+    main_capturer_->CaptureFrame();
+  }
+}
+
+void FallbackDesktopCapturerWrapper::SetExcludedWindow(WindowId window) {
+  main_capturer_->SetExcludedWindow(window);
+  secondary_capturer_->SetExcludedWindow(window);
+}
+
+bool FallbackDesktopCapturerWrapper::GetSourceList(SourceList* sources) {
+  if (main_capturer_permanent_error_) {
+    return secondary_capturer_->GetSourceList(sources);
+  } else {

[Sergey Ulanov, 2017/02/15 23:08:43]

no else after return please.

[Hzj_jie, 2017/02/16 01:57:07]

Done.

+    return main_capturer_->GetSourceList(sources);
+  }
+}
+
+bool FallbackDesktopCapturerWrapper::SelectSource(SourceId id) {
+  if (main_capturer_permanent_error_) {
+    return secondary_capturer_->SelectSource(id);
+  }
+  return main_capturer_->SelectSource(id) &&
+      secondary_capturer_->SelectSource(id);
+}
+
+bool FallbackDesktopCapturerWrapper::FocusOnSelectedSource() {
+  if (main_capturer_permanent_error_) {
+    return secondary_capturer_->FocusOnSelectedSource();
+  }
+  return main_capturer_->FocusOnSelectedSource() &&
+      secondary_capturer_->FocusOnSelectedSource();

[Sergey Ulanov, 2017/02/15 23:08:43]

Do we need to call both capturers here?

[Hzj_jie, 2017/02/16 01:57:07]

I think it should be simpler to always forward the functions to the
secondary_capturer_. Since we cannot guarantee the impact of each functions, if
we do not call the secondary_capturer_, we may need to store all the function
calls, and replay them when we start the secondary_capturer_.

[Sergey Ulanov, 2017/02/16 19:14:17]

This makes sense fro SelectSource(), but not for FocusOnSelectedSource(). Unlike
SelectSource() FocusOnSelectedSource() shouldn't mutate capturer state, so
calling it twice makes no sense. If the concern is the second capturer failing
then this can be (main_capturer_->FocusOnSelectedSource() ||
secondary_capturer_->FocusOnSelectedSource()), so the second capturer would be
called only if the main one fails. Or just always using  secondary_capturer_
would work as well.

[Hzj_jie, 2017/02/16 21:31:28]

Oh, got you. I almost forgot the functionality of this function. I would suggest
to change this function into a const to indicate it won't impact the internal
state of a DesktopCapturer. I would do it in a separated change, what do you
think?

[Sergey Ulanov, 2017/02/17 20:16:23]

I don't think it needs to be const. E.g. an X11 capturer implementation may
initialize X11 connection lazily and making FocusOnSelectedSource() const would
prohibit such implementation.

[Hzj_jie, 2017/02/17 21:55:17]

Done.

+}
+
+void FallbackDesktopCapturerWrapper::OnCaptureResult(
+    Result result,
+    std::unique_ptr<DesktopFrame> frame) {
+  RTC_DCHECK(callback_);
+  if (result == Result::SUCCESS) {
+    callback_->OnCaptureResult(result, std::move(frame));
+    return;
+  }
+
+  if (result == Result::ERROR_PERMANENT) {

[Sergey Ulanov, 2017/02/15 23:08:43]

it's possible that OnCaptureResult() was called by the secondary capturer after
ERROR_TEMPORARY from the main one.

[Hzj_jie, 2017/02/16 01:57:07]

No, this class does not handle the OnCaptureResult() of the secondary_capturer_.
secondary_capturer_ always get the original callback. (line 33)

+    main_capturer_permanent_error_ = true;
+  }
+  secondary_capturer_->CaptureFrame();

[Sergey Ulanov, 2017/02/15 23:08:43]

this may loop indefinitely if secondary_capturer_ keeps returning
ERROR_TEMPORARY. I think we need to remember if the current Capture() call is
being handled by main or the secondary capturer. Alternatively you could use a
separate callback handler for each of the two capturers (which would require a
separate class to implement DesktopCapturer::Callback).

[Hzj_jie, 2017/02/16 01:57:07]

Same as the reason above.

+}
+
+}  // namespace webrtc