TIFF: subtle bug -- re-opening forgot about rawcolor hint. (AcademySo…

…ftwareFoundation#2285)

When scanlines are accessed out of order, to emulate true random access,
sometimes we close the file and re-open. But in this one spot, instead
of calling close_tif() we called close(), and the difference is that close()
also calls init() which resets the options that might have been set by
configuration hints -- such as "oiio:RawColor" and "oiio:UnassociatedAlpha".
In this case, we may want to close/open the TIFF file itself, but we don't
want to "forget" about any configuration hints from the original open call.

The circumstances that led to this being symptomatic are obscure and amusing:

- Scanline TIFF file with CMYK color space, and opened with a
  configuration hint "oiio:RawColor" which causes the CMYK values (4
  chans) to be preserved rather than it all get auto-converted to RGB
  and appear to the app as if it were a 3 channel file.

- The image was held in an ImageCache-backed ImageBuf. And the IC has
  the "autotile" feature turned on (so it will try to break it up into
  tiles in the cache, instead of storing the whole scanline image as a
  single tile).

- An IBA function was running parallelized over multiple threads, and
  it happened that a thread needing a lower part of the image (high
  scanline number) made the request for its tile before another thread
  that needed a tile with lower scanline numbers. This caused the actual
  reads of the scanlines to not be in proper order, therefore necessitating
  a close and re-open of the file.

- Upon the re-open, it had forgotten that "oiio:RawColor" was set, so
  even though the app had asked for the pixels to be written into a 2k
  x 4 chan CMYK buffer, the confused TIFF reader thought it was in the
  mode where it was emulating a 3 channel RGB file. So it would not
  only convert to RGB when it should have left it as cmyk, but since the
  RGB is only 3 channels, the 2048 pixels only filled in the left-most
  1536 (that is, 3/4!) of the value slots in each scanline of the app's
  buffer.

- When running valgrind on the app that was reading the image (for
  unrelated reasons), it was complaining about reading uninitialized
  memory when it would get to pixel 1536, mystifying everybody.

... And I'm so glad today I work on monster movies and cartoons, and not
nuclear power plant control software or life support systems.

Additional fixes included:

* And add a test case

* Python ImageBuf: add constructor that takes a configuration hint spec

* ImageBuf: when adding file to cache, careful with configuration.
If you open an ImageCache-backed ImageBuf, close, then have another
ImageBuf opened later with a different set of "config" hints... you
may not get expected behaviors. Instead, be sure to invalidate it and
add it to the cache in "replace" mode.

src/libOpenImageIO/imagebuf.cpp

@@ -795,8 +795,13 @@ ImageBufImpl::init_spec(string_view filename, int subimage, int miplevel)
     m_nmiplevels   = 0;
     static ustring s_subimages("subimages"), s_miplevels("miplevels");
     static ustring s_fileformat("fileformat");
-    if (m_configspec)  // Pass configuration options to cache
-        m_imagecache->add_file(m_name, nullptr, m_configspec.get());
+    if (m_configspec) {  // Pass configuration options to cache
+        // Invalidate the file in the cache, and add with replacement
+        // because it might have a different config than last time.
+        m_imagecache->invalidate(m_name, true);
+        m_imagecache->add_file(m_name, nullptr, m_configspec.get(),
+                               /*replace=*/true);
+    }
     m_imagecache->get_image_info(m_name, subimage, miplevel, s_subimages,
                                  TypeInt, &m_nsubimages);
     m_imagecache->get_image_info(m_name, subimage, miplevel, s_miplevels,

src/python/py_imagebuf.cpp

@@ -224,6 +224,11 @@ declare_imagebuf(py::module& m)
             auto z = zero ? InitializePixels::Yes : InitializePixels::No;
             return ImageBuf(spec, z);
         }))
+        .def(py::init([](const std::string& name, int subimage, int miplevel,
+                         const ImageSpec& config) {
+                 return ImageBuf(name, subimage, miplevel, nullptr, &config);
+             }),
+             "name"_a, "subimage"_a, "miplevel"_a, "config"_a)
         .def("clear", &ImageBuf::clear)
         .def(
             "reset",

src/tiff.imageio/tiffinput.cpp

@@ -197,6 +197,8 @@ class TIFFInput final : public ImageInput {
         m_subimage_specs.clear();
     }
 
+    // Just close the TIFF file handle, but don't forget anything we
+    // learned about the contents of the file or any configuration hints.
     void close_tif()
     {
         if (m_tif) {
@@ -1454,7 +1456,14 @@ TIFFInput::read_native_scanline(int subimage, int miplevel, int y, int z,
             ImageSpec dummyspec;
             int old_subimage = current_subimage();
             int old_miplevel = current_miplevel();
-            if (!close() || !open(m_filename, dummyspec)
+            // We need to close the TIFF file s that we can re-open and
+            // seek back to the beginning of this subimage. The close_tif()
+            // accomplishes that. It's important not to do a full close()
+            // here, because that would also call init() to fully reset
+            // to a fresh ImageInput, thus forgetting any configuration
+            // settings such as raw_color or keep_unassociated_alpha.
+            close_tif();
+            if (!open(m_filename, dummyspec)
                 || !seek_subimage(old_subimage, old_miplevel)) {
                 return false;  // Somehow, the re-open failed
             }

testsuite/python-imageinput/ref/out-alt.txt

@@ -153,4 +153,30 @@ Test read_image into FLOAT:
 Opened "testu16.tif" as a tiff
 Read array typecode float32  [ 12288 ]
 
+Testing write and read of unassociated:
+  writing:  [[[0.5 0.5 0.5 0.5]
+  [0.5 0.5 0.5 0.5]]
+
+ [[0.5 0.5 0.5 0.5]
+  [0.5 0.5 0.5 0.5]]]
+
+  default reading as IB:  [[[0.25 0.25 0.25 0.5 ]
+  [0.25 0.25 0.25 0.5 ]]
+
+ [[0.25 0.25 0.25 0.5 ]
+  [0.25 0.25 0.25 0.5 ]]]
+
+  reading as IB with unassoc hint:  [[[0.5 0.5 0.5 0.5]
+  [0.5 0.5 0.5 0.5]]
+
+ [[0.5 0.5 0.5 0.5]
+  [0.5 0.5 0.5 0.5]]]
+
+  reading as II with hint, read scanlines backward: 
+    [1] =  [[0.5 0.5 0.5 0.5]
+ [0.5 0.5 0.5 0.5]]
+    [0] =  [[0.5 0.5 0.5 0.5]
+ [0.5 0.5 0.5 0.5]]
+
+
 Done.

testsuite/python-imageinput/ref/out-alt2.txt

@@ -153,4 +153,30 @@ Test read_image into FLOAT:
 Opened "testu16.tif" as a tiff
 Read array typecode float32  [ 12288 ]
 
+Testing write and read of unassociated:
+  writing:  [[[0.5 0.5 0.5 0.5]
+  [0.5 0.5 0.5 0.5]]
+
+ [[0.5 0.5 0.5 0.5]
+  [0.5 0.5 0.5 0.5]]]
+
+  default reading as IB:  [[[0.25 0.25 0.25 0.5 ]
+  [0.25 0.25 0.25 0.5 ]]
+
+ [[0.25 0.25 0.25 0.5 ]
+  [0.25 0.25 0.25 0.5 ]]]
+
+  reading as IB with unassoc hint:  [[[0.5 0.5 0.5 0.5]
+  [0.5 0.5 0.5 0.5]]
+
+ [[0.5 0.5 0.5 0.5]
+  [0.5 0.5 0.5 0.5]]]
+
+  reading as II with hint, read scanlines backward: 
+    [1] =  [[0.5 0.5 0.5 0.5]
+ [0.5 0.5 0.5 0.5]]
+    [0] =  [[0.5 0.5 0.5 0.5]
+ [0.5 0.5 0.5 0.5]]
+
+
 Done.

testsuite/python-imageinput/ref/out-python3.txt

@@ -153,4 +153,30 @@ Test read_image into FLOAT:
 Opened "testu16.tif" as a tiff
 Read array typecode float32  [ 12288 ]
 
+Testing write and read of unassociated:
+  writing:  [[[0.5 0.5 0.5 0.5]
+  [0.5 0.5 0.5 0.5]]
+
+ [[0.5 0.5 0.5 0.5]
+  [0.5 0.5 0.5 0.5]]]
+
+  default reading as IB:  [[[0.25 0.25 0.25 0.5 ]
+  [0.25 0.25 0.25 0.5 ]]
+
+ [[0.25 0.25 0.25 0.5 ]
+  [0.25 0.25 0.25 0.5 ]]]
+
+  reading as IB with unassoc hint:  [[[0.5 0.5 0.5 0.5]
+  [0.5 0.5 0.5 0.5]]
+
+ [[0.5 0.5 0.5 0.5]
+  [0.5 0.5 0.5 0.5]]]
+
+  reading as II with hint, read scanlines backward: 
+    [1] =  [[0.5 0.5 0.5 0.5]
+ [0.5 0.5 0.5 0.5]]
+    [0] =  [[0.5 0.5 0.5 0.5]
+ [0.5 0.5 0.5 0.5]]
+
+
 Done.

testsuite/python-imageinput/ref/out-travis.txt

@@ -153,4 +153,30 @@ Test read_image into FLOAT:
 Opened "testu16.tif" as a tiff
 Read array typecode float32  [ 12288 ]
 
+Testing write and read of unassociated:
+  writing:  [[[ 0.5  0.5  0.5  0.5]
+  [ 0.5  0.5  0.5  0.5]]
+
+ [[ 0.5  0.5  0.5  0.5]
+  [ 0.5  0.5  0.5  0.5]]]
+
+  default reading as IB:  [[[ 0.25  0.25  0.25  0.5 ]
+  [ 0.25  0.25  0.25  0.5 ]]
+
+ [[ 0.25  0.25  0.25  0.5 ]
+  [ 0.25  0.25  0.25  0.5 ]]]
+
+  reading as IB with unassoc hint:  [[[ 0.5  0.5  0.5  0.5]
+  [ 0.5  0.5  0.5  0.5]]
+
+ [[ 0.5  0.5  0.5  0.5]
+  [ 0.5  0.5  0.5  0.5]]]
+
+  reading as II with hint, read scanlines backward: 
+    [1] =  [[ 0.5  0.5  0.5  0.5]
+ [ 0.5  0.5  0.5  0.5]]
+    [0] =  [[ 0.5  0.5  0.5  0.5]
+ [ 0.5  0.5  0.5  0.5]]
+
+
 Done.

testsuite/python-imageinput/ref/out.txt

@@ -153,4 +153,30 @@ Test read_image into FLOAT:
 Opened "testu16.tif" as a tiff
 Read array typecode float32  [ 12288 ]
 
+Testing write and read of unassociated:
+  writing:  [[[0.5 0.5 0.5 0.5]
+  [0.5 0.5 0.5 0.5]]
+
+ [[0.5 0.5 0.5 0.5]
+  [0.5 0.5 0.5 0.5]]]
+
+  default reading as IB:  [[[0.25 0.25 0.25 0.5 ]
+  [0.25 0.25 0.25 0.5 ]]
+
+ [[0.25 0.25 0.25 0.5 ]
+  [0.25 0.25 0.25 0.5 ]]]
+
+  reading as IB with unassoc hint:  [[[0.5 0.5 0.5 0.5]
+  [0.5 0.5 0.5 0.5]]
+
+ [[0.5 0.5 0.5 0.5]
+  [0.5 0.5 0.5 0.5]]]
+
+  reading as II with hint, read scanlines backward: 
+    [1] =  [[0.5 0.5 0.5 0.5]
+ [0.5 0.5 0.5 0.5]]
+    [0] =  [[0.5 0.5 0.5 0.5]
+ [0.5 0.5 0.5 0.5]]
+
+
 Done.

testsuite/python-imageinput/src/test_imageinput.py

@@ -198,6 +198,32 @@ def write (image, filename, format=oiio.UNKNOWN) :
         print ("Error writing", filename, ":", image.geterror())
 
 
+# Regression test for #2285: configuration settings were "forgotten" if the
+# scanline read order necessitated closing and reopening the file.
+def test_tiff_remembering_config() :
+    # Create a file that has unassociated alpha
+    print ("Testing write and read of unassociated:")
+    spec = oiio.ImageSpec(2,2,4,"float")
+    spec.attribute("oiio:UnassociatedAlpha", 1)
+    wbuf = oiio.ImageBuf(spec)
+    oiio.ImageBufAlgo.fill(wbuf, (0.5, 0.5, 0.5, 0.5))
+    print ("  writing: ", wbuf.get_pixels())
+    wbuf.write("test_unassoc.tif")
+    rbuf = oiio.ImageBuf("test_unassoc.tif")
+    print ("\n  default reading as IB: ", rbuf.get_pixels())
+    config = oiio.ImageSpec()
+    config.attribute("oiio:UnassociatedAlpha", 1)
+    rbuf = oiio.ImageBuf("test_unassoc.tif", 0, 0, config)
+    print ("\n  reading as IB with unassoc hint: ", rbuf.get_pixels())
+    print ("\n  reading as II with hint, read scanlines backward: ")
+    ii = oiio.ImageInput.open("test_unassoc.tif", config)
+    print ("    [1] = ", ii.read_scanline(1))
+    print ("    [0] = ", ii.read_scanline(0))
+    print ("\n")
+
+
+
+
 ######################################################################
 # main test starts here
 
@@ -258,6 +284,8 @@ def write (image, filename, format=oiio.UNKNOWN) :
     test_readimage ("testu16.tif", method="image", type="float",
                     keep_unknown=True, print_pixels=False)
 
+    test_tiff_remembering_config()
+
     print ("Done.")
 except Exception as detail:
     print ("Unknown exception:", detail)