From 73e61592aa5b8e9535f66d47f394b4248aa52fc1 Mon Sep 17 00:00:00 2001
From: Paul Licameli <paul.licameli@audacityteam.org>
Date: Mon, 28 Nov 2016 10:15:34 -0500
Subject: [PATCH] Sequence::ConvertToSampleFormat gives strong guarantee

---
 src/Sequence.cpp | 76 ++++++++++++++++++------------------------------
 src/Sequence.h   |  5 ++--
 src/WaveClip.cpp |  6 ++--
 3 files changed, 34 insertions(+), 53 deletions(-)

diff --git a/src/Sequence.cpp b/src/Sequence.cpp
index 8e1e9dc0f..8d2d1179b 100644
--- a/src/Sequence.cpp
+++ b/src/Sequence.cpp
@@ -129,19 +129,16 @@ bool Sequence::SetSampleFormat(sampleFormat format)
 }
 */
 
-bool Sequence::ConvertToSampleFormat(sampleFormat format, bool* pbChanged)
+bool Sequence::ConvertToSampleFormat(sampleFormat format)
+// STRONG-GUARANTEE
 {
-   wxASSERT(pbChanged);
-   *pbChanged = false;
-
-   // Caller should check this no-change case before calling; we ignore it here.
    if (format == mSampleFormat)
-      return true;
+      // no change
+      return false;
 
    if (mBlock.size() == 0)
    {
       mSampleFormat = format;
-      *pbChanged = true;
       return true;
    }
 
@@ -153,25 +150,32 @@ bool Sequence::ConvertToSampleFormat(sampleFormat format, bool* pbChanged)
    mMinSamples = sMaxDiskBlockSize / SAMPLE_SIZE(mSampleFormat) / 2;
    mMaxSamples = mMinSamples * 2;
 
-   BlockArray newBlockArray;
-   // Use the ratio of old to NEW mMaxSamples to make a reasonable guess at allocation.
-   newBlockArray.reserve(1 + mBlock.size() * ((float)oldMaxSamples / (float)mMaxSamples));
+   bool bSuccess = false;
+   auto cleanup = finally( [&] {
+      if (!bSuccess) {
+         // Conversion failed. Revert these member vars.
+         mSampleFormat = oldFormat;
+         mMaxSamples = oldMaxSamples;
+         mMinSamples = oldMinSamples;
+      }
+   } );
+
+   BlockArray newBlockArray;
+   // Use the ratio of old to NEW mMaxSamples to make a reasonable guess
+   // at allocation.
+   newBlockArray.reserve
+      (1 + mBlock.size() * ((float)oldMaxSamples / (float)mMaxSamples));
 
-   bool bSuccess = true;
    {
       SampleBuffer bufferOld(oldMaxSamples, oldFormat);
       SampleBuffer bufferNew(oldMaxSamples, format);
 
-      for (size_t i = 0, nn = mBlock.size(); i < nn && bSuccess; i++)
+      for (size_t i = 0, nn = mBlock.size(); i < nn; i++)
       {
          SeqBlock &oldSeqBlock = mBlock[i];
          const auto &oldBlockFile = oldSeqBlock.f;
-
          const auto len = oldBlockFile->GetLength();
-
-         bSuccess = (oldBlockFile->ReadData(bufferOld.ptr(), oldFormat, 0, len) > 0);
-         if (!bSuccess)
-            break;
+         Read(bufferOld.ptr(), oldFormat, oldSeqBlock, 0, len, true);
 
          CopySamples(bufferOld.ptr(), oldFormat, bufferNew.ptr(), format, len);
 
@@ -189,42 +193,20 @@ bool Sequence::ConvertToSampleFormat(sampleFormat format, bool* pbChanged)
          const unsigned prevSize = newBlockArray.size();
          Blockify(*mDirManager, mMaxSamples, mSampleFormat,
                   newBlockArray, blockstart, bufferNew.ptr(), len);
-         bSuccess = (newBlockArray.size() > prevSize);
-         if (bSuccess)
-            *pbChanged = true;
       }
    }
 
-   if (bSuccess)
-   {
-      // Invalidate all the old, non-aliased block files.
-      // Aliased files will be converted at save, per comment above.
+   // Invalidate all the old, non-aliased block files.
+   // Aliased files will be converted at save, per comment above.
 
-      // Replace with NEW blocks.
-      mBlock.swap(newBlockArray);
-   }
-   else
-   {
-      /* vvvvv We *should do the following, but TrackPanel::OnFormatChange() doesn't actually check the conversion results,
-         it just assumes the conversion was successful.
-         TODO: Uncomment this section when TrackPanel::OnFormatChange() is upgraded to check the results.
+   // Commit the changes to block file array
+   CommitChangesIfConsistent
+      (newBlockArray, mNumSamples, wxT("Sequence::ConvertToSampleFormat()"));
 
-         PRL:  I don't understand why the comment above justifies leaving the sequence in an inconsistent state.
-         If this function must fail, better to leave it as a no-op on this sequence.  I am uncommenting the
-         lines below, and adding one to revert mMinSamples too.
-         */
+   // Commit the other changes
+   bSuccess = true;
 
-      // Conversion failed. Revert these member vars.
-      mSampleFormat = oldFormat;
-      mMaxSamples = oldMaxSamples;
-      mMinSamples = oldMinSamples;
-
-      *pbChanged = false;  // Revert overall change flag, in case we had some partial success in the loop.
-   }
-
-   ConsistencyCheck(wxT("Sequence::ConvertToSampleFormat()"));
-
-   return bSuccess;
+   return true;
 }
 
 std::pair<float, float> Sequence::GetMinMax(
diff --git a/src/Sequence.h b/src/Sequence.h
index c416a4d48..11aa8f7dc 100644
--- a/src/Sequence.h
+++ b/src/Sequence.h
@@ -162,8 +162,9 @@ class PROFILE_DLL_API Sequence final : public XMLTagHandler{
    //
 
    sampleFormat GetSampleFormat() const;
-   // bool SetSampleFormat(sampleFormat format);
-   bool ConvertToSampleFormat(sampleFormat format, bool* pbChanged);
+
+   // Return true iff there is a change
+   bool ConvertToSampleFormat(sampleFormat format);
 
    //
    // Retrieving summary info
diff --git a/src/WaveClip.cpp b/src/WaveClip.cpp
index a224f2461..19ef37708 100644
--- a/src/WaveClip.cpp
+++ b/src/WaveClip.cpp
@@ -1342,11 +1342,9 @@ void WaveClip::ConvertToSampleFormat(sampleFormat format)
    // Note:  it is not necessary to do this recursively to cutlines.
    // They get converted as needed when they are expanded.
 
-   bool bChanged;
-   bool bResult = mSequence->ConvertToSampleFormat(format, &bChanged);
-   if (bResult && bChanged)
+   auto bChanged = mSequence->ConvertToSampleFormat(format);
+   if (bChanged)
       MarkChanged();
-   wxASSERT(bResult); // TODO: Throw an actual error.
 }
 
 void WaveClip::UpdateEnvelopeTrackLen()