diff --git a/src/libOpenImageIO/imagebuf.cpp b/src/libOpenImageIO/imagebuf.cpp index dcb8421a27..83579cd584 100644 --- a/src/libOpenImageIO/imagebuf.cpp +++ b/src/libOpenImageIO/imagebuf.cpp @@ -1207,8 +1207,11 @@ ImageBufImpl::init_spec(string_view filename, int subimage, int miplevel, if (m_spec["thumbnail_width"].get() && m_spec["thumbnail_height"].get()) { 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; diff --git a/src/libOpenImageIO/imageinout_test.cpp b/src/libOpenImageIO/imageinout_test.cpp index 3db426fa11..d80b55d4e3 100644 --- a/src/libOpenImageIO/imageinout_test.cpp +++ b/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); diff --git a/src/libOpenImageIO/imageoutput.cpp b/src/libOpenImageIO/imageoutput.cpp index 4ec5300c3a..434a34412d 100644 --- a/src/libOpenImageIO/imageoutput.cpp +++ b/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 } diff --git a/src/oiiotool/oiiotool.cpp b/src/oiiotool/oiiotool.cpp index 8f9a307b67..185d3e43b8 100644 --- a/src/oiiotool/oiiotool.cpp +++ b/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 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 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 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 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)) {