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

@@ -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<char> 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<char> 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<char> &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

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<char> &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<char> &data) override;
 
    wxFileNameWrapper mAliasedFileName;
    sampleCount mAliasStart;

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

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<char> &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

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<char> &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;

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<char> &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,

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<char> &data) override;
 
    ///Returns the type of audiofile this blockfile is loaded from.
    unsigned int GetDecodeType() /* not override */ const { return mType; }

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<char> &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.

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<char> &data) override;
 
    ///sets the file name the summary info will be saved in.  threadsafe.
    void SetFileName(wxFileNameWrapper &&name) override;

src/blockfile/SilentBlockFile.cpp

@@ -24,9 +24,10 @@ SilentBlockFile::~SilentBlockFile()
 {
 }
 
-bool SilentBlockFile::ReadSummary(void *data)
+bool SilentBlockFile::ReadSummary(ArrayOf<char> &data)
 {
-   memset(data, 0, mSummaryInfo.totalSummaryBytes);
+   data.reinit( mSummaryInfo.totalSummaryBytes );
+   memset(data.get(), 0, mSummaryInfo.totalSummaryBytes);
    return true;
 }
 

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<char> &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;

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<char> &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;
    }
 }
 

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<char> &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;