KhronosGroup / glTF

glTF – Runtime 3D Asset Delivery
Other
7.12k stars 1.13k forks source link

KHR_texture_basisu samples #1750

Open donmccurdy opened 4 years ago

donmccurdy commented 4 years ago

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

zeux commented 4 years ago

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

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 https://github.com/KhronosGroup/glTF/pull/1612, we'll update KHR_texture_basisu to explicitly allow KTX2 references without a dependency on another extension.

zeux commented 4 years ago

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

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

Avocado.zip

MarkCallow commented 4 years ago

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

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

MarkCallow commented 4 years ago

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

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

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

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

@bghgary See https://github.com/KhronosGroup/KTX-Specification/issues/18#issuecomment-439462702

bghgary commented 4 years ago

Ahh, thanks.

zeux commented 4 years ago

Here's an Avocado model built with new padding rules as per https://github.com/KhronosGroup/KTX-Specification/pull/117

Avocado-newpadding.zip

MarkCallow commented 4 years ago

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

donmccurdy commented 4 years ago

@MarkCallow @zeux I'm getting two odd results with the Avocado base color .ktx2 file, while working on https://github.com/mrdoob/three.js/pull/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 commented 4 years ago

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

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

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

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

https://github.com/KhronosGroup/KTX-Software/pull/175

bghgary commented 4 years ago

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

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

Correct.

donmccurdy commented 4 years ago

Thanks for tracking this down so quickly! :)

photon82 commented 4 years ago

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

Similar situation is for Avocado_baseColor.ktx2

donmccurdy commented 4 years ago

@photon82 is this helpful? (from https://github.com/mrdoob/three.js/pull/18490)

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

photon82 commented 4 years ago

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

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

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

donmccurdy commented 4 years ago

Two large samples:

prideout commented 2 years ago

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

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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: