Closed Bug 773071 Opened 12 years ago Closed 12 years ago

use KHR_fence_sync instead of GuaranteeResolve with EGLImage WebGL path

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla17

People

(Reporter: vlad, Assigned: vlad)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

use KHR_fence_sync for shared textures when we can
2.67 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
better followup fix, 1s timeout
1.47 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review 
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.
Attachment #641234 - Flags: review?
Attachment #641234 - Flags: review? → review?(bgirard)
We already have a better implementation of fences in bug 721115. Can we finish that instead?
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.
Ohh alright, I'll educate myself tomorrow so give me a bit of time.
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.
Attachment #641234 - Flags: review-
(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.
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.
Attachment #641234 - Attachment is obsolete: true
Attachment #641234 - Flags: review?(bgirard)
Attachment #641501 - Flags: review?(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.
Attachment #641501 - Flags: review?(jgilbert) → review+
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!
https://hg.mozilla.org/mozilla-central/rev/b2def0ae47bb
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
(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?
Attached patch i fail at merging (obsolete) — — Splinter Review 
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!).
Attachment #645329 - Flags: review?(jgilbert)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #645329 - Flags: review?(jgilbert) → review-
Yeah, I went back and forth on the value, but you're right, 1s is better.  I'll update.
Updated, with 1s wait
Attachment #645329 - Attachment is obsolete: true
Attachment #646180 - Flags: review?(jgilbert)
Comment on attachment 646180 [details] [diff] [review]
better followup fix, 1s timeout

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

Great, thanks.
Attachment #646180 - Flags: review?(jgilbert) → review+
https://hg.mozilla.org/mozilla-central/rev/90d30c9c57fd
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Err, I guess WaitSync was accidentially removed by bug 687267
http://hg.mozilla.org/mozilla-central/rev/f07aca82e80e#l10.348
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.
(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.
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: