1774026 - Un-make the EGLSurface current before destroying it in RenderCompositorEGL/OGLSWGL

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, task)

Description

[Jamie Nicol [:jnicol]]

Perhaps bug 1772839 is because the when one compositor is paused the egl surface is still current, so does not get destroyed and detached from the Android Surface. Then when the next compositor is resumed we fail to create the new surface.

Just wild speculation, but worth a try.

Comment 1

[Jamie Nicol [:jnicol]]

Attachment: Bug 1774026 - Make EGL Surface non-current before destroying. r?#gfx-reviewers

This is a purely speculative attempt to fix bug 1772839. The theory
being perhaps that due to the EGL Surface being current,
eglDestroySurface does not detach it from the Android Surface. This
would cause the subsequent eglCreateWindowSurface to fail as the
Android Surface is still attached.

Comment 2

[Jamie Nicol [:jnicol]]

For the record this is what we used to do with layers: CompositorOGL::Pause called GLContextProviderEGL::ReleaseSurface(), which called eglMakeCurrent(EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT).

But then again, we also used to silently ignore the error when we couldn't create a surface, so it's possible the root cause of bug 1772839 has existed for years.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad0d465dcf91
Make EGL Surface non-current before destroying. r=gfx-reviewers,aosmond

Comment 4

[Marian-Vasile Laza]

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

Comment 5

[Jamie Nicol [:jnicol]]

It's definitely too early to tell definitively whether this has worked, but there's been no crashes on nightly in the 24h since it shipped. Let's uplift to beta.

Comment 6

[Jamie Nicol [:jnicol]]

Comment on attachment 9281039 [details]
Bug 1774026 - Make EGL Surface non-current before destroying. r?#gfx-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Crashes for some Android 9 users. Not entirely confident this will fix it but it seems worthwhile trying.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Obviously drivers can have bugs, but there should be no problem calling eglMakeCurrent(NO_SURFACE) before we call eglDestroySurface. - we must call eglMakeCurrent again prior to rendering anything anyway.

This is also what we used to do when using the old layers compositor.

• String changes made/needed:
• Is Android affected?: Yes

Comment 7

[Jamie Nicol [:jnicol]]

Comment on attachment 9281039 [details]
Bug 1774026 - Make EGL Surface non-current before destroying. r?#gfx-reviewers

A couple crashes have come in on nightly since this patch landed, so it doesn't appear to have worked.