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.

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

Fix ktx cubemap size calculation #64

Merged
merged 1 commit into from
Oct 26, 2021

Conversation

goodartistscopy
Copy link
Contributor

Change was introduced by commit #63 (b8a7a90)

Change was introduced by commit bkaradzic#63 (b8a7a90)
@goodartistscopy
Copy link
Contributor Author

goodartistscopy commented Oct 26, 2021

The attached file is a zipped ktx file (128x128 cubemap with 8 mip levels) that texturev refuses to load on current master with this assert:

image.cpp(5054): BX ASSERT KTX: Image size mismatch 131072 (expected 786432).

autoshop_01_ggx.zip

@bkaradzic bkaradzic merged commit 9e4d2b1 into bkaradzic:master Oct 26, 2021
@bkaradzic
Copy link
Owner

bkaradzic commented Oct 26, 2021

@attilaz This changes line previously added by you, and you might want to check logic.

@attilaz
Copy link
Contributor

attilaz commented Oct 26, 2021

autoshop_01_ggx has imageSize = faceSize * 6.
But AFAICT according to docs it should be only faceSize.
See similar issue and solutions here:
BinomialLLC/basis_universal#40 (comment)

The texture I had the problem with was exported with PVRTexTool.

@goodartistscopy What tool did You use for creating this ktx?

@attilaz
Copy link
Contributor

attilaz commented Oct 27, 2021

I have checked autoshop_01_ggx with ktxinfo from here: https://github.com/KhronosGroup/KTX-Software/releases/tag/v4.0.0

ktxinfo autoshop_01_ggx.ktx
Header

identifier: «KTX 11»\r\n\x1A\n
endianness: 0x4030201
glType: 0
glTypeSize: 1
glFormat: 0
glInternalformat: 0x881a
glBaseInternalformat: 0x1908
pixelWidth: 128
pixelHeight: 128
pixelDepth: 1
numberOfArrayElements: 1
numberOfFaces: 6
numberOfMipLevels: 8
bytesOfKeyValueData: 0
The KTX 1 file pHeader is invalid:
it describes a 3D array that is unsupported

@bkaradzic
Copy link
Owner

pixelDepth: 1
numberOfArrayElements: 1
numberOfFaces: 6
...
The KTX 1 file pHeader is invalid:
it describes a 3D array that is unsupported

What numbers should be here to be correct cubemap?

@attilaz
Copy link
Contributor

attilaz commented Oct 27, 2021

https://github.com/KhronosGroup/KTX-Software/blob/4e0a2cea60431ef757d7e7d93b245fc074c60e19/lib/checkheader.c#L115

From https://www.khronos.org/registry/KTX/specs/1.0/ktxspec_v1.html

pixelWidth, pixelHeight, pixelDepth
...
For 1D textures pixelHeight and pixelDepth must be 0. For 2D and cube textures pixelDepth must be 0.

numberOfArrayElements

numberOfArrayElements specifies the number of array elements. If the texture is not an array texture, numberOfArrayElements must equal 0.

@goodartistscopy
Copy link
Contributor Author

@attilaz I used the bimg API to write this texture (bimg::imageWriteKtx() taking an ImageContainer).
You're right that I might do something incorrect by not setting numLayers and depth to 0.

@goodartistscopy
Copy link
Contributor Author

goodartistscopy commented Oct 28, 2021

So IIUC, this test:

size = mipSize * ((_imageContainer.m_numLayers<=1 && _imageContainer.m_cubeMap) ? 1 : numSides);

aims to implement this part of KTX spec:

For most textures imageSize is the number of bytes of pixel data in the current LOD level. This includes all array layers, all z slices, all faces, all rows (or rows of blocks) and all pixels (or blocks) in each row for the mipmap level. It does not include any bytes in mipPadding.

The exception is non-array cubemap textures (any texture where numberOfFaces is 6 and numberOfArrayElements is 0). For these textures imageSize is the number of bytes in each face of the texture for the current LOD level, not including bytes in cubePadding or mipPadding.

Maybe it should be

numSides = max(_imageContainer.m_numLayers,1) * (_imageContainer.m_cubeMap ? 6 : 1);
...
size = mipSize * ((_imageContainer.m_numLayers == 0 && _imageContainer.m_cubeMap) ? 1 : numSides);

?

@attilaz
Copy link
Contributor

attilaz commented Oct 28, 2021

Reading ktx numLayers

_imageContainer.m_numLayers = uint16_t(bx::max<uint32_t>(numberOfArrayElements, 1) );

So I think _imageContainer.m_numLayers can't be zero. But yes, exception should only happen when numLayers was 0 in the file.

@goodartistscopy
Copy link
Contributor Author

Yes I think this is why I have m_numLayers = 1 in my code, because I suspected that saturation to 1 may not be done consistently throughout bimg. I think m_numLayers = 0 should be used to signal non arrayed textures. Same for depth = 0 probably.

@goossens
Copy link

goossens commented Nov 3, 2021

This still doesn't work for KTX files generated with toktx which show the following ktxinfo:

Header

identifier: «KTX 11»\r\n\x1A\n
endianness: 0x4030201
glType: 0x1401
glTypeSize: 1
glFormat: 0x1907
glInternalformat: 0x8c41
glBaseInternalformat: 0x1907
pixelWidth: 1024
pixelHeight: 1024
pixelDepth: 0
numberOfArrayElements: 0
numberOfFaces: 6
numberOfMipLevels: 1
bytesOfKeyValueData: 28

Key/Value Data

KTXorientation: S=r,T=d

Total data size = 18874368

I did a ktxinfo on all the KTX files in bgfx/examples/runtime/textures and they are all invalid with the message:

The KTX 1 file pHeader is invalid:
it describes a 3D array that is unsupported

I assume these files were created with BIMG and the issue seems to be:

pixelDepth: 1
numberOfArrayElements: 1

I checked some of the BGFX examples that use these invalid KTX files and they are working implying the current BIMG logic only works with invalid files.

If I comment out:

BX_ASSERT(size == imageSize, "KTX: Image size mismatch %d (expected %d).", size, imageSize);

or change:

const uint32_t size = mipSize * numSides;

back to the previous version of src/image.cpp, my KTX files (generated with toktx) work just fine.

Recommend undoing pull request #64 until this is sorted out. BTW: is there a way to load cubemaps in BGFX from 6 separate images instead of having to create these KTX files?

@goodartistscopy
Copy link
Contributor Author

goodartistscopy commented Nov 3, 2021

Recommend undoing pull request #64 until this is sorted out.

Agreed. It would be better to have bimg only use numLayers > 0 for layered textures and depth > 0 for 3D textures.

BTW: is there a way to load cubemaps in BGFX from 6 separate images instead of having to create these KTX files?

bgfx::updateTextureCube() takes data face by face.

@bkaradzic
Copy link
Owner

@goossens Where did you get this ktxinfo tool?

@goodartistscopy
Copy link
Contributor Author

@bkaradzic its one of the tool from Khronos KTX SDK https://github.com/KhronosGroup/KTX-Software

@goossens
Copy link

goossens commented Nov 4, 2021

@bkaradzic The Khronos KTX SDK also includes the "toktx" tool that I used to create my cubemaps.

@goodartistscopy Thanks for the hint on "updateTextureCube", it works like a charm even though it appears slow. I need to figure out why.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants