api(IB): Add span-based constuctors to ImageBuf (#4401)

The flavor of ImageBuf that wraps caller-owned memory is constructed or
reset by merely passing a raw pointer and it assumes that the range of
memory implied by the strides is entirely safe to read from or write to.
And even if correct, there is not much within ImageBuf internals to
prevent bugs from buffer overruns.

This PR introduces a new constructor and reset() method to ImageBuf
where instead of taking a raw pointer, it takes a span describing the
extent of the safely addressible memory (and also an optional "pointer
to first pixel" within the buffer, which is necessary if you're using
negative strides or for some other reason are describing safe memory
that starts before the location of pixel 0). That span is stored in the
ImageBuf, and also it remembers if it was a cspan (which should indicate
a read-only IB) or a non-const span (which should wrap writable memory).

At this moment, I'm just adding this API and will start using and
documenting it as the perferred method, with the raw pointer versions
being regarded as unsafe for now (and maybe some day will be
deprecated?). But we're not yet using the span internally for safety
checks.

In the future, my intent is to gradually add safeguards to do bounds
checking against the span for IB operations that touch the wrapped
memory. Because we'll most likely bounds check only in debug mode, it
shouldn't ever impact performance, but it will make debugging,
sanitizer, and fuzz tests more robust to catch and zero in on buffer
overruns. I also like how it forces the caller, at the call site, to
think hard about exactly what range of memory the IB is wrapping, and
not merely pass a single pointer and hope it works out.

I also changed all ImageBuf ctrs and resets within the OIIO codebase to
use the new span-based versions.

This is part of an overall trend to migrate from raw pointer based API
calls to span-based API design as much as possible.

Signed-off-by: Larry Gritz <[email protected]>

src/doc/Doxyfile

@@ -2187,6 +2187,7 @@ PREDEFINED             = DOXYGEN_SHOULD_SKIP_THIS \
                          OIIO_NAMESPACE_END="}" \
                          OIIO_CONSTEXPR17=constexpr \
                          OIIO_CONSTEXPR20=constexpr \
+                         OIIO_IB_DEPRECATE_RAW_PTR:= \
                          OIIO_INLINE_CONSTEXPR="inline constexpr" \
                          OIIO_PURE_FUNC= \
                          OIIO_CONST_FUNC= \

src/include/OpenImageIO/fmath.h

@@ -693,9 +693,13 @@ sincos (double x, double* sine, double* cosine)
 }
 
 
-inline OIIO_HOSTDEVICE float sign (float x)
+
+/// Return -1 if x<0, 0 if x==0, 1 if x>0. For floating point types, this is
+/// not friendly to NaN inputs!
+template<typename T>
+inline OIIO_HOSTDEVICE T sign(T x)
 {
-    return x < 0.0f ? -1.0f : (x==0.0f ? 0.0f : 1.0f);
+    return x < T(0) ? T(-1) : (x == T(0) ? T(0) : T(1));
 }
 
 

src/include/OpenImageIO/imagebuf.h

@@ -20,6 +20,23 @@
 #include <memory>
 
 
+// Signal that this version of ImageBuf has constructors from spans
+#define OIIO_IMAGEBUF_SPAN_CTR 1
+
+// If before this header is included, OIIO_IMAGEBUF_DEPRECATE_RAW_PTR is
+// defined, then we will deprecate the methods taking raw pointers. This is
+// helpful for downstream apps aiming to transition to the new span-based API
+// and want to ensure they are not using the old raw pointer API.
+// #define OIIO_IMAGEBUF_DEPRECATE_RAW_PTR
+#ifdef OIIO_IMAGEBUF_DEPRECATE_RAW_PTR
+#    define OIIO_IB_DEPRECATE_RAW_PTR \
+        [[deprecated("Prefer the version that takes a span")]]
+#else
+#    define OIIO_IB_DEPRECATE_RAW_PTR
+#endif
+
+
+
 OIIO_NAMESPACE_BEGIN
 
 class ImageBuf;
@@ -224,7 +241,7 @@ class OIIO_API ImageBuf {
 
     /// Construct a writable ImageBuf that "wraps" existing pixel memory
     /// owned by the calling application. The ImageBuf does not own the
-    /// pixel storage and will will not free/delete that memory, even when
+    /// pixel storage and will not free/delete that memory, even when
     /// the ImageBuf is destroyed. Upon successful initialization, the
     /// storage will be reported as `APPBUFFER`.
     ///
@@ -235,16 +252,38 @@ class OIIO_API ImageBuf {
     ///             format), the ImageBuf will remain in an UNINITIALIZED
     ///             state.
     /// @param buffer
-    ///             A pointer to the caller-owned memory containing the
-    ///             storage for the pixels. It must be already allocated
-    ///             with enough space to hold a full image as described by
-    ///             `spec`.
+    ///             A span delineating the extent of the safely accessible
+    ///             memory comprising the pixel data.
+    /// @param buforigin
+    ///            A pointer to the first pixel of the buffer. If null, it
+    ///            will be assumed to be the beginning of the buffer. (This
+    ///            parameter is useful if any negative strides are used to
+    ///            give an unusual layout of pixels within the buffer.)
     /// @param  xstride/ystride/zstride
     ///             The distance in bytes between successive pixels,
     ///             scanlines, and image planes in the buffer (or
     ///             `AutoStride` to indicate "contiguous" data in any of
     ///             those dimensions).
     ///
+    template<typename T>
+    ImageBuf(const ImageSpec& spec, span<T> buffer, void* buforigin = nullptr,
+             stride_t xstride = AutoStride, stride_t ystride = AutoStride,
+             stride_t zstride = AutoStride)
+        : ImageBuf(spec, as_bytes(buffer), buforigin, xstride, ystride, zstride)
+    {
+    }
+    // Special base case for read-only byte spans, this one does the hard work.
+    ImageBuf(const ImageSpec& spec, cspan<std::byte> buffer,
+             void* buforigin = nullptr, stride_t xstride = AutoStride,
+             stride_t ystride = AutoStride, stride_t zstride = AutoStride);
+    // Special base case for mutable byte spans, this one does the hard work.
+    ImageBuf(const ImageSpec& spec, span<std::byte> buffer,
+             void* buforigin = nullptr, stride_t xstride = AutoStride,
+             stride_t ystride = AutoStride, stride_t zstride = AutoStride);
+
+    // Unsafe constructor of an ImageBuf that wraps an existing buffer, where
+    // only the origin pointer and the strides are given. Use with caution!
+    OIIO_IB_DEPRECATE_RAW_PTR
     ImageBuf(const ImageSpec& spec, void* buffer, stride_t xstride = AutoStride,
              stride_t ystride = AutoStride, stride_t zstride = AutoStride);
 
@@ -293,9 +332,51 @@ class OIIO_API ImageBuf {
         set_name(name);
     }
 
-    /// Destroy any previous contents of the ImageBuf and re-initialize it
-    /// as if newly constructed with the same arguments, to "wrap" existing
-    /// pixel memory owned by the calling application.
+    /// Destroy any previous contents of the ImageBuf and re-initialize it as
+    /// if newly constructed with the same arguments, to "wrap" existing pixel
+    /// memory owned by the calling application.
+    ///
+    /// @param spec
+    ///            An ImageSpec describing the image and its metadata.
+    /// @param buffer
+    ///             A span delineating the extent of the safely accessible
+    ///             memory comprising the pixel data.
+    /// @param buforigin
+    ///            A pointer to the first pixel of the buffer. If null, it
+    ///            will be assumed to be the beginning of the buffer. (This
+    ///            parameter is useful if any negative strides are used to
+    ///            give an unusual layout of pixels within the buffer.)
+    /// @param xstride/ystride/zstride
+    ///            The distance in bytes between successive pixels,
+    ///            scanlines, and image planes in the buffer (or
+    ///            `AutoStride` to indicate "contiguous" data in any of
+    ///            those dimensions).
+    template<typename T>
+    void reset(const ImageSpec& spec, span<T> buffer,
+               const void* buforigin = nullptr, stride_t xstride = AutoStride,
+               stride_t ystride = AutoStride, stride_t zstride = AutoStride)
+    {
+        // The general case for non-byte data types just converts to bytes and
+        // calls the byte version.
+        reset(spec, as_writable_bytes(buffer), buforigin, xstride, ystride,
+              zstride);
+    }
+    // Special base case for read-only byte spans, this one does the hard work.
+    void reset(const ImageSpec& spec, cspan<std::byte> buffer,
+               const void* buforigin = nullptr, stride_t xstride = AutoStride,
+               stride_t ystride = AutoStride, stride_t zstride = AutoStride);
+    // Special base case for mutable byte spans, this one does the hard work.
+    void reset(const ImageSpec& spec, span<std::byte> buffer,
+               const void* buforigin = nullptr, stride_t xstride = AutoStride,
+               stride_t ystride = AutoStride, stride_t zstride = AutoStride);
+
+    /// Unsafe reset of a "wrapped" buffer, mostly for backward compatibility.
+    /// This version does not pass a span that explicitly delineates the
+    /// memory bounds, but only passes a raw pointer and assumes that the
+    /// caller has ensured that the buffer pointed to is big enough to
+    /// accommodate accessing any valid pixel as describes by the spec and the
+    /// strides. Use with caution!
+    OIIO_IB_DEPRECATE_RAW_PTR
     void reset(const ImageSpec& spec, void* buffer,
                stride_t xstride = AutoStride, stride_t ystride = AutoStride,
                stride_t zstride = AutoStride);

src/include/OpenImageIO/imagebufalgo_opencv.h

@@ -218,9 +218,11 @@ ImageBufAlgo::to_OpenCV(cv::Mat& dst, const ImageBuf& src, ROI roi,
 
     size_t pixelsize = dstSpecFormat.size() * chans;
     size_t linestep  = pixelsize * roi.width();
+    size_t bufsize   = linestep * roi.height();
     // Make an IB that wraps the OpenCV buffer, then IBA:: copy to it
     ImageBuf cvib(ImageSpec(roi.width(), roi.height(), chans, dstSpecFormat),
-                  dst.ptr(), pixelsize, linestep, AutoStride);
+                  make_span((std::byte*)dst.ptr(), bufsize), nullptr, pixelsize,
+                  linestep);
     bool converted = ImageBufAlgo::copy(cvib, src);
     if (!converted) {
         OIIO::errorfmt(