[Chromium-dev] VBO support in array SPU

[Peter D. <dj...@cs...>]

Sorry if you receive this email more than once.  I sent the original 
version to the dev list about a week and a half ago (before I was a list 
member), and that email still has not gone through the moderation process, 
so I thought I'd resubmit.  -- Peter

Here's the original message:

Hi, I've been working on Chromium for some time now trying to get Doom 3 
to run smoothly on it.  I added support for Vertex Buffer Objects (VBO's) 
in the array SPU and wanted to submit my changes for review.  I've tested 
my modified version on a handful of applications that use VBO's and normal 
vertex arrays, and it seems OK so far.  Here's what I've included:

1.  Attached, an archive containing patches to the array SPU that adds 
support for regular VBO arays and for VBO element arrays.  A minor 
(hopefully correct) fix is also included for the buffer object in the 
state tracker.  This patch is diff'd with the cvs repository from Mar 1.

2.  Below, a summary of what changes I made and where, for anyone 
interested.

3.  Below, questions I came across while putting together the patch.  If 
anyone could answer these, I'd really appreciate it.

        Sincerely,
        Peter Djeu


<<< Summary >>>


spu/array/arrayspu.c
    - For the array SPU -- added offset calculations whenever a client 
pointer has valid data in its VBO data ptr.  All uses are protected by a 
#define guard so that the calculations are only done when the Vertex 
Bufffer Object extension is enabled in cr_version.h.

    - Added missing gl function "IndexPointer" to pass along the color index 
pointer to crStateIndexPointer().

    - Added gl functions to pass along all VBO interface calls to the 
corresponding state tracker functions.

    - Added support in DrawElements() for Vertex Buffer Object element 
arrays.


spu/array/arrayspu_init.c
    - For the array SPU -- on context initialization, if the Vertex Buffer 
Object extension is enabled, set the buffer object's retainData flag to 
GL_TRUE.


state_tracker/state_bufferobject.c
    - In the state tracker for VBO's -- when the null buffer is first 
initialized in crStateBufferObjectInit(), its reference count is set to 2. 
However, if the arrayBuffer and elementsBuffer both enter use, then the 
nullBuffer reference count drops to 0, which causes an unnecessary attempt 
to remove it from the hash table every time it is swapped out.  A fix is 
added to set the reference count to 3 on initialization (this also makes 
more sense since the nullBuffer is being pointed to by 3 pointers anyway).

    - In crStateDeleteBuffersARB(), added a line to increment the reference 
count of the nullBuffer when it replaces either arrayBuffer or 
elementsBuffer. Otherwise, repeated swapping out of the nullBuffer causes 
its reference count to wrap around, which is harmless although odd.


<<< Questions >>>

Each question is preceded by the file it pertains to.

* state_tracker/state_bufferobject.c

    - Why are there ref counts within CRBufferObject's?  We can't free the 
data associated with a buffer or remove it from the hashtable unless the 
buffer is explicitly deleted by a DeleteBuffersARB call, so it seems 
unusual what there is even logic present that frees a buffer when its ref 
count drops to zero.  Also, because the buffer object initializer starts 
the refcount at 1 and it immediately gets incremented to 2 
(newObj->refCount++;) in crStateBindBufferARB(), no buffer object, except 
for the nullBuffer, will ever get swapped out enough times for its ref 
count to reach zero.

* spu/array/arrayspu.c

    - Is it customary to NOT put #define guards around entire GL functions 
in a SPU, but to use them within the body of a parent function call?  For 
example, use of the fog coordinate pointer is protected with a "#ifdef 
CR_EXT_fog_coord" guard within arrayspu_ArrayElement(). However, at the 
top level, the function arrayspu_FogCoordPointerEXT(), at both its 
definition and listing in the SPU's named function table, do not have this 
guard.  I have tried to follow the existing standard in the current patch, 
but I can submit an update if using the missing guards everywhere is more 
correct.

* opengl_stub/wgl.c

    - I had a lot of trouble (on Windows) trying to get Chromium to choose 
the right pixel format for the render window (it was always selecting 
Pixel Format 1, despite the visual attributes I was asking for via the 
render SPU).  Is there some easy way to set the pixel format?  I 
ultimately hacked on opengl_stub/wgl.c until it worked, but I didn't 
include these changes in this patch because there might have been a good 
reason for always using Pixel Format 1.  I'm using an NV40 card on Windows 
XP, in case that helps.

EOF