Bug 1180

Summary: Buffers.newDirectIntBuffer(int[]) does not set limit
Product: [JogAmp] Gluegen Reporter: johan
Component: coreAssignee: Sven Gothel <sgothel>
Status: RESOLVED INVALID    
Severity: normal    
Priority: ---    
Version: 2.3.2   
Hardware: All   
OS: all   
Type: --- SCM Refs:
gluegen ce9187bbbf62389fc7897a87f36952cdd23674f6 gluegen c7ecc12a3b9281360e2121f02e9985be3b680f7f gluegen c7edf7debd03ac688fca32d91b4f98f21de2a7af jogl 8e1f5fc43ba84d5e6373f0c29089ac32b7ce95dd
Workaround: ---
Attachments: gluegen-rewind-to-clear.patch
junit-Test-Buffers-positionLimitCapacityAfterArrayAllocation.patch

Description johan 2015-07-24 15:41:45 CEST
Discussion here
http://forum.jogamp.org/Buffers-newDirectIntBuffer-int-does-not-set-limit-td4034959.html

Details reproduced from the forum here:
I noticed that Buffers.newDirectIntBuffer(int[]) does not set the limit of the buffer. This causes a driver crash on OSX. 

When using this form: 
final IntBuffer indexBuf = Buffers.newDirectIntBuffer(triIndexes);
This is the trace output when I upload the data to the driver: 
glBindBuffer(<int> 0x8893, <int> 0x3)
glBufferData(<int> 0x8893, <long> 0, <java.nio.Buffer> java.nio.DirectIntBufferU[pos=0 lim=0 cap=89034], <int> 0x88E4)

and finally the driver crashes when it is time to render: 
glDrawElements(<int> 0x4, <int> 0x15BCA, <int> 0x1405, <long> 0)#
# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGSEGV (0xb) at pc=0x000000010fd4b3b0, pid=1443, tid=31031
#
# JRE version: Java(TM) SE Runtime Environment (8.0-b132) (build 1.8.0-b132)
# Java VM: Java HotSpot(TM) 64-Bit Server VM (25.0-b70 mixed mode bsd-amd64 compressed oops)
# Problematic frame:
# C  [libsystem_platform.dylib+0x13b0]  _platform_memmove$VARIANT$Nehalem+0x70

But if I do this: 
final IntBuffer indexBuf = Buffers.newDirectIntBuffer(triIndexes.length);
for (int triIndex : triIndexes) {
	indexBuf.put(triIndex) ;
}

The trace for uploading the buffer is this: 
glBindBuffer(<int> 0x8893, <int> 0x3)
glBufferData(<int> 0x8893, <long> 356136, <java.nio.Buffer> java.nio.DirectIntBufferU[pos=0 lim=89034 cap=89034], <int> 0x88E4)

Notice the limit is set to the number of ints I put into the buffer and now the driver does not crash.
Comment 1 johan 2015-07-24 15:46:08 CEST
In the forum Xerxes suggested using niobuffer#clear() instead of rewind. Maybe flip() is more appropriate in this case. Anyway, the forum has a patch from Xerxes that I currently can not test since I haven't set up to build gluegen.
Comment 2 Xerxes Rånby 2015-07-24 16:52:47 CEST
Created attachment 708 [details]
gluegen-rewind-to-clear.patch

I took a quick look and wrote a proposed "fix". By replacing the use of NIO Buffer rewind with clear.
gluegen-rewind-to-clear.patch
Please test!

All gluegen junit.run tests pass on my machine with the patch applied.

Using clear sounds counter intuitive but the java doc confirm that it is the right thing to do.

http://docs.oracle.com/javase/7/docs/api/java/nio/Buffer.html#clear%28%29
"public final Buffer clear()

Clears this buffer. The position is set to zero, the limit is set to the capacity, and the mark is discarded.

Invoke this method before using a sequence of channel-read or put operations to fill this buffer. For example:

         buf.clear();     // Prepare buffer for reading
         in.read(buf);    // Read data

This method does not actually erase the data in the buffer, but it is named as if it did because it will most often be used in situations in which that might as well be the case."
Comment 3 Xerxes Rånby 2015-07-24 18:56:12 CEST
Created attachment 709 [details]
junit-Test-Buffers-positionLimitCapacityAfterArrayAllocation.patch

junit test to check that position limit and capacity is set as intended after Buffers.newDirect*Buffer(*[]) allocation.
Comment 4 Sven Gothel 2015-07-24 19:02:01 CEST
(In reply to comment #2)
> Created attachment 708 [details]
> gluegen-rewind-to-clear.patch
> 
> I took a quick look and wrote a proposed "fix". By replacing the use of NIO
> Buffer rewind with clear.

ByteBuffer.asIntBuffer():
The new buffer's position will be zero, its capacity and its limit
will be the number of bytes remaining in this buffer divided by
four, and its mark will be undefined.

For all the array NIO Buffer create method in Buffers, we do as:
  'ByteBuffer' -> as<Type>() -> nativeOrder -> 'fill data' -> rewind()

The limit/capacity shall be handled by 'as<Type>()', see above.

Hence the patch shall not be required.

+++

Further, we need to see how the 'limit' gets broken 
and why (and how) it is used in our glBufferData(..) ?

Quite weird ..
Comment 5 johan 2015-07-24 19:43:09 CEST
I am pressed for time so can not make a test case for several days. However I strongly suspect the reason for the driver crash is my code. I do this:


gl.glBindBuffer(GL4.GL_ELEMENT_ARRAY_BUFFER, this.vbos[bufferIndex]);
final int bufferSizeInBytes = values.remaining() * Buffers.SIZEOF_INT;
gl.glBufferData(GL4.GL_ELEMENT_ARRAY_BUFFER, bufferSizeInBytes, values, GL4.GL_STATIC_DRAW);


Notice that I use 'remaining()' to see how much data is in the buffer. Remaining is the difference between limit-position. So my code assumes that the buffer is at position 0 and limit is set to the number of elements in the buffer. When limit is 0 that is probably the reason for breaking the driver. 

However, I would assume (maybe falsely) that:
Buffers.newDirectIntBuffer(int[]) would be equivalent to allocating a buffer and then 'put' all the data from the array i.e. limit should be the number of elements in the array. So that I can flip() the buffer and be ready to read from it.
Comment 6 Sven Gothel 2015-07-24 22:02:42 CEST
(In reply to comment #3)
> Created attachment 709 [details]
> junit-Test-Buffers-positionLimitCapacityAfterArrayAllocation.patch
> 
> junit test to check that position limit and capacity is set as intended
> after Buffers.newDirect*Buffer(*[]) allocation.

Your git patch has been merged.

Minor nagging:

Please use 'Bug 1180' instead of any other notation
in you git commit message and elsewhere, 
otherwise we have a hard time to find related pieces of code.
Comment 7 Sven Gothel 2015-07-24 22:04:54 CEST
(In reply to comment #5)
> I am pressed for time so can not make a test case for several days. However
> I strongly suspect the reason for the driver crash is my code. I do this:
> 
> 
> gl.glBindBuffer(GL4.GL_ELEMENT_ARRAY_BUFFER, this.vbos[bufferIndex]);
> final int bufferSizeInBytes = values.remaining() * Buffers.SIZEOF_INT;
> gl.glBufferData(GL4.GL_ELEMENT_ARRAY_BUFFER, bufferSizeInBytes, values,
> GL4.GL_STATIC_DRAW);
> 
> 
> Notice that I use 'remaining()' to see how much data is in the buffer.
> Remaining is the difference between limit-position. So my code assumes that
> the buffer is at position 0 and limit is set to the number of elements in
> the buffer. When limit is 0 that is probably the reason for breaking the
> driver. 

Thank you  ... I will attempt to make a unit test.

> 
> However, I would assume (maybe falsely) that:
> Buffers.newDirectIntBuffer(int[]) would be equivalent to allocating a buffer
> and then 'put' all the data from the array i.e. limit should be the number
> of elements in the array. So that I can flip() the buffer and be ready to
> read from it.

Yes .. limit shall be equal to capacity.

A unit test for Buffers in this regard has been added to GlueGen:
ce9187bbbf62389fc7897a87f36952cdd23674f6
c7ecc12a3b9281360e2121f02e9985be3b680f7f
Comment 8 Sven Gothel 2015-07-24 22:53:06 CEST
gluegen c7edf7debd03ac688fca32d91b4f98f21de2a7af:
  Add test for remaining()

jogl 8e1f5fc43ba84d5e6373f0c29089ac32b7ce95dd:
  Refine TestMapBufferRead01NEWT, add TestMapBufferRead02NEWT: 
    Add assertion checks and latter test uses FloatBuffer


.. so far, everything passes
Comment 9 johan 2015-07-25 09:29:33 CEST
The problem was that I assumed position to be set so that the buffer should be flipped before reading from it. This is not the case:

public class BuffersTest {
	@Test
	public void testFlipReadOnExplicitlyPutBuffer() {
		IntBuffer explicit = Buffers.newDirectIntBuffer(3);
		explicit.put(1) ;
		explicit.put(2) ;
		explicit.put(3) ;
		explicit.flip() ;
		
		assertTrue(explicit.get() == 1) ;
		assertTrue(explicit.get() == 2) ;
		assertTrue(explicit.get() == 3) ;
	}
	
	@Test
	public void testFlipReadOnArrayCreatedBuffer() {
		IntBuffer implicit = Buffers.newDirectIntBuffer(new int[] {1,2,3});
		implicit.flip() ;
		
		assertTrue(implicit.get() == 1) ;
		assertTrue(implicit.get() == 2) ;
		assertTrue(implicit.get() == 3) ;
	}
}

I don't think this is a bug, more like I made the wrong assumption about how the Buffers class worked. Will close this issue if I can.
Comment 10 Sven Gothel 2015-07-25 14:38:42 CEST
Indeed, the Buffers method acts as if flip() has already being called,
i.e. pos=0 and limit=capacity=array.lenght.

Hence this bug report is invalid, not 'fixed'.

Thank you.

We may want to consider to add API documentation 
about the state of created buffers, i.e. pos, limit and capacity.