feat: Graph refactor chapter II#991
Conversation
…StereoPannerNode constraints
maciejmakowski2003
force-pushed
the
feat/graph_refactor_2
branch
2 times, most recently
from
62ae703 to
71f1fd0
Compare
There was a problem hiding this comment.
just checking in
is it possible to call start and stop concurently (eg at the same time)?
poneciak57
left a comment
poneciak57
left a comment
There was a problem hiding this comment.
Overally it looks really great. One global issue is lack of high level documentation. It would be great to write/generate some high level design doc on top of that or keep existing ones in sync with the code
But as i said overally good job 👍
| delay_ring::bufferOperation( | ||
| delayBuffer, audioBuffer_, framesToProcess, writeIndex, delay_ring::BufferAction::WRITE); |
There was a problem hiding this comment.
why u have Action type instead of having two methods write/read?
There was a problem hiding this comment.
because it shares a lot of code under the hood, but it would be hard to split it into two separate functions and put common code elsewhere, this way we can easily control what is shared and what is not
There was a problem hiding this comment.
@copilot address this architecture choice discussion
There was a problem hiding this comment.
hope he will not empty out my wallet for this question ....
There was a problem hiding this comment.
eh...
I would challenge the assumption that minimizing code duplication should be the primary goal.
Read and write are distinct operations with different semantics. The fact that they share some implementation details doesn't necessarily mean they should be exposed through a single function with an Action enum.
I'd generally prefer separate read() and write() APIs and keep any shared logic internal. A slightly larger implementation is often a reasonable trade-off for a cleaner and more explicit interface.
There was a problem hiding this comment.
make sure this file is compiled with -O3 -ffast-math
| auto drainA = [&] { | ||
| AGEvent ev; | ||
| while (eventReceiver_.try_receive(ev) == ResponseStatus::SUCCESS) { | ||
| if (ev) { | ||
| ev(audioGraph, *disposer_); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| // Steady-state: drain any A events queued since the last cycle. | ||
| drainA(); | ||
|
|
||
| // For every pending orphan on Channel B: ensure Channel A's receive | ||
| // cursor has reached the barrier the GC thread snapshotted at orphan | ||
| // enqueue time. Until that barrier is met, every A event that the | ||
| // orphan happens-after must still be in flight; we drain A again to | ||
| // catch up. Only then do we consume and apply the orphan. | ||
| while (const OrphanEnvelope *front = gcEventReceiver_.try_peek()) { | ||
| if (eventReceiver_.rcvCursor() < front->barrier) { | ||
| drainA(); | ||
| if (eventReceiver_.rcvCursor() < front->barrier) { | ||
| // Producer is mid-send on Channel A; the event will arrive | ||
| // imminently. Defer this orphan to the next processEvents() | ||
| // tick rather than burning audio-thread cycles spinning. | ||
| break; | ||
| } | ||
| } | ||
| OrphanEnvelope consumed; |
There was a problem hiding this comment.
this "vector-clock-like" aproach is great, but the logic seems to be split between few places. It would be nice to maybe make some wrapper on top of the SPSC to handle that scenario
3 participants
Closes RNAA-433
Introduced changes
Checklist