api(IB)!: Add span based ImageBuf methods for getpixel/setpixel

Next round of making safer interfaces. Primarily, this adds new
ImageBuf::get_pixels, set_pixels, setpixel, getpixel, interppixel,
interppixel_NDC, interppixel_bucibuc, interppixel_bicubic_NDC.

The old ones took raw pointers and implied sizes -- these still exist
but are considered "unsafe" and aimed at experts and special
cases. (If a downstream project wants to be sure not to use them, they
can define symbol "OIIO_IMAGEBUF_DEPRECATE_RAW_PTR" before including
imagebuf.h to force deprecation warnings.)

The new additions instead take a span -- a combined pointer and length
that can be bounds-checked (in debug mode). I strongly encourage
people to use the new interface, as it forces the caller to think hard
about the exact region of memory that's ok to read from or write to,
and then for the OIIO side to honor that and enfoce/debug with bounds
checks against that if we add such plumbing.

Along the way, I added some interesting helper functions:

In span.h:
* `span_within()` checks that one byte span lies entirely within
  another.
* `check_span()` checks if a pointer and length lies entirely within a
  span.
* `OIIO_ALLOCA_SPAN` is a macro much like OIIO_ALLOCA, except what it
  returns is a span rather than a raw pointer to the stack-allocated
  region.
In imageio.h:
* `span_from_buffer()`/`cspan_from_bufffer()` : Given a raw pointer,
  pixel data type (TypeDesc), and image dimensions and strides,
  returns a byte stride describing the extend of memory that
  encompasses all pixels laid out by that description.

I hope the switch to spans it not very onerous for people. I really
think that this will help us write more solid and deliberate code to
begin with, and when things do go wrong, the various debug-mode checks
and assertions that spans provide should help us more quickly identify
exactly where buffer overruns and the like occurred.

Signed-off-by: Larry Gritz <lg@larrygritz.com>

Author: lgritz

CMakeLists.txt

@@ -4,7 +4,7 @@
 
 cmake_minimum_required (VERSION 3.15)
 
-set (OpenImageIO_VERSION "2.6.6.0")
+set (OpenImageIO_VERSION "2.6.7.0")
 set (OpenImageIO_VERSION_OVERRIDE "" CACHE STRING
      "Version override (use with caution)!")
 mark_as_advanced (OpenImageIO_VERSION_OVERRIDE)

src/include/OpenImageIO/imagebuf.h

@@ -721,7 +721,9 @@ class OIIO_API ImageBuf {
     /// @param x/y/z
     ///             The pixel coordinates.
     /// @param c
-    ///             The channel index to retrieve.
+    ///             The channel index to retrieve. If `c` is not in the
+    ///             valid channel range 0..nchannels-1, then `getchannel()`
+    ///             will return 0.
     /// @param wrap
     ///             WrapMode that determines the behavior if the pixel
     ///             coordinates are outside the data window: `WrapBlack`,
@@ -732,67 +734,114 @@ class OIIO_API ImageBuf {
                      WrapMode wrap = WrapBlack) const;
 
     /// Retrieve the pixel value by x, y, z pixel indices, placing its
-    /// contents in `pixel[0..n-1]` where *n* is the smaller of
-    /// `maxchannels` the actual number of channels stored in the buffer.
+    /// contents in `pixel[0..n-1]` where *n* is the smaller of the span's
+    /// size and the actual number of channels stored in the buffer.
     ///
     /// @param x/y/z
     ///             The pixel coordinates.
     /// @param pixel
-    ///             The results are stored in `pixel[0..nchannels-1]`. It is
-    ///             up to the caller to ensure that `pixel` points to enough
-    ///             memory to hold the required number of channels.
-    /// @param maxchannels
-    ///             Optional clamp to the number of channels retrieved.
+    ///             A span giving the location where results will be stored.
     /// @param wrap
     ///             WrapMode that determines the behavior if the pixel
     ///             coordinates are outside the data window: `WrapBlack`,
     ///             `WrapClamp`, `WrapPeriodic`, `WrapMirror`.
-    void getpixel(int x, int y, int z, float* pixel, int maxchannels = 1000,
+    void getpixel(int x, int y, int z, span<float> pixel,
                   WrapMode wrap = WrapBlack) const;
 
-    // Simplified version: 2D, black wrap.
+    /// Simplified version of getpixel(): 2D, black wrap.
+    void getpixel(int x, int y, span<float> pixel) const
+    {
+        getpixel(x, y, 0, pixel);
+    }
+
+    /// Unsafe version of getpixel using raw pointer. Avoid if possible.
+    OIIO_IB_DEPRECATE_RAW_PTR
+    void getpixel(int x, int y, int z, float* pixel, int maxchannels = 1000,
+                  WrapMode wrap = WrapBlack) const
+    {
+        getpixel(x, y, z, make_span(pixel, size_t(maxchannels)), wrap);
+    }
+
+    /// Unsafe version of getpixel using raw pointer. Avoid if possible.
+    OIIO_IB_DEPRECATE_RAW_PTR
     void getpixel(int x, int y, float* pixel, int maxchannels = 1000) const
     {
         getpixel(x, y, 0, pixel, maxchannels);
     }
 
     /// Sample the image plane at pixel coordinates (x,y), using linear
-    /// interpolation between pixels, placing the result in `pixel[]`.
+    /// interpolation between pixels, placing the result in `pixel[0..n-1]`
+    /// where *n* is the smaller of the span's size and the actual number of
+    /// channels stored in the buffer.
     ///
     /// @param x/y
     ///             The pixel coordinates. Note that pixel data values
     ///             themselves are at the pixel centers, so pixel (i,j) is
     ///             at image plane coordinate (i+0.5, j+0.5).
     /// @param pixel
-    ///             The results are stored in `pixel[0..nchannels-1]`. It is
-    ///             up to the caller to ensure that `pixel` points to enough
-    ///             memory to hold the number of channels in the image.
+    ///             A span giving the location where results will be stored.
     /// @param wrap
     ///             WrapMode that determines the behavior if the pixel
     ///             coordinates are outside the data window: `WrapBlack`,
     ///             `WrapClamp`, `WrapPeriodic`, `WrapMirror`.
-    void interppixel(float x, float y, float* pixel,
+    void interppixel(float x, float y, span<float> pixel,
                      WrapMode wrap = WrapBlack) const;
 
+    /// Unsafe version of interppixel using raw pointer. Avoid if possible.
+    OIIO_IB_DEPRECATE_RAW_PTR
+    void interppixel(float x, float y, float* pixel,
+                     WrapMode wrap = WrapBlack) const
+    {
+        interppixel(x, y, make_span(pixel, size_t(nchannels())), wrap);
+    }
+
     /// Linearly interpolate at NDC coordinates (s,t), where (0,0) is
     /// the upper left corner of the display window, (1,1) the lower
     /// right corner of the display window.
     ///
     /// @note `interppixel()` uses pixel coordinates (ranging 0..resolution)
     /// whereas `interppixel_NDC()` uses NDC coordinates (ranging 0..1).
-    void interppixel_NDC(float s, float t, float* pixel,
+    void interppixel_NDC(float s, float t, span<float> pixel,
                          WrapMode wrap = WrapBlack) const;
 
+    /// Unsafe version of interppixel_NDC using raw pointer. Avoid if
+    /// possible.
+    OIIO_IB_DEPRECATE_RAW_PTR
+    void interppixel_NDC(float s, float t, float* pixel,
+                         WrapMode wrap = WrapBlack) const
+    {
+        interppixel_NDC(s, t, make_span(pixel, size_t(nchannels())), wrap);
+    }
+
     /// Bicubic interpolation at pixel coordinates (x,y).
-    void interppixel_bicubic(float x, float y, float* pixel,
+    void interppixel_bicubic(float x, float y, span<float> pixel,
                              WrapMode wrap = WrapBlack) const;
 
+    /// Unsafe version of interppixel_bicubic_NDC using raw pointer.
+    /// Avoid if possible.
+    OIIO_IB_DEPRECATE_RAW_PTR
+    void interppixel_bicubic(float x, float y, float* pixel,
+                             WrapMode wrap = WrapBlack) const
+    {
+        interppixel_bicubic(x, y, make_span(pixel, size_t(nchannels())), wrap);
+    }
+
     /// Bicubic interpolation at NDC space coordinates (s,t), where (0,0)
     /// is the upper left corner of the display (a.k.a. "full") window,
     /// (1,1) the lower right corner of the display window.
-    void interppixel_bicubic_NDC(float s, float t, float* pixel,
+    void interppixel_bicubic_NDC(float s, float t, span<float> pixel,
                                  WrapMode wrap = WrapBlack) const;
 
+    /// Unsafe version of interppixel_bicubic_NDC using raw pointer.
+    /// Avoid if possible.
+    OIIO_IB_DEPRECATE_RAW_PTR
+    void interppixel_bicubic_NDC(float s, float t, float* pixel,
+                                 WrapMode wrap = WrapBlack) const
+    {
+        interppixel_bicubic_NDC(s, t, make_span(pixel, size_t(nchannels())),
+                                wrap);
+    }
+
 
     /// Set the pixel with coordinates (x,y,0) to have the values in span
     /// `pixel[]`.  The number of channels copied is the minimum of the span
@@ -805,48 +854,111 @@ class OIIO_API ImageBuf {
     /// Set the pixel with coordinates (x,y,z) to have the values in span
     /// `pixel[]`.  The number of channels copied is the minimum of the span
     /// length and the actual number of channels in the image.
-    void setpixel(int x, int y, int z, cspan<float> pixel)
-    {
-        setpixel(x, y, z, pixel.data(), int(pixel.size()));
-    }
+    void setpixel(int x, int y, int z, cspan<float> pixel);
 
     /// Set the `i`-th pixel value of the image (out of width*height*depth),
     /// from floating-point values in span `pixel[]`.  The number of
     /// channels copied is the minimum of the span length and the actual
     /// number of channels in the image.
     void setpixel(int i, cspan<float> pixel)
     {
-        setpixel(i, pixel.data(), int(pixel.size()));
+        setpixel(spec().x + (i % spec().width), spec().y + (i / spec().width),
+                 pixel);
     }
 
     /// Set the pixel with coordinates (x,y,0) to have the values in
     /// pixel[0..n-1].  The number of channels copied, n, is the minimum
     /// of maxchannels and the actual number of channels in the image.
+    OIIO_IB_DEPRECATE_RAW_PTR
     void setpixel(int x, int y, const float* pixel, int maxchannels = 1000)
     {
-        setpixel(x, y, 0, pixel, maxchannels);
+        int n = std::min(spec().nchannels, maxchannels);
+        setpixel(x, y, 0, make_cspan(pixel, size_t(n)));
     }
 
     /// Set the pixel with coordinates (x,y,z) to have the values in
     /// `pixel[0..n-1]`.  The number of channels copied, n, is the minimum
     /// of `maxchannels` and the actual number of channels in the image.
+    OIIO_IB_DEPRECATE_RAW_PTR
     void setpixel(int x, int y, int z, const float* pixel,
-                  int maxchannels = 1000);
+                  int maxchannels = 1000)
+    {
+        int n = std::min(spec().nchannels, maxchannels);
+        setpixel(x, y, z, make_cspan(pixel, size_t(n)));
+    }
 
     /// Set the `i`-th pixel value of the image (out of width*height*depth),
     /// from floating-point values in `pixel[]`.  Set at most
     /// `maxchannels` (will be clamped to the actual number of channels).
-    void setpixel(int i, const float* pixel, int maxchannels = 1000);
+    OIIO_IB_DEPRECATE_RAW_PTR
+    void setpixel(int i, const float* pixel, int maxchannels = 1000)
+    {
+        int n = std::min(spec().nchannels, maxchannels);
+        setpixel(i, make_cspan(pixel, size_t(n)));
+    }
 
     /// Retrieve the rectangle of pixels spanning the ROI (including
     /// channels) at the current subimage and MIP-map level, storing the
-    /// pixel values beginning at the address specified by result and with
-    /// the given strides (by default, AutoStride means the usual contiguous
-    /// packing of pixels) and converting into the data type described by
-    /// `format`.  It is up to the caller to ensure that result points to an
-    /// area of memory big enough to accommodate the requested rectangle.
-    /// Return true if the operation could be completed, otherwise return
-    /// false.
+    /// pixel values into the `buffer`.
+    ///
+    /// @param roi
+    ///             The region of interest to copy.
+    /// @param spec
+    ///             An ImageSpec describing the image and its metadata. If
+    ///             not enough information is given to know the "shape" of
+    ///             the image (width, height, depth, channels, and data
+    ///             format), the ImageBuf will remain in an UNINITIALIZED
+    ///             state.
+    /// @param buffer
+    ///             A span delineating the extent of the safely accessible
+    ///             memory where the results should be stored.
+    /// @param buforigin
+    ///             In the version of the template that takes this parameter,
+    ///             it is a pointer to the first pixel of the buffer. If null,
+    ///             it will be assumed to be the beginning of the buffer.
+    ///             THis 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).
+    /// @returns
+    ///             Return true if the operation could be completed,
+    ///             otherwise return false.
+    ///
+    template<typename T, OIIO_ENABLE_IF(!std::is_const_v<T>
+                                        && !std::is_same_v<T, std::byte>)>
+    bool get_pixels(ROI roi, span<T> buffer, stride_t xstride = AutoStride,
+                    stride_t ystride = AutoStride,
+                    stride_t zstride = AutoStride) const
+    {
+        return get_pixels(roi, TypeDescFromC<T>::value(),
+                          as_writable_bytes(buffer), buffer.data(), xstride,
+                          ystride, zstride);
+    }
+
+    template<typename T, OIIO_ENABLE_IF(!std::is_const_v<T>
+                                        && !std::is_same_v<T, std::byte>)>
+    bool get_pixels(ROI roi, span<T> buffer, T* buforigin,
+                    stride_t xstride = AutoStride,
+                    stride_t ystride = AutoStride,
+                    stride_t zstride = AutoStride) const
+    {
+        return get_pixels(roi, TypeDescFromC<T>::value(),
+                          as_writable_bytes(buffer), buforigin, xstride,
+                          ystride, zstride);
+    }
+
+    /// Base case of get_pixels: read into a span of generic bytes. The
+    /// requested data type is supplied by `format.
+    bool get_pixels(ROI roi, TypeDesc format, span<std::byte> buffer,
+                    void* buforigin = nullptr, stride_t xstride = AutoStride,
+                    stride_t ystride = AutoStride,
+                    stride_t zstride = AutoStride) const;
+
+    /// Potentially unsafe get_pixels() using raw pointers. Use with catution!
+    OIIO_IB_DEPRECATE_RAW_PTR
     bool get_pixels(ROI roi, TypeDesc format, void* result,
                     stride_t xstride = AutoStride,
                     stride_t ystride = AutoStride,
@@ -860,6 +972,37 @@ class OIIO_API ImageBuf {
     /// the data buffer is assumed to have the same resolution as the ImageBuf
     /// itself. Return true if the operation could be completed, otherwise
     /// return false.
+    template<typename T, OIIO_ENABLE_IF(!std::is_same_v<T, std::byte>)>
+    bool set_pixels(ROI roi, span<T> buffer, stride_t xstride = AutoStride,
+                    stride_t ystride = AutoStride,
+                    stride_t zstride = AutoStride)
+    {
+        return set_pixels(roi, TypeDescFromC<std::remove_const_t<T>>::value(),
+                          as_bytes(buffer), buffer.data(), xstride, ystride,
+                          zstride);
+    }
+
+    template<typename T, OIIO_ENABLE_IF(!std::is_same_v<T, std::byte>)>
+    bool set_pixels(ROI roi, span<T> buffer, const T* buforigin,
+                    stride_t xstride = AutoStride,
+                    stride_t ystride = AutoStride,
+                    stride_t zstride = AutoStride)
+    {
+        return set_pixels(roi, TypeDescFromC<std::remove_const_t<T>>::value(),
+                          as_bytes(buffer), buforigin, xstride, ystride,
+                          zstride);
+    }
+
+    /// Base case of get_pixels: read into a span of generic bytes. The
+    /// requested data type is supplied by `format.
+    bool set_pixels(ROI roi, TypeDesc format, cspan<std::byte> buffer,
+                    const void* buforigin = nullptr,
+                    stride_t xstride      = AutoStride,
+                    stride_t ystride      = AutoStride,
+                    stride_t zstride      = AutoStride);
+
+    /// Potentially unsafe set_pixels() using raw pointers. Use with catution!
+    OIIO_IB_DEPRECATE_RAW_PTR
     bool set_pixels(ROI roi, TypeDesc format, const void* data,
                     stride_t xstride = AutoStride,
                     stride_t ystride = AutoStride,
@@ -1125,8 +1268,8 @@ class OIIO_API ImageBuf {
     /// Error reporting for ImageBuf: call this with std::format style
     /// formatting specification. It is not necessary to have the error
     /// message contain a trailing newline.
-    template<typename... Args>
-    void errorfmt(const char* fmt, const Args&... args) const
+    template<typename Str, typename... Args>
+    void errorfmt(const Str& fmt, Args&&... args) const
     {
         error(Strutil::fmt::format(fmt, args...));
     }

src/include/OpenImageIO/imageio.h

@@ -3370,6 +3370,19 @@ OIIO_API bool copy_image (int nchannels, int width, int height, int depth,
                           void *dst, stride_t dst_xstride,
                           stride_t dst_ystride, stride_t dst_zstride);
 
+/// Helper: manufacture a span given an image pointer, format, size, and
+/// strides. Use with caution! This is making a lot of assumptions that the
+/// data pointer really does point to memory that's ok to access according to
+/// the sizes and strides you give.
+OIIO_API span<std::byte>
+span_from_buffer(void* data, TypeDesc format, int nchannels, int width,
+                 int height, int depth, stride_t xstride = AutoStride, stride_t ystride = AutoStride,
+                 stride_t zstride = AutoStride);
+OIIO_API cspan<std::byte>
+cspan_from_buffer(const void* data, TypeDesc format, int nchannels, int width,
+                 int height, int depth, stride_t xstride = AutoStride, stride_t ystride = AutoStride,
+                 stride_t zstride = AutoStride);
+
 
 // All the wrap_foo functions implement a wrap mode, wherein coord is
 // altered to be origin <= coord < origin+width.  The return value

src/include/OpenImageIO/span.h

@@ -558,6 +558,35 @@ spanzero(span<T> dst, size_t offset = 0, size_t n = size_t(-1))
 }
 
 
+
+/// Does the byte span `query` lie entirely within the safe `bounds` span?
+inline bool
+span_within(cspan<std::byte> bounds, cspan<std::byte> query)
+{
+    return query.data() >= bounds.data()
+           && query.data() + query.size() <= bounds.data() + bounds.size();
+}
+
+
+
+/// Verify the `ptr[0..len-1]` lies entirely within the given span `s`, which
+/// does not need to be the same data type.  Return true if that is the case,
+/// false if it extends beyond the safe limits fo the span.
+template<typename SpanType, typename PtrType>
+inline bool
+check_span(span<SpanType> s, const PtrType* ptr, size_t len = 1)
+{
+    return span_within(as_bytes(s), as_bytes(make_cspan(ptr, len)));
+}
+
+
+
+/// OIIO_ALLOCASPAN is used to allocate smallish amount of memory on the
+/// stack, equivalent of C99 type var_name[size], and then return a span
+/// encompassing it.
+#define OIIO_ALLOCA_SPAN(type, size) span<type>(OIIO_ALLOCA(type, size), size)
+
+
 OIIO_NAMESPACE_END
 
 

src/iv/imageviewer.h

@@ -116,7 +116,7 @@ class IvImage final : public ImageBuf {
     /// color space correction when indicated.
     void pixel_transform(bool srgb_to_linear, int color_mode, int channel);
 
-    bool get_pixels(ROI roi, TypeDesc format, void* result)
+    bool get_pixels(ROI roi, TypeDesc format, span<std::byte> result)
     {
         if (m_corrected_image.localpixels())
             return m_corrected_image.get_pixels(roi, format, result);