Support negative stride

[huww98]

Add support for negative stride to FrameBuffer Slice.

Change data type of stride from size_t to ptrdiff_t.

Add support for negative stride in copyIntoFrameBuffer and copyFromFrameBuffer.

Add a new testNegativeStride test.

Fixes: #614
Signed-off-by: 胡玮文 [email protected]

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: huww98 / name: 胡玮文 (1ec9090)

[meshula]

same comment about pre-testing the validity of the input parameters.

[meshula]

since an API change is involved, there'd be a version bump to OpenEXR required. Perhaps we should retain the existing signature in addition to introducing a new one, and have the size_t variant call the ptrdiff_t variant with appropriate casting to silence warnings.

[huww98]

I think both gcc and clang (-Wall) would not produce warnings when passing size_t argument to ptrdiff_t parameter.

But these changes do break the ABI. However, OpenEXR usually has version number in its soname. So the binary capability would break after the next minor release anyway. Should I try to keep ABI capability in this PR?

[peterhillman]

It strikes me that supporting flipping images on read is definitely a useful missing feature, but doing it by passing a negative stride is kind of a hack, rather than a clean friendly API feature. In particular, it's not obvious what the base pointer should be when the image dataWindow doesn't match the displayWindow.
One option would be to leave the constructor using size_t, but use ptrdiff_t internally, and add a new Make() static method for flipped images that createa a Slice with a negative stride and computes the correct base pointer for you.
That should also be a less significant ABI change than modifying the constructor.

[huww98]

I'm writing a python extension. In my use case, I would definitely want to pass negative strides directly, because I could get negative strides from NumPy.

If we want to be more friendly to C++ users, instead of adding more Make overloads, how about adding Slice::flipX() and Slice::flipY() methods that negate the strides of an existing Slice and compute the base pointer?

[meshula]

To my way of thinking, passing ptrdiff_t is less confusing at first blush than creating an unflipped datawindow. The displayWindow has to be an "increasing in memory, 0,0 upper left window".

would it make sense to say displaywindow and datawindow have to be in harmony?

in other words, if xs < 0, then origin is right instead of left, if ys < 0, origin is bottom instead of top?

[huww98]

If you mean the base pointer, then yes. the base pointer always points to the logical (0,0) pixel in the .exr file, but may not point to the first byte in the FrameBuffer Slice with negative stride. And this formula should always hold true:

openexr/src/lib/OpenEXR/ImfFrameBuffer.h

Line 49 in 55bdfd1

// base + (xp / xSampling) * xStride + (yp / ySampling) * yStride

.header().dataWindow().min should still less than or equal to .header().dataWindow().max

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: huww98 / name: 胡玮文 (55bdfd1, e212b8f, ef6ab36)

[huww98]

CI now works in my fork. I've added the missing includes.

[huww98]

Hi all, anything I can do to push this PR forward?

[meshula]

@peterhillman ~ @huww98 has addressed my structural comments about the code. Have you had a chance to think about the negative stride issues that you brought up earlier? In my mind, I'm not quite picturing how the proposed MakeFlipped use case would look, since it would seem like you'd have to set up an incorrect positive stride and then heal it later. The advantage of the negative stride at construction is that the pointers are never in an unworkable state. But maybe I'm misunderstanding the construction suggested?

[peterhillman]

I think all those changes are good.
In addition, I'm suggesting we also add another 'Make' function to build a Slice object for reading flipped images: the caller passes the pointer to the first byte of their array, and it computes the correct base pointer for them, setting up the negative yStride. So the user would never need to compute a yStride, either positive or negative.
@kdt3rd added the static Make functions because computing the base pointer correctly is hard, particularly if there's a possibility of integer overflow, and I think it's harder still when you want to read images flipped. Code that gets this wrong would crash (or worse) when opening an image that has a dataWindow that doesn't match the displayWindow.

[meshula]

Ah, thank you, now I understand :)
@huww98 could we ask you to add the additional Make function please?

[huww98]

@peterhillman I agree that computing the base pointer for the user can be helpful. But should we defer that to another PR?

I think adding more 'Make' overload can result in a combination explosion in the future. Maybe we should just add two parameters (bool flipX, bool flipY) to the existing Make overload (Another ABI breaking change)? Or as I proposed before, Add some method like Slice::flipY() so:

auto slice = Slice::Make(..., /*flipY=*/true);

is equivalent to

auto slice = Slice::Make(...);
slice.flipY();

[huww98]

I just noticed

openexr/src/lib/OpenEXR/ImfFrameBuffer.cpp

Line 72 in 55bdfd1

if (yStride == 0) yStride = (w / xSampling) * xStride;

Should I change it to yStride = (w / xSampling) * abs(xStride);?

[peterhillman]

I don't think Make() is useful with negative xStride or yStride, as currently implemented. Make() assumes it is provided with the pointer to the beginning of the user array (plus an offset to account for the channel index), and it computes the offset to pixel 0,0 for you, so the Slice is initialized correctly.

If we are happy with 'negative stride' implying the image will be flipped on read, The Make method might as well follow the same convention, which should mean new overloads or extra parameters aren't required to indicate flips. But it does need extra logic to test for negatives and get the pointer right.

I've not properly thought this through but the overload that takes a dataWindow could be passed negative strides to indicate flipping or flopping, and that would compute the correct base pointer for you, using dataWindow.max as well as dataWindow.min. It might also be possible to implement that in the "origin,width,height" overload, but perhaps that's more confusing, as it's not obvious which 'origin' it wants when the image is flipped, and whether width and height should also be negative. Perhaps it would be clearer if that overload stayed as it was, with unsigned strides, so it' doesn't support flipping.

Is it possible to implement Slice::flipY()? Slice doesn't know the size of the dataWindow, which is required to adjust the base pointer.

Usually Make is used something like frameBuffer.insert( "R" , Slice::Make(base,...)) so there's no elegant place to call flipY()

[huww98]

I would like the new semantic of Make with "negative stride" to be:

• negative stride indicates flipped image
• ptr points to the expected destination of the pixel at origin, so that I can just pass in dataWindow from header, strides and base pointer from NumPy to get the correct setting. (This is my motivation of this PR)
• if any stride is passed as 0, then densely-packed non-flipped stride is assumed, that is yStride = (w / xSampling) * abs(xStride);
• w and h are always positive so that dataWindow.max - dataWindow.min can always be passed in.

If we decide to add filpX and flipY parameters:

• When flipY = true is passed, the semantic is constructing a slice as usual then flipping it. That is:

if (flipY) {
    ptr += yStride * (h - 1);
    yStride = -yStride;
}

So we can just pass in the pointer to the beginning of the user array and get a flipped slice.

I think this is simple for most use cases and easy to understand.

Is it possible to implement Slice::flipY()? Slice doesn't know the size of the dataWindow, which is required to adjust the base pointer.

Usually Make is used something like frameBuffer.insert( "R" , Slice::Make(base,...)) so there's no elegant place to call flipY()

Yes, we may need Slice::flipY(int64_t h). This may not be a good design.