CC @hyangah @eliasnaur

Change https://golang.org/cl/213077 mentions this issue: gl: enable to use buffer objects for TexSubImage2D

Looks like TexImage2D or other functions already accepts nil when using buffers. TexSubImage2D should follow that.

@hajimehoshi In https://www.khronos.org/registry/OpenGL-Refpages/es3.0/html/glTexImage2D.xhtml, it states very clearly that:

data may be a null pointer. In this case, texture memory is allocated to accommodate a texture of width width and height height. You can then download subtextures to initialize this texture memory. The image is undefined if the user tries to apply an uninitialized portion of the texture image to a primitive.

I'm not seeing an equivalent note present at https://www.khronos.org/registry/OpenGL-Refpages/es3.0/html/glTexSubImage2D.xhtml.

In the original issue, do I understand correctly that you were referring to this part of glTexSubImage2D documentation?

If a non-zero named buffer object is bound to the GL_PIXEL_UNPACK_BUFFER target (see glBindBuffer) while a texture image is specified, data is treated as a byte offset into the buffer object's data store.

If a non-zero named buffer object is bound to the GL_PIXEL_UNPACK_BUFFER target (see glBindBuffer) while a texture image is specified, data is treated as a byte offset into the buffer object's data store.

Ah good point. My current understanding is that meanings of the null data are different between glTexImage2D and glTexSubImage2D: Null for glTexImage2D means allocation without initialization, while null for glTexSubImage2D means zero offset for a bound buffer.

To satisfy all glTexSubImage2D features, we might need a new function to pass uintptr instead. To my case, just 0 (nil) is enough, so I'm fine with passing nil to the existing function though.

I'm not confident but passing 0 (nil) accounts for the most usages of glTexSubImage2D with buffers, then I still want to pass nil to glTexSubImage2D. If we need other offset values, we can add another function. What do you think?

ping

I really want this feature. @dmitshur I was wondering if you are OK with passing nil or not.

@hajimehoshi Thanks for working on this.

To satisfy all glTexSubImage2D features, we might need a new function to pass uintptr instead. To my case, just 0 (nil) is enough, so I'm fine with passing nil to the existing function though.

I've talked with @hyangah and we're willing to consider breaking API changes to x/mobile/gl, which is experimental and v0, if we find out that one of its endpoints has a usability problem.

To me, this seems like such a case, because the current glTexSubImage2D API does not permit the caller to use some aspects of the underlying OpenGL call. Specifically, the caller cannot specify byte offset into the buffer object's data store when a non-zero named buffer object is bound to the GL_PIXEL_UNPACK_BUFFER target while a texture image is specified.

@hajimehoshi Given this, do you have a preference on whether we should proceed with adding support only for nil now without changing the signature of glTexSubImage2D (and defer handling non-zero offsets until someone needs it in the future), or should we change it to accept data as uintptr or unsafe.Pointer now?

If we consider making a change, we should determine which of uintptr or unsafe.Pointer is more suitable and memory-safe. (There's a chance it may not be possible to do this safely without adding another argument or a separate method.)

I'm fine with breaking the API :-) but there is a way not to break the API. The idea is creating a new API like BufferInit for BufferData. glBufferInit does not exist and BufferInit works as glBufferData with a null pointer. This is an interesting approach as original glBufferData has multiple meanings, and gomobile separated it into two.

Given this, do you have a preference on whether we should proceed with adding support only for nil now without changing the signature of glTexSubImage2D (and defer handling non-zero offsets until someone needs it in the future), or should we change it to accept data as uintptr or unsafe.Pointer now?

Another option would be passing interface{} which can be an int or a []byte. I prefer this to uintptr or unsafe.Pointer since 1) this breaks the API but most of existing code would work 2) taking a pointer of a slice should be done internally, not on the caller side.

BTW, I'm not sure whether even the current implementation is safe: what would happen if GC collects the given slice before GPU finishes the process?

@dmitshur So, what do you think? IMO, change the type from []byte to interface{} is the best.

I agree with that rationale for preferring changing type to interface{} and accepting []byte or int, instead of using uintptr. Let's do that. We should document the two types it accepts and what they're for, and make the implementation panic on any other type.

BTW, I'm not sure whether even the current implementation is safe: what would happen if GC collects the given slice before GPU finishes the process?

I understand the current implementation is safe because it sets blocking to true. From a comment in fn.go:

A GL call is marked as blocking if it returns a value, or if it takes a
Go pointer. In this case the call will not return until C.process sends a
value on the retvalue channel.