Skip to content

Conversation

@dochoi-bot
Copy link

#93 (comment)

image

It seems to be causing a race condition with the resetBuffers function.
Are there any potential risks with this implementation?


rendererContext.lock.lock()
bufferContext.frameStartIndex = (bufferContext.frameStartIndex + totalFramesCopied) % bufferContext.totalFrameCount
bufferContext.frameUsedCount -= totalFramesCopied
Copy link
Author

@dochoi-bot dochoi-bot Dec 2, 2025

Choose a reason for hiding this comment

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

Suggested change
bufferContext.frameUsedCount -= totalFramesCopied
if bufferContext.frameUsedCount >= totalFramesCopied {
bufferContext.frameUsedCount -= totalFramesCopied
} else {
bufferContext.frameUsedCount = 0
}

As another option,
What are your thoughts on adding overflow protection here and at line 124?

@dimitris-c
Copy link
Owner

Expanding the lock scope is not ideal, even the locks I have shouldn't be there because the audio thread is a high priority and will not "wait" for a lock to get unlocked. So if you expand the lock you might hit more issues...

Ideally I would have to move into a lock-less ring-buffer in order to properly handle this, but this is needs time and testing

@dochoi-bot
Copy link
Author

dochoi-bot commented Dec 5, 2025

#125 (comment)

@dimitris-c
Got it, that makes sense.
Given that, do you think applying a guard like this (to avoid underflow when bufferContext.frameUsedCount < totalFramesCopied ) still carries potential risks, even as a temporary workaround?

I understand it’s not the ideal long-term solution compared to a proper lock-free ring buffer, but I’m wondering if it’s acceptable as a short-term mitigation until we refactor the buffer logic.

@dimitris-c
Copy link
Owner

I will try to add this in on a separate fix, thanks - most likely I'll add a temporary solution, thanks for the PR —
closing in favour of #127

@dimitris-c dimitris-c closed this Dec 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants