fix: ImageOutput::check_open logic was flawed (#4779)

We documented this function as checking the valid range of pixel
coordinates, but we CALLED it everywhere as if we were only checking the
resolution.

But we IMPLEMENTED as if we were checking the range, generating errors
for outputting exr files with negative pixel window origins. But why was
nobody seeing those errors when writing exr files with negative origins?
Because the openexr writer BOTCHED it by not noticing the return value
and considering it a failure.

Oh boy. All my fault.

So the fix:

* Change the comment documenting check_open() to match the way we had
been using it at the call sites.

* Change the implementation of check_open() to match this by no longer
checking whether all four corners were in the range -- only check if
each dimension's size is within the limit for that dimension.

* Fix the OpenEXR writer's failure to notice the check_open failure.

* Does this eliminate necessary checks? No! We already separately
checked that nonzero origins were only allowed for formats that support
it, and that negative origins were only allowed for formats that support
that. So the flawed check was redundant anyway. Ha!

* Add a test verifying that we can write exr files with negative
origins. (Technically, that would have passed before, but only because
we were ignoring the incorrect error.)

---------

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

Author: lgritz

src/include/OpenImageIO/imageio.h

@@ -2164,11 +2164,12 @@ class OIIO_API ImageInput {
     ///     The ImageSpec that we are validating.
     ///
     /// @param range
-    ///     An ROI that describes the allowable pixel coordinates and channel
-    ///     indices as half-open intervals.  For example, the default value
-    ///     `{0, 65535, 0, 65535, 0, 1, 0, 4}` means that pixel coordinates
-    ///     must be non-negative and the width and height be representable by
-    ///     a uint16 value, up to 4 channels are allowed, and volumes are not
+    ///     An ROI that describes the allowable resolution and channel count:
+    ///     the width, height, depth, and channel count of the ROI are the
+    ///     maximum allowed for the file type. For example, the default value
+    ///     `{0, 65535, 0, 65535, 0, 1, 0, 4}` means that pixel data width
+    ///     and height must be non-negative and representable by uint16
+    ///     values, up to 4 channels are allowed, and volumes are not
     ///     permitted (z coordinate may only be 0). File formats that can
     ///     handle larger resolutions, or volumes, or >4 channels must
     ///     override these limits!

src/libOpenImageIO/imageoutput.cpp

@@ -1124,24 +1124,6 @@ ImageOutput::check_open(OpenMode mode, const ImageSpec& userspec, ROI range,
                 m_spec.z = 0;
             }
         }
-        if (m_spec.x < range.xbegin || m_spec.x + m_spec.width > range.xend
-            || m_spec.y < range.ybegin || m_spec.y + m_spec.height > range.yend
-            || m_spec.z < range.zbegin
-            || m_spec.z + m_spec.depth > range.zend) {
-            if (m_spec.depth == 1)
-                errorfmt(
-                    "{} requested pixel data window [{}, {}) x [{}, {}) exceeds the allowable range of [{}, {}) x [{}, {})",
-                    format_name(), m_spec.x, m_spec.x + m_spec.width, m_spec.y,
-                    m_spec.y + m_spec.height, range.xbegin, range.xend,
-                    range.ybegin, range.yend);
-            else
-                errorfmt(
-                    "{} requested pixel data window [{}, {}) x [{}, {}) x [{}, {}) exceeds the allowable range of [{}, {}) x [{}, {}) x [{}, {})\n{} vs {}\n",
-                    format_name(), m_spec.x, m_spec.x + m_spec.width, m_spec.y,
-                    m_spec.y + m_spec.height, m_spec.z, m_spec.z + m_spec.depth,
-                    range.xbegin, range.xend, range.ybegin, range.yend,
-                    range.zbegin, range.zend);
-        }
     }
 
     if (m_spec.extra_attribs.contains("ioproxy") && !supports("ioproxy")) {

src/openexr.imageio/exrinput.cpp

@@ -1359,8 +1359,8 @@ OpenEXRInput::read_native_tiles(int subimage, int miplevel, int xbegin,
         tmpbuf.resize(nxtiles * nytiles * m_spec.tile_bytes(true));
         data = tmpbuf.data();
     }
-    char* buf = (char*)data - xbegin * pixelbytes
-                - ybegin * pixelbytes * m_spec.tile_width * nxtiles;
+    char* buf = (char*)data - ptrdiff_t(xbegin * pixelbytes)
+                - ptrdiff_t(ybegin * pixelbytes * m_spec.tile_width * nxtiles);
 
     try {
         Imf::FrameBuffer frameBuffer;

src/openexr.imageio/exroutput.cpp

@@ -375,7 +375,8 @@ OpenEXROutput::open(const std::string& name, const ImageSpec& userspec,
         m_nmiplevels = 1;
         m_miplevel   = 0;
         m_headers.resize(1);
-        copy_and_check_spec(userspec, m_spec);
+        if (!copy_and_check_spec(userspec, m_spec))
+            return false;
         sanity_check_channelnames();
         const ParamValue* param = m_spec.find_attribute("oiio:ioproxy",
                                                         TypeDesc::PTR);
@@ -1321,7 +1322,7 @@ OpenEXROutput::sanity_check_channelnames()
             if (m_spec.channelnames[c].empty()
                 || m_spec.channelnames[c] == m_spec.channelnames[i]) {
                 // Duplicate or missing channel name! We don't want
-                // libIlmImf to drop the channel (as it will do for
+                // libOpenEXR to drop the channel (as it will do for
                 // duplicates), so rename it and hope for the best.
                 m_spec.channelnames[c] = Strutil::fmt::format("channel{}", c);
                 break;
@@ -1384,7 +1385,8 @@ OpenEXROutput::write_scanline(int y, int z, TypeDesc format, const void* data,
     // where the address of the "virtual framebuffer" for the whole
     // image.
     imagesize_t scanlinebytes = m_spec.scanline_bytes(native);
-    char* buf = (char*)data - m_spec.x * pixel_bytes - y * scanlinebytes;
+    char* buf                 = (char*)data - ptrdiff_t(m_spec.x * pixel_bytes)
+                - ptrdiff_t(y * scanlinebytes);
 
     try {
         Imf::FrameBuffer frameBuffer;
@@ -1647,7 +1649,8 @@ OpenEXROutput::write_tiles(int xbegin, int xend, int ybegin, int yend,
         data = &padded[0];
     }
 
-    char* buf = (char*)data - xbegin * pixelbytes - ybegin * widthbytes;
+    char* buf = (char*)data - ptrdiff_t(xbegin * pixelbytes)
+                - ptrdiff_t(ybegin * widthbytes);
 
     try {
         Imf::FrameBuffer frameBuffer;

testsuite/openexr-suite/ref/out.txt

@@ -347,3 +347,17 @@ Reading ../openexr-images/Beachball/singlepart.0001.exr
     openexr:lineOrder: "increasingY"
 Comparing "../openexr-images/Beachball/singlepart.0001.exr" and "singlepart.0001.exr"
 PASS
+Reading negoverscan.exr
+negoverscan.exr      :   64 x   64, 3 channel, half openexr
+    SHA-1: EBDD38B69CD5B9F2D00D273C981E16960FBBB4F7
+    channel list: R, G, B
+    pixel data origin: x=-16, y=-16
+    full/display size: 64 x 64
+    full/display origin: -16, -16
+    compression: "zip"
+    PixelAspectRatio: 1
+    screenWindowCenter: 0, 0
+    screenWindowWidth: 1
+    oiio:ColorSpace: "lin_rec709"
+    oiio:subimages: 1
+    openexr:lineOrder: "increasingY"

testsuite/openexr-suite/run.py

@@ -4,6 +4,8 @@
 # SPDX-License-Identifier: Apache-2.0
 # https://github.com/AcademySoftwareFoundation/OpenImageIO
 
+redirect = " >> out.txt 2>&1"
+
 # ../openexr-images/ScanLines:
 # Blobbies.exr     Cannon.exr  MtTamWest.exr     StillLife.exr
 # CandleGlass.exr  Desk.exr    PrismsLenses.exr  Tree.exr
@@ -44,3 +46,7 @@
 files = [ "singlepart.0001.exr" ]
 for f in files:
     command += rw_command (imagedir, f)
+
+# Check writing overscan and negative range
+command += oiiotool("--create 64x64-16-16 3 -d half -o negoverscan.exr")
+command += info_command("negoverscan.exr", safematch=True)