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

NEW
Assigned to

Status

()

3 years ago
2 years ago

People

(Reporter: ethlin, Assigned: ethlin)

Tracking

(Blocks: 1 bug, {feature})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

3 years ago
Created attachment 8592590 [details] [diff] [review]
Part 1 - Generate fence for CompositableHost

Generate the fence for CompositableHost.
Attachment #8592590 - Flags: feedback?(hshih)
Attachment #8592590 - Flags: feedback?(chung)
(Assignee)

Comment 2

3 years ago
Created attachment 8592592 [details] [diff] [review]
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.
Attachment #8592592 - Flags: feedback?(hshih)
Attachment #8592592 - Flags: feedback?(chung)
(Assignee)

Comment 3

3 years ago
Created attachment 8592594 [details] [diff] [review]
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.
Attachment #8592594 - Flags: feedback?(hshih)
Attachment #8592594 - Flags: feedback?(chung)
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.
Attachment #8592590 - Flags: feedback?(chung) → feedback-
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.
Attachment #8592592 - Flags: feedback?(chung) → feedback-
Comment on attachment 8592594 [details] [diff] [review]
Part 3 - Remove the rest unused functions

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

LGTM.
Attachment #8592594 - Flags: feedback?(chung) → feedback+
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()?
Attachment #8592590 - Flags: feedback?(hshih)
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.
Attachment #8592592 - Flags: feedback?(hshih)
Attachment #8592594 - Flags: feedback?(hshih) → feedback+
(Assignee)

Comment 9

3 years ago
Created attachment 8592671 [details] [diff] [review]
Part 1 - Generate fence for CompositableHost

Apply comments from Chiajung and Jerry.
Attachment #8592590 - Attachment is obsolete: true
(Assignee)

Comment 10

3 years ago
Created attachment 8592672 [details] [diff] [review]
Part 2 - Use CompositableHost fence to replace the original fence

Apply comments from Chiajung and Jerry.
Attachment #8592592 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8592671 - Flags: review?(sotaro.ikeda.g)
Attachment #8592671 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

3 years ago
Attachment #8592672 - Flags: review?(sotaro.ikeda.g)
Attachment #8592672 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

3 years ago
Attachment #8592594 - Flags: review?(sotaro.ikeda.g)
Attachment #8592594 - Flags: review?(nical.bugzilla)
(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.

Updated

3 years ago
Flags: needinfo?(etlin)
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
Attachment #8592672 - Flags: feedback+
(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.
(Assignee)

Comment 14

3 years ago
(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.
Flags: needinfo?(etlin)
(Assignee)

Updated

3 years ago
Attachment #8592671 - Flags: review?(sotaro.ikeda.g)
Attachment #8592671 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

3 years ago
Attachment #8592594 - Flags: review?(sotaro.ikeda.g)
Attachment #8592594 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

3 years ago
Attachment #8592672 - Flags: review?(sotaro.ikeda.g)
Attachment #8592672 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

3 years ago
Blocks: 1155492
Keywords: feature
You need to log in before you can comment on or make changes to this bug.