1869527 - Disable SurfaceControl rendering path

Status: RESOLVED FIXED

(GeckoView :: General, task)

Description

[Jamie Nicol [:jnicol]]

This may be responsible for bug 1866555, at least partly. It's certainly responsible for some broken rendering I can reproduce locally on my Samsung S22, though it may not be exactly the same issue.

When we encounter a GPU process crash we release the old SurfaceControl, then create a new one when we go to start rendering again. However, we do not explicitly remove the SurfaceControl from the layer tree. See this comment.

Explicitly removing the SurfaceControl from the layer tree avoids the broken rendering that I can reproduce locally. However, it results in a long black flash (~1 second) while rendering is reinitialized following a GPU process crash.

Since bug 1824083 landed we now don't actually need to use SurfaceControl at all: when the GPU process crashes if we cannot create a new EGLSurface, then we will request an entirely new Surface from the frontend. This isn't quite as seamless as how we currently use SurfaceControl (when it works as intended!), but is much more seamless than using SurfaceControl if we were explicitly remove old Surfaces from the layer tree: It results in a split-second black flash as opposed to a whole-second one.

So let's disable SurfaceControl, but keep an eye on the signature in bug 1869353 in case it spikes.

Comment 1

[Jamie Nicol [:jnicol]]

Attachment: Flickering caused by not removing old SurfaceControls following a GPU process crash

Here's a video of the bug I can reproduce locally. It doesn't look quite the same as what we've seen in bug 1866555, but it's at the very least a bug in its own right if not related.

To reproduce software webrender (with OpenGL compositing) needs to be enabled, then simply trigger a GPU process crash.

Comment 2

[Jamie Nicol [:jnicol]]

Attachment: Bug 1869527 - Explicitly unparent old SurfaceControls following GPU process crash. r?#geckoview-reviewers

We used to avoid doing this in order to avoid a blank screen for a
second whilst rendering is reinitialized. However, not doing so is
causing severe rendering glitches on Samsung S22 devices.

The subsequent patch in this series is actually going to disable
SurfaceControl rendering anyway, as bug 1824083 provides an
alternative solution to recovering from GPU process crashes. However,
it is better to fix this issue now anyway in case we decide to
re-enable it one day.

Comment 3

[Jamie Nicol [:jnicol]]

Attachment: Bug 1869527 - Disable SurfaceControl rendering path. r?#geckoview-reviewers

We are seeing severe rendering glitches using the SurfaceControl
rendering path on Samsung S22 devices. This path was added to allow us
to gracefully recover from GPU process crashes on modern Android
versions (See bug 1762424). However, since bug 1824083 we have an
alternative solution to that problem.

Depends on D196168

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9e895d2fdd1e
Disable SurfaceControl rendering path. r=geckoview-reviewers,owlish

Comment 5

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/9e895d2fdd1e

Comment 6

[Jamie Nicol [:jnicol]]

Attachment: Bug 1869527 - Disable SurfaceControl rendering path.

We are seeing severe rendering glitches using the SurfaceControl
rendering path on Samsung S22 devices. This path was added to allow us
to gracefully recover from GPU process crashes on modern Android
versions (See bug 1762424). However, since bug 1824083 we have an
alternative solution to that problem.

Original Revision: https://phabricator.services.mozilla.com/D196169

Depends on D196260

Comment 7

[Phabricator Automation]

Uplift Approval Request

• Is Android affected?: yes
• Code covered by automated testing: yes
• Risk associated with taking this patch: Low
• Fix verified in Nightly: no
• String changes made/needed: None
• Explanation of risk level: Moves us away from using an advanced API to get a rendering Surface and back to the simple option of rendering in to the system-provided Surface. This should be better tested by device manufacturers, and is what we used to do. The advanced workaround is no longer required since we landed bug 1824083, which reduced the crash rate on Android of that signature practically to zero from very high, so we are confident it works as intended.
• User impact if declined: Glitches for Samsung S22 users after upgrading to Android 14
• Steps to reproduce for manual QE testing: N/A
• Needs manual QE test: no

Comment 8

[Jamie Nicol [:jnicol]]

Attachment: Bug 1869527 - Disable SurfaceControl rendering path.

We are seeing severe rendering glitches using the SurfaceControl
rendering path on Samsung S22 devices. This path was added to allow us
to gracefully recover from GPU process crashes on modern Android
versions (See bug 1762424). However, since bug 1824083 we have an
alternative solution to that problem.

Original Revision: https://phabricator.services.mozilla.com/D196169

Depends on D196262

Comment 9

[Phabricator Automation]

Uplift Approval Request

• Fix verified in Nightly: no
• Is Android affected?: yes
• Code covered by automated testing: yes
• Risk associated with taking this patch: Low
• User impact if declined: Rendering glitches on Samsung S22 devices after Android 14 update
• Steps to reproduce for manual QE testing: N/A
• Needs manual QE test: no
• Explanation of risk level: Moves us away from using an uncommon API to obtain a render surface on to the common path of using the OS-provided Surface directly. This should be much better tested. This is what most apps do and what Firefox did for years until we added the complex path to workaround an OS issue. Since bug 1824083 landed we no longer need that workaround so we can revert to the simple path.
• String changes made/needed: None

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/00826681d51d
Explicitly unparent old SurfaceControls following GPU process crash. r=geckoview-reviewers,owlish

Comment 11

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/00826681d51d

Comment 12

[Jamie Nicol [:jnicol]]

For avoidance of doubt, D196168 does not need uplifted to release. Only D196169.

Comment 13

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/0de7b97945ba