[Impeller] change default sampler descriptor to use nearest mip level and remove kNone #40460
Conversation
There was a problem hiding this comment.
This is the more accurate default. If there is only one mip level, then the nearest mip index should be zero. I can't think of any other case where we'd need to have mip levels but always sample from zero. Makes me wonder why @bdero preferred trilinear filtering here https://github.com/flutter/engine/pull/40425/files though. So he may have additional context.
|
Ok, we should remove MipFilter::kNone altogether. It is not supported in Vulkan either. |
jonahwilliams
changed the title
| @@ -74,7 +67,7 @@ bool SamplerGLES::ConfigureBoundTexture(const TextureGLES& texture, | |||
| gl.TexParameteri(target.value(), GL_TEXTURE_MIN_FILTER, | |||
| ToParam(desc.min_filter, desc.mip_filter)); | |||
| gl.TexParameteri(target.value(), GL_TEXTURE_MAG_FILTER, | |||
| ToParam(desc.mag_filter, MipFilter::kNone)); | |||
| ToParam(desc.mag_filter, MipFilter::kNearest)); | |||
There was a problem hiding this comment.
@chinmaygarde what is this supposed to be? This line seems sus before
There was a problem hiding this comment.
It was patched by @bdero in 4becf4942e8. Looking closer.
There was a problem hiding this comment.
I'm not sure why I changed this when landing the blit pass... This might be debugging leftovers. I'd try changing it back and seeing if Play/RendererTest.CanGenerateMipmaps/OpenGLES is broken once this is addressed.
There was a problem hiding this comment.
Just reread this and realized kNone is actually correct here because of #40460 (comment)
| @@ -21,13 +21,6 @@ bool SamplerGLES::IsValid() const { | |||
|
|
|||
| static GLint ToParam(MinMagFilter minmag_filter, MipFilter mip_filter) { | |||
| switch (mip_filter) { | |||
| case MipFilter::kNone: | |||
There was a problem hiding this comment.
I think sampling with GL_[NEAREST|LINEAR]_MIPMAP_[NEAREST|LINEAR] will fail in GLES if the mip count is 1, and either GL_NEAREST or GL_LINEAR needs to be set when this is the case.
There was a problem hiding this comment.
Please verify that this doesn't break the GLES playgrounds if landing.
There was a problem hiding this comment.
we can retain this behavior for now without the enum variant. Make the mip_filter optional and treat nullopt as if it were none.
There was a problem hiding this comment.
This still needs to be addressed for the min filter. Right now, if a texture has a mip count of 1 (as all subpass and filter-created textures do), it will get a min filter that includes mipmap sampling. This will fail and sample with all zeros on some drivers:
There was a problem hiding this comment.
The capture above is Play/DisplayListTest.CanDrawBackdropFilter/OpenGLES, which was rendering nothing a couple of days ago on both M1 Mac and the Windows Nvidia driver prior to this fix: https://github.com/flutter/engine/pull/40425/files
jonahwilliams
added
the
autosubmit
… mip level and remove kNone (flutter/engine#40460)
By default we create mipmaps for every decoded image. Choosing the nearest mip level is the only reasonable default.