fix(thumb): don't write thumbnail_* metadata to formats that can't hold a thumbnail

src/libOpenImageIO/imagebuf.cpp

@@ -1207,8 +1207,11 @@ ImageBufImpl::init_spec(string_view filename, int subimage, int miplevel,
         if (m_spec["thumbnail_width"].get<int>()
             && m_spec["thumbnail_height"].get<int>()) {
             m_thumbnail.reset(new ImageBuf);
-            m_imagecache->get_thumbnail(m_name, *m_thumbnail, subimage);
-            m_has_thumbnail = true;
+            m_has_thumbnail = m_imagecache->get_thumbnail(m_name, *m_thumbnail,
+                                                          subimage);
+            // Don't keep a thumbnail buffer we failed to fill in.
+            if (!m_has_thumbnail)
+                m_thumbnail.reset();
         }
 
         // Subtlety: m_nativespec will have the true formats of the file, but
@@ -1291,6 +1294,9 @@ ImageBufImpl::init_spec(string_view filename, int subimage, int miplevel,
             m_thumbnail.reset(new ImageBuf);
             m_has_thumbnail = input->get_thumbnail(*m_thumbnail.get(),
                                                    subimage);
+            // Don't keep a thumbnail buffer we failed to fill in.
+            if (!m_has_thumbnail)
+                m_thumbnail.reset();
         }
 
         m_current_subimage = subimage;

src/libOpenImageIO/imageinout_test.cpp

@@ -460,6 +460,62 @@ test_all_formats()
 
 
 
+// A format that can't embed a thumbnail must not write the thumbnail_*
+// attributes, where they'd describe a thumbnail that isn't in the file.
+void
+test_thumbnail_attribs()
+{
+    std::cout << "Testing that thumbnail attributes don't leak:\n";
+    for (auto& e :
+         Strutil::splitsv(OIIO::get_string_attribute("extension_list"), ";")) {
+        auto fmtexts           = Strutil::splitsv(e, ":");
+        string_view formatname = fmtexts[0];
+        if (formatname == "term")
+            continue;  // writes to the terminal, not a file
+        if (onlyformat.size() && formatname != onlyformat)
+            continue;
+        std::string filename
+            = Strutil::fmt::format("imageinout_test_thumb-{}.{}", formatname,
+                                   Strutil::splitsv(fmtexts[1], ",")[0]);
+        auto out = ImageOutput::create(filename);
+        if (!out) {
+            (void)OIIO::geterror();
+            continue;
+        }
+        if (out->supports("thumbnail"))
+            continue;
+
+        // Write an image whose spec claims a thumbnail it doesn't have - the lie.
+        ImageBuf buf   = make_test_image(formatname);
+        ImageSpec spec = buf.spec();
+        spec.attribute("thumbnail_width", 16);
+        spec.attribute("thumbnail_height", 12);
+        spec.attribute("thumbnail_nchannels", 3);
+        std::string errmsg;
+        if (!checked_write(out.get(), filename, spec, spec.format,
+                           buf.localpixels_as_writable_byte_image_span(),
+                           false /*do_asserts*/, &errmsg))
+            continue;  // can't write this image at all - not our problem
+
+        auto in = ImageInput::open(filename);
+        OIIO_CHECK_ASSERT(in && "could not reopen the file we just wrote");
+        if (in) {
+            std::cout << "    " << formatname << "\n";
+            const ImageSpec& s(in->spec());
+            OIIO_CHECK_EQUAL(s.get_int_attribute("thumbnail_width"), 0);
+            OIIO_CHECK_EQUAL(s.get_int_attribute("thumbnail_height"), 0);
+            OIIO_CHECK_EQUAL(s.get_int_attribute("thumbnail_nchannels"), 0);
+        } else {
+            (void)OIIO::geterror();
+        }
+        if (!nodelete)
+            Filesystem::remove(filename);
+    }
+    std::cout << "\n";
+}
+
+
+
 // This tests a particular troublesome case where we got the logic wrong.
 // Read 1-channel float exr into 4-channel uint8 buffer with 4-byte xstride.
 // The correct behavior is to translate the one channel from float to uint8
@@ -654,6 +710,7 @@ main(int argc, char* argv[])
     }
 
     test_all_formats();
+    test_thumbnail_attribs();
     test_read_tricky_sizes();
     benchmark_tile_sizes("exr", TypeHalf, 4);
     benchmark_tile_sizes("tif", TypeUInt16, 16);

src/libOpenImageIO/imageoutput.cpp

@@ -1179,6 +1179,15 @@ ImageOutput::check_open(OpenMode mode, const ImageSpec& userspec, ROI range,
         return false;
     }
 
+    // Don't write thumbnail_* metadata to a format that can't embed a
+    // thumbnail, where they'd describe one that isn't there.
+    if (!supports("thumbnail")) {
+        m_spec.erase_attribute("thumbnail_width");
+        m_spec.erase_attribute("thumbnail_height");
+        m_spec.erase_attribute("thumbnail_nchannels");
+        m_spec.erase_attribute("thumbnail_image");
+    }
+
     return true;  // all is ok
 }
 

src/oiiotool/oiiotool.cpp

@@ -887,7 +887,6 @@ adjust_output_options(string_view filename, ImageSpec& spec,
                       const ImageSpec* nativespec, const Oiiotool& ot,
                       int subimage_index, int nsubimages,
                       bool format_supports_tiles,
-                      bool format_supports_thumbnail,
                       const ParamValueList& fileoptions,
                       bool was_direct_read = false)
 {
@@ -1060,16 +1059,6 @@ adjust_output_options(string_view filename, ImageSpec& spec,
     spec.erase_attribute("oiio:SHA-1");
     spec.erase_attribute("oiio:ConstantColor");
     spec.erase_attribute("oiio:AverageColor");
-
-    // If the output format can't embed a thumbnail, don't let the thumbnail
-    // bookkeeping attributes leak into the file as metadata describing a
-    // thumbnail that isn't actually there.
-    if (!format_supports_thumbnail) {
-        spec.erase_attribute("thumbnail_width");
-        spec.erase_attribute("thumbnail_height");
-        spec.erase_attribute("thumbnail_nchannels");
-        spec.erase_attribute("thumbnail_image");
-    }
 }
 
 
@@ -6074,9 +6063,8 @@ output_file(Oiiotool& ot, cspan<const char*> argv)
     }
     bool supports_displaywindow  = out->supports("displaywindow");
     bool supports_negativeorigin = out->supports("negativeorigin");
-    bool supports_tiles     = out->supports("tiles") || ot.output_force_tiles;
-    bool procedural         = out->supports("procedural");
-    bool supports_thumbnail = out->supports("thumbnail");
+    bool supports_tiles = out->supports("tiles") || ot.output_force_tiles;
+    bool procedural     = out->supports("procedural");
     if (!ot.read()) {
         return;
     }
@@ -6256,7 +6244,7 @@ output_file(Oiiotool& ot, cspan<const char*> argv)
     if (do_tex || do_latlong || do_bumpslopes) {
         ImageSpec configspec;
         adjust_output_options(filename, configspec, nullptr, ot, 0, 1,
-                              supports_tiles, supports_thumbnail, fileoptions);
+                              supports_tiles, fileoptions);
         prep_texture_config(ot, configspec, fileoptions);
         ImageBufAlgo::MakeTextureMode mode = ImageBufAlgo::MakeTxTexture;
         if (do_shad)
@@ -6286,8 +6274,8 @@ output_file(Oiiotool& ot, cspan<const char*> argv)
         for (int s = 0, send = ir->subimages(); s < send; ++s) {
             ImageSpec spec = *ir->spec(s, 0);
             adjust_output_options(filename, spec, ir->nativespec(s), ot, s,
-                                  send, supports_tiles, supports_thumbnail,
-                                  fileoptions, (*ir)[s].was_direct_read());
+                                  send, supports_tiles, fileoptions,
+                                  (*ir)[s].was_direct_read());
             // If it's not tiled and MIP-mapped, remove any "textureformat"
             if (!spec.tile_pixels() || ir->miplevels(s) <= 1)
                 spec.erase_attribute("textureformat");
@@ -6328,8 +6316,7 @@ output_file(Oiiotool& ot, cspan<const char*> argv)
             for (int m = 0, mend = ir->miplevels(s); m < mend && ok; ++m) {
                 ImageSpec spec = *ir->spec(s, m);
                 adjust_output_options(filename, spec, ir->nativespec(s, m), ot,
-                                      s, send, supports_tiles,
-                                      supports_thumbnail, fileoptions,
+                                      s, send, supports_tiles, fileoptions,
                                       (*ir)[s].was_direct_read());
                 if (s > 0 || m > 0) {  // already opened first subimage/level
                     if (!out->open(tmpfilename, spec, mode)) {