Skip to content

Commit 999d37a

Browse files
lgritzlgritz
committed
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 <[email protected]>
1 parent cc508f7 commit 999d37a

7 files changed

+130
-1
lines changed

‎src/lib/OpenEXR/ImfInputFile.cpp

+31
Original file line numberDiff line numberDiff line change
@@ -899,12 +899,43 @@ InputFile::readPixels (int scanLine1, int scanLine2)
899899
}
900900
}
901901

902+
void
903+
InputFile::readPixels (const FrameBuffer& frameBuffer, int scanLine1, int scanLine2)
904+
{
905+
if (_data->compositor)
906+
{
907+
#if ILMTHREAD_THREADING_ENABLED
908+
std::lock_guard<std::mutex> lock (*_data);
909+
#endif
910+
setFrameBuffer (frameBuffer);
911+
_data->compositor->readPixels (scanLine1, scanLine2);
912+
}
913+
else if (_data->isTiled)
914+
{
915+
#if ILMTHREAD_THREADING_ENABLED
916+
std::lock_guard<std::mutex> lock (*_data);
917+
#endif
918+
setFrameBuffer (frameBuffer);
919+
bufferedReadPixels (_data, scanLine1, scanLine2);
920+
}
921+
else
922+
{
923+
_data->sFile->readPixels (frameBuffer, scanLine1, scanLine2);
924+
}
925+
}
926+
902927
void
903928
InputFile::readPixels (int scanLine)
904929
{
905930
readPixels (scanLine, scanLine);
906931
}
907932

933+
void
934+
InputFile::readPixels (const FrameBuffer& frameBuffer, int scanLine)
935+
{
936+
readPixels (frameBuffer, scanLine, scanLine);
937+
}
938+
908939
void
909940
InputFile::rawPixelData (
910941
int firstScanLine, const char*& pixelData, int& pixelDataSize)

‎src/lib/OpenEXR/ImfInputFile.h

+12
Original file line numberDiff line numberDiff line change
@@ -147,13 +147,25 @@ class IMF_EXPORT_TYPE InputFile : public GenericInputFile
147147
//
148148
// readPixels(s) calls readPixels(s,s).
149149
//
150+
// The readPixels flavors that are passed a `FrameBuffer&` are
151+
// thread-safe when called at the same time as other threads make
152+
// calls to readPixels(fb,...). The reading functions that rely
153+
// on saved state from a prior call to setFrameBuffer() are NOT
154+
// safe to call when multiple threads are using the same InputFile.
155+
//
150156
//---------------------------------------------------------------
151157

152158
IMF_EXPORT
153159
void readPixels (int scanLine1, int scanLine2);
154160
IMF_EXPORT
155161
void readPixels (int scanLine);
156162

163+
IMF_EXPORT
164+
void readPixels (const FrameBuffer& frameBuffer, int scanLine1, int scanLine2);
165+
IMF_EXPORT
166+
void readPixels (const FrameBuffer& frameBuffer, int scanLine);
167+
168+
157169
//----------------------------------------------
158170
// Read a block of raw pixel data from the file,
159171
// without uncompressing it (this function is

‎src/lib/OpenEXR/ImfInputPart.h

+4
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,10 @@ class IMF_EXPORT_TYPE InputPart
4141
IMF_EXPORT
4242
void readPixels (int scanLine);
4343
IMF_EXPORT
44+
void readPixels (const FrameBuffer& frameBuffer, int scanLine1, int scanLine2);
45+
IMF_EXPORT
46+
void readPixels (const FrameBuffer& frameBuffer, int scanLine);
47+
IMF_EXPORT
4448
void rawPixelData (
4549
int firstScanLine, const char*& pixelData, int& pixelDataSize);
4650

‎src/lib/OpenEXR/ImfScanLineInputFile.cpp

+19
Original file line numberDiff line numberDiff line change
@@ -1748,12 +1748,31 @@ ScanLineInputFile::readPixels (int scanLine1, int scanLine2)
17481748
}
17491749
}
17501750

1751+
void
1752+
ScanLineInputFile::readPixels (const FrameBuffer& frameBuffer, int scanLine1, int scanLine2)
1753+
{
1754+
// FIXME: For now, this just locks, then calls setFrameBuffer() and
1755+
// readPixels(). It would be better to implement this function to avoid
1756+
// locking by not needing any saved framebuffer state.
1757+
#if ILMTHREAD_THREADING_ENABLED
1758+
std::lock_guard<std::mutex> lock (*_data);
1759+
#endif
1760+
setFrameBuffer(frameBuffer);
1761+
readPixels(scanLine1, scanLine2);
1762+
}
1763+
17511764
void
17521765
ScanLineInputFile::readPixels (int scanLine)
17531766
{
17541767
readPixels (scanLine, scanLine);
17551768
}
17561769

1770+
void
1771+
ScanLineInputFile::readPixels (const FrameBuffer& frameBuffer, int scanLine)
1772+
{
1773+
readPixels (frameBuffer, scanLine, scanLine);
1774+
}
1775+
17571776
void
17581777
ScanLineInputFile::rawPixelData (
17591778
int firstScanLine, const char*& pixelData, int& pixelDataSize)

‎src/lib/OpenEXR/ImfScanLineInputFile.h

+11
Original file line numberDiff line numberDiff line change
@@ -136,13 +136,24 @@ class IMF_EXPORT_TYPE ScanLineInputFile : public GenericInputFile
136136
// If threading is enabled, readPixels (s1, s2) tries to perform
137137
// decopmression of multiple scanlines in parallel.
138138
//
139+
// The readPixels flavors that are passed a `FrameBuffer&` are
140+
// thread-safe when called at the same time as other threads make
141+
// calls to readPixels(fb,...). The reading functions that rely
142+
// on saved state from a prior call to setFrameBuffer() are NOT
143+
// safe to call when multiple threads are using the same InputFile.
144+
//
139145
//---------------------------------------------------------------
140146

141147
IMF_EXPORT
142148
void readPixels (int scanLine1, int scanLine2);
143149
IMF_EXPORT
144150
void readPixels (int scanLine);
145151

152+
IMF_EXPORT
153+
void readPixels (const FrameBuffer& frameBuffer, int scanLine1, int scanLine2);
154+
IMF_EXPORT
155+
void readPixels (const FrameBuffer& frameBuffer, int scanLine);
156+
146157
//----------------------------------------------
147158
// Read a block of raw pixel data from the file,
148159
// without uncompressing it (this function is

‎src/lib/OpenEXR/ImfTiledInputFile.h

+31-1
Original file line numberDiff line numberDiff line change
@@ -317,6 +317,13 @@ class IMF_EXPORT_TYPE TiledInputFile : public GenericInputFile
317317
// Attempting to access a tile that is not present in the file
318318
// throws an InputExc exception.
319319
//
320+
// The readTile/readTiles flavors that are passed a `FrameBuffer&`
321+
// are thread-safe when called at the same time as other threads
322+
// make calls to readTiles(fb,...). The reading functions that
323+
// rely on saved state from a prior call to setFrameBuffer() are
324+
// NOT safe to call when multiple threads are using the same
325+
// TiledInputFile.
326+
//
320327
//------------------------------------------------------------
321328

322329
IMF_EXPORT
@@ -326,10 +333,33 @@ class IMF_EXPORT_TYPE TiledInputFile : public GenericInputFile
326333

327334
IMF_EXPORT
328335
void readTiles (int dx1, int dx2, int dy1, int dy2, int lx, int ly);
329-
330336
IMF_EXPORT
331337
void readTiles (int dx1, int dx2, int dy1, int dy2, int l = 0);
332338

339+
IMF_EXPORT
340+
void readTile (const FrameBuffer& frameBuffer, int dx, int dy, int l = 0);
341+
IMF_EXPORT
342+
void
343+
readTile (const FrameBuffer& frameBuffer, int dx, int dy, int lx, int ly);
344+
345+
IMF_EXPORT
346+
void readTiles (
347+
const FrameBuffer& frameBuffer,
348+
int dx1,
349+
int dx2,
350+
int dy1,
351+
int dy2,
352+
int lx,
353+
int ly);
354+
IMF_EXPORT
355+
void readTiles (
356+
const FrameBuffer& frameBuffer,
357+
int dx1,
358+
int dx2,
359+
int dy1,
360+
int dy2,
361+
int l = 0);
362+
333363
//--------------------------------------------------
334364
// Read a tile of raw pixel data from the file,
335365
// without uncompressing it (this function is

‎src/lib/OpenEXR/ImfTiledInputPart.h

+22
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,28 @@ class IMF_EXPORT_TYPE TiledInputPart
7979
IMF_EXPORT
8080
void readTiles (int dx1, int dx2, int dy1, int dy2, int l = 0);
8181
IMF_EXPORT
82+
void readTile (const FrameBuffer& frameBuffer, int dx, int dy, int l = 0);
83+
IMF_EXPORT
84+
void
85+
readTile (const FrameBuffer& frameBuffer, int dx, int dy, int lx, int ly);
86+
IMF_EXPORT
87+
void readTiles (
88+
const FrameBuffer& frameBuffer,
89+
int dx1,
90+
int dx2,
91+
int dy1,
92+
int dy2,
93+
int lx,
94+
int ly);
95+
IMF_EXPORT
96+
void readTiles (
97+
const FrameBuffer& frameBuffer,
98+
int dx1,
99+
int dx2,
100+
int dy1,
101+
int dy2,
102+
int l = 0);
103+
IMF_EXPORT
82104
void rawTileData (
83105
int& dx,
84106
int& dy,

0 commit comments

Comments
 (0)