[Chromoting] Use latest API to check whether DX capturer is supported

remoting/host/host_attributes.cc

 // Copyright 2016 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/host_attributes.h"
 
 #if defined(OS_WIN)
 #include <D3DCommon.h>
 #endif
 
 #include <type_traits>
 
 #include "base/atomicops.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/win/windows_version.h"
 #include "build/build_config.h"
 
 #if defined(OS_WIN)
 #include "third_party/webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h"
 #include "third_party/webrtc/modules/desktop_capture/win/screen_capturer_win_directx.h"
 #endif
 
 namespace remoting {
 
 namespace {
+
 static constexpr char kSeparator[] = ",";
 
 struct Attribute {
   const char* name;
   bool(* get_value_func)();
 };
 
+void AppendAttribute(std::string* result, const char* attribute) {

[Do not use (sergeyu), 2017/06/27 05:54:44]

It would be cleaner to collect all attributes into a vector<StringPiece> and
then join them together using base::JoinString(). 

[Hzj_jie, 2017/06/27 23:03:06]

Done.

+  DCHECK_EQ(std::string(attribute).find(kSeparator), std::string::npos);
+  if (!result->empty()) {
+    result->append(kSeparator);
+  }
+  result->append(attribute);
+}
+
 inline constexpr bool IsDebug() {
 #if defined(NDEBUG)
   return false;
 #else
   return true;
 #endif
 }
 
 inline constexpr bool IsChromeBranded() {
 #if defined(GOOGLE_CHROME_BUILD)
@@ skipping 15 matching lines @@
 #else
   return false;
 #endif
 }
 
 inline constexpr bool IsNonOfficialBuild() {
   return !IsOfficialBuild();
 }
 
 #if defined(OS_WIN)
-inline bool MinD3DFeatureLevelGreatThan10() {
-  webrtc::DxgiDuplicatorController::D3dInfo info;
-  if (webrtc::DxgiDuplicatorController::Instance()->RetrieveD3dInfo(&info)) {
-    return info.min_feature_level >= D3D_FEATURE_LEVEL_10_0;
+// TODO(zijiehe): Use ScreenCapturerWinDirectx::IsSessionUnsupported().
+bool IsRunningInSession0() {
+  DWORD session_id = 0;
+  if (!::ProcessIdToSessionId(::GetCurrentProcessId(), &session_id)) {
+    LOG(WARNING) << "Failed to retrieve current session id.";

[joedow, 2017/06/27 15:44:58]

If you use PLOG here, the platform error code will also be logged.

[Hzj_jie, 2017/06/27 23:03:06]

Done.

+    // Assume the binary is running in session 0 if the session id cannot be
+    // retrieved, so IsDirectxCapturerSupported() will fallback to use Windows
+    // version.
+    return true;
   }
-  return false;
+  return session_id == 0;
 }
 
-inline bool MinD3DFeatureLevelGreatThan11() {
-  webrtc::DxgiDuplicatorController::D3dInfo info;
-  if (webrtc::DxgiDuplicatorController::Instance()->RetrieveD3dInfo(&info)) {
-    return info.min_feature_level >= D3D_FEATURE_LEVEL_11_0;
+bool IsDirectxCapturerSupported() {
+  if (webrtc::ScreenCapturerWinDirectx::IsSupported()) {

[Do not use (sergeyu), 2017/06/27 05:54:44]

Is there anything that we get by calling ScreenCapturerWinDirectx::IsSupported()
here? Would it be simpler to just always use windows version? otherwise the
result of GetHostAttributes() is not consistent across the host processes.

[Hzj_jie, 2017/06/27 23:03:06]

I believe this used to work well before some upgrade of my Windows 10 machine.
Or maybe we can return Windows version in the HostAttributes to avoid the
confusion: DirectX capturer will fallback to GDI capturer if the initialization
failed anyway. So if we cannot rely on the
ScreenCapturerWinDirectx::IsSupported() in session 0 anymore, using Windows
version does not really introduce new biases.

+    return true;
   }
-  return false;
-}
 
-inline bool MinD3DFeatureLevelGreatThan12() {
-  webrtc::DxgiDuplicatorController::D3dInfo info;
-  if (webrtc::DxgiDuplicatorController::Instance()->RetrieveD3dInfo(&info)) {
-    return info.min_feature_level >= D3D_FEATURE_LEVEL_12_0;
+  if (IsRunningInSession0()) {
+    LOG(WARNING) << "Current binary is running in session 0. DXGI components "
+                    "cannot be initialized. Fallback to use Windows version to "
+                    "decide the support of DirectX capturer. It may be "
+                    "inaccurate.";

[joedow, 2017/06/27 15:44:58]

IIUC this method is going to be called in session 0 every time a connection is
made (since the host will need to query it to send its DX support info to the
client).  If so, then this warning isn't very useful.  We can downgrade it to a
VLOG or LOG(INFO) as nothing is going wrong, it is the expected behavior for
session 0.

[Hzj_jie, 2017/06/27 23:03:06]

Done.

+    return base::win::GetVersion() >= base::win::VERSION_WIN8;
   }
+
   return false;
 }
 #endif
 
 // By using arraysize() macro in base/macros.h, it's illegal to have empty
 // arrays.
 //
 // error: no matching function for call to 'ArraySizeHelper'
 // note: candidate template ignored: substitution failure
 // [with T = const remoting::StaticAttribute, N = 0]:
 // zero-length arrays are not permitted in C++.
 //
 // So we need IsDebug() function, and "Debug-Build" Attribute.
 
 static constexpr Attribute kAttributes[] = {
   { "Debug-Build", &IsDebug },
   { "ChromeBrand", &IsChromeBranded },
   { "ChromiumBrand", &IsChromiumBranded },
   { "OfficialBuild", &IsOfficialBuild },
   { "NonOfficialBuild", &IsNonOfficialBuild },
 #if defined(OS_WIN)
-  { "DirectX-Capturer", &webrtc::ScreenCapturerWinDirectx::IsSupported },
-  { "MinD3DGT10", &MinD3DFeatureLevelGreatThan10 },
-  { "MinD3DGT11", &MinD3DFeatureLevelGreatThan11 },
-  { "MinD3DGT12", &MinD3DFeatureLevelGreatThan12 },
+  { "DirectX-Capturer", &IsDirectxCapturerSupported },
 #endif
 };
 
 }  // namespace
 
 static_assert(std::is_pod<Attribute>::value, "Attribute should be POD.");
 
 std::string GetHostAttributes() {
+#if defined(OS_WIN)
+  // Ensure the following ScreenCapturerWinDirectx contructions won't

[Do not use (sergeyu), 2017/06/27 05:54:44]

This is still confusing. Remove IsDirectxCapturerSupported() and handle DX
support same as DX version attributes? that would allow to have only one ifdef
for Windows in this function.

[Hzj_jie, 2017/06/27 23:03:06]

In that case, I would prefer to return Windows version instead of
DirectX-Capturer to avoid any confusing.
Windows 8 requires only video cards that support DirectX 9, but DirectX capturer
requires DirectX 11. So using Windows version is inaccurate anyway: naming the
attribute as DirectX-Capturer is improper here.

+  // initialize and unload DxgiDuplicatorController.
+  auto controller = webrtc::DxgiDuplicatorController::Instance();
+#endif
   std::string result;
   // By using ranged for-loop, MSVC throws error C3316:
   // 'const remoting::StaticAttribute [0]':
   // an array of unknown size cannot be used in a range-based for statement.
   for (size_t i = 0; i < arraysize(kAttributes); i++) {
     const auto& attribute = kAttributes[i];
     DCHECK_EQ(std::string(attribute.name).find(kSeparator), std::string::npos);
     if (attribute.get_value_func()) {
-      if (!result.empty()) {
-        result.append(kSeparator);
-      }
-      result.append(attribute.name);
+      AppendAttribute(&result, attribute.name);
     }
   }
+#if defined(OS_WIN)
+  {
+    webrtc::DxgiDuplicatorController::D3dInfo info;
+    webrtc::ScreenCapturerWinDirectx::RetrieveD3dInfo(&info);
+    if (info.min_feature_level >= D3D_FEATURE_LEVEL_10_0) {
+      AppendAttribute(&result, "MinD3DGT10");
+    }
+    if (info.min_feature_level >= D3D_FEATURE_LEVEL_11_0) {
+      AppendAttribute(&result, "MinD3DGT11");
+    }
+    if (info.min_feature_level >= D3D_FEATURE_LEVEL_12_0) {
+      AppendAttribute(&result, "MinD3DGT12");
+    }
+  }
+  // Avoid the warning of unused variable.
+  controller = nullptr;
+#endif
 
   return result;
 }
 
 }  // namespace remoting