Last active 3 months ago

#8 Contributing instructions? a month ago

Comment by ~cannam on ~breakfastquay/rubberband

Thanks for querying this! For short patches, I actually quite like them just pasted into the issue or comment text in the tracker, surrounded by the Markdown triple-backquotes for code formatting.

However I probably should set up a list as well, it would make more sense for longer patches and back-and-forth. I've only relatively recently started doing serious work with this platform, so I'm still gradually figuring out a sensible approach. I'll leave this issue open as a reminder - not just to decide on this but also to document it.

#2 Unbounded memory usage and non-termination with certain tempo maps 3 months ago

Comment by ~cannam on ~breakfastquay/rubberband

This should be fixed now in commit:b5f64f41c499.

The problem resulted from the handling of very extreme ratios at the end of the timemap. In the case above, there is a timemap specified that (overall) reduces the audio duration from about 11 million to about 9 million samples - but the --time 1.0 option is also specified, and this means that Rubber Band will attempt to produce a file with unchanged duration, i.e. the original 11 million samples. So at the end of the area defined by the timemap, there is a period spanning effectively nothing at the input side, that has to somehow map to over 2 million samples at the output.

The code can deal with this, and in fact the loop it had entered was not an infinite one - given enough time and memory it would have completed the job eventually. But it required reallocating one of the internal buffers to a great duration, and unfortunately it was doing this only a few samples at a time, as it kept adding each new frame of output and finding it still hadn't got enough. It was this constant reallocation that was the problem, especially as the old buffers weren't being reaped until after the whole sequence had completed.

The obvious fix is to do as container implementations everywhere do, and reallocate to a multiple of the original size (e.g. twice the length) each time instead of just adding a bit. This reduces the overall time taken from "essentially infinite" to "essentially unnoticeable", which, while not nothing, is I think acceptable for such an extreme case.

We obviously also need to clarify the use of the --timemap and --time options together, as the documentation in the command-line tool usage output is unhelpful. The original submitter of this report had been surprised to find they needed to use --time or related arguments at all when --timemap was provided. The reason for it is that the timemap is not required (or expected) to span the whole of the file - you can provide a timemap with just one or two key points in it and they don't have to include the start or end of the file - and providing the global option as well is the simplest way of telling Rubber Band where to place the end of the audio.

#6 Audible clicks when changing pitch-shift factor 3 months ago

Comment by ~cannam on ~breakfastquay/rubberband

(same author but switching to personal login)

Some investigation later, I found two separate problems that both produce similar-sounding artifacts (when using libsamplerate - as mentioned above, there are other problems with the other libraries and that's not changing just at the moment):

  • In Resampler the output frame space provided needed to be (at least) one sample larger, or else the build-up of accumulated fractional samples would eventually result in a lost sample. Fixed in commit:dbfc749bce16 in the default branch.

  • In StretcherProcess::writeChunk, when zeroing the tail of the accumulator and window accumulator after shifting, the buffer was zeroed starting from the current window length - but it was possible that it still had valid values after that point in the case where the window length had been reduced recently because of a change in ratio. This code needed to use the accumulator-fill length instead of the current window length. Fixed in commit:6c0f84bf30b3 in the default branch.

I'm still reviewing these and looking at some regression tests but so far these appear to amount to a complete fix for the issue. Any feedback based on testing the current default branch (with libsamplerate) is welcome.