Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Joining multiple clips sometimes causes data loss #2226

Closed
jserquera opened this issue Dec 3, 2021 · 13 comments · Fixed by #2246
Closed

Joining multiple clips sometimes causes data loss #2226

jserquera opened this issue Dec 3, 2021 · 13 comments · Fixed by #2246
Assignees
@vsverchinsky
Labels
bug An error, undesired behaviour, or missed functionality P1 Highest level priority (ship blocker / must fix)
Projects
Sprint 10 - Enhancements&Bug fixes
Milestone
Audacity 3.1.3

Comments

@jserquera
Copy link

When trying to join many audio blocks at once, the result is not well done. I mean, using the Edit menu (I have the Spanish version: Recortar limites-> Unir).

You have to go one by one.

I think it happens in all Windows and Mac versions....
@LWinterberg
Copy link
Member

In the latest 3.1.3 build, having clips like this:
image

causes Select all followed by Edit > Clip Boundaries > Join to make this
image

Which seems correct to me.

@jserquera can you provide more information? What exactly is the problem? What Audacity version are you using? Can you maybe send a sample of the AUP3 file (you'll need to ZIP it first)?

@LWinterberg LWinterberg added the Could not reproduce We tried to reproduce this problem, but did not succeed. More detail needed? label Dec 3, 2021
@DavidBailes
Copy link
Collaborator

DavidBailes commented Dec 3, 2021 •
edited
Loading

Steps to reproduce:

  1. Split an audio clip into three clips
  2. Move the clips so that there are spaces between the clips.
  3. Select all the audio (Ctrl+A)
  4. Join the clips (Ctrl+J)
  5. Observe that clips are not joined correctly.

@LWinterberg
Copy link
Member

following your STR results in the following clip:
image

I still don't understand what the problem is.

@jserquera
Copy link
Author

Attached is a project that gives me the error
error.zip

@DavidBailes
Copy link
Collaborator

Using commit d2afbf (3.1.3 beta 1):
clips selected:
image

clips joined:
image

@LWinterberg
Copy link
Member

Ah. I'm on commit 9ca1f56 / CMake Build #3257 so it might be fixed in the final 3.1.3 after all.

@jserquera
Copy link
Author

Did you try with the project I sent?

@SteveDaulton
Copy link
Member

I can reproduce this bug on Linux with b0dade3

  1. Generate 10 second Chirp
  2. Select from about 3s to about 6s
  3. "Ctrl + i" to split
  4. Zoom out a bit
  5. Drag the third clip so that it ends at about 15 seconds
  6. Drag the second clip so that it is roughly mid way between the other two clips
  7. "Ctrl + A" to select All

before

8 "Ctrl + J" to join the clips

after

@DavidBailes
Copy link
Collaborator

Ah. I'm on commit 9ca1f56 / CMake Build #3257 so it might be fixed in the final 3.1.3 after all.

There aren't any commits in between those commits which should have fixed the problem.
The problem could well be intermittent. If I play around for a while, then sometimes it just starts working properly again.

I've had a quick look at the problem using master, where the problem also occurs.
I'm not that familiar this area of the code, but...
void WaveClip::InsertSilence( ) is called for each of the silences which are inserted using the STR.
The first time it's called, it does not execute ClearSequence(t, GetSequenceEndTime())
But the second time, it does execute that function, which I wasn't expecting.
If I comment out the two lines:
else if (t == GetPlayEndTime() && t < GetSequenceEndTime())
ClearSequence(t, GetSequenceEndTime());

Then this fixes the bug for me. I'm not suggesting this as a fix, just an indication of where the problem might be.

@DavidBailes
Copy link
Collaborator

Did you try with the project I sent?

I did try your project, and couldn't reproduce the problem. However, there is a problem, even if it can't be reproduced all the time.

@DavidBailes
Copy link
Collaborator

Ah. I'm on commit 9ca1f56 / CMake Build #3257 so it might be fixed in the final 3.1.3 after all.

Just for the record, I can reproduce the problem using commit 9ca1f56

@LWinterberg LWinterberg added bug An error, undesired behaviour, or missed functionality P3 Medium Priority and removed Could not reproduce We tried to reproduce this problem, but did not succeed. More detail needed? labels Dec 5, 2021
@LWinterberg LWinterberg changed the title error joining many audio blocks Joining multiple clips sometimes time-shifts them Dec 5, 2021
DavidBailes added a commit to DavidBailes/audacity that referenced this issue Dec 7, 2021
@DavidBailes
Issue audacity#2226: Joining multiple clips sometimes time-shifts them
e3cf76b 
Steps to reproduce:

1. Split an audio clip into three clips
2. Move the clips so that there are spaces between the clips.
3. Select all the audio (Ctrl+A)
4. Join the clips (Ctrl+J)
5. Observe that clips are not joined correctly.

Problem:
In  WaveClip::InsertSilence(), the following two lines:

else if (t == GetPlayEndTime() && t < GetSequenceEndTime())
      ClearSequence(t, GetSequenceEndTime());

After ClearSequence is called to remove the data to the end of the sequence, then there is not call to update mTrimRight, as is done in WaveClip::ClearRight().

Fix:
Update mTrimeRight.

Notes:
1. I don't know why the bug was not always reproduceable.
2. There appears to be a similar issue with the previous two lines:

   if (t == GetPlayStartTime() && t > GetSequenceStartTime())
      ClearSequence(GetSequenceStartTime(), t);

Compare with WaveClip::ClearLeft().
However I have left that unfixed, both because it doesn't affect this bug, and also because I couldn't work out when that call to ClearSequence would get called, and so I couldn't test any fix.
DavidBailes added a commit to DavidBailes/audacity that referenced this issue Dec 7, 2021
@DavidBailes
Issue audacity#2226: Joining multiple clips sometimes time-shifts them
667925a 
Steps to reproduce:

1. Split an audio clip into three clips
2. Move the clips so that there are spaces between the clips.
3. Select all the audio (Ctrl+A)
4. Join the clips (Ctrl+J)
5. Observe that clips are not joined correctly.

Problem:
In  WaveClip::InsertSilence(), the following two lines:

else if (t == GetPlayEndTime() && t < GetSequenceEndTime())
      ClearSequence(t, GetSequenceEndTime());

After ClearSequence is called to remove the data to the end of the sequence, then there is not call to update mTrimRight, as is done in WaveClip::ClearRight(). So when more data is pasted, it's pasted in the wrong place.

Fix:
Update mTrimeRight.

Notes:
1. I don't know why the bug was not always reproduceable.
2. There appears to be a similar issue with the previous two lines:

   if (t == GetPlayStartTime() && t > GetSequenceStartTime())
      ClearSequence(GetSequenceStartTime(), t);

Compare with WaveClip::ClearLeft().
However I have left that unfixed, both because it doesn't affect this bug, and also because I couldn't work out when that call to ClearSequence would get called, and so I couldn't test any fix.
@DavidBailes DavidBailes mentioned this issue Dec 7, 2021
Merged
@DavidBailes
Copy link
Collaborator

Using the steps to reproduce, if the length of the third clip is greater than the gap between the second and third clips, then data is lost - it's not just a question of incorrect time-shifting.
See, eg, #2226 (comment)

@SteveDaulton
Copy link
Member

I can confirm David's comment about data loss. This bug should be P1 in my opinion - destroying the user's data is arguably the worst possible bug.

@LWinterberg LWinterberg added P1 Highest level priority (ship blocker / must fix) and removed P3 Medium Priority labels Dec 7, 2021
@LWinterberg LWinterberg changed the title Joining multiple clips sometimes time-shifts them Joining multiple clips sometimes causes data loss Dec 7, 2021
@AnitaBats AnitaBats added this to To do in Sprint 10 - Enhancements&Bug fixes via automation Dec 8, 2021
@AnitaBats AnitaBats added this to the Audacity 3.1.3 milestone Dec 8, 2021
@AnitaBats AnitaBats moved this from To do to Review in progress in Sprint 10 - Enhancements&Bug fixes Dec 8, 2021
Sprint 10 - Enhancements&Bug fixes automation moved this from Review in progress to Ready for QA Dec 8, 2021
vsverchinsky pushed a commit that referenced this issue Dec 8, 2021
@DavidBailes @vsverchinsky
Issue #2226: Joining multiple clips sometimes time-shifts them
21b975e 
Steps to reproduce:

1. Split an audio clip into three clips
2. Move the clips so that there are spaces between the clips.
3. Select all the audio (Ctrl+A)
4. Join the clips (Ctrl+J)
5. Observe that clips are not joined correctly.

Problem:
In  WaveClip::InsertSilence(), the following two lines:

else if (t == GetPlayEndTime() && t < GetSequenceEndTime())
      ClearSequence(t, GetSequenceEndTime());

After ClearSequence is called to remove the data to the end of the sequence, then there is not call to update mTrimRight, as is done in WaveClip::ClearRight(). So when more data is pasted, it's pasted in the wrong place.

Fix:
Update mTrimeRight.

Notes:
1. I don't know why the bug was not always reproduceable.
2. There appears to be a similar issue with the previous two lines:

   if (t == GetPlayStartTime() && t > GetSequenceStartTime())
      ClearSequence(GetSequenceStartTime(), t);

Compare with WaveClip::ClearLeft().
However I have left that unfixed, both because it doesn't affect this bug, and also because I couldn't work out when that call to ClearSequence would get called, and so I couldn't test any fix.
vsverchinsky pushed a commit that referenced this issue Dec 8, 2021
@DavidBailes @vsverchinsky
Issue #2226: Joining multiple clips sometimes time-shifts them
c47d856 
Steps to reproduce:

1. Split an audio clip into three clips
2. Move the clips so that there are spaces between the clips.
3. Select all the audio (Ctrl+A)
4. Join the clips (Ctrl+J)
5. Observe that clips are not joined correctly.

Problem:
In  WaveClip::InsertSilence(), the following two lines:

else if (t == GetPlayEndTime() && t < GetSequenceEndTime())
      ClearSequence(t, GetSequenceEndTime());

After ClearSequence is called to remove the data to the end of the sequence, then there is not call to update mTrimRight, as is done in WaveClip::ClearRight(). So when more data is pasted, it's pasted in the wrong place.

Fix:
Update mTrimeRight.

Notes:
1. I don't know why the bug was not always reproduceable.
2. There appears to be a similar issue with the previous two lines:

   if (t == GetPlayStartTime() && t > GetSequenceStartTime())
      ClearSequence(GetSequenceStartTime(), t);

Compare with WaveClip::ClearLeft().
However I have left that unfixed, both because it doesn't affect this bug, and also because I couldn't work out when that call to ClearSequence would get called, and so I couldn't test any fix.

(cherry picked from commit 21b975e)
@Penikov Penikov moved this from Ready for QA to In QA in Sprint 10 - Enhancements&Bug fixes Dec 9, 2021
@Penikov Penikov moved this from In QA to Done in Sprint 10 - Enhancements&Bug fixes Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vsverchinsky vsverchinsky

Labels
bug An error, undesired behaviour, or missed functionality P1 Highest level priority (ship blocker / must fix)
Projects
No open projects
Sprint 10 - Enhancements&Bug fixes
Done
Milestone
Audacity 3.1.3
Development

Successfully merging a pull request may close this issue.

Issue #2226: Joining multiple clips sometimes time-shifts them DavidBailes/audacity
6 participants
@SteveDaulton @DavidBailes @vsverchinsky @jserquera @AnitaBats @LWinterberg