Skip to content

Commit 7ff7c8a

Browse files
nruschnrusch
committed
Address review notes
- Remove ST buffer datatype guard - Simplify additional ROI prep - Remove unnecessary buffer reallocation
1 parent f21a641 commit 7ff7c8a

File tree

4 files changed

+36
-68
lines changed

4 files changed

+36
-68
lines changed

‎src/include/OpenImageIO/imagebufalgo.h

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -836,7 +836,7 @@ ImageBuf OIIO_API st_warp (const ImageBuf &src, const ImageBuf& stbuf,
836836
bool flip_s=false, bool flip_t=false, ROI roi={},
837837
int nthreads=0);
838838
ImageBuf OIIO_API st_warp (const ImageBuf &src, const ImageBuf& stbuf,
839-
Filter2D *filter, int chan_s=0, int chan_t=1,
839+
const Filter2D *filter, int chan_s=0, int chan_t=1,
840840
bool flip_s=false, bool flip_t=false, ROI roi={},
841841
int nthreads=0);
842842
bool OIIO_API st_warp (ImageBuf &dst, const ImageBuf &src,
@@ -846,9 +846,9 @@ bool OIIO_API st_warp (ImageBuf &dst, const ImageBuf &src,
846846
bool flip_s=false, bool flip_t=false, ROI roi={},
847847
int nthreads=0);
848848
bool OIIO_API st_warp (ImageBuf &dst, const ImageBuf &src,
849-
const ImageBuf& stbuf, Filter2D *filter, int chan_s=0,
850-
int chan_t=1, bool flip_s=false, bool flip_t=false,
851-
ROI roi={}, int nthreads=0);
849+
const ImageBuf& stbuf, const Filter2D *filter,
850+
int chan_s=0, int chan_t=1, bool flip_s=false,
851+
bool flip_t=false, ROI roi={}, int nthreads=0);
852852
/// @}
853853

854854

‎src/libOpenImageIO/imagebufalgo_test.cpp

Lines changed: 10 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -882,31 +882,22 @@ test_validate_st_warp_checks()
882882
const int size = 16;
883883
ImageSpec srcSpec(size, size, 3, TypeDesc::FLOAT);
884884
ImageBuf SRC(srcSpec);
885-
ImageBufAlgo::zero(SRC);
886-
887-
{
888-
ImageSpec stSpec_uint8(size, size, 2, TypeDesc::UINT8);
889-
ImageBuf ST(stSpec_uint8);
890-
ImageBufAlgo::zero(ST);
891-
ImageBuf DST;
892-
// Fail: non-float ST buffer
893-
OIIO_CHECK_ASSERT(!ImageBufAlgo::st_warp(DST, SRC, ST));
894-
}
895-
896-
897885
ImageBuf ST;
898886
ImageBuf DST;
887+
888+
ImageBufAlgo::zero(SRC);
889+
899890
// Fail: Uninitialized ST buffer
900891
OIIO_CHECK_ASSERT(!ImageBufAlgo::st_warp(DST, SRC, ST));
901892

902-
ImageSpec stSpec_half(size, size, 2, TypeDesc::HALF);
903-
ST.reset(stSpec_half);
904-
905-
ImageSpec destSpecBad(size + 2, size + 2, 3, TypeDesc::FLOAT);
906-
DST.reset(destSpecBad);
907-
// Fail: Mismatch between dst and st full sizes
893+
ROI disjointROI(size, size, size * 2, size * 2, 0, 1, 0, 2);
894+
ImageSpec stSpec(disjointROI, TypeDesc::HALF);
895+
ST.reset(stSpec);
896+
// Fail: Non-intersecting ST and output ROIs
908897
OIIO_CHECK_ASSERT(!ImageBufAlgo::st_warp(DST, SRC, ST));
909898

899+
stSpec = ImageSpec(size, size, 2, TypeDesc::HALF);
900+
ST.reset(stSpec);
910901

911902
DST.reset();
912903
// Fail: Out-of-range chan_s
@@ -1033,8 +1024,8 @@ main(int argc, char** argv)
10331024
test_computePixelStats();
10341025
histogram_computation_test();
10351026
test_maketx_from_imagebuf();
1036-
test_validate_st_warp_checks();
10371027
test_IBAprep();
1028+
test_validate_st_warp_checks();
10381029
test_opencv();
10391030

10401031
benchmark_parallel_image(64, iterations * 64);

‎src/libOpenImageIO/imagebufalgo_xform.cpp

Lines changed: 20 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1159,7 +1159,7 @@ ImageBufAlgo::rotate(const ImageBuf& src, float angle, string_view filtername,
11591159
template<typename DSTTYPE, typename SRCTYPE, typename STTYPE>
11601160
static bool
11611161
st_warp_(ImageBuf& dst, const ImageBuf& src, const ImageBuf& stbuf, int chan_s,
1162-
int chan_t, bool flip_s, bool flip_t, Filter2D* filter, ROI roi,
1162+
int chan_t, bool flip_s, bool flip_t, const Filter2D* filter, ROI roi,
11631163
int nthreads)
11641164
{
11651165
OIIO_DASSERT(filter);
@@ -1199,11 +1199,8 @@ st_warp_(ImageBuf& dst, const ImageBuf& src, const ImageBuf& stbuf, int chan_s,
11991199
// the outer loop.
12001200
// XXX: Sampling of the source buffer can be entirely random, so there
12011201
// are probably some opportunities for optimization in here...
1202-
for (; !st_iter.done(); ++st_iter) {
1202+
for (; !st_iter.done(); ++st_iter, ++out_iter) {
12031203
// Look up source coordinates from ST channels.
1204-
// We don't care about faithfully maintaining `STTYPE`: Half will be
1205-
// promoted accurately, and double isn't really useful, since filter
1206-
// lookups don't support that (excessive) level of precision.
12071204
float src_s = st_iter[chan_s];
12081205
float src_t = st_iter[chan_t];
12091206

@@ -1251,7 +1248,6 @@ st_warp_(ImageBuf& dst, const ImageBuf& src, const ImageBuf& stbuf, int chan_s,
12511248
out_iter[chan] = 0;
12521249
}
12531250
}
1254-
++out_iter;
12551251
}
12561252
}); // end of parallel_image
12571253
return true;
@@ -1269,7 +1265,7 @@ check_st_warp_args(ImageBuf& dst, const ImageBuf& src, const ImageBuf& stbuf,
12691265
return false;
12701266
}
12711267

1272-
const ImageSpec& stSpec(stbuf.spec());
1268+
const ImageSpec& stSpec = stbuf.spec();
12731269
// XXX: Wanted to use `uint32_t` for channel indices, but I don't want to
12741270
// break from the rest of the API and introduce a bunch of compile warnings.
12751271
if (chan_s >= stSpec.nchannels) {
@@ -1282,44 +1278,25 @@ check_st_warp_args(ImageBuf& dst, const ImageBuf& src, const ImageBuf& stbuf,
12821278
chan_t);
12831279
return false;
12841280
}
1285-
// N.B. We currently require floating-point ST channels
1286-
if (!stSpec.format.is_floating_point()) {
1287-
dst.error("ImageBufAlgo::st_warp : ST buffer must be holding floating-"
1288-
"point data");
1289-
return false;
1290-
}
12911281

1292-
if (dst.initialized()) {
1293-
// XXX: Currently requiring the pixel scales of ST and a preallocated
1294-
// output buffer to match.
1295-
if (dst.spec().full_width != stSpec.full_width
1296-
|| dst.spec().full_height != stSpec.full_height) {
1297-
dst.error("ImageBufAlgo::st_warp : Output and ST buffers must have "
1298-
"the same full width and height");
1299-
return false;
1300-
}
1301-
}
1302-
1303-
// Prep the dest spec using the channels from `src`, and the intersection of
1304-
// `roi` and the ROI from the ST buffer (since the ST warp is only defined
1305-
// for pixels in the ST buffer). We grab a copy of the input ROI before
1306-
// `IBAprep`, since we want to intersect it ourselves.
1307-
ROI inputROI(roi);
1282+
// Prep the output buffer, and then intersect the resulting ROI with the ST
1283+
// buffer's ROI, since the ST warp is only defined for pixels in the latter.
13081284
bool res
13091285
= ImageBufAlgo::IBAprep(roi, &dst, &src,
13101286
ImageBufAlgo::IBAprep_NO_SUPPORT_VOLUME
13111287
| ImageBufAlgo::IBAprep_NO_COPY_ROI_FULL);
13121288
if (res) {
1313-
roi = roi_intersection(inputROI, stSpec.roi());
1314-
roi.chbegin = std::max(roi.chbegin, 0);
1315-
roi.chend = std::min(roi.chend, src.spec().nchannels);
1316-
1317-
ImageSpec destSpec(dst.spec());
1318-
destSpec.set_roi(roi);
1319-
destSpec.set_roi_full(stSpec.roi_full());
1320-
// XXX: It would be nice to be able to tell `IBAprep` to skip the buffer
1321-
// initialization. Worth adding a new prep flag?
1322-
dst.reset(destSpec);
1289+
const int chbegin = roi.chbegin;
1290+
const int chend = roi.chend;
1291+
roi = roi_intersection(roi, stSpec.roi());
1292+
if (roi.npixels() <= 0) {
1293+
dst.errorfmt("ImageBufAlgo::st_warp : Output ROI does not "
1294+
"intersect ST buffer.");
1295+
return false;
1296+
}
1297+
// Make sure to preserve the channel range determined by `IBAprep`.
1298+
roi.chbegin = chbegin;
1299+
roi.chend = chend;
13231300
}
13241301
return res;
13251302
}
@@ -1328,8 +1305,8 @@ check_st_warp_args(ImageBuf& dst, const ImageBuf& src, const ImageBuf& stbuf,
13281305

13291306
bool
13301307
ImageBufAlgo::st_warp(ImageBuf& dst, const ImageBuf& src, const ImageBuf& stbuf,
1331-
Filter2D* filter, int chan_s, int chan_t, bool flip_s,
1332-
bool flip_t, ROI roi, int nthreads)
1308+
const Filter2D* filter, int chan_s, int chan_t,
1309+
bool flip_s, bool flip_t, ROI roi, int nthreads)
13331310
{
13341311
pvt::LoggedTimer logtime("IBA::st_warp");
13351312

@@ -1376,8 +1353,8 @@ ImageBufAlgo::st_warp(ImageBuf& dst, const ImageBuf& src, const ImageBuf& stbuf,
13761353

13771354
ImageBuf
13781355
ImageBufAlgo::st_warp(const ImageBuf& src, const ImageBuf& stbuf,
1379-
Filter2D* filter, int chan_s, int chan_t, bool flip_s,
1380-
bool flip_t, ROI roi, int nthreads)
1356+
const Filter2D* filter, int chan_s, int chan_t,
1357+
bool flip_s, bool flip_t, ROI roi, int nthreads)
13811358
{
13821359
ImageBuf result;
13831360
bool ok = st_warp(result, src, stbuf, filter, chan_s, chan_t, flip_s,

‎src/oiiotool/oiiotool.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3679,8 +3679,8 @@ OIIOTOOL_OP(st_warp, 2, [](OiiotoolOp& op, span<ImageBuf*> img) {
36793679
std::string filtername = op.options()["filter"];
36803680
int chan_s = op.options().get_int("chan_s");
36813681
int chan_t = op.options().get_int("chan_t", 1);
3682-
bool flip_s = op.options().get_int("flip_s");
3683-
bool flip_t = op.options().get_int("flip_t");
3682+
bool flip_s = static_cast<bool>(op.options().get_int("flip_s"));
3683+
bool flip_t = static_cast<bool>(op.options().get_int("flip_t"));
36843684
return ImageBufAlgo::st_warp(*img[0], *img[1], *img[2], filtername, 0.0f,
36853685
chan_s, chan_t, flip_s, flip_t);
36863686
});

0 commit comments

Comments
 (0)