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

KHR_texture_basisu samples #1750

Open
donmccurdy opened this issue Jan 30, 2020 · 35 comments
Open

KHR_texture_basisu samples #1750

donmccurdy opened this issue Jan 30, 2020 · 35 comments

Comments

@donmccurdy
Copy link
Contributor

Provide samples with Basis/KTX2 for color textures. Note mipmaps will have to be done manually for now.

@zeux
Copy link
Contributor

zeux commented Jan 30, 2020

Provide samples with Basis/KTX2 for color textures. Note mipmaps will have to be done manually for now.

Can you expand on what you mean by "manually"? I can generate samples with gltfpack because it knows how to do this, although it's not able to generate the extra JSON metadata.

@donmccurdy
Copy link
Contributor Author

My understanding is that the ktx2 branch of KTX-Software doesn't yet generate mipmaps, you'd need to create them in advance with imagemagick or similar. There is also some final work to be done on the mip padding section of the KTX2 spec... I'm working on support for ktx2 in three.js now.

If you're able to generate samples with .ktx2 files that'd be great! Note that although gltfpack is using KHR_image_ktx2, that extension will not be necessary. Per comment at the end of #1612, we'll update KHR_texture_basisu to explicitly allow KTX2 references without a dependency on another extension.

@zeux
Copy link
Contributor

zeux commented Jan 30, 2020

My understanding is that the ktx2 branch of KTX-Software doesn't yet generate mipmaps

Ah - I see. gltfpack doesn't use toktx/libktx because honestly these tools didn't seem ready. It can generate files with full mip chain using basisu and re-encode them using a ktx2 container.

Thanks for the heads up, I'm going to update my code to not use KHR_image_ktx2.

@zeux
Copy link
Contributor

zeux commented Jan 30, 2020

Here's an example model, converted using -tc command line flag (needs basisu executable in path):

Avocado.zip

@MarkCallow
Copy link
Collaborator

My understanding is that the ktx2 branch of KTX-Software doesn't yet generate mipmaps

It is toktx that does not generate mipmaps yet. libktx is fully capable of handling files containing mipmap pyramids.

gltfpack doesn't use toktx/libktx because honestly these tools didn't seem ready

@zeus, in what way did they not seem ready?

@zeux
Copy link
Contributor

zeux commented Jan 30, 2020

@MarkCallow Two biggest issues were no mip-map generation support and no JPEG conversion support.

@MarkCallow
Copy link
Collaborator

mipmap generation is next on my list. I'm not planning to add jpeg support as you will be starting with an already compressed image. But perhaps in view of all the jpeg images produced by digital cameras I should revisit that.

@donmccurdy donmccurdy changed the title Provide samplers for KHR_texture_basisu Provide samples for KHR_texture_basisu Jan 31, 2020
@donmccurdy
Copy link
Contributor Author

donmccurdy commented Jan 31, 2020

@zeux thanks! I now have mips loading in threejs using the base color .ktx2 in your model. Haven't started on support in GLTFLoader yet.

@zeux
Copy link
Contributor

zeux commented Jan 31, 2020

But perhaps in view of all the jpeg images produced by digital cameras I should revisit that.

Since glTF supports PNG and JPEG, I would expect any tool that transcodes images in glTF context to support both. To be fair, basisu doesn’t support it either but there’s an open PR for that.

@bghgary
Copy link
Contributor

bghgary commented Jan 31, 2020

Thanks @zeux for the Avocado model!

Can we discuss the file extension for KTX2? I have been using .ktx and this model is using .ktx2.

@lexaknyazev
Copy link
Member

@bghgary
Copy link
Contributor

bghgary commented Jan 31, 2020

Ahh, thanks.

@zeux
Copy link
Contributor

zeux commented Feb 8, 2020

Here's an Avocado model built with new padding rules as per KhronosGroup/KTX-Specification#117

Avocado-newpadding.zip

@MarkCallow
Copy link
Collaborator

@zeux you'll be glad to hear that the files in Avocado-newpadding all pass the updated validator. KhronosGroup/KTX-Specification#117 and the matching KhronosGroup/KTX-Software#170 will be merged soon.

@donmccurdy
Copy link
Contributor Author

donmccurdy commented Feb 23, 2020

@MarkCallow @zeux I'm getting two odd results with the Avocado base color .ktx2 file, while working on mrdoob/three.js#18490:

  1. Transcoding each mip level overwrites part of the array returned by the previous level. If I transcode the 1st mip alone, it's fine. If I continue transcoding more levels, each previous level changes. Mips 1-12, from left to right, when transcoded in that order:

Screen Shot 2020-02-22 at 9 57 26 PM

  1. By making a copy of the data between each transcodeImage() call I can work around that, but there's still a black pixel (or a few?) in the upper left of the original image, and it expands with each mip level.

Screen Shot 2020-02-22 at 9 57 51 PM

The first issue seems like a transcoder bug, unless there's some technical reason overwriting the old response array is necessary? The second, I'm not sure... has anyone else got this sample rendering correctly?

@MarkCallow
Copy link
Collaborator

MarkCallow commented Feb 23, 2020

I am transcoding a file with 12 levels in my libktx.js test and a 12-level cubemap (hence 72 images) in my native tests. Both work fine so I don't think there is any bug in the underlying native transcoder, which is used by both libktx.js and msc_basis_transcoder.js.

msc_basis_transcoder.js has the following:

            std::vector<uint8_t> dst;
            dst.resize(getTranscodedImageByteLength(static_cast<transcoder_texture_format>(cTargetFormat),
                                                    width, height));

            KTX_error_code error;
            error = ktxBasisImageTranscoder::transcode_image(
                           ...);
            val ret = val::object();
            ret.set("error", static_cast<uint32_t>(error));
            if (error == KTX_SUCCESS) {
                // FIXME: Who deletes dst and how?
                ret.set("transcodedImage", typed_memory_view(dst.size(), dst.data()));
            }
            return std::move(ret);

Since dst is a stack variable, I suppose the memory gets freed when the function exits so it is quite likely the same piece of memory is allocated each time thereby overwriting previous levels. Unfortunately I not very familiar with embind and so far, have failed to find a complete example of returning data to JS. I welcome advice from anyone familiar with embind. I'll post a question to emscripten_discuss to see if I can get some help.

@MarkCallow
Copy link
Collaborator

Probably msc_basis_transcoder should do std::vector<uint8> dst = new std::vector<uint8>; and the JS side should call .delete() on the typed array buffer it receives back once it is finished with it. @donmccurdy, could you give that a try. I'm rather busy right now. It will be several days before I can try it.

@zeux
Copy link
Contributor

zeux commented Feb 23, 2020

Yeah this looks like a bug in transcoder. The Basis transcoder has the same code as Mark quoted:

std::vector<uint8_t> dst_data;

...

emscripten::val memory = emscripten::val::module_property("HEAP8")["buffer"];
emscripten::val memoryView = emscripten::val::global("Uint8Array").new_(memory, reinterpret_cast<uintptr_t>(dst_data.data()), dst_data.size());

Repeat calls would definitely reuse the allocated memory causing the old mip content to change, but even within a singe call there are no guarantees and it's possible that heap data gets corrupted in the first pixel, causing the issue #2 as well. (it's also possible that my encoded data somehow "poisons" the first mip, I'll double check the code I was using to produce this image)

Something like this is probably the correct solution:

std::vector<uint8_t> dst_data;

...

emscripten::val memory = emscripten::val::module_property("HEAP8")["buffer"];
emscripten::val memoryView = emscripten::val::global("Uint8Array").new_(memory, reinterpret_cast<uintptr_t>(dst_data.data()), dst_data.size());

emscripten::val memoryCopy = emscripten::val::global("Uint8Array").new_(dst_data.size());
memoryCopy.call<void>("set", memoryView);

dst.call<void>("set", memoryCopy);

@zeux
Copy link
Contributor

zeux commented Feb 23, 2020

Yeah - both issues are one and the same.

I've fixed both in this branch by fixing the transcoder:
https://github.com/zeux/three.js/tree/feat-ktx2loader

You can grab the .js/.wasm from this branch, and I'll open a PR to KTX-Software shortly.

@zeux
Copy link
Contributor

zeux commented Feb 23, 2020

@bghgary
Copy link
Contributor

bghgary commented Feb 24, 2020

The Avocado-newpadding now works with libktx.js/wasm in the Babylon sandbox. I believe @zeux's fix has no effect on the libktx version.

@MarkCallow
Copy link
Collaborator

I believe @zeux's fix has no effect on the libktx version.

Correct.

@donmccurdy
Copy link
Contributor Author

Thanks for tracking this down so quickly! :)

@donmccurdy donmccurdy changed the title Provide samples for KHR_texture_basisu KHR_texture_basisu samples Feb 26, 2020
@photon82
Copy link

photon82 commented Mar 20, 2020

Hello, I'm trying to parse avocado_normal.ktx2 texture file by myself and parsing seems to be OK till sgd block, but with sgd looks something wrong. Could you guide me how to find the problem.
I have read following values for sgd:
sgdByteOffset = 472
sgdByteLength = 6979

globalFlags = 1
endpointCount = 1220
selectorCount = 2526
endpointByteLength = 1686
selectorByteLength = 3655
tablesByteLength = 0
extendedByteLength = 0

For this variables I read 28 bytes from sgd block

  • for endpointsData + for selectorsData it will be 28+1656+3655 = 5339, so for ImageDescs we have 6979-5339 = 1640 bytes
    But these 1640 bytes does not correspond to sizeof(ImageDesc) * imageCount...

Similar situation is for Avocado_baseColor.ktx2

@donmccurdy
Copy link
Contributor Author

donmccurdy commented Mar 20, 2020

@photon82
Copy link

@donmccurdy , thank you

https://github.com/mrdoob/three.js/blob/6d8d3fd86d5a84c85d3b207a2c56051ff3b0f6a2/examples/jsm/loaders/KTX2Loader.js#L288-L316

It looks very similar to my code except

  1. I'm using C++
  2. A bit different part for ImageDescs push following http://github.khronos.org/KTX-Specification/#_supercompression_global_data to take into account possible different number of faces (not just 1, but 6 as well). Actually, I'm confused with formula from KTX specs for calc of imageCount

int imageCount = max(levelCount, 1) * max(layerCount, 1) * faceCount * totalPixelDepth;

// where totalPixelDepth can be derived as
int totalPixelDepth = max(pixelDepth, 1);
for(int i = 1; i < levelCount; i++)
totalPixelDepth += max(pixelDepth >> i, 1);

because here for avocado we have imageCount = 12 * 1 * 1 * 12 = 144 instead of just 12. But anyway sgd total size does not match with sum of sizes of its components.

@MarkCallow
Copy link
Collaborator

Here are the values I am seeing, with ktxinfo in avocado_normal.ktx2 - the version compliant with the padding requirements that were changed in draft 18 of the KTX 2 spec. on 2020-02-08.

supercompressionGlobalData.byteOffset: 0x1d8
supercompressionGlobalData.byteLength: 6974

Global flags: 0x1
endpointCount: 1217
selectorCount: 2523
endpointsByteLength: 1686
selectorsByteLength: 3663
tablesByteLength: 1361
extendedByteLength: 0

tablesByteLength should most definitely not be 0. Given your sgd.byteLength is 6969 it looks like you may be working with a version of avocado_normal.ktx2 that predates the padding changes.

There was an error in the imageCount calculation, fixed in draft19 published on 2020-03-04. Sorry about that.

@photon82
Copy link

@MarkCallow Thank you, this data will be helpful. I'll doblecheck and compare

@donmccurdy
Copy link
Contributor Author

Two large samples:

@prideout
Copy link

This ticket is over two years old and StainedGlassLamp is still the only BasisU example in KhronosGroup/glTF-Sample-Models, which uses many extensions and is therefore not a very targeted test. Do we plan on adding a BasisU version of Avocado, FlightHelmet, and Sponza to that repo, or some subset thereof?

@atteneder
Copy link
Contributor

This ticket is over two years old and StainedGlassLamp is still the only BasisU example in KhronosGroup/glTF-Sample-Models, which uses many extensions and is therefore not a very targeted test. Do we plan on adding a BasisU version of Avocado, FlightHelmet, and Sponza to that repo, or some subset thereof?

I agree that there should be a targeted sample that uses no other extension besides BasisU. With the tools to create them freely available anyone can add them via PR.

Imho adding a BasisU version for all (or some more) assets is not a good idea though. Same could be requested for Draco or glTF-embed variants. It just adds a lot of redundancy (wrt functionality).

Unfortunately there's already an ambivalence in glTF-Sample-Models between models for functional feature tests and visually appealing demonstration objects.

@hecodeit
Copy link

Two large samples:

@donmccurdy

I download this Sponza file, and it's running 60fps on my old iPhone SE (1st generation) with Threejs.
The original jpg version crashed all the time.

Can I ask for the compression settings of this files, gltf-transform uastc?

@donmccurdy
Copy link
Contributor Author

@hecodeit I don't seem to have the full, original encoder settings used for these textures, sorry. They were all encoded with glTF-Transform, using the UASTC mode of toktx... for most use cases there are good default settings recommended in this article:

https://github.com/KhronosGroup/3D-Formats-Guidelines/blob/main/KTXArtistGuide.md#compressing-to-ktx-with-gltf-transform

@MarkCallow
Copy link
Collaborator

They must have been created a long time ago because the early beta version of toktx used did not write the compression settings into the .ktx2 file. It would be great if the samples could be regenerated with at least the Release 4.0 version of toktx so that KTXwriterScParams metadata will be written into the files.

@donmccurdy
Copy link
Contributor Author

Right, it was an older version of toktx. These are just examples thrown into a github thread, I'm not planning to keep them updated over time. If more examples are wanted (I suspect that is still true) let's add them to the glTF-Sample-Models repository. Related:

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

No branches or pull requests

9 participants