[audio] added signal node#33223
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThis change introduces a new 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/framework/audio/engine/internal/mixerchannel.cpp (1)
222-232:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnguarded
m_signalNode->isSilent()calls in processing logic.Lines 223 and 228 call
m_signalNode->isSilent()without null checks, whilem_sourceNodeon line 222 is properly guarded. This is inconsistent with the pattern at line 257.🛡️ Proposed fix using consistent pattern
if (m_sourceNode) { - if (!m_params.muted || !m_signalNode->isSilent()) { + bool isSilent = m_signalNode ? m_signalNode->isSilent() : true; + if (!m_params.muted || !isSilent) { m_sourceNode->process(buffer, samplesPerChannel); } } - if (m_params.muted && m_signalNode->isSilent()) { + bool isSilent = m_signalNode ? m_signalNode->isSilent() : true; + if (m_params.muted && isSilent) { std::fill(buffer, buffer + samplesPerChannel * audioChannelCount, 0.f); setNoAudioSignal(); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/audio/engine/internal/mixerchannel.cpp` around lines 222 - 232, The code calls m_signalNode->isSilent() without null checks (while m_sourceNode is guarded), causing potential null derefs; update the logic in the processing block to check m_signalNode before calling isSilent()—for example, guard both uses of m_signalNode->isSilent() with a nullptr check (e.g., if (m_signalNode && !m_params.muted && !m_signalNode->isSilent()) for the process decision and if (m_params.muted && (!m_signalNode || m_signalNode->isSilent())) before zeroing the buffer), and keep calls to setNoAudioSignal() and m_sourceNode->process(...) unchanged otherwise so behavior remains consistent with the pattern used later around line 257.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/framework/audio/engine/internal/mixerchannel.cpp`:
- Around line 153-166: The three methods audioSignalChanges(),
setNoAudioSignal(), and notifyAboutAudioSignalChanges() access m_signalNode
without null checks which can lead to crashes; either ensure init() is always
called from the MixerChannel constructor (so m_signalNode is guaranteed
initialized, referencing init() and m_signalNode) or add defensive guards inside
each method (e.g., check m_signalNode and return a safe default for
audioSignalChanges() and no-op for setNoAudioSignal() and
notifyAboutAudioSignalChanges()), keeping behavior consistent with isSilent()
which uses m_signalNode ? ... : true.
- Around line 207-210: Add a null check around the signal node before entering
the chain-processing block: only set m_chainProcessing = true and call
m_signalNode->process(buffer, samplesPerChannel) if m_signalNode != nullptr, and
ensure m_chainProcessing is reset to false in all paths (i.e., do not set it
true unless you will later set it false). Update the code in mixerchannel.cpp
around the existing m_chainProcessing / m_signalNode usage so the audio thread
will skip processing when m_signalNode is null and will not leave
m_chainProcessing stuck true.
- Around line 185-190: m_signalNode and m_controlNode are accessed without null
checks while m_sourceNode is guarded; update the code around the setOutputSpec
calls to perform the same defensive null-check pattern used for m_sourceNode by
wrapping m_signalNode->setOutputSpec(spec) and
m_controlNode->setOutputSpec(spec) in if (m_signalNode) { ... } and if
(m_controlNode) { ... } respectively so all three nodes are only dereferenced
when non-null; keep the existing call to m_sourceNode->setOutputSpec(spec)
unchanged.
---
Outside diff comments:
In `@src/framework/audio/engine/internal/mixerchannel.cpp`:
- Around line 222-232: The code calls m_signalNode->isSilent() without null
checks (while m_sourceNode is guarded), causing potential null derefs; update
the logic in the processing block to check m_signalNode before calling
isSilent()—for example, guard both uses of m_signalNode->isSilent() with a
nullptr check (e.g., if (m_signalNode && !m_params.muted &&
!m_signalNode->isSilent()) for the process decision and if (m_params.muted &&
(!m_signalNode || m_signalNode->isSilent())) before zeroing the buffer), and
keep calls to setNoAudioSignal() and m_sourceNode->process(...) unchanged
otherwise so behavior remains consistent with the pattern used later around line
257.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6ff73973-5d0e-49db-b992-29ec732468b2
📒 Files selected for processing (10)
src/framework/audio/engine/CMakeLists.txtsrc/framework/audio/engine/internal/audiocontext.cppsrc/framework/audio/engine/internal/audiofactory.cppsrc/framework/audio/engine/internal/mixer.cppsrc/framework/audio/engine/internal/mixer.hsrc/framework/audio/engine/internal/mixerchannel.cppsrc/framework/audio/engine/internal/mixerchannel.hsrc/framework/audio/engine/internal/nodes/audiooutputnode.hsrc/framework/audio/engine/internal/nodes/signalnode.cppsrc/framework/audio/engine/internal/nodes/signalnode.h
💤 Files with no reviewable changes (1)
- src/framework/audio/engine/internal/nodes/audiooutputnode.h
| AudioSignalChanges MixerChannel::audioSignalChanges() const | ||
| { | ||
| return m_audioSignalNotifier.audioSignalChanges; | ||
| return m_signalNode->audioSignalChanges(); | ||
| } | ||
|
|
||
| void MixerChannel::setNoAudioSignal() | ||
| { | ||
| m_signalNode->setNoAudioSignal(); | ||
| } | ||
|
|
||
| void MixerChannel::notifyAboutAudioSignalChanges() | ||
| { | ||
| m_signalNode->notifyAboutAudioSignalChanges(); | ||
| } |
There was a problem hiding this comment.
Inconsistent null checking for m_signalNode risks null pointer dereference.
These methods access m_signalNode without null checks, but isSilent() at line 257 does check: m_signalNode ? m_signalNode->isSilent() : true. If any of these methods is called before init(), or if the initialization order changes, the code will crash.
Either add defensive null checks for consistency, or ensure init() is called from the constructor to guarantee the invariant.
🛡️ Proposed fix to add null guards
AudioSignalChanges MixerChannel::audioSignalChanges() const
{
- return m_signalNode->audioSignalChanges();
+ return m_signalNode ? m_signalNode->audioSignalChanges() : AudioSignalChanges();
}
void MixerChannel::setNoAudioSignal()
{
- m_signalNode->setNoAudioSignal();
+ if (m_signalNode) {
+ m_signalNode->setNoAudioSignal();
+ }
}
void MixerChannel::notifyAboutAudioSignalChanges()
{
- m_signalNode->notifyAboutAudioSignalChanges();
+ if (m_signalNode) {
+ m_signalNode->notifyAboutAudioSignalChanges();
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| AudioSignalChanges MixerChannel::audioSignalChanges() const | |
| { | |
| return m_audioSignalNotifier.audioSignalChanges; | |
| return m_signalNode->audioSignalChanges(); | |
| } | |
| void MixerChannel::setNoAudioSignal() | |
| { | |
| m_signalNode->setNoAudioSignal(); | |
| } | |
| void MixerChannel::notifyAboutAudioSignalChanges() | |
| { | |
| m_signalNode->notifyAboutAudioSignalChanges(); | |
| } | |
| AudioSignalChanges MixerChannel::audioSignalChanges() const | |
| { | |
| return m_signalNode ? m_signalNode->audioSignalChanges() : AudioSignalChanges(); | |
| } | |
| void MixerChannel::setNoAudioSignal() | |
| { | |
| if (m_signalNode) { | |
| m_signalNode->setNoAudioSignal(); | |
| } | |
| } | |
| void MixerChannel::notifyAboutAudioSignalChanges() | |
| { | |
| if (m_signalNode) { | |
| m_signalNode->notifyAboutAudioSignalChanges(); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framework/audio/engine/internal/mixerchannel.cpp` around lines 153 - 166,
The three methods audioSignalChanges(), setNoAudioSignal(), and
notifyAboutAudioSignalChanges() access m_signalNode without null checks which
can lead to crashes; either ensure init() is always called from the MixerChannel
constructor (so m_signalNode is guaranteed initialized, referencing init() and
m_signalNode) or add defensive guards inside each method (e.g., check
m_signalNode and return a safe default for audioSignalChanges() and no-op for
setNoAudioSignal() and notifyAboutAudioSignalChanges()), keeping behavior
consistent with isSilent() which uses m_signalNode ? ... : true.
| m_signalNode->setOutputSpec(spec); | ||
| m_controlNode->setOutputSpec(spec); | ||
|
|
||
| if (m_sourceNode) { | ||
| m_sourceNode->setOutputSpec(spec); | ||
| } |
There was a problem hiding this comment.
Same null-check inconsistency for m_signalNode and m_controlNode.
Lines 185-186 access m_signalNode and m_controlNode without null checks, while m_sourceNode on line 188 is properly guarded. Apply the same defensive pattern here.
🛡️ Proposed fix
void MixerChannel::onOutputSpecChanged(const OutputSpec& spec)
{
ONLY_AUDIO_ENGINE_THREAD;
- m_signalNode->setOutputSpec(spec);
- m_controlNode->setOutputSpec(spec);
+ if (m_signalNode) {
+ m_signalNode->setOutputSpec(spec);
+ }
+ if (m_controlNode) {
+ m_controlNode->setOutputSpec(spec);
+ }
if (m_sourceNode) {
m_sourceNode->setOutputSpec(spec);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| m_signalNode->setOutputSpec(spec); | |
| m_controlNode->setOutputSpec(spec); | |
| if (m_sourceNode) { | |
| m_sourceNode->setOutputSpec(spec); | |
| } | |
| if (m_signalNode) { | |
| m_signalNode->setOutputSpec(spec); | |
| } | |
| if (m_controlNode) { | |
| m_controlNode->setOutputSpec(spec); | |
| } | |
| if (m_sourceNode) { | |
| m_sourceNode->setOutputSpec(spec); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framework/audio/engine/internal/mixerchannel.cpp` around lines 185 - 190,
m_signalNode and m_controlNode are accessed without null checks while
m_sourceNode is guarded; update the code around the setOutputSpec calls to
perform the same defensive null-check pattern used for m_sourceNode by wrapping
m_signalNode->setOutputSpec(spec) and m_controlNode->setOutputSpec(spec) in if
(m_signalNode) { ... } and if (m_controlNode) { ... } respectively so all three
nodes are only dereferenced when non-null; keep the existing call to
m_sourceNode->setOutputSpec(spec) unchanged.
| if (!m_chainProcessing) { | ||
| m_chainProcessing = true; | ||
| m_signalNode->process(buffer, samplesPerChannel); | ||
| m_chainProcessing = false; |
There was a problem hiding this comment.
Missing null check for m_signalNode in audio processing path.
m_signalNode->process() is called without a null guard. Since this runs on the audio thread (ONLY_AUDIO_PROC_THREAD), a crash here could cause audio dropouts or system instability.
🛡️ Proposed fix
if (!m_chainProcessing) {
m_chainProcessing = true;
- m_signalNode->process(buffer, samplesPerChannel);
+ if (m_signalNode) {
+ m_signalNode->process(buffer, samplesPerChannel);
+ }
m_chainProcessing = false;
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!m_chainProcessing) { | |
| m_chainProcessing = true; | |
| m_signalNode->process(buffer, samplesPerChannel); | |
| m_chainProcessing = false; | |
| if (!m_chainProcessing) { | |
| m_chainProcessing = true; | |
| if (m_signalNode) { | |
| m_signalNode->process(buffer, samplesPerChannel); | |
| } | |
| m_chainProcessing = false; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framework/audio/engine/internal/mixerchannel.cpp` around lines 207 - 210,
Add a null check around the signal node before entering the chain-processing
block: only set m_chainProcessing = true and call m_signalNode->process(buffer,
samplesPerChannel) if m_signalNode != nullptr, and ensure m_chainProcessing is
reset to false in all paths (i.e., do not set it true unless you will later set
it false). Update the code in mixerchannel.cpp around the existing
m_chainProcessing / m_signalNode usage so the audio thread will skip processing
when m_signalNode is null and will not leave m_chainProcessing stuck true.
2 participants
No description provided.