[FEATURE REQUEST] Make ImageInput and ImageOutput APIs more cpp like

[EmilDohne]

Is your feature request related to a problem? Please describe.

The current OpenImageIO ImageInput and ImageOutput APIs are quite C-like with their type erasure through void* and TypeDesc. Since e.g. reading is already irrespective of the native bitdepth of the image performing conversion on read I believe it would be time to add more C++-like APIs to wrap the existing functionality (not replacing it to keep backwards compatibility). This should greatly help with preventing errors when using the API and e.g. passing an incorrectly sized buffer.

Describe the solution you'd like
An example of the current ImageInput::read_image signature could be converted to the following:

template <typename T>
class ImageInputT
{
    bool read_image (int subimage, int miplevel,
                     int chbegin, int chend,
                     span<T> data,
                     stride_t xstride=AutoStride,
                     stride_t ystride=AutoStride,
                     stride_t zstride=AutoStride,
                     std::optional<std::function<bool (span<std::byte>, float>)> progress_callback = std::nullopt,
                     std::optional<span<std::byte>> progress_callback _data = std::nullopt)
    {
        // Convert to types expected by ImageInput::read_image() but make this opaque to the user
    }
}

All the above proposed items are in the C++17 standard which is now the default.

With this, I would then also propose to make the docs more C++-like to e.g. convert the 'simple read' example to this:

#include <OpenImageIO/imageio.h>
using namespace OIIO;

void simple_read()
{
    using bit_depth = uint8_t;

    std::string filename = "tahoe.tif";

    auto input_ptr= ImageInputT<bit_depth>::open(filename);
    if (!input_ptr)
        return;
    const ImageSpec &spec = input_ptr->spec();
    int xres = spec.width;
    int yres = spec.height;
    int nchannels = spec.nchannels;
    auto pixels = std::vector<bit_depth>(xres * yres * nchannels);
    input_ptr->read_image(0, 0, 0, nchannels, span<bit_depth>(pixels.begin(), pixels.end()));
    // Input gets implicitly deleted on scope exit, to explicitly close call input_ptr->close()
}

The 'old' way should definitely still be kept as an additional section in the documentation

I know this feature request is fairly opinionated but I truly do believe that this is the way forward to adopt to more modern standards. By wrapping ImageInput backwards compatibility would be kept but we provide better interfaces for new users of OpenImageIO.

I was unable to find similar feature requests in the list of currently open issues but if this is a duplicate feel free to let me know. I am happy to hear your opinions

[EmilDohne]

As an addendum, I would also not be opposed to adding these functions directly into the ImageInput and ImageOutput classes as free-standing variations that call into the non-typed functions.

[lgritz]

@EmilDohne, you and I are very much on the same wavelength!

In fact, I have been working on exactly this, on and off, since December, and am very close to starting to stage the pieces as various PRs.

[EmilDohne]

Awesome! I'd be very happy to take a look at the smaller PRs once you have something ready :). Should I close this issue as duplicate or keep it open as a tracker?

[lgritz]

Let's leave it, and we'll mark it done when the last of the pieces are in.

While I have your attention, I do have a design question for you, though: Where non-contiguous strides are concerned, would you prefer to do it as you have written above where there is an ordinary span that gives the WHOLE extent of possible reading/writing, plus separate strides? Or do you think it would be better to have a new type (say, image_span) that somehow is a multi-dimensional span that combines the pointer, length of each dimension, and stride for each dimension?

[EmilDohne]

Hmm if I understand correctly you mean where e.g. you might have a stride of 3 to get only the 'G' channel how to interpret that? I would personally probably go with a design where the strided data gets copied into your buffer so you on the receiving end only get every third stride. But I have to be honest I don't do enough with non-scanline strided data to have a strong opinion on this.

[lgritz]

An example of strided input is when you are reading one tile into a buffer that will eventually hold the whole image. If each "line" of the tile is written contiguously, it won't be in the right position -- you need the y stride of how it places the data in your buffer to be the scanline-to-scanline spacing of the whole image, not merely the width or line-to-line stride of a single tile that you're reading. Another example would be if you want to read an RGB (3-channel) file into an RGBA (4-channel) buffer -- you need each pixel to start in 4 byte increments (assuming you're reading into uint8 data), not the 3-byte increments that the file might suggest. And yes, also your example of wanting to read just one channel into a buffer that holds room for all the channels.

So what I'm asking is, which of the two APIs looks better to you:

1. read_image(int subimage, int miplevel, span<T> data, stride_t xstride=AutoStride, stride_t ystride=AutoStride, stride_t zstride=AutoStride)

In this case, the interface is almost the same as now, but instead of T* data, we have span<T> data. But for non-default strides, there are actually places in data where we should not be writing, but the span just has a pointer and total size, so it can't check that.

2. read_image(int subimage, int miplevel, image_span<T> data)

No more stride parameters, but we use a new image_span type that is basically an N-dimensional span with bounds and strides for each dimension (channel, pixel, scanline, plane), and accessing data through the image_span automatically does the stride math and ensures that you're accessing within the bounds of each dimension properly, including not touching data in the "space in between" where the strides say should be left alone.

[EmilDohne]

Thanks for the examples! I think visually I would prefer the second API as that leaves the stride handling to its own struct that the user can interact with if they need, but if they only need auto-strides they don't have to worry about that (similar to now AutoStride).

In fact, I think simplifying the interface to just have three parameters will be a very nice QOL improvement on the caller side as well so you don't have to do this:

read_image(
   0, // subimage
   0, // miplevel
   data, //data
   1, // xstride
   10, // ystride
   1 // zstride
)

but instead

read_image(0, 0, data);
data.stride_x = 1;
data.stride_y = 10;
data.stride_z = 1;

I'm not sure how the image_span API will look like but I think that would be a lot cleaner and separate responsibilities a bit

[lgritz]

Yeah, I think you get the gist of what I'm doing. Thanks for the feedback.

I've been working on this all part-time since December. It's a lot of tedious changes everywhere, so I'm breaking it up into several discrete steps that individually are easy to understand and review and easier to test.

[EmilDohne]

Yeah I can imagine its quite a large change, thank you for tackling this though and feel free to tag me on MRs to have a look over

[lgritz]

Can I ask you another question about preference for the design of image_span?

1. image_span<Dims> template to specialize it for dimensions (2 = channel+x / scanline, 3 = c,x,y / 2D image or tile, 4 = c,x,y,z / volume or volumetric tile), which leads to less storage and simpler construction for the more common 2 and 3D cases.
2. Just one image_span handling the fully general (up to 4D) case. A bit more storage and more params passed to construct it if you really only need to describe a scanline or 2D image, but it's simpler by not having a proliferation of types or being able to choose them wrong.

[EmilDohne]

I think also here the second option is better and likely easier to maintain, especially if all the ctors take e.g. AutoStride as param and then the first two dims are x and y which most people will likely be concerned with so you only have to specify those