Multithreaded rubberband

[acolombier]

Slightly outdated info

This is a naive attempt to parallelise rubberband processing as per suggested here.

(Test using 44100Hz/11.6ms output settings)

So far the result are very promising! previously, audio drop would occur as soon as start playing a second stem deck, when using powersave governor, and third stem deck when using performance, with both R2 and R3.

Now I can play 4 stem decks in R3 in powersave, with near no audio drop, but it increase when I increase pitches on decks. No issue in performance, although the indicator is very high

In R2 there is no audio drop at all, and CPU indicator remains in the green, even in powersave!

$ nproc --all
20
$ cat /proc/cpuinfo
...
model name	: 12th Gen Intel(R) Core(TM) i7-12700H
cpu MHz (max): 2200 in powersave, 4100 in performance

Adds a worker pool for RubberBand. This pool should be used a single threaded environment (engine thread) and will have to be reviewed if the engine is moved to a multi thread design. This decision was made since such a refactor will likely invalidate the need for such a task specific pool.

[JoergAtGithub]

This looks indeed very promissing! I can now run 2 stem decks in parallel and it worked stable.

But the thread utilization is not optimal yet. Here you see 3 stem decks running in parallel (green is calculation time, red is syncronisation betwee threads (waiting for a mutex etc.)):

Here you see one deck playing a normal stereo track, and two stem decks (main engine thread is on top):

[acolombier]

I assume you would like to see the deck computing in parallel, right?

I'm afraid that won't only require a refactor of the RubberBand, but rather a refactor of the audio engine which processes the channel sequentially.

Also, note that simple deck will not use the threaded version and will perform RubberBand in the calling thread

Or did you mean something else?

[acolombier]

Note that this is only a PoC, and production ready solution could include to make a global pool of thread, instead of owned thread per channel (meaning a max of 4 thread, rather than the current 4(deck)x4(stem))

[JoergAtGithub]

I assume you would like to see the deck computing in parallel, right?

Everything in parallel is too much, at least for my laptop. A thread stealing pool with e.g. 4 workers seems more appropriated.

I'm afraid that won't only require a refactor of the RubberBand, but rather a refactor of the audio engine which processes the channel sequentially.

This would be of cause beyond the scope of the stems project, but from the pure technical viewpoint, I think this is the way to go. There can be many running decks, if we think about the use of samplers.

[acolombier]

With the amount of in-flight PR for the STEM feature, I will pause that one for now. I guess that proves that we have some quick wins to action in order to make STEM more usable.
Once we have some of these out of the way (at least till #13086 as this is based on it), I will look in implementing this properly if that's okay with you.

[acolombier]

I think this feature should be ready for review and testing. I believe the thread usage should now be optimal. I have tested parameters and the current one seems to be the best, slightly better than my initial commit.

[daschuer]

The last commit contains a lot of useful refactoring.
Unfortunately it makes it hard to review the essence of multi threading.

Can you please create a separate PR with these changes. I expect that we can merge them quickly.

[acolombier]

No provider(s) registered for file extension "stem.mp4"

That's very odd! Just to double check, did you use STEM=ON?

Can you please create a separate PR with these changes. I expect that we can merge them quickly.

Yes, no problem, I will remove the STEM changes, only dependency is the stem channel count constant, so I will hardcore it for now and put a TODO

[daschuer]

This solution unfortunately involves a priority inversion.
The critical high prio real time thread waits for the lower prio workers, which the OS can freely reassign to other tasks. The whole engine is on the risk.

In the best case you gain a better CPU utilisation of otherwise idling cores. This comes with the cost of lost deterministic behavior which is more important IMHO.

A better working solution would be a buffer swap algorithm, that just swaps the prepared buffers from the engine thread and does the preparation in the low priority workers. The workers will have a whole cycle to finish the task which may be finished even in critical time situations, if not, only one stem is effected.

This may doubles the latency, but likely allows to compensate that by halving the minimum possible latency.

[daschuer]

Did you consider the poor man's solution of mixing only two buffers of temps before doing sound stretching?

[acolombier]

I have added a version with QThreadPool/QRunnable. I don't have much experience with this so I'm sure it could be done better.

If any, please let me know once all nits/code-smell comments are in so I can solve them all at once and not build up on my frustration.

[JoergAtGithub]

The new version asserts at first clicking on Play button for me:

[Swiftb0y]

If any, please let me know once all nits/code-smell comments are in so I can solve them all at once and not build up on my frustration.

This is difficult to do during review, but I can label the respective comments as nits and you can postpone them until we've decided on a more concrete course. Does that sound like a reasonable compromise?

[Swiftb0y]

only nits so far. its too late in the day to wrap my head around this

[acolombier]

The new version asserts at first clicking on Play button for me

Some last minute untested change... I believe it should be fixed now

[JoergAtGithub]

Works now again:

[Swiftb0y]

fyi LGTM if you address the nits

[Swiftb0y]

As a follow up PR, it would be nice if this wasn't required. It's fine if it means that the audio engine must block when applying the new settings, but forcing to restart is much more annoying (especially considering mixxx's startup times).

[acolombier]

Yes, agreed. This was my attempt at first, but it turns out to require far more changes that I wanted to do in this PR. Happy to try and give it a go in a future one.

[Swiftb0y]

Are you sure it needs that more work? We support changing the stretcher without restarting too, this doesn't seem that much more complicated.

[acolombier]

This is what I thought too, the problem is the way we manage the dynamic stretcher (with a CO) somewhat make things more complicated, and I was left with the only option to add another CO and quite a few synchronisation bits. Potentially, some of the changes I did in the end will make it not as hard as my first attempt, but I'd like to still do that later as this feature is MVP for STEM, while dynamic setting update is nice to have

[Swiftb0y]

LGTM. merge @daschuer?

[daschuer]

Im finally done with the review.
Thank you for this good solution.

[daschuer]

Did some brief test and it works good.
The preferences could be improved though.
The Multithreaded Check Box should be unchecked when using SoundTouch.
Does it really require a Restart when changing it?

[acolombier]

Does it really require a Restart when changing it?

Yes, as discussed here, from my first attempt, adding dynamic reload was at least doubling the size of this PR if not more. Now, with the QThreadPool refactor, it might not be as bad, but I still would like this to be merged first as @JoergAtGithub and @Swiftb0y have already reviewed the existing code. I still have some time free so I'm happy to look into this right away if we me quick enough on this one.