Rewrite scrub without mutex, condition variable, or message queue...

... Instead there is just a message buffer where the polling thread leaves last
observed state, not requiring each state to be processed before it is
overwritten.  The thread doing  FillBuffers is now responsible instead for the
complete interpretation of the message whenever it is reinvoked, which happens
at very regular time intervals.

In fact the separate polling thread might be eliminated, having FillBuffers
invoking the polling of the mouse directly in its own wake-ups -- unless the
platform really allows ::wxMouseState to be called safely only from the UI
thread, as appears to be the case in Linux.

Note that this complicated rewrite is accomplished incrementally in the commits
of this merge branch, not all of which leave scrubbing in a working state.

src/AudioIO.cpp

@@ -524,99 +524,74 @@ constexpr size_t TimeQueueGrainSize = 2000;
 
 struct AudioIO::ScrubState
 {
-   ScrubState(double t0, double t1, wxLongLong startClockMillis,
+   ScrubState(double t0, wxLongLong startClockMillis,
               double rate,
               const ScrubbingOptions &options)
-      : mTrailingIdx(0)
+      : mRate(rate)
-      , mMiddleIdx(1)
-      , mLeadingIdx(1)
-      , mRate(rate)
       , mLastScrubTimeMillis(startClockMillis)
-      , mUpdating()
+      , mStartTime( t0 )
    {
-      const sampleCount s0 { llrint( mRate *
+      const double t1 = options.bySpeed ? 1.0 : t0;
-         std::max( options.minTime, std::min( options.maxTime, t0 ) ) ) };
+      Update( t1, options );
-      const sampleCount s1 { llrint(t1 * mRate) };
-      Duration dd { *this };
-      auto actualDuration = std::max(sampleCount{1}, dd.duration);
-      auto success = mEntries[mMiddleIdx].Init(nullptr,
-         s0, s1, actualDuration, options, mRate);
-      if (success)
-         ++mLeadingIdx;
-      else {
-         // If not, we can wait to enqueue again later
-         dd.Cancel();
-      }
    }
 
-   bool Update(double end, const ScrubbingOptions &options)
+   void Update(double end, const ScrubbingOptions &options)
    {
-      // Main thread indicates a scrubbing interval
+      // Called by another thread
-
+      mMessage.Write({ end, options });
-      // MAY ADVANCE mLeadingIdx, BUT IT NEVER CATCHES UP TO mTrailingIdx.
-
-      wxMutexLocker locker(mUpdating);
-      bool result = true;
-      unsigned next = (mLeadingIdx + 1) % Size;
-      if (next != mTrailingIdx)
-      {
-         auto current = &mEntries[mLeadingIdx];
-         auto previous = &mEntries[(mLeadingIdx + Size - 1) % Size];
-
-         // Use the previous end as NEW start.
-         const auto s0 = previous->mS1;
-         Duration dd { *this };
-         if (dd.duration <= 0)
-            return false;
-
-         const sampleCount s1 ( options.bySpeed
-            ? s0.as_double() +
-               lrint(dd.duration.as_double() * end) // end is a speed
-            : lrint(end * mRate)            // end is a time
-         );
-         auto success =
-            current->Init(previous, s0, s1, dd.duration, options, mRate);
-         if (success)
-            mLeadingIdx = next;
-         else {
-            dd.Cancel();
-            return false;
-         }
-
-         mAvailable.Signal();
-         return result;
-      }
-      else
-      {
-         // ??
-         // Queue wasn't long enough.  Write side (UI thread)
-         // has overtaken the trailing read side (Audio thread), despite
-         // my comments above!  We lose some work requests then.
-         // wxASSERT(false);
-         return false;
-      }
    }
 
    void Get(sampleCount &startSample, sampleCount &endSample,
-            sampleCount &duration,
+         sampleCount &duration)
-            Maybe<wxMutexLocker> &cleanup)
    {
-      // Audio thread is ready for the next interval.
+      // Called by the thread that calls AudioIO::FillBuffers
+      startSample = endSample = duration = -1LL;
+      Duration dd { *this };
+      if (dd.duration <= 0)
+         return;
 
-      // MAY ADVANCE mMiddleIdx, WHICH MAY EQUAL mLeadingIdx, BUT DOES NOT PASS IT.
+      Message message{ mMessage.Read() };
-
+      if ( !mStarted ) {
-      if (!cleanup) {
+         const sampleCount s0 { llrint( mRate *
-         cleanup.create(mUpdating);
+            std::max( message.options.minTime,
+               std::min( message.options.maxTime, mStartTime ) ) ) };
+         const sampleCount s1 ( message.options.bySpeed
+            ? s0.as_double() +
+               llrint(dd.duration.as_double() * message.end) // end is a speed
+            : llrint(message.end * mRate)            // end is a time
+         );
+         auto actualDuration = std::max(sampleCount{1}, dd.duration);
+         auto success = mData.Init(nullptr,
+            s0, s1, actualDuration, message.options, mRate);
+         if ( !success ) {
+            // If not, we can wait to enqueue again later
+            dd.Cancel();
+            return;
+         }
+         mStarted = true;
       }
-      while(! mStopped.load( std::memory_order_relaxed )&&
+      else {
-           mMiddleIdx == mLeadingIdx)
+         Data newData;
-         mAvailable.Wait();
+         auto previous = &mData;
 
-      auto now = ::wxGetLocalTimeMillis();
+         // Use the previous end as NEW start.
+         const auto s0 = previous->mS1;
+         const sampleCount s1 ( message.options.bySpeed
+            ? s0.as_double() +
+               lrint(dd.duration.as_double() * message.end) // end is a speed
+            : lrint(message.end * mRate)            // end is a time
+         );
+         auto success =
+            newData.Init(previous, s0, s1, dd.duration, message.options, mRate);
+         if ( !success ) {
+            dd.Cancel();
+            return;
+         }
+         mData = newData;
+      }
 
-      if ( ! mStopped.load( std::memory_order_relaxed )  &&
+      if ( ! mStopped.load( std::memory_order_relaxed ) ) {
-           mMiddleIdx != mLeadingIdx ) {
+         Data &entry = mData;
-         Data &entry = mEntries[mMiddleIdx];
          if (entry.mDuration > 0) {
             // First use of the entry
             startSample = entry.mS0;
@@ -630,32 +605,27 @@ struct AudioIO::ScrubState
             duration = entry.mSilence;
             entry.mSilence = 0;
          }
-         if (entry.mSilence == 0) {
-            // Entry is used up
-            mTrailingIdx = mMiddleIdx;
-            mMiddleIdx = (mMiddleIdx + 1) % Size;
-         }
       }
       else {
          // We got the shut-down signal, or we discarded all the work.
-         startSample = endSample = duration = -1L;
+         // Output the -1 values.
       }
    }
 
    void Stop()
    {
       mStopped.store( true, std::memory_order_relaxed );
-      wxMutexLocker locker(mUpdating);
-      mAvailable.Signal();
    }
 
+#if 0
+   // Needed only for the DRAG_SCRUB experiment
+   // Should make mS1 atomic then?
    double LastTrackTime() const
    {
       // Needed by the main thread sometimes
-      wxMutexLocker locker(mUpdating);
+      return mData.mS1.as_double() / mRate;
-      const Data &previous = mEntries[(mLeadingIdx + Size - 1) % Size];
-      return previous.mS1.as_double() / mRate;
    }
+#endif
 
    ~ScrubState() {}
 
@@ -825,17 +795,17 @@ private:
       bool cancelled { false };
    };
 
-   enum { Size = 10 };
+   double mStartTime;
-   Data mEntries[Size];
+   bool mStarted{ false };
-   unsigned mTrailingIdx;
-   unsigned mMiddleIdx;
-   unsigned mLeadingIdx;
    std::atomic<bool> mStopped { false };
+   Data mData;
    const double mRate;
    wxLongLong mLastScrubTimeMillis;
-
+   struct Message {
-   mutable wxMutex mUpdating;
+      double end;
-   mutable wxCondition mAvailable { mUpdating };
+      ScrubbingOptions options;
+   };
+   MessageBuffer<Message> mMessage;
 };
 #endif
 
@@ -911,6 +881,8 @@ int audacityAudioCallback(const void *inputBuffer, void *outputBuffer,
 //
 //////////////////////////////////////////////////////////////////////
 
+#include <thread>
+
 #ifdef __WXMAC__
 
 // On Mac OS X, it's better not to use the wxThread class.
@@ -946,7 +918,6 @@ class AudioThread {
  private:
    bool mDestroy;
    pthread_t mThread;
-
 };
 
 #else
@@ -1986,7 +1957,7 @@ int AudioIO::StartStream(const TransportTracks &tracks,
       const auto &scrubOptions = *options.pScrubbingOptions;
       mScrubState =
          std::make_unique<ScrubState>(
-            mPlaybackSchedule.mT0, mPlaybackSchedule.mT1,
+            mPlaybackSchedule.mT0,
             scrubOptions.startClockTimeMillis,
             mRate,
             scrubOptions);
@@ -2761,13 +2732,11 @@ bool AudioIO::IsPaused() const
 }
 
 #ifdef EXPERIMENTAL_SCRUBBING_SUPPORT
-bool AudioIO::UpdateScrub
+void AudioIO::UpdateScrub
    (double endTimeOrSpeed, const ScrubbingOptions &options)
 {
    if (mScrubState)
-      return mScrubState->Update(endTimeOrSpeed, options);
+      mScrubState->Update(endTimeOrSpeed, options);
-   else
-      return false;
 }
 
 void AudioIO::StopScrub()
@@ -2776,6 +2745,8 @@ void AudioIO::StopScrub()
       mScrubState->Stop();
 }
 
+#if 0
+// Only for DRAG_SCRUB
 double AudioIO::GetLastScrubTime() const
 {
    if (mScrubState)
@@ -2783,6 +2754,7 @@ double AudioIO::GetLastScrubTime() const
    else
       return -1.0;
 }
+#endif
 
 #endif
 
@@ -3229,6 +3201,10 @@ AudioThread::ExitCode AudioThread::Entry()
 {
    while( !TestDestroy() )
    {
+      using Clock = std::chrono::steady_clock;
+      auto loopPassStart = Clock::now();
+      const auto interval = Scrubber::ScrubPollInterval_ms;
+
       // Set LoopActive outside the tests to avoid race condition
       gAudioIO->mAudioThreadFillBuffersLoopActive = true;
       if( gAudioIO->mAudioThreadShouldCallFillBuffersOnce )
@@ -3242,16 +3218,11 @@ AudioThread::ExitCode AudioThread::Entry()
       }
       gAudioIO->mAudioThreadFillBuffersLoopActive = false;
 
-      if (gAudioIO->mPlaybackSchedule.Interactive()) {
+      if ( gAudioIO->mPlaybackSchedule.Interactive() )
-         // Rely on the Wait() in ScrubState::Consumer()
+         std::this_thread::sleep_until(
-         // This allows the scrubbing update interval to be made very short without
+            loopPassStart + std::chrono::milliseconds( interval ) );
-         // playback becoming intermittent.
+      else
-      }
-      else {
-         // Perhaps this too could use a condition variable, for available space in the
-         // ring buffer, instead of a polling loop?  But no harm in doing it this way.
          Sleep(10);
-      }
    }
 
    return 0;
@@ -3848,7 +3819,6 @@ void AudioIO::FillBuffers()
          // PRL: or, when scrubbing, we may get work repeatedly from the
          // user interface.
          bool done = false;
-         Maybe<wxMutexLocker> cleanup;
          do {
             // How many samples to produce for each channel.
             auto frames = available;
@@ -3934,7 +3904,7 @@ void AudioIO::FillBuffers()
                {
                   sampleCount startSample, endSample;
                   mScrubState->Get(
-                     startSample, endSample, mScrubDuration, cleanup);
+                     startSample, endSample, mScrubDuration);
                   if (mScrubDuration < 0)
                   {
                      // Can't play anything

src/AudioIO.h

@@ -362,7 +362,7 @@ class AUDACITY_DLL_API AudioIO final {
    * scrub speed, adjust the beginning of the scrub interval rather than the
    * end, so that the scrub skips or "stutters" to stay near the cursor.
    */
-   bool UpdateScrub(double endTimeOrSpeed, const ScrubbingOptions &options);
+   void UpdateScrub(double endTimeOrSpeed, const ScrubbingOptions &options);
 
    void StopScrub();
 

src/tracks/ui/Scrubbing.cpp

@@ -522,14 +522,13 @@ void Scrubber::ContinueScrubbingPoll()
    // timer callback, to a left click event detected elsewhere.)
    const bool seek = TemporarilySeeks() || Seeks();
 
-   bool result = false;
    if (mPaused) {
       // When paused, make silent scrubs.
       mOptions.minSpeed = 0.0;
       mOptions.maxSpeed = mMaxSpeed;
       mOptions.adjustStart = false;
       mOptions.bySpeed = true;
-      result = gAudioIO->UpdateScrub(0, mOptions);
+      gAudioIO->UpdateScrub(0, mOptions);
    }
    else if (mSpeedPlaying) {
       // default speed of 1.3 set, so that we can hear there is a problem
@@ -543,7 +542,7 @@ void Scrubber::ContinueScrubbingPoll()
       mOptions.maxSpeed = speed +0.01;
       mOptions.adjustStart = false;
       mOptions.bySpeed = true;
-      result = gAudioIO->UpdateScrub(speed, mOptions);
+      gAudioIO->UpdateScrub(speed, mOptions);
    } else {
       const wxMouseState state(::wxGetMouseState());
       const auto trackPanel = mProject->GetTrackPanel();
@@ -558,7 +557,7 @@ void Scrubber::ContinueScrubbingPoll()
          mOptions.maxSpeed = mMaxSpeed;
          mOptions.adjustStart = true;
          mOptions.bySpeed = false;
-         result = gAudioIO->UpdateScrub(time, mOptions);
+         gAudioIO->UpdateScrub(time, mOptions);
          mLastScrubPosition = position.x;
       }
       else
@@ -572,17 +571,16 @@ void Scrubber::ContinueScrubbingPoll()
          if (mSmoothScrollingScrub) {
             const double speed = FindScrubSpeed(seek, time);
             mOptions.bySpeed = true;
-            result = gAudioIO->UpdateScrub(speed, mOptions);
+            gAudioIO->UpdateScrub(speed, mOptions);
          }
          else {
             mOptions.bySpeed = false;
-            result = gAudioIO->UpdateScrub(time, mOptions);
+            gAudioIO->UpdateScrub(time, mOptions);
          }
       }
    }
 
-   if (result)
+   mScrubSeekPress = false;
-      mScrubSeekPress = false;
 
    // else, if seek requested, try again at a later time when we might
    // enqueue a long enough stutter

src/tracks/ui/Scrubbing.h

@@ -76,7 +76,7 @@ public:
    
    Scrubber(AudacityProject *project);
    ~Scrubber();
-
+   
    // Assume xx is relative to the left edge of TrackPanel!
    void MarkScrubStart(wxCoord xx, bool smoothScrolling, bool seek);