From 999d37a93e57b58e4d79336fc687011c2185678c Mon Sep 17 00:00:00 2001 From: Larry Gritz Date: Thu, 29 Sep 2022 13:41:10 -0700 Subject: [PATCH] PROTOTYPE WIP: Add thread-safe read functions to input files The fact that OpenEXR read methods (readPixels, readTiles, etc.) all require a prior call to setFrameBuffer() prevents truly concurrent reads by multiple threads because the calling application needs to maintain a mutex that keeps any other threads from reading from the same file between the two function calls. This PR is aimed to implement a new set of API calls, variety of the readPixels et al. that take a `const FrameBuffer&` as parameter rather than relying on saved state, and thus allows truly concurrent reads by multiple threads -- if they use these new safe versions, obviously the old ones continue to be unsafe to use concurrently. It's a lot of work to do this right! And I'm not aiming to make it good right now, but only to minimally stake out the API and make it functional (even if inefficient) so that we are essentially "reserving" the API just in time for a 3.2 release, and then we can iterate on improving the implementation underneath in subsequent patches, without changing APIs or breaking ABI compatibility within the 3.2 family. At the moment, I'm just posting a subset of the work as a prototype so people can see where I'm going with it. I've implemented a simple design (just lock internally) for a couple classes. So I'm seeking feedback on which of the following options are preferred: 1. Flesh this out for ALL the classes and relevant methods ASAP in time for 3.2. 2. Since these are not virtual methods, adding them doesn't actually break the ABIs, so don't rush this, we can add them (all at once, or incrementally) in subsequent 3.2.x patch releases. 3. Just hold off on all of it until 3.3. Signed-off-by: Larry Gritz --- src/lib/OpenEXR/ImfInputFile.cpp | 31 +++++++++++++++++++++++ src/lib/OpenEXR/ImfInputFile.h | 12 +++++++++ src/lib/OpenEXR/ImfInputPart.h | 4 +++ src/lib/OpenEXR/ImfScanLineInputFile.cpp | 19 ++++++++++++++ src/lib/OpenEXR/ImfScanLineInputFile.h | 11 ++++++++ src/lib/OpenEXR/ImfTiledInputFile.h | 32 +++++++++++++++++++++++- src/lib/OpenEXR/ImfTiledInputPart.h | 22 ++++++++++++++++ 7 files changed, 130 insertions(+), 1 deletion(-) diff --git a/src/lib/OpenEXR/ImfInputFile.cpp b/src/lib/OpenEXR/ImfInputFile.cpp index 936075981f..ad8c6e1da8 100644 --- a/src/lib/OpenEXR/ImfInputFile.cpp +++ b/src/lib/OpenEXR/ImfInputFile.cpp @@ -899,12 +899,43 @@ InputFile::readPixels (int scanLine1, int scanLine2) } } +void +InputFile::readPixels (const FrameBuffer& frameBuffer, int scanLine1, int scanLine2) +{ + if (_data->compositor) + { +#if ILMTHREAD_THREADING_ENABLED + std::lock_guard lock (*_data); +#endif + setFrameBuffer (frameBuffer); + _data->compositor->readPixels (scanLine1, scanLine2); + } + else if (_data->isTiled) + { +#if ILMTHREAD_THREADING_ENABLED + std::lock_guard lock (*_data); +#endif + setFrameBuffer (frameBuffer); + bufferedReadPixels (_data, scanLine1, scanLine2); + } + else + { + _data->sFile->readPixels (frameBuffer, scanLine1, scanLine2); + } +} + void InputFile::readPixels (int scanLine) { readPixels (scanLine, scanLine); } +void +InputFile::readPixels (const FrameBuffer& frameBuffer, int scanLine) +{ + readPixels (frameBuffer, scanLine, scanLine); +} + void InputFile::rawPixelData ( int firstScanLine, const char*& pixelData, int& pixelDataSize) diff --git a/src/lib/OpenEXR/ImfInputFile.h b/src/lib/OpenEXR/ImfInputFile.h index 28ec427389..02f80cad05 100644 --- a/src/lib/OpenEXR/ImfInputFile.h +++ b/src/lib/OpenEXR/ImfInputFile.h @@ -147,6 +147,12 @@ class IMF_EXPORT_TYPE InputFile : public GenericInputFile // // readPixels(s) calls readPixels(s,s). // + // The readPixels flavors that are passed a `FrameBuffer&` are + // thread-safe when called at the same time as other threads make + // calls to readPixels(fb,...). The reading functions that rely + // on saved state from a prior call to setFrameBuffer() are NOT + // safe to call when multiple threads are using the same InputFile. + // //--------------------------------------------------------------- IMF_EXPORT @@ -154,6 +160,12 @@ class IMF_EXPORT_TYPE InputFile : public GenericInputFile IMF_EXPORT void readPixels (int scanLine); + IMF_EXPORT + void readPixels (const FrameBuffer& frameBuffer, int scanLine1, int scanLine2); + IMF_EXPORT + void readPixels (const FrameBuffer& frameBuffer, int scanLine); + + //---------------------------------------------- // Read a block of raw pixel data from the file, // without uncompressing it (this function is diff --git a/src/lib/OpenEXR/ImfInputPart.h b/src/lib/OpenEXR/ImfInputPart.h index 440cc80bed..7b7f7b3044 100644 --- a/src/lib/OpenEXR/ImfInputPart.h +++ b/src/lib/OpenEXR/ImfInputPart.h @@ -41,6 +41,10 @@ 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 readPixels (const FrameBuffer& frameBuffer, int scanLine); + IMF_EXPORT void rawPixelData ( int firstScanLine, const char*& pixelData, int& pixelDataSize); diff --git a/src/lib/OpenEXR/ImfScanLineInputFile.cpp b/src/lib/OpenEXR/ImfScanLineInputFile.cpp index adaaa9f86f..0d4efd46ef 100644 --- a/src/lib/OpenEXR/ImfScanLineInputFile.cpp +++ b/src/lib/OpenEXR/ImfScanLineInputFile.cpp @@ -1748,12 +1748,31 @@ ScanLineInputFile::readPixels (int scanLine1, int scanLine2) } } +void +ScanLineInputFile::readPixels (const FrameBuffer& frameBuffer, int scanLine1, int scanLine2) +{ + // FIXME: For now, this just locks, then calls setFrameBuffer() and + // readPixels(). It would be better to implement this function to avoid + // locking by not needing any saved framebuffer state. +#if ILMTHREAD_THREADING_ENABLED + std::lock_guard lock (*_data); +#endif + setFrameBuffer(frameBuffer); + readPixels(scanLine1, scanLine2); +} + void ScanLineInputFile::readPixels (int scanLine) { readPixels (scanLine, scanLine); } +void +ScanLineInputFile::readPixels (const FrameBuffer& frameBuffer, int scanLine) +{ + readPixels (frameBuffer, scanLine, scanLine); +} + void ScanLineInputFile::rawPixelData ( int firstScanLine, const char*& pixelData, int& pixelDataSize) diff --git a/src/lib/OpenEXR/ImfScanLineInputFile.h b/src/lib/OpenEXR/ImfScanLineInputFile.h index 9014fd3057..692ea3cb1e 100644 --- a/src/lib/OpenEXR/ImfScanLineInputFile.h +++ b/src/lib/OpenEXR/ImfScanLineInputFile.h @@ -136,6 +136,12 @@ class IMF_EXPORT_TYPE ScanLineInputFile : public GenericInputFile // If threading is enabled, readPixels (s1, s2) tries to perform // decopmression of multiple scanlines in parallel. // + // The readPixels flavors that are passed a `FrameBuffer&` are + // thread-safe when called at the same time as other threads make + // calls to readPixels(fb,...). The reading functions that rely + // on saved state from a prior call to setFrameBuffer() are NOT + // safe to call when multiple threads are using the same InputFile. + // //--------------------------------------------------------------- IMF_EXPORT @@ -143,6 +149,11 @@ class IMF_EXPORT_TYPE ScanLineInputFile : public GenericInputFile IMF_EXPORT void readPixels (int scanLine); + IMF_EXPORT + void readPixels (const FrameBuffer& frameBuffer, int scanLine1, int scanLine2); + IMF_EXPORT + void readPixels (const FrameBuffer& frameBuffer, int scanLine); + //---------------------------------------------- // Read a block of raw pixel data from the file, // without uncompressing it (this function is diff --git a/src/lib/OpenEXR/ImfTiledInputFile.h b/src/lib/OpenEXR/ImfTiledInputFile.h index d867b2deab..e53bc9cca3 100644 --- a/src/lib/OpenEXR/ImfTiledInputFile.h +++ b/src/lib/OpenEXR/ImfTiledInputFile.h @@ -317,6 +317,13 @@ class IMF_EXPORT_TYPE TiledInputFile : public GenericInputFile // Attempting to access a tile that is not present in the file // throws an InputExc exception. // + // The readTile/readTiles flavors that are passed a `FrameBuffer&` + // are thread-safe when called at the same time as other threads + // make calls to readTiles(fb,...). The reading functions that + // rely on saved state from a prior call to setFrameBuffer() are + // NOT safe to call when multiple threads are using the same + // TiledInputFile. + // //------------------------------------------------------------ IMF_EXPORT @@ -326,10 +333,33 @@ class IMF_EXPORT_TYPE TiledInputFile : public GenericInputFile IMF_EXPORT void readTiles (int dx1, int dx2, int dy1, int dy2, int lx, int ly); - IMF_EXPORT void readTiles (int dx1, int dx2, int dy1, int dy2, int l = 0); + IMF_EXPORT + void readTile (const FrameBuffer& frameBuffer, int dx, int dy, int l = 0); + IMF_EXPORT + void + readTile (const FrameBuffer& frameBuffer, int dx, int dy, int lx, int ly); + + IMF_EXPORT + void readTiles ( + const FrameBuffer& frameBuffer, + int dx1, + int dx2, + int dy1, + int dy2, + int lx, + int ly); + IMF_EXPORT + void readTiles ( + const FrameBuffer& frameBuffer, + int dx1, + int dx2, + int dy1, + int dy2, + int l = 0); + //-------------------------------------------------- // Read a tile of raw pixel data from the file, // without uncompressing it (this function is diff --git a/src/lib/OpenEXR/ImfTiledInputPart.h b/src/lib/OpenEXR/ImfTiledInputPart.h index fafe7353fa..e1ee4cb093 100644 --- a/src/lib/OpenEXR/ImfTiledInputPart.h +++ b/src/lib/OpenEXR/ImfTiledInputPart.h @@ -79,6 +79,28 @@ class IMF_EXPORT_TYPE TiledInputPart IMF_EXPORT void readTiles (int dx1, int dx2, int dy1, int dy2, int l = 0); IMF_EXPORT + void readTile (const FrameBuffer& frameBuffer, int dx, int dy, int l = 0); + IMF_EXPORT + void + readTile (const FrameBuffer& frameBuffer, int dx, int dy, int lx, int ly); + IMF_EXPORT + void readTiles ( + const FrameBuffer& frameBuffer, + int dx1, + int dx2, + int dy1, + int dy2, + int lx, + int ly); + IMF_EXPORT + void readTiles ( + const FrameBuffer& frameBuffer, + int dx1, + int dx2, + int dy1, + int dy2, + int l = 0); + IMF_EXPORT void rawTileData ( int& dx, int& dy,