Last Comment Bug 773071 - use KHR_fence_sync instead of GuaranteeResolve with EGLImage WebGL path
: use KHR_fence_sync instead of GuaranteeResolve with EGLImage WebGL path
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: ARM Android
: -- normal (vote)
: mozilla17
Assigned To: Vladimir Vukicevic [:vlad] [:vladv]
:
Mentors:
Depends on: 728524
Blocks: 775548
  Show dependency treegraph
 
Reported: 2012-07-11 15:48 PDT by Vladimir Vukicevic [:vlad] [:vladv]
Modified: 2012-07-31 06:13 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
use KHR_fence_sync for shared textures when we can (2.50 KB, patch)
2012-07-11 15:48 PDT, Vladimir Vukicevic [:vlad] [:vladv]
jgilbert: review-
Details | Diff | Splinter Review
use KHR_fence_sync for shared textures when we can (2.67 KB, patch)
2012-07-12 09:00 PDT, Vladimir Vukicevic [:vlad] [:vladv]
jgilbert: review+
Details | Diff | Splinter Review
i fail at merging (1.22 KB, patch)
2012-07-24 08:49 PDT, Vladimir Vukicevic [:vlad] [:vladv]
jgilbert: review-
Details | Diff | Splinter Review
better followup fix, 1s timeout (1.47 KB, patch)
2012-07-26 09:43 PDT, Vladimir Vukicevic [:vlad] [:vladv]
jgilbert: review+
Details | Diff | Splinter Review

Description Vladimir Vukicevic [:vlad] [:vladv] 2012-07-11 15:48:54 PDT
Created attachment 641234 [details] [diff] [review]
use KHR_fence_sync for shared textures when we can

The EGLImage texture sharing patches in 728524 rely on a GuaranteeResolve/glFinish to ensure that textures have been correctly updated before using them by the compositor.  This is a pretty serious hammer and hurts performance.  If we just get rid of it, we often have some artifacts, e.g. showing a previous frame on a Tegra 3, but we get about 50% more performance (10fps -> 15fps on the webgl fishtank demo).

We can get much of the same effect by using KHR_fence_sync and inserting a sync object right after our CopyTexImage2D, then waiting for it on the compositor side.  This gets us similar perf wins, without the artifacting.  Note that an even better and higher performance way to do this is the double/triple buffering in bug 761859, but this is a quick and easy win that we can take as soon as the EGLImage stuff lands.
Comment 1 Benoit Girard (:BenWa) 2012-07-11 16:32:35 PDT
We already have a better implementation of fences in bug 721115. Can we finish that instead?
Comment 2 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-11 16:46:48 PDT
No -- different use cases.  ARB_sync is different from EGL_KHR_fence_sync: KHR_fence_sync creates a global EGL sync object that you wait for using EGL commands.  The fence is inserted into whatever API's current when you create it, but then you can wait for it without needing any kind of context current.  This is what we need to be able to wait for the fence on a different thread than where it was created.  ARB_sync, on the other hand, is purely a GL extension and creates sync objects that are waitable only while the context that created them is current... thus they're not useful for this particular use case.
Comment 3 Benoit Girard (:BenWa) 2012-07-11 17:00:40 PDT
Ohh alright, I'll educate myself tomorrow so give me a bit of time.
Comment 4 Jeff Gilbert [:jgilbert] 2012-07-11 17:20:14 PDT
Comment on attachment 641234 [details] [diff] [review]
use KHR_fence_sync for shared textures when we can

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

BenWa is free to review this also, but I probably have worked with this the most.

::: gfx/gl/GLContextProviderEGL.cpp
@@ +333,4 @@
>                               sEGLLibrary.HasKHRImageTexture2D() &&
>                               IsExtensionSupported(OES_EGL_image);
>  
> +        mUseSync = sEGLLibrary.IsExtensionSupported(GLLibraryEGL::KHR_fence_sync);

Why add a new variable for this?

@@ +710,5 @@
> +        // assumes that KHR_fence_sync is enabled!
> +        MOZ_ASSERT(sEGLLibrary.IsExtensionSupported(GLLibraryEGL::KHR_fence_sync));
> +        MOZ_ASSERT(mSyncObject == nsnull);
> +
> +        mSyncObject = sEGLLibrary.fCreateSync(EGL_DISPLAY(), LOCAL_EGL_SYNC_FENCE, 0);

We need a glFlush after creating the sync, or risk it rotting in the pipeline.

Also, please use |nsnull| instead of |0| for |attrib_list|.

@@ +716,5 @@
> +    }
> +
> +    bool WaitSync() {
> +        if (!mSyncObject)
> +            return true;

Probably add a note that this is the branch we take when we're using GuaranteeResolve() instead of MakeSync().

@@ +718,5 @@
> +    bool WaitSync() {
> +        if (!mSyncObject)
> +            return true;
> +
> +        EGLint result = sEGLLibrary.fClientWaitSync(EGL_DISPLAY(), mSyncObject, LOCAL_EGL_NONE, -1);

Timeout should be EGL_FOREVER. We should assert that result is never EGL_TIMEOUT_EXPIRED.

I don't love using EGL_NONE for a bitfield, but it does work, I suppose. (EGL_NONE is only really used as a sentinal value. It just so happens it's also 0)

@@ +722,5 @@
> +        EGLint result = sEGLLibrary.fClientWaitSync(EGL_DISPLAY(), mSyncObject, LOCAL_EGL_NONE, -1);
> +        sEGLLibrary.fDestroySync(EGL_DISPLAY(), mSyncObject);
> +        mSyncObject = nsnull;
> +
> +        return result != LOCAL_EGL_FALSE;

I would rather this test against EGL_CONDITION_SATISFIED, which is explicit that it worked. Testing against FALSE *should* work per spec, but is not really the question we're asking here.
Comment 5 Jeff Gilbert [:jgilbert] 2012-07-11 17:23:12 PDT
(In reply to Benoit Girard (:BenWa) from comment #1)
> We already have a better implementation of fences in bug 721115. Can we
> finish that instead?

This is more sort of quick-fix stuff like bug 728524. Keeping it simple and quick is the goal for the upcoming deadline.
Comment 6 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-12 09:00:23 PDT
Created attachment 641501 [details] [diff] [review]
use KHR_fence_sync for shared textures when we can

Updated, with comments addressed.  I also moved things around a tiny bit, just to do all the work in MakeSync so that we could call Finish() easier if the sync creation failed.
Comment 7 Jeff Gilbert [:jgilbert] 2012-07-12 16:52:45 PDT
Comment on attachment 641501 [details] [diff] [review]
use KHR_fence_sync for shared textures when we can

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

r=me, with a nit in one of the comments.

::: gfx/gl/GLContextProviderEGL.cpp
@@ +785,5 @@
>      BindUserReadFBO(prevRead);
>  
> +    // Make sure our copy is finished, so that we can be ready to draw
> +    // in different thread GLContext.  If we have KHR_fence_sync, then
> +    // we insert a sync object, otherwise we have to do a GuaranteeResolve.

We're not actually calling GuaranteeResolve, just glFinish. Since this is mobile, that's fine, though. I would s/GuaranteeResolve/glFinish in this comment though.
Comment 8 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-12 18:26:16 PDT
Whoops, yes; I changed it to just calling Finish() explicitly after rewriting that comment.  I'll fix.

I'll land this after the EGLImage stuff lands, thanks!
Comment 9 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-17 18:21:24 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2def0ae47bb
Comment 10 Ed Morley [:emorley] 2012-07-18 05:53:41 PDT
https://hg.mozilla.org/mozilla-central/rev/b2def0ae47bb
Comment 11 Oleg Romashin (:romaxa) 2012-07-24 03:00:47 PDT
http://mxr.mozilla.org/mozilla-central/source/gfx/gl/GLContextProviderEGL.cpp#813
Did not we forget call MakeSync here?
Comment 12 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-24 07:40:33 PDT
(In reply to Oleg Romashin (:romaxa) from comment #11)
> http://mxr.mozilla.org/mozilla-central/source/gfx/gl/GLContextProviderEGL.
> cpp#813
> Did not we forget call MakeSync here?

Not sure what you mean -- that line is inside MakeSync, and will only be hit if we failed to create a sync object...  did you mean to link a different line?
Comment 13 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-24 08:49:26 PDT
Created attachment 645329 [details] [diff] [review]
i fail at merging

I failed at merging.  Sigh.

This also changes the timeout to 50ms instead of FOREVER, to avoid strange deadlocks (which should never happen, but could maybe due to driver bugs!).
Comment 14 Jeff Gilbert [:jgilbert] 2012-07-24 16:28:45 PDT
Comment on attachment 645329 [details] [diff] [review]
i fail at merging

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

I really think that 50ms is too low.

::: gfx/gl/GLContextProviderEGL.cpp
@@ +823,5 @@
>              return true;
>          }
>  
> +        EGLint result = sEGLLibrary.fClientWaitSync(EGL_DISPLAY(), mSyncObject, 0,
> +                                                    50 * 1000000 /* 50 ms (as ns) */);

Pull this out into a variable or three. Generally I do it as:
uint64_t ms = 50;
const uint64_t ns_per_ms = 1000 * 1000;
EGLTime timeout = ms * ns_per_ms;

Really, I would encourage a timeout of one second (so 1000ms), since 50ms is only 20fps, which is actually a reasonable framerate for some demos to run. This timeout shouldn't ever be being hit, and changing this to something other than FOREVER is mostly to protect against truly exceptional cases.
Comment 15 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-24 16:33:07 PDT
Yeah, I went back and forth on the value, but you're right, 1s is better.  I'll update.
Comment 16 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-26 09:43:58 PDT
Created attachment 646180 [details] [diff] [review]
better followup fix, 1s timeout

Updated, with 1s wait
Comment 17 Jeff Gilbert [:jgilbert] 2012-07-26 15:55:15 PDT
Comment on attachment 646180 [details] [diff] [review]
better followup fix, 1s timeout

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

Great, thanks.
Comment 18 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-26 18:32:42 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/90d30c9c57fd
Comment 19 :Ehsan Akhgari (away Aug 1-5) 2012-07-27 08:57:41 PDT
https://hg.mozilla.org/mozilla-central/rev/90d30c9c57fd
Comment 20 Oleg Romashin (:romaxa) 2012-07-29 17:20:24 PDT
Err, I guess WaitSync was accidentially removed by bug 687267
http://hg.mozilla.org/mozilla-central/rev/f07aca82e80e#l10.348
Comment 21 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-30 08:05:21 PDT
Arrghghghghh.
Comment 22 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-30 08:08:28 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f72d5754b42
Comment 23 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-30 09:18:10 PDT
Note: god forbid we want to uplift this to anywhere other than m-c, but if we do, I'll make a new patch -- easier than try to grab the various landing bits.
Comment 24 Jeff Gilbert [:jgilbert] 2012-07-30 12:39:43 PDT
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #23)
> Note: god forbid we want to uplift this to anywhere other than m-c, but if
> we do, I'll make a new patch -- easier than try to grab the various landing
> bits.

I think much of this got uplifted all the way to beta with the Honeycomb Flash support, so it shouldn't actually be that hard.
Comment 25 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-30 12:47:55 PDT
Yeah, I mean the patch for this specifically -- beta currently has none of the fence_sync code, it has a GuranteeResolve() for WebGL EGLImage sharing.
Comment 26 Ed Morley [:emorley] 2012-07-31 06:13:21 PDT
https://hg.mozilla.org/mozilla-central/rev/5f72d5754b42

Note You need to log in before you can comment on or make changes to this bug.