Implemented GL_NV_fence extension....

Implemented GL_NV_fence extension.
I believe I have implemented all features according to the spec.
The application is to allow the Chrome command buffer scheduler to be smarter about deciding which command buffer to process. For example, if a WebGL app issued a call to ReadPixels, the scheduler will issue a fence and defer executing the ReadPixels until the status goes true. It can continue to work on other command buffers in the meantime.
I tested by modifying the vertex shader demo. After issuing the SwapBuffers i made issue a fence and loop until the status went true and verified it looped several times. I also tested that by calling FinishFence before going into the loop that is did not loop at all.

[apatrick1, 2010-08-24 00:22:50]

Hi Daniel,

Can you review this please?

Thanks,

Al

[dgkoch, 2010-08-25 04:54:27]

It is (theoretically) possible that a d3d9 implementation won't support event
queries.  We should detect this on startup and avoid advertising the extension
in that case.  I believe that CreateQuery will return D3DERR_NOTAVAILABLE if
they aren't supported.

Daniel

http://codereview.appspot.com/1965043/diff/2001/3002
File src/libGLESv2/Context.cpp (right):

http://codereview.appspot.com/1965043/diff/2001/3002#newcode740
src/libGLESv2/Context.cpp:740: return mResourceManager->createFence();
According to the NV_fence spec (second issue), fences are not sharable between
multiple contexts.  The resource manager is only used for shared objects. 
Follow the example of createFramebuffer/deleteFramebuffer for an example of
non-shared objects.

http://codereview.appspot.com/1965043/diff/2001/3002#newcode790
src/libGLESv2/Context.cpp:790: mResourceManager->deleteFence(fence);
again, shouldn't go through resource manager here -- see deleteFramebuffer

http://codereview.appspot.com/1965043/diff/2001/3002#newcode843
src/libGLESv2/Context.cpp:843: return mResourceManager->getFence(handle);
no resource manager here either -- see getFramebuffer

http://codereview.appspot.com/1965043/diff/2001/3002#newcode2548
src/libGLESv2/Context.cpp:2548: // My interpretation is that it does not
generate a GL error if the fence does not exist.
I agree. Not sure the comment is really required here though.

http://codereview.appspot.com/1965043/diff/2001/3002#newcode2872
src/libGLESv2/Context.cpp:2872: 
this will have to be updated due to recent merges

http://codereview.appspot.com/1965043/diff/2001/3003
File src/libGLESv2/Context.h (right):

http://codereview.appspot.com/1965043/diff/2001/3003#newcode371
src/libGLESv2/Context.h:371: void getFenceiv(GLuint fence, GLenum pname, GLint*
params);
I don't think there is any need to get the context involved in the
isFence/setFence/testFence/finishFence/getFenceiv calls.  I believe the logic
that is implemented in Context.cpp can reasonable just be done in libGLESv2.cpp
(similar to say how IsBuffer/BufferData/GetBufferParameteriv, etc are done).

http://codereview.appspot.com/1965043/diff/2001/3004
File src/libGLESv2/Fence.cpp (right):

http://codereview.appspot.com/1965043/diff/2001/3004#newcode7
src/libGLESv2/Fence.cpp:7: // Buffer.cpp: Implements the gl::Buffer class,
representing storage of vertex and/or
comments need to be updated here.

http://codereview.appspot.com/1965043/diff/2001/3004#newcode18
src/libGLESv2/Fence.cpp:18: Fence::Fence(GLuint id) : RefCountObject(id)
remove RefCountObject

http://codereview.appspot.com/1965043/diff/2001/3004#newcode39
src/libGLESv2/Fence.cpp:39: void Fence::setFence(GLenum condition)
although there is only one possible "condition" we should probably just stuff it
into a member variable here, and then return that in
getFenceiv(GL_FENCE_CONDITION_NV) instead of hard-coding it there.

http://codereview.appspot.com/1965043/diff/2001/3004#newcode63
src/libGLESv2/Fence.cpp:63: HRESULT result = mQuery->GetData(NULL, 0,
D3DGETDATA_FLUSH);
this can also return D3DERR_DEVICELOST, which I believe would put finishFence
into an infinite loop, so we should probably explicitly handle this.

http://codereview.appspot.com/1965043/diff/2001/3004#newcode95
src/libGLESv2/Fence.cpp:95: // My interpretation is that
getFenceiv(GL_FENCE_STATUS_NV) is equivalent to testFence().
I disagree here.  I think we should explicitly have a "status" state variable as
the extension suggests.  Once "status" is has been set to TRUE we no longer need
to do GetData calls to ask the d3d-runtime if the query has succeeded (for
either FENCE_STATUS_NV or for testFence). 

I'd also make getFenceiv(FENCE_STATUS_NV) call GetData without specifying a
flush to make it a lighter-weight call.

http://codereview.appspot.com/1965043/diff/2001/3005
File src/libGLESv2/Fence.h (right):

http://codereview.appspot.com/1965043/diff/2001/3005#newcode9
src/libGLESv2/Fence.h:9: // [OpenGL ES 2.0.24] section 2.9 page 21.
description of file needs to be updated for new file

http://codereview.appspot.com/1965043/diff/2001/3005#newcode19
src/libGLESv2/Fence.h:19: #include "libGLESv2/RefCountObject.h"
this include probably isn't necessary if these are made non-shareable.

http://codereview.appspot.com/1965043/diff/2001/3005#newcode24
src/libGLESv2/Fence.h:24: class Fence : public RefCountObject
I don't think the RefCountObject is required since fences aren't shareable.

http://codereview.appspot.com/1965043/diff/2001/3006
File src/libGLESv2/ResourceManager.cpp (right):

http://codereview.appspot.com/1965043/diff/2001/3006#newcode13
src/libGLESv2/ResourceManager.cpp:13: #include "libGLESv2/Fence.h"
as per my previous comments about these not being shared, I all this
functionality should be in Context.cpp and not in ResourceManager.cpp

http://codereview.appspot.com/1965043/diff/2001/3006#newcode90
src/libGLESv2/ResourceManager.cpp:90: unsigned int handle = 1;
I can't find any evidence that "0" is are special or reserved fence id so they
can likely begin at 0 instead of 1 here.

http://codereview.appspot.com/1965043/diff/2001/3007
File src/libGLESv2/ResourceManager.h (right):

http://codereview.appspot.com/1965043/diff/2001/3007#newcode82
src/libGLESv2/ResourceManager.h:82: FenceMap mFenceMap;
This should go in Context.h

http://codereview.appspot.com/1965043/diff/2001/3008
File src/libGLESv2/libGLESv2.cpp (right):

http://codereview.appspot.com/1965043/diff/2001/3008#newcode1089
src/libGLESv2/libGLESv2.cpp:1089: if (fences[i] != 0)
I don't think fence id=0 is reserved like it is for most other objects.  This
probably also affects the name generation algorithm

http://codereview.appspot.com/1965043/diff/2001/3008#newcode1091
src/libGLESv2/libGLESv2.cpp:1091: context->deleteTexture(fences[i]);
this should be deleteFence not deleteTexture

http://codereview.appspot.com/1965043/diff/2001/3008#newcode3329
src/libGLESv2/libGLESv2.cpp:3329: return context->isFence(fence);
I don't think a context method is required to implement this function.  do
something similar to glIsFramebuffer (for example)

http://codereview.appspot.com/1965043/diff/2001/3008#newcode5234
src/libGLESv2/libGLESv2.cpp:5234: };
this section of the patch will need to be regenerated due to recent changes

http://codereview.appspot.com/1965043/diff/2001/3009
File src/libGLESv2/libGLESv2.def (right):

http://codereview.appspot.com/1965043/diff/2001/3009#newcode155
src/libGLESv2/libGLESv2.def:155: 
this section will have to be regenerated due to recent changes

[apatrick1, 2010-08-25 20:17:38]

Hi Daniel,

I think I fixed everything you noted. I repeated the tests I originally
described. I also tested that the NV_fence extension is not reported when I
simulate failure of CreateQuery, that glDeleteFencesNV works, that glGetFenceiv
does not flush and that it always returns true status after testFence returns
true.

Thanks,

Al

[dgkoch, 2010-08-28 02:50:03]

looks good.  shippit!