bug: ImageInput::read_image()/read_tiles() with an image_span transpose x and y on tiled files

[KMean]

Describe the bug

ImageInput::read_image(subimage, miplevel, chbegin, chend, format, image_span) returns false whenever a tiled image's width or height is not an exact multiple of the tile size. No error is recorded, so geterror() returns an empty string. The pointer overload reads the same files correctly.

The destination buffer here is exactly width * height * nchannels values, and the docs describe data as "the memory extent of the data buffer" without mentioning any tile-alignment requirement, so as far as I can tell there is no buffer a caller could pass that would work. That makes tiled images with partial edge tiles unreadable through this overload, and most real tiled images have partial edge tiles.

I expected this overload to read the same images the pointer overload reads, given it is documented as "the 'safe' preferred alternative to the version of read_image that takes raw pointers" — and, on failure, to record an error saying why.

I have not tried to pinpoint the cause in the reading code, so the above is only what is observable from the outside. It is possible the intended contract is different from what I assumed, in which case the silent false with no error message is still the part worth looking at.

OpenImageIO version and dependencies

OIIO 3.2.0.2dev | Windows/x86_64
    Build compiler: MSVS 1951 | C++17/199711
    HW features enabled at build: sse2
Dependencies: BZip2 1.0.8, fmt 12.1.0, Freetype 2.13.3, GIF 5.2.2, Imath 3.2.2,
libdeflate 1.25, libjpeg-turbo 3.1.3, LibRaw 0.22.0, OpenColorIO 2.5.1,
OpenEXR 3.4.7, OpenJPEG 2.5.4, openjph 0.26.3, PNG 1.6.55, Robinmap 1.4.1,
TIFF 4.7.1, WebP 1.6.0, ZLIB 1.3.1

Built from main at 9b96c37 with the vcpkg toolchain. Also reproduced identically on the released 3.1.12.0 (vcpkg binary, same machine and compiler), so this is not new in 3.2.

To Reproduce

Self-contained repro, public OpenImageIO API only, covering just this behaviour:
https://github.com/KMean/oiio-bind/blob/12025630fddf902417d79951d0e5d766bd515498/contrib/span_tiled_read_repro.cpp

It writes tiled OpenEXR files with write_image, then reads each back three ways and exits non-zero if the overloads disagree. Output on 3.2.0.2dev:

32x32, 16x16 tiles (exact multiple)
  read_image(image_span, explicit strides) : ok
  read_image(image_span, default strides)  : ok
  read_image(pointer)                      : ok

16x16, 16x16 tiles (exact multiple)
  read_image(image_span, explicit strides) : ok
  read_image(image_span, default strides)  : ok
  read_image(pointer)                      : ok

40x32, 16x16 tiles (PARTIAL edge tiles)
  read_image(image_span, explicit strides) : FAILED
  read_image(image_span, default strides)  : FAILED
  read_image(pointer)                      : ok

32x24, 16x16 tiles (PARTIAL edge tiles)
  read_image(image_span, explicit strides) : FAILED
  read_image(image_span, default strides)  : FAILED
  read_image(pointer)                      : ok

40x24, 16x16 tiles (PARTIAL edge tiles)
  read_image(image_span, explicit strides) : FAILED
  read_image(image_span, default strides)  : FAILED
  read_image(pointer)                      : ok

Both image_span forms are covered, and they fail identically:

• the image_span<std::byte> plus TypeDesc::FLOAT overload, with every stride spelled out;
• the typed image_span<float> overload, letting OpenImageIO compute every stride itself:

const image_span<float> data(buffer.data(), nchannels, width, height);
in->read_image(0, 0, 0, nchannels, data);

So the result does not depend on my stride arithmetic. Images whose dimensions are an exact multiple of the tile size are unaffected, and the pointer overload reads all five files correctly.

Steps:

1. Write a tiled EXR, e.g. 40x24 with 16x16 tiles, 3 channels, float.
2. Read it back into a 40*24*3 float buffer using either the image_span<std::byte> + TypeDesc::FLOAT overload or the typed image_span<float> overload.
3. It returns false and geterror() is empty.
4. The same file read with read_image(0, 0, 0, 3, TypeDesc::FLOAT, ptr) succeeds.

I could not reproduce this through oiiotool, since the failing path is the image_span overload rather than anything the tools call.

Additional context

Found while writing Rust bindings over the 3.1 API. Happy to test a patch or provide more detail.

[KMean]

Correction. I under-reported this, and I found the cause.

My original report said tiled images whose dimensions are exact multiples of the
tile size "read fine". That is wrong, and it is the part I most want to retract:
a 32x32 image with 16x16 tiles returns true and delivers 992 of its 1024
pixels from the wrong place. The only shape I have found that reads correctly
is an image that fits in a single tile.

Cause

ImageInput::read_tiles taking an image_span forwards to the pointer overload
with x and y transposed. imageinput.cpp:694 on v3.1.12.0, :702 on
v3.1.16.0, :719 on main:

    // Default implementation (for now): call the old pointer+stride
    return read_tiles(subimage, miplevel, ybegin, yend, xbegin, xend, zbegin,
                      zend, chbegin, chend, format, data.data(),
                      data.xstride());

The overload it calls is declared

ImageInput::read_tiles(int subimage, int miplevel, int xbegin, int xend,
                       int ybegin, int yend, int zbegin, int zend, int chbegin,
                       int chend, TypeDesc format, void* data, ...)

so the y range arrives as x and the x range as y.

ImageInput::read_image's image_span overload reads a row of tiles per call
(imageinput.cpp:1239), passing x: 0..width, y: y0..y0+tile_height. That
range is full-width by one tile high, so it is essentially never square, and the
transposition changes it every time. That is why almost every tiled image is
affected, not just the ones with partial edge tiles.

Where the transposed range still passes valid_tile_range, the read succeeds
and returns a different rectangle of the image. Where it does not, read_tiles
returns false, which is the case I originally reported, and no error is recorded.

ImageOutput::write_tiles's image_span overload forwards in the declared
order (imageoutput.cpp:279), which is what convinced me this is a slip rather
than a convention I had misread.

Measured

Reading each file whole through read_image, once with an image_span<float>
and once with the pointer overload, then comparing the pixels. 16x16 tiles
throughout, single channel, float:

image	image_span returns	pixels wrong	left unwritten
16x16	true	0 of 256	0
32x32	true	992 of 1024	0
48x48	true	2272 of 2304	0
40x40	true	1560 of 1600	0
32x16	true	496 of 512	256
16x32	true	256 of 512	256
40x32	false	,	, (no error recorded)
32x24	false	,	, (no error recorded)
40x24	false	,	, (no error recorded)

Identical on 3.1.12.0 and on 3.2.0.2dev.

The forwarding line is present in every 3.1.x tag I can reach, from
v3.1.4.0-beta, the first release with the image_span overload, through
v3.1.16.0, and on main. v3.0.21.0 and earlier have no image_span
overload at all, so there is no released version that has this API without the
transposition.

The last column is worth a separate mention: for 32x16 and 16x32 the call
reports success while leaving half the destination buffer untouched. The program
below pre-fills it with -1 and finds -1 still there afterwards, so a caller that
trusts the true reads uninitialised memory.

Reproduction

Public API only. Each pixel is written as x + y/1000, so a returned value
says which pixel it came from.

// c++ -std=c++17 span_read_image.cpp -lOpenImageIO -lOpenImageIO_Util
#include <OpenImageIO/image_span.h>
#include <OpenImageIO/imageio.h>
#include <cmath>
#include <cstdio>
#include <string>
#include <vector>
using namespace OIIO;

static constexpr int TILE = 16;

static bool write_tiled(const std::string& path, int w, int h)
{
    auto out = ImageOutput::create(path);
    if (!out)
        return false;
    ImageSpec spec(w, h, 1, TypeDesc::FLOAT);
    spec.tile_width = spec.tile_height = TILE;
    spec.tile_depth                    = 1;
    if (!out->open(path, spec))
        return false;
    std::vector<float> px(size_t(w) * h);
    for (int y = 0; y < h; ++y)
        for (int x = 0; x < w; ++x)
            px[size_t(y) * w + x] = float(x) + float(y) / 1000.0f;
    bool ok = out->write_image(TypeDesc::FLOAT, px.data());
    return ok & out->close();
}

static void probe(int w, int h)
{
    char name[64];
    std::snprintf(name, sizeof name, "probe_%dx%d.exr", w, h);
    if (!write_tiled(name, w, h)) {
        std::printf("could not write %s\n", name);
        return;
    }

    std::vector<float> viaspan(size_t(w) * h, -1.0f);
    std::vector<float> viaptr(size_t(w) * h, -1.0f);

    auto a = ImageInput::open(name);
    const image_span<float> data(viaspan.data(), 1u, uint32_t(w), uint32_t(h));
    const bool span_ok         = a->read_image(0, 0, 0, 1, data);
    const std::string span_err = a->geterror();
    a->close();

    auto b            = ImageInput::open(name);
    const bool ptr_ok = b->read_image(0, 0, 0, 1, TypeDesc::FLOAT,
                                      viaptr.data());
    b->close();

    size_t wrong = 0, untouched = 0;
    for (size_t i = 0; i < viaspan.size(); ++i) {
        if (std::fabs(viaspan[i] - viaptr[i]) > 1e-6f)
            ++wrong;
        if (viaspan[i] == -1.0f)
            ++untouched;
    }

    std::printf("%3dx%-3d  span=%-5s ptr=%-5s  %zu of %zu wrong, %zu unwritten%s\n",
                w, h, span_ok ? "true" : "false", ptr_ok ? "true" : "false",
                wrong, viaspan.size(), untouched,
                (!span_ok && span_err.empty()) ? "   (no error recorded)" : "");
    std::remove(name);
}

int main()
{
    std::printf("OpenImageIO %s, %dx%d tiles\n\n", OIIO_VERSION_STRING, TILE,
                TILE);
    probe(16, 16);
    probe(32, 32);
    probe(48, 48);
    probe(40, 40);
    probe(32, 16);
    probe(16, 32);
    probe(40, 32);
    probe(32, 24);
    probe(40, 24);
}

I have retitled the issue, since "partial edge tiles" described only the subset
that fails loudly, and the quiet majority matters more.