-
Notifications
You must be signed in to change notification settings - Fork 452
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
Add FP16 NaN/infinity handling flags #100
Open
elasota
wants to merge
5
commits into
microsoft:main
Choose a base branch
from
elasota:fp16-flags
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
549e4ba
Move half float sanitization out of StoreScanline and to Convert, add…
elasota cbfe494
Merge branch 'main' into fp16-flags
walbourn 1b531b2
Update DirectXTex.h
walbourn e610950
Update DirectXTexConvert.cpp
walbourn 691e4e5
Update DirectXTexConvert.cpp
walbourn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1659,9 +1659,7 @@ bool DirectX::Internal::StoreScanline( | |
for (size_t icount = 0; icount < (size - sizeof(XMHALF4) + 1); icount += sizeof(XMHALF4)) | ||
{ | ||
if (sPtr >= ePtr) break; | ||
XMVECTOR v = *sPtr++; | ||
v = XMVectorClamp(v, g_HalfMin, g_HalfMax); | ||
XMStoreHalf4(dPtr++, v); | ||
XMStoreHalf4(dPtr++, *sPtr++); | ||
} | ||
return true; | ||
} | ||
|
@@ -1852,9 +1850,7 @@ bool DirectX::Internal::StoreScanline( | |
for (size_t icount = 0; icount < (size - sizeof(HALF) + 1); icount += sizeof(HALF)) | ||
{ | ||
if (sPtr >= ePtr) break; | ||
float v = XMVectorGetX(*sPtr++); | ||
v = std::max<float>(std::min<float>(v, 65504.f), -65504.f); | ||
*(dPtr++) = XMConvertFloatToHalf(v); | ||
*(dPtr++) = XMConvertFloatToHalf(XMVectorGetX(*sPtr++)); | ||
} | ||
return true; | ||
} | ||
|
@@ -3723,6 +3719,27 @@ void DirectX::Internal::ConvertScanline( | |
} | ||
} | ||
} | ||
|
||
// Half-float sanitization | ||
if (((out->flags & (CONVF_FLOAT | CONVF_DEPTH)) == CONVF_FLOAT) | ||
&& (out->datasize == 16) | ||
&& ((flags & (TEX_FILTER_FLOAT16_SATURATE_TO_INF | TEX_FILTER_FLOAT16_KEEP_NANS)) != (TEX_FILTER_FLOAT16_SATURATE_TO_INF | TEX_FILTER_FLOAT16_KEEP_NANS))) | ||
{ | ||
const XMVECTOR zero = XMVectorZero(); | ||
XMVECTOR* ptr = pBuffer; | ||
for (size_t i = 0; i < count; ++i, ++ptr) | ||
{ | ||
XMVECTOR v = *ptr; | ||
|
||
if (!(flags & TEX_FILTER_FLOAT16_SATURATE_TO_INF)) | ||
v = XMVectorClamp(v, g_HalfMin, g_HalfMax); | ||
|
||
if (!(flags & TEX_FILTER_FLOAT16_KEEP_NANS)) | ||
v = XMVectorSelect(v, zero, XMVectorIsNaN(v)); | ||
|
||
*ptr = v; | ||
} | ||
} | ||
} | ||
|
||
|
||
|
@@ -4441,6 +4458,12 @@ namespace | |
return false; | ||
} | ||
|
||
if (filter & (TEX_FILTER_FLOAT16_SATURATE_TO_INF | TEX_FILTER_FLOAT16_KEEP_NANS)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Before I can sign off here, I really need to see what WIC does. Does it preserve specials or always clamp them? |
||
{ | ||
// Float16 specials preservation not supported by WIC code paths | ||
return false; | ||
} | ||
|
||
// Check for special cases | ||
#if (defined(_XBOX_ONE) && defined(_TITLE)) || defined(_GAMING_XBOX) | ||
if (sformat == DXGI_FORMAT_R16G16B16A16_FLOAT | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little nervous about removing the clamp behavior here, but it's probably the best choice if we can do it in a way that doesn't regress behavior.
The main places where the clamping is now 'missing' is:
I think the
ConvertFromR32G32B32A32
helpers needs an explicit clamp added for the float16 cases. This is the primarily used for WIC interactions where WIC doesn't support the format.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BC6H signed can have -INF, even though it's not recommended.