feat: add SignalSmith keylock engine - #15902
Conversation
|
I have not had a closer look, but in case of the other time stretchers, the timing need to controlled by the output. The filter needs to be settled with enough samples that we can fetch instantly the output samples we need. Refilling is than done on demand to keep the filter settled. I think in this case it is input + output latency. A problem is always changing settings. parts of the buffer needs to be flushed or we need to predict the behaviour. This was a bit of a challenge with the other stretchers. |
| SignalSmith_LIBRARY | ||
| "${CMAKE_INSTALL_LIBDIR}/${CMAKE_STATIC_LIBRARY_PREFIX}signalsmith${CMAKE_STATIC_LIBRARY_SUFFIX}" | ||
| ) | ||
| ExternalProject_Add( |
There was a problem hiding this comment.
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.
What is the issue with debian? I guess the copy option could be viable.
There was a problem hiding this comment.
They do not allow to download third party software during the build process.
There was a problem hiding this comment.
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.
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.
I can try to contact the author to see if he has any interest.
There was a problem hiding this comment.
That's fair, but if we have the opportunity to centralise one in a single place, I guess this is better.
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.
Thanks for the explanation.
There was a problem hiding this comment.
They do not allow to download third party software during the build process.
same for fedora/rpms afaik.
There was a problem hiding this comment.
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.
Thanks for the tip. I am just trying to rework it now so this is done like so. |
|
Here is some heavy load test. Screencast.From.2026-01-25.17-22-23-10MMB.mp4The cheaper preset is "almost" capable of working with no under-runs, in this maxed out setup! (Note the very annoying unrelated bug with the spinny scratch mode remaining enabled after release - which I reckon has been fixed...?) SignalStretch comes with the advantages to perform really well with slowdown track, all the way to zero (see the web-based demo), so I have updated the engine to allow this keylock engine to go beyond the currently set limits. However, there seem to be a cross-faded silence buffer mixed on top, between the [0.1; 0] range which is not allowing to use the full capability of this new feature. Perhaps @daschuer you would know which one I missed? |
47e17e8 to
b39a8e7
Compare
|
There seems to be a loudness reduction when lowering the tempo. This also happens with Rubberband but way less. I think we have the acquirement that the loudness does not change by the tempo. |
I don't think this is the case. You can test it by manually set the |
|
I took this new stretcher for a live set - performance and quality are reeeeally good! There still seem to be some offset occasionaly happening so I will need to investigate that. |
|
I have build this Branch and play a track, crank up the gain to just no clip. With +50 % tempo it starts to clip. This does not happen with the other stretchers or without key lock. When I do instant doubles and remove key lock from one track, I here a small timing offset as a double beat. This must not happen, because you will also hear this double beat in transitions. The issue is also a pending issue with Soundtouch at extreme tempos changes. Rubberband does not suffer this issue. |
| // 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[0].data(frameRead), frameRequired - frameRead); |
There was a problem hiding this comment.
This can also be out of bounds write.
There was a problem hiding this comment.
This should now be covered by the the verify assert above. Note that the maximum buffer size is still well under the maximum value, and maximum tempo rate is 4, with a static assert safety in EngineBuffer.
There was a problem hiding this comment.
An explicit verify assert would be nice to not zero the area behind the buffer which is a hard to find bug.
There was a problem hiding this comment.
Sorry, I cannot quite get my head around this. Could you clarify how you would like this verify assert to work?
acolombier
left a comment
There was a problem hiding this comment.
There still seem to be some offset occasionaly happening so I will need to investigate that.
When I do instant doubles and remove key lock from one track, I here a small timing offset as a double beat.
This should be fixed now. I had missed on the doc that the input latency needs to be computed with the stretch factor.
With +50 % tempo it starts to clip.
I realised that you are not supposed to use seek (now outputSeek that handles more for us) repeatedly as you would end up smashing samples in flight for process. I have now corrected this and now use both the outputSeek has a "best effort" + a correction factor and I couldn't see any obvious clip. I did take this for a big test yet tho so I could be missing some more issues.
| // 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[0].data(frameRead), frameRequired - frameRead); |
There was a problem hiding this comment.
This should now be covered by the the verify assert above. Note that the maximum buffer size is still well under the maximum value, and maximum tempo rate is 4, with a static assert safety in EngineBuffer.
| SignalSmith_LIBRARY | ||
| "${CMAKE_INSTALL_LIBDIR}/${CMAKE_STATIC_LIBRARY_PREFIX}signalsmith${CMAKE_STATIC_LIBRARY_SUFFIX}" | ||
| ) | ||
| ExternalProject_Add( |
There was a problem hiding this comment.
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.
| return tr("Signal Smith Stretch (Cheaper)"); | ||
| case KeylockEngine::SignalSmithDefault: | ||
| return tr("Signal Smith Stretch (Default)"); |
There was a problem hiding this comment.
It is Signalsmith is spelled as one word. Cheaper is the used mode like R3 not our hint in ().
The term Default can be dropped, I think.
| return tr("Signal Smith Stretch (Cheaper)"); | |
| case KeylockEngine::SignalSmithDefault: | |
| return tr("Signal Smith Stretch (Default)"); | |
| return tr("Signalsmith Stretch Cheaper"); | |
| case KeylockEngine::SignalSmithDefault: | |
| return tr("Signalsmith Stretch"); |
There was a problem hiding this comment.
Not quite. "cheaper" and "default" are two provided presets. Since we said there is no need to make that user friendly yet, lets keep them align for now.
| // 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 |
There was a problem hiding this comment.
For my expectation we alwasy need this m_currentFrameOffset == m_expectedFrameLatency.
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).
This means there is no expectation that these value are equal at the end, there is expectation that they will be closer tho, so I can put an assert which capture this (like if we start with values that differ, we expect them to have move closer by the end of the process)
| // 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[0].data(frameRead), frameRequired - frameRead); |
There was a problem hiding this comment.
An explicit verify assert would be nice to not zero the area behind the buffer which is a hard to find bug.
|
Wow, this performs really well! I merged it into my isolate-cpu working branch and I could finally halve the latency to 5ms without getting underruns with the default version Regarding the quality: |
|
Btw let's use SiS as short name, so we have ST, RB2, RB3, SiS |
|
Could you clarify where you would like to see this abbreviation? |
|
I mean, In the GUI we use the full name, so SiS is just for code comments, discussion etc. |
|
Sooo, instead of the AutoDJ test I went for a real 1:1 test.
I tested with a high-res vocal track as Flac Olicía - Zwei
|
|
Thank you for that detailed benchmark @ronso0! |
|
I have fixed the conflict now. Let me know if there anything that still needs updating @daschuer |
|
Just a headsup: SiS vs. RB3: you can hear the crackling at the crossover, vs. no issue with RB3 Any idea what might be causing this? |
|
There is a CMake warning: |
| if (m_currentFrameOffset == 0 && | ||
| m_currentFrameOffset < m_expectedFrameLatency |
There was a problem hiding this comment.
Can this become:
| if (m_currentFrameOffset == 0 && | |
| m_currentFrameOffset < m_expectedFrameLatency | |
| if (m_currentFrameOffset == 0 |
There was a problem hiding this comment.
The m_expectedFrameLatency is always > 0 and therefore is the second condition redundant.
There was a problem hiding this comment.
Yes but m_currentFrameOffset is not always zero, as you have assumed
| std::min(m_expectedFrameLatency - m_currentFrameOffset, | ||
| SINT(MAX_BUFFER_LEN)))); | ||
| m_stretch.outputSeek(m_bufferPtrs.data(), frameRead); | ||
| m_currentFrameOffset += frameRead; |
There was a problem hiding this comment.
| m_currentFrameOffset += frameRead; | |
| m_currentFrameOffset = frameRead; |
There was a problem hiding this comment.
The current code is correct. frameRead contains the relative offset, but m_currentFrameOffset is absolute
There was a problem hiding this comment.
m_currentFrameOffset is 0 here. Therefore the + operation is redundant.
There was a problem hiding this comment.
You have probably missed that we are her in the if (m_currentFrameOffset == 0 && block.
| std::min(m_expectedFrameLatency - m_currentFrameOffset, | ||
| SINT(MAX_BUFFER_LEN)))); |
There was a problem hiding this comment.
| std::min(m_expectedFrameLatency - m_currentFrameOffset, | |
| SINT(MAX_BUFFER_LEN)))); | |
| std::min(m_expectedFrameLatency, SINT(MAX_BUFFER_LEN)))); |
There was a problem hiding this comment.
No, the current code is correct - m_currentFrameOffset contains the current offset, but we need to see how much more (or less) frames we need to adjust the offset.
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.
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.
This is a warning related to the SiS project itself. I'll look to issue a patch upstream in the future. |
Why not vendor the library now. We want to do it anyway, right? This way we get also rid of the unnecessary cmake changes and solve the PPA build right away- |
|
Why is the filter reset when the track is paused? I think we need only reset the filter in case we have a seek. Looping is handled inside the 'readAheadManager::getNextSamples()' so the resampler should work independent form looping. Do you have an idea why this is not the case? Maybe both issues are source by the same bug? |
|
This is an experiment and I don't want to clutter the repo for now. It is clear that we will need to rework library distribution ahead of making this feature globally available, but there is no need to do it for the experimental phase. |
Before we can merge a PR it must be mature. Thats the principle of Continious Code integration! I suggest to create another PR which just adds the signalsmith-strech code to the lib folder first. This would be a no-brainer that we could easily merge before this PR. |
Done: #15986 |
|
Just a heads-up as I have the impression that fact is not obvious: |
Why you marked it as [Don't merge] and experimental ? It's only purpose is to merge it before this PR, to get rid of the problematic code in this PR. |
Because SiS is not recommended for stable use yet, as a few issues have been highlighted in this PR. Some of those issues are releated to upstream problem, so why it is experiemental, I want to keep the git fetching to easily pin new version. Since the source is composed of two indirections of git submodules, it is quite a pain to take a new snapshot on every changes.
Thanks @ronso0! Indeed, when we decide that SiS is ready, we can merge #15986 and consider setting SIGNALSMITH=true to make it available by default. |
Which part is a pain. I consider it just a tree replace operation in you favorite file explorer.
Which one? This one already performed way better than Rubberband. The only issue it the missing filter settlement. Which can be fixed in our code. I also share the concerns to merge this premature. |
I answered previously, the multiple indirection of git modules.
The CMake warning for example
Great, let's just block this feature then. |
|
Does this mean you have no interest to look into the looping and setting issue? |
|
Not in the immediate term. I consider that to be enhancement, which could happen once we consider this experiment mature enough to distribute in the development branch. |
|
Despite the rare issue with loop wrap-around I've been using this for the last weeks and I'm pretty happy with it.
edit hmm, the loop pops also affect short looprolls, so I need to be very cautious when using them However, this reveals an issue with the |
|
The branch and this comment were prepared with ChatGPT assistance. I pushed a separate SignalSmith follow-up branch: https://github.com/0cwa/mixxx/tree/feat/signalsmith-stretch-followups It contains two SignalSmith-specific commits: Each commit was built in the mixxx-build distrobox and its focused engine suite passed 14/14. Bungee, Rekordbox, and unrelated selector changes were intentionally excluded. |



This adds a new stretching engine, based of Signal Smith.
Quality appears to be as good as RB3, potentially even better, but performance looks way better, easily allowing 4 stems playing without drops.
I still haven't managed to come up with an elegant solution to address the latency, leading to desync at the moment.
If anybody has some ideas in mind, I am all hears!
edit(ronso0) requires setting the CMake flag
SIGNALSMITH