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.

Sign up for GitHub

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

Set BC7 sRGB flag for the relevant textures, and compress in parallel if no GPU #19

Merged
merged 3 commits into from
Apr 16, 2018
Merged

Set BC7 sRGB flag for the relevant textures, and compress in parallel if no GPU #19

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bbc7b5a
Compress textures in parallel if no GPU available, with TEX_COMPRESS_…
Feb 26, 2018
679ab98
Add and use BC7_SRGB compression format, for Oasis.
Feb 27, 2018
eda03af
Use BC7_SRGB compression format for emissive textures too.
Feb 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions glTF-Toolkit/inc/GLTFTextureCompressionUtils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ namespace Microsoft::glTF::Toolkit
BC3,
BC5,
BC7,
BC7_SRGB
};

/// <summary>
Expand Down
14 changes: 10 additions & 4 deletions glTF-Toolkit/src/GLTFTextureCompressionUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ GLTFDocument GLTFTextureCompressionUtils::CompressTextureAsDDS(const IStreamRead
outputImagePath += "_BC5";
break;
case TextureCompression::BC7:
case TextureCompression::BC7_SRGB:
outputImagePath += "_BC7";
break;
default:
Expand Down Expand Up @@ -196,8 +197,8 @@ GLTFDocument GLTFTextureCompressionUtils::CompressAllTexturesForWindowsMR(const
};

// Compress base and emissive texture as BC7
compressIfNotEmpty(material.metallicRoughness.baseColorTextureId, TextureCompression::BC7);
compressIfNotEmpty(material.emissiveTextureId, TextureCompression::BC7);
compressIfNotEmpty(material.metallicRoughness.baseColorTextureId, TextureCompression::BC7_SRGB);
compressIfNotEmpty(material.emissiveTextureId, TextureCompression::BC7_SRGB);

// Get other textures from the MSFT_packing_occlusionRoughnessMetallic extension
if (material.extensions.find(EXTENSION_MSFT_PACKING_ORM) != material.extensions.end())
Expand Down Expand Up @@ -237,6 +238,7 @@ void GLTFTextureCompressionUtils::CompressImage(DirectX::ScratchImage& image, Te
return;
}

DWORD compressionFlags = DirectX::TEX_COMPRESS_DEFAULT;
DXGI_FORMAT compressionFormat = DXGI_FORMAT_BC7_UNORM;
switch (compression)
{
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://github.com/Microsoft/DirectXTex/wiki/Compress this is implied if you use any SRGB format

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@walbourn walbourn Apr 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd make the _PARALLEL flag always be there, since there's no downside (it is always available on the Nuget version of DXTex).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

break;
default:
throw std::invalid_argument("Invalid compression specified.");
break;
Expand All @@ -265,7 +271,7 @@ void GLTFTextureCompressionUtils::CompressImage(DirectX::ScratchImage& image, Te

if (device != nullptr)
{
if (SUCCEEDED(DirectX::Compress(device.Get(), image.GetImages(), image.GetImageCount(), image.GetMetadata(), compressionFormat, DirectX::TEX_COMPRESS_DEFAULT, 0, compressedImage)))
if (SUCCEEDED(DirectX::Compress(device.Get(), image.GetImages(), image.GetImageCount(), image.GetMetadata(), compressionFormat, compressionFlags, 0, compressedImage)))
{
gpuCompressionSuccessful = true;
}
Expand All @@ -279,7 +285,7 @@ void GLTFTextureCompressionUtils::CompressImage(DirectX::ScratchImage& image, Te
if (!gpuCompressionSuccessful)
{
// Try software compression
if (FAILED(DirectX::Compress(image.GetImages(), image.GetImageCount(), image.GetMetadata(), compressionFormat, DirectX::TEX_COMPRESS_DEFAULT, 0, compressedImage)))
if (FAILED(DirectX::Compress(image.GetImages(), image.GetImageCount(), image.GetMetadata(), compressionFormat, compressionFlags | DirectX::TEX_COMPRESS_PARALLEL, 0, compressedImage)))
{
throw GLTFException("Failed to compress data using software compression");
}
Expand Down