-
Notifications
You must be signed in to change notification settings - Fork 239
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
Rename function to transferFunction #482
Rename function to transferFunction #482
Conversation
include/ktx.h
Outdated
@@ -1198,7 +1198,13 @@ typedef struct ktxAstcParams { | |||
i.e. 6x6, 8x8, 6x5 etc*/ | |||
|
|||
ktx_uint32_t transferFunction; | |||
/*!< Input/output transfer function for astcenc can be {linear/srgb}*/ | |||
/*!< Input/output transfer function for astcenc that can be {linear/srgb} | |||
ASTC supports non-linear sRGB color space conversion at both compression and decompression time. |
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.
What conversion is actually done? I thought the data was encoded as sRGB with perhaps changes to the way error is computed during the encoding process.
Actually do we even need this field? I haven't exposed the equivalent for BasisU. The underlying encoder setting m_perceptual
is always set from the transferFunction field of the texture's DFD. I can't see anything useful and correct that can be done by allowing override. BasisU's m_perceptual
causes the error metrics to be computed in YCbCr space rather than RGB.
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 have submitted a fix to remove the conversion
bit. AFAIK it doesn't really do any conversion per se. What this means is that if your image is in sRGB color space you need to use the transferFunction = srgb
for the compression to be correct. It will create an image whose type is something like COMPRESSED_SRGB8_ALPHA8_ASTC_4x4_KHR
. If the image is linear you need to provide transferFunction = linear
and the image created will have a format of COMPRESSED_RGB8_ALPHA8_ASTC_4x4_KHR
. If a user has sRGB input image and want linear output, they first have to convert to Linear manually then pass it to the compressor with transferFunction = linear
and same for Linear to sRGB, i.e. the encoder won't do conversion for them.
I just needed this flag in here so I can carry our what the transfer function in the image was to the encoder. Is there a better way to get this in the astc_encoder.cpp
scope without this flag?
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.
For BasisU I don't expose the equivalent in ktxBasisParams
. Instead in ktxCompressBasisEx
I check whether the texture is sRGB or not and set the underlying basisu encoder's m_perceptual
parameter to true if it is. See here.
Presumably the ASTC encoder has an equivalent parameter, something you must be setting from the value of transferFunction
.
I think I just found a bug. I probably should be setting m_perceptual for uastc encoding as well.
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 have used something similar now in ca5309f I think that works.
I was actually already using the DFD way of getting this in astcEncoderAction
if the image provided option failed. Now cleaned that up.
Thanks! |
No description provided.