1633432 - Assertion error in RenderAndroidSurfaceTextureHostOGL::NotifyNotUsed running webrender media mochitests on android

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P3)

Description

[Jamie Nicol [:jnicol] out of office until 6th Jan]

I did a try run of webrender mochitests for bug 1585050. In the media suite, we reliably hit this assertion.

Here is the try run, with some logging: https://treeherder.mozilla.org/#/jobs?repo=try&revision=17e19e7ef16ded07e8daa3c110a85fae179b3289

The issue is that a texture was created, and we call PrepareForUse(), but do not call Lock(). This means mPrepareStatus == STATUS_UPDATE_TEX_IMAGE_NEEDED. Then the compositor is paused, and since bug 1626142 that means DetachedFromGLContext() is called which means mAttachedToGLContext = false. Then the texture is destroyed, so NotifyNotUsed() is called, and because mPrepareStatus == STATUS_UPDATE_TEX_IMAGE_NEEDED && !mAttachedToGLContext we hit the assertion.

I think the solution is to set mPrepareStatus = STATUS_NONE in DetachedFromGLContext(). Does that sound right to you, Sotaro?

Comment 1

[Sotaro Ikeda [:sotaro]]

(In reply to Jamie Nicol [:jnicol] from comment #0)

I did a try run of webrender mochitests for bug 1585050. In the media suite, we reliably hit this assertion.

I think the solution is to set mPrepareStatus = STATUS_NONE in DetachedFromGLContext(). Does that sound right to you, Sotaro?

I have a concern that it seems to make mPrepareStatus unpredictable for RenderAndroidSurfaceTextureHostOGL. And also for android::Surface Texture, mSurfTex->UpdateTexImage()/mSurfTex->ReleaseTexImage() needs to be called in RenderAndroidSurfaceTextureHostOGL::NotifyNotUsed().

We call GeckoSurfaceTexture::DetachAllFromGLContext() in RenderCompositorEGL::Pause(). Can we remove the EnsureAttachedToGLContext() call in the Pause()? I added it by mimicking CompositorOGL. But it seems not necessary for RenderCompositorEGL, since it uses shared GL context.

If we remove the call, it seems that we do not need "set mPrepareStatus = STATUS_NONE".

Though, we might need to call the DetachAllFromGLContext() during shutting down a shared gl context.

Comment 2

[Jamie Nicol [:jnicol] out of office until 6th Jan]

(In reply to Sotaro Ikeda [:sotaro] from comment #1)

And also for android::Surface Texture, mSurfTex->UpdateTexImage()/mSurfTex->ReleaseTexImage() needs to be called in RenderAndroidSurfaceTextureHostOGL::NotifyNotUsed().

Do we need to call them so that the Surface/SurfaceTexture buffer is advanced? Do we want to do that when the compositor is paused?

We call GeckoSurfaceTexture::DetachAllFromGLContext() in RenderCompositorEGL::Pause(). Can we remove the EnsureAttachedToGLContext() call in the Pause()? I added it by mimicking CompositorOGL. But it seems not necessary for RenderCompositorEGL, since it uses shared GL context.

We don't call EnsureAttachedToGLContext() in RenderCompositorEGL::Pause(). Do you mean remove the GeckoSurfaceTexture::DetachAllFromGLContext() in RenderCompositorEGL::Pause()? (And presumably remove the RenderThread::NotifyAllAndroidSurfaceTexturesDetatched() call too?)

Is there any circumstance where we do need to be able to detach and attach SurfaceTextures when using SharedGL?

Comment 3

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

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

Comment 4

[Jamie Nicol [:jnicol] out of office until 6th Jan]

Okay, if we stop detaching the SurfaceTextures in pause then obviously it prevents this assertion, and it doesn't seem to cause any other issues.

I'm curious as to why we detach all surface textures in Pause() for the layers compositor in the first place. It seems like we detach all surface textures on Pause, then they are immediately re-attached in SurfaceTextureHost::NotifyNotUsed. Maybe we could try calling EnsureAttached() in RenderAndroidSurfaceTextureHostOGL::NotifyNotUsed, though there might be additional threading issues there.

The detach was added in bug 1470348 for VR. I wouldn't want to add any more obstacles preventing VR from working with webrender. Kip, Randall, do either of you recall why VR needs to detach all surfacetextures on Pause? I couldn't tell from reading that bug or the patch.

Comment 5

[Randall Barker [:rbarker]]

Since Imanol wrote the patch, maybe he has better insight.

Comment 6

[:kip (Kearwood Gilbert)]

Also feel Imanol could answer this better than I. Feel free to NI me again if we need to dig a bit deeper.

Comment 7

[Imanol Fernandez]

In the first version of WebVR we shared the surface textues between the compositor and the VR surface. When we enter VR presentation we stop the Compositor, so we added that call to avoid a atachment ownership between the compositor and the VR render thread. SurfaceTextures can only be attached to one context at a time.

The good news is that we switched to a separate SurfaceTexture set some time ago, so VR doesn't need that DetachAllFromGLContext anymore. It should be pretty safe to rever.t. Anyway I'll make some tests in VR to make sure we are not missing anything.

Comment 8

[Imanol Fernandez]

I have tested WebXR and WebVR without the DetachAllFromGLContext removed and everything works as expected. Feel free to remove that call if you need it to fix this issue :)

Comment 9

[Jamie Nicol [:jnicol] out of office until 6th Jan]

Thanks Imanol, that's very helpful!

In that case, I'm inclined to remove the DetachAllFromGLContext call for both CompositorOGL and RenderCompositorEGL, to keep things consistent.

Comment 10

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

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

For more information, please visit auto_nag documentation.

Comment 11

[Jamie Nicol [:jnicol] out of office until 6th Jan]

Attachment: Bug 1633432 - Don't detach SurfaceTextures from GL context on Pause(). r?sotaro

In bug 1470348 we started to detach all SurfaceTextures from the
current GL context in CompositorOGL::Pause(). This was required for
VR, so that when the VR presentation was entered the SurfaceTextures
could be attached to the VR context instead.

When RenderCompositorEGL was implemented for webrender, we copied the
call to detach from CompositorOGL. However, due to extra complexity in
webrender's threading model, this is causing assertion failures.

VR no longer relies upon the SurfaceTextures being detached when the
compositor is paused, as it now uses its own SurfaceTexture
set. Therefore we can remove the detach call from both
CompositorOGL::Pause and RenderCompositorEGL::Pause.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae83a793e7c9
Don't detach SurfaceTextures from GL context on Pause(). r=sotaro,geckoview-reviewers,snorp,imanol

Comment 13

[Noemi Erli[:noemi_erli]]

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