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.

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

Fixes issue with track length mismatch after resample #2136

Conversation

vsverchinsky
Copy link
Collaborator

@vsverchinsky vsverchinsky commented Nov 15, 2021

It could happen so that after resampling number of samples contained in GetPlayEndTime() - GetPlayStartTime()
didn't match to number of samples in GetPlayEndSample() - GetPlayStartSample()

Resolves: #2113

  • I signed CLA
  • The title of the pull request describes an issue it addresses
  • If changes are extensive, then there is a sequence of easily reviewable commits
  • Each commit's message describes its purpose and effects
  • There are no behavior changes unnecessary for the stated purpose of the PR

Recommended:

  • Each commit compiles and runs on my machine without known undesirable changes of behavior

It could happen so that after resampling number of samples contained in GetPlayEndTime() - GetPlayStartTime()
didn't match to number of samples in GetPlayEndSample() - GetPlayStartSample(),
@vsverchinsky vsverchinsky requested a review from Paul-Licameli Nov 15, 2021
@AnitaBats AnitaBats added this to In progress in Sprint 8 - 3.1 Stabilisation via automation Nov 15, 2021
@AnitaBats AnitaBats moved this from In progress to Review in progress in Sprint 8 - 3.1 Stabilisation Nov 15, 2021
@Paul-Licameli
Copy link
Member

Paul-Licameli commented Nov 15, 2021

The small changes look harmless, but I don't understand why they fix a crash.

Would it be better to apply the small corrections in mTrimLeft and mTrimRight within WaveClip::Resample?

@vsverchinsky
Copy link
Collaborator Author

vsverchinsky commented Nov 15, 2021

The small changes look harmless, but I don't understand why they fix a crash.

Because the amount of samples returned by WaveClip::TimeToSamples(trim) depends on current sample rate. When applying resampling it may happen so that trim value does not exactly match the beginning of the sample any more. And the bug comes from difference in the way number of samples is calculated: WaveClip::GetPlaySamplesCount adjusts trimmings to the sample rate, and WaveClip::GetPlayEndTime() and others - do not, so WaveClip::TimeToSamples(WaveClip::GetPlayEndTime() - WaveClip::GetPlayStartTime()) may not be equal to WaveClip::GetPlaySamplesCount, sometimes leading to an OOR.

Would it be better to apply the small corrections in mTrimLeft and mTrimRight within WaveClip::Resample?

It may work in that particular case, but I don't see why it's better. Functions mentioned above still return different values, which should be fixed anyway, so it becomes unnecessary to fix Resample. Also, resample(newSampleRate)->resample(originalSampleRate) may change original trimming values, while with this fix preserves them.

@Paul-Licameli Paul-Licameli merged commit 1efd888 into audacity:release-3.1.2 Nov 16, 2021
4 checks passed
Sprint 8 - 3.1 Stabilisation automation moved this from Review in progress to Ready for QA Nov 16, 2021
@vsverchinsky vsverchinsky deleted the i2113-fix-track-length-mismatch branch Nov 16, 2021
@Penikov Penikov moved this from Ready for QA to Done in Sprint 8 - 3.1 Stabilisation Nov 16, 2021
@LWinterberg LWinterberg linked an issue Nov 16, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Smart Clips. In some cases Audacity crashes at exporting
2 participants