Closed
Bug 1001417
Opened 11 years ago
Closed 10 years ago
Forward fence objects in SharedSurfaceGralloc to Compositor
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
People
(Reporter: sotaro, Assigned: sotaro)
References
Details
(Keywords: verifyme, Whiteboard: [games])
Attachments
(2 files, 14 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
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.
Assignee | ||
Comment 1•11 years ago
|
||
We need to be careful how to forwarding Fence object. It could cause a problem like Bug 1000525.
Assignee | ||
Comment 2•11 years ago
|
||
I confirmed that nexus-4 and nexus-5 support Android's propritetary EGL Fence sync.
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
Updated•11 years ago
|
Summary: Forward fence objects in SharedSurfaceGralloc to Composotor → Forward fence objects in SharedSurfaceGralloc to Compositor
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.
blocking-b2g: 2.0+ → 2.0?
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → sotaro.ikeda.g
Assignee | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Updated•11 years ago
|
blocking-b2g: 2.0? → 2.0+
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);
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.
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.
Assignee | ||
Comment 10•10 years ago
|
||
I am going to compare the performance with Bug 767484 after this bug is fixed.
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.
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.
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.
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?
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
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.
Assignee | ||
Comment 17•10 years ago
|
||
Fix file descriptor leak. Apply the comment from mwu.
Attachment #8426542 -
Attachment is obsolete: true
Assignee | ||
Comment 18•10 years ago
|
||
Fix build failure.
Attachment #8427925 -
Attachment is obsolete: true
Assignee | ||
Comment 19•10 years ago
|
||
Un-bitrot. Fix nits.
Attachment #8428028 -
Attachment is obsolete: true
Assignee | ||
Comment 20•10 years ago
|
||
Nominate to b2g-v1.4. This fix is necessary for good WebGL performance in b2g-v1.4.
blocking-b2g: 2.0+ → 1.4?
Assignee | ||
Comment 21•10 years ago
|
||
Assignee | ||
Comment 22•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8431831 -
Flags: review?(jgilbert)
Assignee | ||
Updated•10 years ago
|
Attachment #8431832 -
Flags: review?(nical.bugzilla)
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.
Attachment #8431831 -
Flags: review?(jgilbert) → review-
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
Assignee | ||
Comment 26•10 years ago
|
||
Apply the comment.
Attachment #8431831 -
Attachment is obsolete: true
Assignee | ||
Comment 27•10 years ago
|
||
Update the roll-up patch.
Attachment #8431833 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8433338 -
Flags: review?(jgilbert)
Updated•10 years ago
|
blocking-b2g: 1.4? → 1.4+
Updated•10 years ago
|
Attachment #8431832 -
Flags: review?(nical.bugzilla) → review+
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?
Attachment #8433338 -
Flags: review?(jgilbert) → review+
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.
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.
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.
Assignee | ||
Comment 32•10 years ago
|
||
Apply the comment. Carry "r=jgilbert".
Attachment #8433338 -
Attachment is obsolete: true
Attachment #8434581 -
Flags: review+
Assignee | ||
Comment 34•10 years ago
|
||
Fix nits.
Attachment #8431832 -
Attachment is obsolete: true
Attachment #8434581 -
Attachment is obsolete: true
Attachment #8434582 -
Attachment is obsolete: true
Attachment #8434977 -
Flags: review+
Assignee | ||
Comment 36•10 years ago
|
||
Fix build failures on tryserver.
Attachment #8434977 -
Attachment is obsolete: true
Attachment #8435470 -
Flags: review+
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.
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.
blocking-b2g: 1.4+ → ---
Assignee | ||
Comment 40•10 years ago
|
||
Add workaround to shutdown problem.
Attachment #8435470 -
Attachment is obsolete: true
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.
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.
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.
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.
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].
Assignee | ||
Comment 47•10 years ago
|
||
Remove android version check from GLLibraryEGL::EnsureInitialized().
Attachment #8435799 -
Attachment is obsolete: true
Assignee | ||
Comment 49•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/229dc47b5059
Comment 50•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/229dc47b5059
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment 51•10 years ago
|
||
For some reason the 1.4 blocker flag dropped. +ing again.
blocking-b2g: --- → 1.4+
Comment 52•10 years ago
|
||
Needs a branch patch for v1.4.
status-b2g-v1.4:
--- → affected
status-b2g-v2.0:
--- → fixed
status-firefox30:
--- → wontfix
status-firefox31:
--- → wontfix
status-firefox32:
--- → fixed
Flags: needinfo?(sotaro.ikeda.g)
Keywords: branch-patch-needed
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?
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.
Flags: needinfo?(sotaro.ikeda.g)
Assignee | ||
Updated•10 years ago
|
Keywords: branch-patch-needed → checkin-needed
Comment 56•10 years ago
|
||
Is it possible to get this uplifted in time for tomorrow's 1.4 build?
Flags: needinfo?(ryanvm)
Comment 58•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/5f0f0c3da017
Keywords: checkin-needed
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.
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•