-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Added support for the extension OES_EGL_image_external(_essl3) for GLES2 #2021
Conversation
@@ -50,7 +50,7 @@ static inline const char* get_precision_string (glsl_precision p) | |||
static const int tex_sampler_type_count = 7; | |||
// [glsl_sampler_dim] | |||
static const char* tex_sampler_dim_name[tex_sampler_type_count] = { | |||
"1D", "2D", "3D", "Cube", "Rect", "Buf", "External", | |||
"1D", "2D", "3D", "Cube", "Rect", "Buf", "2D", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"2D" already appears in array...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The glsl-optimizer replaced all texture lookups in the shader that used a "uniform samplerExternalOES" with the function "textureExternal" (which is deprecated) instead of "texture2D" (which also supportes samplerExternal).
e.g. the input:
uniform lowp samplerExternalOES s_texColor;
...
gl_FragColor = texture2D(s_texColor, v_texcoord0)
would become:
uniform lowp samplerExternalOES s_texColor;
...
gl_FragColor = textureExternal(s_texColor, v_texcoord0)
I felt it was easiest to just change the name of the sampler in the array since the rest was handled correctly
scripts/bgfx.idl
Outdated
--- yet from the main thread. | ||
.handle "TextureHandle" --- Texture handle. | ||
.ptr "uintptr_t" --- Native API pointer to texture. | ||
.target "uintptr_t" --- Native API sampler target type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are possible "target" enums? I'm just curious what user should use here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In our case when using a texture from the Camera/MediaPlayer/SurfaceTexture on Android we pass GL_TEXTURE_EXTERNAL_OES as the target. And it could be GL_TEXTURE_RECTANGLE when using the textures from CoreVideo on osx with OpenGL (even if that is deprecated)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would then figure out better way to handle this, and not introduce another API. For example, since this is only GL thing, you could use overrideInternal
, and then use GL shader reflection to set GL_TEXTURE_EXTERNAL_OES
for sampler correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I am not 100% sure what you mean by "gl shader reflection", but this is a possible alternative solution that does not expose any new api.
I do not like the #else part, but I have not found any other way to get the target from a texture id for older opengl versions / opengl es.
void TextureGL::overrideInternal(uintptr_t _ptr)
{
destroy();
m_flags |= BGFX_SAMPLER_INTERNAL_SHARED;
m_id = (GLuint)_ptr;
#if BGFX_CONFIG_RENDERER_OPENGL >= 43
// The target of a texture can be checked directly
GLenum target;
glGetTexParameteriv(m_id, GL_TEXTURE_TARGET, &target);
m_target = target;
#else
// Try to bind the texture to all possible targets
// and selects the one that does not give an error
const GLenum s_targets[] = {
GL_TEXTURE_2D,
// ... etc ...
GL_TEXTURE_EXTERNAL_OES,
};
for(int i = 0; i < sizeof(s_targets); i++){
GLenum entry = s_targets[i];
glBindTexture(target, m_id);
// NOTE: might need to rebind the previous bound texture here
if(0 == glGetError()){
m_target = target;
break;
}
}
#endif
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, in code you detect that program is using GLSL_TYPE(GL_SAMPLER_EXTERNAL_OES);
and at that point you can assume that whatever if bound to that sampler slot will need to use GL_TEXTURE_EXTERNAL_OES
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reworked the pullrequest so that the user-api is unchanged and the texture is bound as a GL_SAMPLER_EXTERNAL_OES if the shader uses that type. Do you think it looks better this way? Any other changes that should be done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it looks much better.
This looks useful. I'm currently figuring out Android MediaCodec decoding via ffmpeg and the best way for interop with bgfx. It's tricky but it seems like I need to: Setup:
Each frame:
PS and for purely selfish reasons, I liked the suggestion of keeping the same API and having this automatic, because my change to propagate overrideInternal through the updateTexture2D system from a while back (#1984) would be able to use this transparently. |
bf2b6df
to
3f5ac15
Compare
src/renderer_gl.cpp
Outdated
if (m_numSamplers < BX_COUNTOF(m_sampler) ) | ||
{ | ||
BX_TRACE("Sampler #%d at location %d.", m_numSamplers, loc); | ||
m_sampler[m_numSamplers] = loc; | ||
GLenum target = gltype == GL_SAMPLER_EXTERNAL_OES ? GL_TEXTURE_EXTERNAL_OES : 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're already saving target, if there a good reason why you should not save it for all samplers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not. The only reason I did it like that was to only change how the GL_SAMPLER_EXTERNAL_OES was handled and keep the other textures/samplers as before. But the value is only used to bind the texture in TextureGL::commit, so there should not be any problem by saving it for all samplers. I can fix that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or one other reason is that we have to map each sampler to their correct target (e.g. GL_SAMPLER_2D -> GL_TEXTURE_2D). I am not sure how samplers with GL_IMAGE_* should be mapped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think something like this could work?
GLenum glSamplerTarget(GLenum _sampler){
switch (_sampler)
{
case GL_SAMPLER_2D:
case GL_INT_SAMPLER_2D:
case GL_UNSIGNED_INT_SAMPLER_2D:
case GL_SAMPLER_2D_SHADOW:
return GL_TEXTURE_2D;
...
case GL_IMAGE_1D:
case GL_IMAGE_2D:
...
return 0;
}
BX_CHECK(false, "Unrecognized GL sampler type 0x%04x.", _sampler);
return 0;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a solution where the target is saved for each sampler type, and not just for GL_SAMPLER_EXTERNAL_OES.
I noticed that the AppVeyor build failed, but the errors does not look like they originate from my changes. Or what do you think, @bkaradzic ?
scripts/bgfx.idl
Outdated
--- yet from the main thread. | ||
.handle "TextureHandle" --- Texture handle. | ||
.ptr "uintptr_t" --- Native API pointer to texture. | ||
.target "uintptr_t" --- Native API sampler target type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it looks much better.
3f5ac15
to
0beff63
Compare
Can you figure out why CI is failing? |
The CI is failing because the latest version of imgui includes the "windows.h" header without NOMINMAX being defined before, so the min/max functions in bx.h conflicts with the windows macros. bgfx/3rdparty/dear-imgui/imgui.cpp Line 1522 in aea0759
|
Fixed. |
0beff63
to
67ca879
Compare
I fetched your fix and the CI succeedes now : ) |
Hi again! Just bumping this pr to see if it acceptable or if something more needs to be changed? : ) |
All good 👍 |
…ES2/3 (bkaradzic#2021) Co-authored-by: Gabriel <gabriel.sulka@visiarc.com>
* master: Allow passing config macros from env (bkaradzic#2147) Update examples to build on Emscripten Wasm. Add separate wasm and wasm2js targets. (bkaradzic#2145) Updated meshoptimizer. Updated ImGui. Updated vulkan headers. Updated spirv-cross. Updated spirv-tools. Updated glslang. Cleanup. Fixed build. Cleanup dead code (bkaradzic#2143) Updated ImGui. Updated ImGui. Reverted bkaradzic#2021.
Hi!
I added support for the OpenGL ES extension OES_EGL_image_external. We use this on Android to be able to render textures from the camera and video player without copying them to a regular Texture2D.
This adds a function "bgfx::overrideInternal(ptr, target)" to the api, that allows the user to override the sampler target, and a "SAMPLEREXTERNAL" macro to use in the shaders.
I also patched glsl-optimizer to correctly handle the uniform "samplerExternal".
On backends other then OpenGL ES, the SAMPLEREXTERNAL is treated as a regular SAMPLER2D
Any feedback is welcome : )