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

fix: mitigate white screen flash after occlusion by disabling compositor recycling #19873

Merged
merged 2 commits into from Aug 22, 2019

Conversation

@loc
Copy link
Contributor

@loc loc commented Aug 21, 2019

Description of Change

Fix #19858, caused by compositor recycling in response to OS occlusion events.

Compositor recycling is useful for Chrome because there can be many tabs and spinning up a compositor for each one would be costly. In practice, Chrome uses the parent compositor code path of browser_compositor_view_mac.mm; the NSView of each tab is detached when it's hidden and attached when it's shown. For Electron, there is no parent compositor, so we're forced into the "own compositor" code path, which seems to be non-optimal and pretty ruthless in terms of the release of resources. Electron has no real concept of multiple tabs per window, so it should be okay to disable this ruthless recycling altogether in Electron.

There's some risk:
a) it might degrade CPU performance
b) it might have a memory cost
c) I largely don't know what I'm doing

To satiate a), I loaded giphy.com before and after the patch with the window both visible and occluded. After the patch, the renderer and GPU processes still drop to zero, as we would hope.

No patch, visible:
without-patch-before

No patch, occluded:
without-patch-after

Patch, visible:
with-patch-before

Patch, occluded:
with-patch-after

Through inspection, I think I can allay concern b): the recycleable_compositor keeps around at least one compositor of the pool that it manages (per window). So, with this change, I don't think we're actually affecting anything. It's possible that detaching from the compositor frees up some surface buffers or something, but I imagine that's negligible.

For c) reviewers will have to weigh in with more context on potential risks to other features that I'm unaware of.

As a bonus, with this change, we're able to remove a workaround introduced in #18661. The occlusion event from macOS was errantly firing when fullscreening, causing the compositor to be recycled. Now that we don't do that, we no longer need the debounce.

Checklist

Release Notes

Notes: Fixed white flash after restoring an app from the background.

@loc loc requested a review from as a code owner Aug 21, 2019
Copy link
Member

@codebytere codebytere left a comment

patch lgtm given extra context from @MarshallOfSound - you'll need to resolve patch conflict though.

Loading

@deepak1556
Copy link
Member

@deepak1556 deepak1556 commented Aug 21, 2019

Electron has no real concept of multiple tabs per window, so it should be okay to disable this ruthless recycling altogether in Electron.

While this is true in general, @loc have you tested this change's impact for macOS native tabs code path ?

/cc requesting review from @brenca @zcbenz for compositor code path.

Loading

@MarshallOfSound
Copy link
Member

@MarshallOfSound MarshallOfSound commented Aug 22, 2019

have you tested this change's impact for macOS native tabs code path ?

We hadn't but now I have and the same got the results as our other tests. Renderers backgrounded in native tabs don't white flash but they do drop to 0% CPU usage when occluded 👍

Loading

zcbenz
zcbenz approved these changes Aug 22, 2019
Copy link
Member

@zcbenz zcbenz left a comment

This looks awesome!

Loading

@brenca brenca self-requested a review Aug 22, 2019
brenca
brenca approved these changes Aug 22, 2019
Copy link
Contributor

@brenca brenca left a comment

LGTM, this shouldn't have any effect on OSR AFAIK, we are already treating those windows as visible all the time if I remember correctly.

Loading

@MarshallOfSound
Copy link
Member

@MarshallOfSound MarshallOfSound commented Aug 22, 2019

This appears to have destabilized a visibilityState test. Will investigate today 😄

Loading

@MarshallOfSound MarshallOfSound force-pushed the fix-white-screen-flash branch from 50888f0 to ee18a2f Aug 22, 2019
@MarshallOfSound
Copy link
Member

@MarshallOfSound MarshallOfSound commented Aug 22, 2019

This appears to have destabilized a visibilityState test. Will investigate today 😄

So my debounce patch was hiding the fact that this test has been broken for a while. I've disabled it in this PR to unblock it and will investigate after it has landed.

Loading

@MarshallOfSound MarshallOfSound merged commit f7e3e1f into electron:master Aug 22, 2019
15 checks passed
Loading
@release-clerk
Copy link

@release-clerk release-clerk bot commented Aug 22, 2019

Release Notes Persisted

Fixed white flash after restoring an app from the background.

Loading

@trop
Copy link
Contributor

@trop trop bot commented Aug 22, 2019

I was unable to backport this PR to "6-0-x" cleanly;
you will need to perform this backport manually.

Loading

@trop
Copy link
Contributor

@trop trop bot commented Aug 22, 2019

I was unable to backport this PR to "7-0-x" cleanly;
you will need to perform this backport manually.

Loading

@trop
Copy link
Contributor

@trop trop bot commented Aug 22, 2019

A maintainer has manually backported this PR to "6-0-x", please check out #19900

Loading

@trop
Copy link
Contributor

@trop trop bot commented Aug 22, 2019

A maintainer has manually backported this PR to "7-0-x", please check out #19901

Loading

@sofianguy sofianguy added this to Fixed in 6.0.4 in 6.1.x Aug 26, 2019
@sofianguy sofianguy added this to Fixed in 7.0.0-beta.4 in 7.2.x Sep 3, 2019
@poiru
Copy link
Contributor

@poiru poiru commented Oct 28, 2019

@MarshallOfSound @zcbenz Unfortunately this patch caused a regression with BrowserViews. In our app, switching tabs switches the visible BrowserView. Due to a change in this PR, the BrowserViews are momentarily blank when you switch to them. Prior to this change and e.g. in Chrome (the browser), switching to a tab makes it immediately visible without any sort of flicker.

Reverting a bit of this PR restores the old and correct behavior:

 void BrowserCompositorMac::SetRenderWidgetHostIsHidden(bool hidden) {
-  render_widget_host_is_hidden_ = false;
+  render_widget_host_is_hidden_ = hidden;
   UpdateState();
 }

I'm not sure what to do here - should we revert this PR or should we try to find another solution? This is a critical issue for us, we cannot upgrade to 6.x and beyond due to this.

Loading

@MarshallOfSound
Copy link
Member

@MarshallOfSound MarshallOfSound commented Oct 28, 2019

should we revert this PR or should we try to find another solution?

IMO no, this fixes a pretty nasty bug which affects the majority of users. I'm unsure why it would negatively impact BrowserView users but that shouldn't mean we revert this.

I'd be open to a forward-fix PR here but I don't have time to dig into this right now.

Loading

poiru added a commit to poiru/electron that referenced this issue Oct 29, 2019
In electron#19873, we completely disabled compositor recycling. This has adverse
effects in our tabbed app where switching tabs (i.e. `BrowserView`s) now
results in a flicker because we now also switch compositors.

To fix this without regressing the original fix, we now recycle the
compositor when the view is removed from a window or explicitly marked
as hidden. These can only happen with `BrowserView`s so `BrowserWindow`s
are unaffected.
@poiru
Copy link
Contributor

@poiru poiru commented Oct 29, 2019

@MarshallOfSound I managed to find a fix that resolves our issue without regressing the common case. See #20829.

Loading

poiru added a commit to poiru/electron that referenced this issue Oct 29, 2019
In electron#19873, we completely disabled compositor recycling. This has adverse
effects in our tabbed app where switching tabs (i.e. `BrowserView`s) now
results in a flicker because we now also switch compositors.

To fix this without regressing the original fix, we now recycle the
compositor when the view is removed from a window or explicitly marked
as hidden. These can only happen with `BrowserView`s so `BrowserWindow`s
are unaffected.
poiru added a commit to poiru/electron that referenced this issue Oct 29, 2019
In electron#19873, we completely disabled compositor recycling. This has adverse
effects in our tabbed app where switching tabs (i.e. `BrowserView`s) now
results in a flicker because we now also switch compositors.

To fix this without regressing the original fix, we now recycle the
compositor when the view is removed from a window or explicitly marked
as hidden. These can only happen with `BrowserView`s so `BrowserWindow`s
are unaffected.
poiru added a commit to poiru/electron that referenced this issue Oct 29, 2019
In electron#19873, we completely disabled compositor recycling. This has adverse
effects in our tabbed app where switching tabs (i.e. `BrowserView`s) now
results in a flicker because we now also switch compositors.

To fix this without regressing the original fix, we now recycle the
compositor when the view is removed from a window or explicitly marked
as hidden. These can only happen with `BrowserView`s so `BrowserWindow`s
are unaffected.
poiru added a commit to poiru/electron that referenced this issue Oct 30, 2019
In electron#19873, we completely disabled compositor recycling. This has adverse
effects in our tabbed app where switching tabs (i.e. `BrowserView`s) now
results in a flicker because we now also switch compositors.

To fix this without regressing the original fix, we now recycle the
compositor when the view is removed from a window. This situation can
only happen with `BrowserView`s and the common case with `BrowserWindow`
is unaffected.
MarshallOfSound added a commit that referenced this issue Oct 30, 2019
In #19873, we completely disabled compositor recycling. This has adverse
effects in our tabbed app where switching tabs (i.e. `BrowserView`s) now
results in a flicker because we now also switch compositors.

To fix this without regressing the original fix, we now recycle the
compositor when the view is removed from a window. This situation can
only happen with `BrowserView`s and the common case with `BrowserWindow`
is unaffected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
6.1.x
Fixed in 6.0.4
7.2.x
Fixed in 7.0.0-beta.4
Linked issues

Successfully merging this pull request may close these issues.

7 participants