Sotaro Ikeda [:sotaro] Assignee
	Description • 11 years ago

+++ 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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 1 • 11 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 2 • 11 years ago

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

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 3 • 11 years ago

"Moving" 2.0+ from bug 767484.

	Kelsey Gilbert [:jgilbert]
	Comment 4 • 11 years ago

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

	Jason Smith [:jsmith]
	Comment 5 • 11 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 6 • 10 years ago

(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);

	Sotaro Ikeda [:sotaro] Assignee
	Comment 7 • 10 years ago

We might need to use eglDupNativeFenceFDANDROID() like android.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 8 • 10 years ago

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

	Michael Wu [:mwu]
	Comment 9 • 10 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 10 • 10 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 11 • 10 years ago

(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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 12 • 10 years ago

(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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 13 • 10 years ago

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.

	Michael Wu [:mwu]
	Comment 14 • 10 years ago

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?

	Sotaro Ikeda [:sotaro] Assignee
	Comment 15 • 10 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 16 • 10 years ago

(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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 17 • 10 years ago

Fix file descriptor leak. Apply the comment from mwu.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 18 • 10 years ago

Fix build failure.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 19 • 10 years ago

Un-bitrot. Fix nits.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 20 • 10 years ago

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

	Kelsey Gilbert [:jgilbert]
	Comment 24 • 10 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 25 • 10 years ago

(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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 26 • 10 years ago

Apply the comment.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 27 • 10 years ago

Update the roll-up patch.

	Kelsey Gilbert [:jgilbert]
	Comment 28 • 10 years ago

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?

	Kelsey Gilbert [:jgilbert]
	Comment 29 • 10 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 30 • 10 years ago

(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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 31 • 10 years ago

(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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 32 • 10 years ago

Apply the comment. Carry "r=jgilbert".

	Sotaro Ikeda [:sotaro] Assignee
	Comment 34 • 10 years ago

Fix nits.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 35 • 10 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 36 • 10 years ago

Fix build failures on tryserver.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 37 • 10 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 38 • 10 years ago

(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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 39 • 10 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 40 • 10 years ago

Add workaround to shutdown problem.

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 41 • 10 years ago

(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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 42 • 10 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 43 • 10 years ago

(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.

	Kelsey Gilbert [:jgilbert]
	Comment 44 • 10 years ago

(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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 45 • 10 years ago

(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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 46 • 10 years ago

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].

	Sotaro Ikeda [:sotaro] Assignee
	Comment 47 • 10 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 48 • 10 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 49 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 50 • 10 years ago

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

	Erin Lancaster [:elan]
	Comment 51 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 52 • 10 years ago

Needs a branch patch for v1.4.

	Jukka Jylänki
	Comment 53 • 10 years ago

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?

	Sotaro Ikeda [:sotaro] Assignee
	Comment 54 • 10 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 55 • 10 years ago

A patch for b2g v1.4.

	Erin Lancaster [:elan]
	Comment 56 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 57 • 10 years ago

Yes.

	Ryan VanderMeulen [:RyanVM]
	Comment 58 • 10 years ago

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

	Kelsey Gilbert [:jgilbert]
	Comment 59 • 10 years ago

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.

	u279076
	Comment 60 • 10 years ago

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.