Bug 942: Bug 942 - Review GLBuffer[State|Size]Tracker and NIO mapped buffers

Commit f8a74c9831c65725a699320c27e62161a0378241 reverted
commit 7c5483d5b20aed9c87c5ce3f6bc840b6546edcd1
due to the fact that the buffer binding itself is _not_
shared across shared GLContext!

Apply uncritical changes of 7c5483d5b20aed9c87c5ce3f6bc840b6546edcd1:

+++

Simplify GLBufferSizeTracker creation @ GLContextImpl ctor,
make it final.

+++

Clear the GLBufferSizeTracker (@destruction) only if no more
created shares are left!

+++

Refine API doc.

+++

4 files changed, 73 insertions, 92 deletions

diff --git a/src/jogl/classes/jogamp/opengl/GLBufferSizeTracker.java b/src/jogl/classes/jogamp/opengl/GLBufferSizeTracker.java
index fa05902d5..b6d9b5682 100644
--- a/src/jogl/classes/jogamp/opengl/GLBufferSizeTracker.java
+++ b/src/jogl/classes/jogamp/opengl/GLBufferSizeTracker.java
@@ -45,47 +45,56 @@ import com.jogamp.common.util.IntLongHashMap;
 
 /**
  * Tracks as closely as possible the sizes of allocated OpenGL buffer
- * objects. When glMapBuffer or glMapBufferARB is called, in order to
- * turn the resulting base address into a java.nio.ByteBuffer, we need
- * to know the size in bytes of the allocated OpenGL buffer object.
- * Previously we would compute this size by using
- * glGetBufferParameterivARB with a pname of GL_BUFFER_SIZE, but
- * it appears doing so each time glMapBuffer is called is too costly
- * on at least Apple's new multithreaded OpenGL implementation. <P>
- *
- * Instead we now try to track the sizes of allocated buffer objects.
- * We watch calls to glBindBuffer to see which buffer is bound to
- * which target and to glBufferData to see how large the buffer's
- * allocated size is. When glMapBuffer is called, we consult our table
+ * objects.
+ * <p>
+ * <code>glMapBuffer</code> or <code>glMapBufferRange</code> etc
+ * returns a <code>java.nio.ByteBuffer</code>
+ * instance reflecting the returned native address of respective calls
+ * and the actual buffer size.
+ * </p>
+ * <p>
+ * In case the buffer size is unknown, we need to compute this size by using
+ * <code>glGetBufferParameteriv</code> with a pname of <code>GL_BUFFER_SIZE</code>.
+ * The latter appears to be problematic due to the returned <code>int</code> value,
+ * where size should be of type <code>long</code>.
+ * Further more, this query appears to be costly for each glMapBuffer call
+ * at for Apple's new multithreaded OpenGL implementation.
+ * </p>
+ * <p>
+ * The buffer size state is shared across all shared OpenGL context,
+ * hence we share the GLBufferSizeTracker instance across all shared GLContexts.
+ * Hence utilizing this instance must be synchronized to be thread safe due to multithreading usage.
+ * </p>
+ * <p>
+ * We track the sizes of allocated buffer objects.
+ * We track calls to <code>glBindBuffer</code> etc to see which buffer is bound to
+ * which target and to <code>glBufferData</code> to see how large the buffer's
+ * allocated size is. When <code>glMapBuffer</code> is called, we consult our table
  * of buffer sizes to see if we can return an answer without a glGet
- * call. <P>
- *
- * We share the GLBufferSizeTracker objects among all GLContexts for
- * which sharing is enabled, because the namespace for buffer objects
- * is the same for these contexts. <P>
- *
- * Tracking the state of which buffer objects are bound is done in the
- * GLBufferStateTracker and is not completely trivial. In the face of
- * calls to glPushClientAttrib / glPopClientAttrib we currently punt
+ * call.
+ * </p>
+ * <p>
+ * In the face of calls to glPushClientAttrib / glPopClientAttrib we currently punt
  * and re-fetch the bound buffer object for the state in question;
- * see, for example, glVertexPointer and the calls down to
- * GLBufferStateTracker.getBoundBufferObject(). Note that we currently
- * ignore new binding targets such as GL_TRANSFORM_FEEDBACK_BUFFER_NV;
+ * see, for example, <code>glVertexPointer</code> and the calls down to
+ * <code>GLBufferStateTracker.getBoundBufferObject()</code>. Note that we currently
+ * ignore new binding targets such as <code>GL_TRANSFORM_FEEDBACK_BUFFER_NV</code>;
  * the fact that new binding targets may be added in the future makes
- * it impossible to cache state for these new targets. <P>
- *
+ * it impossible to cache state for these new targets.
+ * </p>
+ * <p>
  * Ignoring new binding targets, the primary situation in which we may
  * not be able to return a cached answer is in the case of an error,
- * where glBindBuffer may not have been called before trying to call
- * glBufferData. Also, if external native code modifies a buffer
+ * where <code>glBindBuffer</code> may not have been called before trying to call
+ * <code>glBufferData</code>. Also, if external native code modifies a buffer
  * object, we may return an incorrect answer. (FIXME: this case
  * requires more thought, and perhaps stochastic and
  * exponential-fallback checking. However, note that it can only occur
  * in the face of external native code which requires that the
  * application be signed anyway, so there is no security risk in this
  * area.)
+ * </p>
  */
-
 public class GLBufferSizeTracker {
   protected static final boolean DEBUG;
 
@@ -102,11 +111,11 @@ public class GLBufferSizeTracker {
   // pattern of buffer objects indicates that the fact that this map
   // never shrinks is probably not that bad.
   private final IntLongHashMap bufferSizeMap;
-  private final long keyNotFount = 0xFFFFFFFFFFFFFFFFL;
+  private final long sizeNotFount = 0xFFFFFFFFFFFFFFFFL;
 
   public GLBufferSizeTracker() {
       bufferSizeMap = new IntLongHashMap();
-      bufferSizeMap.setKeyNotFoundValue(keyNotFount);
+      bufferSizeMap.setKeyNotFoundValue(sizeNotFount);
   }
 
   public final void setBufferSize(GLBufferStateTracker bufferStateTracker,
@@ -155,7 +164,7 @@ public class GLBufferSizeTracker {
       // point we almost certainly should if the application is
       // written correctly
       long sz = bufferSizeMap.get(buffer);
-      if (keyNotFount == sz) {
+      if (sizeNotFount == sz) {
         // For robustness, try to query this value from the GL as we used to
         // FIXME: both functions return 'int' types, which is not suitable,
         // since buffer lenght is 64bit ?
diff --git a/src/jogl/classes/jogamp/opengl/GLBufferStateTracker.java b/src/jogl/classes/jogamp/opengl/GLBufferStateTracker.java
index 16b7edca8..8f79990a8 100644
--- a/src/jogl/classes/jogamp/opengl/GLBufferStateTracker.java
+++ b/src/jogl/classes/jogamp/opengl/GLBufferStateTracker.java
@@ -50,8 +50,13 @@ import com.jogamp.common.util.IntIntHashMap;
  * GLBufferStateTracker objects are allocated on a per-OpenGL-context basis.
  * This class is used to verify that e.g. the vertex
  * buffer object extension is in use when the glVertexPointer variant
- * taking a long as argument is called. <P>
- *
+ * taking a long as argument is called.
+ * <p>
+ * The buffer binding state is local to it's OpenGL context,
+ * i.e. not shared across multiple OpenGL context.
+ * Hence this code is thread safe due to no multithreading usage.
+ * </p>
+ * <p>
  * Note that because the enumerated value used for the binding of a
  * buffer object (e.g. GL_ARRAY_BUFFER) is different than that used to
  * query the binding using glGetIntegerv (e.g.
@@ -61,19 +66,15 @@ import com.jogamp.common.util.IntIntHashMap;
  * to a particular state. It turns out that for some uses, such as
  * finding the size of the currently bound buffer, this doesn't
  * matter, though of course without knowing the buffer object we can't
- * re-associate the queried size with the buffer object ID. <P>
- *
- * Because the namespace of buffer objects is the unsigned integers
- * with 0 reserved by the GL, and because we have to be able to return
- * both 0 and other integers as valid answers from
- * getBoundBufferObject(), we need a second query, which is to ask
- * whether we know the state of the binding for a given target. For
- * "unknown" targets such as GL_TRANSFORM_FEEDBACK_BUFFER_NV we return
+ * re-associate the queried size with the buffer object ID.
+ * </p>
+ * <p>
+ * For <i>unknown</i> targets such as GL_TRANSFORM_FEEDBACK_BUFFER_NV we return
  * false from this, but we also clear the valid bit and later refresh
  * the binding state if glPushClientAttrib / glPopClientAttrib are
  * called, since we don't want the complexity of tracking stacks of
  * these attributes.
- *
+ * </p>
  */
 
 public class GLBufferStateTracker {
@@ -90,13 +91,13 @@ public class GLBufferStateTracker {
   // OpenGL specifications.
   // http://www.opengl.org/sdk/docs/man/xhtml/glBindBuffer.xml
   private final IntIntHashMap bindingMap;
-  private final int keyNotFound = 0xFFFFFFFF;
+  private final int bindingNotFound = 0xFFFFFFFF;
 
   private final int[] bufTmp = new int[1];
 
   public GLBufferStateTracker() {
     bindingMap = new IntIntHashMap();
-    bindingMap.setKeyNotFoundValue(keyNotFound);
+    bindingMap.setKeyNotFoundValue(bindingNotFound);
 
     // Start with known unbound targets for known keys
     // setBoundBufferObject(GL2ES3.GL_VERTEX_ARRAY_BINDING, 0); // not using default VAO (removed in GL3 core) - only explicit
@@ -139,20 +140,20 @@ public class GLBufferStateTracker {
       return value is valid. */
   public final int getBoundBufferObject(int target, GL caller) {
     int value = bindingMap.get(target);
-    if (keyNotFound == value) {
+    if (bindingNotFound == value) {
       // User probably either called glPushClientAttrib /
       // glPopClientAttrib or is querying an unknown target. See
       // whether we know how to fetch this state.
       boolean gotQueryTarget = true;
-      int queryTarget = 0;
+      final int queryTarget;
       switch (target) {
         case GL2ES3.GL_VERTEX_ARRAY_BINDING: queryTarget = GL2ES3.GL_VERTEX_ARRAY_BINDING;  break;
-        case GL.GL_ARRAY_BUFFER:          queryTarget = GL.GL_ARRAY_BUFFER_BINDING;         break;
-        case GL.GL_ELEMENT_ARRAY_BUFFER:  queryTarget = GL.GL_ELEMENT_ARRAY_BUFFER_BINDING; break;
-        case GL2.GL_PIXEL_PACK_BUFFER:    queryTarget = GL2.GL_PIXEL_PACK_BUFFER_BINDING;    break;
-        case GL2.GL_PIXEL_UNPACK_BUFFER:  queryTarget = GL2.GL_PIXEL_UNPACK_BUFFER_BINDING;  break;
-        case GL4.GL_DRAW_INDIRECT_BUFFER: queryTarget = GL4.GL_DRAW_INDIRECT_BUFFER_BINDING;  break;
-        default:                          gotQueryTarget = false; break;
+        case GL.GL_ARRAY_BUFFER:             queryTarget = GL.GL_ARRAY_BUFFER_BINDING;         break;
+        case GL.GL_ELEMENT_ARRAY_BUFFER:     queryTarget = GL.GL_ELEMENT_ARRAY_BUFFER_BINDING; break;
+        case GL2.GL_PIXEL_PACK_BUFFER:       queryTarget = GL2.GL_PIXEL_PACK_BUFFER_BINDING;    break;
+        case GL2.GL_PIXEL_UNPACK_BUFFER:     queryTarget = GL2.GL_PIXEL_UNPACK_BUFFER_BINDING;  break;
+        case GL4.GL_DRAW_INDIRECT_BUFFER:    queryTarget = GL4.GL_DRAW_INDIRECT_BUFFER_BINDING;  break;
+        default:                             queryTarget = 0; gotQueryTarget = false; break;
       }
       if (gotQueryTarget) {
         final int glerrPre = caller.glGetError(); // clear
diff --git a/src/jogl/classes/jogamp/opengl/GLContextImpl.java b/src/jogl/classes/jogamp/opengl/GLContextImpl.java
index 8f1ba4585..66eed9d96 100644
--- a/src/jogl/classes/jogamp/opengl/GLContextImpl.java
+++ b/src/jogl/classes/jogamp/opengl/GLContextImpl.java
@@ -102,8 +102,8 @@ public abstract class GLContextImpl extends GLContext {
 
   // Tracks creation and initialization of buffer objects to avoid
   // repeated glGet calls upon glMapBuffer operations
-  private GLBufferSizeTracker bufferSizeTracker; // Singleton - Set by GLContextShareSet
-  private final GLBufferStateTracker bufferStateTracker = new GLBufferStateTracker();
+  private final GLBufferSizeTracker bufferSizeTracker;
+  private final GLBufferStateTracker bufferStateTracker;
   private final GLStateTracker glStateTracker = new GLStateTracker();
   private GLDebugMessageHandler glDebugHandler = null;
   private final int[] boundFBOTarget = new int[] { 0, 0 }; // { draw, read }
@@ -138,10 +138,14 @@ public abstract class GLContextImpl extends GLContext {
   public GLContextImpl(GLDrawableImpl drawable, GLContext shareWith) {
     super();
 
-    if (shareWith != null) {
+    bufferStateTracker = new GLBufferStateTracker();
+    if ( null != shareWith ) {
       GLContextShareSet.registerSharing(this, shareWith);
+      bufferSizeTracker = ((GLContextImpl)shareWith).getBufferSizeTracker();
+      assert (bufferSizeTracker != null) : "shared context hash null bufferSizeTracker: "+shareWith;
+    } else {
+      bufferSizeTracker = new GLBufferSizeTracker();
     }
-    GLContextShareSet.synchronizeBufferObjectSharing(shareWith, this);
 
     this.drawable = drawable;
     this.drawableRead = drawable;
@@ -150,13 +154,8 @@ public abstract class GLContextImpl extends GLContext {
   }
 
   private final void clearStates() {
-      // Because we don't know how many other contexts we might be
-      // sharing with (and it seems too complicated to implement the
-      // GLObjectTracker's ref/unref scheme for the buffer-related
-      // optimizations), simply clear the cache of known buffers' sizes
-      // when we destroy contexts
-      if (bufferSizeTracker != null) {
-          bufferSizeTracker.clearCachedBufferSizes();
+      if( !GLContextShareSet.hasCreatedSharedLeft(this) ) {
+        bufferSizeTracker.clearCachedBufferSizes();
       }
       bufferStateTracker.clearBufferObjectState();
       glStateTracker.setEnabled(false);
@@ -2123,10 +2122,6 @@ public abstract class GLContextImpl extends GLContext {
   //----------------------------------------------------------------------
   // Helpers for buffer object optimizations
 
-  public final void setBufferSizeTracker(GLBufferSizeTracker bufferSizeTracker) {
-    this.bufferSizeTracker = bufferSizeTracker;
-  }
-
   public final GLBufferSizeTracker getBufferSizeTracker() {
     return bufferSizeTracker;
   }
diff --git a/src/jogl/classes/jogamp/opengl/GLContextShareSet.java b/src/jogl/classes/jogamp/opengl/GLContextShareSet.java
index 483767b44..c057c904c 100644
--- a/src/jogl/classes/jogamp/opengl/GLContextShareSet.java
+++ b/src/jogl/classes/jogamp/opengl/GLContextShareSet.java
@@ -262,30 +262,6 @@ public class GLContextShareSet {
     return false;
   }
 
-  /** In order to avoid glGet calls for buffer object checks related
-      to glVertexPointer, etc. calls as well as glMapBuffer calls, we
-      need to share the same GLBufferSizeTracker object between
-      contexts sharing textures and display lists. For now we keep
-      this mechanism orthogonal to the GLObjectTracker to hopefully
-      keep things easier to understand. (The GLObjectTracker is
-      currently only needed in a fairly esoteric case, when the
-      Java2D/JOGL bridge is active, but the GLBufferSizeTracker
-      mechanism is now always required.) */
-  public static void synchronizeBufferObjectSharing(final GLContext olderContextOrNull, final GLContext newContext) {
-    final GLContextImpl older = (GLContextImpl) olderContextOrNull;
-    final GLContextImpl newer = (GLContextImpl) newContext;
-    GLBufferSizeTracker tracker = null;
-    if (older != null) {
-      tracker = older.getBufferSizeTracker();
-      assert (tracker != null)
-        : "registerForBufferObjectSharing was not called properly for the older context, or has a bug in it";
-    }
-    if (tracker == null) {
-      tracker = new GLBufferSizeTracker();
-    }
-    newer.setBufferSizeTracker(tracker);
-  }
-
   //----------------------------------------------------------------------
   // Internals only below this point