fix: incorrect test for when it's safe to use hwy code path (#5138)

We didn't have a complete enough test for when it was safe to hwy on a
buffer. Encapsulate in new function HwySupports so that all the logic
lives in one place and doesn't have to be replicated everywhere. The
more complete test now checks for:

- contiguous scanlines
- right number of channels (only indirectly checked)
- 2D image, not 3D volume (not previously checked)
- pixel data window fully contains the ROI (not previously checked)
- correct data type (should always be true, but was not checked)

This eliminates some test failures, which were specifically cases
where the ROI being operated on did not overlap with the data area of
the ImageBufs, and the Highway related code did not handle those in
the way that the old ImageBuf::Iterator transparently handled running
off the edge and various wrap modes.

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

Author: lgritz

src/libOpenImageIO/imagebufalgo_addsub.cpp

@@ -138,39 +138,29 @@ static bool
 add_impl(ImageBuf& R, const ImageBuf& A, const ImageBuf& B, ROI roi,
          int nthreads)
 {
-#if defined(OIIO_USE_HWY) && OIIO_USE_HWY
-    if (OIIO::pvt::enable_hwy && R.localpixels() && A.localpixels()
-        && B.localpixels()) {
-        auto Rv             = HwyPixels(R);
-        auto Av             = HwyPixels(A);
-        auto Bv             = HwyPixels(B);
-        const int nchannels = roi.nchannels();
-        const bool contig   = ChannelsContiguous<Rtype>(Rv, nchannels)
-                            && ChannelsContiguous<Atype>(Av, nchannels)
-                            && ChannelsContiguous<Btype>(Bv, nchannels);
-        if (contig) {
-            // Use native integer path for scale-invariant add when all types
-            // match and are integer types (much faster: 6-12x vs 3-5x with
-            // float conversion).
-            constexpr bool all_same = std::is_same_v<Rtype, Atype>
-                                      && std::is_same_v<Atype, Btype>;
-            constexpr bool is_integer = std::is_integral_v<Rtype>;
-            if constexpr (all_same && is_integer)
-                return add_impl_hwy_native_int<Rtype>(R, A, B, roi, nthreads);
-            return add_impl_hwy<Rtype, Atype, Btype>(R, A, B, roi, nthreads);
-        }
-
+#if OIIO_USE_HWY
+    // First case: hwy enabled, all images have local pixels and the
+    // number of channels in the ROI. and fully encompass the ROI.
+    if (OIIO::pvt::enable_hwy && HwySupports<Rtype>(R, roi)
+        && HwySupports<Atype>(A, roi) && HwySupports<Btype>(B, roi)) {
+        // Use native integer path for scale-invariant add when all types
+        // match and are integer types (much faster: 6-12x vs 3-5x with
+        // float conversion).
+        constexpr bool all_same = std::is_same_v<Rtype, Atype>
+                                  && std::is_same_v<Atype, Btype>;
+        constexpr bool is_integer = std::is_integral_v<Rtype>;
+        if constexpr (all_same && is_integer)
+            return add_impl_hwy_native_int<Rtype>(R, A, B, roi, nthreads);
+        return add_impl_hwy<Rtype, Atype, Btype>(R, A, B, roi, nthreads);
+    }
+    // Second case: the buffers are RGBA but we are only adding RGB
+    // (preserving alpha).
+    // Is this a case we will actually encounter?
+    if (OIIO::pvt::enable_hwy && HwySupports<Rtype>(R, roi, 4)
+        && HwySupports<Atype>(A, roi, 4) && HwySupports<Btype>(B, roi, 4)
+        && (roi.chbegin == 0 && roi.chend == 3)) {
         // Handle the common RGBA + RGB ROI strided case (preserving alpha).
-        if (roi.chbegin == 0 && roi.chend == 3) {
-            const bool contig4 = (Rv.nchannels >= 4 && Av.nchannels >= 4
-                                  && Bv.nchannels >= 4)
-                                 && ChannelsContiguous<Rtype>(Rv, 4)
-                                 && ChannelsContiguous<Atype>(Av, 4)
-                                 && ChannelsContiguous<Btype>(Bv, 4);
-            if (contig4)
-                return add_impl_hwy<Rtype, Atype, Btype>(R, A, B, roi,
-                                                         nthreads);
-        }
+        return add_impl_hwy<Rtype, Atype, Btype>(R, A, B, roi, nthreads);
     }
 #endif
     return add_impl_scalar<Rtype, Atype, Btype>(R, A, B, roi, nthreads);
@@ -181,7 +171,7 @@ static bool
 add_impl(ImageBuf& R, const ImageBuf& A, cspan<float> b, ROI roi, int nthreads)
 {
 #if OIIO_USE_HWY
-    if (OIIO::pvt::enable_hwy && R.localpixels() && A.localpixels())
+    if (OIIO::pvt::enable_hwy && HwySupports<Rtype>(R, roi))
         return add_impl_hwy<Rtype, Atype>(R, A, b, roi, nthreads);
 #endif
     return add_impl_scalar<Rtype, Atype>(R, A, b, roi, nthreads);
@@ -238,39 +228,29 @@ static bool
 sub_impl(ImageBuf& R, const ImageBuf& A, const ImageBuf& B, ROI roi,
          int nthreads)
 {
-#if defined(OIIO_USE_HWY) && OIIO_USE_HWY
-    if (OIIO::pvt::enable_hwy && R.localpixels() && A.localpixels()
-        && B.localpixels()) {
-        auto Rv             = HwyPixels(R);
-        auto Av             = HwyPixels(A);
-        auto Bv             = HwyPixels(B);
-        const int nchannels = roi.nchannels();
-        const bool contig   = ChannelsContiguous<Rtype>(Rv, nchannels)
-                            && ChannelsContiguous<Atype>(Av, nchannels)
-                            && ChannelsContiguous<Btype>(Bv, nchannels);
-        if (contig) {
-            // Use native integer path for scale-invariant sub when all types
-            // match and are integer types (much faster: 6-12x vs 3-5x with
-            // float conversion).
-            constexpr bool all_same = std::is_same_v<Rtype, Atype>
-                                      && std::is_same_v<Atype, Btype>;
-            constexpr bool is_integer = std::is_integral_v<Rtype>;
-            if constexpr (all_same && is_integer)
-                return sub_impl_hwy_native_int<Rtype>(R, A, B, roi, nthreads);
-            return sub_impl_hwy<Rtype, Atype, Btype>(R, A, B, roi, nthreads);
-        }
-
+#if OIIO_USE_HWY
+    // First case: hwy enabled, all images have local pixels and the
+    // number of channels in the ROI. and fully encompass the ROI.
+    if (OIIO::pvt::enable_hwy && HwySupports<Rtype>(R, roi)
+        && HwySupports<Atype>(A, roi) && HwySupports<Btype>(B, roi)) {
+        // Use native integer path for scale-invariant sub when all types
+        // match and are integer types (much faster: 6-12x vs 3-5x with
+        // float conversion).
+        constexpr bool all_same = std::is_same_v<Rtype, Atype>
+                                  && std::is_same_v<Atype, Btype>;
+        constexpr bool is_integer = std::is_integral_v<Rtype>;
+        if constexpr (all_same && is_integer)
+            return sub_impl_hwy_native_int<Rtype>(R, A, B, roi, nthreads);
+        return sub_impl_hwy<Rtype, Atype, Btype>(R, A, B, roi, nthreads);
+    }
+    // Second case: the buffers are RGBA but we are only subtracting RGB
+    // (preserving alpha).
+    // Is this a case we will actually encounter?
+    if (OIIO::pvt::enable_hwy && HwySupports<Rtype>(R, roi, 4)
+        && HwySupports<Atype>(A, roi, 4) && HwySupports<Btype>(B, roi, 4)
+        && (roi.chbegin == 0 && roi.chend == 3)) {
         // Handle the common RGBA + RGB ROI strided case (preserving alpha).
-        if (roi.chbegin == 0 && roi.chend == 3) {
-            const bool contig4 = (Rv.nchannels >= 4 && Av.nchannels >= 4
-                                  && Bv.nchannels >= 4)
-                                 && ChannelsContiguous<Rtype>(Rv, 4)
-                                 && ChannelsContiguous<Atype>(Av, 4)
-                                 && ChannelsContiguous<Btype>(Bv, 4);
-            if (contig4)
-                return sub_impl_hwy<Rtype, Atype, Btype>(R, A, B, roi,
-                                                         nthreads);
-        }
+        return sub_impl_hwy<Rtype, Atype, Btype>(R, A, B, roi, nthreads);
     }
 #endif
     return sub_impl_scalar<Rtype, Atype, Btype>(R, A, B, roi, nthreads);

src/libOpenImageIO/imagebufalgo_hwy_pvt.h

@@ -87,6 +87,35 @@ RoiRowPtr(const HwyLocalPixelsView<ByteT>& v, int y, const ROI& roi) noexcept
     return ChannelPtr<T>(v, roi.xbegin, y, roi.chbegin);
 }
 
+
+/// Can we use Hwy acceleration with this ImageBuf, over the specified ROI? If
+/// not supplied, the ROI defaults to the entire data window of the ImageBuf,
+/// and the number of channels defaults to the number of channels in the ROI.
+template<typename T>
+inline bool
+HwySupports(const ImageBuf& A, const ROI& roi = ROI(), int nchannels = 0)
+{
+    if (nchannels <= 0)
+        nchannels = roi.defined() ? roi.nchannels() : A.nchannels();
+    return
+        // The data type must match what we're looking for (T)
+        (A.spec().format.basetype == BaseTypeFromC<T>::value)
+
+        // The ImageBuf has the number of pixels we're expecting
+        && A.spec().nchannels == nchannels
+
+        // The scanlines must consist of contiguous pixels in memory (no
+        // padding between channels or pixels within a scanline). Note that
+        // this test implicitly also ensures "local" in-memory pixels.
+        && A.contiguous_scanline()
+        // For now, we only support 2D images (no volumes)
+        && A.spec().depth == 1
+        // The data window must fully encompass the ROI we're operating on
+        // (if an ROI was supplied)
+        && (!roi.defined() || A.roi().contains(roi));
+}
+
+
 // -----------------------------------------------------------------------
 // Type Traits
 // -----------------------------------------------------------------------

src/libOpenImageIO/imagebufalgo_mad.cpp

@@ -71,32 +71,21 @@ mad_impl(ImageBuf& R, const ImageBuf& A, const ImageBuf& B, const ImageBuf& C,
          ROI roi, int nthreads)
 {
 #if OIIO_USE_HWY
-    if (OIIO::pvt::enable_hwy && R.localpixels() && A.localpixels()
-        && B.localpixels() && C.localpixels()) {
-        auto Rv             = HwyPixels(R);
-        auto Av             = HwyPixels(A);
-        auto Bv             = HwyPixels(B);
-        auto Cv             = HwyPixels(C);
-        const int nchannels = roi.nchannels();
-        const bool contig   = ChannelsContiguous<Rtype>(Rv, nchannels)
-                            && ChannelsContiguous<ABCtype>(Av, nchannels)
-                            && ChannelsContiguous<ABCtype>(Bv, nchannels)
-                            && ChannelsContiguous<ABCtype>(Cv, nchannels);
-        if (contig)
-            return mad_impl_hwy<Rtype, ABCtype>(R, A, B, C, roi, nthreads);
-
+    if (OIIO::pvt::enable_hwy && HwySupports<Rtype>(R, roi)
+        && HwySupports<ABCtype>(A, roi) && HwySupports<ABCtype>(B, roi)
+        && HwySupports<ABCtype>(C, roi)) {
+        // All-channel case
+        return mad_impl_hwy<Rtype, ABCtype>(R, A, B, C, roi, nthreads);
+    }
+#    if 0
+    if (OIIO::pvt::enable_hwy && HwySupports<Rtype>(R, roi, 4)
+        && HwySupports<Atype>(A, roi, 4) && HwySupports<Btype>(B, roi, 4)
+        && HwySupports<Btype>(C, roi, 4)
+        && (roi.chbegin == 0 && roi.chend == 3)) {
         // Handle the common RGBA + RGB ROI strided case (preserving alpha).
-        if (roi.chbegin == 0 && roi.chend == 3) {
-            const bool contig4 = (Rv.nchannels >= 4 && Av.nchannels >= 4
-                                  && Bv.nchannels >= 4 && Cv.nchannels >= 4)
-                                 && ChannelsContiguous<Rtype>(Rv, 4)
-                                 && ChannelsContiguous<ABCtype>(Av, 4)
-                                 && ChannelsContiguous<ABCtype>(Bv, 4)
-                                 && ChannelsContiguous<ABCtype>(Cv, 4);
-            if (contig4)
-                return mad_impl_hwy<Rtype, ABCtype>(R, A, B, C, roi, nthreads);
-        }
+        return mad_impl_hwy<Rtype, ABCtype>(R, A, B, C, roi, nthreads);
     }
+#    endif
 #endif
     return mad_impl_scalar<Rtype, ABCtype>(R, A, B, C, roi, nthreads);
 }

src/libOpenImageIO/imagebufalgo_muldiv.cpp

@@ -175,30 +175,19 @@ static bool
 mul_impl(ImageBuf& R, const ImageBuf& A, const ImageBuf& B, ROI roi,
          int nthreads)
 {
-#if defined(OIIO_USE_HWY) && OIIO_USE_HWY
-    if (OIIO::pvt::enable_hwy && R.localpixels() && A.localpixels()
-        && B.localpixels()) {
-        auto Rv             = HwyPixels(R);
-        auto Av             = HwyPixels(A);
-        auto Bv             = HwyPixels(B);
-        const int nchannels = roi.nchannels();
-        const bool contig   = ChannelsContiguous<Rtype>(Rv, nchannels)
-                            && ChannelsContiguous<Atype>(Av, nchannels)
-                            && ChannelsContiguous<Btype>(Bv, nchannels);
-        if (contig)
-            return mul_impl_hwy<Rtype, Atype, Btype>(R, A, B, roi, nthreads);
-
+#if OIIO_USE_HWY
+    if (OIIO::pvt::enable_hwy && HwySupports<Rtype>(R, roi)
+        && HwySupports<Atype>(A, roi) && HwySupports<Btype>(B, roi)) {
+        return mul_impl_hwy<Rtype, Atype, Btype>(R, A, B, roi, nthreads);
+    }
+    // Second case: the buffers are RGBA but we are only multiplying RGB
+    // (preserving alpha).
+    // Is this a case we will actually encounter?
+    if (OIIO::pvt::enable_hwy && HwySupports<Rtype>(R, roi, 4)
+        && HwySupports<Atype>(A, roi, 4) && HwySupports<Btype>(B, roi, 4)
+        && (roi.chbegin == 0 && roi.chend == 3)) {
         // Handle the common RGBA + RGB ROI strided case (preserving alpha).
-        if (roi.chbegin == 0 && roi.chend == 3) {
-            const bool contig4 = (Rv.nchannels >= 4 && Av.nchannels >= 4
-                                  && Bv.nchannels >= 4)
-                                 && ChannelsContiguous<Rtype>(Rv, 4)
-                                 && ChannelsContiguous<Atype>(Av, 4)
-                                 && ChannelsContiguous<Btype>(Bv, 4);
-            if (contig4)
-                return mul_impl_hwy<Rtype, Atype, Btype>(R, A, B, roi,
-                                                         nthreads);
-        }
+        return mul_impl_hwy<Rtype, Atype, Btype>(R, A, B, roi, nthreads);
     }
 #endif
     return mul_impl_scalar<Rtype, Atype, Btype>(R, A, B, roi, nthreads);
@@ -343,30 +332,17 @@ static bool
 div_impl(ImageBuf& R, const ImageBuf& A, const ImageBuf& B, ROI roi,
          int nthreads)
 {
-#if defined(OIIO_USE_HWY) && OIIO_USE_HWY
-    if (OIIO::pvt::enable_hwy && R.localpixels() && A.localpixels()
-        && B.localpixels()) {
-        auto Rv             = HwyPixels(R);
-        auto Av             = HwyPixels(A);
-        auto Bv             = HwyPixels(B);
-        const int nchannels = roi.nchannels();
-        const bool contig   = ChannelsContiguous<Rtype>(Rv, nchannels)
-                            && ChannelsContiguous<Atype>(Av, nchannels)
-                            && ChannelsContiguous<Btype>(Bv, nchannels);
-        if (contig)
-            return div_impl_hwy<Rtype, Atype, Btype>(R, A, B, roi, nthreads);
-
+#if OIIO_USE_HWY
+    if (OIIO::pvt::enable_hwy && HwySupports<Rtype>(R, roi)
+        && HwySupports<Atype>(A, roi) && HwySupports<Btype>(B, roi)) {
+        return div_impl_hwy<Rtype, Atype, Btype>(R, A, B, roi, nthreads);
+    }
+    // Handle the common RGBA + RGB ROI strided case (preserving alpha).
+    if (OIIO::pvt::enable_hwy && HwySupports<Rtype>(R, roi, 4)
+        && HwySupports<Atype>(A, roi, 4) && HwySupports<Btype>(B, roi, 4)
+        && (roi.chbegin == 0 && roi.chend == 3)) {
         // Handle the common RGBA + RGB ROI strided case (preserving alpha).
-        if (roi.chbegin == 0 && roi.chend == 3) {
-            const bool contig4 = (Rv.nchannels >= 4 && Av.nchannels >= 4
-                                  && Bv.nchannels >= 4)
-                                 && ChannelsContiguous<Rtype>(Rv, 4)
-                                 && ChannelsContiguous<Atype>(Av, 4)
-                                 && ChannelsContiguous<Btype>(Bv, 4);
-            if (contig4)
-                return div_impl_hwy<Rtype, Atype, Btype>(R, A, B, roi,
-                                                         nthreads);
-        }
+        return div_impl_hwy<Rtype, Atype, Btype>(R, A, B, roi, nthreads);
     }
 #endif
     return div_impl_scalar<Rtype, Atype, Btype>(R, A, B, roi, nthreads);

src/libOpenImageIO/imagebufalgo_xform.cpp

@@ -1435,7 +1435,8 @@ resample_(ImageBuf& dst, const ImageBuf& src, bool interpolate, ROI roi,
           int nthreads)
 {
 #if OIIO_USE_HWY
-    if (OIIO::pvt::enable_hwy && dst.localpixels() && src.localpixels())
+    if (OIIO::pvt::enable_hwy && HwySupports<DSTTYPE>(dst, roi)
+        && HwySupports<SRCTYPE>(src, ROI()))
         return resample_hwy<DSTTYPE, SRCTYPE>(dst, src, interpolate, roi,
                                               nthreads);
 #endif