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
| @@ -26,7 +24,7 @@ ConvolverNode::ConvolverNode( | |||
| void ConvolverNode::setBuffer( | |||
| const std::shared_ptr<AudioBuffer> &buffer, | |||
| std::vector<std::unique_ptr<Convolver>> convolvers, | |||
| const std::shared_ptr<ThreadPool> &threadPool, | |||
| const std::shared_ptr<ThreadPool<32>> &threadPool, | |||
There was a problem hiding this comment.
32 is deffinitly too much
threads are used here for compute heavy operations so should be determined by hardware_concurrency. Otherwise they will hog cpu and steal power from each other.
Usually it does not matter that much but here they are not sleeping theyre job is to compute
There was a problem hiding this comment.
i get that this number can be confusing, but its not amount of threads, but task size so threadpool can be safely used on audio thread
There was a problem hiding this comment.
make sure this file is compiled with -O3 -ffast-math
| /// Envelope threshold at which the impulse response is considered inaudible (-80 dB). | ||
| static constexpr double kTailEpsilon = 1e-4; | ||
|
|
||
| /// Hard upper bound on the computed tail length (seconds). | ||
| static constexpr float kMaxTailSeconds = 30.0f; | ||
|
|
||
| /// Keeps `log(r)` bounded away from zero when the pole sits on or outside | ||
| /// the unit circle. | ||
| static constexpr double kPoleRadiusEpsilon = 1e-5; |
There was a problem hiding this comment.
constexpr does not need to be marked static
There was a problem hiding this comment.
but it is inside of class,
"Non-static data member cannot be constexpr; did you intend to make it static? (fix available)clang(invalid_constexpr_member)"
| @@ -108,7 +108,7 @@ class PromiseVendor { | |||
| private: | |||
| jsi::Runtime *runtime_; | |||
| std::shared_ptr<react::CallInvoker> callInvoker_; | |||
| std::shared_ptr<ThreadPool> threadPool_; | |||
| std::shared_ptr<ThreadPool<96>> threadPool_; | |||
There was a problem hiding this comment.
I rewrote threadpool to fatfunction instead of moveonly function and task size needs to be passed in threadpool with new changes
There was a problem hiding this comment.
now it should be slightly more informative
| 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