Closed Bug 1001417 Opened 5 years ago Closed 5 years ago

Forward fence objects in SharedSurfaceGralloc to Compositor

Categories

(Core :: Graphics: Layers, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
blocking-b2g 1.4+
Tracking Status
firefox30 --- wontfix
firefox31 --- wontfix
firefox32 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

(Keywords: verifyme, Whiteboard: [games])

Attachments

(2 files, 14 obsolete files)

56.59 KB, patch
Details | Diff | Splinter Review
52.71 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
+++ 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.
We need to be careful how to forwarding Fence object. It could cause a problem like Bug 1000525.
I confirmed that nexus-4 and nexus-5 support Android's propritetary EGL Fence sync.
"Moving" 2.0+ from bug 767484.
blocking-b2g: --- → 2.0+
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
Summary: Forward fence objects in SharedSurfaceGralloc to Composotor → Forward fence objects in SharedSurfaceGralloc to Compositor
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.
blocking-b2g: 2.0+ → 2.0?
Assignee: nobody → sotaro.ikeda.g
Status: NEW → ASSIGNED
blocking-b2g: 2.0? → 2.0+
(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);
We might need to use eglDupNativeFenceFDANDROID() like android.
This is WIP patch. It forward fence objects to compositor side and sync in compositor side. Sometimes it causes crash during WebGL.
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.
I am going to compare the performance with Bug 767484 after this bug is fixed.
(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.
Blocks: 767484
(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.
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.
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?
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
(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.
Fix file descriptor leak. Apply the comment from mwu.
Attachment #8426542 - Attachment is obsolete: true
Fix build failure.
Attachment #8427925 - Attachment is obsolete: true
Un-bitrot. Fix nits.
Attachment #8428028 - Attachment is obsolete: true
Nominate to b2g-v1.4. This fix is necessary for good WebGL performance in b2g-v1.4.
blocking-b2g: 2.0+ → 1.4?
Attached patch roll-up patch (obsolete) — Splinter Review
Attachment #8428825 - Attachment is obsolete: true
Attachment #8431831 - Flags: review?(jgilbert)
Attachment #8431832 - Flags: review?(nical.bugzilla)
Whiteboard: [games]
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.
Attachment #8431831 - Flags: review?(jgilbert) → review-
Blocks: 1014011
(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
Apply the comment.
Attachment #8431831 - Attachment is obsolete: true
Attached patch roll-up patch (obsolete) — Splinter Review
Update the roll-up patch.
Attachment #8431833 - Attachment is obsolete: true
Attachment #8433338 - Flags: review?(jgilbert)
blocking-b2g: 1.4? → 1.4+
Attachment #8431832 - Flags: review?(nical.bugzilla) → review+
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?
Attachment #8433338 - Flags: review?(jgilbert) → review+
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.
(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.
(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.
Apply the comment. Carry "r=jgilbert".
Attachment #8433338 - Attachment is obsolete: true
Attachment #8434581 - Flags: review+
Attached patch roll-up patch (obsolete) — Splinter Review
Attachment #8433339 - Attachment is obsolete: true
Fix nits.
Attachment #8431832 - Attachment is obsolete: true
Attachment #8434581 - Attachment is obsolete: true
Attachment #8434582 - Attachment is obsolete: true
Attachment #8434977 - Flags: review+
Fix build failures on tryserver.
Attachment #8434977 - Attachment is obsolete: true
Attachment #8435470 - Flags: review+
(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.
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.
blocking-b2g: 1.4+ → ---
Add workaround to shutdown problem.
Attachment #8435470 - Attachment is obsolete: true
(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.
(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.
(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.
(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.
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].
Remove android version check from GLLibraryEGL::EnsureInitialized().
Attachment #8435799 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/229dc47b5059
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
For some reason the 1.4 blocker flag dropped. +ing again.
blocking-b2g: --- → 1.4+
Needs a branch patch for v1.4.
Flags: needinfo?(sotaro.ikeda.g)
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?
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.
Flags: needinfo?(sotaro.ikeda.g)
Keywords: verifyme
Is it possible to get this uplifted in time for tomorrow's 1.4 build?
Flags: needinfo?(ryanvm)
Yes.
Flags: needinfo?(ryanvm)
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.
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.
Depends on: 1061435
You need to log in before you can comment on or make changes to this bug.