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

Add missing Rubber Band padding, preventing it from eating the initial transient #11120

Merged
merged 13 commits into from
Dec 21, 2022
Merged

Add missing Rubber Band padding, preventing it from eating the initial transient #11120

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
dabf1d4
Mark kRubberBandBlockSize constexpr
robbert-vdh Dec 6, 2022
d4ad20d
Replace manual memory management in rubberband
robbert-vdh Dec 7, 2022
e1db59a
Run required padding through Rubber Band on reset
robbert-vdh Dec 7, 2022
5d4f5c6
Fix Rubberband v2 compatibility in padding
robbert-vdh Dec 7, 2022
3fec0ea
Mark EngineBufferScaleRubberBand final
robbert-vdh Dec 7, 2022
1769fea
Replace uses of std::vector with SampleBuffer
robbert-vdh Dec 7, 2022
4ebe88f
Fix start pad calculation for librubberband <3.x
robbert-vdh Dec 15, 2022
78f5252
Fix dropping silence padding in Rubber Band
robbert-vdh Dec 15, 2022
755a4a4
Consider the offset in receive buffer size assert
robbert-vdh Dec 16, 2022
9b8e82a
Get rid of block size limit in Rubberband
robbert-vdh Dec 16, 2022
b73bdc0
Link to comment explaining why the padding works
robbert-vdh Dec 16, 2022
c108ac9
Fix debug assertions in retrieveAndDeinterleave
robbert-vdh Dec 18, 2022
d3aa210
Immediately read all padding with Rubberband
robbert-vdh Dec 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 25 additions & 12 deletions src/engine/bufferscalers/enginebufferscalerubberband.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<SINT>(m_pRubberBand->retrieve(
m_bufferPtrs.data(), frames_to_read));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to read here frames_to_read + frame_offset;
Even though the buffer is more than big enough, it would be correct to check for its size.
We may consider to use a way smaller buffer than MAX_BUFFER_LEN b.t.w.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, did you miss to push a commit?

The function expects frames and we need to discard frame_offset from the retrieved buffer. So we need to retrieve frames + frame_offset.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See 755a4a4.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linked code makes that the function returns less frames than it might could have read. So we should not clamp the value to frames , but to frames + frame_offset in line 132

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No we should not. I can't check what m_pRubberBand->available() returns here, but logically it would return 1024 if it's doing overlap-add with four times overlap. At a 1.0 pitch ratio/when the time stretcher should not be making any adjustments, we need to throw away half a window's worth of samples, which is 2048 in this case. So in that situation the first two calls to this function must read and return 0 samples or else we've been reading the processed padding that we're trying to throw away here.

The linked code makes that the function returns less frames than it might could have read.

It can only read a small window of output at a time. Presumably this is 1/4th of the FFT window size. So 1024 samples. Which means that the first two calls to this function (each producing 1024 samples of garbage output) should be thrown away.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a bit debug code to shows the issue after a seek:
qDebug() << "padding = " << m_remainingPaddingInOutput << "frames = " << frames << "available = " << frames_available;

debug [Engine] padding  =  1025 frames  =  256 available  =  0
debug [Engine] padding  =  1025 frames  =  256 available  =  512
debug [Engine] padding  =  769 frames  =  256 available  =  768                 
debug [Engine] padding  =  513 frames  =  256 available  =  1024
debug [Engine] padding  =  257 frames  =  256 available  =  1280
debug [Engine] padding  =  1 frames  =  256 available  =  1536
debug [Engine] padding  =  0 frames  =  1 available  =  1792
debug [Engine] padding  =  0 frames  =  256 available  =  1791
debug [Engine] padding  =  0 frames  =  256 available  =  1535
debug [Engine] padding  =  0 frames  =  256 available  =  1279

In the second call, we read only 256 frames from the available 512. The available frames are stacked up before they are reduced again. We need to read all available frames in that case. This needs to be fixed.

Copy link
Contributor Author

@robbert-vdh robbert-vdh Dec 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah I see what you mean now! Fixed in d3aa210.

Somewhat related, would there be any interest in exposing the Rubberband R3 + short window option? I added it for myself here: robbert-vdh@af570f4 Its performance is in line with R2 (and also no huge spikes when searching), and quality wise it's more or less a sidegrade to R2. It doesn't have R2's time domain transient preservation, but it also doesn't introduce spurious transients and it does still sound better than R2 without transient preservation. Hmm maybe not. It usually does better than R2, but it sometimes also transforms kickdrums into smeary messes.

SINT frame_offset = 0;

DEBUG_ASSERT(received_frames <= static_cast<ssize_t>(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<ssize_t>(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;
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<size_t>(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<SINT>(getStartDelay());
}
4 changes: 4 additions & 0 deletions src/engine/bufferscalers/enginebufferscalerubberband.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};