bkaradzic / bimg

Image library.
BSD 2-Clause "Simplified" License
327 stars 268 forks source link

Fix ktx cubemap size calculation #64

Closed goodartistscopy closed 2 years ago

goodartistscopy commented 2 years ago

Change was introduced by commit #63 (b8a7a90)

goodartistscopy commented 2 years ago

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 commented 2 years ago

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

attilaz commented 2 years ago

autoshop_01_ggx has imageSize = faceSize * 6. But AFAICT according to docs it should be only faceSize. See similar issue and solutions here: https://github.com/BinomialLLC/basis_universal/issues/40#issuecomment-504042169

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

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

attilaz commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

Reading ktx numLayers https://github.com/bkaradzic/bimg/blob/9e4d2b167ffb4ecf4dba625dee59f6be7cf2587a/src/image.cpp#L4097

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

goodartistscopy commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@goossens Where did you get this ktxinfo tool?

goodartistscopy commented 2 years ago

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

goossens commented 2 years ago

@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.