Bug 1180

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.

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

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.

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

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.

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

(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

gluegen c7edf7debd03ac688fca32d91b4f98f21de2a7af:
  Add test for remaining()

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


.. so far, everything passes

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.

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.