feat: disable fetching thumbnails if thumbnailSize is 0 #14906

CapOM · 2018-10-01T22:08:27Z

Description of Change

Capturing window thumbnails is expensive as it actually use the
window capturer and record one full frame per window and then
downscale then to the default size 150x150. When only interested
in the app icons we do not need all of this.

Example: desktopCapturer.getSources({thumbnailSize: {width: 0, height: 0}}, ...)

Release Notes

Notes: Added ability disable fetching thumbnails for in desktopCapturer.getSources()

MarshallOfSound

Hey @CapOM thanks for the PR

Can you make this backwards compatible by setting the thumbnails to nativeImage.createEmpty() when width and height are 0. This should be consistent with what was happening previously making this semver/minor instead of semver/major.

CapOM · 2018-10-02T17:32:04Z

Hi, thx for the suggestion, where exactly ? Because

dict.Set("thumbnail",
atom::api::NativeImage::Create(
isolate, gfx::Image(source.media_list_source.thumbnail)));

Should already make it backward compatible cause the media_list_source.thumbnail is initialized with the gfx::ImageSkia 's default constructor, i.e. an empty image.

CapOM · 2018-10-04T18:18:56Z

Gentle ping ?

MarshallOfSound · 2018-10-04T23:31:23Z

Should already make it backward compatible

If that's the case can you add a test to verify that 👍

Also you'll need to rebase this onto latest master as the desktop capturer API was refactored recently

MarshallOfSound · 2018-10-19T00:30:52Z

@CapOM This is now conflicting with master as well, just need to rebase and address

If that's the case can you add a test to verify that 👍

so that we can review and merge

CapOM · 2018-10-19T19:55:00Z

Since that chromium/src/chrome is now pulled from upstream instead of the old electron/chromium_src duplication I think I will first try to have the change on chromium_src/chrome/browser/media/native_desktop_media_list.cc in chromium upstream.
Once it is done this should reduce this MR to just the doc.

CapOM · 2018-10-25T17:44:14Z

Patch is now merged in chromium: https://chromium-review.googlesource.com/c/chromium/src/+/1295111 . So once it reaches electron's chromium I will rebase this PR which will just contain the doc part so.

miniak · 2019-01-05T14:48:20Z

@CapOM please update the PR description with the proper template (copy from a new PR)

CapOM · 2019-01-10T19:29:43Z

@miniak Hi, can you provide more details about how to do that, I am not sure to follow, thx

poiru · 2019-01-10T20:54:12Z

@CapOM The pull request should follow the format described here: https://raw.githubusercontent.com/electron/electron/master/.github/PULL_REQUEST_TEMPLATE.md

CapOM · 2019-01-10T21:05:45Z

Thx I will do it once electron uses chromium72 (see comment above)

codebytere · 2019-01-10T23:47:35Z

@CapOM a PR for that has been opened: #16334

miniak · 2019-01-27T16:12:02Z

@CapOM Chromium 72 is merged, can you please rebase your PR?

CapOM · 2019-01-29T21:53:13Z

Done

CapOM · 2019-01-30T19:40:42Z

Hi, I added 'feat: ' in the PR and I added a release note in the commit description. Please take a look, thx!

codebytere · 2019-02-04T17:52:48Z

@CapOM could you rebase this on master? It should solve the CI issues.

CapOM · 2019-02-05T19:03:52Z

@MarshallOfSound Hi, I want to add a test for what you suggested but I get:

npm run test 

> electron@6.0.0-nightly.20190123 test /home/julien/dev/electron-gn/src/electron
> node ./script/spec-runner.js electron/spec


> abstract-socket@2.0.0 install /home/julien/dev/electron-gn/src/electron/spec/node_modules/abstract-socket
> node-gyp rebuild

gyp: /home/julien/dev/electron-gn/src/out/Debug/gen/node_headers/common.gypi not found (cwd: /home/julien/dev/electron-gn/src/electron/spec/node_modules/abstract-socket) while reading includes of binding.gyp while trying to load binding.gyp
gyp ERR! configure error 
gyp ERR! stack Error: `gyp` failed with exit code: 1
gyp ERR! stack     at ChildProcess.onCpExit (/usr/local/lib/node_modules/npm/node_modules/node-gyp/lib/configure.js:336:16)
gyp ERR! stack     at ChildProcess.emit (events.js:160:13)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:209:12)
gyp ERR! System Linux 4.15.0-13-generic
gyp ERR! command "/usr/local/bin/node" "/usr/local/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
gyp ERR! cwd /home/julien/dev/electron-gn/src/electron/spec/node_modules/abstract-socket
gyp ERR! node -v v9.5.0
gyp ERR! node-gyp -v v3.6.2
gyp ERR! not ok 

> electron-test@0.1.0 postinstall /home/julien/dev/electron-gn/src/electron/spec
> node ../tools/run-if-exists.js node_modules/robotjs node-gyp rebuild

gyp: /home/julien/dev/electron-gn/src/out/Debug/gen/node_headers/common.gypi not found (cwd: /home/julien/dev/electron-gn/src/electron/spec/node_modules/robotjs) while reading includes of binding.gyp while trying to load binding.gyp
gyp ERR! configure error 
gyp ERR! stack Error: `gyp` failed with exit code: 1
gyp ERR! stack     at ChildProcess.onCpExit (/usr/local/lib/node_modules/npm/node_modules/node-gyp/lib/configure.js:336:16)
gyp ERR! stack     at ChildProcess.emit (events.js:160:13)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:209:12)
gyp ERR! System Linux 4.15.0-13-generic
gyp ERR! command "/usr/local/bin/node" "/usr/local/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
gyp ERR! cwd /home/julien/dev/electron-gn/src/electron/spec/node_modules/robotjs
gyp ERR! node -v v9.5.0
gyp ERR! node-gyp -v v3.6.2
gyp ERR! not ok 
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! electron-test@0.1.0 postinstall: `node ../tools/run-if-exists.js node_modules/robotjs node-gyp rebuild`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the electron-test@0.1.0 postinstall script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/julien/.npm/_logs/2019-02-05T18_36_12_066Z-debug.log
An error occurred inside the spec runner: Error: Failed to npm install in the spec folder
    at installSpecModules (/home/julien/dev/electron-gn/src/electron/script/spec-runner.js:69:11)
    at main (/home/julien/dev/electron-gn/src/electron/script/spec-runner.js:23:11)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:160:7)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! electron@6.0.0-nightly.20190123 test: `node ./script/spec-runner.js electron/spec`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the electron@6.0.0-nightly.20190123 test script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/julien/.npm/_logs/2019-02-05T18_36_12_103Z-debug.log

Any idea ? This is on Ubuntu
Thx!

codebytere · 2019-02-05T19:06:43Z

@CapOM did you consult https://electronjs.org/docs/development/build-instructions-gn#tests?

CapOM · 2019-02-05T19:14:36Z

@codebytere Thx, this other way you pointed works.

CapOM · 2019-02-07T22:47:29Z

Hi @MarshallOfSound , I added a unit test to verify what you suggested, please take a look, thx

MarshallOfSound

Just some docs change suggestions 👍 Thanks for the test 🎉

MarshallOfSound · 2019-02-11T18:40:30Z

docs/api/desktop-capturer.md

@@ -82,7 +82,9 @@ The `desktopCapturer` module has the following methods:
  * `types` String[] - An array of Strings that lists the types of desktop sources
    to be captured, available types are `screen` and `window`.
  * `thumbnailSize` [Size](structures/size.md) (optional) - The size that the media source thumbnail
-    should be scaled to. Default is `150` x `150`.
+    should be scaled to. Default is `150` x `150`. Set width or height to 0 when not interested in


Suggested change

should be scaled to. Default is `150` x `150`. Set width or height to 0 when not interested in

should be scaled to. Default is `150` x `150`. Set width or height to 0 when you do not need

MarshallOfSound · 2019-02-11T18:40:58Z

docs/api/desktop-capturer.md

@@ -82,7 +82,9 @@ The `desktopCapturer` module has the following methods:
  * `types` String[] - An array of Strings that lists the types of desktop sources
    to be captured, available types are `screen` and `window`.
  * `thumbnailSize` [Size](structures/size.md) (optional) - The size that the media source thumbnail
-    should be scaled to. Default is `150` x `150`.
+    should be scaled to. Default is `150` x `150`. Set width or height to 0 when not interested in
+    the thumbnails. This save some processing like capturing the content of each window and screen


Suggested change

the thumbnails. This save some processing like capturing the content of each window and screen

the thumbnails. This will save the processing time required for capturing the content of each window and screen.

MarshallOfSound · 2019-02-11T18:41:10Z

docs/api/desktop-capturer.md

-    should be scaled to. Default is `150` x `150`.
+    should be scaled to. Default is `150` x `150`. Set width or height to 0 when not interested in
+    the thumbnails. This save some processing like capturing the content of each window and screen
+    plus the downscaling to the thumbnail size.


Suggested change

plus the downscaling to the thumbnail size.

Capturing window thmubnails is expensive as it actually uses the window capturer and it records one full frame per window and then downscale to the default size 150x150. When only interested in the window names or the app icons we do not need all of this. Underlying change is merged in chromium72 so this patch only modifies the doc, see: https://chromium.googlesource.com/chromium/src.git/+log/72.0.3626.52/chrome/browser/media/webrtc/native_desktop_media_list.cc Example: desktopCapturer.getSources({thumbnailSize: {width: 0, height: 0}}, ...) Also added a unit test in spec/api-desktop-capturer-spec.js that verifies that the returned thumbails are of type NativeImage and empty, when the user disable fetching thumbnails. notes: Can disable fetching the thumbnails for the DesktopCapturer. electron#14872

CapOM · 2019-02-11T19:54:31Z

Thx for the suggested changes. Done. I also copy/paster the new doc to the doc of the promisified version of desktopCapturer.getSources .

CapOM · 2019-02-13T17:12:55Z

Hi, gentle ping ?

release-clerk · 2019-02-13T18:27:46Z

Release Notes Persisted

Added ability disable fetching thumbnails for in desktopCapturer.getSources()

CapOM requested review from a team October 1, 2018 22:08

MarshallOfSound requested changes Oct 2, 2018

View reviewed changes

CapOM force-pushed the disable_thumbnail branch from 3a09180 to e6aa4e5 Compare January 29, 2019 21:52

CapOM changed the title ~~chromium_src: disable fetching thumbnails if thumbnailSize is 0~~ feat: disable fetching thumbnails if thumbnailSize is 0 Jan 30, 2019

CapOM force-pushed the disable_thumbnail branch from e6aa4e5 to 308fa22 Compare January 30, 2019 19:38

CapOM force-pushed the disable_thumbnail branch from 308fa22 to 697a010 Compare February 5, 2019 17:18

CapOM force-pushed the disable_thumbnail branch from 697a010 to b8f272b Compare February 7, 2019 22:46

CapOM force-pushed the disable_thumbnail branch from b8f272b to 11b6878 Compare February 7, 2019 23:45

jkleinsc requested a review from MarshallOfSound February 11, 2019 18:38

MarshallOfSound requested changes Feb 11, 2019

View reviewed changes

CapOM force-pushed the disable_thumbnail branch from 11b6878 to 4f385e6 Compare February 11, 2019 19:51

CapOM force-pushed the disable_thumbnail branch from 4f385e6 to d253f23 Compare February 11, 2019 19:52

jkleinsc requested a review from MarshallOfSound February 13, 2019 18:25

MarshallOfSound approved these changes Feb 13, 2019

View reviewed changes

MarshallOfSound merged commit 9b29bef into electron:master Feb 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: disable fetching thumbnails if thumbnailSize is 0 #14906

feat: disable fetching thumbnails if thumbnailSize is 0 #14906

CapOM commented Oct 1, 2018 •

edited by codebytere

MarshallOfSound left a comment

CapOM commented Oct 2, 2018

CapOM commented Oct 4, 2018

MarshallOfSound commented Oct 4, 2018

MarshallOfSound commented Oct 19, 2018

CapOM commented Oct 19, 2018

CapOM commented Oct 25, 2018

miniak commented Jan 5, 2019

CapOM commented Jan 10, 2019

poiru commented Jan 10, 2019

CapOM commented Jan 10, 2019

codebytere commented Jan 10, 2019

miniak commented Jan 27, 2019

CapOM commented Jan 29, 2019

CapOM commented Jan 30, 2019

codebytere commented Feb 4, 2019

CapOM commented Feb 5, 2019

codebytere commented Feb 5, 2019

CapOM commented Feb 5, 2019

CapOM commented Feb 7, 2019

MarshallOfSound left a comment

MarshallOfSound Feb 11, 2019

CapOM Feb 11, 2019

MarshallOfSound Feb 11, 2019

CapOM Feb 11, 2019

MarshallOfSound Feb 11, 2019

CapOM Feb 11, 2019

CapOM commented Feb 11, 2019

CapOM commented Feb 13, 2019

release-clerk bot commented Feb 13, 2019

	should be scaled to. Default is `150` x `150`. Set width or height to 0 when not interested in
	should be scaled to. Default is `150` x `150`. Set width or height to 0 when you do not need

	the thumbnails. This save some processing like capturing the content of each window and screen
	the thumbnails. This will save the processing time required for capturing the content of each window and screen.

feat: disable fetching thumbnails if thumbnailSize is 0 #14906

feat: disable fetching thumbnails if thumbnailSize is 0 #14906

Conversation

CapOM commented Oct 1, 2018 • edited by codebytere

Description of Change

Release Notes

MarshallOfSound left a comment

Choose a reason for hiding this comment

CapOM commented Oct 2, 2018

CapOM commented Oct 4, 2018

MarshallOfSound commented Oct 4, 2018

MarshallOfSound commented Oct 19, 2018

CapOM commented Oct 19, 2018

CapOM commented Oct 25, 2018

miniak commented Jan 5, 2019

CapOM commented Jan 10, 2019

poiru commented Jan 10, 2019

CapOM commented Jan 10, 2019

codebytere commented Jan 10, 2019

miniak commented Jan 27, 2019

CapOM commented Jan 29, 2019

CapOM commented Jan 30, 2019

codebytere commented Feb 4, 2019

CapOM commented Feb 5, 2019

codebytere commented Feb 5, 2019

CapOM commented Feb 5, 2019

CapOM commented Feb 7, 2019

MarshallOfSound left a comment

Choose a reason for hiding this comment

MarshallOfSound Feb 11, 2019

Choose a reason for hiding this comment

CapOM Feb 11, 2019

Choose a reason for hiding this comment

MarshallOfSound Feb 11, 2019

Choose a reason for hiding this comment

CapOM Feb 11, 2019

Choose a reason for hiding this comment

MarshallOfSound Feb 11, 2019

Choose a reason for hiding this comment

CapOM Feb 11, 2019

Choose a reason for hiding this comment

CapOM commented Feb 11, 2019

CapOM commented Feb 13, 2019

release-clerk bot commented Feb 13, 2019

CapOM commented Oct 1, 2018 •

edited by codebytere