Fix for reading memory mapped files with DWA compression

[darbyjohnston]

Hi, I mentioned this issue at a TSC meeting a couple of months ago, sorry for the delay in creating a PR.

I use a custom class derived from Imf::IStream for reading .exr files that supports memory mapping on Windows and Linux/macOS. I noticed I was getting crashes when reading DWA compressed files, and after looking into it discovered that the DWA code was writing into read only data by casting away the const. I guess for regular I/O this didn't cause an issue since it is buffered, but it does cause a crash with memory mapped data. For example in Visual Studio:

Exception thrown: write access violation.

>	OpenEXR-3_2_d.dll!Imf_3_2::Xdr::read<Imf_3_2::CharPtrIO,char const *>(const char * & in, unsigned __int64 & v) Line 522	C++
 	OpenEXR-3_2_d.dll!Imf_3_2::DwaCompressor::uncompress(const char * inPtr, int inSize, Imath_3_1::Box<Imath_3_1::Vec2<int>> range, const char * & outPtr) Line 2260	C++
 	OpenEXR-3_2_d.dll!Imf_3_2::DwaCompressor::uncompress(const char * inPtr, int inSize, int minY, const char * & outPtr) Line 2218	C++
 	OpenEXR-3_2_d.dll!Imf_3_2::`anonymous namespace'::LineBufferTaskIIF::execute() Line 844	C++
 	IlmThread-3_2_d.dll!IlmThread_3_2::`anonymous namespace'::NullThreadPoolProvider::addTask(IlmThread_3_2::Task * t) Line 372	C++
 	IlmThread-3_2_d.dll!IlmThread_3_2::ThreadPool::addTask(IlmThread_3_2::Task * task) Line 700	C++
 	IlmThread-3_2_d.dll!IlmThread_3_2::ThreadPool::addGlobalTask(IlmThread_3_2::Task * task) Line 723	C++
 	OpenEXR-3_2_d.dll!Imf_3_2::ScanLineInputFile::readPixels(int scanLine1, int scanLine2) Line 1704	C++
 	OpenEXR-3_2_d.dll!Imf_3_2::InputFile::readPixels(int scanLine1, int scanLine2) Line 900	C++
 	OpenEXR-3_2_d.dll!Imf_3_2::InputPart::readPixels(int scanLine1, int scanLine2) Line 65	C++
 	OpenEXR-3_2_d.dll!Imf_3_2::RgbaInputFile::readPixels(int scanLine1, int scanLine2) Line 1356	C++
 	OpenEXRTest.exe!`anonymous namespace'::writeReadScanLines(const char * fileName, int width, int height, Imf_3_2::Compression compression, const Imf_3_2::Array2D<Imf_3_2::Rgba> & p1) Line 353	C++
 	OpenEXRTest.exe!testExistingStreams(const std::string & tempDir) Line 1007	C++
 	OpenEXRTest.exe!main(int argc, char * * argv) Line 245	C++
 	[External Code]	

This PR includes both a fix for the DWA code and adds support for memory mapped I/O to the tests. Specifically:

• Fix the DWA code by using a small temporary buffer
• Add memory map support to testExistingStreams
• Modify testExistingStreams to run the tests with all compression types
• Move the utility function WidenFilename() to ImfMisc.h/.cpp for use with Windows filenames
• Change WidenFilename() to use std::wstring_convert()

It would also be nice to change char* IStream::readMemoryMapped() to const char* IStream::readMemoryMapped() since the memory should be read only, or that could be a separate PR.

[meshula]

I imagine since the data is all going to be touched subsequently, this has no performance implication, and may improve things through cache prewarming?

[darbyjohnston]

It's a fairly small buffer, currently NUM_SIZES_SINGLE = 11, so 88 bytes?

[meshula]

This all looks good to me, and having the extra test coverage for memmapped files is great.

[darbyjohnston]

Thanks!

It looks like the CI is failing from a network timeout:

Warning: Failed to download action 'https://api.github.com/repos/actions/checkout/tarball/ac593985615ec2ede58e132d2e21d2b1cbd6127c'. Error: The SSL connection could not be established, see inner exception.
Warning: Back off 19.262 seconds before retry.
Warning: Failed to download action 'https://api.github.com/repos/actions/checkout/tarball/ac593985615ec2ede58e132d2e21d2b1cbd6127c'. Error: The SSL connection could not be established, see inner exception.
Warning: Back off 26.111 seconds before retry.
Error: The SSL connection could not be established, see inner exception.

[cary-ilm]

Thanks for the fix, and especially for the tests! Just to confirm, this should be ABI compatible, right? It looks like you've exposed a previously hidden symbol WidenFilename, but that's the only API/ABI change?

[darbyjohnston]

Yes, the only change to the API is exposing the WidenFilename() function. The function only has two lines of code and is sort of Windows specific, so it could also be left out of the public API and just copied to where it is needed.

[cary-ilm]

It looks like WidenFilename was already duplicated in test/OpenEXRTest/TestUtilFStream.h. Would you mind swapping that out for your version in ImfMisc.h? Better to reduce the duplication

[darbyjohnston]

Sure thing, I removed the function in test/OpenEXRTest/TestUtilFStream.h and updated the PR.

[cary-ilm]

LGTM, thanks!