powervr-graphics / Native_SDK

C++ cross-platform 3D graphics SDK. Includes demos & helper code (resource loading etc.) to speed up development of Vulkan, OpenGL ES 2.0 & 3.x applications
https://docs.imgtec.com/sdk-documentation/html/introduction.html
MIT License
702 stars 197 forks source link

Disable mipmapping for default bilinear sampler #51

Closed c0d1f1ed closed 4 years ago

c0d1f1ed commented 4 years ago

Textures used for text rendering are created without mipmaps, but the sampler is configured to perform nearest mipmap filtering. This makes the texture incomplete:

OpenGL ES 3.2 section 8.17.1: "if the texture is not mipmap complete and the sampler object specifies a TEXTURE_MIN_FILTER requiring mipmaps, the texture will be considered incomplete for the purposes of that texture unit"

This fixes text rendering with ANGLE.

graptis commented 4 years ago

Hi c0d1f1ed, thank you very much for the contribution! I was also looking into this, and my opinion is that it is a bit less clear-cut than it originally looks:

From OpenGL ES 3.0 on - UIRenderer allocates the textures using glTexStorage, so the textures should be mipmap complete, regardless of the amount of used mipmaps, as it should be handled automatically, so GL_LINEAR_MIPMAP_NEAREST should work. See below: https://www.khronos.org/opengl/wiki/Texture#Mipmap_completeness "Textures that use storage allocated with immutable storage functions (and Buffer Textures) will always be mipmap complete. For other textures, the following rules apply..."

The underlying reason for that being that mipmap completeness just means "have defined as many mipmaps as expected" and immutable sets this expectation to the number actually provided. I believe it should be perfectly fine to use ..._MIPMAP_NEAREST or ..._MIPMAP_LINEAR with a mipmap-complete single-mipmap texture.

That said, I have already implemented a different workaround with a slightly different solution , it's just you sort of beat me to the punch with this: My workaround was to always set the glTexParameteri samplers for our textures regardless of if we are using OpenGL ES 2.0 or 3.0 (i.e. in the UIRendererGles.cpp::createImage function, remove the "if (api==OpenGLES2)" and if we are using sampler objects. However this might not cover your case. Would you mind telling me which example you were using? I used swiftshader for testing the SDK and for the examples I used the issue was fixed.

Update: My workaround does not fix all the SDK examples, only ones that use textures without samplers.

graptis commented 4 years ago

Just to clarify something - the reason I am still looking at this is that I since we are talking about a UI renderer, it is very conceivable for a user to not want to use trilinear, but still want to use mipmaps to select e.g. the font mipmap level that most closely matches a resolution. Hence, the bilinear sampler, if it can be made to work in all cases, should be able to accomodate any texture. At least, as I mentioned, that's the intent.

c0d1f1ed commented 4 years ago

Thanks for having a closer look at this!

I agree having a full mipmap chain is preferred for UI rendering. I was just making the minimal change that preserved the previously intended behavior of just sampling the one level.

Note that https://www.khronos.org/opengl/wiki/Texture#Mipmap_completeness is actually not normative, despite being hosted by Khronos. As far as I'm aware the OpenGL ES 3.x spec makes no mention of glTexStorage always producing a complete texture. The Wiki appears to reflect legacy behavior of the ARB_texture_storage extension: https://www.khronos.org/registry/OpenGL/extensions/ARB/ARB_texture_storage.txt

Can we land this patch as the short-term fix to make things work identically on other drivers, and then make the changes to add mipmaps to these UI textures?

graptis commented 4 years ago

Yes. Let me have a few hours to see if I can't divine some other workaround that would allow me to still use the mipmapped fonts in other cases, and I'll merge it in :).

As far as the other issue though, yes the statement itself is not in the OpenGL ES specification, but the result is - i.e. even though the spec does not explicitly state so like in OpenGL, it is still (as far as I can tell) impossible to actually get a mipmap-incomplete texture with glTexStorage2D because you are stating its number of mipmap levels in the glTexStorage2D call, and in any case in our case it is not.

This is unfortunately implied all over the OpenGL ES spec, checking p's and q's - but as far as I can tell the most telling part is In https://www.khronos.org/registry/OpenGL/specs/es/3.2/es_spec_3.2.pdf : Page 203:

For immutable-format textures, p is one less than levelimmut. Otherwise, p = blog2 (maxsize)c + levelbase, and all arrays from levelbase through q = min{p, levelmax} must be defined, as discussed in section 8.17.

where levelimmut is the number of levels you have passed in glTexStorageXD Then if you follow the p's and q's later you can see that everything is well-defined.

In other words, in a non-immutable texture, you must manually define all texture levels implied by its size. In an immutatble texture, you (automatically) define as many levels as you will have.

c0d1f1ed commented 4 years ago

You're right, this looks like a bug on our end! The key part of the OpenGL ES 3.x spec is:

Array levels k where k < levelbase or k > q are insignificant to the definition of completeness.

Where q is indeed no larger than levelmax, which is in turn limited to levelimmut - 1 which was specified at glTexStorage.

I'll abandon this change since it hides a bug, and work on the proper fix in SwiftShader instead.

Thanks again!