Bug 942

Summary: Track GLBufferObject's Data-Store and it's NIO mapped buffers accurately, synchronized and secure.
Product: [JogAmp] Jogl Reporter: Sven Gothel <sgothel>
Component: coreAssignee: Sven Gothel <sgothel>
Status: RESOLVED FIXED    
Severity: major CC: fzanelli, gouessej, sgothel
Priority: ---    
Version: 2   
Hardware: All   
OS: all   
Type: DEFECT SCM Refs:
7c5483d5b20aed9c87c5ce3f6bc840b6546edcd1 f8a74c9831c65725a699320c27e62161a0378241 30bd30d6563041b71f40e4c336e636768ae26f9d 09fc7aa5539731bb0fba835caee61f6eb837ecff 343deff48d81b0abf58c275d9e0aced12a911802 97f4ef2763596993bcb8a6b84150c9ec906dde08 e7002849ff0b55d86e351a9ccfc279bddd1a6d51
Workaround: ---
Bug Depends on:    
Bug Blocks: 1287    

Description Sven Gothel 2014-01-14 05:36:51 CET
Currently we are 'lucky' that the JVM implementation's NewDirectByteBuffer JNI method
allows us to return the local-reference and use it beyond it's scope!

By specification, leaving the stack frame, i.e. the native method, 
the local reference is no more guaranteed to exist and free to be GC'ed.

We need to evaluate adding a global reference via NewGlobalRef,
track this object until it's native deletion and issue DeleteGlobalRef.

glMapBuffer's mapped memory region is released by the OpenGL driver if:
  - glUnmapBuffer  - unmap buffer
  - glBufferData      - buffer recreation
  - glDeleteBuffers - buffer deletion

Implementation may track the ByteBuffer while mapping it
to the target buffer type in GLBufferStateTracker.

+++

Due to the above, the GLBufferStateTracker instance must be shared
across shared GLContextImpl instances similar to GLSizeStateTracker!

This allows us to merge GLSizeStateTracker code into GLBufferStateTracker
to simplify the implementation.

+++
Comment 1 Sven Gothel 2014-01-14 07:27:36 CET
The following assumption as derived from local/global references from the jni-spec 
doesn't seem to be correct for an NIO buffer created w/ NewDirectByteBuffer 
and _returned_ to the java side:

+++
Currently we are 'lucky' that the JVM implementation's NewDirectByteBuffer JNI method
allows us to return the local-reference and use it beyond it's scope!

By specification, leaving the stack frame, i.e. the native method, 
the local reference is no more guaranteed to exist and free to be GC'ed.
+++

Currently we only use the above (global-ref) to store a reference on the JNI side.

+++

However, tracking the lifecycle of such a memory mapped object would be useful.

Further more, contemplate whether glMapBuffer (etc) methods shall simply
return a pre-mapped ByteBuffer (as it is now),
or better a 2nd view of such via 'slice()' having it's own position and limit state.
Comment 2 Sven Gothel 2014-01-14 17:05:45 CET
GLBufferStateTracker needs to be thread safe due 
to shared GLContext.
Comment 3 Sven Gothel 2014-01-14 19:30:16 CET
(In reply to comment #2)
> GLBufferStateTracker needs to be thread safe due 
> to shared GLContext.

Note: Only the buffer object is shared - not the binding state!
Comment 4 Sven Gothel 2014-01-14 20:26:22 CET
7c5483d5b20aed9c87c5ce3f6bc840b6546edcd1
f8a74c9831c65725a699320c27e62161a0378241

30bd30d6563041b71f40e4c336e636768ae26f9d

    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.
    
    +++
Comment 5 Sven Gothel 2014-01-21 17:49:39 CET
GLBufferSizeTracker shall become GLBufferObjectTracker
and track the buffer's data store, GLBufferStorage, accurately, synchronized and secure.

Synchronization is required, since the GLBufferStorage can be 
shared across many GLContext on multiple threads.

This requires all GLBufferStorage lifecycle affecting GL functions
to utilize synchronized GLBufferObjectTracker methods 
while passing a native GL-func callback.

These GL functions are:
  - glBufferData, glBufferStorage (GL 4.4), glNamedBufferDataEXT
    Creating the GLBufferStorage object
  - glMapBuffer, glMapBufferRange, and their *Named*EXT variants
  - glUnmapBuffer, glUnmapNamedBufferEXT

'glDeleteBuffers' can simply notify the GLBufferObjectTracker

No more HashMap is required to associate the mapped buffer address
to the mapped ByteBuffer.

GLBufferObjectTracker simply utilizes a
   buffer-name (int) -> GLBufferStorage
map.
Comment 6 Sven Gothel 2014-01-21 18:07:55 CET
(In reply to comment #5)
> GLBufferSizeTracker shall become GLBufferObjectTracker
> and track the buffer's data store, GLBufferStorage, accurately, synchronized
> and secure.

The security aspect shall be implemented by validating all arguments
whether they match the required GL constraints,
as well as validating tracked states like 'size'.

The following functions will throw an GLException accordingly:
  - glBufferData, glNamedBufferDataEXT
     * @throws GLException if size is less-than zero
     * @throws GLException if a native GL-Error occurs

  - glBufferStorage (GL 4.4)
     * @throws GLException if size is less-or-eqaul zero
     * @throws GLException if a native GL-Error occurs

  - glMapBuffer, and it's *Named*EXT variant
    * @throws GLException if buffer is not bound to target
    * @throws GLException if buffer is not tracked
    * @throws GLException if buffer is already mapped
    * @throws GLException if buffer has invalid store size, i.e. less-than zero

  - glMapBufferRange, and it's *Named*EXT variant
    * @throws GLException if buffer is not bound to target
    * @throws GLException if buffer is not tracked
    * @throws GLException if buffer is already mapped
    * @throws GLException if buffer has invalid store size, i.e. less-than zero
    * @throws GLException if buffer mapping range does not fit, incl. offset

  - glMapBufferRange, and it's *Named*EXT variant
    Only clear mapped buffer reference of GLBufferStorage
    if native unmap was successful.

Further more special error handling shall be applied to:
  - glMapBuffer, and it's *Named*EXT variant,
    glMapBuffer, and it's *Named*EXT variant
    - A zero GLBufferStorage size will avoid a native call and 
      returns null
    - A null native mapping result indicating an error will
     not cause a GLException but returns null
     This allows the user to handle this case.
Comment 7 Sven Gothel 2014-01-21 18:59:45 CET
09fc7aa5539731bb0fba835caee61f6eb837ecff
  As described in Comment 5 and Comment 6

343deff48d81b0abf58c275d9e0aced12a911802
    Add mapped buffer capabilities to GLArrayDataServer and add unit tests
    
    GLArrayDataServer:
      - Add create*Mapped(..) variants for GPU mapped buffer usage
        w/o client buffers.
    
      - Fix API documentation (arguments)
    
      - Fix 'addGLSLSubArray(..)'
        - properly compute and pass 'subStrideB' in bytes to GLArrayDataWrapper ctor.
    
      - Add 'mapStorage(..)' and 'unmapStorage(..)'
        allowing to map the GPU buffer.
    
    GLArrayDataWrapper:
      - Fix getElementCount(): Consider stride in bytes and consider 'mappedElementCount'
      - getSizeInBytes(): Consider 'mappedElementCount'
    
    Tests:
      - Use new GLBase methods, e.g. getBoundBuffer(..) instead of glGetBoundBuffer(..)
    
      - TestMapBufferRead01NEWT: Validate GLBufferStorage (i.e. GLBufferObjectTracker)
    
      - Add RedSquareMappedES2 using GPU mapped buffer
        - Test w/ TestRedSquareES2NEWT, cmd-line 'mappedBuffers'
    
      - GearsES1 and GearsES2: Add GPU buffer mapping mode for all test cases
        - Add buffer validation mode, i.e. test whether GLBufferObjectTracker
          works properly.
    
        - Test w/ TestGearsES2NEWT, cmd-line 'mappedBuffers'
    
      - TestSharedContextVBOES2NEWT0, TestSharedContextVBOES2NEWT3:
        - Add GPU mapped buffers tests to validate GLBufferObjectTracker
          code path with shared GLContext across multiple threads.
Comment 8 Sven Gothel 2014-01-21 19:00:34 CET
97f4ef2763596993bcb8a6b84150c9ec906dde08
    Better shared GLAutoDrawable synchronization.
    Block slave instances to also block until all master's GLEventListener.init(..) methods have been called
    
    - GLSharedContextSetter: Add areAllGLEventListenerInitialized()
      - GLCanvas (SWT, AWT)
      - GLJPanel
      - GLAutoDrawableBase (GLWindow, ..)
    
    - GLDrawableHelper's isSharedGLContextPending(..)
      takes 'areAllGLEventListenerInitialized()' into consideration
      allowing to block the slave creation until master is completed.
    
    This solves teh use case, where the master creates resources in it's
    GLEventListener initialization (buffers), which are shared with
    it's slaves.
Comment 9 Sven Gothel 2014-01-22 13:38:47 CET
e7002849ff0b55d86e351a9ccfc279bddd1a6d51
  Adding missing 'gl-if-CustomJavaCode-gl2.java' 
  of commit 09fc7aa5539731bb0fba835caee61f6eb837ecff