Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support negative stride #1324

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Support negative stride #1324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
23e7270
Support negative stride
huww98 Jan 13, 2023
785c1fa
Add parameter check to copy{Into,From}FrameBuffer
huww98 Jan 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions src/lib/OpenEXR/ImfAcesFile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ AcesOutputFile::~AcesOutputFile ()

void
AcesOutputFile::setFrameBuffer (
const Rgba* base, size_t xStride, size_t yStride)
const Rgba* base, ptrdiff_t xStride, ptrdiff_t yStride)
{
_data->rgbaFile->setFrameBuffer (base, xStride, yStride);
}
Expand Down Expand Up @@ -282,11 +282,11 @@ class AcesInputFile::Data

RgbaInputFile* rgbaFile;

Rgba* fbBase;
size_t fbXStride;
size_t fbYStride;
int minX;
int maxX;
Rgba* fbBase;
ptrdiff_t fbXStride;
ptrdiff_t fbYStride;
int minX;
int maxX;

bool mustConvertColor;
M44f fileToAces;
Expand Down Expand Up @@ -440,7 +440,7 @@ AcesInputFile::~AcesInputFile ()
}

void
AcesInputFile::setFrameBuffer (Rgba* base, size_t xStride, size_t yStride)
AcesInputFile::setFrameBuffer (Rgba* base, ptrdiff_t xStride, ptrdiff_t yStride)
{
_data->rgbaFile->setFrameBuffer (base, xStride, yStride);
_data->fbBase = base;
Expand Down
5 changes: 3 additions & 2 deletions src/lib/OpenEXR/ImfAcesFile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
#include "ImfRgba.h"
#include "ImfThreading.h"

#include <cstddef>
#include <string>

OPENEXR_IMF_INTERNAL_NAMESPACE_HEADER_ENTER
Expand Down Expand Up @@ -150,7 +151,7 @@ class IMF_EXPORT_TYPE AcesOutputFile
//------------------------------------------------

IMF_EXPORT
void setFrameBuffer (const Rgba* base, size_t xStride, size_t yStride);
void setFrameBuffer (const Rgba* base, ptrdiff_t xStride, ptrdiff_t yStride);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


//-------------------------------------------------
// Write pixel data (see class Imf::OutputFile)
Expand Down Expand Up @@ -247,7 +248,7 @@ class IMF_EXPORT_TYPE AcesInputFile
//-----------------------------------------------------

IMF_EXPORT
void setFrameBuffer (Rgba* base, size_t xStride, size_t yStride);
void setFrameBuffer (Rgba* base, ptrdiff_t xStride, ptrdiff_t yStride);

//--------------------------------------------
// Read pixel data (see class Imf::InputFile)
Expand Down
10 changes: 5 additions & 5 deletions src/lib/OpenEXR/ImfCRgbaFile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -947,7 +947,7 @@ ImfCloseOutputFile (ImfOutputFile* out)

int
ImfOutputSetFrameBuffer (
ImfOutputFile* out, const ImfRgba* base, size_t xStride, size_t yStride)
ImfOutputFile* out, const ImfRgba* base, ptrdiff_t xStride, ptrdiff_t yStride)
{
try
{
Expand Down Expand Up @@ -1043,8 +1043,8 @@ int
ImfTiledOutputSetFrameBuffer (
ImfTiledOutputFile* out,
const ImfRgba* base,
size_t xStride,
size_t yStride)
ptrdiff_t xStride,
ptrdiff_t yStride)
{
try
{
Expand Down Expand Up @@ -1165,7 +1165,7 @@ ImfCloseInputFile (ImfInputFile* in)

int
ImfInputSetFrameBuffer (
ImfInputFile* in, ImfRgba* base, size_t xStride, size_t yStride)
ImfInputFile* in, ImfRgba* base, ptrdiff_t xStride, ptrdiff_t yStride)
{
try
{
Expand Down Expand Up @@ -1245,7 +1245,7 @@ ImfCloseTiledInputFile (ImfTiledInputFile* in)

int
ImfTiledInputSetFrameBuffer (
ImfTiledInputFile* in, ImfRgba* base, size_t xStride, size_t yStride)
ImfTiledInputFile* in, ImfRgba* base, ptrdiff_t xStride, ptrdiff_t yStride)
{
try
{
Expand Down
11 changes: 6 additions & 5 deletions src/lib/OpenEXR/ImfCRgbaFile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "ImfExport.h"

#include <stdlib.h>
#include <stddef.h>

#ifdef __cplusplus
extern "C" {
Expand Down Expand Up @@ -297,7 +298,7 @@ int ImfCloseOutputFile (ImfOutputFile* out);

IMF_EXPORT
int ImfOutputSetFrameBuffer (
ImfOutputFile* out, const ImfRgba* base, size_t xStride, size_t yStride);
ImfOutputFile* out, const ImfRgba* base, ptrdiff_t xStride, ptrdiff_t yStride);

IMF_EXPORT
int ImfOutputWritePixels (ImfOutputFile* out, int numScanLines);
Expand Down Expand Up @@ -335,8 +336,8 @@ IMF_EXPORT
int ImfTiledOutputSetFrameBuffer (
ImfTiledOutputFile* out,
const ImfRgba* base,
size_t xStride,
size_t yStride);
ptrdiff_t xStride,
ptrdiff_t yStride);

IMF_EXPORT
int ImfTiledOutputWriteTile (
Expand Down Expand Up @@ -385,7 +386,7 @@ int ImfCloseInputFile (ImfInputFile* in);

IMF_EXPORT
int ImfInputSetFrameBuffer (
ImfInputFile* in, ImfRgba* base, size_t xStride, size_t yStride);
ImfInputFile* in, ImfRgba* base, ptrdiff_t xStride, ptrdiff_t yStride);

IMF_EXPORT
int ImfInputReadPixels (ImfInputFile* in, int scanLine1, int scanLine2);
Expand Down Expand Up @@ -414,7 +415,7 @@ int ImfCloseTiledInputFile (ImfTiledInputFile* in);

IMF_EXPORT
int ImfTiledInputSetFrameBuffer (
ImfTiledInputFile* in, ImfRgba* base, size_t xStride, size_t yStride);
ImfTiledInputFile* in, ImfRgba* base, ptrdiff_t xStride, ptrdiff_t yStride);

IMF_EXPORT
int
Expand Down
4 changes: 2 additions & 2 deletions src/lib/OpenEXR/ImfDeepFrameBuffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ OPENEXR_IMF_INTERNAL_NAMESPACE_SOURCE_ENTER
DeepSlice::DeepSlice (
PixelType t,
char* b,
size_t xst,
size_t yst,
ptrdiff_t xst,
ptrdiff_t yst,
size_t spst,
int xsm,
int ysm,
Expand Down
4 changes: 2 additions & 2 deletions src/lib/OpenEXR/ImfDeepFrameBuffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ struct IMF_EXPORT_TYPE DeepSlice : public Slice
DeepSlice (
PixelType type = HALF,
char* base = 0,
size_t xStride = 0,
size_t yStride = 0,
ptrdiff_t xStride = 0,
ptrdiff_t yStride = 0,
size_t sampleStride = 0,
int xSampling = 1,
int ySampling = 1,
Expand Down
12 changes: 6 additions & 6 deletions src/lib/OpenEXR/ImfDeepScanLineInputFile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@ struct InSliceInfo
PixelType typeInFile;
char* base;
char* pointerArrayBase;
size_t xPointerStride;
size_t yPointerStride;
ptrdiff_t xPointerStride;
ptrdiff_t yPointerStride;
size_t sampleStride;
int xSampling;
int ySampling;
Expand All @@ -75,8 +75,8 @@ struct InSliceInfo
PixelType typeInFrameBuffer = HALF,
char* base = NULL,
PixelType typeInFile = HALF,
size_t xPointerStride = 0,
size_t yPointerStride = 0,
ptrdiff_t xPointerStride = 0,
ptrdiff_t yPointerStride = 0,
size_t sampleStride = 0,
int xSampling = 1,
int ySampling = 1,
Expand All @@ -89,8 +89,8 @@ InSliceInfo::InSliceInfo (
PixelType tifb,
char* b,
PixelType tifl,
size_t xpst,
size_t ypst,
ptrdiff_t xpst,
ptrdiff_t ypst,
size_t spst,
int xsm,
int ysm,
Expand Down
12 changes: 6 additions & 6 deletions src/lib/OpenEXR/ImfDeepTiledInputFile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ struct TInSliceInfo
PixelType typeInFrameBuffer;
PixelType typeInFile;
char* pointerArrayBase;
size_t xStride;
size_t yStride;
ptrdiff_t xStride;
ptrdiff_t yStride;
ptrdiff_t sampleStride;
bool fill;
bool skip;
Expand All @@ -73,8 +73,8 @@ struct TInSliceInfo
PixelType typeInFrameBuffer = HALF,
char* base = NULL,
PixelType typeInFile = HALF,
size_t xStride = 0,
size_t yStride = 0,
ptrdiff_t xStride = 0,
ptrdiff_t yStride = 0,
ptrdiff_t sampleStride = 0,
bool fill = false,
bool skip = false,
Expand All @@ -87,8 +87,8 @@ TInSliceInfo::TInSliceInfo (
PixelType tifb,
char* b,
PixelType tifl,
size_t xs,
size_t ys,
ptrdiff_t xs,
ptrdiff_t ys,
ptrdiff_t spst,
bool f,
bool s,
Expand Down
12 changes: 6 additions & 6 deletions src/lib/OpenEXR/ImfDeepTiledOutputFile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,17 +71,17 @@ struct TOutSliceInfo
PixelType type;
const char* base;
size_t sampleStride;
size_t xStride;
size_t yStride;
ptrdiff_t xStride;
ptrdiff_t yStride;
bool zero;
int xTileCoords;
int yTileCoords;

TOutSliceInfo (
PixelType type = HALF,
size_t sampleStride = 0,
size_t xStride = 0,
size_t yStride = 0,
ptrdiff_t xStride = 0,
ptrdiff_t yStride = 0,
bool zero = false,
int xTileCoords = 0,
int yTileCoords = 0);
Expand All @@ -90,8 +90,8 @@ struct TOutSliceInfo
TOutSliceInfo::TOutSliceInfo (
PixelType t,
size_t spst,
size_t xStride,
size_t yStride,
ptrdiff_t xStride,
ptrdiff_t yStride,
bool z,
int xtc,
int ytc)
Expand Down
14 changes: 7 additions & 7 deletions src/lib/OpenEXR/ImfFrameBuffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ OPENEXR_IMF_INTERNAL_NAMESPACE_SOURCE_ENTER
Slice::Slice (
PixelType t,
char* b,
size_t xst,
size_t yst,
ptrdiff_t xst,
ptrdiff_t yst,
int xsm,
int ysm,
double fv,
Expand All @@ -49,8 +49,8 @@ Slice::Make (
const IMATH_NAMESPACE::V2i& origin,
int64_t w,
int64_t h,
size_t xStride,
size_t yStride,
ptrdiff_t xStride,
ptrdiff_t yStride,
int xSampling,
int ySampling,
double fillValue,
Expand All @@ -69,7 +69,7 @@ Slice::Make (
THROW (IEX_NAMESPACE::ArgExc, "Invalid pixel type.");
}
}
if (yStride == 0) yStride = static_cast<size_t> (w / xSampling) * xStride;
if (yStride == 0) yStride = (w / xSampling) * xStride;

// data window is an int, so force promote to higher type to avoid
// overflow for off y (degenerate size checks should be in
Expand Down Expand Up @@ -99,8 +99,8 @@ Slice::Make (
PixelType type,
const void* ptr,
const IMATH_NAMESPACE::Box2i& dataWindow,
size_t xStride,
size_t yStride,
ptrdiff_t xStride,
ptrdiff_t yStride,
int xSampling,
int ySampling,
double fillValue,
Expand Down
19 changes: 10 additions & 9 deletions src/lib/OpenEXR/ImfFrameBuffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include <ImathBox.h>

#include <cstdint>
#include <cstddef>
#include <map>
#include <string>

Expand Down Expand Up @@ -62,9 +63,9 @@ struct IMF_EXPORT_TYPE Slice
//
//---------------------------------------------------------------------

char* base;
size_t xStride;
size_t yStride;
char* base;
ptrdiff_t xStride;
ptrdiff_t yStride;

//--------------------------------------------
// Subsampling: pixel (x, y) is present in the
Expand Down Expand Up @@ -107,8 +108,8 @@ struct IMF_EXPORT_TYPE Slice
Slice (
PixelType type = HALF,
char* base = 0,
size_t xStride = 0,
size_t yStride = 0,
ptrdiff_t xStride = 0,
ptrdiff_t yStride = 0,
int xSampling = 1,
int ySampling = 1,
double fillValue = 0.0,
Expand All @@ -127,8 +128,8 @@ struct IMF_EXPORT_TYPE Slice
const IMATH_NAMESPACE::V2i& origin,
int64_t w,
int64_t h,
size_t xStride = 0,
size_t yStride = 0,
ptrdiff_t xStride = 0,
ptrdiff_t yStride = 0,
int xSampling = 1,
int ySampling = 1,
double fillValue = 0.0,
Expand All @@ -141,8 +142,8 @@ struct IMF_EXPORT_TYPE Slice
PixelType type,
const void* ptr,
const IMATH_NAMESPACE::Box2i& dataWindow,
size_t xStride = 0,
size_t yStride = 0,
ptrdiff_t xStride = 0,
ptrdiff_t yStride = 0,
int xSampling = 1,
int ySampling = 1,
double fillValue = 0.0,
Expand Down
Loading