diff --git a/.claude/skills/audio-nodes/SKILL.md b/.claude/skills/audio-nodes/SKILL.md index 1845fc7bd..45f9a27ee 100644 --- a/.claude/skills/audio-nodes/SKILL.md +++ b/.claude/skills/audio-nodes/SKILL.md @@ -377,8 +377,10 @@ All graph mutations are queued via `AudioGraphManager` using its own SPSC channe ### Settable channel attributes (channelCount / channelCountMode / channelInterpretation) These are mutable after construction. `AudioNode` (core) exposes virtual `setChannelCount` / `setChannelCountMode` / `setChannelInterpretation`. `channelCount` and `channelCountMode` are read only on the host thread during negotiation, so the JSI setter updates the core field directly then calls `HostNode::renegotiate()` → `Graph::renegotiateNodeChannels()` → `HostGraph::renegotiateNodeChannels()` (reuses `collectNegotiations` + an `AGEvent` buffer swap, self-drain aware when there is no audio/render consumer — offline construction/suspend and realtime suspended/stopped windows). When `AudioBufferSourceNode` `setBuffer` changes channel width, update `outputChannelNumber_` together with the buffer then `renegotiate()` so MAX/CLAMPED_MAX downstream nodes update; the audio event still installs the prebuilt buffer (no audio-thread alloc). `channelInterpretation` is read on the audio thread in `processInputs` (`getInputBuffer()->sum(*input, channelInterpretation_)`), so it MUST be applied via `scheduleAudioEvent`, not mutated directly. +Negotiation decides whether to queue a buffer swap by comparing against `HostVertex::pendingChannelCount` (the width of the last swap queued, seeded in `HostGraph::addNode`), never against the live `getNegotiatedBuffer()`. On a running context every event queued in one JS task lands in the same render quantum, so the live buffer has not seen earlier swaps from the batch yet: a node going 2 → 1 → 2 would see the live 2, skip the second swap, and keep the queued 1 (issue #1332, e.g. `source.connect(gain); source.buffer = stereo;`). The width a node presents to its consumers is derived on demand as `getOutputChannelNumber().value_or(pendingChannelCount)`; it is not cached per pass, so a negotiation pass only walks upstream (`outputs`) from the changed node — inputs are already current. A source (`getNumberOfInputs() == 0`, `ownsOutputBuffer`) is never given a buffer by the graph: it swaps `audioBuffer_` itself, in the same audio event as the content that defines its width, so the two can never disagree on the audio thread. A pass never reaches a source: `connect()` rejects it as a destination (`HostGraph::addEdge` asserts this — it has no guard of its own, so C++ tests calling it raw must respect it too) and `renegotiateNodeChannels(source)` starts the pass at the source's outputs. The same rule applies to any new host-side "is a swap needed?" check. + ### Output channel number vs `channelCount` (sources) -`channelCount` is the spec's input-mixing attribute and never decides how many channels a node emits. `AudioNode` keeps a separate atomic `outputChannelNumber_` (`getOutputChannelNumber()`), initialised from `AudioNodeOptions::outputChannelNumber` and falling back to `channelCount` for nodes whose output follows their negotiated input layout. `HostGraph` negotiation reads the output channel number for source inputs (`getUpstreamChannelCount` returns it when `numberOfInputs_ == 0`), so a spec-default `channelCount = 2` oscillator still negotiates a mono downstream buffer. Rules: +`channelCount` is the spec's input-mixing attribute and never decides how many channels a node emits. `AudioNode` keeps a separate atomic `outputChannelNumber_` (`getOutputChannelNumber()`), initialised from `AudioNodeOptions::outputChannelNumber` and falling back to `channelCount` for nodes whose output follows their negotiated input layout. `HostGraph` negotiation reads each input's `getOutputChannelNumber()` and falls back to its `computedNumberOfChannels` only when that is `nullopt` (`resolveOutputChannelNumberForNode`), so a spec-default `channelCount = 2` oscillator still negotiates a mono downstream buffer. Rules: - A node that always emits one channel (Oscillator, ConstantSource) derives its options from `MonoSourceNodeOptions`; do not lower `channelCount` or override `setChannelCount` to fake it. - A source that learns its width later (AudioBufferSource, AudioFileSource, AudioBufferQueueSource, RecorderAdapter) writes `outputChannelNumber_` in the same step it swaps `audioBuffer_`, never `channelCount_`. - When the host object schedules that swap (AudioBufferSourceNode `setBuffer`), it must first call `setOutputChannelNumber()` on the host thread and `renegotiate()`, otherwise downstream MAX / CLAMPED_MAX nodes negotiate against the stale width until the audio event lands (this broke the StereoPanner WPT `stereopannernode-panning` test when it lived on `updateChannelCount`). Never route this through `updateChannelCount`: that changes the JS-visible `channelCount` attribute. diff --git a/packages/react-native-audio-api/common/cpp/audioapi/core/AudioNode.h b/packages/react-native-audio-api/common/cpp/audioapi/core/AudioNode.h index 2bb8d138d..a3e5ade86 100644 --- a/packages/react-native-audio-api/common/cpp/audioapi/core/AudioNode.h +++ b/packages/react-native-audio-api/common/cpp/audioapi/core/AudioNode.h @@ -36,6 +36,11 @@ class AudioNode : public utils::graph::GraphObject, public std::enable_shared_fr /// `setChannelCount` from the JS thread is race-free with audio processing. [[nodiscard]] size_t getChannelCount() const; + /// @brief Number of inputs fixed at construction; 0 for source nodes. + [[nodiscard]] int getNumberOfInputs() const { + return numberOfInputs_; + } + void setOutputChannelNumber(size_t outputChannelNumber) { outputChannelNumber_.store(static_cast(outputChannelNumber), std::memory_order_release); } @@ -127,9 +132,8 @@ class AudioNode : public utils::graph::GraphObject, public std::enable_shared_fr setOutputBuffer(buffer); } - /// @brief Number of channels this node emits toward AudioDestinationNode - /// when that does not follow its `computedNumberOfChannels`. Default: - /// nullopt, the output is as wide as the inputs are mixed to. Sources + /// @brief Number of channels this node emits toward AudioDestinationNode. + /// Default: nullopt, the output is as wide as the inputs are mixed to. Sources /// present `outputChannelNumber_`; StereoPanner always outputs stereo. [[nodiscard]] virtual std::optional getOutputChannelNumber() const { return std::nullopt; diff --git a/packages/react-native-audio-api/common/cpp/audioapi/core/sources/AudioBufferSourceNode.cpp b/packages/react-native-audio-api/common/cpp/audioapi/core/sources/AudioBufferSourceNode.cpp index fc151b1d5..d06fda2c3 100644 --- a/packages/react-native-audio-api/common/cpp/audioapi/core/sources/AudioBufferSourceNode.cpp +++ b/packages/react-native-audio-api/common/cpp/audioapi/core/sources/AudioBufferSourceNode.cpp @@ -105,7 +105,7 @@ void AudioBufferSourceNode::start(double when, double offset, double duration) { offset = std::min(offset, loopEnd_); } - vReadIndex_ = static_cast(buffer_->getSampleRate() * offset); + vReadIndex_ = buffer_->getSampleRate() * offset; } void AudioBufferSourceNode::disable() { diff --git a/packages/react-native-audio-api/common/cpp/audioapi/core/utils/graph/HostGraph.cpp b/packages/react-native-audio-api/common/cpp/audioapi/core/utils/graph/HostGraph.cpp index 374f770af..f6ea75bba 100644 --- a/packages/react-native-audio-api/common/cpp/audioapi/core/utils/graph/HostGraph.cpp +++ b/packages/react-native-audio-api/common/cpp/audioapi/core/utils/graph/HostGraph.cpp @@ -8,8 +8,8 @@ #include #include #include -#include +#include #include #include #include @@ -47,6 +47,26 @@ inline audioapi::AudioNode *audioNodeOf(const HostGraph::HostVertex *node) { return node->handle->audioNode->asAudioNode(); } +/// @brief Width `node` will emit toward AudioDestinationNode once every +/// queued swap has landed: a fixed override (StereoPanner's stereo output, a +/// source's declared width) or else the width of its pending negotiated +/// buffer. 0 for payloads without an AudioNode (e.g. BridgeNode). +size_t pendingOutputChannelNumber(const HostGraph::HostVertex *node) { + auto *audio = audioNodeOf(node); + if (audio == nullptr) { + return 0; + } + return audio->getOutputChannelNumber().value_or(node->pendingChannelCount); +} + +/// @brief True when `node` is a source: it has no inputs to mix, so it owns +/// its output buffer and swaps it together with the content that defines its +/// width (f.e. AudioBufferSourceNode's buffer number of channels). +bool ownsOutputBuffer(const HostGraph::HostVertex *node) { + auto *audio = audioNodeOf(node); + return audio != nullptr && audio->getNumberOfInputs() == 0; +} + /// @brief Computes the spec's `computedNumberOfChannels` for `dest`: the width /// its inputs (`dest->inputs`) are mixed to, which is also the width of its /// negotiated buffer. Follows the Web Audio rules for `channelCountMode`: @@ -56,12 +76,7 @@ inline audioapi::AudioNode *audioNodeOf(const HostGraph::HostVertex *node) { /// /// When there are no inputs the node keeps its own `channelCount` attribute /// (matches the shape the buffer already had at construction time). -/// -/// `term` identifies the current negotiation pass. -/// @pre Every input is resolved for `term` -/// (`resolveOutputChannelNumberForNode`); its pending output channel number is -/// read from `channelLayout`, never from the live node. -size_t computeNumberOfChannels(const HostGraph::HostVertex *dest, size_t term) { +size_t computeNumberOfChannels(const HostGraph::HostVertex *dest) { auto *destAudio = audioNodeOf(dest); if (destAudio == nullptr) { return 0; @@ -76,8 +91,7 @@ size_t computeNumberOfChannels(const HostGraph::HostVertex *dest, size_t term) { size_t maxInputChannels = 0; for (const HostGraph::HostVertex *input : dest->inputs) { - assert(input->channelLayout.isResolvedFor(term)); - maxInputChannels = std::max(input->channelLayout.outputChannelNumber, maxInputChannels); + maxInputChannels = std::max(pendingOutputChannelNumber(input), maxInputChannels); } if (maxInputChannels == 0) { @@ -91,25 +105,21 @@ size_t computeNumberOfChannels(const HostGraph::HostVertex *dest, size_t term) { } /// @brief If `dest` needs a newly negotiated channel layout, allocates a -/// replacement `DSPAudioBuffer` on the host thread and returns it. -/// Returns `nullptr` when no swap is needed (no audio payload, negotiation -/// converged, or context gone). +/// replacement `DSPAudioBuffer` on the host thread, records its width as +/// pending, and returns it. Returns `nullptr` when no swap is needed (no audio +/// payload, or the pending buffer already has `computedNumberOfChannels`). std::shared_ptr buildNegotiatedBufferIfNeeded( - const HostGraph::HostVertex *dest, + HostGraph::HostVertex *dest, size_t computedNumberOfChannels) { auto *destAudio = audioNodeOf(dest); if (destAudio == nullptr) { return nullptr; } - if (computedNumberOfChannels == 0) { - return nullptr; - } - - const auto current = destAudio->getNegotiatedBuffer(); - if (current != nullptr && current->getNumberOfChannels() == computedNumberOfChannels) { + if (computedNumberOfChannels == 0 || dest->pendingChannelCount == computedNumberOfChannels) { return nullptr; } + dest->pendingChannelCount = computedNumberOfChannels; return std::make_shared( audioapi::RENDER_QUANTUM_SIZE, @@ -124,52 +134,16 @@ struct ChannelNegotiation { using NegotiationBatch = std::vector; -/// @brief Recursively resolves output channel numbers for `node` and every -/// downstream ancestor (`node->inputs`) in negotiation pass `term`. -void resolveOutputChannelNumberForNode(HostGraph::HostVertex *node, size_t term) { - if (node == nullptr || node->channelLayout.isResolvedFor(term)) { - return; - } - - for (HostGraph::HostVertex *input : node->inputs) { - resolveOutputChannelNumberForNode(input, term); - } - - auto *audio = audioNodeOf(node); - if (audio == nullptr) { - // Stamp visited for non-AudioNode payloads (e.g. BridgeNode) so shared DAG - // paths cannot re-enter this node in the same negotiation pass. - node->channelLayout.setResolved(term, 0); - return; - } - - const size_t computedNumberOfChannels = computeNumberOfChannels(node, term); - node->channelLayout.setResolved( - term, audio->getOutputChannelNumber().value_or(computedNumberOfChannels)); -} - /// @brief Starting at `dest` (the connect `to` node), negotiates channel /// layouts for `dest` and every node further upstream (toward /// AudioDestinationNode via `dest->outputs`). void collectChannelNegotiations(HostGraph::HostVertex *dest, size_t term, NegotiationBatch &out) { - if (dest == nullptr || dest->channelLayout.isResolvedFor(term)) { + if (dest == nullptr || !dest->channelLayoutTraversal.visit(term)) { return; } - for (HostGraph::HostVertex *input : dest->inputs) { - resolveOutputChannelNumberForNode(input, term); - } - - if (auto *destAudio = audioNodeOf(dest)) { - const size_t computedNumberOfChannels = computeNumberOfChannels(dest, term); - dest->channelLayout.setResolved( - term, destAudio->getOutputChannelNumber().value_or(computedNumberOfChannels)); - - if (auto negotiatedBuffer = buildNegotiatedBufferIfNeeded(dest, computedNumberOfChannels)) { - out.push_back({.node = dest, .buffer = std::move(negotiatedBuffer)}); - } - } else { - dest->channelLayout.setResolved(term, 0); + if (auto negotiatedBuffer = buildNegotiatedBufferIfNeeded(dest, computeNumberOfChannels(dest))) { + out.push_back({.node = dest, .buffer = std::move(negotiatedBuffer)}); } for (HostGraph::HostVertex *upstream : dest->outputs) { @@ -177,6 +151,8 @@ void collectChannelNegotiations(HostGraph::HostVertex *dest, size_t term, Negoti } } +// clang-tidy does not search deep enough to find the usage of this fun +// NOLINTNEXTLINE void applyChannelNegotiations( const NegotiationBatch &negotiations, Disposer &disposer) { @@ -283,6 +259,13 @@ auto HostGraph::addNode(std::shared_ptr handle) -> std::pairhandle = handle; + // AudioGraph learns about the node only from the event below, so the audio + // thread cannot be swapping this buffer yet. + if (auto *audio = audioNodeOf(newNode)) { + if (auto negotiatedBuffer = audio->getNegotiatedBuffer()) { + newNode->pendingChannelCount = negotiatedBuffer->getNumberOfChannels(); + } + } nodes.push_back(newNode); auto event = [h = std::move(handle)](auto &graph, auto &) mutable { @@ -325,6 +308,11 @@ auto HostGraph::addEdge(HostVertex *from, HostVertex *to) -> Resultoutputs.push_back(to); to->inputs.push_back(from); edgeCount_++; @@ -451,8 +439,11 @@ auto HostGraph::renegotiateNodeChannels(HostVertex *node) -> Resultoutputs) + : collectNegotiations(++channelLayoutTerm_, node); return Ok([negotiations = std::move(negotiations)](AudioGraph &graph, auto &disposer) mutable { applyChannelNegotiations(*negotiations, disposer); diff --git a/packages/react-native-audio-api/common/cpp/audioapi/core/utils/graph/HostGraph.h b/packages/react-native-audio-api/common/cpp/audioapi/core/utils/graph/HostGraph.h index a4bd981cf..47896383e 100644 --- a/packages/react-native-audio-api/common/cpp/audioapi/core/utils/graph/HostGraph.h +++ b/packages/react-native-audio-api/common/cpp/audioapi/core/utils/graph/HostGraph.h @@ -56,34 +56,6 @@ class HostGraph { bool visit(size_t currentTerm); }; - /// Per-node scratch for channel-layout negotiation (host thread only). - /// - /// Follows the same generation-stamp pattern as `TraversalState`: each - /// `addEdge` / `removeEdge` bumps `HostGraph::channelLayoutTerm_`, and nodes - /// visited in that pass stamp `term` with the current value. A node is - /// considered resolved for the pass when `term == channelLayoutTerm_`. - /// - /// During the pass, `outputChannelNumber` stores how many channels this - /// node will present on upstream connections (toward AudioDestinationNode) - /// after negotiation — including overrides such as StereoPanner's fixed - /// stereo output. Downstream nodes read these pending widths from their - /// inputs instead of live output buffers, so multiple connects in one batch - /// (before the AGEvent applies buffer swaps on the audio thread) still see - /// consistent layouts. Values are never read on the audio thread. - struct ChannelLayoutState { - size_t term = 0; - size_t outputChannelNumber = 0; - - [[nodiscard]] bool isResolvedFor(size_t currentTerm) const { - return term == currentTerm; - } - - void setResolved(size_t currentTerm, size_t resolvedOutputChannelNumber) { - term = currentTerm; - outputChannelNumber = resolvedOutputChannelNumber; - } - }; - /// A single node in the HostGraph. struct HostVertex { std::vector inputs; // reversed edges @@ -98,7 +70,14 @@ class HostGraph { /// scrubbed when a linked node is disposed. std::vector linkedNodes; TraversalState traversalState; - ChannelLayoutState channelLayout; + /// Visited stamp for channel-negotiation passes; `HostGraph::channelLayoutTerm_` + /// is bumped once per structural change. + TraversalState channelLayoutTraversal; + /// Width of the last negotiated buffer queued for the audio thread, seeded + /// in `addNode` from the buffer built at construction (host thread only). + /// Negotiation compares against this, not the live buffer: the live one + /// still lacks swaps queued earlier in the same render quantum. + size_t pendingChannelCount = 0; std::shared_ptr handle; // shared handle bridging to AudioGraph bool ghost = false; // kept for cycle detection until AudioGraph confirms deletion diff --git a/packages/react-native-audio-api/common/cpp/test/src/graph/GraphTest.cpp b/packages/react-native-audio-api/common/cpp/test/src/graph/GraphTest.cpp index cd6ee2b65..fe0e53974 100644 --- a/packages/react-native-audio-api/common/cpp/test/src/graph/GraphTest.cpp +++ b/packages/react-native-audio-api/common/cpp/test/src/graph/GraphTest.cpp @@ -605,6 +605,28 @@ TEST_F(GraphTest, RenegotiateNode_SourceOutputChannelNumberChange_CascadesDownst "downstream buffer must widen to 2 channels"; } +TEST_F(GraphTest, RenegotiateNode_LeavesSourceBufferToTheSource) { + // A source swaps its own output buffer together with the content that sets + // its width (AudioBufferSourceNode `setBuffer`); the graph must only cascade + // the declared width downstream, never install a buffer on the source. + auto *source = addSourceNode(*graph, /*channelCountAttribute=*/2, /*outputChannelNumber=*/1); + auto *dest = addChannelCountNode(*graph, {.channelCount = 2, .mode = ChannelCountMode::MAX}); + graph->processEvents(); + + ASSERT_TRUE(graph->addEdge(source, dest).is_ok()); + graph->processEvents(); + ASSERT_EQ(channelsOf(dest), 1u); + + // Declare stereo without swapping the source's buffer, as the host object + // does before its own swap event lands. + source->handle->audioNode->asAudioNode()->setOutputChannelNumber(2); + ASSERT_TRUE(graph->renegotiateNodeChannels(source).is_ok()); + graph->processEvents(); + + EXPECT_EQ(channelsOf(source), 1u) << "The graph must not touch a source's output buffer"; + EXPECT_EQ(channelsOf(dest), 2u) << "The declared width still cascades downstream"; +} + TEST_F(GraphTest, RenegotiateNode_CascadesDownstream) { auto *source = addChannelCountNode(*graph, {.channelCount = 2, .mode = ChannelCountMode::EXPLICIT}); @@ -630,4 +652,54 @@ TEST_F(GraphTest, RenegotiateNode_CascadesDownstream) { << "MAX-mode downstream must follow the renegotiated 6-channel upstream output"; } +// ─── Several changes applied in one render quantum ───────────────────────── +// +// On a running context the audio thread applies every queued event at the +// start of the next quantum, so these tests skip `processEvents()` between the +// changes. A node that goes back to its live width within the batch must still +// queue a swap, otherwise the earlier queued swap is applied last. + +TEST_F(GraphTest, ComputedNumberOfChannels_MaxMode_BatchedChangesBackToLiveWidth) { + auto *mono = addChannelCountNode(*graph, {.channelCount = 1, .mode = ChannelCountMode::EXPLICIT}); + auto *stereo = + addChannelCountNode(*graph, {.channelCount = 2, .mode = ChannelCountMode::EXPLICIT}); + auto *dest = addChannelCountNode(*graph, {.channelCount = 2, .mode = ChannelCountMode::MAX}); + graph->processEvents(); + + ASSERT_TRUE(graph->addEdge(mono, dest).is_ok()); + ASSERT_TRUE(graph->addEdge(stereo, dest).is_ok()); + graph->processEvents(); + + EXPECT_EQ(channelsOf(dest), 2u) + << "Connecting a mono and a stereo input in one batch must leave a stereo buffer"; +} + +TEST_F(GraphTest, ComputedNumberOfChannels_MaxMode_BatchedConnectThenDisconnect) { + auto *mono = addChannelCountNode(*graph, {.channelCount = 1, .mode = ChannelCountMode::EXPLICIT}); + auto *dest = addChannelCountNode(*graph, {.channelCount = 2, .mode = ChannelCountMode::MAX}); + graph->processEvents(); + + ASSERT_TRUE(graph->addEdge(mono, dest).is_ok()); + ASSERT_TRUE(graph->removeEdge(mono, dest).is_ok()); + graph->processEvents(); + + EXPECT_EQ(channelsOf(dest), 2u) + << "Without inputs the buffer falls back to channelCount, even if a mono swap was queued"; +} + +TEST_F(GraphTest, RenegotiateNode_BatchedSourceWideningAfterConnect) { + // Mirrors `source.connect(gain); source.buffer = stereoBuffer;` in one JS task. + auto *source = addSourceNode(*graph, /*channelCountAttribute=*/2, /*outputChannelNumber=*/1); + auto *dest = addChannelCountNode(*graph, {.channelCount = 2, .mode = ChannelCountMode::MAX}); + graph->processEvents(); + + ASSERT_TRUE(graph->addEdge(source, dest).is_ok()); + source->handle->audioNode->asAudioNode()->setOutputChannelNumber(2); + ASSERT_TRUE(graph->renegotiateNodeChannels(source).is_ok()); + graph->processEvents(); + + EXPECT_EQ(channelsOf(dest), 2u) + << "A source widened to stereo in the same batch as its connect must leave a stereo buffer"; +} + } // namespace audioapi::utils::graph