Re: [Chromium-dev] VBO support in array SPU

[Brian P. <bri...@tu...>]

Peter Djeu wrote:
> 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.

Buffer objects can potentially be shared between rendering contexts 
(like textures and display lists).  So in one context we might call 
glDeleteBuffersARB while the named buffer is still bound in another 
context.  We can't _really_ delete that buffer until the other 
contexts unbinds it.

That said, context sharing isn't fully implemented in Chromium so this 
case won't be hit as things are now.  Perhaps in the future...


> * 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.

The #ifdef conventions for that are pretty loose.  The idea was that 
one could recompile Chromium without the code for a particular 
extension, if desired.  I don't think anyone's ever done that though. 
  So, the #ifdef stuff mostly just serves to highlight when code is 
needed for the sake of a particular extension.


> * 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.

Someone with more WGL experience than I will have to help with that.

I'll try to get to your patch by the end of the week.  I also want to 
try to put together a Cr 1.8 release candidate soon.

-Brian