Skip to content

Commit

Permalink
PROTOTYPE WIP: Add thread-safe read functions to input files
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@lgritz
lgritz committed Sep 29, 2022
1 parent cc508f7 commit 999d37a
Show file tree
Hide file tree
Showing 7 changed files with 130 additions and 1 deletion.
31 changes: 31 additions & 0 deletions src/lib/OpenEXR/ImfInputFile.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::mutex> lock (*_data);
#endif
setFrameBuffer (frameBuffer);
_data->compositor->readPixels (scanLine1, scanLine2);
}
else if (_data->isTiled)
{
#if ILMTHREAD_THREADING_ENABLED
std::lock_guard<std::mutex> 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)
Expand Down
12 changes: 12 additions & 0 deletions src/lib/OpenEXR/ImfInputFile.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,13 +147,25 @@ 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
void readPixels (int scanLine1, int scanLine2);
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
Expand Down
4 changes: 4 additions & 0 deletions src/lib/OpenEXR/ImfInputPart.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
19 changes: 19 additions & 0 deletions src/lib/OpenEXR/ImfScanLineInputFile.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::mutex> 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)
Expand Down
11 changes: 11 additions & 0 deletions src/lib/OpenEXR/ImfScanLineInputFile.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,13 +136,24 @@ 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
void readPixels (int scanLine1, int scanLine2);
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
Expand Down
32 changes: 31 additions & 1 deletion src/lib/OpenEXR/ImfTiledInputFile.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
22 changes: 22 additions & 0 deletions src/lib/OpenEXR/ImfTiledInputPart.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit 999d37a

Please sign in to comment.