api: Adjust IBA::perpixel_op function signature (#4409)

When I added perpixel_op recently, I didn't get the function signature right. It only would work for plain old function pointers, and would not work properly for "functors" or lambdas with capture. I should have used function_view which can do a light-weight wrapper of all three, so now I do. Also modified imagebufalgo_test to explicitly test all three forms to be sure they work. Also update function_view ever so slightly. --------- Signed-off-by: Larry Gritz <lg@larrygritz.com>
AcademySoftwareFoundation · Sep 14, 2024 · 37a814f · 37a814f
1 parent c62dec6
commit 37a814f
Show file tree

Hide file tree

Showing 5 changed files with 69 additions and 21 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -4,7 +4,7 @@
 
 cmake_minimum_required (VERSION 3.15)
 
-set (OpenImageIO_VERSION "2.6.5.1")
+set (OpenImageIO_VERSION "2.6.6.0")
 set (OpenImageIO_VERSION_OVERRIDE "" CACHE STRING
      "Version override (use with caution)!")
 mark_as_advanced (OpenImageIO_VERSION_OVERRIDE)

diff --git a/src/include/OpenImageIO/function_view.h b/src/include/OpenImageIO/function_view.h
@@ -77,7 +77,8 @@ template<typename Ret, typename... Params> class function_view<Ret(Params...)> {
     template<typename Callable>
     static Ret callback_fn(intptr_t callable, Params... params)
     {
-        return (*reinterpret_cast<Callable*>(callable))(params...);
+        return (*reinterpret_cast<Callable*>(callable))(
+            std::forward<Params>(params)...);
     }
 
 public:
@@ -97,7 +98,7 @@ template<typename Ret, typename... Params> class function_view<Ret(Params...)> {
 
     Ret operator()(Params... params) const
     {
-        return callback(callable, params...);
+        return callback(callable, std::forward<Params>(params)...);
     }
 
     operator bool() const { return callback; }

diff --git a/src/include/OpenImageIO/imagebufalgo_util.h b/src/include/OpenImageIO/imagebufalgo_util.h
@@ -7,6 +7,7 @@
 
 #include <functional>
 
+#include <OpenImageIO/function_view.h>
 #include <OpenImageIO/imagebufalgo.h>
 #include <OpenImageIO/parallel.h>
 
@@ -610,6 +611,8 @@ inline TypeDesc type_merge (TypeDesc a, TypeDesc b, TypeDesc c)
 #define IBA_FIX_PERCHAN_LEN_DEF(av,len)                                 \
     IBA_FIX_PERCHAN_LEN (av, len, 0.0f, av.size() ? av.back() : 0.0f);
 
+// clang-format on
+
 
 
 /// Simple image per-pixel unary operation: Given a source image `src`, return
@@ -658,22 +661,20 @@ inline TypeDesc type_merge (TypeDesc a, TypeDesc b, TypeDesc c)
 ///   version of the operation that allows specialization to any other pixel
 ///   data types
 //
-OIIO_NODISCARD OIIO_API
-ImageBuf
-perpixel_op(const ImageBuf& src, bool(*op)(span<float>, cspan<float>),
+OIIO_NODISCARD OIIO_API ImageBuf
+perpixel_op(const ImageBuf& src,
+            function_view<bool(span<float>, cspan<float>)> op,
             KWArgs options = {});
 
 /// A version of perpixel_op that performs a binary operation, taking two
 /// source images and a 3-argument `op` function that receives a destination
 /// and two source pixels.
-OIIO_NODISCARD OIIO_API
-ImageBuf
+OIIO_NODISCARD OIIO_API ImageBuf
 perpixel_op(const ImageBuf& srcA, const ImageBuf& srcB,
-            bool(*op)(span<float>, cspan<float>, cspan<float>),
+            function_view<bool(span<float>, cspan<float>, cspan<float>)> op,
             KWArgs options = {});
 
 }  // end namespace ImageBufAlgo
 
-// clang-format on
 
 OIIO_NAMESPACE_END
diff --git a/src/libOpenImageIO/imagebufalgo.cpp b/src/libOpenImageIO/imagebufalgo.cpp
@@ -629,7 +629,8 @@ ImageBufAlgo::IBAprep(ROI& roi, ImageBuf& dst, cspan<const ImageBuf*> srcs,
 
 ImageBuf
 ImageBufAlgo::perpixel_op(const ImageBuf& src,
-                          bool (*op)(span<float>, cspan<float>), KWArgs options)
+                          function_view<bool(span<float>, cspan<float>)> op,
+                          KWArgs options)
 {
     using namespace ImageBufAlgo;
     ImageBuf result;
@@ -680,9 +681,10 @@ ImageBufAlgo::perpixel_op(const ImageBuf& src,
 
 
 ImageBuf
-ImageBufAlgo::perpixel_op(const ImageBuf& srcA, const ImageBuf& srcB,
-                          bool (*op)(span<float>, cspan<float>, cspan<float>),
-                          KWArgs options)
+ImageBufAlgo::perpixel_op(
+    const ImageBuf& srcA, const ImageBuf& srcB,
+    function_view<bool(span<float>, cspan<float>, cspan<float>)> op,
+    KWArgs options)
 {
     using namespace ImageBufAlgo;
     ImageBuf result;

diff --git a/src/libOpenImageIO/imagebufalgo_test.cpp b/src/libOpenImageIO/imagebufalgo_test.cpp
@@ -1182,6 +1182,29 @@ test_yee()
 
 
 
+// Raw function to reverse channels
+bool
+chan_reverse(span<float> d, cspan<float> s)
+{
+    for (size_t c = 0, nc = size_t(d.size()); c < nc; ++c)
+        d[c] = s[nc - 1 - c];
+    return true;
+}
+
+// Functor to reverse channels
+class ChannelReverser {
+public:
+    ChannelReverser() {}
+    bool operator()(span<float> d, cspan<float> s)
+    {
+        for (size_t c = 0, nc = size_t(d.size()); c < nc; ++c)
+            d[c] = s[nc - 1 - c];
+        return true;
+    }
+};
+
+
+
 template<typename T>
 static void
 test_simple_perpixel()
@@ -1191,12 +1214,33 @@ test_simple_perpixel()
     {
         print("  unary op\n");
         ImageBuf src = filled_image({ 0.25f, 0.5f, 0.75f, 1.0f }, 4, 4, td);
-        ImageBuf result
-            = ImageBufAlgo::perpixel_op(src, [](span<float> d, cspan<float> s) {
-                  for (size_t c = 0, nc = size_t(d.size()); c < nc; ++c)
-                      d[c] = s[nc - 1 - c];
-                  return true;
-              });
+        ImageBuf result;
+        // Test with raw function pointer
+        result = ImageBufAlgo::perpixel_op(src, chan_reverse);
+        OIIO_CHECK_EQUAL(result.spec().format, td);
+        for (ImageBuf::ConstIterator<T> r(result); !r.done(); ++r) {
+            OIIO_CHECK_EQUAL(r[0], 1.0f);
+            OIIO_CHECK_EQUAL(r[1], 0.75f);
+            OIIO_CHECK_EQUAL(r[2], 0.5f);
+            OIIO_CHECK_EQUAL(r[3], 0.25f);
+        }
+        // Test with functor
+        result = ImageBufAlgo::perpixel_op(src, ChannelReverser());
+        OIIO_CHECK_EQUAL(result.spec().format, td);
+        for (ImageBuf::ConstIterator<T> r(result); !r.done(); ++r) {
+            OIIO_CHECK_EQUAL(r[0], 1.0f);
+            OIIO_CHECK_EQUAL(r[1], 0.75f);
+            OIIO_CHECK_EQUAL(r[2], 0.5f);
+            OIIO_CHECK_EQUAL(r[3], 0.25f);
+        }
+        // Test with lambda, including variable capture
+        float bias = 0.0;  // Force capture of this variable
+        result     = ImageBufAlgo::perpixel_op(src, [&](span<float> d,
+                                                    cspan<float> s) {
+            for (size_t c = 0, nc = size_t(d.size()); c < nc; ++c)
+                d[c] = s[nc - 1 - c] + bias;
+            return true;
+        });
         OIIO_CHECK_EQUAL(result.spec().format, td);
         for (ImageBuf::ConstIterator<T> r(result); !r.done(); ++r) {
             OIIO_CHECK_EQUAL(r[0], 1.0f);
@@ -1210,7 +1254,7 @@ test_simple_perpixel()
         ImageBuf srcA   = filled_image({ 0.25f, 0.5f, 0.75f, 1.0f }, 4, 4, td);
         ImageBuf srcB   = filled_image({ 1.0f, 2.0f, 3.0f, 4.0f }, 4, 4, td);
         ImageBuf result = ImageBufAlgo::perpixel_op(
-            srcA, srcB, [](span<float> d, cspan<float> a, cspan<float> b) {
+            srcA, srcB, [&](span<float> d, cspan<float> a, cspan<float> b) {
                 for (size_t c = 0, nc = size_t(d.size()); c < nc; ++c)
                     d[c] = a[c] + b[c];
                 return true;