On JB gonk, HW composer provides release fence to notify buffer release by HwComposer. But in OpenGL composition case, there is no such thing. One alternate candidate is next Compositor'seglSwapBuffers() call. Investigate to implement a fence kind of thing by uisng it.

In gecko's architecture, related buffer swap between parent and child. Old buffer is send back to child side without waiting for new buffer's rendering complete. This could cause a tearing problem on gonk.

This is something to consider if tiling gets us into more OpenGL composition.

recycling in Bug 979489 could be used to fix the problem.

Extract recycling part from Bug 979489. A patch in the bug could not applied to recent master gecko.

Unbitrot.

(In reply to Sotaro Ikeda [:sotaro] from comment #0) > On JB gonk, HW composer provides release fence to notify buffer release by > HwComposer. But in OpenGL composition case, there is no such thing. One > alternate candidate is next Compositor'seglSwapBuffers() call. Investigate > to implement a fence kind of thing by uisng it. Hi Sotaro, In OpenGL composition case, do we still get the release fence here? http://dxr.mozilla.org/mozilla-central/source/widget/gonk/libdisplay/GonkDisplayJB.cpp?from=gonkdisplayjb.cpp&case=true#274

Thanks Jerry, I forgot to think about it! I am going to check if it works.

Confirmed that the patch works on youtube video tearing problem on nexus-5.

Diego, can you confirm that the patch works for the tearing problem like bug 962152.

Please check this: https://bugzilla.mozilla.org/show_bug.cgi?id=977880 as well.

After re-thinking about attachment 8387074 [details] [diff] [review], I feel that FB AquireFence also seems to work. FB AquireFence request shorter fence wait time. I am going to confirm about it.

(In reply to Sotaro Ikeda [:sotaro] from comment #12) > Created attachment 8388267 [details] [diff] [review] > patch - Use FrameBuffer's Fence as Layer buffer's ReleaseFence on gonk Use FrameBuffer's AcquireFence as Layer buffer's ReleaseFence. AcquireFence says OpenGL composition end. Therefore, it should also works to fix the problem. Confirmed that the patch fix the problem on nexus-5.

FB AcquireFence is preferred than FB ReleaseFence. Wait time could be shorter.

Hi Sotaro, Both patches fix bug 962152.

Hi Sotaro, Both patches fix bug 962152.

attachment 8388267 [details] [diff] [review] is better solution. It require shorter fence time.

This bug prevents a video tearing problem on b2g JB device. It seems better to nominate to 1.4.

Comment on attachment 8388267 [details] [diff] [review] patch - Use FrameBuffer's Fence as Layer buffer's ReleaseFence on gonk Review of attachment 8388267 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/opengl/CompositorOGL.cpp @@ +1392,5 @@ > + // Set FBAcquireFence to child > + ContainerLayer* container = aLayer->AsContainerLayer(); > + if (container) { > + for (Layer* child = container->GetFirstChild(); child; child = child->GetNextSibling()) { > + SetFBAcquireFence(child); I think it should be called only on leaf layers (Visible layers). So this loop should be similar to the loop in ContainerRender() in ContainerLayerComposite. ::: widget/gonk/libdisplay/GonkDisplayJB.cpp @@ +264,5 @@ > #endif > mList->hwLayers[1].displayFrame = r; > mList->hwLayers[1].visibleRegionScreen.numRects = 1; > mList->hwLayers[1].visibleRegionScreen.rects = &mList->hwLayers[1].displayFrame; > + mList->hwLayers[1].acquireFenceFd = mPrevFBAcquireFence->dup(); Similar change is needed in HwcComposer2D::Prepare() as well, since HwcComposer2D::Render() gets called for GPU Composition on devices with HWC.

Comment on attachment 8388267 [details] [diff] [review] patch - Use FrameBuffer's Fence as Layer buffer's ReleaseFence on gonk Review of attachment 8388267 [details] [diff] [review]: ----------------------------------------------------------------- I am not a good review for the Gonk* files so I leave this part to Sushil. The rest looks good to me. ::: gfx/layers/opengl/CompositorOGL.cpp @@ +1394,5 @@ > + if (container) { > + for (Layer* child = container->GetFirstChild(); child; child = child->GetNextSibling()) { > + SetFBAcquireFence(child); > + } > + } nit: it'd be nice if you'd explicitly return at the end of the "if (container) { ... }" block. This would make understanding the flow of this logic easier because then you don't have to check that container layers return an empty RenderState to know whether the logic below is only for leaf layers.

Sotaro, Strangely enough, I see that first patch (attachment 8387074 [details] [diff] [review]) fixes bug 981532. Whereas the latest patch (attachment 8388267 [details] [diff] [review]) does not. Perhaps the fence in attachment 8388267 [details] [diff] [review] is too short.

Sotaro, I think HwcComposer2D::Render() needs to be modified to pass the fence to Prepare(), to have it work on devices which use HWC (see Comment 20).

Comment on attachment 8388267 [details] [diff] [review] patch - Use FrameBuffer's Fence as Layer buffer's ReleaseFence on gonk Review of attachment 8388267 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/opengl/CompositorOGL.cpp @@ +1380,5 @@ > mGLContext->SwapBuffers(); > mGLContext->fBindBuffer(LOCAL_GL_ARRAY_BUFFER, 0); > } > > +#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 17 How about moving this check inside function?

(In reply to Diego Wilson [:diego] from comment #22) > Sotaro, > > Strangely enough, I see that first patch (attachment 8387074 [details] [diff] [review] > [diff] [review]) fixes bug 981532. Whereas the latest patch (attachment > 8388267 [details] [diff] [review]) does not. > > Perhaps the fence in attachment 8388267 [details] [diff] [review] is too > short. Thanks for the feedback! attachment 8387074 [details] [diff] [review] seems better.

(In reply to Nicolas Silva [:nical] from comment #21) > Comment on attachment 8388267 [details] [diff] [review] > patch - Use FrameBuffer's Fence as Layer buffer's ReleaseFence on gonk > > Review of attachment 8388267 [details] [diff] [review]: > ----------------------------------------------------------------- > > I am not a good review for the Gonk* files so I leave this part to Sushil. > The rest looks good to me. > > ::: gfx/layers/opengl/CompositorOGL.cpp > @@ +1394,5 @@ > > + if (container) { > > + for (Layer* child = container->GetFirstChild(); child; child = child->GetNextSibling()) { > > + SetFBAcquireFence(child); > > + } > > + } > > nit: it'd be nice if you'd explicitly return at the end of the "if > (container) { ... }" block. This would make understanding the flow of this > logic easier because then you don't have to check that container layers > return an empty RenderState to know whether the logic below is only for leaf > layers. Will update in a next patch.

(In reply to Sushil from comment #24) > > ::: gfx/layers/opengl/CompositorOGL.cpp > @@ +1380,5 @@ > > mGLContext->SwapBuffers(); > > mGLContext->fBindBuffer(LOCAL_GL_ARRAY_BUFFER, 0); > > } > > > > +#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 17 > > How about moving this check inside function? Hmm, I prefer to define totally different functions, if everything is different.

(In reply to Sotaro Ikeda [:sotaro] from comment #25) > (In reply to Diego Wilson [:diego] from comment #22) > > Sotaro, > > > > Strangely enough, I see that first patch (attachment 8387074 [details] [diff] [review] > > [diff] [review]) fixes bug 981532. Whereas the latest patch (attachment > > 8388267 [details] [diff] [review]) does not. > > > > Perhaps the fence in attachment 8388267 [details] [diff] [review] is too > > short. > > Thanks for the feedback! attachment 8387074 [details] [diff] [review] seems > better. Sorry, from the Sushil's comment. I recognize the problem. It does not work when HWC is enabled. It seems better to reconfirm again after the problem fixed.

Apply comments.

(In reply to Diego Wilson [:diego] from comment #22) > Sotaro, > > Strangely enough, I see that first patch (attachment 8387074 [details] [diff] [review] > [diff] [review]) fixes bug 981532. Whereas the latest patch (attachment > 8388267 [details] [diff] [review]) does not. > > Perhaps the fence in attachment 8388267 [details] [diff] [review] is too > short. Diego, can you confirm the updated patch if the problem fixed?

Comment on attachment 8389722 [details] [diff] [review] patch v2 - Use FrameBuffer's Acquire Fence as Layer buffer's ReleaseFence on gonk(r=nical)) Review of attachment 8389722 [details] [diff] [review]: ----------------------------------------------------------------- The latest patch fixes bug 962152 and bug 981532.

(In reply to Diego Wilson [:diego] from comment #31) > Comment on attachment 8389722 [details] [diff] [review] > patch v2 - Use FrameBuffer's Acquire Fence as Layer buffer's ReleaseFence on > gonk(r=nical)) > > Review of attachment 8389722 [details] [diff] [review]: > ----------------------------------------------------------------- > > The latest patch fixes bug 962152 and bug 981532. Thanks! Good news.

Sotaro, Patch looks fine. Can you please explain how "setting the previous Acquire fence of FB as the Release Fence of layer buffers of current draw cycle" is fixing tearing ? I believe the right approach should be: "To set the FB Release Fence of current draw cycle (which we get from driver after hwc set) as the Release Fence of layer buffers of current draw cycle". Please correct me, if I have missed something.

Comment on attachment 8389722 [details] [diff] [review] patch v2 - Use FrameBuffer's Acquire Fence as Layer buffer's ReleaseFence on gonk(r=nical)) Review of attachment 8389722 [details] [diff] [review]: ----------------------------------------------------------------- I noticed Comment 13. If we are controlling the release of current layer buffer's with previous AcquireFence of FB, then does it make sure that buffers are not released much early, i.e. we need to release these buffers only after OpenGL is done with composing current FB layer with them. Is that the reason, you have added it after "RootLayer()->RenderLayer()" call in LayerManagerComposite ? ::: gfx/layers/opengl/CompositorOGL.cpp @@ +1411,5 @@ > + TextureHostOGL* texture = state.mTexture->AsHostOGL(); > + if (!texture) { > + return; > + } > + texture->SetReleaseFence(new android::Fence(GetGonkDisplay()->GetPrevFBAcquireFd())); At start of function, you can add a check if this new Fence is "Fence::NO_FENCE", then (return) no need to set release fence on any layer buffer. I mean to cover the cases where previous AcquireFence of FB has already been signalled long back. For ex: cases such as GPU - HWC - HWC - HWC - HWC - GPU.

(In reply to Sushil from comment #34) > Comment on attachment 8389722 [details] [diff] [review] > patch v2 - Use FrameBuffer's Acquire Fence as Layer buffer's ReleaseFence on > gonk(r=nical)) > > Review of attachment 8389722 [details] [diff] [review]: > ----------------------------------------------------------------- > > I noticed Comment 13. If we are controlling the release of current layer > buffer's with previous AcquireFence of FB, then does it make sure that > buffers are not released much early, i.e. we need to release these buffers > only after OpenGL is done with composing current FB layer with them. Is that > the reason, you have added it after "RootLayer()->RenderLayer()" call in > LayerManagerComposite ? Yes, I actually added the function call after "mGLContext->SwapBuffers()". It triggers buffer swap and FramebufferSurface::onFrameAvailable() call. The call provices the FB AcquireFence. It should say that OpenGL composition to FB is already complete. Actually we need the layer buffers' release fences. But it is not provided by OpenGL. Instead, using FB AcquireFence. The FB AcquireFence should come after the layer buffers' release fences.

> ::: gfx/layers/opengl/CompositorOGL.cpp > @@ +1411,5 @@ > > + TextureHostOGL* texture = state.mTexture->AsHostOGL(); > > + if (!texture) { > > + return; > > + } > > + texture->SetReleaseFence(new android::Fence(GetGonkDisplay()->GetPrevFBAcquireFd())); > > At start of function, you can add a check if this new Fence is > "Fence::NO_FENCE", then (return) no need to set release fence on any layer > buffer. I mean to cover the cases where previous AcquireFence of FB has > already been signalled long back. For ex: cases such as GPU - HWC - HWC - > HWC - HWC - GPU. Thanks. I am going to update it tomorrow(Monday).

(In reply to Sotaro Ikeda [:sotaro] from comment #35) > > Yes, I actually added the function call after "mGLContext->SwapBuffers()". > It triggers buffer swap and FramebufferSurface::onFrameAvailable() call. The > call provices the FB AcquireFence. It should say that OpenGL composition to > FB is already complete. Actually we need the layer buffers' release fences. > But it is not provided by OpenGL. Instead, using FB AcquireFence. The FB > AcquireFence should come after the layer buffers' release fences. The FB AcquireFence complete says that no more OpenGL rendering to FB(drawing complete). Therefore there seems no problem to reuse the layer buffers.

(In reply to Sotaro Ikeda [:sotaro] from comment #36) > > ::: gfx/layers/opengl/CompositorOGL.cpp > > @@ +1411,5 @@ > > > + TextureHostOGL* texture = state.mTexture->AsHostOGL(); > > > + if (!texture) { > > > + return; > > > + } > > > + texture->SetReleaseFence(new android::Fence(GetGonkDisplay()->GetPrevFBAcquireFd())); > > > > At start of function, you can add a check if this new Fence is > > "Fence::NO_FENCE", then (return) no need to set release fence on any layer > > buffer. I mean to cover the cases where previous AcquireFence of FB has > > already been signalled long back. For ex: cases such as GPU - HWC - HWC - > > HWC - HWC - GPU. > > Thanks. I am going to update it tomorrow(Monday). I checked the above on nexus-5. There is no case that "Fence::NO_FENCE" is used. Even in the following case, valid FB AcquireFence is provided. The AcquireFence for the last GPU composition is used for it. - GPU - HWC - HWC - HWC - HWC - GPU.

Fix build failure on linux.

https://hg.mozilla.org/integration/b2g-inbound/rev/86e2d20a5483