feat: Implementation of getGPUInfo API. #13486

nitsakh · 2018-06-28T19:31:45Z

This PR implements the app.getGPUInfo api to get the GPU details available on chrome://gpu.
This can be useful to clients for various purposes like working around bugs based on available details.

Chromium GPUInfo class provides an interface for an enumerator. Creating a custom enumerator and calling EnumerateFields on the GPUInfo class, will get us all the attributes in the class. Using this enumerator also saves us the maintenance burden when attributes are added/changed with newer versions of chromium.

I haven't updated the docs or added tests yet, will do that once I get some feedback on the API.

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
relevant documentation is changed or added
commit messages or PR title follow semantic commit guidelines

Notes: Implemented new getGPUInfo API to get details about the GPU.

nitsakh · 2018-06-28T19:49:29Z

This is the example response of the API.

{ auxAttributes:
   { amdSwitchable: true,
     canSupportThreadedTextureMailbox: false,
     directComposition: false,
     directRendering: true,
     driverDate: '',
     driverVendor: '',
     driverVersion: '',
     glExtensions: '',
     glRenderer: '',
     glResetNotificationStrategy: 0,
     glVendor: '',
     glVersion: '',
     glWsExtensions: '',
     glWsVendor: '',
     glWsVersion: '',
     inProcessGpu: false,
     initializationTime: 218,
     jpegDecodeAcceleratorSupported: false,
     maxMsaaSamples: '',
     optimus: false,
     passthroughCmdDecoder: false,
     pixelShaderVersion: '',
     sandboxed: false,
     softwareRendering: false,
     supportsOverlays: false,
     vertexShaderVersion: '',
     videoDecodeAcceleratorFlags: 0,
     videoDecodeAcceleratorSupportedProfile:
      { encrypted_only: false,
        maxResolutionHeight: 2160,
        maxResolutionWidth: 4096,
        minResolutionHeight: 16,
        minResolutionWidth: 16,
        profile: 3 },
     videoEncodeAcceleratorSupportedProfile:
      { maxFramerateDenominator: 1,
        maxFramerateNumerator: 30,
        maxResolutionHeight: 2160,
        maxResolutionWidth: 4096,
        profile: 3 } },
  gpuDevice:
   [ { active: true,
       deviceId: 26657,
       deviceString: '',
       vendorId: 4098,
       vendorString: '' },
     { active: false,
       deviceId: 3366,
       deviceString: '',
       vendorId: 32902,
       vendorString: '' } ],
  machineModelName: 'MacBookPro',
  machineModelVersion: '11.5' }

nitsakh · 2018-06-28T19:50:06Z

~~Also, I should change the code to omit attributes with empty strings.~~ Done.

felixrieseberg · 2018-06-28T19:52:02Z

You might want to take out the package_lock.json changes unless they were on purpose!

8000

miniak · 2018-06-29T23:25:43Z

atom/browser/api/atom_api_app.cc

@@ -1162,6 +1163,14 @@ v8::Local<v8::Value> App::GetGPUFeatureStatus(v8::Isolate* isolate) {
  return mate::ConvertToV8(isolate, status ? *status : temp);
 }

+v8::Local<v8::Value> App::GetGPUInfo(v8::Isolate* isolate) {
+  const auto& gpu_info =


you may need to call content::GpuDataManagerImpl::GetInstance()->RequestCompleteGpuInfoIfNeeded() to make the information available

Thanks Milan! I'll try that and will update the PR. 👍

That worked. Was able to fetch all the info with that.

YurySolovyov · 2018-07-06T12:49:37Z

This API looks a bit awkward. How about making it

Return Promise
Parametrize the kind of info you want to get:

const info = await app.getGPUInfo('full');
// or
const info = await app.getGPUInfo('short');
// potentially use 'short' as default so above is the same as:
const info = await app.getGPUInfo();

zeke

A few typos, but otherwise the docs look good.

zeke · 2018-07-06T20:32:03Z

docs/api/app.md

@@ -904,6 +904,15 @@ Returns [`ProcessMetric[]`](structures/process-metric.md): Array of `ProcessMetr

 Returns [`GPUFeatureStatus`](structures/gpu-feature-status.md) - The Graphics Feature Status from `chrome://gpu/`.

+### `app.getGPUInfo([callback])`
+
+Returns `Object` - GPU Information available in chromium's GPUInfo object. It's contains the version and driver information that's shown on `chrome://gpu` page.


Typo: It's contains

zeke · 2018-07-06T20:33:28Z

docs/api/app.md

+* `callback` Function (optional)
+  * `gpuInfo` Object - List of all attributes and values available in [chromium's GPUInfo object](https://chromium.googlesource.com/chromium/src/gpu/+/master/config/gpu_info.cc).
+
+Providing the optional callback will fetch the complete information available from the GPU process. In this case, the callback object will have all the available information, which can be more that what the returned object had.


Type: more that should be more than.

Can you also explain why the callback function will yield more results than the returned object?

Thanks @zeke! I'll update these with the other changes. 👍

nitsakh · 2018-07-09T17:23:47Z

Thanks @YurySolovyov . I like your idea. However, the main idea here was to get the short information synchronously, so that apps can immediately act upon it. It most likely would be used at startup, so getting the minimal information synchronously is better.

YurySolovyov · 2018-07-09T19:45:05Z

I would argue that consistency is better. Besides, promises work well with async/await so that it can be used even at startup with almost the same ergonomics

8000

MarshallOfSound · 2018-07-09T23:12:06Z

docs/api/app.md

@@ -904,6 +904,15 @@ Returns [`ProcessMetric[]`](structures/process-metric.md): Array of `ProcessMetr

 Returns [`GPUFeatureStatus`](structures/gpu-feature-status.md) - The Graphics Feature Status from `chrome://gpu/`.

+### `app.getGPUInfo([callback])`
+
+Returns `Object` - GPU Information available in chromium's GPUInfo object. It's contains the version and driver information that's shown on `chrome://gpu` page.


Returning an object and accepting a callback 😕

Would it be better to callback with everything, it seems really weird for a single API to be both synchronous and asynchronous

Yeah, it definitely is hacky. I'm leaning towards the promise way now. Will make changes and update the branch. Thanks @MarshallOfSound and @YurySolovyov !

alexeykuzmin · 2018-07-11T23:20:40Z

docs/api/app.md

+Returns `Object` - GPU Information available in chromium's GPUInfo object. It's contains the version and driver information that's shown on `chrome://gpu` page.
+
+* `callback` Function (optional)
+  * `gpuInfo` Object - List of all attributes and values available in [chromium's GPUInfo object](https://chromium.googlesource.com/chromium/src/gpu/+/master/config/gpu_info.cc).


Can you please use a link to a Chromium release tag, not to a master?
That file might get renamed or removed.

Yes, I thought of doing that. We don't do that in other places in the docs though. Also, we should add another item on the release checklist to update the docs.

alexeykuzmin · 2018-07-11T23:24:23Z

spec/api-app-spec.js

+      const activeDevice = gpuInfo.gpuDevice.find((device) => {
+        return device.active === true
+      })
+      expect(activeDevice).to.not.be.null()


Array#find() returns undefined if nothing is found.
Can you please check that activeDevice is an object instead of !== null?:

expect(activeDevice).to.be.an('object')

alexeykuzmin · 2018-07-11T23:27:30Z

spec/api-app-spec.js

+        return device.active === true
+      })
+      expect(activeDevice).to.not.be.null()
+      expect(activeDevice.deviceId).to.be.a('number').not.lessThan(0)


Maybe it would be better to check for deviceId property existence first.
It will produce a failure message better than expected undefined to be a number.

expect(activeDevice).to.have.property('deviceId').that.is.a('number').not.lessThan(0)

YurySolovyov · 2018-07-12T18:53:18Z

docs/api/app.md

@@ -904,6 +904,14 @@ Returns [`ProcessMetric[]`](structures/process-metric.md): Array of `ProcessMetr

 Returns [`GPUFeatureStatus`](structures/gpu-feature-status.md) - The Graphics Feature Status from `chrome://gpu/`.

+### `app.getGPUInfo(info_type)`
+
+* `info_type` String. Values can be either 'available' for currently available info or `complete` for complete info.


Docs are not consistent with tests: test use full/short and docs say complete/available

Camel case info_type -> infoType or InfoType ?

Yeah, still working on this PR. I have those changes made locally.

MarshallOfSound · 2018-07-13T03:05:43Z

atom/browser/api/atom_api_app.cc

@@ -1162,6 +1195,30 @@ v8::Local<v8::Value> App::GetGPUFeatureStatus(v8::Isolate* isolate) {
  return mate::ConvertToV8(isolate, status ? *status : temp);
 }

+util::Promise* App::GetGPUInfo(const std::string& info_type) {
+  CHECK(!is_fetching_gpuinfo_);


This is scary

Can't we just return gpinfo_promise_ instead here 😄

That's a good idea.

MarshallOfSound · 2018-07-13T03:07:28Z

atom/browser/api/atom_api_app.h

@@ -228,6 +232,10 @@ class App : public AtomBrowserClient::Delegate,

  base::FilePath app_path_;

+  util::Promise* gpuinfo_promise_;


Can we explicitly initialize this to nullptr, if people do a truthy check on this in the future it wouldn't work without explicit initialization.

I'm doing that in the constructor.

MarshallOfSound · 2018-07-13T03:07:46Z

atom/browser/api/atom_api_app.h

@@ -228,6 +232,10 @@ class App : public AtomBrowserClient::Delegate,

  base::FilePath app_path_;

+  util::Promise* gpuinfo_promise_;
+  bool has_complete_gpu_info_;
+  bool is_fetching_gpuinfo_;


A truthy check on gpuinfo_promise_ could replace this variable right?

~~Not exactly. Also actually I'm still working on the changes.~~

Yes, if we return the same promise, then we don't need the variable.

please initialize the values here, we are using C++11 class member variable initialization:
#13030

miniak · 2018-07-13T06:43:06Z

atom/browser/api/atom_api_app.h

@@ -228,6 +232,10 @@ class App : public AtomBrowserClient::Delegate,

  base::FilePath app_path_;

+  util::Promise* gpuinfo_promise_;
+  bool has_complete_gpu_info_;
+  bool is_fetching_gpuinfo_;


please initialize the values here, we are using C++11 class member variable initialization:
#13030

nitsakh · 2018-07-13T09:02:07Z

atom/browser/api/atom_api_app.cc

+  if (info_type == "complete") {
+    // In this case, we create an object on the heap and GPUInfoManager is
+    // responsible for deleting it
+    GPUInfoManager* info_mgr = new GPUInfoManager(isolate);


Is there something like enable_shared_from_this for chromium, so that I do not have to do this memory management dance?

The GPUInfoManager can be a base::Singleton<>, which takes ownership of the util::Promise. This also allows the class to be moved outside the api layer.

deepak1556 · 2018-07-13T16:56:20Z

atom/browser/api/atom_api_app.cc

+    return promise;
+  }
+  const auto gpu_data_manager = content::GpuDataManagerImpl::GetInstance();
+  if (!gpu_data_manager->GpuAccessAllowed(nullptr)) {


This check can be merged with the previous one,

if (info_type != "..." || !gpu_data_manager->GpuAccessAllowed(nullptr))

deepak1556 · 2018-07-13T17:15:37Z

atom/browser/api/atom_api_app.cc

@@ -1162,6 +1164,33 @@ v8::Local<v8::Value> App::GetGPUFeatureStatus(v8::Isolate* isolate) {
  return mate::ConvertToV8(isolate, status ? *status : temp);
 }

+util::Promise* App::GetGPUInfo(v8::Isolate* isolate,
+                               const std::string& info_type) {
+  if (info_type != "available" && info_type != "complete") {


Can you rename available to basic , the former seems too generic. Also the result from CollectBasicGraphicsInfo seems fixed, it would be best to list the data from it in the docs explicitly, that way users would have better idea to choose among the options.

deepak1556 · 2018-07-13T17:28:48Z

atom/browser/api/gpuinfo_manager.cc

+  if (NeedsCompleteGpuInfoCollection()) {
+    gpu_data_manager_->RequestCompleteGpuInfoIfNeeded();
+  } else {
+    content::BrowserThread::PostTask(


Was the idea here to delay the call till next tick ? If so, use the thread task runner and post tasks with it.

deepak1556 · 2018-07-13T17:38:24Z

atom/browser/api/atom_api_app.cc

+  if (info_type == "complete") {
+    // In this case, we create an object on the heap and GPUInfoManager is
+    // responsible for deleting it
+    GPUInfoManager* info_mgr = new GPUInfoManager(isolate);


The GPUInfoManager can be a base::Singleton<>, which takes ownership of the util::Promise. This also allows the class to be moved outside the api layer.

nitsakh · 2018-07-17T18:57:02Z

@zeke @miniak @deepak1556 @YurySolovyov Please let me know how you feel about this change now?

YurySolovyov

API shape looks good to me. Haven't studied the implementation.

atom/browser/api/gpuinfo_manager.cc

MarshallOfSound · 2018-07-18T01:57:30Z

atom/browser/api/atom_api_app.cc

+  const auto& promise = new util::Promise(isolate);
+  if ((info_type != "basic" && info_type != "complete") ||
+      !gpu_data_manager->GpuAccessAllowed(nullptr)) {
+    promise->Reject();


Can we reject with a helpful error message here?

MarshallOfSound · 2018-07-18T02:00:24Z

docs/api/app.md

+
+For `infoType` equal to `basic`:
+  Promise is fulfilled with `Object` containing fewer attributes than when requested with `complete`. Here's an example of basic response:
+```json


Change this to js to make it highlighted correctly

codebytere · 2018-08-24T15:36:06Z

What's the current status of this PR?

…to api-gpuinfo

deepak1556

LGTM, Thanks!

docs are looking good now

release-clerk · 2018-09-27T14:59:27Z

Release Notes Persisted

Implemented new getGPUInfo API to get details about the GPU.

* Implementation of getGPUInfo API. * Clear promise set * Changes to promise usage * Minor fixes * Fix linux build * Update spec * Fix lint (linter didn't run on windows locally) * Test running single test for CI * Update spec

nitsakh requested a review from a team June 28, 2018 19:31

nitsakh force-pushed the api-gpuinfo branch from df05249 to 54be9b2 Compare June 28, 2018 19:36

miniak reviewed Jun 29, 2018

View reviewed changes

nitsakh requested a review from a team July 3, 2018 18:37

nitsakh force-pushed the api-gpuinfo branch from eb29aaf to 5ac16ba Compare July 5, 2018 18:35

zeke previously requested changes Jul 6, 2018

View reviewed changes

MarshallOfSound reviewed Jul 9, 2018

View reviewed changes

alexeykuzmin reviewed Jul 11, 2018

View reviewed changes

nitsakh force-pushed the api-gpuinfo branch from 5ac16ba to aac6bff Compare July 12, 2018 17:45

YurySolovyov reviewed Jul 12, 2018

View reviewed changes

nitsakh changed the title ~~feat: Implementation of getGPUInfo API.~~ WIP:feat: Implementation of getGPUInfo API. Jul 12, 2018

MarshallOfSound reviewed Jul 13, 2018

View reviewed changes

miniak previously requested changes Jul 13, 2018

View reviewed changes

nitsakh commented Jul 13, 2018

View reviewed changes

nitsakh changed the title ~~WIP:feat: Implementation of getGPUInfo API.~~ feat: Implementation of getGPUInfo API. Jul 13, 2018

deepak1556 requested changes Jul 13, 2018

View reviewed changes

nitsakh force-pushed the api-gpuinfo branch from ac83b7c to e72b21d Compare July 17, 2018 18:55

YurySolovyov approved these changes Jul 17, 2018

View reviewed changes

deepak1556 reviewed Jul 17, 2018

View reviewed changes

atom/browser/api/gpuinfo_manager.cc Show resolved Hide resolved

atom/browser/api/gpuinfo_manager.cc Show resolved Hide resolved

MarshallOfSound reviewed Jul 18, 2018

View reviewed changes

nitsakh added 3 commits September 23, 2018 16:43

Implementation of getGPUInfo API.

6dc03f6

Clear promise set

0e3f918

Changes to promise usage

6e072a9

nitsakh force-pushed the api-gpuinfo branch from a76a461 to 6e072a9 Compare September 23, 2018 23:44

nitsakh added 7 commits September 24, 2018 11:15

Minor fixes

694d498

Fix linux build

c88fa85

Update spec

4f63f59

Fix lint (linter didn't run on windows locally)

69033e8

Test running single test for CI

a795f54

Merge branch 'api-gpuinfo' of https://github.com/electron/electron in…

7c81055

…to api-gpuinfo

Update spec

86945d9

deepak1556 approved these changes Sep 27, 2018

View reviewed changes

MarshallOfSound approved these changes Sep 27, 2018

View reviewed changes

MarshallOfSound merged commit 5c10872 into master Sep 27, 2018

MarshallOfSound deleted the api-gpuinfo branch September 27, 2018 14:59

MarshallOfSound added the semver/minor backwards-compatible functionality label Sep 27, 2018

ckerr mentioned this pull request Dec 18, 2018

Draft 4.0.0 Release Notes #16117

Closed

robatwilliams mentioned this pull request Mar 1, 2019

getGPUInfo('complete') often never settles the returned promise #17187

Closed

		@@ -228,6 +232,10 @@ class App : public AtomBrowserClient::Delegate,

		base::FilePath app_path_;

		util::Promise* gpuinfo_promise_;

feat: Implementation of getGPUInfo API. #13486

feat: Implementation of getGPUInfo API. #13486

Uh oh!

Conversation

Uh oh!

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment