More changes to avoid copying of wxFileName.

Access BlockFile::mFileName without copying, with proper multithreading cautions
  Redefine ODLocker as movable, and it may try-lock only.
  Avoid some wxString copies
  fix bugs in previous

src/BlockFile.cpp

@@ -120,9 +120,9 @@ BlockFile::~BlockFile()
 /// but most BlockFiles have at least their summary data here.
 /// (some, i.e. SilentBlockFiles, do not correspond to a file on
 ///  disk and have empty file names)
-wxFileName BlockFile::GetFileName() const
+auto BlockFile::GetFileName() const -> GetFileNameResult
 {
-   return mFileName;
+   return { mFileName };
 }
 
 ///sets the file name the summary info will be saved in.  threadsafe.

src/BlockFile.h

@@ -24,6 +24,8 @@
 
 #include "wxFileNameWrapper.h"
 
+#include "ondemand/ODTaskThread.h"
+
 
 class SummaryInfo {
  public:
@@ -72,7 +74,24 @@ class PROFILE_DLL_API BlockFile /* not final, abstract */ {
    /// Gets the filename of the disk file associated with this BlockFile
    /// (can be empty -- some BlockFiles, like SilentBlockFile, correspond to
    ///  no file on disk)
-   virtual wxFileName GetFileName() const;
+   /// Avoids copying wxFileName by returning a reference, but for some subclasses
+   /// of BlockFile, you must exclude other threads from changing the name so long
+   /// as you have only a reference.  Thus, this wrapper object that guarantees release
+   /// of any lock when it goes out of scope.  Call mLocker.reset() to unlock it sooner.
+   struct GetFileNameResult {
+      const wxFileName &name;
+      ODLocker mLocker;
+
+      GetFileNameResult(const wxFileName &name_, ODLocker &&locker = ODLocker{})
+      : name{ name_ }, mLocker{ std::move(locker) } {}
+
+      GetFileNameResult(const GetFileNameResult&) PROHIBITED;
+      GetFileNameResult &operator= (const GetFileNameResult&) PROHIBITED;
+
+      GetFileNameResult(GetFileNameResult &&that)
+      : name{ that.name }, mLocker{ std::move(that.mLocker) } {}
+   };
+   virtual GetFileNameResult GetFileName() const;
    virtual void SetFileName(wxFileNameWrapper &&name);
 
    virtual sampleCount GetLength() const { return mLen; }

src/Dependencies.cpp

@@ -405,7 +405,7 @@ void DependencyDialog::PopulateList()
 
    mHasMissingFiles = false;
    mHasNonMissingFiles = false;
-   unsigned int i;
+   long i = 0;
    for (const auto &aliasedFile : mAliasedFiles) {
       const wxFileName &fileName = aliasedFile.mFileName;
       wxLongLong byteCount = (aliasedFile.mByteCount * 124) / 100;
@@ -426,6 +426,8 @@ void DependencyDialog::PopulateList()
       }
       mFileListCtrl->SetItem(i, 1, Internat::FormatSize(byteCount));
       mFileListCtrl->SetItemData(i, long(bOriginalExists));
+
+      ++i;
    }
 
    wxString msg = kStdMsg;

src/DirManager.cpp

@@ -941,7 +941,8 @@ bool DirManager::ContainsBlockFile(const BlockFile *b) const
 {
    if (!b)
       return false;
-   BlockHash::const_iterator it = mBlockFileHash.find(b->GetFileName().GetName());
+   auto result = b->GetFileName();
+   BlockHash::const_iterator it = mBlockFileHash.find(result.name.GetName());
    return it != mBlockFileHash.end() && it->second == b;
 }
 
@@ -957,7 +958,8 @@ bool DirManager::ContainsBlockFile(const wxString &filepath) const
 // the BlockFile.
 BlockFile *DirManager::CopyBlockFile(BlockFile *b)
 {
-   const auto &fn = b->GetFileName();
+   auto result = b->GetFileName();
+   const auto &fn = result.name;
 
    if (!b->IsLocked()) {
       b->Ref();
@@ -996,6 +998,9 @@ BlockFile *DirManager::CopyBlockFile(BlockFile *b)
             return NULL;
       }
 
+      // Done with fn
+      result.mLocker.reset();
+
       b2 = b->Copy(std::move(newFile));
 
       if (b2 == NULL)
@@ -1090,7 +1095,7 @@ bool DirManager::HandleXMLTag(const wxChar *tag, const wxChar **attrs)
    // return a reference to the existing object instead.
    //
 
-   wxString name = target->GetFileName().GetName();
+   wxString name = target->GetFileName().name.GetName();
    BlockFile *retrieved = mBlockFileHash[name];
    if (retrieved) {
       // Lock it in order to DELETE it safely, i.e. without having
@@ -1114,23 +1119,24 @@ bool DirManager::HandleXMLTag(const wxChar *tag, const wxChar **attrs)
 
 bool DirManager::MoveOrCopyToNewProjectDirectory(BlockFile *f, bool copy)
 {
-   const auto oldFileName = f->GetFileName();
+   auto result = f->GetFileName();
+   const auto &oldFileNameRef = result.name;
 
    // Check that this BlockFile corresponds to a file on disk
    //ANSWER-ME: Is this checking only for SilentBlockFiles, in which case
    //    (!oldFileName.IsOk()) is a more correct check?
-   if (oldFileName.GetName().IsEmpty()) {
+   if (oldFileNameRef.GetName().IsEmpty()) {
       return true;
    }
 
    wxFileNameWrapper newFileName;
-   if (!this->AssignFile(newFileName, oldFileName.GetFullName(), false))
+   if (!this->AssignFile(newFileName, oldFileNameRef.GetFullName(), false))
       return false;
 
-   if (newFileName != oldFileName) {
+   if (newFileName != oldFileNameRef) {
       //check to see that summary exists before we copy.
       bool summaryExisted = f->IsSummaryAvailable();
-      auto oldPath = oldFileName.GetFullPath();
+      auto oldPath = oldFileNameRef.GetFullPath();
       auto newPath = newFileName.GetFullPath();
       if (summaryExisted) {
          auto success = copy
@@ -1139,11 +1145,21 @@ bool DirManager::MoveOrCopyToNewProjectDirectory(BlockFile *f, bool copy)
          if (!success)
             return false;
       }
-      f->SetFileName(std::move(newFileName));
 
-      //there is a small chance that the summary has begun to be computed on a different thread with the
-      //original filename.  we need to catch this case by waiting for it to finish and then copy.
       if (!summaryExisted && (f->IsSummaryAvailable() || f->IsSummaryBeingComputed())) {
+
+         // We will need to remember the old file name, so copy it
+         wxFileName oldFileName{ oldFileNameRef };
+
+         // Now we can free any lock (and should, if as the comment below says, we need
+         // the other threads to progress)
+         result.mLocker.reset();
+
+         f->SetFileName(std::move(newFileName));
+
+         //there is a small chance that the summary has begun to be computed on a different thread with the
+         //original filename.  we need to catch this case by waiting for it to finish and then copy.
+
          //block to make sure OD files don't get written while we are changing file names.
          //(It is important that OD files set this lock while computing their summary files.)
          while(f->IsSummaryBeingComputed() && !f->IsSummaryAvailable())
@@ -1160,6 +1176,11 @@ bool DirManager::MoveOrCopyToNewProjectDirectory(BlockFile *f, bool copy)
                return false;
          }
       }
+      else {
+         // Can free this now, and must, because of nonrecursive mutexes
+         result.mLocker.reset();
+         f->SetFileName(std::move(newFileName));
+      }
    }
 
    return true;
@@ -1190,7 +1211,7 @@ int DirManager::GetRefCount(BlockFile * f)
 
 void DirManager::Deref(BlockFile * f)
 {
-   wxString theFileName = f->GetFileName().GetName();
+   const wxString theFileName = f->GetFileName().name.GetName();
 
    //printf("Deref(%d): %s\n",
    //       f->mRefCount-1,
@@ -1336,13 +1357,14 @@ bool DirManager::EnsureSafeFilename(const wxFileName &fName)
 
             if (b->IsAlias() && ab->GetAliasedFileName() == fName)
             {
-               ab->ChangeAliasedFileName(std::move(renamedFileName));
+               ab->ChangeAliasedFileName(wxFileNameWrapper{ renamedFileName });
                ab->UnlockRead();
-               wxPrintf(_("Changed block %s to new alias name\n"), b->GetFileName().GetFullName().c_str());
+               wxPrintf(_("Changed block %s to new alias name\n"),
+                        b->GetFileName().name.GetFullName().c_str());
 
             }
             else if (!b->IsDataAvailable() && db->GetEncodedAudioFilename() == fName) {
-               db->ChangeAudioFile(std::move(renamedFileName));
+               db->ChangeAudioFile(wxFileNameWrapper{ renamedFileName });
                db->UnlockRead();
             }
             ++iter;

src/FileFormats.h

@@ -118,13 +118,11 @@ OSType sf_header_mactype(int format);
 // This function wrapper uses a mutex to serialize calls to the SndFile library.
 #include "MemoryX.h"
 #include "ondemand/ODTaskThread.h"
-class ODLock;
-class ODLocker;
 extern ODLock libSndFileMutex;
 template<typename R, typename F, typename... Args>
 inline R SFCall(F fun, Args&&... args)
 {
-   ODLocker locker{ libSndFileMutex };
+   ODLocker locker{ &libSndFileMutex };
    return fun(std::forward<Args>(args)...);
 }
 

src/Project.h

@@ -247,7 +247,7 @@ class AUDACITY_DLL_API AudacityProject final : public wxFrame,
    #endif
    void Clear();
 
-   wxString GetFileName() { return mFileName; }
+   const wxString &GetFileName() { return mFileName; }
    bool GetDirty() { return mDirty; }
    void SetProjectTitle();
 

src/Sequence.cpp

@@ -987,7 +987,7 @@ void Sequence::HandleXMLEndTag(const wxChar *tag)
    for (unsigned b = 0, nn = mBlock.size(); b < nn;  b++) {
       SeqBlock &block = mBlock[b];
       if (block.start != numSamples) {
-         wxString sFileAndExtension = block.f->GetFileName().GetFullName();
+         wxString sFileAndExtension = block.f->GetFileName().name.GetFullName();
          if (sFileAndExtension.IsEmpty())
             sFileAndExtension = wxT("(replaced with silence)");
          else
@@ -1829,7 +1829,7 @@ void Sequence::DebugPrintf(wxString *dest) const
           seqBlock.f ? mDirManager->GetRefCount(seqBlock.f) : 0);
 
       if (seqBlock.f)
-         *dest += seqBlock.f->GetFileName().GetFullName();
+         *dest += seqBlock.f->GetFileName().name.GetFullName();
       else
          *dest += wxT("<missing block file>");
 

src/WaveClip.cpp

@@ -135,7 +135,7 @@ public:
          invalEnd = len;
 
 
-      ODLocker locker(mRegionsMutex);
+      ODLocker locker(&mRegionsMutex);
 
       //look thru the region array for a place to insert.  We could make this more spiffy than a linear search
       //but right now it is not needed since there will usually only be one region (which grows) for OD loading.
@@ -420,7 +420,7 @@ bool WaveClip::AfterClip(double t) const
 ///Delete the wave cache - force redraw.  Thread-safe
 void WaveClip::DeleteWaveCache()
 {
-   ODLocker locker(mWaveCacheMutex);
+   ODLocker locker(&mWaveCacheMutex);
    if(mWaveCache!=NULL)
       delete mWaveCache;
    mWaveCache = new WaveCache();
@@ -429,7 +429,7 @@ void WaveClip::DeleteWaveCache()
 ///Adds an invalid region to the wavecache so it redraws that portion only.
 void WaveClip::AddInvalidRegion(long startSample, long endSample)
 {
-   ODLocker locker(mWaveCacheMutex);
+   ODLocker locker(&mWaveCacheMutex);
    if(mWaveCache!=NULL)
       mWaveCache->AddInvalidRegion(startSample,endSample);
 }
@@ -525,7 +525,7 @@ bool WaveClip::GetWaveDisplay(WaveDisplay &display, double t0,
    }
    else {
       // Lock the list of invalid regions
-      ODLocker locker(mWaveCacheMutex);
+      ODLocker locker(&mWaveCacheMutex);
 
       const double tstep = 1.0 / pixelsPerSecond;
       const double samplesPerPixel = mRate * tstep;

src/blockfile/ODDecodeBlockFile.cpp

@@ -369,13 +369,9 @@ void ODDecodeBlockFile::SetFileName(wxFileNameWrapper &&name)
 }
 
 ///sets the file name the summary info will be saved in.  threadsafe.
-wxFileName ODDecodeBlockFile::GetFileName() const
+auto ODDecodeBlockFile::GetFileName() const -> GetFileNameResult
 {
-   wxFileName name;
+   return { mFileName, ODLocker{ &mFileNameMutex } };
-   mFileNameMutex.Lock();
-   name = mFileName;
-   mFileNameMutex.Unlock();
-   return name;
 }
 
 /// A thread-safe version of CalcSummary.  BlockFile::CalcSummary

src/blockfile/ODDecodeBlockFile.h

@@ -133,7 +133,7 @@ class ODDecodeBlockFile final : public SimpleBlockFile
 
    ///sets the file name the summary info will be saved in.  threadsafe.
    void SetFileName(wxFileNameWrapper &&name) override;
-   wxFileName GetFileName() const override;
+   GetFileNameResult GetFileName() const override;
 
    /// Prevents a read on other threads of the encoded audio file.
    void LockRead() const override;

src/blockfile/ODPCMAliasBlockFile.cpp

@@ -381,13 +381,9 @@ void ODPCMAliasBlockFile::SetFileName(wxFileNameWrapper &&name)
 }
 
 ///sets the file name the summary info will be saved in.  threadsafe.
-wxFileName ODPCMAliasBlockFile::GetFileName() const
+auto ODPCMAliasBlockFile::GetFileName() const -> GetFileNameResult
 {
-   wxFileName name;
+   return { mFileName, ODLocker{ &mFileNameMutex } };
-   mFileNameMutex.Lock();
-   name = mFileName;
-   mFileNameMutex.Unlock();
-   return name;
 }
 
 /// Write the summary to disk, using the derived ReadData() to get the data

src/blockfile/ODPCMAliasBlockFile.h

@@ -125,7 +125,7 @@ class ODPCMAliasBlockFile final : public PCMAliasBlockFile
 
    ///sets the file name the summary info will be saved in.  threadsafe.
    void SetFileName(wxFileNameWrapper &&name) override;
-   wxFileName GetFileName() const override;
+   GetFileNameResult GetFileName() const override;
 
    //when the file closes, it locks the blockfiles, but only conditionally.
    // It calls this so we can check if it has been saved before.

src/commands/OpenSaveCommands.cpp

@@ -48,9 +48,9 @@ bool OpenProjectCommand::Apply(CommandExecutionContext context)
    }
    else
    {
-      context.GetProject()->OpenFile(fileName,addToHistory);
+      context.GetProject()->OpenFile(fileName, addToHistory);
    }
-   wxString newFileName = context.GetProject()->GetFileName();
+   const wxString &newFileName = context.GetProject()->GetFileName();
 
    // Because Open does not return a success or failure, we have to guess
    // at this point, based on whether the project file name has

src/ondemand/ODDecodeTask.cpp

@@ -290,8 +290,8 @@ int ODDecodeTask::GetNumFileDecoders()
 
 ///This should handle unicode converted to UTF-8 on mac/linux, but OD TODO:check on windows
 ODFileDecoder::ODFileDecoder(const wxString & fName)
+   : mFName{ fName }
 {
-   mFName = fName;
    mInited = false;
 }
 

src/ondemand/ODDecodeTask.h

@@ -120,7 +120,7 @@ public:
    ///returns negative value for failure, 0 or positive value for success.
    virtual int Decode(SampleBuffer & data, sampleFormat & format, sampleCount start, sampleCount len, unsigned int channel)=0;
 
-   wxString GetFileName(){return mFName;}
+   const wxString &GetFileName(){return mFName;}
 
    bool IsInitialized();
 
@@ -131,7 +131,7 @@ protected:
    bool     mInited;
    ODLock   mInitedLock;
 
-   wxString  mFName;
+   const wxString  mFName;
 
    unsigned int mSampleRate;
    unsigned int mNumSamples;//this may depend on the channel - so TODO: we should probably let the decoder create/modify the track info directly.

src/ondemand/ODManager.cpp

@@ -277,7 +277,7 @@ void ODManager::Start()
       // JKC: If there are no tasks ready to run, or we're paused then
       // we wait for there to be tasks in the queue.
       {
-         ODLocker locker{ mQueueNotEmptyCondLock };
+         ODLocker locker{ &mQueueNotEmptyCondLock };
          if( (!tasksInArray) || paused)
             mQueueNotEmptyCond->Wait();
       }

src/ondemand/ODTaskThread.h

@@ -24,6 +24,7 @@
 #include <wx/thread.h>
 
 #include "../Audacity.h"	// contains the set-up of AUDACITY_DLL_API
+#include "../MemoryX.h"
 
 class ODTask;
 
@@ -170,25 +171,44 @@ protected:
 
 #endif // __WXMAC__
 
-// Like wxMutexLocker
+// class ODLocker
 // So you can use the RAII idiom with ODLock, on whatever platform
-class ODLocker
+// Construct with pointer to the lock, or default-construct and later
-{
+// reset()
+// If constructed with only a try-lock, and the lock was not acquired,
+// then it returns false when cast to bool
+struct ODUnlocker { void operator () (ODLock *p) const { if(p) p->Unlock(); } };
+using ODLockerBase = std::unique_ptr<ODLock, ODUnlocker>;
+class ODLocker : public ODLockerBase {
 public:
-   ODLocker(ODLock &lock)
+   // Lock any bare pointer to ODLock at construction time or when resetting.
-      : mLock(lock)
+   explicit ODLocker(ODLock *p = nullptr, bool tryOnly = false)
    {
-      mLock.Lock();
+      reset(p, tryOnly);
    }
 
-   ~ODLocker()
+   void reset(ODLock *p = nullptr, bool tryOnly = false)
    {
-      mLock.Unlock();
+      ODLockerBase::reset(p);
+      if(p) {
+         if (tryOnly) {
+            if (p->TryLock() != 0)
+               ODLockerBase::reset(nullptr);
+         }
+         else
+            p->Lock();
+      }
    }
 
-private:
+   // Assume already locked when moving ODLocker.  Don't lock again.
+   ODLocker(ODLocker&& that) : ODLockerBase { std::move(that) } {}
+   ODLocker &operator= (ODLocker && that) {
+      ODLockerBase::operator= ( std::move(that) );
+      return *this;
+   }
 
-   ODLock &mLock;
+   ODLocker(const ODLocker &that) PROHIBITED;
+   ODLocker &operator= (const ODLocker &that) PROHIBITED;
 };
 
 #endif //__AUDACITY_ODTASKTHREAD__