Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: disable fetching thumbnails if thumbnailSize is 0 #14906

Merged
merged 1 commit into from Feb 13, 2019

Conversation

@CapOM
Copy link
Contributor

commented Oct 1, 2018

Description of Change

Refs: #14872.

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()

@CapOM CapOM requested review from as code owners Oct 1, 2018

@MarshallOfSound
Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2018

Gentle ping ?

@MarshallOfSound

This comment has been minimized.

Copy link
Member

commented Oct 4, 2018

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

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2019

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

@CapOM

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

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

@poiru

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

@CapOM

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

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

@codebytere

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

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

@miniak

This comment has been minimized.

Copy link
Contributor

commented Jan 27, 2019

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

@CapOM CapOM force-pushed the CapOM:disable_thumbnail branch from 3a09180 to e6aa4e5 Jan 29, 2019

@CapOM

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2019

Done

@CapOM 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 CapOM force-pushed the CapOM:disable_thumbnail branch from e6aa4e5 to 308fa22 Jan 30, 2019

@CapOM

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2019

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

@codebytere

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

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

@CapOM CapOM force-pushed the CapOM:disable_thumbnail branch from 308fa22 to 697a010 Feb 5, 2019

@CapOM

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

@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

This comment has been minimized.

@CapOM

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

@codebytere Thx, this other way you pointed works.

@CapOM CapOM force-pushed the CapOM:disable_thumbnail branch from 697a010 to b8f272b Feb 7, 2019

@CapOM

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

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

@CapOM CapOM force-pushed the CapOM:disable_thumbnail branch from b8f272b to 11b6878 Feb 7, 2019

@jkleinsc jkleinsc requested a review from MarshallOfSound Feb 11, 2019

@MarshallOfSound
Copy link
Member

left a comment

Just some docs change suggestions 👍 Thanks for the test 🎉

@@ -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

This comment has been minimized.

Copy link
@MarshallOfSound

MarshallOfSound Feb 11, 2019

Member
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

This comment has been minimized.

Copy link
@CapOM

CapOM Feb 11, 2019

Author Contributor

Done

@@ -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

This comment has been minimized.

Copy link
@MarshallOfSound

MarshallOfSound Feb 11, 2019

Member
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.

This comment has been minimized.

Copy link
@CapOM

CapOM Feb 11, 2019

Author Contributor

Done

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.

This comment has been minimized.

Copy link
@MarshallOfSound

MarshallOfSound Feb 11, 2019

Member
Suggested change
plus the downscaling to the thumbnail size.

This comment has been minimized.

Copy link
@CapOM

CapOM Feb 11, 2019

Author Contributor

Done

@CapOM CapOM force-pushed the CapOM:disable_thumbnail branch from 11b6878 to 4f385e6 Feb 11, 2019

Julien Isorce
feat: disable fetching thumbnails if thumbnailSize is 0
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.

#14872

@CapOM CapOM force-pushed the CapOM:disable_thumbnail branch from 4f385e6 to d253f23 Feb 11, 2019

@CapOM

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

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

@CapOM

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

Hi, gentle ping ?

@jkleinsc jkleinsc requested a review from MarshallOfSound Feb 13, 2019

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

8 of 9 checks passed

Artifact Comparison Changes Detected
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Feb 13, 2019

Release Notes Persisted

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.