From 8716e05202a6f8c6fe4b1fb7d1e4487caa6f214d Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Thu, 11 Jul 2019 17:31:18 -0700 Subject: [PATCH] TIFF: subtle bug -- re-opening forgot about rawcolor hint. (#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 | 9 ++++-- src/python/py_imagebuf.cpp | 5 ++++ src/tiff.imageio/tiffinput.cpp | 11 +++++++- testsuite/python-imageinput/ref/out-alt.txt | 26 +++++++++++++++++ testsuite/python-imageinput/ref/out-alt2.txt | 26 +++++++++++++++++ .../python-imageinput/ref/out-python3.txt | 26 +++++++++++++++++ .../python-imageinput/ref/out-travis.txt | 26 +++++++++++++++++ testsuite/python-imageinput/ref/out.txt | 26 +++++++++++++++++ .../python-imageinput/src/test_imageinput.py | 28 +++++++++++++++++++ 9 files changed, 180 insertions(+), 3 deletions(-) diff --git a/src/libOpenImageIO/imagebuf.cpp b/src/libOpenImageIO/imagebuf.cpp index a2426b9b5d..f71e058324 100644 --- a/src/libOpenImageIO/imagebuf.cpp +++ b/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, diff --git a/src/python/py_imagebuf.cpp b/src/python/py_imagebuf.cpp index 5be48d3a4b..34303db1d6 100644 --- a/src/python/py_imagebuf.cpp +++ b/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", diff --git a/src/tiff.imageio/tiffinput.cpp b/src/tiff.imageio/tiffinput.cpp index f2b99a70c9..7c7f7e58fa 100644 --- a/src/tiff.imageio/tiffinput.cpp +++ b/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 } diff --git a/testsuite/python-imageinput/ref/out-alt.txt b/testsuite/python-imageinput/ref/out-alt.txt index 646e0b5f6d..f9b0457b16 100644 --- a/testsuite/python-imageinput/ref/out-alt.txt +++ b/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. diff --git a/testsuite/python-imageinput/ref/out-alt2.txt b/testsuite/python-imageinput/ref/out-alt2.txt index 6ee0c516e6..a582d7cce1 100644 --- a/testsuite/python-imageinput/ref/out-alt2.txt +++ b/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. diff --git a/testsuite/python-imageinput/ref/out-python3.txt b/testsuite/python-imageinput/ref/out-python3.txt index 1798a2006c..0c5434ca55 100644 --- a/testsuite/python-imageinput/ref/out-python3.txt +++ b/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. diff --git a/testsuite/python-imageinput/ref/out-travis.txt b/testsuite/python-imageinput/ref/out-travis.txt index 373205a56f..e3c2c91da9 100644 --- a/testsuite/python-imageinput/ref/out-travis.txt +++ b/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. diff --git a/testsuite/python-imageinput/ref/out.txt b/testsuite/python-imageinput/ref/out.txt index 6ee0c516e6..a582d7cce1 100644 --- a/testsuite/python-imageinput/ref/out.txt +++ b/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. diff --git a/testsuite/python-imageinput/src/test_imageinput.py b/testsuite/python-imageinput/src/test_imageinput.py index 34f2c7d13a..93a9f04aec 100755 --- a/testsuite/python-imageinput/src/test_imageinput.py +++ b/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)