773071 - use KHR_fence_sync instead of GuaranteeResolve with EGLImage WebGL path

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: 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)]

We already have a better implementation of fences in bug 721115. Can we finish that instead?

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Ohh alright, I'll educate myself tomorrow so give me a bit of time.

Comment 4

[Kelsey Gilbert [:jgilbert]]

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

[Kelsey Gilbert [:jgilbert]]

(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] (needinfo me, slow to respond)]

Attachment: 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

[Kelsey Gilbert [:jgilbert]]

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] (needinfo me, slow to respond)]

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] (needinfo me, slow to respond)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2def0ae47bb

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b2def0ae47bb

Comment 11

[Oleg Romashin (:romaxa)]

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] (needinfo me, slow to respond)]

(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] (needinfo me, slow to respond)]

Attachment: 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

[Kelsey Gilbert [:jgilbert]]

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] (needinfo me, slow to respond)]

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] (needinfo me, slow to respond)]

Attachment: better followup fix, 1s timeout

Updated, with 1s wait

Comment 17

[Kelsey Gilbert [:jgilbert]]

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] (needinfo me, slow to respond)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90d30c9c57fd

Comment 19

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/90d30c9c57fd

Comment 20

[Oleg Romashin (:romaxa)]

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] (needinfo me, slow to respond)]

Arrghghghghh.

Comment 22

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f72d5754b42

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

[Kelsey Gilbert [:jgilbert]]

(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] (needinfo me, slow to respond)]

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

https://hg.mozilla.org/mozilla-central/rev/5f72d5754b42