1326376 - Crash at GLScreenBuffer::AssureBlitted while clone CanvasClientSharedSurface

Status: RESOLVED WORKSFORME

(Core :: Graphics, defect)

Description

[Daosheng Mu[:daoshengmu]]

This only happens in Nightly 53.0a1 Debug version when open https://webvr.info/samples/XX-vr-controllers.html and press Enter VR icon.

Callstack:

if (mDebugFlags & DebugFlagAbortOnError) {
  MOZ_CRASH("Unexpected error with MOZ_GL_DEBUG_ABORT_ON_ERROR. (Run"
                              " with MOZ_GL_DEBUG_ABORT_ON_ERROR=0 to disable)");
}

 xul.dll!mozilla::gl::GLContext::AfterGLCall(const char * funcName) Line 770	C++
 xul.dll!mozilla::gl::GLContext::raw_fBlitFramebuffer(int srcX0, int srcY0, int srcX1, int srcY1, int dstX0, int dstY0, int dstX1, int dstY1, unsigned int mask, unsigned int filter) Line 2422	C++
 xul.dll!mozilla::gl::GLScreenBuffer::AssureBlitted() Line 456	C++
 xul.dll!mozilla::gl::GLContextEGL::SetEGLSurfaceOverride(void * surf) Line 317	C++
 xul.dll!mozilla::gl::SharedSurface_ANGLEShareHandle::LockProdImpl() Line 117	C++
 xul.dll!mozilla::gl::SharedSurface::LockProd() Line 236	C++
 xul.dll!mozilla::gl::SharedSurface::ProdCopy(mozilla::gl::SharedSurface * src, mozilla::gl::SharedSurface * dest, mozilla::gl::SurfaceFactory * factory) Line 136	C++
 xul.dll!mozilla::gl::SharedSurface::ProdCopy(mozilla::gl::SharedSurface * src, mozilla::gl::SharedSurface * dest, mozilla::gl::SurfaceFactory * factory) Line 45	C++
 xul.dll!mozilla::layers::CloneSurface(mozilla::gl::SharedSurface * src, mozilla::gl::SurfaceFactory * factory) Line 372	C++
xul.dll!mozilla::layers::CanvasClientSharedSurface::UpdateRenderer(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> aSize, mozilla::MaybeOneOf<mozilla::layers::ClientCanvasLayer *,mozilla::layers::AsyncCanvasRenderer *> & aRenderer) Line 418	C++
xul.dll!mozilla::layers::CanvasClientSharedSurface::Update(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> aSize, mozilla::layers::ClientCanvasLayer * aLayer) Line 384	C++
xul.dll!mozilla::layers::ClientCanvasLayer::RenderLayer() Line 143	C++
xul.dll!mozilla::layers::ClientLayer::RenderLayerWithReadback(mozilla::layers::ReadbackProcessor * aReadback) Line 361	C++
xul.dll!mozilla::layers::ClientContainerLayer::RenderLayer() Line 62	C++
xul.dll!mozilla::layers::ClientLayerManager::EndTransactionInternal(void(*)(mozilla::layers::PaintedLayer *, gfxContext *, const mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> &, const mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> &, mozilla::layers::DrawRegionClip, const mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> &, void *) aCallback, void * aCallbackData, mozilla::layers::LayerManager::EndTransactionFlags __formal) Line 326	C++
 xul.dll!mozilla::layers::ClientLayerManager::EndTransaction(void(*)(mozilla::layers::PaintedLayer *, gfxContext *, const mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> &, const mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> &, mozilla::layers::DrawRegionClip, const mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> &, void *) aCallback, void * aCallbackData, mozilla::layers::LayerManager::EndTransactionFlags aFlags) Line 378	C++
 xul.dll!nsDisplayList::PaintRoot(nsDisplayListBuilder * aBuilder, nsRenderingContext * aCtx, unsigned int aFlags) Line 2068	C++
 xul.dll!nsLayoutUtils::PaintFrame(nsRenderingContext * aRenderingContext, nsIFrame * aFrame, const nsRegion & aDirtyRegion, unsigned int aBackstop, nsDisplayListBuilderMode aBuilderMode, nsLayoutUtils::PaintFrameFlags aFlags) Line 3674	C++
 xul.dll!mozilla::PresShell::Paint(nsView * aViewToPaint, const nsRegion & aDirtyRegion, unsigned int aFlags) Line 6352	C++

Comment 1

[Daosheng Mu[:daoshengmu]]

[gl:1D913800] mozilla::gl::GLContext::raw_fBlitFramebuffer: Generated unexpected GL_INVALID_OPERATION error.

Comment 2

[Daosheng Mu[:daoshengmu]]

I notice the reason of this GL_INVALID_OPERATION is because our DrawBuffer and ReadBuffer does not match on multi-samples. It only happen when ReadBuffer's attachmentType is Screen.

We can find it has been mentioned on OpenGL ES 3 spec, https://www.khronos.org/opengles/sdk/docs/man3/html/glBlitFramebuffer.xhtml,

GL_INVALID_OPERATION is generated if the value of GL_SAMPLE_BUFFERS for the draw buffer is greater than zero.

GL_INVALID_OPERATION is generated if GL_SAMPLE_BUFFERS for the read buffer is greater than zero and the formats of draw and read buffers are not identical, or the source and destination rectangles are not defined with the same (X0, Y0) and (X1, Y1) bounds.

If I make caps.antialias to be false here, https://dxr.mozilla.org/mozilla-central/rev/6a23526fe5168087d7e4132c0705aefcaed5f571/gfx/gl/GLScreenBuffer.cpp#852, it can resolve our problem, but we would see some aliasing on our screen that would not we want. So, I purpose if we should let ReadBuffer which has screen attachmentType can generate a new buffer?

Comment 3

[Kelsey Gilbert [:jgilbert]]

GLScreenBuffer is our way of pretending to have a natively multisampled backbuffer.
In the case of no antialiasing, we never blit, because there's no reason to have a separate virtual read and draw buffer for the backbuffer.

I suspect ANGLE (this is on ANGLE?) is rejecting it due to a format mismatch. We shouldn't ever have a mismatch though. Please dig into this more, stepping into ANGLE to figure out exactly why they're rejecting the command.

Comment 4

[Daosheng Mu[:daoshengmu]]

It caused by IsPartialBlit() at ANGLE for checking if the dstSize is equal to the writeBuffer size. The dstSize here is (3024, 1680) and writeBuffer is (1268, 845). (1268, 845) is our current back buffer size that is from the last swap(). But in WebVR examples, we will change the webglCanvas size when onResize() and try to blit at https://dxr.mozilla.org/mozilla-central/rev/a3978751f45108ff1ae002ecebdc0fa23fc52b84/gfx/gl/GLContextProviderEGL.cpp#314. But it didn't swap buffer yet, it will have the previous back buffer and would make us fail when checking the back buffer size is equal to the write buffer size.

Therefore, I suppose we should check if the size is equal to our front buffer size at CanvasClientSharedSurface::UpdateRenderer() before doing this bltting.

Comment 5

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1326376 - Using BlitTextureToFramebuffer() when the sizes are different;

Review commit: https://reviewboard.mozilla.org/r/106100/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106100/

Comment 6

[Daosheng Mu[:daoshengmu]]

(In reply to Daosheng Mu[:daoshengmu] from comment #4)
> It caused by IsPartialBlit() at ANGLE for checking if the dstSize is equal
> to the writeBuffer size. The dstSize here is (3024, 1680) and writeBuffer is
> (1268, 845). (1268, 845) is our current back buffer size that is from the
> last swap(). But in WebVR examples, we will change the webglCanvas size when
> onResize() and try to blit at
> https://dxr.mozilla.org/mozilla-central/rev/
> a3978751f45108ff1ae002ecebdc0fa23fc52b84/gfx/gl/GLContextProviderEGL.cpp#314.
> But it didn't swap buffer yet, it will have the previous back buffer and
> would make us fail when checking the back buffer size is equal to the write
> buffer size.
> 
> Therefore, I suppose we should check if the size is equal to our front
> buffer size at CanvasClientSharedSurface::UpdateRenderer() before doing this
> bltting.

I think it would happen when any canvas would like to do resize.
Probably, we can consider make (gl->Screen()->Front()->GetSize() == aSize) checking before all CloneSurface cases.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8828799 [details]
Bug 1326376 - Using BlitTextureToFramebuffer() when the sizes are different;

https://reviewboard.mozilla.org/r/106100/#review110414

CloneSurface shouldn't care what size things are.
It sounds like there's a bug in CloneSurface.

Comment 8

[Michael Leu[:Lenzak](UTC+8)]

Since newer ANGLE reject to clone surface when src and dst have different size.

I think CloneSurface should check whether its src and dst size are equal or not.

If size mismatch, it can just return nullptr and UpdateRenderer can just handle it.

Comment 9

[Kelsey Gilbert [:jgilbert]]

CloneSurface makes a new surface that's a clone of an existing one.
CloneSurface chooses what size to make the new destination surface, and evidently chooses wrong right now.
Please make CloneSurface choose the correct size to match the surface it's cloning.

Comment 10

[Daosheng Mu[:daoshengmu]]

This is only happened on HTC Vive, Oculus Rift is fine.

Comment 11

[Daosheng Mu[:daoshengmu]]

(In reply to Daosheng Mu[:daoshengmu] from comment #10)
> This is only happened on HTC Vive, Oculus Rift is fine.

Correct my comment. Oculus Rift happens it as well, and its dst buffer size is (2688, 1600) srcSize is (1268, 845)

Comment 12

[Daosheng Mu[:daoshengmu]]

ValidateBlitFramebufferANGLE() will check whether the dest size and src size are the same. If the size is different, BlitFramebufferANGLE() would be return. Therefore, we should consider the case of the size is different and change to BlitTextureToFramebuffer() method.

Comment 13

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8828799 [details]
Bug 1326376 - Using BlitTextureToFramebuffer() when the sizes are different;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106100/diff/1-2/

Comment 14

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8828799 [details]
Bug 1326376 - Using BlitTextureToFramebuffer() when the sizes are different;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106100/diff/2-3/

Comment 15

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8828799 [details]
Bug 1326376 - Using BlitTextureToFramebuffer() when the sizes are different;

https://reviewboard.mozilla.org/r/106100/#review125532

::: gfx/gl/GLBlitHelper.cpp:922
(Diff revision 3)
>  {
>      MOZ_ASSERT(mGL->fIsTexture(srcTex));
>      MOZ_ASSERT(!destFB || mGL->fIsFramebuffer(destFB));
>  
> -    if (mGL->IsSupported(GLFeature::framebuffer_blit)) {
> +    if (mGL->IsSupported(GLFeature::framebuffer_blit)
> +        && (srcSize == destSize)) {

The sizes resulting from CloneSurface should never be different. How is this supposed to fix the root bug here?

::: gfx/layers/client/CanvasClient.cpp:413
(Diff revision 3)
>    gl->MakeCurrent();
>  
>    RefPtr<TextureClient> newFront;
>  
>    if (layer && layer->mGLFrontbuffer) {
> -    mShSurfClient = CloneSurface(layer->mGLFrontbuffer.get(), layer->mFactory.get());
> +    mShSurfClient = CloneSurface(layer->mGLFrontbuffer.get(), aSize, layer->mFactory.get());

CloneSurface should be making a full-size copy of the surface. Why are you adding a size parameter?

Comment 16

[Daosheng Mu[:daoshengmu]]

(In reply to Jeff Gilbert [:jgilbert] from comment #15)
> Comment on attachment 8828799 [details]
> Bug 1326376 - Using BlitTextureToFramebuffer() when the sizes are different;
> 
> https://reviewboard.mozilla.org/r/106100/#review125532
> 
> ::: gfx/gl/GLBlitHelper.cpp:922
> (Diff revision 3)
> >  {
> >      MOZ_ASSERT(mGL->fIsTexture(srcTex));
> >      MOZ_ASSERT(!destFB || mGL->fIsFramebuffer(destFB));
> >  
> > -    if (mGL->IsSupported(GLFeature::framebuffer_blit)) {
> > +    if (mGL->IsSupported(GLFeature::framebuffer_blit)
> > +        && (srcSize == destSize)) {
> 
> The sizes resulting from CloneSurface should never be different. How is this
> supposed to fix the root bug here?
> 

It happens on the width or height attributes of HTMLCanvasElement is changed, and the WebGLContext will change its dimension. Then, WebGLContext must resize its backbuffer. As I said at Comment 4, the dst buffer is a new size (3024, 1680) and is different to the original size (1268, 845). Therefore, I suppose we should allow CloneSurface can support copying to a different size surface.

Comment 17

[Kelsey Gilbert [:jgilbert]]

Oook, I think I see what the issue is.

I believe the issue is that we call fBlitFramebuffer, which calls GLScreenBuffer::AfterDrawCall(). However, AfterDrawCall checks against mUserDrawFB, not mInternalDrawFB. This may be causing AfterDrawCall to mark our blit in GLBlitHelper::BlitFramebufferToFramebuffer with RequireBlit while we temporarily have a different EGLSurfaceOverride set. The subsequent attempt to lock the correct EGLSurfaceOverride incurs AssureBlitted, which will fail due to differing sizes.

Actually, with AttachmentType::Screen, checking mInternalDrawFB isn't even enough, since it will be 0 for the temporary EGLSurfaceOverride as well.

We should probably drop the BindInternal stuff and have a GLScreenBuffer::ScopedBypass.

Comment 18

[Daosheng Mu[:daoshengmu]]

WebVR has already sent SurfaceDescriptor to GPU process instead of copying the sharedSurface. So, this bug would not be happened. We can close it.