KhronosGroup / KTX-Software

KTX (Khronos Texture) Library and Tools
Other
875 stars 230 forks source link

ktxTexture_GetImageSize doesn't handle 3D textures #792

Closed AlexRouSg closed 11 months ago

AlexRouSg commented 12 months ago

I am trying to create a 3D ktxTexture2 from image data in memory however it fails at ktxTexture_SetImageFromMemory with KTX_INVALID_OPERATION.

Debugging the code it appears as if ktxTexture_GetImageSize doesn't handle 3D texture sizes at all which then triggers this failure condition in ktxTexture2_setImageFromStream

    if (srcSize != imageByteLength)
        return KTX_INVALID_OPERATION;

This is on the main branch just pulled at the time of this issue creation.

Unless I am meant to set the individual 2D slices of the 3D texture then the documentation is not clear that is what I have to do.

MarkCallow commented 12 months ago

The documentation for `ktxTexture[12]_GetImageSize says

 * For arrays, this is the size of a layer, for cubemaps, the size of a face
 * and for 3D textures, the size of a depth slice.

The documentation for the faceSlice parameter of ktxTexture2_setImageFromStream says

 * @param[in] faceSlice cube map face or depth slice of the image to set.

which to me clearly indicates you have to upload a slice at a time. Happy to make clarifications, if you'd like to propose something concrete.

I'd also be happy to better support creation of 3d textures. Are you working a file format that supports 3d image data? Please propose an API that would helpful to you. Probably will need new SetImage functions that take all slices at once and would not be usable with any other type of texture.

Are you generating mip levels for your 3d textures? The generator currently used by the KTX tools only supports 2d mip level generation. An extra dimension of filtering is needed for 3d.

Note that a ktxTexture's data pointer is public. You can do your own calculations and then memcpy data to the offset you calculate. ktxTexture2_GetImageOffset with afaceSliceparam of 0 will return the offset of the data for the givenlevel`. The size of a whole 3d image is the same as the mip level size: level image size * number of slices. There is an internal function to calculate this but it is not exposed in the library.

AlexRouSg commented 12 months ago

which to me clearly indicates you have to upload a slice at a time. Happy to make clarifications, if you'd like to propose something concrete.

That's kinda surprising to me that you have to do it that way, cause I've edited the ktxTexture_GetImageSize function to include depth and it works and creates a valid ktx2 file when given an uncompressed R8G8B8A8 image. But I don't know what is the correct size calculation to handle every format.

On a related note, ktxTexture2_CompressBasisEx with UASTC breaks on 3D images. However the prebuilt ktxsc from the latest stable version works fine. Getting 0xc0000374 STATUS_HEAP_CORRUPTION

Are you generating mip levels for your 3d textures?

Not currently but maybe, still investigating what to do atm.

MarkCallow commented 12 months ago

That's kinda surprising to me that you have to do it that way,

It works as documented...

cause I've edited the ktxTexture_GetImageSize function to include depth and it works

Do not do this. It will break the level size and level offset calculations as the 2D image size (in bytes) will now be multiplied by the square of the number of depth slices. Since you only have a base mip level at present you won't notice the breakage but broken it is.

The proper fix is to add new ktxTexture_Set3dImageFrom* functions which use the level size instead of 2d image size as a sanity check. A modified ktxTexture2_setImageFromStream` should underlie both old and new functions. A flag parameter can be used to tell it which sanity check to use.

MarkCallow commented 12 months ago

The proper fix is to add new ktxTexture_Set3dImageFrom* functions

On reflection, this is overkill. A simple fix to the sanity check will work. Compare the provided size with the result of ktxTexture_GetImageSize() for non-3d textures and for 3d textures when the faceSlice parameter != 0. For 3d textures with faceSlice == 0, allow size equal to either ktxTexture_GetImagesize() or ktxTexture_calcLevelSize(). The level size equals the sum of the 2d slice sizes. With that fix you'll be able to upload your 3d texture.

Do you have a file format you are working with?

AlexRouSg commented 12 months ago

A simple fix to the sanity check will work

yes that does look like better UX, maybe instead of 0, use -1 for less chance of user error?

Do you have a file format you are working with?

I have a private file format I am working on but it's in too early a state to know what more I need from libktx

MarkCallow commented 12 months ago

A simple fix to the sanity check will work

yes that does look like better UX, maybe instead of 0, use -1 for less chance of user error?

You mean a faceSlice value of -1 to modify the sanity check, right? Yes that sounds good. If you create a PR to fix ktxTexture2_setImageFromStream I'll be happy to accept it.

Do you have a file format you are working with?

I have a private file format I am working on but it's in too early a state to know what more I need from libktx

When you do know, please let me know. I'll be happy to provide better support for 3d textures.