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

[SOFT_events extension] Events are being dropped if the event queue overflows #1003

Open
RaphiMC opened this issue Jun 15, 2024 · 2 comments

Comments

@RaphiMC
Copy link

RaphiMC commented Jun 15, 2024

My application (A "tracker-like" song player (https://github.com/RaphiMC/NoteBlockTool/blob/7815dc3ef7cfbe9ee956e185e5b9de669ec86a5a/src/main/java/net/raphimc/noteblocktool/audio/soundsystem/impl/OpenALSoundSystem.java)) uses the SOFT_events extension to listen for AL_EVENT_TYPE_SOURCE_STATE_CHANGED_SOFT events in order to maintain a list of currently active sources. I am facing an issue where this list keeps growing due to some sources not being removed (Which I assume happens because the stop event for that source was dropped). My application accepts user supplied song files which can reach close to thousand of new sources being generated every second, which I assume causes the event buffer to overfill / not keep up and drop old/new events. This is worsened by the fact, that my application synchronizes access to the sound sources list, which causes some blocking inside the OpenAL event thread and slightly slows down event comsumption.

From a quick look through the OpenAL Soft source code I found this line which initializes a ring buffer used as event queue:

mAsyncEvents = RingBuffer::Create(511, sizeof(AsyncEvent), false);

With the size being relatively small (511) my assumption is that old events are being overwritten or new events being dropped if the buffer is full.

A potential fix could be to make the ring buffer size grow with the amount of concurrent sources configured when creating the context. Or have a parameter for the event queue when creating the context (So applications can define themselves how large the event queue should be)

@kcat
Copy link
Owner

kcat commented Jun 16, 2024

The main issue here is that the mixer thread is the thing generating events, dictating when the source actually starts/stops/pauses, when it finishes with a buffer, etc, which needs to be real-time safe. Because of that, it can't allocate or deallocate memory, so the events are stored in preallocated memory (the ring buffer), queued to be handled by a separate event handler thread (which your callback is called on, allowing it to be non-real-time safe). If the preallocated buffer gets filled up with pending events, either because too many events are generated too fast and/or because the callback takes too long to process them, those excess events get lost since the mixer can't allocate more memory or wait for space to become available to store them.

Obviously this causes a problem when a significant number of events are generated without enough time to handle them. And there's no way to accurately predict how many events can get queued, as it depends on how well the event handler thread is scheduled by the OS, how often the audio device does mixing updates, how fast/slow the callback can be, and the behavior of the app/sounds. Basing the queue size on the number of sources can result in an unnecessarily large queue, as it depends on how many start and/or stop very close together (within a few dozen milliseconds normally). Assuming the app even enables notification of those events, which isn't known at the time. As it is, a size of 511 is on the high side for most uses, but this is the first report I'm getting of it being too small.

Having the app specify/hint the queue size seems like it'd rely a lot on trial-and-error, and be somewhat system dependent. Some systems may be more prone to clogging the event queue than others. The amount of memory used for each queue element also isn't public and can change between versions, so specifying a large queue just the be safe can cause a significant memory increase when updating.

I unfortunately don't have an answer for how to best deal with the problem, yet. I'd really like to find a way that isn't limited to a fixed size queue, and also won't suffer from the A-B-A problem. For now, I have been able to increase the queue size while also lowering the memory use (reduced the size of each queue entry to a bit less than 1/4th, and doubled the size of the queue). Ways to improve it further are open to discussion.

RaphiMC added a commit to RaphiMC/NoteBlockTool that referenced this issue Jun 16, 2024
AL_SOFT_events is currently not well suited for use in NoteBlockTool. See kcat/openal-soft#1003
@RaphiMC
Copy link
Author

RaphiMC commented Jun 16, 2024

I understand that it isn't as easy to fix as I thought, especially fixing it properly. For now I reverted back to polling OpenAL due to the described issue and a potential race condition I discovered as well, so no preassure to find a quick fix. Thanks for your efforts nonetheless.

If anyone is curious, here is the race condition I found (Specific to my application and I am not expecting it to be fixed):
My application has a builtin concurrent sound source limit (Works by tracking all currently playing sources). If a new source should be played, but the limit is already reached the oldest source will be deleted, so the new one can start playing. When the source is deleted its removed from the tracked list and the new one is added to it. Due the async nature of SOFT_events the stop event for the deleted source could still be in the event queue. The issue arises when the newly allocated source has the same id as the deleted one and the event for the old one is still in the event queue. As soon as the event gets to my application, it causes the newly created source to be deleted because my application doesn't know if the event is outdated or not (outdated meaning, the source which generated the event doesn't exist anymore and thus all queued events from it aren't relevant anymore and would instead affect the newly created source)

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

No branches or pull requests

2 participants