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

Specify a MIN_FILTER for render target textures #220

Closed
wants to merge 1 commit into from

Conversation

jwmcglynn
Copy link
Contributor

Previously when rendering with Swiftshader, enabling the postprocessing
pipeline would result in nothing being rendered to the screen.

I narrowed down the issue due to Swiftshader not considering the
postProcess_colorBuffer sampler as complete. This occurs because the
default min filter for textures is GL_NEAREST_MIPMAP_LINEAR, which
requires mipmaps, and the render target texture doesn't include those,
so Swiftshader doesn't consider the texture to be complete when it's
binding it to a sampler, resulting in it failing to bind.

The OpenGL ES 3.1 spec is vague around this area, see section 8.16.1,
which states that if the sampler min filter requires mipmaps and the
texture doesn't include them the sampler may be considered incomplete,
but it doesn't say what happens if the texture is incomplete by itself
and complete after the sampler min filter overrides the texture min
filter.

Since the language is vague, and the current behavior is arguably
incorrect, update OpenGLDriver::textureStorage to explicitly specify a
min filter so that render target textures are considered complete
standalone.

Previously when rendering with Swiftshader, enabling the postprocessing
pipeline would result in nothing being rendered to the screen.

I narrowed down the issue due to Swiftshader not considering the
postProcess_colorBuffer sampler as complete.  This occurs because the
default min filter for textures is GL_NEAREST_MIPMAP_LINEAR, which
requires mipmaps, and the render target texture doesn't include those,
so Swiftshader doesn't consider the texture to be complete when it's
binding it to a sampler, resulting in it failing to bind.

The OpenGL ES 3.1 spec is vague around this area, see section 8.16.1,
which states that if the sampler min filter requires mipmaps and the
texture doesn't include them the sampler may be considered incomplete,
but it doesn't say what happens if the texture is incomplete by itself
and complete after the sampler min filter overrides the texture min
filter.

Since the language is vague, and the current behavior is arguably
incorrect, update OpenGLDriver::textureStorage to explicitly specify a
min filter so that render target textures are considered complete
standalone.
@romainguy
Copy link
Collaborator

We usually don't use glTexParameter, we use sampler objects instead. The render target path is the only place where we don't (can't) use samplers, that's why I'd rather we change the texture parameter there instead.

@jwmcglynn
Copy link
Contributor Author

jwmcglynn commented Sep 6, 2018

Yeah, in this situation the sampler is correct and initialized with a min filter of GL_LINEAR, but Swiftshader is failing because the texture itself isn't complete, per the check in swiftshader at:

https://github.com/google/swiftshader/blob/a062f321768dc39a533188806c571cf8a9479ca7/src/OpenGL/libGLESv2/Context.cpp#L3090
(calls into)
https://github.com/google/swiftshader/blob/a062f321768dc39a533188806c571cf8a9479ca7/src/OpenGL/libGLESv2/Texture.cpp#L724

@jwmcglynn
Copy link
Contributor Author

Romain, to confirm you're proposing making this change in OpenGLDriver::createRenderTarget instead?

@romainguy
Copy link
Collaborator

Yes that's what I'm proposing. I just want to avoid messing with state as much as possible.

@c0d1f1ed
Copy link

c0d1f1ed commented Sep 7, 2018

This workaround shouldn't be needed once the SwiftShader-side fix lands: https://swiftshader-review.googlesource.com/20529

@jwmcglynn
Copy link
Contributor Author

Oh nice! Filament is yet again a stress test against the OpenGL ES API :-)

Given this new information, would you like me to continue on this fix? This is an uncommon scenario, with the combination of samplers, creating a texture without explicitly setting the glTexParameters, and using the incomplete texture, but it does appear to be accepted everywhere else.

@romainguy
Copy link
Collaborator

I wouldn't mind doing the right thing here for render targets. Up to you :)

@c0d1f1ed
Copy link

c0d1f1ed commented Sep 7, 2018

Using sampler objects instead of setting the texture's parameters is definitely the modern way of using OpenGL, and shouldn't be considered uncommon. It's a blatant bug in SwiftShader for not handling it correctly, and I'm happy Filament exposed it. I'll look into why dEQP didn't catch it.

Concretely, I would advise against working around this bug. Our patch should land shortly.

@romainguy
Copy link
Collaborator

Alright, thanks!

@romainguy romainguy closed this Sep 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants