Skip to content

api: add OIIO_NODISCARD_ERROR to ImageOutput methods #5201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lgritz merged 6 commits into from
May 20, 2026
Merged

api: add OIIO_NODISCARD_ERROR to ImageOutput methods #5201

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3ead3a8
api: add OIIO_NODISCARD_ERROR to ImageOutput methods
rose413 May 15, 2026
e092be1
Fix review comments in #5201; apply clang-format
rose413 May 17, 2026
ac22aaa
style: apply clang-format-17 fixes
rose413 May 19, 2026
2334d18
fix: check return values of ImageOutput methods
rose413 May 19, 2026
0ab82c6
fix: check return value of ImageOutput::open in imagebufalgo_test.cpp
rose413 May 20, 2026
bb5e9bd
Apply suggestion from @lgritz
lgritz May 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/iconvert/iconvert.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,9 @@ convert_file(const std::string& in_filename, const std::string& out_filename)
} while (ok && in->seek_subimage(subimage, miplevel));
}

out->close();
if (!out->close()) {
errorfmt("Error closing \"{}\" : {}", out_filename, out->geterror());
}
Comment on lines +498 to +500

@lgritz lgritz May 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should also set ok=false so that the function's return value indicates that something went wrong and that we don't overwrite the real file (at this point we are writing to a temporary file, and will, later i the function, replace the destination file only if everything appeared to go right).

Suggested change
errorfmt("Error closing \"{}\" : {}", out_filename, out->geterror());
}
errorfmt("Error closing \"{}\" : {}", out_filename, out->geterror());
ok = false;
}

in->close();

// Figure out a time for the input file -- either one supplied by
Expand Down
103 changes: 59 additions & 44 deletions src/include/OpenImageIO/imageio.h
View file
Open in desktop

Large diffs are not rendered by default.

5 changes: 4 additions & 1 deletion src/libOpenImageIO/imagebuf.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1811,7 +1811,10 @@ ImageBuf::write(string_view _filename, TypeDesc dtype, string_view _fileformat,
}
if (!write(out.get(), progress_callback, progress_callback_data))
return false;
out->close();
if (!out->close()) {
error(out->geterror());
return false;
}
if (progress_callback)
progress_callback(progress_callback_data, 0);
return true;
Expand Down
6 changes: 3 additions & 3 deletions src/libOpenImageIO/imagebufalgo_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1613,7 +1613,7 @@ test_demosaic_algo(const ImageBuf& src_image, const ImageBuf& mosaiced_image,
std::string ext = type.is_floating_point() ? "exr" : "png";
std::string path = file_name + "_" + test_name + "." + ext;
auto imageOutput = ImageOutput::create(ext);
imageOutput->open(path, demosaiced_image.spec());
(void)imageOutput->open(path, demosaiced_image.spec());
demosaiced_image.write(imageOutput.get());
}
}
Expand Down Expand Up @@ -1650,7 +1650,7 @@ test_demosaic(const DemosaicTestConfig& config, const ImageBuf& src_image,
std::string path = file_name + "_src." + ext;

auto imageOutput = ImageOutput::create(ext);
imageOutput->open(path, mosaiced_image.spec());
(void)imageOutput->open(path, mosaiced_image.spec());
mosaiced_image.write(imageOutput.get());
}

Expand Down Expand Up @@ -1741,7 +1741,7 @@ test_demosaic()

if (write_files) {
auto imageOutput = OIIO::ImageOutput::create("exr");
imageOutput->open("source.exr", src_image.spec());
(void)imageOutput->open("source.exr", src_image.spec());
Comment thread
rose413 marked this conversation as resolved.
Outdated
src_image.write(imageOutput.get());
}

Expand Down
24 changes: 12 additions & 12 deletions src/libOpenImageIO/imagespeed_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ time_write_image()
OIIO_ASSERT(out);
bool ok = out->open(output_filename, outspec);
OIIO_ASSERT(ok);
out->write_image(bufspec.format, &buffer[0]);
(void)out->write_image(bufspec.format, &buffer[0]);

@lgritz lgritz May 16, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe?

Suggested change
(void)out->write_image(bufspec.format, &buffer[0]);
ok = out->write_image(bufspec.format, &buffer[0]);
OIIO_ASSERT(ok);

@lgritz lgritz May 16, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote OIIO_ASSERT, but probably OIIO_CONTRACT_ASSERT(ok) is the "more modern" idiom we're aiming for most of the time.

}


Expand All @@ -203,7 +203,7 @@ time_write_scanline_at_a_time()
size_t pixelsize = outspec.nchannels * sizeof(float);
imagesize_t scanlinesize = outspec.width * pixelsize;
for (int y = 0; y < outspec.height; ++y) {

@lgritz lgritz May 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You assign the result to ok, but ok isn't actually used anywhere. I think that it would be better if you had an OIIO_ASSERT(ok) (or OIIO_CONTRACT_ASSERT, I guess) after the loop. And the loop should probably terminate as soon as there's an error, say:

for (int y = 0; y < outspec.height && ok; ++y)

This file is a bunch of unit tests, so it's not important to issue meaningful error messages with errorfmt or anything like that. But it is a test, so we should at least have an assertion to let us know that it's not really successfully testing the thing that we think it's testing.

I'm only writing this comment here, but I think all of the changes in this file should follow a similar pattern.

out->write_scanline(y + outspec.y, outspec.z, bufspec.format,
(void)out->write_scanline(y + outspec.y, outspec.z, bufspec.format,

@lgritz lgritz May 16, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe

    bool ok = true;
    for (int y = 0; y < outspec.height && ok; ++y) {
        ok = out->write_scanline(y + outspec.y, outspec.z, bufspec.format,
                                 &buffer[scanlinesize * y]);
    }

&buffer[scanlinesize * y]);
}
}
Expand All @@ -221,7 +221,7 @@ time_write_64_scanlines_at_a_time()
size_t pixelsize = outspec.nchannels * sizeof(float);
imagesize_t scanlinesize = outspec.width * pixelsize;
for (int y = 0; y < outspec.height; y += 64) {
out->write_scanlines(y + outspec.y,
(void)out->write_scanlines(y + outspec.y,
std::min(y + outspec.y + 64,
outspec.y + outspec.height),
outspec.z, bufspec.format,
Expand All @@ -245,10 +245,10 @@ time_write_tile_at_a_time()
for (int z = 0; z < outspec.depth; z += outspec.tile_depth) {
for (int y = 0; y < outspec.height; y += outspec.tile_height) {
for (int x = 0; x < outspec.width; x += outspec.tile_width) {
out->write_tile(x + outspec.x, y + outspec.y, z + outspec.z,
bufspec.format,
&buffer[scanlinesize * y + pixelsize * x],
pixelsize, scanlinesize, planesize);
(void)out->write_tile(x + outspec.x, y + outspec.y, z + outspec.z,

@lgritz lgritz May 16, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, maybe more like

bool ok = true;
for (int z = 0; z < outspec.depth && ok; ...) {
    for (int y = 0; y < outspec.height && ok; ...) {
        for (int x = 0; y < outspec.width && ok; ...) {
            ok = out->write_tile(...)

I guess I'm superstitious about ignoring the return code, that's what we're trying to be caching. Even though this is a test... what better time to catch a problem then when we're testing?

bufspec.format,
&buffer[scanlinesize * y + pixelsize * x],
pixelsize, scanlinesize, planesize);
}
}
}
Expand All @@ -268,11 +268,11 @@ time_write_tiles_row_at_a_time()
imagesize_t scanlinesize = outspec.width * pixelsize;
for (int z = 0; z < outspec.depth; z += outspec.tile_depth) {
for (int y = 0; y < outspec.height; y += outspec.tile_height) {
out->write_tiles(outspec.x, outspec.x + outspec.width,
y + outspec.y, y + outspec.y + outspec.tile_height,
z + outspec.z, z + outspec.z + outspec.tile_depth,
bufspec.format, &buffer[scanlinesize * y],
pixelsize /*xstride*/, scanlinesize /*ystride*/);
(void)out->write_tiles(outspec.x, outspec.x + outspec.width,

@lgritz lgritz May 16, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here, too

y + outspec.y, y + outspec.y + outspec.tile_height,
z + outspec.z, z + outspec.z + outspec.tile_depth,
bufspec.format, &buffer[scanlinesize * y],
pixelsize /*xstride*/, scanlinesize /*ystride*/);
}
}
}
Expand Down
8 changes: 6 additions & 2 deletions src/libOpenImageIO/maketexture.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,7 +792,9 @@ write_mipmap(ImageBufAlgo::MakeTextureMode mode, std::shared_ptr<ImageBuf>& img,
// ImageBuf::write transfers any errors from the ImageOutput to
// the ImageBuf.
errorfmt("Write failed: {}", img->geterror());
out->close();
if (!out->close()) {
errorfmt("Close failed: {}", out->geterror());
}
return false;
}

Expand Down Expand Up @@ -965,7 +967,9 @@ write_mipmap(ImageBufAlgo::MakeTextureMode mode, std::shared_ptr<ImageBuf>& img,
// ImageOutput to the ImageBuf.
errorfmt("Error writing \"{}\" : {}", outputfilename,
small->geterror());
out->close();
if (!out->close()) {
errorfmt("Close failed: {}", out->geterror());
}
return false;
}
double wtime = writetimer();
Expand Down
4 changes: 3 additions & 1 deletion src/python/py_imagebuf.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,9 @@ ImageBuf_repr_png(const ImageBuf& self)
if (!out || !out->open("temp.png", altered_spec))
return py::bytes();
self.write(out.get());
out->close();
if (!out->close()) {
return py::bytes();
}

// Cast to const char* and return as python bytes
const char* char_ptr = reinterpret_cast<const char*>(file_buffer.data());
Expand Down
Loading