Refactored external image handling in PlatformEGLAndroid #8512
Conversation
filament/src/details/Texture.cpp
Outdated
| @@ -187,6 +184,9 @@ Texture* Texture::Builder::build(Engine& engine) { | |||
| // SAMPLER_EXTERNAL implies imported. | |||
| if (mImpl->mTarget == SamplerType::SAMPLER_EXTERNAL) { | |||
| mImpl->mExternal = true; | |||
| } else { | |||
There was a problem hiding this comment.
nit: keep this up top and just condition on non-external, but maybe the real fix.
Also I wonder if the format here has to match the format we'd get from ahardwarebuffer.
There was a problem hiding this comment.
done re: nit
re: " if the format here has to match the format we'd get from ahardwarebuffer" - the format that was passed in via impress was directly derived from the hardwarebuffer via PlatformEGLAndroid::createExternalImage via ExternalImageEGLAndroid
There was a problem hiding this comment.
yes, but I mean in the general case; not every client is impress.
There was a problem hiding this comment.
Given that this whole model relies on the client registering an image or ahardwarebuffer via platform directly, I'm not sure an extra check is needed, but I will defer to FIlament team on if you want this added. We don't currently retain MBuffer on engine in this implementation, but I'm sure there's some way to derive it from the image handle.
There was a problem hiding this comment.
yeah, i'm not sure about this. This code has existed before for other (non AHB) external samplers. I wonder if skipping it entirely is ok. I'll leave this one to Mathias to comment.
alizahlalani
force-pushed
the
externalImageRefactor
branch
from
2d9d4ba to
be2c767
Compare
alizahlalani
force-pushed
the
externalImageRefactor
branch
from
be2c767 to
c2afe9b
Compare
| struct ExternalImageEGLAndroid : public ExternalImageEGL { | ||
| AHardwareBuffer* aHardwareBuffer = nullptr; | ||
| bool sRGB = false; | ||
| unsigned int width; // Texture width | ||
| unsigned int height; // Texture height | ||
| TextureFormat format;// Texture format | ||
| TextureUsage usage; // Texture usage flags | ||
| protected: | ||
| ~ExternalImageEGLAndroid() override; | ||
| }; |
There was a problem hiding this comment.
that definitely should not be public.
There was a problem hiding this comment.
resolved as discussed offline - I left the fields in ExternalImageEGLAndroid and made an extra metadata struct thats public to avoid additional AHardwareBufferDescribe calls in PlatformEGLAndroid::setImage which relies on the correct texture format and usage
filament/backend/include/backend/platforms/PlatformEGLAndroid.h
Outdated
Show resolved
Hide resolved
filament/backend/include/backend/platforms/PlatformEGLAndroid.h
Outdated
Show resolved
Hide resolved
| // Create and bind the OpenGL texture | ||
| glActiveTexture(GL_TEXTURE0); | ||
| glBindTexture(texture->target, texture->id); |
There was a problem hiding this comment.
If you change ANY GL state, you must restore it upon returning -- because the backend caches the state and the cache would become out of date.
There was a problem hiding this comment.
Trying to understand the lifecycle of this - where would I restore the state? In PlatformEGLAndroid?
There was a problem hiding this comment.
resolved as discussed offline
| return false; | ||
| } | ||
| glBindTexture(GL_TEXTURE_2D, prevTexture); |
There was a problem hiding this comment.
IRC the orders of the calls have to be in the opposite order, glActiveTexture first
Second, better restore the state before checking glEGLImageTargetTexture2DOES error so its properly restored also in the case of an error.
No description provided.