1578851 - WebVR immersive mode broken on android when e10s is enabled

Status: RESOLVED FIXED

(Core :: WebVR, defect, P1)

Description

[Randall Barker [:rbarker]]

On android the following always returns false when e10s is enabled:
https://searchfox.org/mozilla-central/rev/e5327b05c822cdac24e233afa37d72c0552dbbaf/gfx/vr/VRManager.cpp#1049
This causes skipPaint to always be false here:
https://searchfox.org/mozilla-central/rev/e5327b05c822cdac24e233afa37d72c0552dbbaf/layout/base/nsRefreshDriver.cpp#2136

Because of this, the refresh driver bails out early and many callbacks are not invoked.

Comment 1

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

The priority flag is not set for this bug.
:kip, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[Daosheng Mu[:daoshengmu]]

I am going to work on this.

Comment 3

[Daosheng Mu[:daoshengmu]]

(In reply to Randall Barker [:rbarker] from comment #0)

On android the following always returns false when e10s is enabled:
https://searchfox.org/mozilla-central/rev/e5327b05c822cdac24e233afa37d72c0552dbbaf/gfx/vr/VRManager.cpp#1049
This causes skipPaint to always be false here:
https://searchfox.org/mozilla-central/rev/e5327b05c822cdac24e233afa37d72c0552dbbaf/layout/base/nsRefreshDriver.cpp#2136

Because of this, the refresh driver bails out early and many callbacks are not invoked.

https://chromatic.funktroniclabs.com/ actually returns true when running https://searchfox.org/mozilla-central/rev/e5327b05c822cdac24e233afa37d72c0552dbbaf/gfx/vr/VRManager.cpp#1049 in e10s.

I am going to investigate other potential issues.

Comment 4

[Daosheng Mu[:daoshengmu]]

The difference between Android and desktop doing immersive mode is we don't use CompositorVsyncScheduler in Android.

For doing animation in WebVR, there are couple of ways can do the update. I have checked VRFrameData.Timestamp() is increased progressively, and VRManagerChild::RunFrameRequestCallbacks() also be executed properly, its timeStamp for running callbacks is increased as well.

So, one last place to do animation is nsGlobalWindowInner::RequestAnimationFrame(). I notice it doesn't call nsRefreshDriver::RunFrameRequestCallbacks() in e10s mode, non-e10s mode does. Both of e10s and non-e10s have HTMLCanvasElement::InvalidateCanvasContent that comes from WebGLContext::Clear()to call nsIFrame::InvalidateLayer(). It will ask mRootRefreshDrivers to repaint layers in nsRefreshDriver. I think this is the root cause.

Comment 5

[Daosheng Mu[:daoshengmu]]

We get stuck at nsRefreshDriver::IsWaitingForPaint() [1]. It looks like we spend too much time at LayerTransaction in e10s mode.

[1] https://searchfox.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp#2377

Comment 6

[Randall Barker [:rbarker]]

There should not be any paint when in immersive mode. Please re-read the original comment in the bug.

Comment 7

[Daosheng Mu[:daoshengmu]]

(In reply to Randall Barker [:rbarker] from comment #6)

There should not be any paint when in immersive mode. Please re-read the original comment in the bug.

We can't use VRManager::Get() in the tab process. It is only able to be instanced in the parent process or GPU process in Windows. That is why it always returns false from IsPresenting().

Comment 8

[Randall Barker [:rbarker]]

We need an alternative then. We need to short circuit painting in the content process.

Comment 9

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1578851 - Using VRManagerChild to check isPresenting to skip painting in nsRefreshDriver.

Comment 10

[Daosheng Mu[:daoshengmu]]

Try looks good, https://treeherder.mozilla.org/#/jobs?repo=try&revision=439dcb8d8a08f2d1e63498aa77f258064f3b2ebe

Comment 11

[Daosheng Mu[:daoshengmu]]

Move r? from :mattwoodrow to :mstange because the original skipPaint patch was reviewed by :mstange.

Comment 12

[Markus Stange [:mstange]]

Sorry for the long delay. I'm trying to make sense of this bug.

The original purpose of the skipPaint check was an optimization: If we don't need to paint, skip the paint. It should not have affected correctness. If skipPaint is false more often than before, that should only cause extra work but be otherwise harmless. The only danger would be if skipPaint returned true in cases where it shouldn't. But that's not the case. So the description in comment 0 is not accurate.

The real description of what's broken here is given in comment 5: We don't run the refresh driver tick in immersive mode because we don't get notifications from the compositor that composites have happened; we keep waiting for those notifications. This is, as far as I can tell, a bug that's completely independent from the skipPaint check. Please let me know if that's not the case.

So now we have two reasons to check whether we're in immersive mode:

1. Don't paint if we're in immersive mode. This is just an optimization.
2. Don't wait for notifications from the compositor that we've composited. This is a correctness issue.

However, I think a better fix would be to deliver notifications correctly even during immersive mode. The IsWaitingForPaint(aNowTime) check is there to make sure that content doesn't get more than two frames ahead of what's on the screen. I think this method of back-pressure is useful even in immersive mode. But it can only work if content is notified when things end up on the screen in immersive mode.

Since this bug is time-critical and needs a fix now, I'm going to accept the current fix (but with a variable rename). But I think the better fix would be to make IsWaitingForPaint work correctly in immersive mode.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d4b62d6f1e64
Using VRManagerChild to check isPresenting to skip painting in nsRefreshDriver. r=rbarker,imanol,mstange

Comment 14

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/d4b62d6f1e64

Comment 15

[Daosheng Mu[:daoshengmu]]

(In reply to Markus Stange [:mstange] from comment #12)

Sorry for the long delay. I'm trying to make sense of this bug.

The original purpose of the skipPaint check was an optimization: If we don't need to paint, skip the paint. It should not have affected correctness. If skipPaint is false more often than before, that should only cause extra work but be otherwise harmless. The only danger would be if skipPaint returned true in cases where it shouldn't. But that's not the case. So the description in comment 0 is not accurate.

The real description of what's broken here is given in comment 5: We don't run the refresh driver tick in immersive mode because we don't get notifications from the compositor that composites have happened; we keep waiting for those notifications. This is, as far as I can tell, a bug that's completely independent from the skipPaint check. Please let me know if that's not the case.

So now we have two reasons to check whether we're in immersive mode:

1. Don't paint if we're in immersive mode. This is just an optimization.
2. Don't wait for notifications from the compositor that we've composited. This is a correctness issue.

However, I think a better fix would be to deliver notifications correctly even during immersive mode. The IsWaitingForPaint(aNowTime) check is there to make sure that content doesn't get more than two frames ahead of what's on the screen. I think this method of back-pressure is useful even in immersive mode. But it can only work if content is notified when things end up on the screen in immersive mode.

Since this bug is time-critical and needs a fix now, I'm going to accept the current fix (but with a variable rename). But I think the better fix would be to make IsWaitingForPaint work correctly in immersive mode.

Agree. That makes senses with what I got from my investigation.