We need to run GL commands async yet take action when the commands have been completed.

This overlaps a bit with the patches in bug 697831 but the semantics provided by this GLFence class are different.

Why do we need the GLFence class?

Because it provides a useful wrapper over ARB_sync, falls back to glFinish and provides a callback mechanism if needed. This is common code that is likely to come up at several points in the code base.

Comment on attachment 591538 [details] [diff] [review] patch Review of attachment 591538 [details] [diff] [review]: ----------------------------------------------------------------- Someone who better understand how callbacks and events work should review that portion. ::: gfx/gl/GLContext.cpp @@ +2718,5 @@ > + : mGLContext(aGL) > + , mFenceId(0) > + , mCallback(aCallback) > + , mClosure(aClosure) > +{ This should MakeCurrent mGLContext before running GL commands on it. @@ +2739,5 @@ > +GLFence::~GLFence() > +{ > + RevokeCallback(); > + > + mGLContext->fDeleteSync(mFenceId); Needs MakeCurrent. @@ +2752,5 @@ > + } else { > + mPollEvent = NS_NewNonOwningRunnableMethod(this, &GLFence::PollCallback); > + if (!NS_SUCCEEDED(NS_DispatchToCurrentThread(mPollEvent))) { > + // We expect the callback to work so force a finish to enforce the semantics. > + mGLContext->fFinish(); Needs MakeCurrent. @@ +2774,5 @@ > + if (!mFenceId || mGLContext->IsContextLost()) > + return true; > + > + GLsizei length = 0; > + GLsizei values = 0; 'values' should be of type GLint. @@ +2775,5 @@ > + return true; > + > + GLsizei length = 0; > + GLsizei values = 0; > + mGLContext->fGetSynciv(mFenceId, LOCAL_GL_SYNC_STATUS, 1, &length, &values); Needs MakeCurrent. ::: gfx/gl/GLContext.h @@ +2629,5 @@ > > + GLsync GLAPIENTRY fFenceSync(GLenum condition, GLbitfield flags) { > + BEFORE_GL_CALL; > + GLsync ret = mSymbols.fFenceSync(condition, flags); > + fFlush(); While calls to FenceSync should be followed by a Flush, if we want to make calls to GLContext::fFenceSync call flush, it should happen in the fFenceSync function, which calls 'raw_fFenceSync', which implements (and only implements) the gl call. (See other raw_f* functions) @@ +2728,5 @@ > > +class GLFence { > + NS_INLINE_DECL_REFCOUNTING(GLFence) > +public: > + typedef void (*FenceCompleteCallbackFun)(void *aClosure, GLFence *aFence); If 'fun' is the standard shortening for 'function', this is fine, though I'd prefer 'func'. (Though 'Callback' should already indicate that this is a function) @@ +2739,5 @@ > + * is completed. This is implemented using pooling by queueing events. > + * A best effort is provided to not block the CPU but a glFinish can > + * be issued at any points that a failure occurs to preserve the semantics. > + */ > + GLFence(GLContext* aGL, FenceCompleteCallbackFun aCallback, void *aClosure); Inconsistent 'T* A' vs 'T *A'. ::: gfx/gl/GLDefs.h @@ +70,5 @@ > typedef ptrdiff_t GLintptr; > #endif > > +// ARB_sync > +typedef struct __GLsync *GLsync; Why not typedef from void*?

(In reply to Jeff Gilbert [:jgilbert] from comment #4) > ::: gfx/gl/GLDefs.h > @@ +70,5 @@ > > typedef ptrdiff_t GLintptr; > > #endif > > > > +// ARB_sync > > +typedef struct __GLsync *GLsync; > > Why not typedef from void*? Because that's the type definition in the specs: http://www.opengl.org/registry/specs/ARB/sync.txt Do you feel strongly that it should be void* instead?

I didn't know that GL specs now did typed pointers, but I like that!

I decide to keep the flush explicit, that way we can make better decisions where to flush if we were going to flush anyways a few lines of code later. Let me know if you disagree with that. Once we get r+ for the GL I'll find a reviewer for the callback code.

Comment on attachment 592865 [details] [diff] [review] patch v2 Review of attachment 592865 [details] [diff] [review]: ----------------------------------------------------------------- One issue with the abort text, but r+ otherwise. ::: gfx/gl/GLContext.cpp @@ +477,5 @@ > + { NULL, { NULL } }, > + }; > + > + if (!LoadSymbols(&syncSymbols[0], trygl, prefix)) { > + NS_RUNTIMEABORT("GL supports ARB_sync without supplying GetGraphicsResetStatusARB."); Copypasta issue: GetGraphicsResetStatusARB is from elsewhere.

Comment on attachment 592865 [details] [diff] [review] patch v2 As we discussed on irc yesterday, I think that refcounting objects (GLFences) that are supposed to have clearly defined lifetimes is a bad idea.

(In reply to Benoit Girard (:BenWa) from comment #7) > Created attachment 592865 [details] [diff] [review] > patch v2 > > I decide to keep the flush explicit, that way we can make better decisions > where to flush if we were going to flush anyways a few lines of code later. > Let me know if you disagree with that. Once we get r+ for the GL I'll find a > reviewer for the callback code. At the end of IsComplete there's a case where it'll call and then revoke the callback, but then it ends up returning true, causing a call and revoke in PollCallback after it's completed. Is this intended, or should the callback only be called once?

Do you mean 'if (!NS_SUCCEEDED(NS_DispatchToCurrentThread(mPollEvent))) {'? The if will only be executed if the vent could not be dispatched.

No, sorry. What I'm thinking is that PollCallback() is called, and it sets up mPollEvent. Once the event triggers, it in turn calls PollCallback(). If at that point IsComplete() is true, then it'll call the callback. But the problem, I believe, lies with this code in IsComplete(): if (isComplete && mPollEvent) { mCallback(mClosure, this); RevokeCallback(); } return isComplete; If it's completed and mPollEvent is set (as it was previously), then the callback will be called and it'll return true, which will then cause the callback to be called from the if(IsComplete()) block in PollCallback(). Sorry if that's a bit unclear.

Ahh yes, I see it now. Thanks!

>+ if (!NS_SUCCEEDED(NS_DispatchToCurrentThread(mPollEvent))) { NS_FAILED, please.

This patch builds on the previous one. It fixes the missing symbol text, switches to NS_FAILED, and removes the callback code from IsComplete as per the bug I noted in comment #12.

This is a modified version of my previous patch. It removes refcounting and adds a comment about it having to be freed manually.

Comment on attachment 595538 [details] [diff] [review] Modified previous patch to remove refcounting Looks good to me, but I wrote the patch :).

(In reply to Benoit Girard (:BenWa) from comment #17) > Comment on attachment 595538 [details] [diff] [review] > Modified previous patch to remove refcounting > > Looks good to me, but I wrote the patch :). The initial bits I mean

Comment on attachment 595538 [details] [diff] [review] Modified previous patch to remove refcounting Review of attachment 595538 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. ::: gfx/gl/GLContext.cpp @@ +2781,5 @@ > + > + > +GLFence::~GLFence() > +{ > + RevokeCallback(); nsRevocableEventPtr will revoke the event for you in the dtor. @@ +2792,5 @@ > +GLFence::PollCallback() > +{ > + if (IsComplete()) { > + mCallback(mClosure, this); > + RevokeCallback(); Replace RevokeCallback (which is horribly named btw) with mEventPtr.Revoke(); @@ +2805,5 @@ > + } > +} > + > +void > +GLFence::RevokeCallback() This should no longer be necessary. ::: gfx/gl/GLContext.h @@ +2768,5 @@ > + GLContext *mGLContext; > + GLsync mFenceId; > + FenceCompleteCallbackFunc mCallback; > + void *mClosure; > + nsRefPtr<nsRunnableMethod<GLFence, void, false> > mPollEvent; Lets make this a bit neater. Typedef the nsRunnableMethod<GLFence, void, false> to something easier to read, and then use an nsRevocableEventPtr instead of an nsRefPtr.

Comment on attachment 595874 [details] [diff] [review] Modified previous patch to reflect khuey's notes Review of attachment 595874 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the threading bits. ::: gfx/gl/GLContext.cpp @@ +2758,5 @@ > + : mGLContext(aGL) > + , mFenceId(0) > + , mCallback(aCallback) > + , mClosure(aClosure) > +{ Please add "MOZ_COUNT_CTOR(GLFence);" to the constructor (this will catch leaks on our test machines if any of these objects are leaked). @@ +2780,5 @@ > +} > + > + > +GLFence::~GLFence() > +{ Similarly add MOZ_COUNT_DTOR(GLFence); here. ::: gfx/gl/GLContext.h @@ +2768,5 @@ > + GLContext *mGLContext; > + GLsync mFenceId; > + FenceCompleteCallbackFunc mCallback; > + void *mClosure; > + nsRefPtr<FencePollCallbackFunc> mPollEvent; You may want the indenting to line up.

Comment on attachment 595874 [details] [diff] [review] Modified previous patch to reflect khuey's notes +1 on MOZ_COUNT_CTOR!

After a bit of work, I've managed to get fences exposed to WebGL and working properly. I'm not sure of where/how to put tests into the tree, so I have a standalone test here: http://demoseen.com/fencetest.html#fence -- leaving off the #fence will cause it to leave out the fence portion of the test (good for comparing/debugging). Other concerns: should the interface/methods be prefixed since they're Mozilla-only? Should the WebGL side of this code be enabled only in debug builds?

Comment on attachment 596849 [details] [diff] [review] Exposed fences to WebGL. Review of attachment 596849 [details] [diff] [review]: ----------------------------------------------------------------- At the very least, this will need cycle collection for WebGLFence. Also, does the JS callback get called on the right thread? That's non-obvious to me.

Comment on attachment 596849 [details] [diff] [review] Exposed fences to WebGL. bjacob, does it make sense to expose fences to WebGL? If we can't expose them to the web perhaps we can expose them only to privileged content for testing only?

(In reply to Benoit Girard (:BenWa) from comment #26) > Comment on attachment 596849 [details] [diff] [review] > Exposed fences to WebGL. > > bjacob, does it make sense to expose fences to WebGL? If we can't expose > them to the web perhaps we can expose them only to privileged content for > testing only? We can't just unilaterally expose new WebGL API to general web content. Unless there is a security issue, the better way to expose mozilla-specific WebGL API is to draft a mozilla experimental WebGL extension. Exposing WebGL fences as a WebGL extension will make sense one WebGL gets the ability to share resources across WebGL contexts. The WebGL working group is currently discussing possible API for such cross-webgl-context resource sharing, so your patch is just a little bit ahead of times :) I have yet to look at the actual code.

(In reply to Cody Brocious [:Daeken] from comment #24) > Created attachment 596849 [details] [diff] [review] > Exposed fences to WebGL. > > After a bit of work, I've managed to get fences exposed to WebGL and working > properly. I'm not sure of where/how to put tests into the tree, Tests go into content/canvas/test/webgl/ , you add a new html file in the suitable subdirectory (probably conformance/extensions ) and add it to the corresponding 00_test_list.txt file, and use this command to run it locally: TEST_PATH=content/canvas/test/webgl/test_webgl_conformance_test_suite.html make -C obj-firefox-debug/ mochitest-plain (assuming that cwd is your mozilla source directory, and where obj-firefox-debug/ is the relative path to your object directory) on tryserver, you want mochitest-1. If we do go down this route, in addition to adding your extension to the registry ( http://www.khronos.org/registry/webgl/extensions/ ) we'll upstream your conformance test. > Other concerns: should the interface/methods be prefixed since they're > Mozilla-only? Should the WebGL side of this code be enabled only in debug > builds? This should be a MOZ_ prefixed extension at this point. You can check in bug 684853 an example of a simple extension being implemented, except you'll have to add the MOZ_ prefix and also, we don't add private IID's anymore to concrete implementation classes (the Foo classes that inherit nsIFoo and actually implement stuff).

(In reply to Benoit Jacob [:bjacob] from comment #28) > (In reply to Cody Brocious [:Daeken] from comment #24) > > Created attachment 596849 [details] [diff] [review] > > Exposed fences to WebGL. > > > > After a bit of work, I've managed to get fences exposed to WebGL and working > > properly. I'm not sure of where/how to put tests into the tree, > > Tests go into content/canvas/test/webgl/ , you add a new html file in the > suitable subdirectory (probably conformance/extensions ) and add it to the > corresponding 00_test_list.txt file, and use this command to run it locally: > > TEST_PATH=content/canvas/test/webgl/test_webgl_conformance_test_suite.html > make -C obj-firefox-debug/ mochitest-plain You'll have to make -C obj-firefox-debug/content/canvas/test/webgl first, which copies test files to the object dir.

Please file a new bug for implementing fence semantics in WebGL. This bug is for implementing fence for our internal GL, so as to dodge the need to glFinish for OMTC, etc.

This patch should be equivalent to the previously reviewed attachment. The WebGLFence portions will be split into their own patch as noted previously.

We added fence support in the tree elsewhere.