1782134 - 60Hz composites forever after running toolkit/components/pictureinpicture/tests/browser_backgroundTab.js on Windows

Status: NEW

(Core :: Graphics, defect)

Description

[Florian Quèze [:florian]]

+++ This bug was initially created as a clone of Bug #1781524 +++

While working on bug 1742842, I had a try run that shows the toolkit/components/pictureinpicture/tests/browser_backgroundTab.js test causing vsync to remain enabled at the end of the test on Windows 7 (but not other platforms).

I can reproduce consistently locally on Windows 10 and 11.

Unlike bug 1781524 where the composite continues in a closed window, in this case it continues in a window that still exists, but is minimized.

If I insert await new Promise(r => setTimeout(r, 500)); before the window.minimize() call at https://searchfox.org/mozilla-central/rev/1061fae5e225a99ef5e43dbdf560a91a0c0d00d1/toolkit/components/pictureinpicture/tests/browser_backgroundTab.js#57, then the test passes.

I verified that the reason for scheduling composition is ANIMATED_PROPERTY.

Profile of the test failing: https://share.firefox.dev/3oziQ2O
Profile of the test passing (with the added 2s wait): https://share.firefox.dev/3vjwMSc

Hiro, any idea? Is this likely the same bug as bug 1781524 but happening slightly differently on Windows, or is this likely entirely different?

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I can reproduce the failure locally on Linux, so I did dig into detail a bit.

The reason why some animations (actually there were three animations) keep running for a while is the refresh driver for the minimized window is being throttled at that moment, thus hose animation end times get delayed (or maybe paints which trigger discarding animations on the compositor get delayed). Thus the animations running on the compositor keep running until the end is notified from the main-thread.

A workaround to make the test pass is adding window.restore() just before the end of the test.

We can also throttle tasks on the compositor during window gets minimized.

Comment 2

[Florian Quèze [:florian]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)

I can reproduce the failure locally on Linux, so I did dig into detail a bit.

Thanks a lot of the investigations!

The reason why some animations (actually there were three animations) keep running for a while is the refresh driver for the minimized window is being throttled at that moment, thus hose animation end times get delayed (or maybe paints which trigger discarding animations on the compositor get delayed).

Are the animation end times delayed, or never happening? I tried waiting 20s at the end of the test, and that didn't help: https://share.firefox.dev/3Bq8t8V

Throttling of background windows was recently added in bug 1779559, and if I revert the changes from that bug, the test passes for me (profile of the test with these changes reverted: https://share.firefox.dev/3OMm2CG We see the animations run until completion and we wait about 700ms for vsync to be disabled).

A workaround to make the test pass is adding window.restore() just before the end of the test.

That indeed makes the test pass for me.

We can also throttle tasks on the compositor during window gets minimized.

Is this easy to do? Or should we clear compositor animations in these windows (ie. bug 1768495)?

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1779559

Comment 4

[Emilio Cobos Álvarez (:emilio)]

(In reply to Florian Quèze [:florian] from comment #2)

Is this easy to do? Or should we clear compositor animations in these windows (ie. bug 1768495)?

Either of these solutions seems preferable, but I'm not so familiar with that code. Did you have any more concrete questions?

Comment 5

[Hiroyuki Ikezoe (:hiro)]

(In reply to Florian Quèze [:florian] from comment #2)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)

I can reproduce the failure locally on Linux, so I did dig into detail a bit.

Thanks a lot of the investigations!

The reason why some animations (actually there were three animations) keep running for a while is the refresh driver for the minimized window is being throttled at that moment, thus hose animation end times get delayed (or maybe paints which trigger discarding animations on the compositor get delayed).

Are the animation end times delayed, or never happening? I tried waiting 20s at the end of the test, and that didn't help: https://share.firefox.dev/3Bq8t8V

I am pretty sure that when I debugged we did call AddCompositorAnimationsIdForDiscard, the function gets called when the corresponding display item gets destroyed, which means the animation reached to its end or the document gets destroyed. The animation informed by the AddCompositorAnimationsIdForDiscard should be discarded in WebRenderLayerManager::EndTransactionWithoutLayer, I guess in the occluded window we never want to trigger the EndTransactionWithoutLayer, thus the needs-to-be-discarded animation wasn't processed. That's a likely scenario. On the other hand, even if the window is occluded, animation resources on the compositor should be discarded when the browser window gets destroyed, if it doesn't happen, it's an gfx issue.

(In reply to Florian Quèze [:florian] from comment #2)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)

We can also throttle tasks on the compositor during window gets minimized.

Is this easy to do? Or should we clear compositor animations in these windows (ie. bug 1768495)?

There are two different issues we need to care;

1. The issue that animation resources on the compositor are not cleared when occluded window gets destroyed
2. Throttle compositor animations inside occluded window

The latter is actually bug 1768495 I believe.

Comment 6

[Florian Quèze [:florian]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)

There are two different issues we need to care;

1. The issue that animation resources on the compositor are not cleared when occluded window gets destroyed
2. Throttle compositor animations inside occluded window

The latter is actually bug 1768495 I believe.

Is bug 1768495 something you might work on? If not, do you have an idea of who could take it? I'm under the impression that it's the cause of a pretty significant waste of power (especially obvious when having google documents in background windows with the cursor that keeps blinking using a CSS animation).

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Florian Quèze [:florian] from comment #6)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)

There are two different issues we need to care;

1. The issue that animation resources on the compositor are not cleared when occluded window gets destroyed
2. Throttle compositor animations inside occluded window

The latter is actually bug 1768495 I believe.

Is bug 1768495 something you might work on? If not, do you have an idea of who could take it?

I'd say sotaro is the best person to do that. My naive guess is that when we detect occlusion state changes, we can just ObserveVsync/UnobserveVsync?

I'm under the impression that it's the cause of a pretty significant waste of power (especially obvious when having google documents in background windows with the cursor that keeps blinking using a CSS animation).

For background tabs, we should probably a different approach because the compositor side has all animations inside a browser window, animations in the browser, animations in the contents in the foreground tabs, animations in the background tab, etc. We can't simply throttle all animations. That's said, this approach will supersede the occluded window case though.

Comment 8

[Florian Quèze [:florian]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)

(In reply to Florian Quèze [:florian] from comment #6)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)

There are two different issues we need to care;

1. The issue that animation resources on the compositor are not cleared when occluded window gets destroyed
2. Throttle compositor animations inside occluded window

The latter is actually bug 1768495 I believe.

Is bug 1768495 something you might work on? If not, do you have an idea of who could take it?

I'd say sotaro is the best person to do that. My naive guess is that when we detect occlusion state changes, we can just ObserveVsync/UnobserveVsync?

sotaro, do you agree?

I'm under the impression that it's the cause of a pretty significant waste of power (especially obvious when having google documents in background windows with the cursor that keeps blinking using a CSS animation).

For background tabs, we should probably a different approach because the compositor side has all animations inside a browser window, animations in the browser, animations in the contents in the foreground tabs, animations in the background tab, etc. We can't simply throttle all animations. That's said, this approach will supersede the occluded window case though.

I think animations in background tabs are already throttled. The waste of power is when we have an animation in the foreground tab of an occluded window, or in the browser chrome of an occluded window.

Comment 9

[Sotaro Ikeda [:sotaro]]

(In reply to Florian Quèze [:florian] from comment #8)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)

(In reply to Florian Quèze [:florian] from comment #6)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)

There are two different issues we need to care;

1. The issue that animation resources on the compositor are not cleared when occluded window gets destroyed
2. Throttle compositor animations inside occluded window

The latter is actually bug 1768495 I believe.

Is bug 1768495 something you might work on? If not, do you have an idea of who could take it?

I'd say sotaro is the best person to do that. My naive guess is that when we detect occlusion state changes, we can just ObserveVsync/UnobserveVsync?

sotaro, do you agree?

I'm under the impression that it's the cause of a pretty significant waste of power (especially obvious when having google documents in background windows with the cursor that keeps blinking using a CSS animation).

For background tabs, we should probably a different approach because the compositor side has all animations inside a browser window, animations in the browser, animations in the contents in the foreground tabs, animations in the background tab, etc. We can't simply throttle all animations. That's said, this approach will supersede the occluded window case though.

I think animations in background tabs are already throttled. The waste of power is when we have an animation in the foreground tab of an occluded window, or in the browser chrome of an occluded window.

To address browser_backgroundTab.js test failure. We could just stop generated wr frame of minimized window. It seems better to handle "animation in the foreground tab of an occluded window" in a different bug for reducing regression risk.
I am going to take this bug.

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: WIP: Bug 1782134 - Supress WebRender frame generation when window is minimized or occluded on Windows

Comment 12

[Sotaro Ikeda [:sotaro]]

In the test, Window minimize is done by calling window.minimize().

Callstack of the function becomes like the following.
window.minimize()
->nsGlobalWindowInner::Minimize()
->nsWindow::SetSizeMode(nsSizeMode_Minimized)
->nsWindow::FrameState::EnsureSizeMode(nsSizeMode_Minimized)
->nsWindow::FrameState::SetSizeModeInternal(nsSizeMode_Minimized)
->ShowWindowWithMode()
->::ShowWindow(aWnd, SW_MINIMIZE)

But it did not trigger nsWindow::FrameState::OnFrameChanged(). Since OnFrameChanged() was ignored by nsWindow::OnWindowPosChanged(). The event ignoring code was added by Bug 566135.

Comment 13

[Sotaro Ikeda [:sotaro]]

nsWindow::FrameState::mSizeMode is updated at the followings

• [1]nsWindow::FrameState::ConsumePreXULSkeletonState()
• [2]nsWindow::FrameState::OnFrameChanged()
• [3]nsWindow::FrameState::SetSizeModeInternal()

Only [2] and [3] might set the mSizeMode to nsSizeMode_Minimized.

Comment 14

[Florian Quèze [:florian]]

Are you still planning to work on this? Is this patch a good first step towards fixing bug 1771902 / bug 1768495?