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[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: huww98 / name: 胡玮文 (1ec90907601452ec72b853da545bc1a354b8a03d)

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: huww98 / name: 胡玮文 (55bdfd181200cc098ddd9f3919019250e1ba7ee6, e212b8f0bf58a9eca6ba164a8f900f245b4839b0, ef6ab36bded45d88f9499f8f54ddc0f8f6ff79f0)

[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 https://github.com/AcademySoftwareFoundation/openexr/blob/55bdfd181200cc098ddd9f3919019250e1ba7ee6/src/lib/OpenEXR/ImfFrameBuffer.cpp#L72

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.