feat: add SignalSmith keylock engine #15902
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,281 @@ | ||||||||
| #include "engine/bufferscalers/enginebufferscalesignalsmith.h" | ||||||||
|
|
||||||||
| #include "engine/engine.h" | ||||||||
| #include "engine/readaheadmanager.h" | ||||||||
| #include "moc_enginebufferscalesignalsmith.cpp" | ||||||||
| #include "util/assert.h" | ||||||||
| #include "util/defs.h" | ||||||||
| #include "util/sample.h" | ||||||||
| #include "util/timer.h" | ||||||||
|
|
||||||||
| EngineBufferScaleSignalSmith::EngineBufferScaleSignalSmith(ReadAheadManager* pReadAheadManager) | ||||||||
| : m_pReadAheadManager(pReadAheadManager), | ||||||||
| m_buffers(), | ||||||||
| m_bufferPtrs(), | ||||||||
| m_interleavedBuffer(mixxx::kMaxSupportedStems * MAX_BUFFER_LEN), | ||||||||
| m_frameFractionalLeftover(0), | ||||||||
| m_expectedFrameLatency(0), | ||||||||
| m_currentFrameOffset(0), | ||||||||
| m_currentPreset(Preset::Default) { | ||||||||
| onSignalChanged(); | ||||||||
| } | ||||||||
|
|
||||||||
| void EngineBufferScaleSignalSmith::setScaleParameters( | ||||||||
| double base_rate, double* pTempoRatio, double* pPitchRatio) { | ||||||||
| m_dBaseRate = base_rate; | ||||||||
| m_bBackwards = *pTempoRatio < 0; | ||||||||
| m_dTempoRatio = std::fabs(*pTempoRatio); | ||||||||
| m_dPitchRatio = *pPitchRatio; | ||||||||
| m_effectiveRate = m_dBaseRate * m_dTempoRatio; | ||||||||
|
|
||||||||
| m_stretch.setTransposeFactor(static_cast<float>(m_dBaseRate * m_dPitchRatio)); | ||||||||
| m_stretch.setFormantFactor(1.0); | ||||||||
|
|
||||||||
| // The following value is calculated from the block and interval samples | ||||||||
| // size, which are set in the above preset. It remains constant during the | ||||||||
|
daschuer marked this conversation as resolved.
|
||||||||
| // stretcher process. | ||||||||
| // As documented in | ||||||||
| // https://signalsmith-audio.co.uk/code/stretch/#how-to-use-latency-starting-and-ending, | ||||||||
| // stretch factor should be used when computing total latency | ||||||||
| m_expectedFrameLatency = | ||||||||
| static_cast<SINT>(m_stretch.inputLatency()) + | ||||||||
| static_cast<SINT>(std::round(m_effectiveRate * | ||||||||
| static_cast<double>(m_stretch.outputLatency()))); | ||||||||
| } | ||||||||
|
|
||||||||
| void EngineBufferScaleSignalSmith::onSignalChanged() { | ||||||||
| if (!getOutputSignal().isValid()) { | ||||||||
| return; | ||||||||
| } | ||||||||
|
|
||||||||
| uint8_t channelCount = getOutputSignal().getChannelCount(); | ||||||||
| if (m_buffers.size() != channelCount) { | ||||||||
| m_buffers.resize(channelCount); | ||||||||
| } | ||||||||
|
|
||||||||
| if (m_bufferPtrs.size() != channelCount) { | ||||||||
| m_bufferPtrs.resize(channelCount); | ||||||||
| } | ||||||||
|
|
||||||||
| for (int chIdx = 0; chIdx < channelCount; chIdx++) { | ||||||||
| if (m_buffers[chIdx].size() == MAX_BUFFER_LEN) { | ||||||||
| continue; | ||||||||
| } | ||||||||
| m_buffers[chIdx] = mixxx::SampleBuffer(MAX_BUFFER_LEN); | ||||||||
| m_bufferPtrs[chIdx] = m_buffers[chIdx].data(); | ||||||||
| } | ||||||||
|
|
||||||||
| // Configure stretcher with preset settings | ||||||||
| switch (m_currentPreset) { | ||||||||
| case Preset::Cheaper: | ||||||||
| m_stretch.presetCheaper(channelCount, getOutputSignal().getSampleRate()); | ||||||||
| break; | ||||||||
| default: | ||||||||
| qWarning() << "Unsupported presset" << m_currentPreset << " so defaulting to default."; | ||||||||
| [[fallthrough]]; | ||||||||
| case Preset::Default: | ||||||||
| m_stretch.presetDefault(channelCount, getOutputSignal().getSampleRate()); | ||||||||
| break; | ||||||||
| } | ||||||||
| clear(); | ||||||||
| } | ||||||||
|
|
||||||||
| void EngineBufferScaleSignalSmith::clear() { | ||||||||
| m_stretch.reset(); | ||||||||
| m_currentFrameOffset = 0; | ||||||||
| m_frameFractionalLeftover = 0; | ||||||||
| } | ||||||||
|
|
||||||||
| SINT EngineBufferScaleSignalSmith::fetchAndDeinterleave(SINT sampleToRead, SINT frameOffset) { | ||||||||
| auto frameRead = getOutputSignal().samples2frames( | ||||||||
| m_pReadAheadManager->getNextSamples( | ||||||||
| // The value doesn't matter here. All that matters is we | ||||||||
| // are going forward or backward. | ||||||||
| (m_bBackwards ? -1 : 1) * m_dBaseRate * m_dTempoRatio, | ||||||||
| m_interleavedBuffer.data(), | ||||||||
| sampleToRead, | ||||||||
| getOutputSignal().getChannelCount())); | ||||||||
|
|
||||||||
| switch (getOutputSignal().getChannelCount()) { | ||||||||
| case mixxx::audio::ChannelCount::stereo(): | ||||||||
| SampleUtil::deinterleaveBuffer( | ||||||||
| m_buffers[0].data(frameOffset), | ||||||||
| m_buffers[1].data(frameOffset), | ||||||||
| m_interleavedBuffer.data(), | ||||||||
| frameRead); | ||||||||
| break; | ||||||||
| case mixxx::audio::ChannelCount::stem(): | ||||||||
| SampleUtil::deinterleaveBuffer( | ||||||||
| m_buffers[0].data(frameOffset), | ||||||||
| m_buffers[1].data(frameOffset), | ||||||||
| m_buffers[2].data(frameOffset), | ||||||||
| m_buffers[3].data(frameOffset), | ||||||||
| m_buffers[4].data(frameOffset), | ||||||||
| m_buffers[5].data(frameOffset), | ||||||||
| m_buffers[6].data(frameOffset), | ||||||||
| m_buffers[7].data(frameOffset), | ||||||||
| m_interleavedBuffer.data(), | ||||||||
| frameRead); | ||||||||
| break; | ||||||||
| default: { | ||||||||
| int chCount = getOutputSignal().getChannelCount(); | ||||||||
| // The sampler are ordered as following in pBuffer | ||||||||
| // 1234..X1234...X... | ||||||||
| // And need to be reordered as following | ||||||||
| // m_buffers#1 = 11.. | ||||||||
| // m_buffers#2 = 22.. | ||||||||
| // m_buffers#3 = 33.. | ||||||||
| // m_buffers#4 = 44..fff | ||||||||
| // m_buffers#X = XX.. | ||||||||
| // | ||||||||
| // Because of the unanticipated number of buffer and channel, we cannot | ||||||||
| // use any SampleUtil in this case | ||||||||
| for (SINT frameIdx = 0; frameIdx < frameRead; ++frameIdx) { | ||||||||
| for (int channel = 0; channel < chCount; channel++) { | ||||||||
| m_buffers[channel].data(frameOffset)[frameIdx] = | ||||||||
| m_interleavedBuffer.data()[frameIdx * chCount + channel]; | ||||||||
| } | ||||||||
| } | ||||||||
| } break; | ||||||||
| } | ||||||||
| return frameRead; | ||||||||
| } | ||||||||
|
|
||||||||
| double EngineBufferScaleSignalSmith::scaleBuffer(CSAMPLE* pOutputBuffer, SINT iOutputBufferSize) { | ||||||||
| ScopedTimer t(QStringLiteral("EngineBufferScaleSignalsmith::scaleBuffer")); | ||||||||
|
|
||||||||
| // Unlike RubberBand, SignalSmith Stretch always output as much audio as it | ||||||||
| // was given. However, it does introduce latency (documented at | ||||||||
| // https://signalsmith-audio.co.uk/code/stretch/#how-to-use-latency) which | ||||||||
| // initially lead to a silence. To compensate that, we need to use the | ||||||||
| // `.outputSeek` method, which allows to pre-roll samples a realign the actual | ||||||||
| // output to real time. | ||||||||
| // However, this method will reset the buffer so it can only be used right after a reset | ||||||||
| if (m_currentFrameOffset == 0 && | ||||||||
| m_currentFrameOffset < m_expectedFrameLatency | ||||||||
|
Member
There was a problem hiding this comment. For my expectation we alwasy need this m_currentFrameOffset == m_expectedFrameLatency.
Member
Author
There was a problem hiding this comment. That's correct, but due to limit listed in the comment, we may not be able to correct this in a single process round (e.g because we have now a too large offset compare to the new latency, but we can only correct by iOuputBufferSize).
Comment on lines
+154
to
+155
Member
There was a problem hiding this comment. Can this become:
Suggested change
Member
There was a problem hiding this comment. The m_expectedFrameLatency is always > 0 and therefore is the second condition redundant.
Member
Author
There was a problem hiding this comment. Yes but m_currentFrameOffset is not always zero, as you have assumed |
||||||||
| // If the track has a zero rate, we skip correction as this is | ||||||||
| // usually a sign that the track is not playing. This will likely | ||||||||
| // create undesired silence (as opposite to a "zero BPM" play affect | ||||||||
| // if a track start playing with a zero BPM, but this is an | ||||||||
| // acceptable trade off for now) | ||||||||
| && m_dTempoRatio > 0) { | ||||||||
| const SINT frameRead = | ||||||||
| fetchAndDeinterleave(getOutputSignal().frames2samples( | ||||||||
| std::min(m_expectedFrameLatency - m_currentFrameOffset, | ||||||||
| SINT(MAX_BUFFER_LEN)))); | ||||||||
|
Comment on lines
+164
to
+165
Member
There was a problem hiding this comment.
Suggested change
Member
Author
There was a problem hiding this comment. No, the current code is correct -
Member
Author
There was a problem hiding this comment. No it is not. It is zero only on first process, then it contains the input latency of the previous process. FYI, I have tested all your suggestion and they don't work.
Member
Author
There was a problem hiding this comment. No it is not. It is zero only on first process, then it contains the input latency of the previous process. FYI, I have tested all your suggestion and they don't work. |
||||||||
| m_stretch.outputSeek(m_bufferPtrs.data(), frameRead); | ||||||||
|
acolombier marked this conversation as resolved.
|
||||||||
| m_currentFrameOffset += frameRead; | ||||||||
|
Member
There was a problem hiding this comment.
Suggested change
Member
Author
There was a problem hiding this comment. The current code is correct.
Member
There was a problem hiding this comment. m_currentFrameOffset is 0 here. Therefore the + operation is redundant.
Member
There was a problem hiding this comment. You have probably missed that we are her in the |
||||||||
| } | ||||||||
|
|
||||||||
| const SINT outputFrames = getOutputSignal().samples2frames(iOutputBufferSize); | ||||||||
| auto dFrameRequired = | ||||||||
| (m_dBaseRate * m_dTempoRatio * static_cast<double>(outputFrames)) + | ||||||||
| m_frameFractionalLeftover; | ||||||||
|
daschuer marked this conversation as resolved.
|
||||||||
|
|
||||||||
| if (m_currentFrameOffset != m_expectedFrameLatency && dFrameRequired > 0) { | ||||||||
| // This happens when the rate changes because the rate scales the input | ||||||||
| // latency. We need more or less latency frames to keep the output steady. | ||||||||
| // The rate changed is immediately applied to the audio without any glitch. | ||||||||
| // Pitch changes do not affect latency. | ||||||||
| double frameOffset = std::max(-dFrameRequired, | ||||||||
| static_cast<double>(m_expectedFrameLatency - m_currentFrameOffset)); | ||||||||
| dFrameRequired += frameOffset; | ||||||||
| m_currentFrameOffset += static_cast<SINT>(frameOffset); | ||||||||
| } | ||||||||
|
|
||||||||
| const SINT frameRequired = static_cast<SINT>(dFrameRequired); | ||||||||
| VERIFY_OR_DEBUG_ASSERT(frameRequired <= MAX_BUFFER_LEN) { | ||||||||
| return 0.0; | ||||||||
| } | ||||||||
|
|
||||||||
| m_frameFractionalLeftover = dFrameRequired - static_cast<double>(frameRequired); | ||||||||
| DEBUG_ASSERT(0 <= m_frameFractionalLeftover && m_frameFractionalLeftover < 1); | ||||||||
|
|
||||||||
| bool last_read_failed = false; | ||||||||
| SINT frameRead = 0; | ||||||||
| while (frameRead < frameRequired) { | ||||||||
| auto currentFrameRead = fetchAndDeinterleave(getOutputSignal().frames2samples( | ||||||||
| frameRequired - frameRead), | ||||||||
| frameRead); | ||||||||
| frameRead += currentFrameRead; | ||||||||
|
|
||||||||
| if (last_read_failed && currentFrameRead <= 0) { | ||||||||
| // flush and break out after | ||||||||
| // the next retrieval. If we are at EOF this serves to get | ||||||||
| // the last samples out of the scaler. | ||||||||
| for (int ch = 0; ch < getOutputSignal().getChannelCount(); ch++) { | ||||||||
| SampleUtil::clear(m_buffers[ch].data(frameRead), frameRequired - frameRead); | ||||||||
| } | ||||||||
| frameRead = frameRequired; | ||||||||
| break; | ||||||||
| } else if (frameRead <= 0) { | ||||||||
| last_read_failed = true; | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| DEBUG_ASSERT(frameRead == frameRequired); | ||||||||
|
|
||||||||
| { | ||||||||
| ScopedTimer t(QStringLiteral("Signalsmith::process")); | ||||||||
| float* outputBufferPtr[8] = { | ||||||||
| m_interleavedBuffer.data(), | ||||||||
| m_interleavedBuffer.data(iOutputBufferSize), | ||||||||
| m_interleavedBuffer.data(2 * iOutputBufferSize), | ||||||||
| m_interleavedBuffer.data(3 * iOutputBufferSize), | ||||||||
| m_interleavedBuffer.data(4 * iOutputBufferSize), | ||||||||
| m_interleavedBuffer.data(5 * iOutputBufferSize), | ||||||||
| m_interleavedBuffer.data(6 * iOutputBufferSize), | ||||||||
| m_interleavedBuffer.data(7 * iOutputBufferSize), | ||||||||
| }; | ||||||||
| m_stretch.process(m_bufferPtrs.data(), frameRead, outputBufferPtr, outputFrames); | ||||||||
| } | ||||||||
|
|
||||||||
| auto outputFrameSize = getOutputSignal().samples2frames(iOutputBufferSize); | ||||||||
| switch (getOutputSignal().getChannelCount()) { | ||||||||
| case mixxx::audio::ChannelCount::stereo(): | ||||||||
| SampleUtil::interleaveBuffer(pOutputBuffer, | ||||||||
| m_interleavedBuffer.data(), | ||||||||
| m_interleavedBuffer.data(iOutputBufferSize), | ||||||||
| outputFrameSize); | ||||||||
| break; | ||||||||
| case mixxx::audio::ChannelCount::stem(): | ||||||||
| SampleUtil::interleaveBuffer(pOutputBuffer, | ||||||||
| m_interleavedBuffer.data(), | ||||||||
| m_interleavedBuffer.data(iOutputBufferSize), | ||||||||
| m_interleavedBuffer.data(2 * iOutputBufferSize), | ||||||||
| m_interleavedBuffer.data(3 * iOutputBufferSize), | ||||||||
| m_interleavedBuffer.data(4 * iOutputBufferSize), | ||||||||
| m_interleavedBuffer.data(5 * iOutputBufferSize), | ||||||||
| m_interleavedBuffer.data(6 * iOutputBufferSize), | ||||||||
| m_interleavedBuffer.data(7 * iOutputBufferSize), | ||||||||
| outputFrameSize); | ||||||||
| break; | ||||||||
| default: { | ||||||||
| int chCount = getOutputSignal().getChannelCount(); | ||||||||
| // The buffers samples are ordered as following | ||||||||
| // m_buffers#1 = 11.. | ||||||||
| // m_buffers#2 = 22.. | ||||||||
| // m_buffers#3 = 33.. | ||||||||
| // m_buffers#4 = 44.. | ||||||||
| // m_buffers#X = XX.. | ||||||||
| // And need to be reordered as following in pBuffer | ||||||||
| // 1234..X1234...X... | ||||||||
| // | ||||||||
| // Because of the unanticipated number of buffer and channel, we cannot | ||||||||
| // use any SampleUtil in this case | ||||||||
| for (SINT frameIdx = 0; | ||||||||
| frameIdx < getOutputSignal().samples2frames(iOutputBufferSize); | ||||||||
| ++frameIdx) { | ||||||||
| for (int channel = 0; channel < chCount; channel++) { | ||||||||
| pOutputBuffer[frameIdx * chCount + channel] = | ||||||||
| m_buffers[channel].data()[frameIdx]; | ||||||||
| } | ||||||||
| } | ||||||||
| } break; | ||||||||
| } | ||||||||
|
|
||||||||
| // readFramesProcessed is interpreted as the total number of frames | ||||||||
| // consumed to produce the scaled buffer. Due to this, we do not take into | ||||||||
| // account directionality or starting point. | ||||||||
| return m_effectiveRate * outputFrames; | ||||||||
| } | ||||||||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not work with debian. We either need to contribute a debian folder for signal smith or copy signalsmith to our lib folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the issue with debian? I guess the copy option could be viable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They do not allow to download third party software during the build process.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh this is going to be a pain for #15888. Though TagLib merged to contribution this morning and I assume will likely release this in the next version, Debian will likely not upgrade to it till the next major release (same goes for Ubuntu).
Not a problem for SignalSmith tho, so let's discuss that there. Is there any objection with using submodules? The SignalSmith stretcher internally relies on submodules too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Submodule do not really fit to the modular concept of vcpkg, Debian and RPM.
Having them in our lib folder is IMHO an unnecessary hassle. We don't have submodules yet. I prefer a literal copy in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can try to contact the author to see if he has any interest.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends. Currently while it is under active development a lib folder copy is better.
When I look after years to Rubberband and Soundtouch the VCPKG and Linux Distro maintained version is "better". Original we had Soundtouch in our lib folder and have removed it as soon a suitable version was found in all distros.
The Linux maintainer anyway try to rip off everything from our libs folder. So we need to keep an eye on the distro maintained version anyway.
See also the mess with "libshout"
In case of taglib we can also decide if we want to ship it via lib folder or PPA.
The same is an issue with Portaudio which has now Pulse support, but is not yet released to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for fedora/rpms afaik.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that Geraint (the library author) came back to me and mentioned they haven't had interest to maintain a Debian package at present, but were happy for us to vendor the code and add a copy to
lib. They also offered to double license the library in case MIT wasn't ideal, but I believe we should be fine with the current license.I will capture that as an issue. It should be pretty straight forward to setup if we want to make SignalSmith stretch part of the stable release, but this solution allows us to easily update during the trial.