Please consider making ktxStream public

[UberLambda]

Hi,

This pull request moves lib/stream.h's contents to ktx.h, and it makes all the previously-private types and functions therein public.
It also adds a custom stream type, with a user pointer and a size_t in its data.

Motivation

I am currently creating a Rust wrapper for this library, and I wanted to read/write KTXs from Rust streams - anything dyn (Read + Write + Seek), like std::fs::File. So I had to:

1. Make ktxStream public.
2. Create C-visible Rust functions for a custom ktxStream (read, write, getpos, setpos...).
3. Construct the ktxStream from Rust, wrapping a heap-allocated trait object.

Custom ktxStreams would be useful in many other scenarios, though. Off the top of my head:

• Reading/writing KTXs from C++ std::iostreams
• Reading/writing KTXs from PhysicsFS' not-quite-FILE * streams
• Reading/writing KTXs from tinygltf filesystem callbacks

Notes

• The custom struct contains both a void * and size_t. This would allow for direct storage of Rust's "fat pointers" (a pointer to a vtable + a size).
  My current workaround is to add another layer of indirection (ktxMem * reinterpret-casted as a pointer to a Rust struct -> heap-allocated fat pointer -> heap-allocated trait object). Having the size_t in the data union would remove the need for heap-allocating the pointer.

Thanks!

[CLAassistant]

All committers have signed the CLA.

[MarkCallow]

Happy to hear you are creating a Rust wrapper. Please add a test (external to the library) that uses the stream functions. This is to ensure that linking will be successful on Windows, i.e. the all the necessary functions are properly exported. Adding the Rust wrapper to the interface directory and making sure it is built as part of CI will be sufficient. There are unit tests for the memory stream. Augmenting those with tests for other stream types would be great.

[MarkCallow]

Rename this custom_ptr to indicate it is for storing a pointer and not for data. Also change data to address.

Is this sufficient for the other uses you mentioned, e.g. a c++ stream wrapper? If someone needs to add a new field to this union for their use case, then their applications will require a new version of libktx. I'm not sure there is any solution that would let people add a new stream type that does not fit into this declaration but will let apps using it run on an older version of the library.

[UberLambda]

Done! I also added another allocatorAddress pointer to it. Hopefully two pointers + a size_t are enough for all usecases.

I suppose that, in the worst case, one would need to allocate on the heap? Without touching the struct definition.

[UberLambda]

Happy to hear you are creating a Rust wrapper. Please add a test (external to the library) that uses the stream functions. This is to ensure that linking will be successful on Windows, i.e. the all the necessary functions are properly exported. Adding the Rust wrapper to the interface directory and making sure it is built as part of CI will be sufficient. There are unit tests for the memory stream. Augmenting those with tests for other stream types would be great.

Adding the wrapper to interface would bring many GBs of dependencies with it (Rust nightly + Cargo + misc. libraries, including a binding generator step), so it might not be optimal. Plus, I currently do not use all of the functions made public here in the wrapper...

I could write some C++ integration tests instead, maybe based on tests/transcodetests/? Something simple/self-contained, just for checking if the exported Create*/Write* funcs (& the stream interface) are indeed working.

[UberLambda]

Hi, I added some streamtests/ that link to libktx dynamically and try to use all now-exported ktxStream symbols (wrapping a C++ streambuf).

[MarkCallow]

I could write some C++ integration tests instead, maybe based on tests/transcodetests/? Something simple/self-contained, just for checking if the exported Create*/Write* funcs (& the stream interface) are indeed working.

Sounds great. Reviewing the tests you've added will take more time that I have at this moment. Hopefully later today.

Sorry about the workflow approval thing. This is new and seems related to GitHub Actions which we've just added for the Android build. I thought the approval I gave yesterday would stick for further changes on this PR. Apparently not.

[UberLambda]

Sorry about the workflow approval thing. This is new and seems related to GitHub Actions which we've just added for the Android build. I thought the approval I gave yesterday would stick for further changes on this PR. Apparently not.

No worries, I'm fixing unforeseen cross-platform bugs in the meantime 👍

[MarkCallow]

Please fix the travis build.

When the Rust wrapper is released, please add a pointer to it to the wiki.

[UberLambda]

Hi, I am not exactly sure of why it is failing? The logs say the repo is not REUSE 3.0 compliant, but all files have a license and the same build does pass on Mac Os.

EDIT: reuse lint is failing even for upstream/master on my end - it lists two unused licenses, GPL-2.0-or-later and LicenseRef-HI-Trademark.

EDIT²: deleted LICENSES/GPL-2.0-or-later.txt and renamed a entry in .reuse/dep5, lint works now

[MarkCallow]

I needed to fix the reuse related issues and have done so in commit aa3f6f1 on master. Please rebase this and fix the conflict. Note that there is a small difference between your reuse/.dep5 and the fix now in master. Make sure you resolve the conflict to the master version.

[MarkCallow]

Thank you for bringing the need for this to my attention and for your incredibly thorough PR. It is impressive work.

[UberLambda]

Thank you for bringing the need for this to my attention and for your incredibly thorough PR. It is impressive work.

I just wrapped it with some Rust glue, but thank you :)
I really hope that KTX2 will become the de-facto standard for texture formats, like GLTF2 is becoming for models.