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.

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

Fix RubberBand segafult when changing audio setting #13649

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

acolombier
Copy link
Member

Fix a bug introduced by #13143 which would lead to segfault when updating the audio output setting (eg. sample rate, buffer length)

This solution is not ideal as it will simply ensure that the main thread does't free the stretcher while the engine thread is still processing, but the other rubberband instance will still be swapped, leading to the swapped stem channel to be offset-ed by one, and creating a weird desync.

I cannot found of a solution without introducing some heavy locking, which is a big no-go for the realtime thread.

Just to summarise the problem

  • Edit the audio setting
  • Trigger sent on keylock_engine CO (main thread)
  • Slot EngineBuffer::slotKeylockEngineChanged is ran on main thread and cycle the RubberBand wrappers with new instances using the correct settings

Mainwhile:

  • RubberBandTask::waitReady is likely blocked on the engine thread, waiting for buffer scaling

Due to the realtime aspect, the engine thread doesn't have a QEventLoop which could have been useful to schedule to stretcher setup slot.

Happy to go with an alternative solution, if you have a suggestion!

@@ -97,7 +97,6 @@ void EngineBufferScaleRubberBand::onSignalChanged() {
// TODO: Resetting the sample rate will cause internal
// memory allocations that may block the real-time thread.
// When is this function actually invoked??
m_rubberBand.clear();
Copy link
Member Author

Choose a reason for hiding this comment

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

This appears to be a duplicate (see line 114)

@JoergAtGithub
Copy link
Member

With this PR I get DEBUG ASSERT: "m_rubberBand.isValid()" in function double __cdecl EngineBufferScaleRubberBand::scaleBuffer(float *,__int64) at D:\mixxx\src\engine\bufferscalers\enginebufferscalerubberband.cpp:288 when I change the buffer size.

@acolombier
Copy link
Member Author

@JoergAtGithub I believe you would also get the assert on main, can you confirm?

@acolombier acolombier added this to the 2.6-beta milestone Sep 19, 2024
@JoergAtGithub
Copy link
Member

@JoergAtGithub I believe you would also get the assert on main, can you confirm?

Indeed, this also occurs with main!

@acolombier
Copy link
Member Author

I'm not sure how to go about this. In the one hand, this quick fix should prevent the crash, but doesn't address the fundamental issue of RubberBand being recreated from another thread. I feel like we should probably get this fix in to mitigate the issue, and decide one what is going to be the approach to add some synchronisation with the audio engine thread

@JoergAtGithub
Copy link
Member

JoergAtGithub commented Sep 19, 2024

If we change these settings, we will have a audio interruption/glitch anyway. Why not stop all these threads and restart them with the new parameters?

@acolombier
Copy link
Member Author

I have tried another fix, using your suggestion @JoergAtGithub . Seems to work well, without any locks! Let me know if it works for you

@JoergAtGithub
Copy link
Member

This works without segfault or assert for me!

@Swiftb0y
Copy link
Member

@acolombier can you squash the two commits once this is ready? The latter one essentially undos the first one so there is not much value in keeping it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants