From dabf1d4eb9ec172bf48026e8975bd2610905b163 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Wed, 7 Dec 2022 00:25:39 +0100 Subject: [PATCH 01/13] Mark kRubberBandBlockSize constexpr --- src/engine/bufferscalers/enginebufferscalerubberband.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/bufferscalers/enginebufferscalerubberband.cpp b/src/engine/bufferscalers/enginebufferscalerubberband.cpp index a24b21df70f..d6e57aab49e 100644 --- a/src/engine/bufferscalers/enginebufferscalerubberband.cpp +++ b/src/engine/bufferscalers/enginebufferscalerubberband.cpp @@ -19,7 +19,7 @@ namespace { // TODO (XXX): this should be removed. It is only needed to work around // a Rubberband 1.3 bug. // This is the default increment from RubberBand 1.8.1. -size_t kRubberBandBlockSize = 256; +constexpr size_t kRubberBandBlockSize = 256; #define RUBBERBANDV3 (RUBBERBAND_API_MAJOR_VERSION >= 2 && RUBBERBAND_API_MINOR_VERSION >= 7) From d4ad20d324347e19c098b4931e270474f8718a65 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Wed, 7 Dec 2022 22:07:04 +0100 Subject: [PATCH 02/13] Replace manual memory management in rubberband These functions still use raw pointers so there's a bunch of stuff that's difficult to catch that can still go wrong, but this at least removes the need for having to manually deallocate these structures. And this class didn't adhere to the rule of five, so it was very easy to misuse it and cause double frees and use after frees. --- .../enginebufferscalerubberband.cpp | 31 +++++++++---------- .../enginebufferscalerubberband.h | 23 ++++++++++++-- 2 files changed, 34 insertions(+), 20 deletions(-) diff --git a/src/engine/bufferscalers/enginebufferscalerubberband.cpp b/src/engine/bufferscalers/enginebufferscalerubberband.cpp index d6e57aab49e..3df1cbfb993 100644 --- a/src/engine/bufferscalers/enginebufferscalerubberband.cpp +++ b/src/engine/bufferscalers/enginebufferscalerubberband.cpp @@ -28,22 +28,16 @@ constexpr size_t kRubberBandBlockSize = 256; EngineBufferScaleRubberBand::EngineBufferScaleRubberBand( ReadAheadManager* pReadAheadManager) : m_pReadAheadManager(pReadAheadManager), - m_buffer_back(SampleUtil::alloc(MAX_BUFFER_LEN)), + m_buffers{std::vector(MAX_BUFFER_LEN), std::vector(MAX_BUFFER_LEN)}, + m_bufferPtrs{m_buffers[0].data(), m_buffers[1].data()}, + m_interleavedReadBuffer(MAX_BUFFER_LEN), m_bBackwards(false), m_useEngineFiner(false) { - m_retrieve_buffer[0] = SampleUtil::alloc(MAX_BUFFER_LEN); - m_retrieve_buffer[1] = SampleUtil::alloc(MAX_BUFFER_LEN); // Initialize the internal buffers to prevent re-allocations // in the real-time thread. onSampleRateChanged(); } -EngineBufferScaleRubberBand::~EngineBufferScaleRubberBand() { - SampleUtil::free(m_buffer_back); - SampleUtil::free(m_retrieve_buffer[0]); - SampleUtil::free(m_retrieve_buffer[1]); -} - void EngineBufferScaleRubberBand::setScaleParameters(double base_rate, double* pTempoRatio, double* pPitchRatio) { @@ -151,22 +145,25 @@ SINT EngineBufferScaleRubberBand::retrieveAndDeinterleave( SINT frames_available = m_pRubberBand->available(); SINT frames_to_read = math_min(frames_available, frames); SINT received_frames = static_cast(m_pRubberBand->retrieve( - m_retrieve_buffer, frames_to_read)); + m_bufferPtrs.data(), frames_to_read)); + + DEBUG_ASSERT(received_frames <= static_cast(m_buffers[0].size())); SampleUtil::interleaveBuffer(pBuffer, - m_retrieve_buffer[0], - m_retrieve_buffer[1], - received_frames); + m_buffers[0].data(), + m_buffers[1].data(), + received_frames); return received_frames; } void EngineBufferScaleRubberBand::deinterleaveAndProcess( const CSAMPLE* pBuffer, SINT frames, bool flush) { + DEBUG_ASSERT(frames <= static_cast(m_buffers[0].size())); SampleUtil::deinterleaveBuffer( - m_retrieve_buffer[0], m_retrieve_buffer[1], pBuffer, frames); + m_buffers[0].data(), m_buffers[1].data(), pBuffer, frames); - m_pRubberBand->process(m_retrieve_buffer, + m_pRubberBand->process(m_bufferPtrs.data(), frames, flush); } @@ -234,13 +231,13 @@ double EngineBufferScaleRubberBand::scaleBuffer( if (iAvailFrames > 0) { last_read_failed = false; - deinterleaveAndProcess(m_buffer_back, iAvailFrames, false); + deinterleaveAndProcess(m_interleavedReadBuffer.data(), iAvailFrames, false); } else { if (last_read_failed) { // Flush and break out after the next retrieval. If we are // at EOF this serves to get the last samples out of // RubberBand. - deinterleaveAndProcess(m_buffer_back, 0, true); + deinterleaveAndProcess(m_interleavedReadBuffer.data(), 0, true); break_out_after_retrieve_and_reset_rubberband = true; } last_read_failed = true; diff --git a/src/engine/bufferscalers/enginebufferscalerubberband.h b/src/engine/bufferscalers/enginebufferscalerubberband.h index ec5e2248c88..c84e4c1a944 100644 --- a/src/engine/bufferscalers/enginebufferscalerubberband.h +++ b/src/engine/bufferscalers/enginebufferscalerubberband.h @@ -1,5 +1,8 @@ #pragma once +#include +#include + #include "engine/bufferscalers/enginebufferscale.h" #include "util/memory.h" @@ -15,7 +18,12 @@ class EngineBufferScaleRubberBand : public EngineBufferScale { public: explicit EngineBufferScaleRubberBand( ReadAheadManager* pReadAheadManager); - ~EngineBufferScaleRubberBand() override; + + EngineBufferScaleRubberBand(const EngineBufferScaleRubberBand&) = delete; + EngineBufferScaleRubberBand& operator=(const EngineBufferScaleRubberBand&) = delete; + + EngineBufferScaleRubberBand(EngineBufferScaleRubberBand&&) = delete; + EngineBufferScaleRubberBand& operator=(EngineBufferScaleRubberBand&&) = delete; // Let EngineBuffer know if engine v3 is available static bool isEngineFinerAvailable(); @@ -48,8 +56,17 @@ class EngineBufferScaleRubberBand : public EngineBufferScale { std::unique_ptr m_pRubberBand; - CSAMPLE* m_retrieve_buffer[2]; - CSAMPLE* m_buffer_back; + /// The audio buffers samples used to send audio to Rubber Band and to + /// receive processed audio from Rubber Band. This is needed because Mixxx + /// uses interleaved buffers in most other places. + std::array, 2> m_buffers; + /// These point to the buffers in `m_buffers`. They can be defined here + /// since this object cannot be moved or copied. + std::array m_bufferPtrs; + + /// Contains interleaved samples read from `m_pReadAheadManager`. These need + /// to be deinterleaved before they can be passed to Rubber Band. + std::vector m_interleavedReadBuffer; // Holds the playback direction bool m_bBackwards; From e1db59ae2b4239a4beecdeb8f25020021e71d893 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Wed, 7 Dec 2022 22:33:30 +0100 Subject: [PATCH 03/13] Run required padding through Rubber Band on reset This needs to be done or else Rubber Band will eat up the initial transient by introducing a short fade in/2048 sample low-pass. --- .../enginebufferscalerubberband.cpp | 47 ++++++++++++++----- .../enginebufferscalerubberband.h | 10 ++-- 2 files changed, 42 insertions(+), 15 deletions(-) diff --git a/src/engine/bufferscalers/enginebufferscalerubberband.cpp b/src/engine/bufferscalers/enginebufferscalerubberband.cpp index 3df1cbfb993..f57e25dbe88 100644 --- a/src/engine/bufferscalers/enginebufferscalerubberband.cpp +++ b/src/engine/bufferscalers/enginebufferscalerubberband.cpp @@ -1,7 +1,5 @@ #include "engine/bufferscalers/enginebufferscalerubberband.h" -#include - #include #include "control/controlobject.h" @@ -136,7 +134,7 @@ void EngineBufferScaleRubberBand::clear() { VERIFY_OR_DEBUG_ASSERT(m_pRubberBand) { return; } - m_pRubberBand->reset(); + reset(); } SINT EngineBufferScaleRubberBand::retrieveAndDeinterleave( @@ -196,10 +194,10 @@ double EngineBufferScaleRubberBand::scaleBuffer( read += getOutputSignal().frames2samples(received_frames); if (break_out_after_retrieve_and_reset_rubberband) { - //qDebug() << "break_out_after_retrieve_and_reset_rubberband"; + // qDebug() << "break_out_after_retrieve_and_reset_rubberband"; // If we break out early then we have flushed RubberBand and need to // reset it. - m_pRubberBand->reset(); + reset(); break; } @@ -218,15 +216,15 @@ double EngineBufferScaleRubberBand::scaleBuffer( iLenFramesRequired = kRubberBandBlockSize; } } - //qDebug() << "iLenFramesRequired" << iLenFramesRequired; + // qDebug() << "iLenFramesRequired" << iLenFramesRequired; if (remaining_frames > 0 && iLenFramesRequired > 0) { SINT iAvailSamples = m_pReadAheadManager->getNextSamples( - // The value doesn't matter here. All that matters is we - // are going forward or backward. - (m_bBackwards ? -1.0 : 1.0) * m_dBaseRate * m_dTempoRatio, - m_buffer_back, - getOutputSignal().frames2samples(iLenFramesRequired)); + // The value doesn't matter here. All that matters is we + // are going forward or backward. + (m_bBackwards ? -1.0 : 1.0) * m_dBaseRate * m_dTempoRatio, + m_interleavedReadBuffer.data(), + getOutputSignal().frames2samples(iLenFramesRequired)); SINT iAvailFrames = getOutputSignal().samples2frames(iAvailSamples); if (iAvailFrames > 0) { @@ -283,3 +281,30 @@ int EngineBufferScaleRubberBand::runningEngineVersion() { return 2; #endif } + +void EngineBufferScaleRubberBand::reset() { + m_pRubberBand->reset(); + + // As mentioned in the docs (https://breakfastquay.com/rubberband/code-doc/) + // and FAQ (https://breakfastquay.com/rubberband/integration.html#faqs), you + // need to run some silent samples through the time stretching engine first + // before using it. Otherwise it will eat add a short fade-in, destroying + // the initial transient. + size_t remaining_padding = m_pRubberBand->getPreferredStartPad(); + std::fill_n(m_buffers[0].begin(), kRubberBandBlockSize, 0.0f); + std::fill_n(m_buffers[1].begin(), kRubberBandBlockSize, 0.0f); + while (remaining_padding > 0) { + const size_t pad_samples = std::min(remaining_padding, kRubberBandBlockSize); + m_pRubberBand->process(m_bufferPtrs.data(), pad_samples, false); + + remaining_padding -= pad_samples; + } + + size_t padding_to_drop = m_pRubberBand->getStartDelay(); + while (padding_to_drop > 0) { + const size_t drop_samples = std::min(padding_to_drop, kRubberBandBlockSize); + m_pRubberBand->retrieve(m_bufferPtrs.data(), drop_samples); + + padding_to_drop -= drop_samples; + } +} diff --git a/src/engine/bufferscalers/enginebufferscalerubberband.h b/src/engine/bufferscalers/enginebufferscalerubberband.h index c84e4c1a944..d1c4a651b94 100644 --- a/src/engine/bufferscalers/enginebufferscalerubberband.h +++ b/src/engine/bufferscalers/enginebufferscalerubberband.h @@ -1,15 +1,13 @@ #pragma once +#include + #include #include #include "engine/bufferscalers/enginebufferscale.h" #include "util/memory.h" -namespace RubberBand { -class RubberBandStretcher; -} // namespace RubberBand - class ReadAheadManager; // Uses librubberband to scale audio. This class is not thread safe. @@ -47,6 +45,10 @@ class EngineBufferScaleRubberBand : public EngineBufferScale { void onSampleRateChanged() override; int runningEngineVersion(); + /// Reset the rubberband instance and run the prerequisite amount of padding + /// through it. This should be used instead of calling + /// `m_pRubberBand->reset()` directly. + void reset(); void deinterleaveAndProcess(const CSAMPLE* pBuffer, SINT frames, bool flush); SINT retrieveAndDeinterleave(CSAMPLE* pBuffer, SINT frames); From 5d4f5c6f040ba7b65f5e935be2fa248688fb66b8 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Wed, 7 Dec 2022 23:44:48 +0100 Subject: [PATCH 04/13] Fix Rubberband v2 compatibility in padding --- .../bufferscalers/enginebufferscalerubberband.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/engine/bufferscalers/enginebufferscalerubberband.cpp b/src/engine/bufferscalers/enginebufferscalerubberband.cpp index f57e25dbe88..ad945640737 100644 --- a/src/engine/bufferscalers/enginebufferscalerubberband.cpp +++ b/src/engine/bufferscalers/enginebufferscalerubberband.cpp @@ -290,7 +290,13 @@ void EngineBufferScaleRubberBand::reset() { // need to run some silent samples through the time stretching engine first // before using it. Otherwise it will eat add a short fade-in, destroying // the initial transient. +#if RUBBERBANDV3 size_t remaining_padding = m_pRubberBand->getPreferredStartPad(); +#else + // This _should_ be equal to the latency in older Rubber Band versions: + // https://github.com/breakfastquay/rubberband/blob/c5f99d5ff2cba2f4f1def6c38c7843bbb9ac7a78/main/main.cpp#L652 + size_t remaining_padding = m_pRubberBand->getLatency(); +#endif std::fill_n(m_buffers[0].begin(), kRubberBandBlockSize, 0.0f); std::fill_n(m_buffers[1].begin(), kRubberBandBlockSize, 0.0f); while (remaining_padding > 0) { @@ -300,7 +306,11 @@ void EngineBufferScaleRubberBand::reset() { remaining_padding -= pad_samples; } +#if RUBBERBANDV3 size_t padding_to_drop = m_pRubberBand->getStartDelay(); +#else + size_t padding_to_drop = m_pRubberBand->getLatency(); +#endif while (padding_to_drop > 0) { const size_t drop_samples = std::min(padding_to_drop, kRubberBandBlockSize); m_pRubberBand->retrieve(m_bufferPtrs.data(), drop_samples); From 3fec0ea63226f5eae28f32677b0c5550e80c84de Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Wed, 7 Dec 2022 23:46:39 +0100 Subject: [PATCH 05/13] Mark EngineBufferScaleRubberBand final To avoid looking up virtual function calls in case this class is used as a superclass for something else. --- src/engine/bufferscalers/enginebufferscalerubberband.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/bufferscalers/enginebufferscalerubberband.h b/src/engine/bufferscalers/enginebufferscalerubberband.h index d1c4a651b94..ff642246193 100644 --- a/src/engine/bufferscalers/enginebufferscalerubberband.h +++ b/src/engine/bufferscalers/enginebufferscalerubberband.h @@ -11,7 +11,7 @@ class ReadAheadManager; // Uses librubberband to scale audio. This class is not thread safe. -class EngineBufferScaleRubberBand : public EngineBufferScale { +class EngineBufferScaleRubberBand final : public EngineBufferScale { Q_OBJECT public: explicit EngineBufferScaleRubberBand( From 1769fea5c862884d5f5fd5b2b25a916cb9e6cc01 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Wed, 7 Dec 2022 23:51:09 +0100 Subject: [PATCH 06/13] Replace uses of std::vector with SampleBuffer As suggested in https://github.com/mixxxdj/mixxx/pull/11120/files/e1db59ae2b4239a4beecdeb8f25020021e71d893#diff-1df0d6d18e318bf8be5dabd0990733f2130277c19452f13d50a05b6fd0527c34. --- src/engine/bufferscalers/enginebufferscalerubberband.cpp | 6 +++--- src/engine/bufferscalers/enginebufferscalerubberband.h | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/engine/bufferscalers/enginebufferscalerubberband.cpp b/src/engine/bufferscalers/enginebufferscalerubberband.cpp index ad945640737..00dbd3757eb 100644 --- a/src/engine/bufferscalers/enginebufferscalerubberband.cpp +++ b/src/engine/bufferscalers/enginebufferscalerubberband.cpp @@ -26,7 +26,7 @@ constexpr size_t kRubberBandBlockSize = 256; EngineBufferScaleRubberBand::EngineBufferScaleRubberBand( ReadAheadManager* pReadAheadManager) : m_pReadAheadManager(pReadAheadManager), - m_buffers{std::vector(MAX_BUFFER_LEN), std::vector(MAX_BUFFER_LEN)}, + m_buffers{mixxx::SampleBuffer(MAX_BUFFER_LEN), mixxx::SampleBuffer(MAX_BUFFER_LEN)}, m_bufferPtrs{m_buffers[0].data(), m_buffers[1].data()}, m_interleavedReadBuffer(MAX_BUFFER_LEN), m_bBackwards(false), @@ -297,8 +297,8 @@ void EngineBufferScaleRubberBand::reset() { // https://github.com/breakfastquay/rubberband/blob/c5f99d5ff2cba2f4f1def6c38c7843bbb9ac7a78/main/main.cpp#L652 size_t remaining_padding = m_pRubberBand->getLatency(); #endif - std::fill_n(m_buffers[0].begin(), kRubberBandBlockSize, 0.0f); - std::fill_n(m_buffers[1].begin(), kRubberBandBlockSize, 0.0f); + std::fill_n(m_buffers[0].span().begin(), kRubberBandBlockSize, 0.0f); + std::fill_n(m_buffers[1].span().begin(), kRubberBandBlockSize, 0.0f); while (remaining_padding > 0) { const size_t pad_samples = std::min(remaining_padding, kRubberBandBlockSize); m_pRubberBand->process(m_bufferPtrs.data(), pad_samples, false); diff --git a/src/engine/bufferscalers/enginebufferscalerubberband.h b/src/engine/bufferscalers/enginebufferscalerubberband.h index ff642246193..f4c7b542dfe 100644 --- a/src/engine/bufferscalers/enginebufferscalerubberband.h +++ b/src/engine/bufferscalers/enginebufferscalerubberband.h @@ -3,10 +3,10 @@ #include #include -#include #include "engine/bufferscalers/enginebufferscale.h" #include "util/memory.h" +#include "util/samplebuffer.h" class ReadAheadManager; @@ -61,14 +61,14 @@ class EngineBufferScaleRubberBand final : public EngineBufferScale { /// The audio buffers samples used to send audio to Rubber Band and to /// receive processed audio from Rubber Band. This is needed because Mixxx /// uses interleaved buffers in most other places. - std::array, 2> m_buffers; + std::array m_buffers; /// These point to the buffers in `m_buffers`. They can be defined here /// since this object cannot be moved or copied. std::array m_bufferPtrs; /// Contains interleaved samples read from `m_pReadAheadManager`. These need /// to be deinterleaved before they can be passed to Rubber Band. - std::vector m_interleavedReadBuffer; + mixxx::SampleBuffer m_interleavedReadBuffer; // Holds the playback direction bool m_bBackwards; From 4ebe88f1cd8d59fb8ae0f2470a713d26722ac2e4 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Thu, 15 Dec 2022 18:55:33 +0100 Subject: [PATCH 07/13] Fix start pad calculation for librubberband <3.x --- .../enginebufferscalerubberband.cpp | 42 +++++++++++++------ .../enginebufferscalerubberband.h | 6 +++ 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/src/engine/bufferscalers/enginebufferscalerubberband.cpp b/src/engine/bufferscalers/enginebufferscalerubberband.cpp index 00dbd3757eb..04fdf893679 100644 --- a/src/engine/bufferscalers/enginebufferscalerubberband.cpp +++ b/src/engine/bufferscalers/enginebufferscalerubberband.cpp @@ -274,6 +274,34 @@ void EngineBufferScaleRubberBand::useEngineFiner(bool enable) { } } +// See +// https://github.com/breakfastquay/rubberband/commit/72654b04ea4f0707e214377515119e933efbdd6c +// for how these two functions were implemented within librubberband itself +size_t EngineBufferScaleRubberBand::getPreferredStartPad() const { +#if RUBBERBANDV3 + return m_pRubberBand->getPreferredStartPad(); +#else + // `getPreferredStartPad()` returns `window_size / 2`, while with + // `getLatency()` both time stretching engines return `window_size / 2 / + // pitch_scale` + return static_cast(std::ceil( + m_pRubberBand->getLatency() * m_pRubberBand->getPitchScale())); +#endif +} + +size_t EngineBufferScaleRubberBand::getStartDelay() const { +#if RUBBERBANDV3 + return m_pRubberBand->getStartDelay(); +#else + // In newer Rubber Band versions `getLatency()` is a deprecated alias for + // `getStartDelay()`, so they should behave the same. In the commit linked + // above the behavior was different for the R3 stretcher, but that was only + // during the initial betas of Rubberband 3.0 so we shouldn't have to worry + // about that. + return m_pRubberBand->getLatency(); +#endif +} + int EngineBufferScaleRubberBand::runningEngineVersion() { #if RUBBERBANDV3 return m_pRubberBand->getEngineVersion(); @@ -290,13 +318,7 @@ void EngineBufferScaleRubberBand::reset() { // need to run some silent samples through the time stretching engine first // before using it. Otherwise it will eat add a short fade-in, destroying // the initial transient. -#if RUBBERBANDV3 - size_t remaining_padding = m_pRubberBand->getPreferredStartPad(); -#else - // This _should_ be equal to the latency in older Rubber Band versions: - // https://github.com/breakfastquay/rubberband/blob/c5f99d5ff2cba2f4f1def6c38c7843bbb9ac7a78/main/main.cpp#L652 - size_t remaining_padding = m_pRubberBand->getLatency(); -#endif + size_t remaining_padding = getPreferredStartPad(); std::fill_n(m_buffers[0].span().begin(), kRubberBandBlockSize, 0.0f); std::fill_n(m_buffers[1].span().begin(), kRubberBandBlockSize, 0.0f); while (remaining_padding > 0) { @@ -306,11 +328,7 @@ void EngineBufferScaleRubberBand::reset() { remaining_padding -= pad_samples; } -#if RUBBERBANDV3 - size_t padding_to_drop = m_pRubberBand->getStartDelay(); -#else - size_t padding_to_drop = m_pRubberBand->getLatency(); -#endif + size_t padding_to_drop = getStartDelay(); while (padding_to_drop > 0) { const size_t drop_samples = std::min(padding_to_drop, kRubberBandBlockSize); m_pRubberBand->retrieve(m_bufferPtrs.data(), drop_samples); diff --git a/src/engine/bufferscalers/enginebufferscalerubberband.h b/src/engine/bufferscalers/enginebufferscalerubberband.h index f4c7b542dfe..1382e7ca49b 100644 --- a/src/engine/bufferscalers/enginebufferscalerubberband.h +++ b/src/engine/bufferscalers/enginebufferscalerubberband.h @@ -44,6 +44,12 @@ class EngineBufferScaleRubberBand final : public EngineBufferScale { // Reset RubberBand library with new audio signal void onSampleRateChanged() override; + /// Calls `m_pRubberBand->getPreferredStartPad()`, with backwards + /// compatibility for older librubberband versions. + size_t getPreferredStartPad() const; + /// Calls `m_pRubberBand->getStartDelay()`, with backwards compatibility for + /// older librubberband versions. + size_t getStartDelay() const; int runningEngineVersion(); /// Reset the rubberband instance and run the prerequisite amount of padding /// through it. This should be used instead of calling From 78f5252961958c1f4997472acae8997747188838 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Thu, 15 Dec 2022 20:15:32 +0100 Subject: [PATCH 08/13] Fix dropping silence padding in Rubber Band This of course didn't work correctly before. We should drop the first n samples from the output, but we can't do that before processing more input. This also fixes the uncompensated latency/clicks issue from #11125 in the process. --- .../enginebufferscalerubberband.cpp | 37 +++++++++++++------ .../enginebufferscalerubberband.h | 4 ++ 2 files changed, 29 insertions(+), 12 deletions(-) diff --git a/src/engine/bufferscalers/enginebufferscalerubberband.cpp b/src/engine/bufferscalers/enginebufferscalerubberband.cpp index 04fdf893679..690e9762b56 100644 --- a/src/engine/bufferscalers/enginebufferscalerubberband.cpp +++ b/src/engine/bufferscalers/enginebufferscalerubberband.cpp @@ -140,17 +140,29 @@ void EngineBufferScaleRubberBand::clear() { SINT EngineBufferScaleRubberBand::retrieveAndDeinterleave( CSAMPLE* pBuffer, SINT frames) { - SINT frames_available = m_pRubberBand->available(); - SINT frames_to_read = math_min(frames_available, frames); + const SINT frames_available = m_pRubberBand->available(); + const SINT frames_to_read = math_min(frames_available, frames); SINT received_frames = static_cast(m_pRubberBand->retrieve( m_bufferPtrs.data(), frames_to_read)); + SINT frame_offset = 0; - DEBUG_ASSERT(received_frames <= static_cast(m_buffers[0].size())); + // As explained below in `reset()`, the first time this is called we need to + // drop the silence we fed into the time stretcher as padding from the + // output + if (m_remainingPaddingInOutput > 0) { + const SINT drop_num_frames = std::min(received_frames, m_remainingPaddingInOutput); + + m_remainingPaddingInOutput -= drop_num_frames; + received_frames -= drop_num_frames; + frame_offset += drop_num_frames; + } + DEBUG_ASSERT(received_frames <= static_cast(m_buffers[0].size())); SampleUtil::interleaveBuffer(pBuffer, - m_buffers[0].data(), - m_buffers[1].data(), + m_buffers[0].data() + frame_offset, + m_buffers[1].data() + frame_offset, received_frames); + return received_frames; } @@ -187,6 +199,9 @@ double EngineBufferScaleRubberBand::scaleBuffer( // enough calls to retrieveAndDeinterleave because CachingReader returns // zeros for reads that are not in cache. So it's safe to loop here // without any checks for failure in retrieveAndDeinterleave. + // If the time stretcher has just been reset then this will throw away + // the first `m_remainingPaddingInOutput` samples of silence padding + // from the output. SINT received_frames = retrieveAndDeinterleave( read, remaining_frames); remaining_frames -= received_frames; @@ -328,11 +343,9 @@ void EngineBufferScaleRubberBand::reset() { remaining_padding -= pad_samples; } - size_t padding_to_drop = getStartDelay(); - while (padding_to_drop > 0) { - const size_t drop_samples = std::min(padding_to_drop, kRubberBandBlockSize); - m_pRubberBand->retrieve(m_bufferPtrs.data(), drop_samples); - - padding_to_drop -= drop_samples; - } + // The silence we just added covers half a window (see the last paragraph of + // https://github.com/mixxxdj/mixxx/pull/11120#discussion_r1050011104). This + // silence should be dropped from the result when the `retrieve()` in + // `retrieveAndDeinterleave()` first starts producing audio. + m_remainingPaddingInOutput = static_cast(getStartDelay()); } diff --git a/src/engine/bufferscalers/enginebufferscalerubberband.h b/src/engine/bufferscalers/enginebufferscalerubberband.h index 1382e7ca49b..48374109370 100644 --- a/src/engine/bufferscalers/enginebufferscalerubberband.h +++ b/src/engine/bufferscalers/enginebufferscalerubberband.h @@ -78,6 +78,10 @@ class EngineBufferScaleRubberBand final : public EngineBufferScale { // Holds the playback direction bool m_bBackwards; + /// The amount of silence padding that still needs to be dropped from the + /// retrieve samples in `retrieveAndDeinterleave()`. See the `reset()` + /// function for an explanation. + SINT m_remainingPaddingInOutput = 0; bool m_useEngineFiner; }; From 755a4a485c81376228a9f4a09d35759f2e9a5626 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Sat, 17 Dec 2022 00:06:35 +0100 Subject: [PATCH 09/13] Consider the offset in receive buffer size assert --- src/engine/bufferscalers/enginebufferscalerubberband.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/bufferscalers/enginebufferscalerubberband.cpp b/src/engine/bufferscalers/enginebufferscalerubberband.cpp index 690e9762b56..b21ccd30f12 100644 --- a/src/engine/bufferscalers/enginebufferscalerubberband.cpp +++ b/src/engine/bufferscalers/enginebufferscalerubberband.cpp @@ -157,7 +157,7 @@ SINT EngineBufferScaleRubberBand::retrieveAndDeinterleave( frame_offset += drop_num_frames; } - DEBUG_ASSERT(received_frames <= static_cast(m_buffers[0].size())); + DEBUG_ASSERT((received_frames + frame_offset) <= static_cast(m_buffers[0].size())); SampleUtil::interleaveBuffer(pBuffer, m_buffers[0].data() + frame_offset, m_buffers[1].data() + frame_offset, From 9b8e82a730812824c2876e468039dc254c0bd1e9 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Sat, 17 Dec 2022 00:17:51 +0100 Subject: [PATCH 10/13] Get rid of block size limit in Rubberband This mostly reverts adb034b3dff2a3b4cbae4a9cec68353d1f9c4fe6 as this bug should in theory only affect librubberband 1.3 and the oldest supported version is currently 1.8.2. --- .../enginebufferscalerubberband.cpp | 50 +++++-------------- 1 file changed, 12 insertions(+), 38 deletions(-) diff --git a/src/engine/bufferscalers/enginebufferscalerubberband.cpp b/src/engine/bufferscalers/enginebufferscalerubberband.cpp index b21ccd30f12..3372648361e 100644 --- a/src/engine/bufferscalers/enginebufferscalerubberband.cpp +++ b/src/engine/bufferscalers/enginebufferscalerubberband.cpp @@ -12,17 +12,8 @@ using RubberBand::RubberBandStretcher; -namespace { - -// TODO (XXX): this should be removed. It is only needed to work around -// a Rubberband 1.3 bug. -// This is the default increment from RubberBand 1.8.1. -constexpr size_t kRubberBandBlockSize = 256; - #define RUBBERBANDV3 (RUBBERBAND_API_MAJOR_VERSION >= 2 && RUBBERBAND_API_MINOR_VERSION >= 7) -} // namespace - EngineBufferScaleRubberBand::EngineBufferScaleRubberBand( ReadAheadManager* pReadAheadManager) : m_pReadAheadManager(pReadAheadManager), @@ -120,9 +111,6 @@ void EngineBufferScaleRubberBand::onSampleRateChanged() { getOutputSignal().getSampleRate(), getOutputSignal().getChannelCount(), rubberbandOptions); - // TODO (XXX): we should always be able to provide rubberband as - // many samples as it wants. So remove this. - m_pRubberBand->setMaxProcessSize(kRubberBandBlockSize); // Setting the time ratio to a very high value will cause RubberBand // to preallocate buffers large enough to (almost certainly) // avoid memory reallocations during playback. @@ -216,35 +204,20 @@ double EngineBufferScaleRubberBand::scaleBuffer( break; } - SINT iLenFramesRequired = static_cast(m_pRubberBand->getSamplesRequired()); - if (iLenFramesRequired == 0) { - // TODO (XXX): Rubberband 1.3 is not being packaged anymore. - // Remove this workaround. - // - // rubberband 1.3 (packaged up through Ubuntu Quantal) has a bug - // where it can report 0 samples needed forever which leads us to an - // infinite loop. To work around this, we check if available() is - // zero. If it is, then we submit a fixed block size of - // kRubberBandBlockSize. - int available = m_pRubberBand->available(); - if (available == 0) { - iLenFramesRequired = kRubberBandBlockSize; - } - } - // qDebug() << "iLenFramesRequired" << iLenFramesRequired; - - if (remaining_frames > 0 && iLenFramesRequired > 0) { - SINT iAvailSamples = m_pReadAheadManager->getNextSamples( + const SINT next_block_frames_required = + static_cast(m_pRubberBand->getSamplesRequired()); + if (remaining_frames > 0 && next_block_frames_required > 0) { + const SINT available_samples = m_pReadAheadManager->getNextSamples( // The value doesn't matter here. All that matters is we // are going forward or backward. (m_bBackwards ? -1.0 : 1.0) * m_dBaseRate * m_dTempoRatio, m_interleavedReadBuffer.data(), - getOutputSignal().frames2samples(iLenFramesRequired)); - SINT iAvailFrames = getOutputSignal().samples2frames(iAvailSamples); + getOutputSignal().frames2samples(next_block_frames_required)); + const SINT available_frames = getOutputSignal().samples2frames(available_samples); - if (iAvailFrames > 0) { + if (available_frames > 0) { last_read_failed = false; - deinterleaveAndProcess(m_interleavedReadBuffer.data(), iAvailFrames, false); + deinterleaveAndProcess(m_interleavedReadBuffer.data(), available_frames, false); } else { if (last_read_failed) { // Flush and break out after the next retrieval. If we are @@ -334,10 +307,11 @@ void EngineBufferScaleRubberBand::reset() { // before using it. Otherwise it will eat add a short fade-in, destroying // the initial transient. size_t remaining_padding = getPreferredStartPad(); - std::fill_n(m_buffers[0].span().begin(), kRubberBandBlockSize, 0.0f); - std::fill_n(m_buffers[1].span().begin(), kRubberBandBlockSize, 0.0f); + const size_t block_size = std::min(remaining_padding, m_buffers[0].size()); + std::fill_n(m_buffers[0].span().begin(), block_size, 0.0f); + std::fill_n(m_buffers[1].span().begin(), block_size, 0.0f); while (remaining_padding > 0) { - const size_t pad_samples = std::min(remaining_padding, kRubberBandBlockSize); + const size_t pad_samples = std::min(remaining_padding, block_size); m_pRubberBand->process(m_bufferPtrs.data(), pad_samples, false); remaining_padding -= pad_samples; From b73bdc0dbd316be60ab285d783b071ff34a0fa3e Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Sat, 17 Dec 2022 00:23:01 +0100 Subject: [PATCH 11/13] Link to comment explaining why the padding works --- src/engine/bufferscalers/enginebufferscalerubberband.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/engine/bufferscalers/enginebufferscalerubberband.cpp b/src/engine/bufferscalers/enginebufferscalerubberband.cpp index 3372648361e..ef60c4420dc 100644 --- a/src/engine/bufferscalers/enginebufferscalerubberband.cpp +++ b/src/engine/bufferscalers/enginebufferscalerubberband.cpp @@ -306,6 +306,9 @@ void EngineBufferScaleRubberBand::reset() { // need to run some silent samples through the time stretching engine first // before using it. Otherwise it will eat add a short fade-in, destroying // the initial transient. + // + // See https://github.com/mixxxdj/mixxx/pull/11120#discussion_r1050011104 + // for more information. size_t remaining_padding = getPreferredStartPad(); const size_t block_size = std::min(remaining_padding, m_buffers[0].size()); std::fill_n(m_buffers[0].span().begin(), block_size, 0.0f); From c108ac96a7cd64746a8ac5169530d7da9ddf9ab7 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Sun, 18 Dec 2022 03:08:39 +0100 Subject: [PATCH 12/13] Fix debug assertions in retrieveAndDeinterleave This was comparing against the wrong thing in 755a4a485c81376228a9f4a09d35759f2e9a5626. --- src/engine/bufferscalers/enginebufferscalerubberband.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/engine/bufferscalers/enginebufferscalerubberband.cpp b/src/engine/bufferscalers/enginebufferscalerubberband.cpp index ef60c4420dc..1f7ccdbf7b5 100644 --- a/src/engine/bufferscalers/enginebufferscalerubberband.cpp +++ b/src/engine/bufferscalers/enginebufferscalerubberband.cpp @@ -130,6 +130,7 @@ SINT EngineBufferScaleRubberBand::retrieveAndDeinterleave( SINT frames) { const SINT frames_available = m_pRubberBand->available(); const SINT frames_to_read = math_min(frames_available, frames); + DEBUG_ASSERT(frames_to_read <= m_buffers[0].size()); SINT received_frames = static_cast(m_pRubberBand->retrieve( m_bufferPtrs.data(), frames_to_read)); SINT frame_offset = 0; @@ -145,7 +146,7 @@ SINT EngineBufferScaleRubberBand::retrieveAndDeinterleave( frame_offset += drop_num_frames; } - DEBUG_ASSERT((received_frames + frame_offset) <= static_cast(m_buffers[0].size())); + DEBUG_ASSERT(received_frames <= frames); SampleUtil::interleaveBuffer(pBuffer, m_buffers[0].data() + frame_offset, m_buffers[1].data() + frame_offset, From d3aa2104c083ba6d0a540d31f8b03560fafa70f2 Mon Sep 17 00:00:00 2001 From: Robbert van der Helm Date: Wed, 21 Dec 2022 02:10:58 +0100 Subject: [PATCH 13/13] Immediately read all padding with Rubberband Instead of reading his in small blocks and then throwing away all output a couple times until we threw away all of the padding. --- src/engine/bufferscalers/enginebufferscalerubberband.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/engine/bufferscalers/enginebufferscalerubberband.cpp b/src/engine/bufferscalers/enginebufferscalerubberband.cpp index 1f7ccdbf7b5..367aaea871f 100644 --- a/src/engine/bufferscalers/enginebufferscalerubberband.cpp +++ b/src/engine/bufferscalers/enginebufferscalerubberband.cpp @@ -129,7 +129,10 @@ SINT EngineBufferScaleRubberBand::retrieveAndDeinterleave( CSAMPLE* pBuffer, SINT frames) { const SINT frames_available = m_pRubberBand->available(); - const SINT frames_to_read = math_min(frames_available, frames); + // NOTE: If we still need to throw away padding, then we can also + // immediately read those frames in addition to the frames we actually + // need for the output + const SINT frames_to_read = math_min(frames_available, frames + m_remainingPaddingInOutput); DEBUG_ASSERT(frames_to_read <= m_buffers[0].size()); SINT received_frames = static_cast(m_pRubberBand->retrieve( m_bufferPtrs.data(), frames_to_read));