Use FrameBuffer's AcquireFence as Layer buffer's ReleaseFence on gonk

RESOLVED FIXED in Firefox 30

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

unspecified
mozilla31
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.4+, firefox29 wontfix, firefox30 fixed, firefox31 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

(Whiteboard: [caf priority: p3][CR 607400])

Attachments

(1 attachment, 5 obsolete attachments)

Assignee

Description

5 years ago
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.
Assignee

Updated

5 years ago
Assignee: nobody → sotaro.ikeda.g
Assignee

Comment 1

5 years ago
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.
Assignee

Updated

5 years ago
Blocks: 962152
This is something to consider if tiling gets us into more OpenGL composition.
Assignee

Updated

5 years ago
Blocks: 950079
Assignee

Updated

5 years ago
No longer blocks: 950079
Assignee

Updated

5 years ago
Status: NEW → ASSIGNED
Assignee

Updated

5 years ago
Depends on: can-tiles
Assignee

Comment 3

5 years ago
recycling in Bug 979489 could be used to fix the problem.
Assignee

Comment 4

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

Updated

5 years ago
Attachment #8386209 - Attachment is patch: true
Attachment #8386209 - Attachment mime type: text/x-patch → text/plain
Assignee

Comment 5

5 years ago
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
Flags: needinfo?(sotaro.ikeda.g)
Assignee

Comment 7

5 years ago
Thanks Jerry, I forgot to think about it! I am going to check if it works.
Flags: needinfo?(sotaro.ikeda.g)
Assignee

Updated

5 years ago
Summary: Investigate to implement a fence to wait next Compositor's eglSwapBuffers() on gonk. → Use FrameBuffer's ReleaseFence as Layer buffer's ReleaseFence on gonk
Assignee

Updated

5 years ago
Attachment #8386209 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8386340 - Attachment is obsolete: true
Assignee

Comment 8

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

Comment 9

5 years ago
Diego, can you confirm that the patch works for the tearing problem like bug 962152.
Flags: needinfo?(dwilson)

Comment 10

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

Updated

5 years ago
No longer depends on: can-tiles
Assignee

Comment 11

5 years ago
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.
Assignee

Updated

5 years ago
Summary: Use FrameBuffer's ReleaseFence as Layer buffer's ReleaseFence on gonk → Use FrameBuffer's AcquireFence as Layer buffer's ReleaseFence on gonk
Assignee

Updated

5 years ago
Attachment #8388267 - Attachment is patch: true
Attachment #8388267 - Attachment mime type: text/x-patch → text/plain
Assignee

Comment 13

5 years ago
(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.
Assignee

Comment 14

5 years ago
FB AcquireFence is preferred than FB ReleaseFence. Wait time could be shorter.
Hi Sotaro,

Both patches fix bug 962152.
Flags: needinfo?(dwilson)
Hi Sotaro,

Both patches fix bug 962152.
Assignee

Updated

5 years ago
No longer blocks: 962152
Assignee

Updated

5 years ago
Duplicate of this bug: 962152
Assignee

Comment 18

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

Updated

5 years ago
Attachment #8387074 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8388267 - Flags: review?(sushilchauhan)
Attachment #8388267 - Flags: review?(nical.bugzilla)
Assignee

Comment 19

5 years ago
This bug prevents a video tearing problem on b2g JB device. It seems better to nominate to 1.4.
blocking-b2g: --- → 1.4?

Comment 20

5 years ago
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.
Attachment #8388267 - Flags: review?(nical.bugzilla) → review+
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.
Blocks: 981532
Flags: needinfo?(sotaro.ikeda.g)

Comment 23

5 years ago
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 24

5 years ago
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?
Assignee

Comment 25

5 years ago
(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.
Flags: needinfo?(sotaro.ikeda.g)
Assignee

Updated

5 years ago
Attachment #8387074 - Attachment is obsolete: false
Assignee

Updated

5 years ago
Attachment #8388267 - Attachment is obsolete: true
Attachment #8388267 - Flags: review?(sushilchauhan)
Assignee

Comment 26

5 years ago
(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.
Assignee

Comment 27

5 years ago
(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.
Assignee

Updated

5 years ago
Attachment #8387074 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8388267 - Attachment is obsolete: false
Assignee

Comment 28

5 years ago
(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.
Assignee

Updated

5 years ago
Attachment #8389722 - Attachment is patch: true
Attachment #8389722 - Attachment mime type: text/x-patch → text/plain
Assignee

Comment 30

5 years ago
(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?
Flags: needinfo?(dwilson)
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.
Attachment #8389722 - Flags: feedback+
blocking-b2g: 1.4? → 1.4+
Flags: needinfo?(dwilson)
Assignee

Comment 32

5 years ago
(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.
Assignee

Updated

5 years ago
Attachment #8389722 - Flags: review?(sushilchauhan)

Comment 33

5 years ago
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.

Updated

5 years ago
Flags: needinfo?(sotaro.ikeda.g)

Updated

5 years ago
Blocks: 960372

Comment 34

5 years ago
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.
Assignee

Comment 35

5 years ago
(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.
Flags: needinfo?(sotaro.ikeda.g)
Assignee

Comment 36

5 years ago
> ::: 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).
Assignee

Comment 37

5 years ago
(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.
Assignee

Comment 38

5 years ago
(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.

Updated

5 years ago
Attachment #8389722 - Flags: review?(sushilchauhan) → review+
Whiteboard: [CR 607400]
https://hg.mozilla.org/mozilla-central/rev/86e2d20a5483
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Whiteboard: [CR 607400] → [caf priority: p3][CR 607400]
You need to log in before you can comment on or make changes to this bug.