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

KTX2Loader: Offset overflows texture dimensions #25908

Closed
donmccurdy opened this issue Apr 23, 2023 · 9 comments · Fixed by #26095
Closed

KTX2Loader: Offset overflows texture dimensions #25908

donmccurdy opened this issue Apr 23, 2023 · 9 comments · Fixed by #26095
Milestone

Comments

@donmccurdy
Copy link
Collaborator

Description

When loading the attached glTF containing KTX2 textures (which pass validation), we see broken mipmaps in the rendering and an error in the console:

GL_INVALID_VALUE: Offset overflows texture dimensions.

The original texture has multiple-of-four dimensions, but is not using powers of two. I believe that should still be valid in WebGL2 contexts.

Reproduction steps

  1. In the webgl_loader_gltf_compressed example, swap the attached glTF for the original. May want to scale the model up by 100x.
  2. Observe errors in JS console and issues with render

Code

Archive.zip

Header from ktxinfo

Header

identifier: «KTX 20»\r\n\x1A\n
vkFormat: VK_FORMAT_UNDEFINED
typeSize: 1
pixelWidth: 1324
pixelHeight: 1236
pixelDepth: 0
layerCount: 0
faceCount: 1
levelCount: 11

Live example

n/a

Screenshots

Preview:

Screenshot 2023-04-22 at 11 08 04 PM

Mipmaps:

Screenshot 2023-04-22 at 11 09 42 PM

Version

dev

Device

Desktop

Browser

Chrome

OS

MacOS

@mrdoob mrdoob added this to the r153 milestone Apr 25, 2023
@SrinivasPrabhu794
Copy link

@donmccurdy Was able to reproduce this error on my system. Are you already working on this? If not, i can give it a try to fix this.

@donmccurdy
Copy link
Collaborator Author

I haven't had the chance to work on this, please feel free! Note that I'm unsure whether it is a bug in KTX2Loader, a bug in WebGLLoader, or a limitation of WebGL's mipmap size requirements.

@SrinivasPrabhu794
Copy link

SrinivasPrabhu794 commented May 8, 2023

Sure, I'll have a look. From my initial observation, it seems like a limitation of WebGL's mipmap size requirements but I'll have to re-confirm by first looking at the loaders. The issue is reproducible on Firefox as well.

Power of 2 sized compressed mipmapped textures work well (as in the original example).

@zeux
Copy link
Contributor

zeux commented May 17, 2023

This code in KTX2Loader.js:

const levelInfo = ktx2File.getImageLevelInfo( mip, layer, face );
mipWidth = levelInfo.origWidth < 4 ? levelInfo.origWidth : levelInfo.width;
mipHeight = levelInfo.origHeight < 4 ? levelInfo.origHeight : levelInfo.height;

should be:

const levelInfo = ktx2File.getImageLevelInfo( mip, layer, face );
mipWidth = levelInfo.origWidth;
mipHeight = levelInfo.origHeight;

levelInfo.width/height represent the size of the mip level rounded up to 4x4 block boundary; I believe glCompressedTexImage2D/glCompressedTexSubImage2D arguments should instead use the original dimensions that have not been rounded.

@donmccurdy
Copy link
Collaborator Author

@zeux hm, we'd previously used that code, and updated it to "fix" an issue displaying this same error message. I'm a bit confused about how the original dimensions persist in a compressed image... 😕

Related:

@zeux
Copy link
Contributor

zeux commented May 18, 2023

Well, 2118 isn't divisible by 4. Some APIs require top mip level size to have dimensions divisible by 4, so you won't be able to load a texture like this. The fix from that PR doesn't fully fix the problem - yes, you're going to be able to create the texture, but attempting to actually sample from it will use pixels that were never encoded, and the texture will be incorrectly sized.

If we want to be conservative we could use width/height just for mip 0 as a workaround, while keeping the issue above in mind (which can lead to visual artifacts if the image is tiled for example).

@zeux
Copy link
Contributor

zeux commented May 18, 2023

(the divisibility by 4 isn't always a problem but it sometimes is, it depends on the encoded format and the underlying API to a certain extent. It was discussed previously here KhronosGroup/KTX-Software#254)

@zeux
Copy link
Contributor

zeux commented May 18, 2023

If we want to be conservative we could use width/height just for mip 0 as a workaround, while keeping the issue above in mind (which can lead to visual artifact if the image is tiled for example).

Actually, let me amend this - we should only apply this workaround when the texture doesn't have a mip chain. If the texture has mip levels, and it's of size 102x102, the mip chain should have sizes 51x51, 25x25, 12x12, 6x6, 3x3, 1x1. If you resize it to 104x104, the mip chain that GL will allocate will have sizes 52x52, 26x26, 13x13, 6x6, 3x3, 1x1. The mip that goes from 12x12 to 13x13 will not have the correct encoded expanded blocks in the file - the file will have encoded it as precisely 12x12 as these match block dimensions, but the renderer will expect 13x13 which is 1 extra row/column of 4x4 blocks...

The only fully correct solution is to reject loading of textures with top mip level's width/height not being divisible by 4 if the extension being targeted by the format doesn't support arbitrary top level dimensions - see linked thread for some details on different extensions, I don't fully recall them OTOH. If we wanted to preserve the original fix, we should only apply it when there's only one mip level.

@donmccurdy
Copy link
Collaborator Author

Thank you! I suspect we should start logging a warning about non-multiple-of-four textures, as well. Opened a PR:

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

Successfully merging a pull request may close this issue.

4 participants