Skip to content

Commit

Permalink
fix(tiff): Fix TIFF export with EXIF data and I/O proxy (#4300)
Browse files Browse the repository at this point in the history
TIFF export with EXIF metadata (e.g. oiio::ColorSpace) creates invalid
TIFF files in the presence of an I/O proxy ("TIFF directory is missing
required "ImageLength" field"). The reasons is that TIFFSetDirectory()
requires to read some of the previously written data, but neither
writer_readproc() nor the available proxy implementations support that.
Note that TIFF output does not yet use this new scheme for I/O proxy
that was used for many other plugins.

I think writer_readproc() should not take any shortcuts and just forward
the call to the I/O proxy.

Is there a reason why IOFile (in write mode) and IOVecOutput do not
allow to re-read written data? This looks like an artificial restriction
of the underlying resources. An alternative not modifiying these two
implementations would be to let the TIFF output use a variant of
IOVecOutput that allows re-reading and forward all data at the very end
to the passed-in proxy.

Is there some policy to add assertions in all methods that are not
supposed to be called? This would have helped here a lot to reduce the
test case.

I did not add an explicit test. I guess the proper way would be to
convert TIFF ouput to use the same I/O proxy scheme that is used by many
other plugins. Small repro case is attached.

[repro.cpp.txt](https://github.com/user-attachments/files/15886829/repro.cpp.txt)

---------

Signed-off-by: Joachim Reichel <43646584+jreichel-nvidia@users.noreply.github.com>
  • Loading branch information
jreichel-nvidia authored Jul 2, 2024
1 parent e211302 commit 24e82d0
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 7 deletions.
4 changes: 3 additions & 1 deletion src/include/OpenImageIO/filesystem.h
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,7 @@ class OIIO_UTIL_API IOProxy {
};


/// IOProxy subclass for reading or writing (but not both) that wraps C
/// IOProxy subclass for reading or writing (plus re-reading) that wraps C
/// stdio 'FILE'.
class OIIO_UTIL_API IOFile : public IOProxy {
public:
Expand Down Expand Up @@ -521,7 +521,9 @@ class OIIO_UTIL_API IOVecOutput : public IOProxy {
{
}
const char* proxytype() const override { return "vecoutput"; }
size_t read(void* buf, size_t size) override;
size_t write(const void* buf, size_t size) override;
size_t pread(void* buf, size_t size, int64_t offset) override;
size_t pwrite(const void* buf, size_t size, int64_t offset) override;
size_t size() const override { return m_buf.size(); }

Expand Down
23 changes: 19 additions & 4 deletions src/libutil/filesystem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1178,7 +1178,7 @@ Filesystem::IOFile::IOFile(string_view filename, Mode mode)
{
// Call Filesystem::fopen since it handles UTF-8 file paths on Windows,
// which std fopen does not.
m_file = Filesystem::fopen(m_filename, m_mode == Write ? "wb" : "rb");
m_file = Filesystem::fopen(m_filename, m_mode == Write ? "w+b" : "rb");
if (!m_file) {
m_mode = Closed;
int e = errno;
Expand Down Expand Up @@ -1230,7 +1230,7 @@ Filesystem::IOFile::seek(int64_t offset)
size_t
Filesystem::IOFile::read(void* buf, size_t size)
{
if (!m_file || !size || m_mode != Read)
if (!m_file || !size || m_mode == Closed)
return 0;
size_t r = fread(buf, 1, size, m_file);
m_pos += r;
Expand All @@ -1249,7 +1249,7 @@ Filesystem::IOFile::read(void* buf, size_t size)
size_t
Filesystem::IOFile::pread(void* buf, size_t size, int64_t offset)
{
if (!m_file || !size || offset < 0 || m_mode != Read)
if (!m_file || !size || offset < 0 || m_mode == Closed)
return 0;
#ifdef _WIN32
std::lock_guard<std::mutex> lock(m_mutex);
Expand Down Expand Up @@ -1317,6 +1317,14 @@ Filesystem::IOFile::flush() const



size_t
Filesystem::IOVecOutput::read(void* buf, size_t size)
{
size = pread(buf, size, m_pos);
m_pos += size;
return size;
}

size_t
Filesystem::IOVecOutput::write(const void* buf, size_t size)
{
Expand All @@ -1325,7 +1333,14 @@ Filesystem::IOVecOutput::write(const void* buf, size_t size)
return size;
}


size_t
Filesystem::IOVecOutput::pread(void* buf, size_t size, int64_t offset)
{
std::lock_guard<std::mutex> lock(m_mutex);
size = std::min(size, size_t(m_buf.size() - offset));
memcpy(buf, &m_buf[offset], size);
return size;
}

size_t
Filesystem::IOVecOutput::pwrite(const void* buf, size_t size, int64_t offset)
Expand Down
5 changes: 3 additions & 2 deletions src/tiff.imageio/tiffoutput.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -398,9 +398,10 @@ allval(const std::vector<T>& d, T v = T(0))


static tsize_t
writer_readproc(thandle_t, tdata_t, tsize_t)
writer_readproc(thandle_t handle, tdata_t data, tsize_t size)
{
return 0;
auto io = static_cast<Filesystem::IOProxy*>(handle);
return io->read(data, size);
}

static tsize_t
Expand Down

0 comments on commit 24e82d0

Please sign in to comment.