Figure out a decent first draft API.

[cessen]

I'd like to take some time to play with API ideas, to see if we can come up with something better than we currently have, and also account for supporting the various features of OpenEXR.

I don't expect to figure out a final 1.0 API or anything, but rather a first draft that we can implement and use for a while to get more experience for the next iteration.

Some notes of my own below.

Using the builder pattern, pros and cons

For OutputFile I'm currently using a builder pattern, so creating a file and writing to it looks something like this:

let mut fb = {
    // Create the frame buffer ...
};

OutputFile::new()
    .resolution(256, 256)
    .channel("R", PixelType::FLOAT)
    .channel("G", PixelType::FLOAT)
    .channel("B", PixelType::FLOAT)
    .open(Path::new("/path/to/file.exr"))
    .unwrap();
    .write_pixels(&mut fb).unwrap();

This seems to make sense, because there are a lot of things that can potentially be specified about the file (but which mostly have obvious defaults), and due to OpenEXR's C++ API they all need to be specified before actually opening the file for writing. It's also convenient for creating an OutputFile inline somewhere.

However, a weakness of this is, for example, that if you have an array of channels already, the builder pattern actually makes it a bit more awkward. Instead of something like this:

let mut out = OutputFile::new().resolution(256, 256);
for (name, pixel_type) in channel_vec.iter() {
    out.add_channel(name, pixel_type);
}
// ...

You have to do this:

let mut out = OutputFile::new().resolution(256, 256);
for (name, pixel_type) in channel_vec.iter() {
    out = out.channel(name, pixel_type);
}
// ...

OutputFile::new()
.resolution(256, 256)

That isn't awful, but it does IMO read a bit strange with the out = out.whatever() in the loop.

Supporting different InputFile and OutputFile types

OpenEXR supports scanline files, tiled files, multi-part files, and deep files and all the permutations thereof. For simple input/output this isn't too much of a concern, and I would like to make sure there are simple API's for cases where people just don't care. But first I want to make sure we wrap all of the lower-level functionality well, and then we can build on top of that later for simpler use-cases.

For output, my current idea is to have multiple different *Writer types that you can get from *_open() calls. So, for example, if you want a create a tiled file you would do:

let mut out: TiledWriter = OutputFile::new()
    .resolution(256, 256)
    // ...
    .open_tiled(Path::new("/path/to/file.exr"));

And for a tiled and multi-part file:

let mut out: TiledMultiPartWriter = OutputFile::new()
    .resolution(256, 256)
    // ...
    .open_tiled_multipart(Path::new("/path/to/file.exr"));

Etc.

This would correspond closely to the various *OutputFile types in the C++ API, and would allow us to present a tailer-made API for each output type (e.g. writing tiles to a tiled file).

For InputFile I'm thinking we could do something similar by returning an enum of possible *Reader types.

Accessing InputFile's channels

To properly read an input file, it should either be verified that the expected channels exist in it and are of the right type, or the channels that do exist should be used to build the frame buffer. This requires accessing the channel descriptors.

So far all I've implemented for that is an iterator API:

let input = // Get input file
for (name, channel) in input.channels() {
    println!("{} {:#?}", name, channel);
}

But that is unlikely to cut it on its own. Some kind of direct access would probably be good:

let channel_descriptor = input.get_channel("R").unwrap();

Custom Attributes

OpenEXR files support custom attributes to be written to and read from exr files. I haven't investigated the API closely enough to know exactly how they work (e.g. what types are supported, if it's extensible, etc.). But at first blush, I think using an API similar to what we have for channels would be good:

OutputFile::new()
    .resolution(256, 256)
    .attribute("attribute_name_1", /* some way of specifying an attribute value */)
    .attribute("attribute_name_2", /* some way of specifying an attribute value */)
    // ...

let input = // Get input file
for (name, attribute) in input.attributes() {
    println!("{} {:#?}", name, attribute);
}

let attribute_value = input.get_attribute("attribute_name_1").unwrap();

[cessen]

@Ralith
Would love your thoughts on this, if you have the time!

[Ralith]

Using the builder pattern, pros and cons

Using a builder whose methods take and return &mut Self, rather than Self, addresses the cons here. This is the approach taken by the cmake and gcc crates, for example, and we've already taken advantage of that in build.rs.

Supporting different InputFile and OutputFile types

Why diverge from the upstream FooOutputFile naming scheme here? I figure we should stay as close as we can except where that would severely compromise API usability. Naming aside, the output stuff makes sense to me.

I poked around the input end of things in the OpenEXR and it wasn't obvious to me how to dynamically identify the type of a file. Is it clear that the different interfaces aren't more about access patterns than the actual file itself? I'd also be concerned about keeping it convenient for people to decode data from a file without necessarily caring what its internal layout is. Neither of these are insurmountable, just things to bear in mind.

Accessing InputFile's channels
Custom Attributes

These both seem like basically reasonable APIs, but is there a good reason for us to be inlining Header accessors into the input/output objects like that? If we stick more closely to the upstream API of mediating through a header object, there might be substantially less boilerplate, and the interface would be less surprising.

[cessen]

Why diverge from the upstream FooOutputFile naming scheme here? [...]

That's an excellent point. Yeah, let's try to match naming wherever possible.

I poked around the input end of things in the OpenEXR and it wasn't obvious to me how to dynamically identify the type of a file. Is it clear that the different interfaces aren't more about access patterns than the actual file itself?

If you take a look at e.g. https://github.com/openexr/openexr/blob/develop/OpenEXR/IlmImf/ImfTiledInputFile.h, the doc comments specify that it throws an exception if the file isn't a tiled file. I haven't checked the others, but I expect the same holds true for the other variants.

The exception to that, I believe, is the vanilla InputFile which treats all files uniformly, but you also can only access the file in the lowest-common-denominator ways. Unless I'm misreading the code (which is entirely possible).

In any case, we could just try matching those semantics first: have different *InputFile types, and they return Err if the file you're trying to open doesn't match its type. Then we can layer other things like a function that returns an enum on top of those at some point in the future.

These both seem like basically reasonable APIs, but is there a good reason for us to be inlining Header accessors into the input/output objects like that?

I don't think there is. So yeah, let's expose the header too. Then we can have HeaderBuilder instead of OutputFileBuilder or whatnot, since the header is the part that takes all of the numerous parameters. And then we can return a header reference from InputFile's.

After reading what you wrote, I'm thinking now that the best approach is to try to match the upstream API semantics as closely as we can, with matching naming schemes and everything.

I'd also be concerned about keeping it convenient for people to decode data from a file without necessarily caring what its internal layout is.

Oh yeah, I definitely agree. But what I'm thinking is that we make low-ish level bindings first, and then we can layer convenience API's on top of that later. For example, for people who just want to do simple input/output of RGB(A) files, and don't care about tiling, deep pixel data, etc.

[Ralith]

In any case, we could just try matching those semantics first: have different *InputFile types, and they return Err if the file you're trying to open doesn't match its type

Reviewing the docs this seems to be the intended use pattern--either you know in advance that your file has a specific layout or you don't care. I have trouble imagining a use case that demands anything other than that.

But what I'm thinking is that we make low-ish level bindings first, and then we can layer convenience API's on top of that later

It sounds like all we really need to do here is retain (something much like) the current InputFile API while exposing the more specialized stuff separately. There's also the Rgba variants for extra simplicity if needed.

[cessen]

Reviewing the docs this seems to be the intended use pattern--either you know in advance that your file has a specific layout or you don't care. I have trouble imagining a use case that demands anything other than that.

I know of at least one, which is represented in e.g. node-based compositing applications. The user would add an OpenEXR node or something like that, and the node would expose all of the data within the selected exr file so the user can use it in the composite. But I think this is pretty niche, and can still be accomplished by just trying opening it as the different types. So yeah, I think you're right that we can ignore this. If it actually comes up at some point in the future, we can address it then.

It sounds like all we really need to do here is retain (something much like) the current InputFile API while exposing the more specialized stuff separately.

Yeah. And I'm pretty sure we get that for free by just wrapping their InputFile API as we've already done, right? So now it's just a matter of also wrapping the other *InputFile types.

There's also the Rgba variants for extra simplicity if needed.

Yeah. I was assuming we'd write our own simplifications for that, rather than wrapping theirs, because it's that much less unsafe FFI surface area and should be straightforward. But I'm not really picky.

Also, forgot to respond to this:

Using a builder whose methods take and return &mut Self, rather than Self, addresses the cons here. This is the approach taken by the cmake and gcc crates, for example, and we've already taken advantage of that in build.rs.

Ah! That makes sense. I've made that change to the new Header type. The only downside is that you can't do this:

let header = Header::new()
    .set_resolution(256, 256)
    .add_channel("R", PixelType::FLOAT)
    .add_channel("G", PixelType::FLOAT)
    .add_channel("B", PixelType::FLOAT);

Because header is just a &mut reference and the actual Header doesn't live beyond the right-hand expression. But I think that's a reasonable trade-off.

[Ralith]

I was assuming we'd write our own simplifications for that, rather than wrapping theirs, because it's that much less unsafe FFI surface area and should be straightforward.

That makes sense to me.

The only downside is that you can't do this:

It looks to me like the upstream OutputFile implementation takes a reference and copies the Header rather than consuming it. If we respect that, I think we still get OutputFile::new(Header::new().set_size(256, 256).add_channel("R". PixelType::FLOAT)..., which is probably the main ergonomic case of interest.