Make null gpu context threadsafe(r) and make sure buffer objects are destroyed.

src/gpu/gl/GrGLCreateNullInterface.cpp

Index: src/gpu/gl/GrGLCreateNullInterface.cpp
diff --git a/src/gpu/gl/GrGLCreateNullInterface.cpp b/src/gpu/gl/GrGLCreateNullInterface.cpp
index d047e730f71a4cdb2db77abea8a412b89c4abad3..fc46e26ac16c42c0005e7f055969a6bafc1bf866 100644
--- a/src/gpu/gl/GrGLCreateNullInterface.cpp
+++ b/src/gpu/gl/GrGLCreateNullInterface.cpp
@@ -10,16 +10,15 @@
 #include "GrGLDefines.h"
 #include "SkTDArray.h"
 #include "GrGLNoOpInterface.h"
+#include "SkTLS.h"
 
-// Functions not declared in GrGLBogusInterface.h (not common with the Debug GL interface).
-
-namespace { // added to suppress 'no previous prototype' warning
-
-class GrBufferObj {
+class BufferObj {
 public:
-    GrBufferObj(GrGLuint id) : fID(id), fDataPtr(NULL), fSize(0), fMapped(false) {
+    SK_DECLARE_INST_COUNT_ROOT(BufferObj);
+
+    BufferObj(GrGLuint id) : fID(id), fDataPtr(NULL), fSize(0), fMapped(false) {
     }
-    ~GrBufferObj() { SkDELETE_ARRAY(fDataPtr); }
+    ~BufferObj() { SkDELETE_ARRAY(fDataPtr); }
 
     void allocate(GrGLsizeiptr size, const GrGLchar* dataPtr) {
         if (NULL != fDataPtr) {
@@ -45,54 +44,101 @@ private:
     bool         fMapped;
 };
 

[robertphillips, 2014/08/14 14:10:17]

// This class maintains a sparsely populated array of buffer pointers. Free
slots
// in the array are threaded by keeping the index of the first free slot in
'fFreeListHead'
// and each subsequent slot as an int stored in the pointer's space.

?

[bsalomon, 2014/08/14 14:52:08]

Acknowledged.

-// In debug builds we do asserts that ensure we agree with GL about when a buffer
-// is mapped.
-static SkTDArray<GrBufferObj*> gBuffers;  // slot 0 is reserved for head of free list
-static GrGLuint gCurrArrayBuffer;
-static GrGLuint gCurrElementArrayBuffer;
+class BufferManager {
+public:
+    SK_DECLARE_INST_COUNT_ROOT(BufferManager);
 
-static GrBufferObj* look_up(GrGLuint id) {
-    GrBufferObj* buffer = gBuffers[id];
-    SkASSERT(NULL != buffer && buffer->id() == id);
-    return buffer;
-}
+    BufferManager() : fFreeListHead(kFreeListEnd) {}
+
+    ~BufferManager() {
+        // NULL out the entries that are really free list links rather than ptrs before deleting.
+        int curr = fFreeListHead;
+        while (kFreeListEnd != curr) {
+            int next = reinterpret_cast<int>(fBuffers[curr]);
+            fBuffers[curr] = NULL;
+            curr = next;
+        }
 
-static GrBufferObj* create_buffer() {
-    if (0 == gBuffers.count()) {
-        // slot zero is reserved for the head of the free list
-        *gBuffers.append() = NULL;
+        fBuffers.deleteAll();
     }
 
-    GrGLuint id;
-    GrBufferObj* buffer;
-
-    if (NULL == gBuffers[0]) {
-        // no free slots - create a new one
-        id = gBuffers.count();
-        buffer = SkNEW_ARGS(GrBufferObj, (id));
-        gBuffers.append(1, &buffer);
-    } else {
-        // recycle a slot from the free list
-        id = SkTCast<GrGLuint>(gBuffers[0]);
-        gBuffers[0] = gBuffers[id];
-
-        buffer = SkNEW_ARGS(GrBufferObj, (id));
-        gBuffers[id] = buffer;
+    BufferObj* lookUp(GrGLuint id) {
+        BufferObj* buffer = fBuffers[id];
+        SkASSERT(NULL != buffer && buffer->id() == id);
+        return buffer;
     }
 
-    return buffer;
-}
+    BufferObj* create() {
+        GrGLuint id;
+        BufferObj* buffer;
+
+        if (kFreeListEnd == fFreeListHead) {
+            // no free slots - create a new one
+            id = fBuffers.count();
+            buffer = SkNEW_ARGS(BufferObj, (id));
+            *fBuffers.append() = buffer;
+        } else {
+            // grab the head of the free list and advance the head to the next free slot.
+            id = fFreeListHead;
+            fFreeListHead = reinterpret_cast<int>(fBuffers[id]);
+
+            buffer = SkNEW_ARGS(BufferObj, (id));
+            fBuffers[id] = buffer;
+        }
 
-static void delete_buffer(GrBufferObj* buffer) {
-    SkASSERT(gBuffers.count() > 0);
+        return buffer;
+    }
 
-    GrGLuint id = buffer->id();
-    SkDELETE(buffer);
+    void free(BufferObj* buffer) {
+        SkASSERT(fBuffers.count() > 0);
 
-    // Add this slot to the free list
-    gBuffers[id] = gBuffers[0];
-    gBuffers[0] = SkTCast<GrBufferObj*>((const void*)(intptr_t)id);
-}
+        GrGLuint id = buffer->id();
+        SkDELETE(buffer);
+
+        fBuffers[id] = reinterpret_cast<BufferObj*>(fFreeListHead);
+        fFreeListHead = id;
+    }
+
+private:
+    static const int kFreeListEnd = -1;

[robertphillips, 2014/08/14 14:10:17]

Line these two up ?

[bsalomon, 2014/08/14 14:52:08]

Done.

+    int                fFreeListHead;
+    SkTDArray<BufferObj*> fBuffers;
+};
+
+/**
+ * The global-to-thread state object for the null interface. All null interfaces on the
+ * same thread currently share one of these. This means two null contexts on the same thread
+ * can interfere with each other. It may make sense to more integrate this into SkNullGLContext
+ * and use it's makeCurrent mechanism.
+ */
+struct ThreadContext {
+public:
+    SK_DECLARE_INST_COUNT_ROOT(ThreadContext);
+

[robertphillips, 2014/08/14 14:10:17]

fBufferMgr ?

[bsalomon, 2014/08/14 14:52:08]

Done.

+    BufferManager fBuffers;

[robertphillips, 2014/08/14 14:10:17]

Line these guys up with fBuffers ?

[bsalomon, 2014/08/14 14:52:08]

Done.

+    GrGLuint fCurrArrayBuffer;
+    GrGLuint fCurrElementArrayBuffer;
+    GrGLuint fCurrProgramID;
+    GrGLuint fCurrShaderID;
+
+    static ThreadContext* Get() {
+        return reinterpret_cast<ThreadContext*>(SkTLS::Get(Create, Delete));
+    }
+
+    ThreadContext()
+        : fCurrArrayBuffer(0)
+        , fCurrElementArrayBuffer(0)
+        , fCurrProgramID(0)
+        , fCurrShaderID(0) {}
+
+private:
+    static void* Create() { return SkNEW(ThreadContext ); }
+    static void Delete(void* context) { SkDELETE(reinterpret_cast<ThreadContext *>(context)); }
+};
+
+// Functions not declared in GrGLBogusInterface.h (not common with the Debug GL interface).
+
+namespace { // added to suppress 'no previous prototype' warning
 
 GrGLvoid GR_GL_FUNCTION_TYPE nullGLActiveTexture(GrGLenum texture) {}
 GrGLvoid GR_GL_FUNCTION_TYPE nullGLAttachShader(GrGLuint program, GrGLuint shader) {}
@@ -102,9 +148,9 @@ GrGLvoid GR_GL_FUNCTION_TYPE nullGLBindTexture(GrGLenum target, GrGLuint texture
 GrGLvoid GR_GL_FUNCTION_TYPE nullGLBindVertexArray(GrGLuint id) {}
 
 GrGLvoid GR_GL_FUNCTION_TYPE nullGLGenBuffers(GrGLsizei n, GrGLuint* ids) {
-
+    ThreadContext* ctx = ThreadContext::Get();
     for (int i = 0; i < n; ++i) {
-        GrBufferObj* buffer = create_buffer();
+        BufferObj* buffer = ctx->fBuffers.create();
         ids[i] = buffer->id();
     }
 }
@@ -115,14 +161,15 @@ GrGLvoid GR_GL_FUNCTION_TYPE nullGLBufferData(GrGLenum target,
                                               GrGLsizeiptr size,
                                               const GrGLvoid* data,
                                               GrGLenum usage) {
+    ThreadContext* ctx = ThreadContext::Get();
     GrGLuint id = 0;
 
     switch (target) {
     case GR_GL_ARRAY_BUFFER:
-        id = gCurrArrayBuffer;
+        id = ctx->fCurrArrayBuffer;
         break;
     case GR_GL_ELEMENT_ARRAY_BUFFER:
-        id = gCurrElementArrayBuffer;
+        id = ctx->fCurrElementArrayBuffer;
         break;
     default:
         SkFAIL("Unexpected target to nullGLBufferData");
@@ -130,7 +177,7 @@ GrGLvoid GR_GL_FUNCTION_TYPE nullGLBufferData(GrGLenum target,
     }
 
     if (id > 0) {
-        GrBufferObj* buffer = look_up(id);
+        BufferObj* buffer = ctx->fBuffers.lookUp(id);
         buffer->allocate(size, (const GrGLchar*) data);
     }
 }
@@ -147,13 +194,11 @@ GrGLvoid GR_GL_FUNCTION_TYPE nullGLFramebufferRenderbuffer(GrGLenum target, GrGL
 GrGLvoid GR_GL_FUNCTION_TYPE nullGLFramebufferTexture2D(GrGLenum target, GrGLenum attachment, GrGLenum textarget, GrGLuint texture, GrGLint level) {}
 
 GrGLuint GR_GL_FUNCTION_TYPE nullGLCreateProgram() {
-    static GrGLuint gCurrID = 0;
-    return ++gCurrID;
+    return ++ThreadContext::Get()->fCurrProgramID;
 }
 
 GrGLuint GR_GL_FUNCTION_TYPE nullGLCreateShader(GrGLenum type) {
-    static GrGLuint gCurrID = 0;
-    return ++gCurrID;
+    return ++ThreadContext::Get()->fCurrShaderID;
 }
 
 // same delete used for shaders and programs
@@ -161,46 +206,49 @@ GrGLvoid GR_GL_FUNCTION_TYPE nullGLDelete(GrGLuint program) {
 }
 
 GrGLvoid GR_GL_FUNCTION_TYPE nullGLBindBuffer(GrGLenum target, GrGLuint buffer) {
+    ThreadContext* ctx = ThreadContext::Get();
     switch (target) {
     case GR_GL_ARRAY_BUFFER:
-        gCurrArrayBuffer = buffer;
+        ctx->fCurrArrayBuffer = buffer;
         break;
     case GR_GL_ELEMENT_ARRAY_BUFFER:
-        gCurrElementArrayBuffer = buffer;
+        ctx->fCurrElementArrayBuffer = buffer;
         break;
     }
 }
 
 // deleting a bound buffer has the side effect of binding 0
 GrGLvoid GR_GL_FUNCTION_TYPE nullGLDeleteBuffers(GrGLsizei n, const GrGLuint* ids) {
+    ThreadContext* ctx = ThreadContext::Get();
     for (int i = 0; i < n; ++i) {
-        if (ids[i] == gCurrArrayBuffer) {
-            gCurrArrayBuffer = 0;
+        if (ids[i] == ctx->fCurrArrayBuffer) {
+            ctx->fCurrArrayBuffer = 0;
         }
-        if (ids[i] == gCurrElementArrayBuffer) {
-            gCurrElementArrayBuffer = 0;
+        if (ids[i] == ctx->fCurrElementArrayBuffer) {
+            ctx->fCurrElementArrayBuffer = 0;
         }
 
-        GrBufferObj* buffer = look_up(ids[i]);
-        delete_buffer(buffer);
+        BufferObj* buffer = ctx->fBuffers.lookUp(ids[i]);
+        ctx->fBuffers.free(buffer);
     }
 }
 
 GrGLvoid* GR_GL_FUNCTION_TYPE nullGLMapBufferRange(GrGLenum target, GrGLintptr offset,
                                                    GrGLsizeiptr length, GrGLbitfield access) {
+    ThreadContext* ctx = ThreadContext::Get();
     GrGLuint id = 0;
     switch (target) {
         case GR_GL_ARRAY_BUFFER:
-            id = gCurrArrayBuffer;
+            id = ctx->fCurrArrayBuffer;
             break;
         case GR_GL_ELEMENT_ARRAY_BUFFER:
-            id = gCurrElementArrayBuffer;
+            id = ctx->fCurrElementArrayBuffer;
             break;
     }
 
     if (id > 0) {
         // We just ignore the offset and length here.
-        GrBufferObj* buffer = look_up(id);
+        BufferObj* buffer = ctx->fBuffers.lookUp(id);
         SkASSERT(!buffer->mapped());
         buffer->setMapped(true);
         return buffer->dataPtr();
@@ -209,18 +257,19 @@ GrGLvoid* GR_GL_FUNCTION_TYPE nullGLMapBufferRange(GrGLenum target, GrGLintptr o
 }
 
 GrGLvoid* GR_GL_FUNCTION_TYPE nullGLMapBuffer(GrGLenum target, GrGLenum access) {
+    ThreadContext* ctx = ThreadContext::Get();
     GrGLuint id = 0;
     switch (target) {
         case GR_GL_ARRAY_BUFFER:
-            id = gCurrArrayBuffer;
+            id = ctx->fCurrArrayBuffer;
             break;
         case GR_GL_ELEMENT_ARRAY_BUFFER:
-            id = gCurrElementArrayBuffer;
+            id = ctx->fCurrElementArrayBuffer;
             break;
     }
 
     if (id > 0) {
-        GrBufferObj* buffer = look_up(id);
+        BufferObj* buffer = ctx->fBuffers.lookUp(id);
         SkASSERT(!buffer->mapped());
         buffer->setMapped(true);
         return buffer->dataPtr();
@@ -236,17 +285,18 @@ GrGLvoid GR_GL_FUNCTION_TYPE nullGLFlushMappedBufferRange(GrGLenum target,
 
 
 GrGLboolean GR_GL_FUNCTION_TYPE nullGLUnmapBuffer(GrGLenum target) {
+    ThreadContext* ctx = ThreadContext::Get();
     GrGLuint id = 0;
     switch (target) {
     case GR_GL_ARRAY_BUFFER:
-        id = gCurrArrayBuffer;
+        id = ctx->fCurrArrayBuffer;
         break;
     case GR_GL_ELEMENT_ARRAY_BUFFER:
-        id = gCurrElementArrayBuffer;
+        id = ctx->fCurrElementArrayBuffer;
         break;
     }
     if (id > 0) {
-        GrBufferObj* buffer = look_up(id);
+        BufferObj* buffer = ctx->fBuffers.lookUp(id);
         SkASSERT(buffer->mapped());
         buffer->setMapped(false);
         return GR_GL_TRUE;
@@ -257,20 +307,21 @@ GrGLboolean GR_GL_FUNCTION_TYPE nullGLUnmapBuffer(GrGLenum target) {
 }
 
 GrGLvoid GR_GL_FUNCTION_TYPE nullGLGetBufferParameteriv(GrGLenum target, GrGLenum pname, GrGLint* params) {
+    ThreadContext* ctx = ThreadContext::Get();
     switch (pname) {
         case GR_GL_BUFFER_MAPPED: {
             *params = GR_GL_FALSE;
             GrGLuint id = 0;
             switch (target) {
                 case GR_GL_ARRAY_BUFFER:
-                    id = gCurrArrayBuffer;
+                    id = ctx->fCurrArrayBuffer;
                     break;
                 case GR_GL_ELEMENT_ARRAY_BUFFER:
-                    id = gCurrElementArrayBuffer;
+                    id = ctx->fCurrElementArrayBuffer;
                     break;
             }
             if (id > 0) {
-                GrBufferObj* buffer = look_up(id);
+                BufferObj* buffer = ctx->fBuffers.lookUp(id);
                 if (buffer->mapped()) {
                     *params = GR_GL_TRUE;
                 }