Bug 2368: Effects with clip boundaries differing across channels...

... Wrong results could happen in effects that don't compute the channels
independently, such as the built-in Reverb.

To fix it, always fetch same position of left & right channels when
computing effects.

src/effects/Effect.cpp

@@ -1403,21 +1403,8 @@ bool Effect::ProcessPass()
          if (!left->GetSelected())
             return fallthrough();
 
-         sampleCount len;
-         sampleCount leftStart;
-         sampleCount rightStart = 0;
-
-         if (!isGenerator)
-         {
-            GetSamples(left, &leftStart, &len);
-            mSampleCnt = len;
-         }
-         else
-         {
-            len = 0;
-            leftStart = 0;
-            mSampleCnt = left->TimeToLongSamples(mDuration);
-         }
+         sampleCount len = 0;
+         sampleCount start = 0;
 
          mNumChannels = 0;
          WaveTrack *right{};
@@ -1444,14 +1431,19 @@ bool Effect::ProcessPass()
                // TODO: more-than-two-channels
                right = channel;
                clear = false;
-               if (!isGenerator)
-                  GetSamples(right, &rightStart, &len);
-
                // Ignore other channels
                break;
             }
          }
 
+         if (!isGenerator)
+         {
+            GetBounds(*left, right, &start, &len);
+            mSampleCnt = len;
+         }
+         else
+            mSampleCnt = left->TimeToLongSamples(mDuration);
+
          // Let the client know the sample rate
          SetSampleRate(left->GetRate());
 
@@ -1513,7 +1505,7 @@ bool Effect::ProcessPass()
 
          // Go process the track(s)
          bGoodResult = ProcessTrack(
-            count, map, left, right, leftStart, rightStart, len,
+            count, map, left, right, start, len,
             inBuffer, outBuffer, inBufPos, outBufPos);
          if (!bGoodResult)
             return;
@@ -1538,8 +1530,7 @@ bool Effect::ProcessTrack(int count,
                           ChannelNames map,
                           WaveTrack *left,
                           WaveTrack *right,
-                          sampleCount leftStart,
-                          sampleCount rightStart,
+                          sampleCount start,
                           sampleCount len,
                           FloatBuffers &inBuffer,
                           FloatBuffers &outBuffer,
@@ -1577,10 +1568,8 @@ bool Effect::ProcessTrack(int count,
    // there is no further input data to process, the loop continues to call the
    // effect with an empty input buffer until the effect has had a chance to
    // return all of the remaining delayed samples.
-   auto inLeftPos = leftStart;
-   auto inRightPos = rightStart;
-   auto outLeftPos = leftStart;
-   auto outRightPos = rightStart;
+   auto inPos = start;
+   auto outPos = start;
 
    auto inputRemaining = len;
    decltype(GetLatency()) curDelay = 0, delayRemaining = 0;
@@ -1633,10 +1622,10 @@ bool Effect::ProcessTrack(int count,
                limitSampleBufferSize( mBufferSize, inputRemaining );
 
             // Fill the input buffers
-            left->Get((samplePtr) inBuffer[0].get(), floatSample, inLeftPos, inputBufferCnt);
+            left->Get((samplePtr) inBuffer[0].get(), floatSample, inPos, inputBufferCnt);
             if (right)
             {
-               right->Get((samplePtr) inBuffer[1].get(), floatSample, inRightPos, inputBufferCnt);
+               right->Get((samplePtr) inBuffer[1].get(), floatSample, inPos, inputBufferCnt);
             }
 
             // Reset the input buffer positions
@@ -1739,8 +1728,7 @@ bool Effect::ProcessTrack(int count,
       // right channels when processing the input samples.  If we flip
       // over to processing delayed samples, they simply become counters
       // for the progress display.
-      inLeftPos += curBlockSize;
-      inRightPos += curBlockSize;
+      inPos += curBlockSize;
 
       // Get the current number of delayed samples and accumulate
       if (isProcessor)
@@ -1792,16 +1780,16 @@ bool Effect::ProcessTrack(int count,
          if (isProcessor)
          {
             // Write them out
-            left->Set((samplePtr) outBuffer[0].get(), floatSample, outLeftPos, outputBufferCnt);
+            left->Set((samplePtr) outBuffer[0].get(), floatSample, outPos, outputBufferCnt);
             if (right)
             {
                if (chans >= 2)
                {
-                  right->Set((samplePtr) outBuffer[1].get(), floatSample, outRightPos, outputBufferCnt);
+                  right->Set((samplePtr) outBuffer[1].get(), floatSample, outPos, outputBufferCnt);
                }
                else
                {
-                  right->Set((samplePtr) outBuffer[0].get(), floatSample, outRightPos, outputBufferCnt);
+                  right->Set((samplePtr) outBuffer[0].get(), floatSample, outPos, outputBufferCnt);
                }
             }
          }
@@ -1821,15 +1809,14 @@ bool Effect::ProcessTrack(int count,
          }
 
          // Bump to the next track position
-         outLeftPos += outputBufferCnt;
-         outRightPos += outputBufferCnt;
+         outPos += outputBufferCnt;
          outputBufferCnt = 0;
       }
 
       if (mNumChannels > 1)
       {
          if (TrackGroupProgress(count,
-               (inLeftPos - leftStart).as_double() /
+               (inPos - start).as_double() /
                (isGenerator ? genLength : len).as_double()))
          {
             rc = false;
@@ -1839,7 +1826,7 @@ bool Effect::ProcessTrack(int count,
       else
       {
          if (TrackProgress(count,
-               (inLeftPos - leftStart).as_double() /
+               (inPos - start).as_double() /
                (isGenerator ? genLength : len).as_double()))
          {
             rc = false;
@@ -1853,16 +1840,16 @@ bool Effect::ProcessTrack(int count,
    {
       if (isProcessor)
       {
-         left->Set((samplePtr) outBuffer[0].get(), floatSample, outLeftPos, outputBufferCnt);
+         left->Set((samplePtr) outBuffer[0].get(), floatSample, outPos, outputBufferCnt);
          if (right)
          {
             if (chans >= 2)
             {
-               right->Set((samplePtr) outBuffer[1].get(), floatSample, outRightPos, outputBufferCnt);
+               right->Set((samplePtr) outBuffer[1].get(), floatSample, outPos, outputBufferCnt);
             }
             else
             {
-               right->Set((samplePtr) outBuffer[0].get(), floatSample, outRightPos, outputBufferCnt);
+               right->Set((samplePtr) outBuffer[0].get(), floatSample, outPos, outputBufferCnt);
             }
          }
       }
@@ -2049,32 +2036,26 @@ bool Effect::TrackGroupProgress(int whichGroup, double frac, const TranslatableS
    return (updateResult != ProgressResult::Success);
 }
 
-void Effect::GetSamples(
-   const WaveTrack *track, sampleCount *start, sampleCount *len)
+void Effect::GetBounds(
+   const WaveTrack &track, const WaveTrack *pRight,
+   sampleCount *start, sampleCount *len)
 {
-   double trackStart = track->GetStartTime();
-   double trackEnd = track->GetEndTime();
-   double t0 = mT0 < trackStart ? trackStart : mT0;
-   double t1 = mT1 > trackEnd ? trackEnd : mT1;
+   auto t0 = std::max( mT0, track.GetStartTime() );
+   auto t1 = std::min( mT1, track.GetEndTime() );
 
-#if 0
-   if (GetType() & INSERT_EFFECT) {
-      t1 = t0 + mDuration;
-      if (mT0 == mT1) {
-         // Not really part of the calculation, but convenient to put here
-         track->InsertSilence(t0, t1);
-      }
+   if ( pRight ) {
+      t0 = std::min( t0, std::max( mT0, pRight->GetStartTime() ) );
+      t1 = std::max( t1, std::min( mT1, pRight->GetEndTime() ) );
    }
-#endif
 
    if (t1 > t0) {
-      *start = track->TimeToLongSamples(t0);
-      auto end = track->TimeToLongSamples(t1);
+      *start = track.TimeToLongSamples(t0);
+      auto end = track.TimeToLongSamples(t1);
       *len = end - *start;
    }
    else {
       *start = 0;
-      *len  = 0;
+      *len = 0;
    }
 }
 

src/effects/Effect.h

@@ -343,9 +343,10 @@ protected:
    int GetNumWaveTracks() { return mNumTracks; }
    int GetNumWaveGroups() { return mNumGroups; }
 
-   // Calculates the start time and selection length in samples
-   void GetSamples(
-      const WaveTrack *track, sampleCount *start, sampleCount *len);
+   // Calculates the start time and length in samples for one or two channels
+   void GetBounds(
+      const WaveTrack &track, const WaveTrack *pRight,
+      sampleCount *start, sampleCount *len);
 
    // Previewing linear effect can be optimised by pre-mixing. However this
    // should not be used for non-linear effects such as dynamic processors
@@ -487,8 +488,7 @@ protected:
                      ChannelNames map,
                      WaveTrack *left,
                      WaveTrack *right,
-                     sampleCount leftStart,
-                     sampleCount rightStart,
+                     sampleCount start,
                      sampleCount len,
                      FloatBuffers &inBuffer,
                      FloatBuffers &outBuffer,

src/effects/vamp/VampEffect.cpp

@@ -369,20 +369,17 @@ bool VampEffect::Process()
       auto channelGroup = TrackList::Channels(leader);
       auto left = *channelGroup.first++;
 
-      sampleCount lstart, rstart = 0;
-      sampleCount len;
-      GetSamples(left, &lstart, &len);
-
       unsigned channels = 1;
 
       // channelGroup now contains all but the first channel
       const WaveTrack *right =
          channelGroup.size() ? *channelGroup.first++ : nullptr;
       if (right)
-      {
          channels = 2;
-         GetSamples(right, &rstart, &len);
-      }
+
+      sampleCount start = 0;
+      sampleCount len = 0;
+      GetBounds(*left, right, &start, &len);
 
       // TODO: more-than-two-channels
 
@@ -447,8 +444,8 @@ bool VampEffect::Process()
       FloatBuffers data{ channels, block };
 
       auto originalLen = len;
-      auto ls = lstart;
-      auto rs = rstart;
+
+      auto pos = start;
 
       while (len != 0)
       {
@@ -456,12 +453,12 @@ bool VampEffect::Process()
 
          if (left)
          {
-            left->Get((samplePtr)data[0].get(), floatSample, ls, request);
+            left->Get((samplePtr)data[0].get(), floatSample, pos, request);
          }
 
          if (right)
          {
-            right->Get((samplePtr)data[1].get(), floatSample, rs, request);
+            right->Get((samplePtr)data[1].get(), floatSample, pos, request);
          }
 
          if (request < block)
@@ -478,7 +475,7 @@ bool VampEffect::Process()
          // UNSAFE_SAMPLE_COUNT_TRUNCATION
          // Truncation in case of very long tracks!
          Vamp::RealTime timestamp = Vamp::RealTime::frame2RealTime(
-            long( ls.as_long_long() ),
+            long( pos.as_long_long() ),
             (int)(mRate + 0.5)
          );
 
@@ -495,13 +492,12 @@ bool VampEffect::Process()
             len = 0;
          }
 
-         ls += step;
-         rs += step;
+         pos += step;
 
          if (channels > 1)
          {
             if (TrackGroupProgress(count,
-                  (ls - lstart).as_double() /
+                  (pos - start).as_double() /
                   originalLen.as_double() ))
             {
                return false;
@@ -510,7 +506,7 @@ bool VampEffect::Process()
          else
          {
             if (TrackProgress(count,
-                  (ls - lstart).as_double() /
+                  (pos - start).as_double() /
                   originalLen.as_double() ))
             {
                return false;