Creation of spurious extra clips (was: Reverse does not clip reverse boundaries)

[Vidyashankar Vellal]

When applying certain effects (Change Speed, Generate DTMF and may be
more), extra clips are sometimes created near the end of the selection
region. These extra clips either have a very small length (order of a
few samples) or have zero length (same start and end times).

I will focus on the Change Speed effect in this post. Corrections to
this may correct the others.

Consider the following cases:

Case 1: New project, generate chirp 30 sec
Case 2: Steps of case 1 + select 5 to 10 sec and edit->split delete
Case 3: Steps of case 1 + split at 5 sec and move to 10 sec

Do effect->change speed 10% after each of the cases. Notice the split
line in the end. This indicates extra clips.

The following changes seem to solve the problem with change speed.

Change 1: In ChangeSpeed.cpp, the new end time is calculated using a
formula (line 253). Instead, set the new end time to be
outputTrack->GetEndTime()
outputTrack here refers to the new track created which has the changed
speed. When the new end time is set using the formula, it is usually
one sample lower than the outputTrack's end time. This difference is
due to approximation in the calculation.

Change 2: In WaveTrack::ClearAndPaste, two of the parameters are t0
and t1 which refer to the start and end time to clear. In the code, t0
and t1 are aligned to samples (see near line 555 in WaveTrack.cpp).
Later the sample-aligned values of t0 and t1 are used to clear. The
change is to use the original values of t0 and t1 to clear.

Change 3: In WaveTrack::SplitAt function, the parameter is the time at
which the split should be made. Currently, this time is compared with
each clip's start and end times to determine in which clip the split
should occur. The change is to compare samples instead of times.
Like I said before, this extra clips problem is mainly due to time
approximations. Sometimes the time parameter is different from a
clip's end time by an extremely tiny amount (different at the 8th
decimal point for example), but the samples are the same (i.e. when
you use TimeToLongSamples function). And the code continues to split
at this time which ultimately results in an empty clip.

I sometimes feel that there is probably a much simpler change to make
instead of all the above changes. If anybody has any ideas, please
share them. Also, let me know if you think these changes are
incorrect/inappropriate. And pardon me for such a long post.

I have attached a patch with the above changes made against the latest
HEAD. This seems to fix extra clips during change speed.

Thanks

Vidyashankar

[ExtraClips1.patch]

---

? ExtraClips1.patch
Index: src/WaveTrack.cpp
===================================================================
RCS file: /cvsroot/audacity/audacity-src/src/WaveTrack.cpp,v
retrieving revision 1.171
diff -u -r1.171 WaveTrack.cpp
--- src/WaveTrack.cpp 20 Oct 2009 23:29:17 -0000 1.171
+++ src/WaveTrack.cpp 31 Oct 2009 22:26:45 -0000
@@ -551,6 +551,9 @@
       warper = new IdentityTimeWarper();
    }
 
+   // Store original values
+   double origT0 = t0;
+   double origT1 = t1;  
    // Align to a sample
    t0 = LongSamplesToTime(TimeToLongSamples(t0));
    t1 = LongSamplesToTime(TimeToLongSamples(t1));
@@ -600,7 +603,7 @@
    }
 
    // Now, clear the selection
-   if (HandleClear(t0, t1, false, false)) {
+   if (HandleClear(origT0, origT1, false, false)) {
 
       // The pasting method depends on how this method was called
       bool pasteResult;
@@ -2041,7 +2044,8 @@
    for (WaveClipList::compatibility_iterator it=GetClipIterator(); it; it=it->GetNext())
    {
       WaveClip* c = it->GetData();
-      if (t > c->GetStartTime() && t < c->GetEndTime())
+      sampleCount splitSample = TimeToLongSamples(t);
+      if (splitSample > c->GetStartSample() && splitSample < c->GetEndSample())
       {
          double val;
          t = LongSamplesToTime(TimeToLongSamples(t)); // put t on a sample
Index: src/effects/ChangeSpeed.cpp
===================================================================
RCS file: /cvsroot/audacity/audacity-src/src/effects/ChangeSpeed.cpp,v
retrieving revision 1.64
diff -u -r1.64 ChangeSpeed.cpp
--- src/effects/ChangeSpeed.cpp 16 Aug 2009 14:44:20 -0000 1.64
+++ src/effects/ChangeSpeed.cpp 31 Oct 2009 22:24:56 -0000
@@ -250,7 +250,7 @@
    // sample data
  if (bLoopSuccess) {
 
-      double mT1Dashed = mT0 + (mT1 - mT0)/(m_PercentChange/100.0 + 1.0);
+      double mT1Dashed = outputTrack->GetEndTime();
       SetTimeWarper(new LinearTimeWarper(mT0, mT0, mT1, mT1Dashed ));
       track->ClearAndPaste(mCurT0, mCurT1, outputTrack, true, false, mOutputTracks, true, first, GetTimeWarper());
 

---

------------------------------------------------------------------------------
Come build with us! The BlackBerry(R) Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel

[Martyn Shaw-2]

Hi Vidyashankar

Thanks for looking into these extremely small clips, which are very
important.  Those Envelope asserts kicking off may well be due to them
as well.

I think the commit log on
http://audacity.cvs.sourceforge.net/viewvc/audacity/audacity-src/src/effects/ChangeSpeed.cpp?sortby=date&view=log 
revision 1.64 is relevant here.

Vidyashankar Vellal wrote:

> When applying certain effects (Change Speed, Generate DTMF and may be
> more), extra clips are sometimes created near the end of the selection
> region. These extra clips either have a very small length (order of a
> few samples) or have zero length (same start and end times).
>
> I will focus on the Change Speed effect in this post. Corrections to
> this may correct the others.
>
> Consider the following cases:
>
> Case 1: New project, generate chirp 30 sec
> Case 2: Steps of case 1 + select 5 to 10 sec and edit->split delete
> Case 3: Steps of case 1 + split at 5 sec and move to 10 sec
>
> Do effect->change speed 10% after each of the cases. Notice the split
> line in the end. This indicates extra clips.
>
> The following changes seem to solve the problem with change speed.
>
> Change 1: In ChangeSpeed.cpp, the new end time is calculated using a
> formula (line 253). Instead, set the new end time to be
> outputTrack->GetEndTime()
> outputTrack here refers to the new track created which has the changed
> speed. When the new end time is set using the formula, it is usually
> one sample lower than the outputTrack's end time. This difference is
> due to approximation in the calculation.

I can see this working and have committed an equivalent change to your
patch.  I see that we are using different times for different things
and this is causing the problem.

> Change 2: In WaveTrack::ClearAndPaste, two of the parameters are t0
> and t1 which refer to the start and end time to clear. In the code, t0
> and t1 are aligned to samples (see near line 555 in WaveTrack.cpp).
> Later the sample-aligned values of t0 and t1 are used to clear. The
> change is to use the original values of t0 and t1 to clear.

I have not committed this as I don't know exactly what problem you are
trying to solve.  Can you give us a clear indication please?  Is it
related to case 2 & 3 above?

> Change 3: In WaveTrack::SplitAt function, the parameter is the time at
> which the split should be made. Currently, this time is compared with
> each clip's start and end times to determine in which clip the split
> should occur. The change is to compare samples instead of times.
> Like I said before, this extra clips problem is mainly due to time
> approximations. Sometimes the time parameter is different from a
> clip's end time by an extremely tiny amount (different at the 8th
> decimal point for example), but the samples are the same (i.e. when
> you use TimeToLongSamples function). And the code continues to split
> at this time which ultimately results in an empty clip.

As above.

> I sometimes feel that there is probably a much simpler change to make
> instead of all the above changes. If anybody has any ideas, please
> share them.

I wish I could.  The 'time' vs 'samples' thing has been discussed
before, and I think I was shouted down with a good argument to use
'time' mostly, but I don't recall the details.

  Also, let me know if you think these changes are
> incorrect/inappropriate. And pardon me for such a long post.

No problem here, some useful work!

TTFN
Martyn

> I have attached a patch with the above changes made against the latest
> HEAD. This seems to fix extra clips during change speed.
>
> Thanks
>
> Vidyashankar
>
>
> ------------------------------------------------------------------------
>
> ------------------------------------------------------------------------------
> Come build with us! The BlackBerry(R) Developer Conference in SF, CA
> is the only developer event you need to attend this year. Jumpstart your
> developing skills, take BlackBerry mobile applications to market and stay
> ahead of the curve. Join us from November 9 - 12, 2009. Register now!
> http://p.sf.net/sfu/devconference
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> audacity-devel mailing list
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/audacity-devel

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel

[Al Dimond]

I noticed a couple problems similar to this, remembered this
discussion, and noticed that this patch from Vidyashankar hadn't
received any attention.  Related to "P2: Boundary drawing problem with
Generate and SoundTouch".  A couple notes below.  tl;dr version at the
very bottom.

On Saturday 31 October 2009 16:31:33 Vidyashankar Vellal wrote:

> When applying certain effects (Change Speed, Generate DTMF and may
>  be more), extra clips are sometimes created near the end of the
>  selection region. These extra clips either have a very small
>  length (order of a few samples) or have zero length (same start
>  and end times).
>
> I will focus on the Change Speed effect in this post. Corrections
>  to this may correct the others.
>
> Consider the following cases:
>
> Case 1: New project, generate chirp 30 sec
> Case 2: Steps of case 1 + select 5 to 10 sec and edit->split delete
> Case 3: Steps of case 1 + split at 5 sec and move to 10 sec
>
> Do effect->change speed 10% after each of the cases. Notice the
>  split line in the end. This indicates extra clips.
>
> The following changes seem to solve the problem with change speed.
>
> Change 1: In ChangeSpeed.cpp, the new end time is calculated using
>  a formula (line 253). Instead, set the new end time to be
> outputTrack->GetEndTime()
> outputTrack here refers to the new track created which has the
>  changed speed. When the new end time is set using the formula, it
>  is usually one sample lower than the outputTrack's end time. This
>  difference is due to approximation in the calculation.
>

A similar change to this was made, and it's actually not completely
correct.  outputTrack starts at the beginning of the selection, so it
needs to have t0 added to it... but this seems to cause assertions --
it's trying to do a split, outside of the track by less than a sample.

> Change 2: In WaveTrack::ClearAndPaste, two of the parameters are t0
> and t1 which refer to the start and end time to clear. In the code,
>  t0 and t1 are aligned to samples (see near line 555 in
>  WaveTrack.cpp). Later the sample-aligned values of t0 and t1 are
>  used to clear. The change is to use the original values of t0 and
>  t1 to clear.
>

I'm not sure why this is better.  Won't Clear() just round to sample-
aligned values anyway?

> Change 3: In WaveTrack::SplitAt function, the parameter is the time
>  at which the split should be made. Currently, this time is
>  compared with each clip's start and end times to determine in
>  which clip the split should occur. The change is to compare
>  samples instead of times. Like I said before, this extra clips
>  problem is mainly due to time approximations. Sometimes the time
>  parameter is different from a clip's end time by an extremely tiny
>  amount (different at the 8th decimal point for example), but the
>  samples are the same (i.e. when you use TimeToLongSamples
>  function). And the code continues to split at this time which
>  ultimately results in an empty clip.
>

The same effect can be had by shifting the existing line:
t = LongSamplesToTime(TimeToLongSamples(t));
out of the if block.  This gets rid of the assertions and zero-length
extra clips.  Sometimes there are 1-sample extra clips, but I don't
think that's to be fixed in SplitAt().

> I sometimes feel that there is probably a much simpler change to
>  make instead of all the above changes. If anybody has any ideas,
>  please share them. Also, let me know if you think these changes
>  are incorrect/inappropriate. And pardon me for such a long post.
>

I think the best change would be to have the Change Speed effect
iterate over clips the way other effects do, instead of using
WaveTrack::Get(), which removes all the clip boundaries, and then
trying to add them back in in WaveTrack::ClearAndPaste().  It wouldn't
be simple in the "fewer lines of code" sense, but it is simpler in
that it doesn't ask us to do perform the rather difficult task of saving
and restoring clip boundaries in WaveTrack::ClearAndPaste() -- that's
just asking for errors, one-sample extra clips, etc.

I'm going to take a crack at iterating over clips properly, for Change
Speed and the SoundTouch effects.  Time permitting I'll try SBSMS,
which does something similar but is a little more complicated.  And
then generators.  I'm not sure how much of this I'll be able to get
in, though.

> I have attached a patch with the above changes made against the
>  latest HEAD. This seems to fix extra clips during change speed.
>
> Thanks
>
> Vidyashankar
>

TL;DR version: I'm putting in a couple small changes along the lines
of Vidyashankar's patch, and then ultimately I'd like to make Change
Speed, the SoundTouch effects, SBSMS, and the Generators (in that
order, if there's not time to get all of them well-tested) iterate
over clips instead of relying on ClearAndPaste() to restore split
lines.

Comments?

 - Al

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel