Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/mobile/gl: change TexSubImage2D signature to allow providing a byte offset via data parameter #36355

Open
hajimehoshi opened this issue Jan 2, 2020 · 12 comments
Labels
Milestone

Comments

@hajimehoshi
Copy link
Contributor

@hajimehoshi hajimehoshi commented Jan 2, 2020

Originally, glTexSubImage2D can take either a pointer or an integer as data (http://www.khronos.org/opengles/sdk/docs/man3/html/glTexSubImage2D.xhtml). A pointer represents a pointer to a byte array, while an integer represents an offset of the current bound pixel buffer object. When using PBO, the latter way is required but the current Go mobile implementation does not allow this. I propose to enable to pass an integer to TexSubImage2D as a data parameter.

@hajimehoshi

This comment has been minimized.

Copy link
Contributor Author

@hajimehoshi hajimehoshi commented Jan 2, 2020

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 2, 2020

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

@hajimehoshi

This comment has been minimized.

Copy link
Contributor Author

@hajimehoshi hajimehoshi commented Jan 2, 2020

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

@hajimehoshi hajimehoshi changed the title x/mobile/gl: request: enable to pass an integer to TexSubImage2D instead of a slice x/mobile/gl: request: enable to pass nil to TexSubImage2D for buffer objects Jan 2, 2020
@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Jan 4, 2020

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

@hajimehoshi

This comment has been minimized.

Copy link
Contributor Author

@hajimehoshi hajimehoshi commented Jan 4, 2020

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.

@hajimehoshi

This comment has been minimized.

Copy link
Contributor Author

@hajimehoshi hajimehoshi commented Jan 4, 2020

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?

@hajimehoshi

This comment has been minimized.

Copy link
Contributor Author

@hajimehoshi hajimehoshi commented Jan 9, 2020

ping

@hajimehoshi

This comment has been minimized.

Copy link
Contributor Author

@hajimehoshi hajimehoshi commented Jan 13, 2020

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

@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Jan 13, 2020

@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.)

@hajimehoshi

This comment has been minimized.

Copy link
Contributor Author

@hajimehoshi hajimehoshi commented Jan 13, 2020

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?

@hajimehoshi

This comment has been minimized.

Copy link
Contributor Author

@hajimehoshi hajimehoshi commented Jan 15, 2020

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

@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Jan 16, 2020

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.
@dmitshur dmitshur changed the title x/mobile/gl: request: enable to pass nil to TexSubImage2D for buffer objects x/mobile/gl: change TexSubImage2D signature to allow providing a byte offset via data parameter Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.