gltfpack fails to write output data to tmp folder when compiling latest
Honeybunch opened this issue · 6 comments
I've been working on adding gltfpack as a tool in vcpkg. I've gotten v0.18 up there already but without basisu support for starters. Going back to add basisu support I've run into a little snag; I get a compilation error in meshopt's basisenc.cpp file:
basisenc.cpp(164): error C2661: 'basisu::job_pool::add_job': no overloaded function takes 2 arguments
The code at commit e62c41e passes nullptr as a second parameter to add_job. But I can't find any commit in https://github.com/zeux/basis_universal where add_job accepts two parameters. Looks like v0.18 of gltfpack was released Jul 31, 2022. The closest commit in zeux/basis_universal looks to be 9009cae5407ef54f5030c1d0fabb1c2919c418bb from Jul 9, 2022. Is there some other patch that was applied to get this to compile?
I have tried to work around this by just using the latest commits from both repos, which does compile, but I've run into a runtime error. When trying to compress a couple different glbs that I have with -tc I get the following outputs:
ERROR: Failed writing output data to file "<appdata>\Local\Temp\gltfpack-a35556.ktx2"
Warning: unable to encode image 0 (?), skipping (error encoding image)
I don't get these issues when using the v0.18 release of gltfpack downloaded off github. But I've tried compiling the latest build on Windows with LLVM and VS 2022 on Windows 11 with no success.
So I guess this issue is two part:
- Is there something I'm missing to compile the same v0.18 version of gltfpack?
- Am I somehow messing up compilation of latest that's causing this runtime failure?
For reference these have been my cmake configurations.
Clang 15.0.3 is set as my default compiler but the VS config uses MSVC.
cmake -S meshoptimizer\ -B build_llvm -G"Ninja Multi-Config" -DMESHOPT_BUILD_GLTFPACK=ON -DMESHOPT_BASISU_PATH=C:/Users/Arsine/Git/gltfpack_test/basis_universal
cmake -S meshoptimizer\ -B build -G "Visual Studio 17 2022" -DMESHOPT_BUILD_GLTFPACK=ON -DMESHOPT_BASISU_PATH=C:/Users/Arsine/Git/gltfpack_test/basis_universal
I dug in with a debugger and it seems like this is an error that comes from basisu_comp.cpp when write_vec_to_file fails. I've seen something kind of like this when contributing to KTX-Software where there is a bug in the msvcrt on large file writes KhronosGroup/KTX-Software#574 but I haven't had time yet to test explicitly linking against the ucrt instead.
Am I just doing something wrong with the build system here? Should I not be using your fork of basisu to compile anymore?
My test glbs are too big to upload here but one of them is just a glb version of Sponza. I can upload it somewhere else and link it if that's helpful.
Going back to add basisu support I've run into a little snag; I get a compilation error in meshopt's basisenc.cpp file
For v0.18, you need to use branch gltfpack of this fork: https://github.com/zeux/basis_universal
The latest master doesn't need that, although the gltfpack branch of the fork above is still recommended for performance (I intend to eventually contribute the changes from the branch above to upstream but unfortunately the rate that upstream takes patches is pretty slow, so I'm waiting for some earlier PR to get merged). But with latest version you should be able to successfully compile on non-forked basis_universal as well.
But I've tried compiling the latest build on Windows with LLVM and VS 2022 on Windows 11 with no success.
Just to double check, you've tried both and both exhibit the same error of writing to a temporary file?
I dug in with a debugger and it seems like this is an error that comes from basisu_comp.cpp when write_vec_to_file fails.
This, however, shouldn't happen regardless of the fork you use I think. I'll try to reproduce your build setup a little later today and see if I can get the same bug.
Can you check if you're getting the issue above on any input glb? Or just on very large models with lots of textures?
Oh... Does your Windows user name contain non-ASCII characters? :)
Oh the gltfpack branch might be what I was missing.
Just to double check, you've tried both and both exhibit the same error of writing to a temporary file?
Yep
Oh... Does your Windows user name contain non-ASCII characters?
Nope
Can you check if you're getting the issue above on any input glb? Or just on very large models with lots of textures?
I have a dozen or so glbs that I run through. Most of them do have textures and it does seem to be the smaller ones that succeed while the larger ones exhibit this failure. In the debugger I noticed that the number of write failures is inconsistent; I saw that Sponza would have between 1-4 failures
Hmm just tested this again making sure to be on the gltfbranch of basisu and I still see the same issues
Oh excuse me, no I'm sorry. I checked out the v0.18 tag of meshoptimizer and that seems to build and run correctly
I still don't have access to a Windows system yet to try to fully reproduce this but looking at the code, I think the problem was indeed introduced after 0.18 and has to do with the style of file handling.
In 0.18, each thread that encoded the texture created a temporary file and then wrote the image to that file, read the file and then deleted it. The file name is created using _mktemp on Windows which doesn't create the file itself (unlike Unix platforms where we use mkstemps that creates the file, guaranteeing uniqueness), but it does use thread id as part of the file name, so we more or less get a file name per thread.
After 0.18, I've switched to an official public API for parallel compression, to make sure that gltfpack can be built off of any fork of basisu (my fork still provides optimal performance, but long term the changes will hopefully be integrated upstream). As part of that, all input and output temp files are pre-created ahead of time. For input files, this is not a problem as they are created and written to one after another, and _mktemp checks if a file exists before returning the name.
For output files, however, they are expected to be written by basisu API... however, repeated invocations of _mktemp return the same file name on Windows, unless you actually create the files in between. This means that all images are using the same output file path to get encoded, which of course won't work very well in general, and will cause races between threads to fail to save the file data if that happens at the same time.
The easiest way to patch this would be to create a dummy file after establishing the file name, but maybe there's a cleaner way to do all this without using temp file APIs - eg maybe we should switch to generating the file name based on process ID, and remove TempFile class altogether.