From 2677796b0cda577404553f566009407a4abc6202 Mon Sep 17 00:00:00 2001 From: Paul Licameli Date: Thu, 22 Dec 2016 12:30:07 -0500 Subject: [PATCH] Zero and return false for all failures to read block file summary... ... Though in the only place where these summaries are used, which is Sequence::GetWaveDisplay, we ignore the correctly reported error code anyway. Also RAII in management of relevant memory buffers and mutexes. --- src/BlockFile.cpp | 108 ++++++++++++++------------ src/BlockFile.h | 4 +- src/Sequence.cpp | 4 + src/blockfile/LegacyBlockFile.cpp | 21 +++-- src/blockfile/LegacyBlockFile.h | 2 +- src/blockfile/ODDecodeBlockFile.cpp | 21 ++--- src/blockfile/ODDecodeBlockFile.h | 2 +- src/blockfile/ODPCMAliasBlockFile.cpp | 45 ++++++----- src/blockfile/ODPCMAliasBlockFile.h | 2 +- src/blockfile/SilentBlockFile.cpp | 5 +- src/blockfile/SilentBlockFile.h | 2 +- src/blockfile/SimpleBlockFile.cpp | 32 ++++---- src/blockfile/SimpleBlockFile.h | 2 +- 13 files changed, 140 insertions(+), 110 deletions(-) diff --git a/src/BlockFile.cpp b/src/BlockFile.cpp index 671eadf62..fcb3f9d2f 100644 --- a/src/BlockFile.cpp +++ b/src/BlockFile.cpp @@ -424,6 +424,8 @@ void BlockFile::GetMinMax(float *outMin, float *outMax, float *outRMS) const /// data provides information about the minimum value, the maximum /// value, and the maximum RMS value for every group of 256 samples in the /// file. +/// Fill with zeroes and return false if data are unavailable for any reason. +/// /// /// @param *buffer The area where the summary information will be /// written. It must be at least len*3 long. @@ -434,54 +436,17 @@ bool BlockFile::Read256(float *buffer, { wxASSERT(start >= 0); - ArrayOf summary{ mSummaryInfo.totalSummaryBytes }; - // FIXME: TRAP_ERR ReadSummary() could return fail. - this->ReadSummary(summary.get()); + ArrayOf< char > summary; + // In case of failure, summary is filled with zeroes + auto result = this->ReadSummary(summary); start = std::min( start, mSummaryInfo.frames256 ); len = std::min( len, mSummaryInfo.frames256 - start ); - CopySamples(summary.get() + mSummaryInfo.offset256 + (start * mSummaryInfo.bytesPerFrame), - mSummaryInfo.format, - (samplePtr)buffer, floatSample, len * mSummaryInfo.fields); - - if (mSummaryInfo.fields == 2) { - // No RMS info - for(auto i = len; i--;) { - buffer[3*i+2] = (fabs(buffer[2*i]) + fabs(buffer[2*i+1]))/4.0; - buffer[3*i+1] = buffer[2*i+1]; - buffer[3*i] = buffer[2*i]; - } - } - - return true; -} - -/// Retrieves a portion of the 64K summary buffer from this BlockFile. This -/// data provides information about the minimum value, the maximum -/// value, and the maximum RMS value for every group of 64K samples in the -/// file. -/// -/// @param *buffer The area where the summary information will be -/// written. It must be at least len*3 long. -/// @param start The offset in 64K-sample increments -/// @param len The number of 64K-sample summary frames to read -bool BlockFile::Read64K(float *buffer, - size_t start, size_t len) -{ - wxASSERT(start >= 0); - - ArrayOf summary{ mSummaryInfo.totalSummaryBytes }; - // FIXME: TRAP_ERR ReadSummary() could return fail. - this->ReadSummary(summary.get()); - - start = std::min( start, mSummaryInfo.frames64K ); - len = std::min( len, mSummaryInfo.frames64K - start ); - - CopySamples(summary.get() + mSummaryInfo.offset64K + + CopySamples(summary.get() + mSummaryInfo.offset256 + (start * mSummaryInfo.bytesPerFrame), mSummaryInfo.format, - (samplePtr)buffer, floatSample, len*mSummaryInfo.fields); + (samplePtr)buffer, floatSample, len * mSummaryInfo.fields); if (mSummaryInfo.fields == 2) { // No RMS info; make guess @@ -492,7 +457,46 @@ bool BlockFile::Read64K(float *buffer, } } - return true; + return result; +} + +/// Retrieves a portion of the 64K summary buffer from this BlockFile. This +/// data provides information about the minimum value, the maximum +/// value, and the maximum RMS value for every group of 64K samples in the +/// file. +/// Fill with zeroes and return false if data are unavailable for any reason. +/// +/// @param *buffer The area where the summary information will be +/// written. It must be at least len*3 long. +/// @param start The offset in 64K-sample increments +/// @param len The number of 64K-sample summary frames to read +bool BlockFile::Read64K(float *buffer, + size_t start, size_t len) +{ + wxASSERT(start >= 0); + + ArrayOf< char > summary; + // In case of failure, summary is filled with zeroes + auto result = this->ReadSummary(summary); + + start = std::min( start, mSummaryInfo.frames64K ); + len = std::min( len, mSummaryInfo.frames64K - start ); + + CopySamples(summary.get() + mSummaryInfo.offset64K + + (start * mSummaryInfo.bytesPerFrame), + mSummaryInfo.format, + (samplePtr)buffer, floatSample, len * mSummaryInfo.fields); + + if (mSummaryInfo.fields == 2) { + // No RMS info; make guess + for(auto i = len; i--;) { + buffer[3*i+2] = (fabs(buffer[2*i]) + fabs(buffer[2*i+1]))/4.0; + buffer[3*i+1] = buffer[2*i+1]; + buffer[3*i] = buffer[2*i]; + } + } + + return result; } size_t BlockFile::CommonReadData( @@ -714,11 +718,13 @@ AliasBlockFile::~AliasBlockFile() /// Read the summary of this alias block from disk. Since the audio data /// is elsewhere, this consists of reading the entire summary file. +/// Fill with zeroes and return false if data are unavailable for any reason. /// /// @param *data The buffer where the summary data will be stored. It must /// be at least mSummaryInfo.totalSummaryBytes long. -bool AliasBlockFile::ReadSummary(void *data) +bool AliasBlockFile::ReadSummary(ArrayOf &data) { + data.reinit( mSummaryInfo.totalSummaryBytes ); wxFFile summaryFile(mFileName.GetFullPath(), wxT("rb")); { @@ -729,24 +735,28 @@ bool AliasBlockFile::ReadSummary(void *data) if (!summaryFile.IsOpened()){ // NEW model; we need to return valid data - memset(data, 0, mSummaryInfo.totalSummaryBytes); + memset(data.get(), 0, mSummaryInfo.totalSummaryBytes); // we silence the logging for this operation in this object // after first occurrence of error; it's already reported and // spewing at the user will complicate the user's ability to // deal mSilentLog = TRUE; - return true; + return false; } else mSilentLog = FALSE; // worked properly, any future error is NEW } - auto read = summaryFile.Read(data, mSummaryInfo.totalSummaryBytes); + auto read = summaryFile.Read(data.get(), mSummaryInfo.totalSummaryBytes); + if (read != mSummaryInfo.totalSummaryBytes) { + memset(data.get(), 0, mSummaryInfo.totalSummaryBytes); + return false; + } - FixSummary(data); + FixSummary(data.get()); - return (read == mSummaryInfo.totalSummaryBytes); + return true; } /// Modify this block to point at a different file. This is generally diff --git a/src/BlockFile.h b/src/BlockFile.h index a45d0c4de..5c6233253 100644 --- a/src/BlockFile.h +++ b/src/BlockFile.h @@ -181,7 +181,7 @@ class PROFILE_DLL_API BlockFile /* not final, abstract */ { float *summary256, float *summary64K); /// Read the summary section of the file. Derived classes implement. - virtual bool ReadSummary(void *data) = 0; + virtual bool ReadSummary(ArrayOf &data) = 0; /// Byte-swap the summary data, in case it was saved by a system /// on a different platform @@ -251,7 +251,7 @@ class AliasBlockFile /* not final */ : public BlockFile /// Write the summary to disk, using the derived ReadData() to get the data virtual void WriteSummary(); /// Read the summary into a buffer - bool ReadSummary(void *data) override; + bool ReadSummary(ArrayOf &data) override; wxFileNameWrapper mAliasedFileName; sampleCount mAliasStart; diff --git a/src/Sequence.cpp b/src/Sequence.cpp index 5be77ac49..911d2c722 100644 --- a/src/Sequence.cpp +++ b/src/Sequence.cpp @@ -1402,6 +1402,8 @@ bool Sequence::GetWaveDisplay(float *min, float *max, float *rms, int* bl, // Read triples //check to see if summary data has been computed if (seqBlock.f->IsSummaryAvailable()) + // Ignore the return value. + // This function fills with zeroes if read fails seqBlock.f->Read256(temp.get(), startPosition, num); else //otherwise, mark the display as not yet computed @@ -1411,6 +1413,8 @@ bool Sequence::GetWaveDisplay(float *min, float *max, float *rms, int* bl, // Read triples //check to see if summary data has been computed if (seqBlock.f->IsSummaryAvailable()) + // Ignore the return value. + // This function fills with zeroes if read fails seqBlock.f->Read64K(temp.get(), startPosition, num); else //otherwise, mark the display as not yet computed diff --git a/src/blockfile/LegacyBlockFile.cpp b/src/blockfile/LegacyBlockFile.cpp index 7e90a7ae2..f64ba2abf 100644 --- a/src/blockfile/LegacyBlockFile.cpp +++ b/src/blockfile/LegacyBlockFile.cpp @@ -151,11 +151,13 @@ LegacyBlockFile::~LegacyBlockFile() } /// Read the summary section of the disk file. +/// Fill with zeroes and return false if data are unavailable for any reason. /// /// @param *data The buffer to write the data to. It must be at least /// mSummaryinfo.totalSummaryBytes long. -bool LegacyBlockFile::ReadSummary(void *data) +bool LegacyBlockFile::ReadSummary(ArrayOf &data) { + data.reinit( mSummaryInfo.totalSummaryBytes ); wxFFile summaryFile(mFileName.GetFullPath(), wxT("rb")); size_t read; { @@ -163,20 +165,25 @@ bool LegacyBlockFile::ReadSummary(void *data) if (mSilentLog) silence.create(); - if (!summaryFile.IsOpened()){ + if (!summaryFile.IsOpened()) { - memset(data, 0, mSummaryInfo.totalSummaryBytes); + memset(data.get(), 0, mSummaryInfo.totalSummaryBytes); mSilentLog = TRUE; - return true; + return false; } - read = summaryFile.Read(data, mSummaryInfo.totalSummaryBytes); + read = summaryFile.Read(data.get(), mSummaryInfo.totalSummaryBytes); } - mSilentLog=FALSE; + mSilentLog = FALSE; - return (read == mSummaryInfo.totalSummaryBytes); + if (read != mSummaryInfo.totalSummaryBytes) { + memset(data.get(), 0, mSummaryInfo.totalSummaryBytes); + return false; + } + + return true; } /// Read the data portion of the block file using libsndfile. Convert it diff --git a/src/blockfile/LegacyBlockFile.h b/src/blockfile/LegacyBlockFile.h index 150472da3..549b216af 100644 --- a/src/blockfile/LegacyBlockFile.h +++ b/src/blockfile/LegacyBlockFile.h @@ -48,7 +48,7 @@ class LegacyBlockFile final : public BlockFile { // Reading /// Read the summary section of the disk file - bool ReadSummary(void *data) override; + bool ReadSummary(ArrayOf &data) override; /// Read the data section of the disk file size_t ReadData(samplePtr data, sampleFormat format, size_t start, size_t len) const override; diff --git a/src/blockfile/ODDecodeBlockFile.cpp b/src/blockfile/ODDecodeBlockFile.cpp index 6788c6cef..c87207948 100644 --- a/src/blockfile/ODDecodeBlockFile.cpp +++ b/src/blockfile/ODDecodeBlockFile.cpp @@ -126,6 +126,7 @@ void ODDecodeBlockFile::GetMinMax(float *outMin, float *outMax, float *outRMS) c } /// Returns the 256 byte summary data block +/// Fill with zeroes and return false if data are unavailable for any reason. bool ODDecodeBlockFile::Read256(float *buffer, size_t start, size_t len) { if(IsSummaryAvailable()) @@ -134,13 +135,13 @@ bool ODDecodeBlockFile::Read256(float *buffer, size_t start, size_t len) } else { - //this should not be reached (client should check IsSummaryAvailable()==true before this. - buffer = NULL; - return true; + ClearSamples((samplePtr)buffer, floatSample, 0, len); + return false; } } /// Returns the 64K summary data block +/// Fill with zeroes and return false if data are unavailable for any reason. bool ODDecodeBlockFile::Read64K(float *buffer, size_t start, size_t len) { if(IsSummaryAvailable()) @@ -149,8 +150,8 @@ bool ODDecodeBlockFile::Read64K(float *buffer, size_t start, size_t len) } else { - //this should not be reached (client should check IsSummaryAvailable()==true before this. - return true; + ClearSamples((samplePtr)buffer, floatSample, 0, len); + return false; } } @@ -450,17 +451,19 @@ size_t ODDecodeBlockFile::ReadData(samplePtr data, sampleFormat format, /// Read the summary of this alias block from disk. Since the audio data /// is elsewhere, this consists of reading the entire summary file. +/// Fill with zeroes and return false if data are unavailable for any reason. /// /// @param *data The buffer where the summary data will be stored. It must /// be at least mSummaryInfo.totalSummaryBytes long. -bool ODDecodeBlockFile::ReadSummary(void *data) +bool ODDecodeBlockFile::ReadSummary(ArrayOf &data) { - //I dont think we need to add a mutex here because only the main thread changes filenames and calls ReadSummarz + //I dont think we need to add a mutex here because only the main thread changes filenames and calls ReadSummary if(IsSummaryAvailable()) return SimpleBlockFile::ReadSummary(data); - memset(data, 0, mSummaryInfo.totalSummaryBytes); - return true; + data.reinit( mSummaryInfo.totalSummaryBytes ); + memset(data.get(), 0, mSummaryInfo.totalSummaryBytes); + return false; } ///set the decoder, diff --git a/src/blockfile/ODDecodeBlockFile.h b/src/blockfile/ODDecodeBlockFile.h index 06d66f58d..3a743ffd9 100644 --- a/src/blockfile/ODDecodeBlockFile.h +++ b/src/blockfile/ODDecodeBlockFile.h @@ -112,7 +112,7 @@ class ODDecodeBlockFile final : public SimpleBlockFile size_t start, size_t len) const override; /// Read the summary into a buffer - bool ReadSummary(void *data) override; + bool ReadSummary(ArrayOf &data) override; ///Returns the type of audiofile this blockfile is loaded from. unsigned int GetDecodeType() /* not override */ const { return mType; } diff --git a/src/blockfile/ODPCMAliasBlockFile.cpp b/src/blockfile/ODPCMAliasBlockFile.cpp index 01d93fa78..44d057c3f 100644 --- a/src/blockfile/ODPCMAliasBlockFile.cpp +++ b/src/blockfile/ODPCMAliasBlockFile.cpp @@ -155,7 +155,8 @@ void ODPCMAliasBlockFile::GetMinMax(float *outMin, float *outMax, float *outRMS) } } -/// Returns the 256 byte summary data block. Clients should check to see if the summary is available before trying to read it with this call. +/// Returns the 256 byte summary data block. +/// Fill with zeroes and return false if data are unavailable for any reason. bool ODPCMAliasBlockFile::Read256(float *buffer, size_t start, size_t len) { if(IsSummaryAvailable()) @@ -165,12 +166,13 @@ bool ODPCMAliasBlockFile::Read256(float *buffer, size_t start, size_t len) else { //return nothing. - buffer = NULL; - return true; + ClearSamples((samplePtr)buffer, floatSample, 0, len); + return false; } } -/// Returns the 64K summary data block. Clients should check to see if the summary is available before trying to read it with this call. +/// Returns the 64K summary data block. +/// Fill with zeroes and return false if data are unavailable for any reason. bool ODPCMAliasBlockFile::Read64K(float *buffer, size_t start, size_t len) { if(IsSummaryAvailable()) @@ -180,8 +182,8 @@ bool ODPCMAliasBlockFile::Read64K(float *buffer, size_t start, size_t len) else { //return nothing - it hasn't been calculated yet - buffer = NULL; - return true; + ClearSamples((samplePtr)buffer, floatSample, 0, len); + return false; } } @@ -505,40 +507,43 @@ size_t ODPCMAliasBlockFile::ReadData(samplePtr data, sampleFormat format, /// Read the summary of this alias block from disk. Since the audio data /// is elsewhere, this consists of reading the entire summary file. +/// Fill with zeroes and return false if data are unavailable for any reason. /// /// @param *data The buffer where the summary data will be stored. It must /// be at least mSummaryInfo.totalSummaryBytes long. -bool ODPCMAliasBlockFile::ReadSummary(void *data) +bool ODPCMAliasBlockFile::ReadSummary(ArrayOf &data) { + data.reinit( mSummaryInfo.totalSummaryBytes ); - mFileNameMutex.Lock(); + ODLocker locker{ &mFileNameMutex }; wxFFile summaryFile(mFileName.GetFullPath(), wxT("rb")); - if( !summaryFile.IsOpened() ){ + if( !summaryFile.IsOpened() ) { // NEW model; we need to return valid data - memset(data, 0, mSummaryInfo.totalSummaryBytes); + memset(data.get(), 0, mSummaryInfo.totalSummaryBytes); // we silence the logging for this operation in this object // after first occurrence of error; it's already reported and // spewing at the user will complicate the user's ability to // deal - mSilentLog=TRUE; - - mFileNameMutex.Unlock(); - return true; + mSilentLog = TRUE; + return false; } else - mSilentLog=FALSE; // worked properly, any future error is NEW + mSilentLog = FALSE; // worked properly, any future error is NEW - auto read = summaryFile.Read(data, mSummaryInfo.totalSummaryBytes); + auto read = summaryFile.Read(data.get(), mSummaryInfo.totalSummaryBytes); - FixSummary(data); + if (read != mSummaryInfo.totalSummaryBytes) { + memset(data.get(), 0, mSummaryInfo.totalSummaryBytes); + return false; + } + + FixSummary(data.get()); - - mFileNameMutex.Unlock(); - return (read == mSummaryInfo.totalSummaryBytes); + return true; } /// Prevents a read on other threads. diff --git a/src/blockfile/ODPCMAliasBlockFile.h b/src/blockfile/ODPCMAliasBlockFile.h index 10b5463df..1653b1e65 100644 --- a/src/blockfile/ODPCMAliasBlockFile.h +++ b/src/blockfile/ODPCMAliasBlockFile.h @@ -121,7 +121,7 @@ class ODPCMAliasBlockFile final : public PCMAliasBlockFile size_t start, size_t len) const override; /// Read the summary into a buffer - bool ReadSummary(void *data) override; + bool ReadSummary(ArrayOf &data) override; ///sets the file name the summary info will be saved in. threadsafe. void SetFileName(wxFileNameWrapper &&name) override; diff --git a/src/blockfile/SilentBlockFile.cpp b/src/blockfile/SilentBlockFile.cpp index 89cbc5b03..a2d33effb 100644 --- a/src/blockfile/SilentBlockFile.cpp +++ b/src/blockfile/SilentBlockFile.cpp @@ -24,9 +24,10 @@ SilentBlockFile::~SilentBlockFile() { } -bool SilentBlockFile::ReadSummary(void *data) +bool SilentBlockFile::ReadSummary(ArrayOf &data) { - memset(data, 0, mSummaryInfo.totalSummaryBytes); + data.reinit( mSummaryInfo.totalSummaryBytes ); + memset(data.get(), 0, mSummaryInfo.totalSummaryBytes); return true; } diff --git a/src/blockfile/SilentBlockFile.h b/src/blockfile/SilentBlockFile.h index 24b6452dd..e1788bccc 100644 --- a/src/blockfile/SilentBlockFile.h +++ b/src/blockfile/SilentBlockFile.h @@ -33,7 +33,7 @@ class SilentBlockFile final : public BlockFile { // Reading /// Read the summary section of the disk file - bool ReadSummary(void *data) override; + bool ReadSummary(ArrayOf &data) override; /// Read the data section of the disk file size_t ReadData(samplePtr data, sampleFormat format, size_t start, size_t len) const override; diff --git a/src/blockfile/SimpleBlockFile.cpp b/src/blockfile/SimpleBlockFile.cpp index 14bad5ad4..c1482b984 100644 --- a/src/blockfile/SimpleBlockFile.cpp +++ b/src/blockfile/SimpleBlockFile.cpp @@ -321,9 +321,8 @@ void SimpleBlockFile::FillCache() } // Read summary data into cache - mCache.summaryData.reinit(mSummaryInfo.totalSummaryBytes); - if (!ReadSummary(mCache.summaryData.get())) - memset(mCache.summaryData.get(), 0, mSummaryInfo.totalSummaryBytes); + // Fills with zeroes in case of failure: + ReadSummary(mCache.summaryData); // Cache is active but already on disk mCache.active = true; @@ -336,12 +335,12 @@ void SimpleBlockFile::FillCache() /// /// @param *data The buffer to write the data to. It must be at least /// mSummaryinfo.totalSummaryBytes long. -bool SimpleBlockFile::ReadSummary(void *data) +bool SimpleBlockFile::ReadSummary(ArrayOf &data) { - if (mCache.active) - { + data.reinit( mSummaryInfo.totalSummaryBytes ); + if (mCache.active) { //wxLogDebug("SimpleBlockFile::ReadSummary(): Summary is already in cache."); - memcpy(data, mCache.summaryData.get(), mSummaryInfo.totalSummaryBytes); + memcpy(data.get(), mCache.summaryData.get(), mSummaryInfo.totalSummaryBytes); return true; } else @@ -357,23 +356,24 @@ bool SimpleBlockFile::ReadSummary(void *data) // FIXME: TRAP_ERR no report to user of absent summary files? // filled with zero instead. if (!file.IsOpened()){ - memset(data, 0, mSummaryInfo.totalSummaryBytes); + memset(data.get(), 0, mSummaryInfo.totalSummaryBytes); mSilentLog = TRUE; - return true; + return false; } } - mSilentLog=FALSE; + mSilentLog = FALSE; // The offset is just past the au header - // FIXME: Seek in summary file could fail. - if( !file.Seek(sizeof(auHeader)) ) + if( !file.Seek(sizeof(auHeader)) || + file.Read(data.get(), mSummaryInfo.totalSummaryBytes) != + mSummaryInfo.totalSummaryBytes ) { + memset(data.get(), 0, mSummaryInfo.totalSummaryBytes); return false; + } - auto read = file.Read(data, mSummaryInfo.totalSummaryBytes); + FixSummary(data.get()); - FixSummary(data); - - return (read == mSummaryInfo.totalSummaryBytes); + return true; } } diff --git a/src/blockfile/SimpleBlockFile.h b/src/blockfile/SimpleBlockFile.h index f733559c9..f95e80bfb 100644 --- a/src/blockfile/SimpleBlockFile.h +++ b/src/blockfile/SimpleBlockFile.h @@ -63,7 +63,7 @@ class PROFILE_DLL_API SimpleBlockFile /* not final */ : public BlockFile { // Reading /// Read the summary section of the disk file - bool ReadSummary(void *data) override; + bool ReadSummary(ArrayOf &data) override; /// Read the data section of the disk file size_t ReadData(samplePtr data, sampleFormat format, size_t start, size_t len) const override;