Inquiry about Python Bindings for astc-encoder

[K0lb3]

Hello,
Over the past few days, I've developed Python bindings for astc-encoder and would appreciate some feedback from the developers and maintainers.

About the Bindings

The bindings are built using Python's limited API, which ensures that the wheels built for one version are compatible with all subsequent versions. For x86 architectures, the binary distribution (bdist) wheels contain multiple precompiled encoders optimized for different SIMD levels. Python detects the highest SIMD level supported by the local CPU at runtime and loads the appropriate encoder.
No SIMD encoders are shipped for other architectures, including ARM64, with the exception of NEON for ARM64. Once I find a reliable way to detect SVE within Python, I'm working on adding support for it.

Questions

Would you be interested in adding these bindings to the astc-encoder repository?

I understand the potential maintenance overhead might make this unlikely, but I wanted to ask, considering that many tests in the repository are written in Python. These bindings could simplify some of the testing workflows.

How should I handle attribution for parts copied from astc-encoder?

This primarily concerns the docstrings, which I've copied almost verbatim from astc-encoder, with minor modifications or additions. Would it be sufficient to mention this in the README and main documentation, noting that certain parts were copied and slightly adapted from the astc-encoder?

Are there any changes you would recommend for the bindings?

While I'm primarily asking about any legal considerations around attribution, I would also appreciate any feedback or suggestions regarding the code.

Lastly, I express my gratitude for creating and maintaining this excellent library. I look forward to your feedback or suggestions.
Thank you in advance for your time and response!

[solidpixel]

Exciting. Yes, this is something we can accept upstream. Will give you a longer answer when I've had a chance to take a look.

[K0lb3]

Nice, that's great to hear.

I kept the bindings as close to the original C++ code as possible
and only modified some interfaces to make the usage more Pythonic.
e.g., the C++ *_init functions became __init__ in Python, and the compress and decompress functions became a method of the context.

I think that the following points are still subpar and would have to be improved before merging the bindings.

• The bindings currently assume that enums are the same size as int. This has to be handled correctly either via some preprocessor magic defining the correct Python type or the enum access has to happen via property/gets better functions.
• The docs lack examples and explanations for usage.
• The tests need to be more complex.
• No lint checks.

One of the things that I still want to implement is allowing the use of a PIL.Image.Image instead of ASTCImage as input for the compression, as that would be more ergonomic for the user.

[solidpixel]

How should I handle attribution for parts copied from astc-encoder?

In terms of maintaining this as a fork, it's entirely up to you. Attribution is "nice", so feel free to give us a mention, but the license doesn't require it. The only real requirement is keeping the Arm copyright in the Arm authored files and the license.

In terms of contributing upstream, you are contributing under Apache 2.0 so you/your employer need to be happy with contributing to the project under those terms. We don't require copyright assignment, so you can add your/your employer copyright to the files you contribute, or add it to any files you modify.

[solidpixel]

The bindings currently assume that enums are the same size as int.

I'd prefer to use true Python Enums for things that are Enums - it makes it clearer to users and plays better with API typing.

One of the things that I still want to implement is allowing the use of a PIL.Image.Image instead of ASTCImage as input for the compression, as that would be more ergonomic for the user.

Sounds good. I'm not sure how thorough PIL's support is for HDR formats - does it support HDR and EXR?

[K0lb3]

I'd prefer to use true Python Enums for things that are Enums—it makes it clearer to users and works better with API typing.

That's what I'm doing currently for the Python side, but the binding itself still has to interact directly with the C struct somehow.
This is currently implemented via the members list, which tells Python where which member is within the C struct and which type it has. This can be found here.

 {"profile", T_UINT, offsetof(ASTCConfigT, config.profile), 0, "the color profile"},

T_UINT tells Python to interpret the data at the offset as uint32. This also means that from the Python side, one could assign any value there, so it is better to handle this via a getsetter, which checks the input value.

Sounds good. I'm not sure how thorough PIL's support is for HDR formats - does it support HDR and EXR?

Oh, good point. Going by the Pillow Handbook #Modes, it doesn't seem to support HDR and EXR would have to be dumped down to uint8.
I guess this should be noted in the documentation for it, and the decoder should probably throw a warning if quality suffers due to Pillow's inability to handle the specified profile.

[K0lb3]

Attribution is "nice", so feel free to give us a mention,
Ah, ok, thank's a lot for the clarification. I will try to add some sort of footnote to clarify which part of the documentation was copied ad verbatim, slightly modified, or written by me.

In terms of contributing upstream, you are contributing under Apache 2.0 so you/your employer need to be happy with contributing to the project under those terms.

That's totally fine by me, and as I'm currently unemployed to finish my studies, I also don't have to ask an employer^^

The only real requirement is keeping the Arm copyright in the Arm authored files and the license.

That's a given. Sadly I'm currently struggling a bit with keeping the license file within the sdist though, as the github runner refuses to include the manifest files, and that even though it works locally for me on all systems I've tried (Windows, WSL (Ubuntu), Silberblue, Arch, even on Android). I will continue to investigate the issue, but in the meantime the astc-encoder license file is missing from the sdist.....

[solidpixel]

As long as the license is in the source repo, I don't think it's critical that it doesn't exist in the wheel.

[solidpixel]

it doesn't seem to support HDR and EXR would have to be dumped down to uint8.
I guess this should be noted in the documentation for it, and the decoder should probably throw a warning if quality suffers due to Pillow's inability to handle the specified profile.

Dropping HDR down to 8-bit will destroy the hdr-ness of the data, so somewhat defeats the point. For a first version of the bindings, I would propose just exposing LDR only. We can add HDR as a follow-on PR.

One thought, which could be more flexible in terms of data types, would be to have the bindings accept a 2D or 3D NumPy array in addition to/instead of a Pillow image.

[K0lb3]

Well, currently you already can just use the ASTCImage which binds to astcenc_image, and can simply take bytes as data input, so HDR as well. The same way one can also get HDR data out. Ofc in this case some other library has to be used to parse the source data format.
This makes a good point for what should be covered in the examples for it.

The PIL codec is just optional, and would make it comfier to handle LDR data.

numpy arrays and other types are a bit problematic though.
To directly interact with them the buffer interface should be used, which isn't available in the limited api (abi3) until version 3.10.
This means, that Python 3.6 to 3.9 would need version targeted bindings, while only 3.10+ could use abi3 bindings and be future proof for all future python versions.

[K0lb3]

I just noticed that I have a big flaw in the bindings concerning the image size computation.

Am I right to assume following=

• LDR & LDR_SRGB -> 4x u8 = 32bpp
• HDR: 48bpp -> 4x u16 = 64bpp
• HDR_RGB_LDR_A: 40bpp -> RGB in u32, A in u8 = 40bpp

[solidpixel]

In the native API the input data format and the color profile are orthogonal.

LDR and LDR_SRGB are usually going to use 4x u8, but it can be beneficial to pass in the data as fp16 or fp32 if you've done some software computation on the image before compressing (e.g. applying premultipled alpha). Forcing the data back to 4x u8 before compression in these scenarios introduces an additional set of quantization error that can be avoided by passing things in as floats.

The current codec API only supports passing a single format for all channels, so HDR_RGB_LDR_A would need use fp16 or fp32 for all 4 components, and rely on the alpha channel data being in a legal [0.0, 1.0] LDR range.

[K0lb3]

I see, thanks for clarifying this.
I forgot about the ASTCENC_TYPE as input when I wrote the message.

I hope that I'm getting this right now.

The input and output image data are always of the size: width * height * depth * 4 * sizeof(T), with sizeof(T) being determined via the given ASTCENC_TYPE, so 1 for U8, 2 for F16, 4 for F32.

The encoding type for both HDR modes is expected to be f16 or f32.

I know that this is a very basic question for this topic, but I just want to make sure that I can integrate a good size check in the python code, so that it doesn't segfault. For me it's slightly acceptable if C/C++ programs silently crash, as this is basically the expected behavior for an invalid input. In Python it's a different story though, as users expect an exception and get confused by silent crashs....and I would also dare to say that an invalid input is way more likely to be done by Python users.

[K0lb3]

Over the weekend I looked a bit into alternatives to PIL for image loading and writing in Python with mixed results.

• pywuffs would be a great option for loading images, but it doesn't come with exhaustive binary wheels.
• pyvips in combination with the unofficial pyvips-binary should be a sound solution, but requires some attention, as the unofficial binary addition should become official in some time.
• the imagemagick wrappers and bindings are all a bit...weird....and don't seem to be that easy to setup cross platform, to say nothing about exhaustive wheels
• same issue for opencl