1001417 - Forward fence objects in SharedSurfaceGralloc to Compositor

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Sotaro Ikeda [:sotaro]]

+++ This bug was initially created as a clone of Bug #999694 +++

Bug 999694 is going add support of  EGL_SYNC_FENCE to SharedSurfaceGralloc on gonk JB or newer. It is a improvement from current implementation. It still do fence sync in SharedSurfaceGralloc.

If Android's propritetary EGL Fence sync is supported. Almost all qcom JB/KK devices seems to support it. We can get android::Fence object from the Android's propritetary EGL Fence sync. By forward the Fence object, we could defer the fence sync until Composition. It seems to improve the performance more.

Comment 1

[Sotaro Ikeda [:sotaro]]

We need to be careful how to forwarding Fence object. It could cause a problem like Bug 1000525.

Comment 2

[Sotaro Ikeda [:sotaro]]

I confirmed that nexus-4 and nexus-5 support Android's propritetary EGL Fence sync.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

"Moving" 2.0+ from bug 767484.

Comment 4

[Kelsey Gilbert [:jgilbert]]

I believe the naive way for this to work is as follows:
Producer:
prodSync = egl.CreateSync(android_native_fence, null)
gl.Flush() // Flush creates an FD for the fence object
syncFD = GetSyncAttrib(prodSync, fence_fd_property)
submit syncFD to consumer

Consumer:
attribs = {fence_fd_to_use, syncFD}
consSync = egl.CreateSync(android_native_fence, attribs)
egl.ClientWaitSync(consSync)

The actual symbols are in the extension:
https://www.khronos.org/registry/egl/extensions/ANDROID/EGL_ANDROID_native_fence_sync.txt

Comment 5

[Jason Smith [:jsmith]]

This is a feature, which is something we won't block on unless we're past FL & we're planning to still land the feature.

Comment 6

[Sotaro Ikeda [:sotaro]]

(In reply to Jeff Gilbert [:jgilbert] from comment #4)
> syncFD = GetSyncAttrib(prodSync, fence_fd_property)

I tried the above function on flame device, it output the following error.

> 05-21 16:46:42.892  3677  3677 W Adreno-EGL: <qeglDrvAPI_eglGetSyncAttribKHR:5664>: EGL_BAD_ATTRIBUTE

I used the following code.

> bool success = mEGL->fGetSyncAttrib(mEGL->Display(), sync, LOCAL_EGL_SYNC_NATIVE_FENCE_FD_ANDROID, &syncFD);

Comment 7

[Sotaro Ikeda [:sotaro]]

We might need to use eglDupNativeFenceFDANDROID() like android.

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - Forward Fence objects to Compositor

This is WIP patch. It forward fence objects to compositor side and sync in compositor side. Sometimes it causes crash during WebGL.

Comment 9

[Michael Wu [:mwu]]

Comment on attachment 8426542 [details] [diff] [review]
wip patch - Forward Fence objects to Compositor

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

::: gfx/gl/GLLibraryEGL.cpp
@@ +214,5 @@
>          // can only be queried by using a special eglQueryString.
>          { (PRFuncPtr*) &mSymbols.fQueryStringImplementationANDROID,
>            { "_Z35eglQueryStringImplementationANDROIDPvi", nullptr } },
> +        { (PRFuncPtr*) &mSymbols.fDupNativeFenceFDANDROID,
> +          { "eglDupNativeFenceFDANDROID", nullptr } },

I think you can use the SYMBOL macro here instead of specifying everything. I couldn't use the SYMBOL macro for eglQueryStringImplementationANDROID since it's a mangled C++ function.

Comment 10

[Sotaro Ikeda [:sotaro]]

I am going to compare the performance with Bug 767484 after this bug is fixed.

Comment 11

[Sotaro Ikeda [:sotaro]]

(In reply to Michael Wu [:mwu] from comment #9)
> Comment on attachment 8426542 [details] [diff] [review]
> wip patch - Forward Fence objects to Compositor
> 
> Review of attachment 8426542 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLLibraryEGL.cpp
> @@ +214,5 @@
> >          // can only be queried by using a special eglQueryString.
> >          { (PRFuncPtr*) &mSymbols.fQueryStringImplementationANDROID,
> >            { "_Z35eglQueryStringImplementationANDROIDPvi", nullptr } },
> > +        { (PRFuncPtr*) &mSymbols.fDupNativeFenceFDANDROID,
> > +          { "eglDupNativeFenceFDANDROID", nullptr } },
> 
> I think you can use the SYMBOL macro here instead of specifying everything.
> I couldn't use the SYMBOL macro for eglQueryStringImplementationANDROID
> since it's a mangled C++ function.

Thanks. I am going to update it in a next patch.

Comment 12

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> Created attachment 8426542 [details] [diff] [review]
> wip patch - Forward Fence objects to Compositor
> 
> This is WIP patch. It forward fence objects to compositor side and sync in
> compositor side. Sometimes it causes crash during WebGL.

File descriptors are leaking in b2g process.

Comment 13

[Sotaro Ikeda [:sotaro]]

This bug make the compositor to wait acquire fence on OpenGL composition. I received a comment from :BenWa that it is not good. Compositor run by RT priority and do some important roles like APZ. 

Instead of this bug, Bug 1014815 could improve the SurfaceStream's performance significantly.

Comment 14

[Michael Wu [:mwu]]

Bug 1014815 doesn't sound good either - sounds like that'll make latency suffer and increase the memory required. Maybe the compositor can use a really short timeout for the waitsync instead?

Comment 15

[Sotaro Ikeda [:sotaro]]

I found the file leaking place. ParamTraits<FenceHandle> is designed to deliver fence from compositor to child side. The following code, incorrectly works when a fence is delivered from child to parent side.

>    bool sameProcess = (XRE_GetProcessType() == GeckoProcessType_Default);
>    int dupFd = sameProcess ? dup(fd.fd) : fd.fd;


http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/FenceUtilsGonk.cpp#84

Comment 16

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #15)
> I found the file leaking place. ParamTraits<FenceHandle> is designed to
> deliver fence from compositor to child side. The following code, incorrectly
> works when a fence is delivered from child to parent side.

To fix this, ugly way seems necessary.

Comment 17

[Sotaro Ikeda [:sotaro]]

Attachment: patch v2 - Forward Fence objects to Compositor

Fix file descriptor leak. Apply the comment from mwu.

Comment 18

[Sotaro Ikeda [:sotaro]]

Attachment: patch v3 - Forward Fence objects to Compositor

Fix build failure.

Comment 19

[Sotaro Ikeda [:sotaro]]

Attachment: patch v4 - Forward Fence objects to Compositor

Un-bitrot. Fix nits.

Comment 20

[Sotaro Ikeda [:sotaro]]

Nominate to b2g-v1.4. This fix is necessary for good WebGL performance in b2g-v1.4.

Comment 21

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Change to SharedSurfaceGralloc

Comment 22

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Change to gfx layers

Comment 23

[Sotaro Ikeda [:sotaro]]

Attachment: roll-up patch

Comment 24

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8431831 [details] [diff] [review]
patch part 1 - Change to SharedSurfaceGralloc

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

::: gfx/gl/GLLibraryEGL.h
@@ +414,5 @@
>  
> +    EGLint fDupNativeFenceFDANDROID(EGLDisplay dpy, EGLSync sync)
> +    {
> +        BEFORE_GL_CALL;
> +        EGLint ret = mSymbols.fDupNativeFenceFDANDROID(dpy, sync);

Add:
MOZ_ASSERT(mSymbols.fDupNativeFenceFDANDROID);

::: gfx/gl/SharedSurfaceGralloc.cpp
@@ +160,3 @@
>              mGL->fFlush();
> +            EGLint syncFD = -1;
> +            int fenceFd = mEGL->fDupNativeFenceFDANDROID(mEGL->Display(), sync);

Only do this if we have the right extension for it.

Comment 25

[Sotaro Ikeda [:sotaro]]

(In reply to Jeff Gilbert [:jgilbert] from comment #24)
> Comment on attachment 8431831 [details] [diff] [review]
> patch part 1 - Change to SharedSurfaceGralloc
> 
> Review of attachment 8431831 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLLibraryEGL.h
> @@ +414,5 @@
> >  
> > +    EGLint fDupNativeFenceFDANDROID(EGLDisplay dpy, EGLSync sync)
> > +    {
> > +        BEFORE_GL_CALL;
> > +        EGLint ret = mSymbols.fDupNativeFenceFDANDROID(dpy, sync);
> 
> Add:
> MOZ_ASSERT(mSymbols.fDupNativeFenceFDANDROID);

Will update in next patch.

> 
> ::: gfx/gl/SharedSurfaceGralloc.cpp
> @@ +160,3 @@
> >              mGL->fFlush();
> > +            EGLint syncFD = -1;
> > +            int fenceFd = mEGL->fDupNativeFenceFDANDROID(mEGL->Display(), sync);
> 
> Only do this if we have the right extension for it.

eglDupNativeFenceFDANDROID() requires "EGL_ANDROID_native_fence_sync". In current patch, mEGL->fDupNativeFenceFDANDROID() is called only when the following check passed. It seems that the patch already does the right extension check. 

>     if (mEGL->IsExtensionSupported(GLLibraryEGL::ANDROID_native_fence_sync)) {

The following is a link to android's ANDROID_native_fence_sync spec. 
http://androidxref.com/4.4.2_r2/xref/frameworks/native/opengl/specs/EGL_ANDROID_native_fence_sync.txt

Comment 26

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 v2 - Change to SharedSurfaceGralloc

Apply the comment.

Comment 27

[Sotaro Ikeda [:sotaro]]

Attachment: roll-up patch

Update the roll-up patch.

Comment 28

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8433338 [details] [diff] [review]
patch part 1 v2 - Change to SharedSurfaceGralloc

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

::: gfx/gl/SharedSurfaceGralloc.cpp
@@ +160,3 @@
>              mGL->fFlush();
> +            EGLint syncFD = -1;
> +            int fenceFd = mEGL->fDupNativeFenceFDANDROID(mEGL->Display(), sync);

Is it ok that this crashes on B2Gs using android older than 17?

Comment 29

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8433338 [details] [diff] [review]
patch part 1 v2 - Change to SharedSurfaceGralloc

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

::: gfx/gl/SharedSurfaceGralloc.cpp
@@ +160,2 @@
>              mGL->fFlush();
> +            EGLint syncFD = -1;

Er, syncFD appears unused? Delete this line.

Comment 30

[Sotaro Ikeda [:sotaro]]

(In reply to Jeff Gilbert [:jgilbert] from comment #29)
> Comment on attachment 8433338 [details] [diff] [review]
> patch part 1 v2 - Change to SharedSurfaceGralloc
> 
> Review of attachment 8433338 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/SharedSurfaceGralloc.cpp
> @@ +160,2 @@
> >              mGL->fFlush();
> > +            EGLint syncFD = -1;
> 
> Er, syncFD appears unused? Delete this line.

Yes, it is not necessary. Will delete that line.

Comment 31

[Sotaro Ikeda [:sotaro]]

(In reply to Jeff Gilbert [:jgilbert] from comment #28)
> Comment on attachment 8433338 [details] [diff] [review]
> patch part 1 v2 - Change to SharedSurfaceGralloc
> 
> Review of attachment 8433338 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/SharedSurfaceGralloc.cpp
> @@ +160,3 @@
> >              mGL->fFlush();
> > +            EGLint syncFD = -1;
> > +            int fenceFd = mEGL->fDupNativeFenceFDANDROID(mEGL->Display(), sync);
> 
> Is it ok that this crashes on B2Gs using android older than 17?

It is already confirmed on ICS. I am going to confirm on flatfish. Flatfish uses 17.

Comment 32

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 v3 - Change to SharedSurfaceGralloc

Apply the comment. Carry "r=jgilbert".

Comment 33

[Sotaro Ikeda [:sotaro]]

Attachment: roll-up patch

Comment 34

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Forward fence objects in SharedSurfaceGralloc to Compositor (r=jgilbert,nical)

Fix nits.

Comment 35

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=93866ed1d3f9

Comment 36

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Forward fence objects in SharedSurfaceGralloc to Compositor (r=jgilbert,nical)

Fix build failures on tryserver.

Comment 37

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=e906b8f1ce10

Comment 38

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #37)
> https://tbpl.mozilla.org/?tree=Try&rev=e906b8f1ce10

test failures on OS X Debug during shutdown.

Comment 39

[Sotaro Ikeda [:sotaro]]

Clear "1.4+". graphics team is going to uplift Bug 1014815 to b2g-v1.4 now, instead of this bug. Bug 1014815 could provide similar performance improvement to this bug and has less risk than this bug.

Comment 40

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Forward fence objects in SharedSurfaceGralloc to Compositor (r=jgilbert,nical)

Add workaround to shutdown problem.

Comment 41

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Sotaro Ikeda [:sotaro] from comment #39)
> Clear "1.4+". graphics team is going to uplift Bug 1014815 to b2g-v1.4 now,
> instead of this bug. Bug 1014815 could provide similar performance
> improvement to this bug and has less risk than this bug.

This is good for 31+32=2.0 though, especially given the comments in bug 988573, so let's land it, and if after 6/9, get the uplift approval.

Comment 42

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=30d108d10273

Comment 43

[Sotaro Ikeda [:sotaro]]

(In reply to Jeff Gilbert [:jgilbert] from comment #28)
> Comment on attachment 8433338 [details] [diff] [review]
> patch part 1 v2 - Change to SharedSurfaceGralloc
> 
> Review of attachment 8433338 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/SharedSurfaceGralloc.cpp
> @@ +160,3 @@
> >              mGL->fFlush();
> > +            EGLint syncFD = -1;
> > +            int fenceFd = mEGL->fDupNativeFenceFDANDROID(mEGL->Display(), sync);
> 
> Is it ok that this crashes on B2Gs using android older than 17?

I confirmed that the patch works on 17(flatfish). Flatfish does not support GLLibraryEGL::ANDROID_native_fence_sync just supports egl sync fence.

Comment 44

[Kelsey Gilbert [:jgilbert]]

(In reply to Sotaro Ikeda [:sotaro] from comment #43)
> (In reply to Jeff Gilbert [:jgilbert] from comment #28)
> > Comment on attachment 8433338 [details] [diff] [review]
> > patch part 1 v2 - Change to SharedSurfaceGralloc
> > 
> > Review of attachment 8433338 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/gl/SharedSurfaceGralloc.cpp
> > @@ +160,3 @@
> > >              mGL->fFlush();
> > > +            EGLint syncFD = -1;
> > > +            int fenceFd = mEGL->fDupNativeFenceFDANDROID(mEGL->Display(), sync);
> > 
> > Is it ok that this crashes on B2Gs using android older than 17?
> 
> I confirmed that the patch works on 17(flatfish). Flatfish does not support
> GLLibraryEGL::ANDROID_native_fence_sync just supports egl sync fence.

This was not the question I asked. I was asking if it's OK for an android-version<17 to support native_fence_sync. If so, this will crash on those systems, since we only load fDupNativeFence on >=17 systems.

In general, even if no hardware exists that would cause problems, it's better to do this properly, with the extension mechanism. If there's no good reason not to use the extension mechanism, we should use it.

Comment 45

[Sotaro Ikeda [:sotaro]]

(In reply to Jeff Gilbert [:jgilbert] from comment #44)
> (In reply to Sotaro Ikeda [:sotaro] from comment #43)
> > (In reply to Jeff Gilbert [:jgilbert] from comment #28)
> > > Comment on attachment 8433338 [details] [diff] [review]
> > > patch part 1 v2 - Change to SharedSurfaceGralloc
> > > 
> > > Review of attachment 8433338 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: gfx/gl/SharedSurfaceGralloc.cpp
> > > @@ +160,3 @@
> > > >              mGL->fFlush();
> > > > +            EGLint syncFD = -1;
> > > > +            int fenceFd = mEGL->fDupNativeFenceFDANDROID(mEGL->Display(), sync);
> > > 
> > > Is it ok that this crashes on B2Gs using android older than 17?
> > 
> > I confirmed that the patch works on 17(flatfish). Flatfish does not support
> > GLLibraryEGL::ANDROID_native_fence_sync just supports egl sync fence.
> 
> This was not the question I asked. I was asking if it's OK for an
> android-version<17 to support native_fence_sync. 

Sorry, I misunderstood the question. android-version<17 can not add support of ANDROID_native_fence_sync. Because android-version<17 does not have android::Fence class. native_fence is not present on them.

Comment 46

[Sotaro Ikeda [:sotaro]]

We could remove android version check in GLLibraryEGL::EnsureInitialized() could be removed. Instead, android version check is present in SharedSurface_Gralloc::Fence() in attachment 8435799 [details] [diff] [review].

Comment 47

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Forward fence objects in SharedSurfaceGralloc to Compositor (r=jgilbert,nical)

Remove android version check from GLLibraryEGL::EnsureInitialized().

Comment 48

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=1007f67b03f8

Comment 49

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/229dc47b5059

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/229dc47b5059

Comment 51

[Erin Lancaster [:elan]]

For some reason the 1.4 blocker flag dropped. +ing again.

Comment 52

[Ryan VanderMeulen [:RyanVM]]

Needs a branch patch for v1.4.

Comment 53

[Jukka Jylänki]

I flashed latest trunk to my Flame today, building from scratch, and I'm now getting mostly black screen with very rare occassional distorted colors being rendered. I'm not sure if this bug is the cause, but thought to comment here since I know this development is going on. Has anyone been able to confirm whether the FFOS partner app is working for them after these changes?

Comment 54

[Sotaro Ikeda [:sotaro]]

This bug is totally unrelated to the problem in Comment 53. I already commented about it at Bug 1006164  Comment 23. It is before check-in.

Comment 55

[Sotaro Ikeda [:sotaro]]

Attachment: patch for b2g v1.4- Forward fence objects in SharedSurfaceGralloc to Compositor (r=jgilbert,nical)

A patch for b2g v1.4.

Comment 56

[Erin Lancaster [:elan]]

Is it possible to get this uplifted in time for tomorrow's 1.4 build?

Comment 57

[Ryan VanderMeulen [:RyanVM]]

Yes.

Comment 58

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/5f0f0c3da017

Comment 59

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8436309 [details] [diff] [review]
patch - Forward fence objects in SharedSurfaceGralloc to Compositor (r=jgilbert,nical)

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

::: gfx/layers/opengl/GrallocTextureHost.cpp
@@ +94,4 @@
>                                                   android::GraphicBuffer* aGraphicBuffer,
>                                                   gfx::SurfaceFormat aFormat)
>    : mCompositor(aCompositor)
> +  , mTextureHost(aTextureHost)

You might want to assert that this is non-null.

Comment 60

[u279076]

We've seen ~10 fps improvement in the partner app. From that standpoint this is verified fixed.

Jason, I'm not sure if there's something more you need to test for this to be marked verified fixed for B2G.