From 8df539b9d429809eb17a3fdb5211c0f0c75712b9 Mon Sep 17 00:00:00 2001 From: Kimball Thurston Date: Mon, 23 Sep 2024 23:55:23 +1200 Subject: [PATCH 1/3] Add initial api to allow reading streams to be thread safe Signed-off-by: Kimball Thurston --- src/lib/OpenEXR/ImfIO.cpp | 19 ++++++++++++++++ src/lib/OpenEXR/ImfIO.h | 48 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/src/lib/OpenEXR/ImfIO.cpp b/src/lib/OpenEXR/ImfIO.cpp index c5bf8294c2..e11ee83998 100644 --- a/src/lib/OpenEXR/ImfIO.cpp +++ b/src/lib/OpenEXR/ImfIO.cpp @@ -50,6 +50,25 @@ IStream::fileName () const return _fileName.c_str (); } +int64_t +IStream::size () +{ + return -1; +} + +bool +IStream::isStatelessRead () const +{ + return false; +} + +int64_t +IStream::read (void *, uint64_t, uint64_t) +{ + throw IEX_NAMESPACE::InputExc ("Attempt to perform a stateless read " + "on a stream without support."); +} + OStream::OStream (const char fileName[]) : _fileName (fileName) { // empty diff --git a/src/lib/OpenEXR/ImfIO.h b/src/lib/OpenEXR/ImfIO.h index 1008e639e2..c1540c9af3 100644 --- a/src/lib/OpenEXR/ImfIO.h +++ b/src/lib/OpenEXR/ImfIO.h @@ -95,6 +95,54 @@ class IMF_EXPORT_TYPE IStream IMF_EXPORT const char* fileName () const; + //------------------------------------------------------ + // Get the size of the file (or buffer) associated with + // this stream. + // + // by default, this will return -1. That value will skip a few + // safety checks. However, if you provide the size, it will apply + // a number of file consistency checks as the file is read. + // ------------------------------------------------------ + + IMF_EXPORT virtual int64_t size (); + + //------------------------------------------------- + // Does this input stream support stateless reading? + // + // Stateless reading allows multiple threads to + // read from the stream concurrently from different + // locations in the file + //------------------------------------------------- + + IMF_EXPORT virtual bool isStatelessRead () const; + + //------------------------------------------------------ + // Read from the stream with an offset: + // + // read(b,s,o) should read up to sz bytes from the + // stream using something like pread or ReadFileEx with + // overlapped data at the provided offset in the stream. + // + // for this function, the buffer size requested may be + // either larger than the file or request a read past + // the end of the file. This should NOT be treated as + // an error - the library will handle whether that is + // an error (if the offset is past the end, it should + // read 0) + // + // If there is an error, it should either return -1 + // or throw an exception (an exception could provide + // a message). + // + // This will only be used if isStatelessRead returns true. + // + // NB: It is expected that this is thread safe such + // that multiple threads can be reading from the stream + // at the same time + //------------------------------------------------------ + + IMF_EXPORT virtual int64_t read (void *buf, uint64_t sz, uint64_t offset); + protected: IMF_EXPORT IStream (const char fileName[]); From e6014fce97926ba684033fb9faa46ee348a6e01a Mon Sep 17 00:00:00 2001 From: Kimball Thurston Date: Mon, 23 Sep 2024 23:55:43 +1200 Subject: [PATCH 2/3] honor thread-safe stream api if available Signed-off-by: Kimball Thurston --- src/lib/OpenEXR/ImfContextInit.cpp | 61 +++++++++++++++++++++++++++--- 1 file changed, 56 insertions(+), 5 deletions(-) diff --git a/src/lib/OpenEXR/ImfContextInit.cpp b/src/lib/OpenEXR/ImfContextInit.cpp index 5e0355a2ae..d2a3309882 100644 --- a/src/lib/OpenEXR/ImfContextInit.cpp +++ b/src/lib/OpenEXR/ImfContextInit.cpp @@ -26,7 +26,18 @@ struct istream_holder }; static int64_t -istream_read ( +istream_size ( + exr_const_context_t ctxt, + void* userdata) +{ + istream_holder* ih = static_cast (userdata); + IStream* s = ih->_stream; + + return s->size (); +} + +static int64_t +istream_threadsafe_read ( exr_const_context_t ctxt, void* userdata, void* buffer, @@ -36,6 +47,45 @@ istream_read ( { istream_holder* ih = static_cast (userdata); IStream* s = ih->_stream; + int64_t nread = -1; + + try + { + nread = s->read (buffer, sz, offset); + } + catch (std::exception &e) + { + error_cb ( + ctxt, + EXR_ERR_READ_IO, + "Unable to seek to desired offset %" PRIu64 ": %s", + offset, + e.what()); + nread = -1; + } + catch (...) + { + error_cb ( + ctxt, + EXR_ERR_READ_IO, + "Unable to seek to desired offset %" PRIu64 ": Unknown error", + offset); + nread = -1; + } + return nread; +} + +static int64_t +istream_nonparallel_read ( + exr_const_context_t ctxt, + void* userdata, + void* buffer, + uint64_t sz, + uint64_t offset, + exr_stream_error_func_ptr_t error_cb) +{ + istream_holder* ih = static_cast (userdata); + IStream* s = ih->_stream; if (sz > INT_MAX) { @@ -121,10 +171,11 @@ ContextInitializer& ContextInitializer::setInputStream (IStream* istr) { _initializer.user_data = new istream_holder{istr}; - _initializer.read_fn = istream_read; - // TODO: add query to io streams to ask size if possible (such - // that ifstream can return size, others can return -1) - _initializer.size_fn = nullptr; //istream_guess_size; + if (istr->isStatelessRead ()) + _initializer.read_fn = istream_threadsafe_read; + else + _initializer.read_fn = istream_nonparallel_read; + _initializer.size_fn = istream_size; _initializer.write_fn = nullptr; _initializer.destroy_fn = istream_destroy; _ctxt_type = ContextFileType::READ; From c12700a13428a61950ad08f0a0efd0fabbdd52c0 Mon Sep 17 00:00:00 2001 From: Kimball Thurston Date: Mon, 23 Sep 2024 23:57:26 +1200 Subject: [PATCH 3/3] Initial api to provide stateless read api for scanline things This enables reading of scanlines in a "stateless" manner, where a framebuffer does not have to be stored, then the call to readPixels made. Signed-off-by: Kimball Thurston --- src/lib/OpenEXR/ImfInputFile.cpp | 40 ++++++++++++++++++++++++ src/lib/OpenEXR/ImfInputFile.h | 21 +++++++++++++ src/lib/OpenEXR/ImfInputPart.cpp | 7 +++++ src/lib/OpenEXR/ImfInputPart.h | 3 ++ src/lib/OpenEXR/ImfMultiPartInputFile.h | 2 +- src/lib/OpenEXR/ImfScanLineInputFile.cpp | 7 +++++ src/lib/OpenEXR/ImfScanLineInputFile.h | 12 +++++++ 7 files changed, 91 insertions(+), 1 deletion(-) diff --git a/src/lib/OpenEXR/ImfInputFile.cpp b/src/lib/OpenEXR/ImfInputFile.cpp index 2f984bb4c2..7dedf2327c 100644 --- a/src/lib/OpenEXR/ImfInputFile.cpp +++ b/src/lib/OpenEXR/ImfInputFile.cpp @@ -77,10 +77,14 @@ struct InputFile::Data } void setFrameBuffer (const FrameBuffer& frameBuffer); + void lockedSetFrameBuffer (const FrameBuffer& frameBuffer); void readPixels (int scanline1, int scanline2); void bufferedReadPixels (int scanline1, int scanline2); + void readPixels ( + const FrameBuffer& frameBuffer, int scanline1, int scanline2); + void deleteCachedBuffer (void); void copyCachedBuffer (FrameBuffer::ConstIterator to, FrameBuffer::ConstIterator from, @@ -224,6 +228,13 @@ InputFile::readPixels (int scanLine) _data->readPixels (scanLine, scanLine); } +void +InputFile::readPixels ( + const FrameBuffer& frameBuffer, int scanLine1, int scanLine2) +{ + _data->readPixels (frameBuffer, scanLine1, scanLine2); +} + void InputFile::rawPixelData ( int firstScanLine, const char*& pixelData, int& pixelDataSize) @@ -317,7 +328,12 @@ InputFile::Data::setFrameBuffer (const FrameBuffer& frameBuffer) #if ILMTHREAD_THREADING_ENABLED std::lock_guard lk (_mx); #endif + lockedSetFrameBuffer (frameBuffer); +} +void +InputFile::Data::lockedSetFrameBuffer (const FrameBuffer& frameBuffer) +{ if (_storage == EXR_STORAGE_TILED) { // @@ -430,6 +446,30 @@ InputFile::Data::readPixels (int scanLine1, int scanLine2) else { _sFile->readPixels (scanLine1, scanLine2); } } +void +InputFile::Data::readPixels ( + const FrameBuffer& frameBuffer, int scanLine1, int scanLine2) +{ + if (_compositor) + { +#if ILMTHREAD_THREADING_ENABLED + std::lock_guard lock (_mx); +#endif + _compositor->setFrameBuffer (frameBuffer); + _compositor->readPixels (scanLine1, scanLine2); + } + else if (_storage == EXR_STORAGE_TILED) + { +#if ILMTHREAD_THREADING_ENABLED + std::lock_guard lock (_mx); +#endif + + lockedSetFrameBuffer (frameBuffer); + bufferedReadPixels (scanLine1, scanLine2); + } + else { _sFile->readPixels (frameBuffer, scanLine1, scanLine2); } +} + void InputFile::Data::bufferedReadPixels (int scanLine1, int scanLine2) { diff --git a/src/lib/OpenEXR/ImfInputFile.h b/src/lib/OpenEXR/ImfInputFile.h index 3e9da65c8d..f5d67ddb32 100644 --- a/src/lib/OpenEXR/ImfInputFile.h +++ b/src/lib/OpenEXR/ImfInputFile.h @@ -200,6 +200,27 @@ class IMF_EXPORT_TYPE InputFile IMF_EXPORT void readPixels (int scanLine); + //---------------------------------------------- + // Combines the setFrameBuffer and readPixels into a singular + // call. This does more than that in that it can, with the right + // conditions, not require a lock on the file, such that multiple + // (external to OpenEXR) threads can read at the same time on + // different framebuffers + // + // NB: if the underlying file is deep or tiled, that requires + // translation, so will not do the pass through, but will behave + // in a threadsafe manner (where the only way that was possible + // before was to have a larger framebuffer, set the framebuffer + // once, then call readPixels by the external threads, although + // that occured with a mutex and so the reads were serialized. + // There are reasons why that might still be serialized, such as a + // non-threadable stream. + //---------------------------------------------- + + IMF_EXPORT + void readPixels ( + const FrameBuffer& frameBuffer, int scanLine1, int scanLine2); + //---------------------------------------------- // Read a block of raw pixel data from the file, // without uncompressing it (this function is diff --git a/src/lib/OpenEXR/ImfInputPart.cpp b/src/lib/OpenEXR/ImfInputPart.cpp index acfd20db26..8b8d4ef952 100644 --- a/src/lib/OpenEXR/ImfInputPart.cpp +++ b/src/lib/OpenEXR/ImfInputPart.cpp @@ -70,6 +70,13 @@ InputPart::readPixels (int scanLine) file->readPixels (scanLine); } +void +InputPart::readPixels ( + const FrameBuffer& frameBuffer, int scanLine1, int scanLine2) +{ + file->readPixels (frameBuffer, scanLine1, scanLine2); +} + void InputPart::rawPixelData ( int firstScanLine, const char*& pixelData, int& pixelDataSize) diff --git a/src/lib/OpenEXR/ImfInputPart.h b/src/lib/OpenEXR/ImfInputPart.h index 440cc80bed..c25a4fa63f 100644 --- a/src/lib/OpenEXR/ImfInputPart.h +++ b/src/lib/OpenEXR/ImfInputPart.h @@ -41,6 +41,9 @@ class IMF_EXPORT_TYPE InputPart IMF_EXPORT void readPixels (int scanLine); IMF_EXPORT + void readPixels ( + const FrameBuffer& frameBuffer, int scanLine1, int scanLine2); + IMF_EXPORT void rawPixelData ( int firstScanLine, const char*& pixelData, int& pixelDataSize); diff --git a/src/lib/OpenEXR/ImfMultiPartInputFile.h b/src/lib/OpenEXR/ImfMultiPartInputFile.h index 22911ed3b1..0b1fc07a5e 100644 --- a/src/lib/OpenEXR/ImfMultiPartInputFile.h +++ b/src/lib/OpenEXR/ImfMultiPartInputFile.h @@ -99,7 +99,7 @@ class IMF_EXPORT_TYPE MultiPartInputFile // // used internally by 'Part' types to access individual parts of the multipart file // - // TODO: change these to value / reference semantics + // TODO: change these to value / reference semantics (smart ptr) template IMF_HIDDEN T* getInputPart (int partNumber); IMF_HIDDEN InputPartData* getPart (int) const; diff --git a/src/lib/OpenEXR/ImfScanLineInputFile.cpp b/src/lib/OpenEXR/ImfScanLineInputFile.cpp index f7eb144701..a5b3863bce 100644 --- a/src/lib/OpenEXR/ImfScanLineInputFile.cpp +++ b/src/lib/OpenEXR/ImfScanLineInputFile.cpp @@ -312,6 +312,13 @@ ScanLineInputFile::readPixels (int scanLine1, int scanLine2) _data->readPixels (frameBuffer (), scanLine1, scanLine2); } +void +ScanLineInputFile::readPixels ( + const FrameBuffer& frame, int scanLine1, int scanLine2) +{ + _data->readPixels (frame, scanLine1, scanLine2); +} + //////////////////////////////////////// void diff --git a/src/lib/OpenEXR/ImfScanLineInputFile.h b/src/lib/OpenEXR/ImfScanLineInputFile.h index c4e5ee5504..f9b5645039 100644 --- a/src/lib/OpenEXR/ImfScanLineInputFile.h +++ b/src/lib/OpenEXR/ImfScanLineInputFile.h @@ -140,6 +140,18 @@ class IMF_EXPORT_TYPE ScanLineInputFile IMF_EXPORT void readPixels (int scanLine); + //---------------------------------------------- + // Combines the setFrameBuffer and readPixels into a singular + // call. This does more than that in that it can, with the right + // conditions, not require a lock on the file, such that multiple + // (external to OpenEXR) threads can read at the same time on + // different framebuffers + //---------------------------------------------- + + IMF_EXPORT + void readPixels ( + const FrameBuffer& frame, int scanLine1, int scanLine2); + //---------------------------------------------- // Read a block of raw pixel data from the file, // without uncompressing it (this function is