Rewrite OpenEXR python bindings using pybind11 and numpy

[cary-ilm]

This introduces an entirely new python API for reading and writing OpenEXR files that supports all file features (or will soon): scanline, tiled, deep, multi-part, etc. It uses numpy arrays for pixel data, and it supports both separate arrays for each channel and interleaving of RGB data into a single composite channel. It leaves the existing binding API in place for backwards-compatibility; there's no overlap between the two APIs.

See src/wrappers/python/README.md for examples of the new API.

The API is simple: the File object holds a py::list of Part objects, each of which has a py::dict for the header and a py::dict for the channels, and each channel holds a numpy array for the pixel data. There's intentionally no support for selective scanline/time reading; to keep the API simple, reading a file reads the entire channel data for all parts. There is, however, an option to read just the headers and skip the pixel data entirely.

A few things don't work yet but should be fixed before release:

• Reading and writing of deep data samples isn't finished.
• ID manifest attributes aren't supported yet.
• For mip-mapped images, it current only reads the top level.
• Channel subsampling is not working.
• I'm not 100% sure the memory ownership is handled properly.

This also does not (yet) properly integrate the real Imath types from the proper Imath bindings. It leaves in place for now the internal "Imath.py" module, but defines its own internal set of set of Imath classes. This needs to be resolved once the Imath bindings are distributed via pypi.org.

The test suite downloads images from openexr-images and runs them through a battery of reading/writing tests. Currently, the download is enabled via the OPENEXR_TEST_IMAGE_REPO environment variable that is off by default but is on in the python wheel CI workflow.

This also adds operator== methods to KeyCode and PreviewImage, required in order to implement operator== for the Part and File objects.

[meshula]

For the purposes of code review, could you elaborate a little on this ~

It leaves the existing binding API in place for backwards-compatibility; there's no overlap between the two APIs.

Specifically I'm wondering if the existing API implementation is untouched, or if the existing API is re-implemented using pybind11. There's a lot here to read and before starting I'm wondering if also we need to be verifying that existing API is fulfilled in the new implementation.

[cary-ilm]

Thanks for looking! The existing C-python implementation remains untouched. I renamed the file to PyOpenEXR_old.cpp didn't change anything other than to import the symbols into the new pybind11-defined module. The old API implements classes InputFile and OutputFile, which continue to operate as before, while the new implementation has a single new class File, which does both reading and writing.

The test for the old API, which illustrates the usage, is in src/wrappers/python/tests/test_old.py

This seemed a reasonable approach since I wanted to change so much about the API, primarily the switch to numpy for pixel data, but not entirely break everything.

The other .py files in src/wrappers/python/tests use the new API. Definitely start there, since getting your feedback on the usage is probably more important than on the actual pybind11 implementation.

The API is simple enough that the src/wrappers/python/README.md is a pretty good summary. It doesn't illustrate everything but it covers the basics.

[lji-ilm]

If we want the outward facing API to be really pythonic, probably we don't need the IMath types?

A more pythonic way would simply be o.header()["displayWindow"] == ((3 ,4), (5, 6))

I looked up src\wrappers\python\IMath.py, it doesn't seem there is a substantial difference from python tuples, except for the print (repr) method. IMath.Box did not really type it all the way down, with its min and max looks like python tuples (although the author's intention is to use IMath.point).

If we really want to enforce the dimensionality of the parameter but remain pythonic or num-pythonic, maybe set this to an ndarray and assert its shape is (2, 2). I tend to think a few classes (esp. point and box) in Imath.py is reinventing the wheel in a C-style and doesn't really read like modern numpy/scipy code. These data could all just be ndarrays, that's actually less confusion than letting the user figure out whether box.min should be plain tuple or Imath.point. The important things are this parameter's type and shape, which are all enforced in ndarry for free (up on assign, arithmetic and comparison).

what do people think?

[lgritz]

I'm also inclined to believe that maybe we should accept and promote the built-in Python types (tuples, arrays, dictionaries) for all the usual "short" data types for which they seem a decent fit, and use Numpy arrays for blocks of image pixel data, and call it a day. The Imath types are not Pythonic, but rather look like somebody simply wrapped a C++ library. They're not friendly to people who must pass data back and forth between OpenEXR and almost any other Python library (Alembic is the only Python interface I know of that expects people to use classes from Imath's Python bindings).

[cary-ilm]

I removed all the Imath classes and replaced them with numpy arrays and tuples. Vec 2/3 Int/Float/Double attributes appear in the header dict as 2- or 3-element numpy arrays of type int32, float32, or float64, and M33/44 Float/Double as 3x3 or 4x4 numpy arrays. Box2s are tuples of 2-element arrays.

RationalAttributes now appear as Fraction objects from the fractions module. DoubleAttributes appear as single-element numpy arrays of type float64, so they get preserved when written. Chromaticities are 8-element tuples.

I left the TimeCode and KeyCode classes in place, since they have convenient member fields that would be unintuitive in a tuple.

The File object also now implements __enter__/__exit__ so that with File(...) as f works.

The README.md examples have been updated accordingly, so they're more pythonic now.

[lji-ilm]

I am following up with our discussion about how to best represent deep data in numpy, following up our last meeting.

I also want to pay an tribute to A-buffer here since I think EXR deep is probably just thinking about the same thing :) : https://dl.acm.org/doi/pdf/10.1145/965139.807360

Now onto the real topic. array:object[array:dtype:float] seem to be the way to go. By keeping both levels to be numpy data structures, it best supports numpy's slicing and other array based operations. The following code is an excerpt from HDF5's official h5py library's vlen data type implementation (https://docs.h5py.org/en/latest/special.html#arbitrary-vlen-data)

array([array([1, 2, 3], dtype=int32), array([1, 2, 3, 4, 5], dtype=int32)], dtype=object)

A few stackoverflow posts discussed this question, and the above answer is generally prefered due to the fact that it doesn't involve native python lists, which will not support numpy operations. The following is only one example:

https://stackoverflow.com/questions/3386259/how-to-make-a-multidimension-numpy-array-with-a-varying-row-size

A special mention to the "Awkward Arrays" library, which specialize in dealing with this class of "varying data per sample" problems: https://awkward-array.org/doc/main/getting-started/index.html . I don't think we need to add a dependency, however the first two questions in FaQ did a good job in terms of a state-of-the-art review of the implementations.

I reviewed a few scientific imaging formats, including FITS (https://fits.gsfc.nasa.gov/fits_primer.html), but their data container per pixel (or per sample) are homogeneous. HDF5's vlen is the only one I spotted in a major format that deals with deep-like data formats. We might be quite on the frontier here.

[cary-ilm]

Thanks for that consideration, @lji-ilm. I'm satisfied with the dtype=object approach, the implementation is straightforward and the structure is I think what the user would naturally expect.

And for what it's worth, ChatGPT is what originally offered this suggestion, after nobody answered my query on the pybind11 discussion page.

[lji-ilm]

I think the current build install with OPENEXR_BUILD_PYTHON=ON will copy the built artefact to /usr/local/python/OpenEXR, which might be a distro-specific install path.

My wsl ubuntu 22.04 uses a few paths under /usr/local/lib, such as /usr/local/lib/python3.10 etc in the vanilla python apt-get installation for global python paths. I don't think it's consistent across the various distros. It might be worthwhile to have a note about the artefact's cmake install path v.s. vanilla python path to avoid surprises, such as the import test fail.

Maybe we should have a step in CMAKE to ensure python paths... However if at the end of the day we expect people to pip this, maybe it's not a concern.

[lji-ilm]

I would like to have another thought about this API.

What happens with the rgba parameter is essentially this:

1. There is an extra flip in the core Python API C implementation.
2. If you do NOT turn it on, what you get is more low level (more conforming to the file's storage format), but more difficult to use in subsequent python/numpy operations.
3. If you DO turn it on, what you get is higher level, but easier to use in subsequent numpy operations, especially for beginners.
4. This data transformation can be done in python with numpy, without bothering with C++ code.

It caught my attention because:

1. The default is hard to use and more low level, but is general purpose. You have to flip a switch to get something that's easier to use for the noob use cases, but would not work out in advanced cases. This feels backwards.
2. This can be done with numpy calls after the binding layer, instead of doing it in C++ before the binding layer

I suggest we change it to one of the following:

1. Remove this parameter and the channel gathering in C++ code. Provide another OpenEXR.utils module that is entirely written in python that does tasks similar to this. (that can be either inside or outside of the official repo -- we can debate whether that would reinvent wheels of openimageIO)
2. Flip the semantics this parameter, the C++ code will always attempt to gather channel into a nice rgba layout if it could do so, but you can put in an extra parameter to tell it to NOT do this.

[lji-ilm]

I rethought this after a week.

I think the most striaght forward way might be simply

1. don't have this parameter, simplify the API interface
2. always prep a channel "RGB" and "RGBA" numpy array if possible according to the file structure. If not possible, then don't prep it.

There are some python techniques that can do a lazy bind (e.g. only allocate memory on R, G, B initially and only allocate "RGBA" when this key is queried on the channel dict), but i wonder if that worth it.

Overall i think my thought is about that extra parameter. either remove it or move it onto the channel object. It probably should not stay on the File object's initializer

[cary-ilm]

I tend to agree. I've left it in place but inverted the sense of the parameter and renamed it separate_channels, off by default. So the default behavior is the most convenient, even if it differs from the file structure.

I'm still not completely happy with this.

[lji-ilm]

Truncated comment.

Also for this entire method -- is it appropriate to write it on the python side instead of the C side? It seems just some string manipulations and would be expressed more naturally in python, without much performance penalty of moving it off C code.

[meshula]

The function is used throughout this file, moving it would mean you'd have to move most of the implementation to Python....

[lji-ilm]

Good point... Let's leave it then (but finish the comment :)

[lji-ilm]

This sounds like it return a book or 0/1, but in fact it is a mapping between the channel name and array indices, i think.
It might be better to rename it to something along the line of "map to channel indices" or "get channel index" etc...?

[cary-ilm]

I renamed it channelNameToRGBA(). Still confusing, but hopefully a little less so.

[lji-ilm]

I can't find a copy of IMath's python bindings at hand, but should these marshallings better be in IMath rather than OpenEXR...?

[lji-ilm]

Also i understand this is internal, but this is actually a conversion rather than a read-only test, so maybe rename them to "convert_to_v2" or simply "to_v2"

[cary-ilm]

I renamed is_v2() to objectToV2() and the others similarly.

[meshula]

I'm happy if we land this; I haven't got more feedback to give, seems like playing with it for a while is a good next step.

[lji-ilm]

Agreed. Actually I have been constantly using this binding in my experiments of benchmarking/compression in the last few weeks, so not only it works but it also works under a bit of stress.

Hopefully we can see this in pip install OpenEXR soon!