fix: disable background throttling also in the viz::DisplayScheduler#38924
Merged
jkleinsc merged 10 commits intoelectron:mainfrom Sep 26, 2023
Merged
fix: disable background throttling also in the viz::DisplayScheduler#38924jkleinsc merged 10 commits intoelectron:mainfrom
viz::DisplayScheduler#38924jkleinsc merged 10 commits intoelectron:mainfrom
Conversation
`viz::DisplayScheduler` is responsible for drawing and swapping frames in the `DisplayScheduler::DrawAndSwap` which is called from the `DisplayScheduler::AttemptDrawAndSwap` if the `DisplayScheduler::ShouldDraw` returns true. `ShouldDraw` depends on the `DisplayScheduler` visibility and when it is not visible then it returns false. In order to keep producing frames, disabling `backgroundThrottling` should also prevent changing `DisplayScheduler` visibility to false. `DisplayScheduler` lives in the `ui::Compositor` where every `electron::NativewWindow` has its own `Compositor`. `electron::NativewWindow` may be host of the multiple `electron::api::WebContents` instances which may have different `WebPreferences` settings. Therefore if at least one of the `WebContents` requires disabling throttling then all other `WebContents` using the same window will have it disabled in the `ui::Compositor`. BREAKING CHANGE: `backgroundThrottling` set to false will disable frames throttling in the `BrowserWindow` for all `WebContents` displayed by it. Close: [electron#31016](electron#31016)
zcbenz
approved these changes
Jul 13, 2023
Contributor
zcbenz
left a comment
There was a problem hiding this comment.
The change looks good to me.
/cc @electron/wg-upgrades to review the patch.
codebytere
reviewed
Jul 13, 2023
Member
codebytere
left a comment
There was a problem hiding this comment.
Is this something that Chromium would accept upstream in any form? it'd be good to have a roadmap for this patch that isn't us floating it indefinitely.
Contributor
Author
|
I've addressed all issues and resolved conflict. same with I've no access to retry these builds. |
Contributor
Author
I think this may have lower chances to upstream than |
jkleinsc
requested changes
Sep 6, 2023
Member
jkleinsc
left a comment
There was a problem hiding this comment.
Looks like the patch needs to be updated.
You can update the patch by running:
e sync --3
and then running
e patches all
Contributor
Author
Patches updated, builds failed on |
Member
|
@michal-pichlinski-openfin this has been fixed in latest |
jkleinsc
requested changes
Sep 14, 2023
Member
jkleinsc
left a comment
There was a problem hiding this comment.
The breaking change should be documented in https://github.com/electron/electron/blob/main/docs/breaking-changes.md
Contributor
Author
|
Linux test runs failed due to: |
Member
|
@michal-pichlinski-openfin that's a flake that occasionally happens - could you please rebase on main? |
Contributor
Author
|
This failure looks unrelated to my change: |
Contributor
Author
Addressed in: 4212433 |
jkleinsc
approved these changes
Sep 26, 2023
Member
|
API LGTM |
|
Congrats on merging your first pull request! 🎉🎉🎉 |
|
Release Notes Persisted
|
|
Can this be updated to electron26? |
Member
|
@fanchenio, this change included a breaking change, so it can not be backported to Electron 26. |
MrHuangJser
pushed a commit
to MrHuangJser/electron
that referenced
this pull request
Dec 11, 2023
electron#38924) * fix: disable background throttling also in the `viz::DisplayScheduler` `viz::DisplayScheduler` is responsible for drawing and swapping frames in the `DisplayScheduler::DrawAndSwap` which is called from the `DisplayScheduler::AttemptDrawAndSwap` if the `DisplayScheduler::ShouldDraw` returns true. `ShouldDraw` depends on the `DisplayScheduler` visibility and when it is not visible then it returns false. In order to keep producing frames, disabling `backgroundThrottling` should also prevent changing `DisplayScheduler` visibility to false. `DisplayScheduler` lives in the `ui::Compositor` where every `electron::NativewWindow` has its own `Compositor`. `electron::NativewWindow` may be host of the multiple `electron::api::WebContents` instances which may have different `WebPreferences` settings. Therefore if at least one of the `WebContents` requires disabling throttling then all other `WebContents` using the same window will have it disabled in the `ui::Compositor`. BREAKING CHANGE: `backgroundThrottling` set to false will disable frames throttling in the `BrowserWindow` for all `WebContents` displayed by it. Close: [electron#31016](electron#31016) * fixup! fix: disable background throttling also in the `viz::DisplayScheduler` * fixup! fix: disable background throttling also in the `viz::DisplayScheduler` * fixup! fix: disable background throttling also in the `viz::DisplayScheduler` --------- Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of Change
viz::DisplayScheduleris responsible for drawing and swapping frames in theDisplayScheduler::DrawAndSwapwhich is called from theDisplayScheduler::AttemptDrawAndSwapif theDisplayScheduler::ShouldDrawreturns true.ShouldDrawdepends on theDisplaySchedulervisibility and when it is not visible then it returns false.In order to keep producing frames, disabling
backgroundThrottlingshould also prevent changingDisplaySchedulervisibility to false.DisplaySchedulerlives in theui::Compositorwhere everyelectron::NativewWindowhas its ownCompositor.electron::NativewWindowmay be host of the multipleelectron::api::WebContentsinstances which may have differentWebPreferencessettings. Therefore if at least one of theWebContentsrequires disabling throttling then all otherWebContentsusing the same window will have it disabled in theui::Compositor.BREAKING CHANGE:
backgroundThrottlingset to false will disable frames throttling in theBrowserWindowfor allWebContentsdisplayed by it.Closes: #31016
Checklist
npm testpassesRelease Notes
Notes: Fixed generating frames when the window is hidden and
backgroundThrottlingis disabled.