feat(ImageBuf): make IB::Iterator lazy in its making the image writab…

…le (#4033)

Historically, ImageBuf has provided Iterator for writing to mutable IB's
and ConstIterator for reading into IBs. If you initialized an Iterator
to an IB that was ImageCache-backed (and therefore by definition not
mutable), it would convert the IB to the mutable kind (basically by
allocating the memory and reading the file through the IC, thus freeing
the IB of its dependence on the cache.

An important bug was fixed by PR 3997, which kinda proved that people
were not depending on this behavior, since it would tend to fail. But it
also got me thinking about to what degree the whole Iterator vs
ConstIterator was really necessary, or did it just make things harder
for users.

In this proposed patch, we change the Iterator behavior so that instead
of immediately making the IB writable as soon as the Iterator was
instantiated and associated with the IB, we instead treat it as a
ConstIterator UNTIL it actually tries to write a value, at which point
we ensure that the IB is writable.

The bottom line is that this seems to work. If you don't want to have to
think about Iterator vs ConstIterator, you don't have to. Iterator is
fine. If you don't actually write into any of the pixels, it behaves
just like a ConstIterator, including being perfectly happy with an
IC-backed IB. Performance metrics indicate that there is NO penalty for
doing so -- if you are only reading pixel values, traversing an IB with
Iterator is the same speed as doing it with ConstIterator.

I had one sleepless night after implementing this when suddenly I
realized that this was totally not thread-safe, that somehow multiple
iterators traversing the same IB would screw each other up if one of
them wrote to a pixel and converted the image. So I put in a test to
have many threads concurrently traversing the same IB, some with
ConstIterator, some with Iterator but only reading, and some with
Iterator and writing. I have not been able to trigger any failures or
crashes. It seems fine. (I did use the mutex a little more aggressively
than before.)

Why is this not as dangerous as I thought? Mainly because once an
iterator (of either variety) thinks it's dealing with an IC-backed
ImageBuf, basically, it will just happily keep using the IC all on its
own, even if the IB that it's supposedly traversing has, because another
thread's iterator has written to a pixel, "localized" the image and
severed the IC dependence. Then if it also needs to write, it will
(safely, because of a mutex) see that the image is already
localized/writable, and start using that representation.

So herein lies a very important caveat about using IB iterators: If you
have multiple iterators traversing the same IB and any of them are
*writing* to the image, the iterators may not see a globally temporally
consistent version of the image. Put in plain English: iterators that
are strictly reading may see an IC-backed disk image as it was on disk,
and not as it has since been modified by a different active iterator. So
if you are modifying an image in place, beware of using multiple
iterators, because a reading iterator may or may not see the changes
that a writing iterator made to a pixel value.

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

src/include/OpenImageIO/imagebuf.h

@@ -1176,6 +1176,13 @@ class OIIO_API ImageBuf {
 
     /// @}
 
+    /// @{
+    /// @name  Locking the internal mutex
+
+    void lock() const;
+    void unlock() const;
+    /// @}
+
     /// Return the `WrapMode` corresponding to the name (`"default"`,
     /// `"black"`, `"clamp"`, `"periodic"`, `"mirror"`). For an unknown
     /// name, this will return `WrapDefault`.
@@ -1301,7 +1308,7 @@ class OIIO_API ImageBuf {
         ROI range() const
         {
             return ROI(m_rng_xbegin, m_rng_xend, m_rng_ybegin, m_rng_yend,
-                       m_rng_zbegin, m_rng_zend, 0, m_ib->nchannels());
+                       m_rng_zbegin, m_rng_zend, 0, m_nchannels);
         }
 
         /// Reset the iteration range for this iterator and reposition to
@@ -1401,6 +1408,15 @@ class OIIO_API ImageBuf {
         // elsewhere to prevent imagebuf.h needing to know anything more
         // about ImageCache.
         void OIIO_API release_tile();
+
+        // Check if the IB is writable, make it so if it isn't.
+        OIIO_FORCEINLINE void ensure_writable()
+        {
+            if (OIIO_UNLIKELY(m_ib->storage() == IMAGECACHE))
+                make_writable();
+        }
+        // Do the dirty work of making the IB writable.
+        void OIIO_API make_writable();
     };
 
     /// Templated class for referring to an individual pixel in an
@@ -1409,7 +1425,7 @@ class OIIO_API ImageBuf {
     /// [xbegin..xend) X [ybegin..yend).  It is templated on BUFT, the
     /// type known to be in the internal representation of the ImageBuf,
     /// and USERT, the type that the user wants to retrieve or set the
-    /// data (defaulting to float).  the whole idea is to allow this:
+    /// data (defaulting to float). The whole idea is to allow this:
     /// \code
     ///   ImageBuf img (...);
     ///   ImageBuf::Iterator<float> pixel (img, 0, 512, 0, 512);
@@ -1459,36 +1475,59 @@ class OIIO_API ImageBuf {
 
         ~Iterator() {}
 
+    private:
+        // Private helper struct that encapsulates an Interator& and an index,
+        // awaiting a later read or write (which will call the iterator's
+        // get() or set(), respectively).
+        struct IteratorValRef {
+            Iterator& it;
+            int index;
+            IteratorValRef(Iterator& it, int index)
+                : it(it)
+                , index(index)
+            {
+            }
+            operator USERT() const { return it.get(index); }
+            void operator=(USERT val) { it.set(index, val); }
+        };
+
+    public:
         /// Dereferencing the iterator gives us a proxy for the pixel,
         /// which we can index for reading or assignment.
         DataArrayProxy<BUFT, USERT>& operator*()
         {
+            ensure_writable();
             return *(DataArrayProxy<BUFT, USERT>*)(void*)&m_proxydata;
         }
 
-        /// Array indexing retrieves the value of the i-th channel of
-        /// the current pixel.
-        USERT operator[](int i) const
+        /// Retrieve the value of channel i at the current iterator.
+        USERT get(int i) const
         {
-            DataArrayProxy<BUFT, USERT> proxy((BUFT*)m_proxydata);
+            ConstDataArrayProxy<BUFT, USERT> proxy((const BUFT*)m_proxydata);
             return proxy[i];
         }
 
-        /// Array referencing retrieve a proxy (which may be "assigned
-        /// to") of i-th channel of the current pixel, so that this
-        /// works: me[i] = val;
-        DataProxy<BUFT, USERT> operator[](int i)
+        /// Set the value of channel i at the current iterator. If the buffer
+        /// is not writable (for example, it is backed by an ImageCache), it
+        /// will be made writable by copying into a henceforth-local buffer.
+        void set(int i, USERT val)
         {
+            ensure_writable();
             DataArrayProxy<BUFT, USERT> proxy((BUFT*)m_proxydata);
-            return proxy[i];
+            proxy[i] = val;
         }
 
+        /// Array indexing retrieves the value of the i-th channel of
+        /// the current pixel.
+        IteratorValRef operator[](int i) { return IteratorValRef(*this, i); }
+
         void* rawptr() const { return m_proxydata; }
 
         /// Set the number of deep data samples at this pixel. (Only use
         /// this if deep_alloc() has not yet been called on the buffer.)
         void set_deep_samples(int n)
         {
+            ensure_writable();
             return const_cast<ImageBuf*>(m_ib)->set_deep_samples(m_x, m_y, m_z,
                                                                  n);
         }
@@ -1497,11 +1536,13 @@ class OIIO_API ImageBuf {
         /// if deep_alloc() has been called.)
         void set_deep_value(int c, int s, float value)
         {
+            ensure_writable();
             return const_cast<ImageBuf*>(m_ib)->set_deep_value(m_x, m_y, m_z, c,
                                                                s, value);
         }
         void set_deep_value(int c, int s, uint32_t value)
         {
+            ensure_writable();
             return const_cast<ImageBuf*>(m_ib)->set_deep_value(m_x, m_y, m_z, c,
                                                                s, value);
         }

src/libOpenImageIO/imagebuf.cpp

@@ -330,6 +330,9 @@ class ImageBufImpl {
         return m_nativespec.format;
     }
 
+    void lock() const { m_mutex.lock(); }
+    void unlock() const { m_mutex.unlock(); }
+
     const ImageBufImpl operator=(const ImageBufImpl& src);  // unimplemented
     friend class ImageBuf;
 };
@@ -3101,19 +3104,33 @@ ImageBuf::IteratorBase::range_is_image()
 
 
 
+void
+ImageBuf::IteratorBase::make_writable()
+{
+    std::lock_guard<const ImageBuf> lock(*m_ib);
+    if (m_ib->storage() != IMAGECACHE)
+        return;  // already done
+    const_cast<ImageBuf*>(m_ib)->make_writable(true);
+    OIIO_DASSERT(m_ib->storage() != IMAGECACHE);
+    if (m_tile)
+        release_tile();
+    m_tile        = nullptr;
+    m_proxydata   = nullptr;
+    m_localpixels = !m_deep;
+    pos(m_x, m_y, m_z);
+}
+
+
+
 void
 ImageBuf::IteratorBase::init_ib(WrapMode wrap, bool write)
 {
+    ImageBufImpl::lock_t lock(m_ib->m_impl->m_mutex);
     const ImageSpec& spec(m_ib->spec());
     m_deep        = spec.deep;
     m_localpixels = (m_ib->localpixels() != nullptr);
-    if (!m_localpixels && write) {
-        const_cast<ImageBuf*>(m_ib)->make_writable(true);
-        OIIO_DASSERT(m_ib->storage() != IMAGECACHE);
-        m_tile        = nullptr;
-        m_proxydata   = nullptr;
-        m_localpixels = !m_deep;
-    }
+    // if (write)
+    //      ensure_writable();  // Not here; do it lazily
     m_img_xbegin   = spec.x;
     m_img_xend     = spec.x + spec.width;
     m_img_ybegin   = spec.y;
@@ -3235,4 +3252,19 @@ ImageBuf::IteratorBase::rerange(int xbegin, int xend, int ybegin, int yend,
 }
 
 
+
+void
+ImageBuf::lock() const
+{
+    m_impl->lock();
+}
+
+
+void
+ImageBuf::unlock() const
+{
+    m_impl->unlock();
+}
+
+
 OIIO_NAMESPACE_END

src/libOpenImageIO/imagebuf_test.cpp

@@ -9,6 +9,7 @@
 #include <OpenImageIO/imagebufalgo.h>
 #include <OpenImageIO/imagecache.h>
 #include <OpenImageIO/imageio.h>
+#include <OpenImageIO/parallel.h>
 #include <OpenImageIO/unittest.h>
 
 #include <iostream>
@@ -522,17 +523,38 @@ test_mutable_iterator_with_imagecache()
     src.write(srcfilename);
 
     ImageBuf buf(srcfilename, 0, 0, ImageCache::create());
-    // Using the cache, it should look tiled
+    // Using the cache, it should look tiled and using the IC
     OIIO_CHECK_EQUAL(buf.spec().tile_width, buf.spec().width);
+    OIIO_CHECK_EQUAL(buf.storage(), ImageBuf::IMAGECACHE);
 
-    // Make a mutable iterator, even though it's an image file reference.
-    // Merely establishing the iterator ought to read the file and make the
-    // buffer writeable.
-    ImageBuf::Iterator<float> it(buf);
-    OIIO_CHECK_EQUAL(buf.spec().tile_width, 0);  // should look untiled
-    OIIO_CHECK_ASSERT(buf.localpixels());        // should look local
-    for (; !it.done(); ++it)
+    // Iterate with a ConstIterator, make sure it's still IC backed
+    for (ImageBuf::ConstIterator<float> it(buf); !it.done(); ++it) {
+        OIIO_CHECK_EQUAL(it[0], 0.5f);
+    }
+    OIIO_CHECK_EQUAL(buf.spec().tile_width, buf.spec().width);
+    OIIO_CHECK_EQUAL(buf.storage(), ImageBuf::IMAGECACHE);
+    OIIO_CHECK_ASSERT(!buf.localpixels());  // should not look local
+
+    // Make a mutable iterator and traverse the image, even though it's an
+    // image file reference.
+    for (ImageBuf::Iterator<float> it(buf); !it.done(); ++it) {
+        OIIO_CHECK_EQUAL(it.get(0), 0.5f);
+        OIIO_CHECK_EQUAL(it[0], 0.5f);
+    }
+    // The mere existence of the mutable iterator and traversal with it
+    // should still not change anything.
+    OIIO_CHECK_EQUAL(buf.storage(), ImageBuf::IMAGECACHE);
+    OIIO_CHECK_ASSERT(!buf.localpixels());       // should not look local
+    OIIO_CHECK_EQUAL(buf.spec().tile_width, 4);  // should look tiled
+
+    // Make a mutable iterator and traverse the image, altering the pixels.
+    for (ImageBuf::Iterator<float> it(buf); !it.done(); ++it) {
         it[0] = 1.0f;
+        OIIO_CHECK_EQUAL(it[0], 1.0f);
+    }
+    // Writing through the iterator should have localized the IB
+    OIIO_CHECK_ASSERT(buf.localpixels());        // should look local now
+    OIIO_CHECK_EQUAL(buf.spec().tile_width, 0);  // should look untiled
 
     ImageCache::create()->invalidate(ustring(srcfilename));
     Filesystem::remove(srcfilename);
@@ -596,6 +618,62 @@ time_iterators()
 
 
 
+void
+test_iterator_concurrency()
+{
+    print("Testing iterator concurrency safety.\n");
+
+    // Make a source image
+    char srcfilename[] = "tmp2.exr";
+    const int rez = 256, nchans = 4;
+    ImageBuf src(ImageSpec(rez, rez, nchans, TypeFloat));
+    ImageBufAlgo::fill(src, { 0.25f, 0.5f, 0.75f, 1.0f });
+    src.set_write_tiles(64, 64);
+    src.write(srcfilename);
+
+    int nthreads = 2 * Sysutil::hardware_concurrency();
+    for (int trial = 0; trial < 100; ++trial) {
+        ImageBuf img(srcfilename, 0, 0, ImageCache::create());
+        OIIO_CHECK_ASSERT(!img.localpixels());  // should not look local
+        parallel_for(0, nthreads, [&](int index) {
+            double sum = 0.0;
+            int nchans = img.nchannels();
+            int style  = (index + trial) % 3;
+            if (style == 0) {
+                // One in three iterates with ConstIterator
+                for (ImageBuf::ConstIterator<float> it(img); !it.done(); ++it) {
+                    for (int c = 0; c < nchans; ++c)
+                        sum += it[c];
+                }
+            } else if (style == 1) {
+                // One in three iterates with Iterator, but only reads
+                for (ImageBuf::Iterator<float> it(img); !it.done(); ++it) {
+                    for (int c = 0; c < nchans; ++c)
+                        sum += it[c];
+                }
+            } else {
+                // One in every three tries to write
+                for (ImageBuf::Iterator<float> it(img); !it.done(); ++it) {
+                    for (int c = 0; c < nchans; ++c) {
+                        float v = it[c];
+                        it[c]   = v;
+                        sum += it[c];
+                    }
+                }
+            }
+            OIIO_CHECK_EQUAL(sum, 2.5 * rez * rez);
+        });
+        OIIO_CHECK_ASSERT(img.localpixels());  // should look local
+        if (trial % 10 == 9)
+            print("  {} checks out ({} threads)\n", trial + 1, nthreads);
+    }
+
+    ImageCache::create()->invalidate(ustring(srcfilename));
+    Filesystem::remove(srcfilename);
+}
+
+
+
 int
 main(int /*argc*/, char* /*argv*/[])
 {
@@ -620,6 +698,7 @@ main(int /*argc*/, char* /*argv*/[])
                                                        "mirror");
     test_mutable_iterator_with_imagecache();
     time_iterators();
+    test_iterator_concurrency();
 
     ImageBuf_test_appbuffer();
     ImageBuf_test_appbuffer_strided();