1154557 - Generate fence for each CompositiableHost to replace the FrameBufferSurface's fence

Status: RESOLVED INVALID

(Core :: Graphics: Layers, enhancement)

Description

[Ethan Lin[:ethlin]]

When we use the CompositorOGL to compose, we wait the final FrameBufferSurface's fence in each compositable on the content side. I think we can create egl fence for each compositable during composition and the each compositable should wait the self fence. This way should be faster.
I will upload a patch to do this.

Comment 1

[Ethan Lin[:ethlin]]

Attachment: Part 1 - Generate fence for CompositableHost

Generate the fence for CompositableHost.

Comment 2

[Ethan Lin[:ethlin]]

Attachment: Part 2 - Use CompositableHost fence to replace the original fence

After generating the CompositableHost's fence, we can send it to content side to replace the FrameBufferSurface's fence.

Comment 3

[Ethan Lin[:ethlin]]

Attachment: Part 3 - Remove the rest unused functions

Since the content side use the CompositableHost's fence, we can remove the original functions about setting FrameBufferSurface fence.

Comment 4

[Chiajung Hung [:chiajung]]

Comment on attachment 8592590 [details] [diff] [review]
Part 1 - Generate fence for CompositableHost

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

Please fix the comment, and request feedback again.

::: gfx/gl/GLConsts.h
@@ +5286,5 @@
>  #define LOCAL_EGL_NO_RESET_NOTIFICATION_EXT                  0x31BE
>  #define LOCAL_EGL_NO_RESET_NOTIFICATION_KHR                  0x31BE
>  #define LOCAL_EGL_NO_STREAM_KHR                              ((EGLStreamKHR)0)
>  #define LOCAL_EGL_NO_SURFACE                                 ((EGLSurface)0)
> +#define LOCAL_EGL_NO_SYNC_KHR                                ((EGLSync)0)

Why to remove KHR?

The EGL extension object should have KHR.

::: gfx/layers/opengl/GrallocTextureHost.cpp
@@ +250,5 @@
>    return surf.forget();
>  }
>  
> +void GrallocTextureHostOGL::Fence()
> +{

Please ASSERT thread here before using any gl call.
GLContext should be treat as thread local.

@@ +262,5 @@
> +  if (gl->Renderer() == GLRenderer::AdrenoTM200) {
> +      return;
> +  }
> +
> +  if (sEGLLibrary.IsExtensionSupported(GLLibraryEGL::ANDROID_native_fence_sync)) {

Wrong extension!
This extension is for dup. You should use KHR_fence_sync for eglCreateSync.

Comment 5

[Chiajung Hung [:chiajung]]

Comment on attachment 8592592 [details] [diff] [review]
Part 2 - Use CompositableHost fence to replace the original fence

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

Some common GL programming practice fix.

::: gfx/layers/opengl/GrallocTextureHost.cpp
@@ +290,5 @@
>  }
>  
> +FenceHandle
> +GrallocTextureHostOGL::GetAndResetReleaseFenceHandle()
> +{

Assert gl thread here.

@@ +293,5 @@
> +GrallocTextureHostOGL::GetAndResetReleaseFenceHandle()
> +{
> +  FenceHandle handle;
> +  if (mSync != LOCAL_EGL_NO_SYNC_KHR) {
> +    int fenceFd = sEGLLibrary.fDupNativeFenceFDANDROID(EGL_DISPLAY(), mSync);

Check extension before using it.

Comment 6

[Chiajung Hung [:chiajung]]

Comment on attachment 8592594 [details] [diff] [review]
Part 3 - Remove the rest unused functions

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

LGTM.

Comment 7

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8592590 [details] [diff] [review]
Part 1 - Generate fence for CompositableHost

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

::: gfx/layers/composite/ContentHost.cpp
@@ +214,5 @@
>    }
>    GetCompositor()->DrawDiagnostics(diagnostics, nsIntRegion(mBufferRect), aClipRect,
>                                     aTransform, mFlashCounter);
> +
> +  mTextureHost->Fence();

Should we need to wait the drawing task for "DrawDiagnostics"?

::: gfx/layers/composite/ImageHost.cpp
@@ +191,5 @@
>                                       rect, aClipRect,
>                                       aTransform, mFlashCounter);
>    }
> +
> +  mFrontBuffer->Fence();

ditto as ContentHostTexture::Composite

::: gfx/layers/composite/TiledContentHost.cpp
@@ +539,5 @@
>    if (aTile.mTextureHostOnWhite) {
>      flags |= DiagnosticFlags::COMPONENT_ALPHA;
>    }
> +
> +  aTile.mTextureHost->Fence();

How about creating the fence just after DrawQuad()?

Comment 8

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8592592 [details] [diff] [review]
Part 2 - Use CompositableHost fence to replace the original fence

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

::: gfx/layers/opengl/GrallocTextureHost.cpp
@@ +305,5 @@
> +  }
> +
> +#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 17
> +  android::sp<android::Fence> fence = GetAndResetReleaseFence();
> +  if (fence.get() && fence->isValid()) {

I'm confused with these code.
If we have EGL fence, we can have a early return. Since Hwc is reading the FBTarget, not this buffer.
And I think we don't need these code.

a) if have EGL fence(gpu case), return the native fence from EGL fence
b) if we already have native fence(Hwc case), return this native fence.

Comment 9

[Ethan Lin[:ethlin]]

Attachment: Part 1 - Generate fence for CompositableHost

Apply comments from Chiajung and Jerry.

Comment 10

[Ethan Lin[:ethlin]]

Attachment: Part 2 - Use CompositableHost fence to replace the original fence

Apply comments from Chiajung and Jerry.

Comment 11

[Sotaro Ikeda [:sotaro]]

(In reply to Ethan Lin[:elin] from comment #0)
> When we use the CompositorOGL to compose, we wait the final
> FrameBufferSurface's fence in each compositable on the content side. I think
> we can create egl fence for each compositable during composition and the
> each compositable should wait the self fence. This way should be faster.
> I will upload a patch to do this.

Is there a actual data that the change improve the actual performance? I am concerned that if there is a lot of layers(example: css 3d use case) it might degrade the performance. Current FrameBufferSurface's fence usage is intentional to improve CompositorThread's performance. For actual fps,  CompositorThread's performance was important.

Comment 12

[Nicolas Silva [:nical]]

Comment on attachment 8592672 [details] [diff] [review]
Part 2 - Use CompositableHost fence to replace the original fence

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

::: gfx/layers/opengl/GrallocTextureHost.cpp
@@ +295,5 @@
> +{
> +  MOZ_ASSERT(CompositorParent::IsInCompositorThread());
> +
> +  if (sEGLLibrary.IsExtensionSupported(GLLibraryEGL::ANDROID_native_fence_sync) &&
> +      mSync != EGL_NO_SYNC) {

in SharedSurfaceGralloc, we explicitly disable native fences on Adreno TM 200. I assume we should do the same here. see: https://hg.mozilla.org/mozilla-central/file/53ceefb0e1c8/gfx/gl/SharedSurfaceGralloc.cpp#l177

Comment 13

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #12)
> ::: gfx/layers/opengl/GrallocTextureHost.cpp
> @@ +295,5 @@
> > +{
> > +  MOZ_ASSERT(CompositorParent::IsInCompositorThread());
> > +
> > +  if (sEGLLibrary.IsExtensionSupported(GLLibraryEGL::ANDROID_native_fence_sync) &&
> > +      mSync != EGL_NO_SYNC) {
> 
> in SharedSurfaceGralloc, we explicitly disable native fences on Adreno TM
> 200. I assume we should do the same here. see:
> https://hg.mozilla.org/mozilla-central/file/53ceefb0e1c8/gfx/gl/
> SharedSurfaceGralloc.cpp#l177

Yeah, there was such problem in the past. This problem happened in ICS era, at that time fence handling was not mandatory. It says "KHR_fence_sync" support, but the fence did not work.

Comment 14

[Ethan Lin[:ethlin]]

(In reply to Sotaro Ikeda [:sotaro] from comment #11)
> Is there a actual data that the change improve the actual performance? I am
> concerned that if there is a lot of layers(example: css 3d use case) it
> might degrade the performance. Current FrameBufferSurface's fence usage is
> intentional to improve CompositorThread's performance. For actual fps, 
> CompositorThread's performance was important.
Sotaro, I think you are right. I tried a APP with 200 layers (200 x ImageHost) and the total composition time takes extra 7ms for generating egl fence. I will study if there is a better way to generate egl fence without effecting composition performance.

Comment 15

[Andrey Bienkowski]

https://bugzilla.mozilla.org/show_bug.cgi?id=1548706

Comment 16

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

The bug assignee didn't login in Bugzilla in the last 7 months.
:bhood, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 17

[Gregory Pappas [:gregp]]

Closing old B2G bugs