Set BC7 sRGB flag for the relevant textures, and compress in parallel if no GPU #19
Conversation
erikdahlstrom
changed the title
|
Pardon my ignorance, but what's the advantage of the BC7_SRGB compression? |
| @@ -249,6 +251,10 @@ void GLTFTextureCompressionUtils::CompressImage(DirectX::ScratchImage& image, Te | |||
| case TextureCompression::BC7: | |||
| compressionFormat = DXGI_FORMAT_BC7_UNORM; | |||
| break; | |||
| case TextureCompression::BC7_SRGB: | |||
| compressionFormat = DXGI_FORMAT_BC7_UNORM_SRGB; | |||
| compressionFlags |= DirectX::TEX_COMPRESS_SRGB_IN; | |||
There was a problem hiding this comment.
According to https://github.com/Microsoft/DirectXTex/wiki/Compress this is implied if you use any SRGB format
There was a problem hiding this comment.
BC7_SRGB is the same format, it just indicates that the texture is explicitly in the sRGB (gamma 2.2) space instead of in some other (usually linear) colorspace. The use of DirectX::TEX_COMPRESS_SRGB_IN here is going to make sure that both the 'in' and 'out' are considered sRGB, so no gamma ramp is applied in this case.
Otherwise, if the input image is not in sRGB (gamma 2.2) space, it will have that conversion applied.
There was a problem hiding this comment.
DirectX::TEX_COMPRESS_PARALLEL only applies to the CPU compressor and only works if the library itself was built with OpenMP enabled--otherwise you get a E_NOTIMPL.
There was a problem hiding this comment.
All images loaded by GLTFTextureLoadingUtils use DirectX::WIC_FLAGS_IGNORE_SRGB. Since the GLTF spec requires sRGB for the emissive and basecolor textures an assumption is made that this is the case. And like @walbourn says we need to flag that we know that both in and out are sRGB to avoid applying gamma (making the output brighter than it should be). The other difference is that the DDS output texture will have the format (DXGI_FORMAT_BC7_UNORM_SRGB) written into the file. The result when using DXGI_FORMAT_BC7_UNORM is that the rendered result will look too bright if the rendering engine applies gamma to convert the texture to sRGB space. Some DDS viewers do this, some don't, but there's certainly no harm in tagging the textures correctly.
There was a problem hiding this comment.
Possibly the DirectX::TEX_COMPRESS_PARALLEL flag could be tried first, and if E_NOTIMPL is returned we retry without it, how does that sound?
I can add as a datapoint regarding performance, that with DirectX::TEX_COMPRESS_PARALLEL and a common scenario with 4 * 2048x2048px textures, the time it takes to pack textures is roughly half of what it is without the flag specified.
However, I'm also looking at possibly using DirectX::TEX_COMPRESS_BC7_QUICK in some cases, noting that that does result in slightly worse quality - any ideas for how to best include that? I could add a new commandline flag and pass that in.
There was a problem hiding this comment.
If you are using the NuGet package for DirectXTex.lib, then likely it's built with OpenMP enabled.
For BC7, the quick mode is mostly a hack for testing. It only ever uses mode 6 which isn't much better than just using BC1/BC2/BC3.
There was a problem hiding this comment.
I'd make the _PARALLEL flag always be there, since there's no downside (it is always available on the Nuget version of DXTex).
There was a problem hiding this comment.
Ok, given your comments I have nothing more I want to add to this PR, feel free to merge this if you think it's ready.
No description provided.