879297 - ImageLayerOGL keeps lock on external buffers

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Diego Wilson [:diego]]

B2G video playback and camera preview are rendered by binding an external buffer to a GL texture in ImageLayerOGL.

https://mxr.mozilla.org/mozilla-b2g18/source/gfx/layers/opengl/ImageLayerOGL.cpp#969

This binding can lock the external buffer until it is unbound. If someone tries to reuse the buffer before it's unbound/unlocked can cause some locking errors like this:

E/libgenlock(  132): perform_lock_unlock_operation: GENLOCK_IOC_DREADLOCK failed (lockType0x1,        >err=Connection timed out fd=31)
E/omx_vdec(  132): Failed to acquire genlock, ret = 1

Comment 1

[Diego Wilson [:diego]]

I have a working patch that I'll share ASAP

Comment 2

[Jason Smith [:jsmith]]

What's the impact towards YouTube videos here? What's the end user impact?

Comment 3

[Diego Wilson [:diego]]

(In reply to Jason Smith [:jsmith] from comment #2)
> What's the impact towards YouTube videos here? What's the end user impact?

If this isn't fixed, YouTube videos freeze on a single frame mid-way while audio continues.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This will fix 878977 right?

Comment 5

[Diego Wilson [:diego]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> This will fix 878977 right?

Kinda. It will fix the screen freeze caused as a side effect of bug 878977.

Bug 878977 will still make it fall back to GPU when the "network activity" icon blinks, which is bad for power performance. Not ideal, but not a blocker either.

Comment 6

[Diego Wilson [:diego]]

OK, the solution is not as straightforward as I thought.

My first thought was unbinding the buffer after rendering using this:

https://mxr.mozilla.org/mozilla-b2g18/source/gfx/gl/GLContextProviderEGL.cpp#448

But that still sometimes fails to unlock the buffer!

I'll keep trying. Suggestions are welcome.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Indeed, BindExternalBuffer and UnbindExternalBuffer were extremely confusing as, contrary to what their names suggested, they were not doing the converse of each other --- one was operating on TEXTURE_2D and the other on TEXTURE_EXTERNAL. So they have been mercilessly axed from mozilla-central. I suppose that the present bug is b2g-v1, I'll try to look at this tomorrow.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Ah, I've looked at the code link in comment 0 now --- ImageLayerOGL.cpp:969. Hg history suggests that the right person to ask is Kan-Ru

     kanru: #ifdef MOZ_WIDGET_GONK
     kanru:     if (img
     kanru:         && (img->type() == SharedImage::TSurfaceDescriptor)
     kanru:         && (img->get_SurfaceDescriptor().type() == SurfaceDescriptor::TSurfaceDescriptorGralloc)) {
     kanru:         const SurfaceDescriptorGralloc& desc = img->get_SurfaceDescriptor().get_SurfaceDescriptorGralloc();
     kanru:         sp<GraphicBuffer> graphicBuffer = GrallocBufferActor::GetFrom(desc);
     kanru:         mSize = gfxIntSize(graphicBuffer->getWidth(), graphicBuffer->getHeight());
    sikeda:         mPictureRect = nsIntRect(0, 0, desc.size().width, desc.size().height);
     kanru:         if (!mExternalBufferTexture.IsAllocated()) {
     kanru:           mExternalBufferTexture.Allocate(gl());
     kanru:         }
     kanru:         gl()->MakeCurrent();
     kanru:         gl()->fActiveTexture(LOCAL_GL_TEXTURE0);
     kanru:         gl()->BindExternalBuffer(mExternalBufferTexture.GetTextureID(), graphicBuffer->getNativeBuffer());
     kanru:         mImageVersion = imgVersion;
     kanru:     }
     kanru: #endif

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Is the buffer being kept alive by the GPU?  Though the really hardcore unbind (with a teximage2d) is likely going to cause a pipeline flush anyway.

Comment 10

[Kan-Ru Chen [:kanru] (UTC+9)]

Per discussed with chiajung I think we should unlock the buffer before return it to the child. I'll check this today.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #9)
> Is the buffer being kept alive by the GPU?  Though the really hardcore
> unbind (with a teximage2d) is likely going to cause a pipeline flush anyway.

If you're refering to what UnbindExternalBuffer does, it actually doesn't do anything here because of a texture target mismatch (2D != EXTERNAL) just being an INVALID_OPERATION.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I meant if it was actually done on the proper target :)

Comment 13

[Kan-Ru Chen [:kanru] (UTC+9)]

We should unbind and unlock gralloc buffer before this point

https://mxr.mozilla.org/mozilla-b2g18/source/gfx/layers/ipc/ImageContainerParent.cpp#33

Comment 14

[Diego Wilson [:diego]]

(In reply to Kan-Ru Chen (:kanru) (Mozilla Corporation) from comment #13)
> We should unbind and unlock gralloc buffer before this point
> 
> https://mxr.mozilla.org/mozilla-b2g18/source/gfx/layers/ipc/
> ImageContainerParent.cpp#33

Thanks Kan-Ru. Now I know the "where". Just to figure out the "how".

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #12)
> I meant if it was actually done on the proper target :)

If this answers your question: the GPU will definitely be holding on to any buffer that it is sampling from, but that won't cause a lock failure (because different handle), instead that will cause content to wait to acquire its next lock on it.

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Diego Wilson [:diego] from comment #14)
> (In reply to Kan-Ru Chen (:kanru) (Mozilla Corporation) from comment #13)
> > We should unbind and unlock gralloc buffer before this point
> > 
> > https://mxr.mozilla.org/mozilla-b2g18/source/gfx/layers/ipc/
> > ImageContainerParent.cpp#33
> 
> Thanks Kan-Ru. Now I know the "where". Just to figure out the "how".

Indeed the "how" is a bit nontrivial as the EGL API does not have a way of explicitly undoing the effect of a glEGLImageTargetTexture2D call.

I know 2 ways that you can still achieve this:
 - 1. either bind another EGLImage to that GL texture
 - 2. or delete that GL texture (glDeleteTextures).

Way 2. is of course a bit scary to do frequently, so try to do 1. instead. If you look at Android's SurfaceTexture, what is being done is that the same GL texture gets reused so that when a new GraphicBuffer is presented, the previous one automatically gets unlocked (as the new glEGLImageTargetTexture2D undoes the effect of the previous one).

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We're on a tight schedule here so can we all please give as much information as possible instead of just dribbling it out a little at a time? Ideally in the form of a suggested patches?

How would we"bind another EGLImage to that GL texture"? What's the best way to create such an image?

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why is glDeleteTextures scary to do frequently?

Can we use a single EGLImage for this and bind it to every GL texture?

Why would texImage2D on the bound texture cause a GPU pipeline flush?

Comment 19

[Diego Wilson [:diego]]

It appears the only safe way to force an unbind is to bind to another buffer. Using a dummy buffer like bug 775649 right after the draw worked well for me. Unless there's any objections with this approach I'll create a patch.

P.s. I was sure I had already added this comment earlier today. If you see it in another bug pleas ignore it :)

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Please create a patch!

Comment 21

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Patch for b2g18

Could you test this patch?

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated patch

How about this version? I think we can avoid adding glFlush after painting each layer, by deferring the unbinding until after LayerManagerOGL has completed rendering and done its own glFlush.

Testing this now.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 759074 [details] [diff] [review]
updated patch

Review of attachment 759074 [details] [diff] [review]:
-----------------------------------------------------------------

Seems to work OK. Thanks Kan-Ru!

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It would be great to have Diego test this too :-)

Comment 25

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 759074 [details] [diff] [review]
updated patch

Review of attachment 759074 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, this should work.

Important here is that both mDummyGrallocBuffer and mExternalBufferTexture are members of the same class, so we have a 1:1 mapping between them. For a while in mozilla-central (no longer) we had a single "dummy gralloc buffer" for all textures and this ran into slow paths on certain phones' drivers (at least the Peak). Anyway, good that this isn't being done here.

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +1147,5 @@
>  
>    mGLContext->fDisableVertexAttribArray(vcattr);
>    mGLContext->fDisableVertexAttribArray(tcattr);
>  
>    mGLContext->fFlush();

I realize that this is pre-existing code, but I have to ask what is this glFlush call supposed to achieve? The OpenGL specification doesn't say anything definite about glFlush --- it is entirely up to the implementation whether it is a no-operation or not. So any call to glFlush should be accompanied by a comment explaining on what driver it was found to have a useful effect there.

Where a glFlush is used, generally what is wanted instead is either a WaitSync, or a glFinish.

::: gfx/layers/opengl/LayerManagerOGL.h
@@ +414,5 @@
>    /** Region we're clipping our current drawing to. */
>    nsIntRegion mClippingRegion;
>  
> +  /** List of LayerOGLs that need a post-flush callback */
> +  nsTArray<LayerOGL*> mPostFlushCallbacks;

Since it looks like this array will constantly be grown to a small size and then cleared, this is probably a good use case for nsAutoTArray ...?

Comment 26

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> Why is glDeleteTextures scary to do frequently?

Because textures (even without any image data) are not so lightweight objects and it is scary, from a performance perspective, to be creating and destroying them all the time. We had in-depth conversations about this in Taipei as we were still struggling to solve that problem on mozilla-central then (it's now been solved by adopting SurfaceTexture's design, see bug 875211.

> 
> Can we use a single EGLImage for this and bind it to every GL texture?

That is what we have been doing for a little while on mozilla-central before we changed it in bug 875211, and that is what turned out to run slowly in certain phones' drivers, like the Peak, see bug 869696.

> 
> Why would texImage2D on the bound texture cause a GPU pipeline flush?

It depends on fine details of the GPU, like whether it is an immediate or a deferred GPU. I still don't know for sure the extent to which Adreno's are deferred, but have heard several people saying that they are. On a deferred GPU, you can issue GL calls for a frame while the previous frame is still being rendered in fragment shaders. In that case, issuing a texImage2D call on a texture that is still being sampled for the previous frame causes the pipeline to stall. I have been trying to assemble some documentation on deferred GPUs in this wiki page:
   https://wiki.mozilla.org/Platform/GFX/DeferredGPUs
(this page boldly claims that Adrenos are deferred. Again, I am not certain of that).

Comment 27

[Benoit Jacob [:bjacob] (mostly away)]

The page moved to: https://wiki.mozilla.org/Platform/GFX/MobileGPUs

Comment 28

[Benoit Jacob [:bjacob] (mostly away)]

Wait, I r+'d this a little too soon. Please explain why it is safe for the LayerManager to have this array of plain pointers to Layers? Is it guaranteed that Layers cannot go away between the time when the raw pointer is recorded and the time when it is dereferenced and cleared?

Comment 29

[Diego Wilson [:diego]]

Comment on attachment 759074 [details] [diff] [review]
updated patch

Review of attachment 759074 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +1153,5 @@
> +
> +  for (uint32_t i = 0; i < mPostFlushCallbacks.Length(); ++i) {
> +    mPostFlushCallbacks[i]->InvokePostFlushCallback();
> +  }
> +  mPostFlushCallbacks.Clear();

On my device LayerManagerOGL::Render() returns earlier here:

https://mxr.mozilla.org/mozilla-b2g18/source/gfx/layers/opengl/LayerManagerOGL.cpp#1071

But if I paste the chunk above there it works. So LGTM with that.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Benoit Jacob [:bjacob] from comment #25)
> I realize that this is pre-existing code, but I have to ask what is this
> glFlush call supposed to achieve? The OpenGL specification doesn't say
> anything definite about glFlush --- it is entirely up to the implementation
> whether it is a no-operation or not. So any call to glFlush should be
> accompanied by a comment explaining on what driver it was found to have a
> useful effect there.
> 
> Where a glFlush is used, generally what is wanted instead is either a
> WaitSync, or a glFinish.

Yeah, existing code, I don't know, we're definitely not taking it out on the 18 branch :-).

> ::: gfx/layers/opengl/LayerManagerOGL.h
> @@ +414,5 @@
> >    /** Region we're clipping our current drawing to. */
> >    nsIntRegion mClippingRegion;
> >  
> > +  /** List of LayerOGLs that need a post-flush callback */
> > +  nsTArray<LayerOGL*> mPostFlushCallbacks;
> 
> Since it looks like this array will constantly be grown to a small size and
> then cleared, this is probably a good use case for nsAutoTArray ...?

Actually the most common size for this array is probably 0. I don't think this matters much either way.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Benoit Jacob [:bjacob] from comment #28)
> Wait, I r+'d this a little too soon. Please explain why it is safe for the
> LayerManager to have this array of plain pointers to Layers? Is it
> guaranteed that Layers cannot go away between the time when the raw pointer
> is recorded and the time when it is dereferenced and cleared?

It's supposed to be, but Diego pointed out that it's not in all cases. So I'll fix that up.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: v2

Comment 33

[Benoit Jacob [:bjacob] (mostly away)]

Questions for the review:

Can you clarify comment 31? What does "i'll fix that up" mean?

Also, how would you feel about having these pointers be nsRefPtr's? Since layers aren't cycle-collected, and as far as I can see in Layers.h they use NS_INLINE_DECL_REFCOUNTING, addref/release'ing them is cheap.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Benoit Jacob [:bjacob] from comment #33)
> Can you clarify comment 31? What does "i'll fix that up" mean?

I'll make sure that the post-render callbacks are called (and the list cleared) on every possible exit path from Render(). Patch v2 does this by doing it right after the (sole) call to Render.

> Also, how would you feel about having these pointers be nsRefPtr's? Since
> layers aren't cycle-collected, and as far as I can see in Layers.h they use
> NS_INLINE_DECL_REFCOUNTING, addref/release'ing them is cheap.

Unnecessary. Layers can't be deleted during Render(), that would break the world.

Comment 35

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 759484 [details] [diff] [review]
v2

Review of attachment 759484 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +702,1 @@
>        Render();

Does this patch rely on this being the only place where Render() is called? I have only checked that it is so in this file and in the .h file. I don't know about other files. Please check/confirm.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes it does. That's OK I think.

Comment 37

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 759484 [details] [diff] [review]
v2

Review of attachment 759484 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/ImageLayerOGL.cpp
@@ +965,5 @@
>            mExternalBufferTexture.Allocate(gl());
>          }
> +        if (!mDummyGrallocBuffer.get()) {
> +          mDummyGrallocBuffer = new GraphicBuffer(2, 2, HAL_PIXEL_FORMAT_YV12,
> +                                                  GraphicBuffer::USAGE_HW_TEXTURE);

I forgot to add the comment "For YV12 the size of the buffer has to be at least 2x2"

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +702,1 @@
>        Render();

It's important that ImageContainerParent::RecvPublishImage runs on the same thread of the Compositor, otherwise we might return the image to the child too early. Need to document this at somewhere.

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Thanks Kan-Ru. Pushed with your comments addressed.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/cd9f8960933c

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Benoit, can you port this to central? Everything's utterly changed with the layers refactoring so the patch basically has to be rewritten.

Comment 40

[Benoit Jacob [:bjacob] (mostly away)]

Central should not have any such problem anymore --- we ran into many such problems there during layers-refactoring, but for all I know, these problems are behind us now. And since Nical's work in bug 875211, we even have adopted the nice idea from Android's SurfaceTexture so we don't even need dummy GraphicBuffers to bind anymore.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Calling this fixed based on comment 40.

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/cd9f8960933c
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/cedffa70cd98

Comment 42

[Jeff Muizelaar [:jrmuizel]]

(In reply to Benoit Jacob [:bjacob] from comment #40)
> Central should not have any such problem anymore --- we ran into many such
> problems there during layers-refactoring, but for all I know, these problems
> are behind us now. And since Nical's work in bug 875211, we even have
> adopted the nice idea from Android's SurfaceTexture so we don't even need
> dummy GraphicBuffers to bind anymore.

How sure of this are you? I'm seeing a lot of genlock contention with h264 playback on master, and I'm wondering if this would help.