From d3d710e0adac3798ae65f1e2b34fd582e4c02bc6 Mon Sep 17 00:00:00 2001 From: Michael Gregorius Date: Thu, 14 Sep 2023 23:12:22 +0200 Subject: [PATCH 1/5] Remove identical calls to `InstrumentTrack::processAudioBuffer` Remove identical calls to `InstrumentTrack::processAudioBuffer` which appear in the overridden implementations of `Instrument::play` and `Instrument::playNotes`. Instead the call to `processAudioBuffer` has been moved into `InstrumentPlayHandle::play` and `InstrumentTrack::playNote`. These two methods call the aforementioned methods of `Instrument`. Especially in the case of `InstrumentTrack::playNote` the previous implementation resulted in some unncessary "ping pong" where `InstrumentTrack` called a method on an `Instrument` which then in turn called a method on `InstrumentTrack`. And this was done in almost every instrument. In `InstrumentTrack::playNote` an additional check was added which only calls `processAudioBuffer` if the buffer is not `nullptr`. The reason is that under certain circumstances `PlayHandle::doProcessing` calls the `play` method by explicitly passing a `nullptr` as the buffer. This behavior was added with commit 7bc97f5d5bd. Because it is unknown if this was done for some side effects the code was adjusted so that it behaves identical in this case. Move the complex implementation for `InstrumentPlayHandle::play` and `InstrumentPlayHandle::isFromTrack` into the cpp file and optimize the includes. --- include/InstrumentPlayHandle.h | 38 +++--------------- .../AudioFileProcessor/AudioFileProcessor.cpp | 3 -- plugins/BitInvader/BitInvader.cpp | 2 - plugins/CarlaBase/Carla.cpp | 3 -- plugins/FreeBoy/FreeBoy.cpp | 1 - plugins/GigPlayer/GigPlayer.cpp | 2 - plugins/Kicker/Kicker.cpp | 2 - plugins/Lb302/Lb302.cpp | 1 - plugins/Lv2Instrument/Lv2Instrument.cpp | 2 - plugins/Monstro/Monstro.cpp | 2 - plugins/Nes/Nes.cpp | 2 - plugins/OpulenZ/OpulenZ.cpp | 4 -- plugins/Organic/Organic.cpp | 2 - plugins/Patman/Patman.cpp | 2 - plugins/Sf2Player/Sf2Player.cpp | 2 - plugins/Sfxr/Sfxr.cpp | 3 -- plugins/Sid/SidInstrument.cpp | 2 - plugins/Stk/Mallets/Mallets.cpp | 2 - plugins/TripleOscillator/TripleOscillator.cpp | 2 - plugins/Vestige/Vestige.cpp | 2 - plugins/Vibed/Vibed.cpp | 2 - plugins/Watsyn/Watsyn.cpp | 2 - plugins/Xpressive/Xpressive.cpp | 2 - plugins/ZynAddSubFx/ZynAddSubFx.cpp | 1 - src/core/InstrumentPlayHandle.cpp | 39 +++++++++++++++++++ src/tracks/InstrumentTrack.cpp | 7 ++++ 26 files changed, 51 insertions(+), 81 deletions(-) diff --git a/include/InstrumentPlayHandle.h b/include/InstrumentPlayHandle.h index bbf53d16c..1455134e6 100644 --- a/include/InstrumentPlayHandle.h +++ b/include/InstrumentPlayHandle.h @@ -26,13 +26,14 @@ #define LMMS_INSTRUMENT_PLAY_HANDLE_H #include "PlayHandle.h" -#include "Instrument.h" -#include "NotePlayHandle.h" #include "lmms_export.h" namespace lmms { +class Instrument; +class InstrumentTrack; + class LMMS_EXPORT InstrumentPlayHandle : public PlayHandle { public: @@ -40,46 +41,17 @@ public: ~InstrumentPlayHandle() override = default; - - void play( sampleFrame * _working_buffer ) override - { - // ensure that all our nph's have been processed first - ConstNotePlayHandleList nphv = NotePlayHandle::nphsOfInstrumentTrack( m_instrument->instrumentTrack(), true ); - - bool nphsLeft; - do - { - nphsLeft = false; - for( const NotePlayHandle * constNotePlayHandle : nphv ) - { - NotePlayHandle * notePlayHandle = const_cast( constNotePlayHandle ); - if( notePlayHandle->state() != ThreadableJob::ProcessingState::Done && - !notePlayHandle->isFinished()) - { - nphsLeft = true; - notePlayHandle->process(); - } - } - } - while( nphsLeft ); - - m_instrument->play( _working_buffer ); - } + void play( sampleFrame * _working_buffer ) override; bool isFinished() const override { return false; } - bool isFromTrack( const Track* _track ) const override - { - return m_instrument->isFromTrack( _track ); - } - + bool isFromTrack( const Track* _track ) const override; private: Instrument* m_instrument; - } ; diff --git a/plugins/AudioFileProcessor/AudioFileProcessor.cpp b/plugins/AudioFileProcessor/AudioFileProcessor.cpp index a941e773f..667102207 100644 --- a/plugins/AudioFileProcessor/AudioFileProcessor.cpp +++ b/plugins/AudioFileProcessor/AudioFileProcessor.cpp @@ -171,9 +171,6 @@ void AudioFileProcessor::playNote( NotePlayHandle * _n, static_cast( m_loopModel.value() ) ) ) { applyRelease( _working_buffer, _n ); - instrumentTrack()->processAudioBuffer( _working_buffer, - frames + offset, _n ); - emit isPlaying( ((handleState *)_n->m_pluginData)->frameIndex() ); } else diff --git a/plugins/BitInvader/BitInvader.cpp b/plugins/BitInvader/BitInvader.cpp index 98ef1e97c..4ea73dc71 100644 --- a/plugins/BitInvader/BitInvader.cpp +++ b/plugins/BitInvader/BitInvader.cpp @@ -307,8 +307,6 @@ void BitInvader::playNote( NotePlayHandle * _n, } applyRelease( _working_buffer, _n ); - - instrumentTrack()->processAudioBuffer( _working_buffer, frames + offset, _n ); } diff --git a/plugins/CarlaBase/Carla.cpp b/plugins/CarlaBase/Carla.cpp index faff94b57..819736e92 100644 --- a/plugins/CarlaBase/Carla.cpp +++ b/plugins/CarlaBase/Carla.cpp @@ -508,7 +508,6 @@ void CarlaInstrument::play(sampleFrame* workingBuffer) if (fHandle == nullptr) { - instrumentTrack()->processAudioBuffer(workingBuffer, bufsize, nullptr); return; } @@ -556,8 +555,6 @@ void CarlaInstrument::play(sampleFrame* workingBuffer) workingBuffer[i][0] = buf1[i]; workingBuffer[i][1] = buf2[i]; } - - instrumentTrack()->processAudioBuffer(workingBuffer, bufsize, nullptr); } bool CarlaInstrument::handleMidiEvent(const MidiEvent& event, const TimePos&, f_cnt_t offset) diff --git a/plugins/FreeBoy/FreeBoy.cpp b/plugins/FreeBoy/FreeBoy.cpp index 6450253ee..f2dc95699 100644 --- a/plugins/FreeBoy/FreeBoy.cpp +++ b/plugins/FreeBoy/FreeBoy.cpp @@ -419,7 +419,6 @@ void FreeBoyInstrument::playNote(NotePlayHandle* nph, sampleFrame* workingBuffer } framesLeft -= count; } - instrumentTrack()->processAudioBuffer(workingBuffer, frames + offset, nph); } diff --git a/plugins/GigPlayer/GigPlayer.cpp b/plugins/GigPlayer/GigPlayer.cpp index c2e155a20..0713d3100 100644 --- a/plugins/GigPlayer/GigPlayer.cpp +++ b/plugins/GigPlayer/GigPlayer.cpp @@ -494,8 +494,6 @@ void GigInstrument::play( sampleFrame * _working_buffer ) _working_buffer[i][0] *= m_gain.value(); _working_buffer[i][1] *= m_gain.value(); } - - instrumentTrack()->processAudioBuffer( _working_buffer, frames, nullptr ); } diff --git a/plugins/Kicker/Kicker.cpp b/plugins/Kicker/Kicker.cpp index ef1d623c1..e6418e2da 100644 --- a/plugins/Kicker/Kicker.cpp +++ b/plugins/Kicker/Kicker.cpp @@ -197,8 +197,6 @@ void KickerInstrument::playNote( NotePlayHandle * _n, _working_buffer[f+offset][1] *= fac; } } - - instrumentTrack()->processAudioBuffer( _working_buffer, frames + offset, _n ); } diff --git a/plugins/Lb302/Lb302.cpp b/plugins/Lb302/Lb302.cpp index b8fff2c0b..ee49442d5 100644 --- a/plugins/Lb302/Lb302.cpp +++ b/plugins/Lb302/Lb302.cpp @@ -790,7 +790,6 @@ void Lb302Synth::play( sampleFrame * _working_buffer ) const fpp_t frames = Engine::audioEngine()->framesPerPeriod(); process( _working_buffer, frames ); - instrumentTrack()->processAudioBuffer( _working_buffer, frames, nullptr ); // release_frame = 0; //removed for issue # 1432 } diff --git a/plugins/Lv2Instrument/Lv2Instrument.cpp b/plugins/Lv2Instrument/Lv2Instrument.cpp index 1e45f4e91..32f81d23c 100644 --- a/plugins/Lv2Instrument/Lv2Instrument.cpp +++ b/plugins/Lv2Instrument/Lv2Instrument.cpp @@ -197,8 +197,6 @@ void Lv2Instrument::play(sampleFrame *buf) copyModelsToLmms(); copyBuffersToLmms(buf, fpp); - - instrumentTrack()->processAudioBuffer(buf, fpp, nullptr); } diff --git a/plugins/Monstro/Monstro.cpp b/plugins/Monstro/Monstro.cpp index f588d6b78..2201e4ed9 100644 --- a/plugins/Monstro/Monstro.cpp +++ b/plugins/Monstro/Monstro.cpp @@ -1040,8 +1040,6 @@ void MonstroInstrument::playNote( NotePlayHandle * _n, ms->renderOutput( frames, _working_buffer + offset ); //applyRelease( _working_buffer, _n ); // we have our own release - - instrumentTrack()->processAudioBuffer( _working_buffer, frames + offset, _n ); } void MonstroInstrument::deleteNotePluginData( NotePlayHandle * _n ) diff --git a/plugins/Nes/Nes.cpp b/plugins/Nes/Nes.cpp index a530ac19b..47122a0c6 100644 --- a/plugins/Nes/Nes.cpp +++ b/plugins/Nes/Nes.cpp @@ -561,8 +561,6 @@ void NesInstrument::playNote( NotePlayHandle * n, sampleFrame * workingBuffer ) nes->renderOutput( workingBuffer + offset, frames ); applyRelease( workingBuffer, n ); - - instrumentTrack()->processAudioBuffer( workingBuffer, frames + offset, n ); } diff --git a/plugins/OpulenZ/OpulenZ.cpp b/plugins/OpulenZ/OpulenZ.cpp index 64f609995..d90d5f343 100644 --- a/plugins/OpulenZ/OpulenZ.cpp +++ b/plugins/OpulenZ/OpulenZ.cpp @@ -412,10 +412,6 @@ void OpulenzInstrument::play( sampleFrame * _working_buffer ) } } emulatorMutex.unlock(); - - // Throw the data to the track... - instrumentTrack()->processAudioBuffer( _working_buffer, frameCount, nullptr ); - } diff --git a/plugins/Organic/Organic.cpp b/plugins/Organic/Organic.cpp index f8a2b0d13..a70da6421 100644 --- a/plugins/Organic/Organic.cpp +++ b/plugins/Organic/Organic.cpp @@ -312,8 +312,6 @@ void OrganicInstrument::playNote( NotePlayHandle * _n, } // -- -- - - instrumentTrack()->processAudioBuffer( _working_buffer, frames + offset, _n ); } diff --git a/plugins/Patman/Patman.cpp b/plugins/Patman/Patman.cpp index a2b829940..668c7f461 100644 --- a/plugins/Patman/Patman.cpp +++ b/plugins/Patman/Patman.cpp @@ -157,8 +157,6 @@ void PatmanInstrument::playNote( NotePlayHandle * _n, play_freq, m_loopedModel.value() ? SampleBuffer::LoopMode::On : SampleBuffer::LoopMode::Off ) ) { applyRelease( _working_buffer, _n ); - instrumentTrack()->processAudioBuffer( _working_buffer, - frames + offset, _n ); } else { diff --git a/plugins/Sf2Player/Sf2Player.cpp b/plugins/Sf2Player/Sf2Player.cpp index d46af5e4f..79bd4b976 100644 --- a/plugins/Sf2Player/Sf2Player.cpp +++ b/plugins/Sf2Player/Sf2Player.cpp @@ -848,7 +848,6 @@ void Sf2Instrument::play( sampleFrame * _working_buffer ) if( m_playingNotes.isEmpty() ) { renderFrames( frames, _working_buffer ); - instrumentTrack()->processAudioBuffer( _working_buffer, frames, nullptr ); return; } @@ -906,7 +905,6 @@ void Sf2Instrument::play( sampleFrame * _working_buffer ) { renderFrames( frames - currentFrame, _working_buffer + currentFrame ); } - instrumentTrack()->processAudioBuffer( _working_buffer, frames, nullptr ); } diff --git a/plugins/Sfxr/Sfxr.cpp b/plugins/Sfxr/Sfxr.cpp index fc39ea0fa..e79b8e2ad 100644 --- a/plugins/Sfxr/Sfxr.cpp +++ b/plugins/Sfxr/Sfxr.cpp @@ -480,9 +480,6 @@ void SfxrInstrument::playNote( NotePlayHandle * _n, sampleFrame * _working_buffe delete[] pitchedBuffer; applyRelease( _working_buffer, _n ); - - instrumentTrack()->processAudioBuffer( _working_buffer, frameNum + offset, _n ); - } diff --git a/plugins/Sid/SidInstrument.cpp b/plugins/Sid/SidInstrument.cpp index f663c3b69..143003f98 100644 --- a/plugins/Sid/SidInstrument.cpp +++ b/plugins/Sid/SidInstrument.cpp @@ -429,8 +429,6 @@ void SidInstrument::playNote( NotePlayHandle * _n, _working_buffer[frame+offset][ch] = s; } } - - instrumentTrack()->processAudioBuffer( _working_buffer, frames + offset, _n ); } diff --git a/plugins/Stk/Mallets/Mallets.cpp b/plugins/Stk/Mallets/Mallets.cpp index 4fb077de5..b746e9491 100644 --- a/plugins/Stk/Mallets/Mallets.cpp +++ b/plugins/Stk/Mallets/Mallets.cpp @@ -359,8 +359,6 @@ void MalletsInstrument::playNote( NotePlayHandle * _n, _working_buffer[frame][1] = ps->nextSampleRight() * ( m_scalers[p] + add_scale ); } - - instrumentTrack()->processAudioBuffer( _working_buffer, frames + offset, _n ); } diff --git a/plugins/TripleOscillator/TripleOscillator.cpp b/plugins/TripleOscillator/TripleOscillator.cpp index f2340d3d6..5b8f6e8ad 100644 --- a/plugins/TripleOscillator/TripleOscillator.cpp +++ b/plugins/TripleOscillator/TripleOscillator.cpp @@ -380,8 +380,6 @@ void TripleOscillator::playNote( NotePlayHandle * _n, applyFadeIn(_working_buffer, _n); applyRelease( _working_buffer, _n ); - - instrumentTrack()->processAudioBuffer( _working_buffer, frames + offset, _n ); } diff --git a/plugins/Vestige/Vestige.cpp b/plugins/Vestige/Vestige.cpp index dd8e9cbef..aff5b7fdc 100644 --- a/plugins/Vestige/Vestige.cpp +++ b/plugins/Vestige/Vestige.cpp @@ -409,8 +409,6 @@ void VestigeInstrument::play( sampleFrame * _buf ) m_plugin->process( nullptr, _buf ); - instrumentTrack()->processAudioBuffer( _buf, frames, nullptr ); - m_pluginMutex.unlock(); } diff --git a/plugins/Vibed/Vibed.cpp b/plugins/Vibed/Vibed.cpp index 3ed51fe79..ad6a3942a 100644 --- a/plugins/Vibed/Vibed.cpp +++ b/plugins/Vibed/Vibed.cpp @@ -251,8 +251,6 @@ void Vibed::playNote(NotePlayHandle* n, sampleFrame* workingBuffer) } } } - - instrumentTrack()->processAudioBuffer(workingBuffer, frames + offset, n); } void Vibed::deleteNotePluginData(NotePlayHandle* n) diff --git a/plugins/Watsyn/Watsyn.cpp b/plugins/Watsyn/Watsyn.cpp index 7603a9c1b..8e49942e1 100644 --- a/plugins/Watsyn/Watsyn.cpp +++ b/plugins/Watsyn/Watsyn.cpp @@ -445,8 +445,6 @@ void WatsynInstrument::playNote( NotePlayHandle * _n, } applyRelease( _working_buffer, _n ); - - instrumentTrack()->processAudioBuffer( _working_buffer, frames + offset, _n ); } diff --git a/plugins/Xpressive/Xpressive.cpp b/plugins/Xpressive/Xpressive.cpp index b1a17a1ce..babc37231 100644 --- a/plugins/Xpressive/Xpressive.cpp +++ b/plugins/Xpressive/Xpressive.cpp @@ -233,8 +233,6 @@ void Xpressive::playNote(NotePlayHandle* nph, sampleFrame* working_buffer) { const f_cnt_t offset = nph->noteOffset(); ps->renderOutput(frames, working_buffer + offset); - - instrumentTrack()->processAudioBuffer(working_buffer, frames + offset, nph); } void Xpressive::deleteNotePluginData(NotePlayHandle* nph) { diff --git a/plugins/ZynAddSubFx/ZynAddSubFx.cpp b/plugins/ZynAddSubFx/ZynAddSubFx.cpp index 2ec864592..be38bcb79 100644 --- a/plugins/ZynAddSubFx/ZynAddSubFx.cpp +++ b/plugins/ZynAddSubFx/ZynAddSubFx.cpp @@ -341,7 +341,6 @@ void ZynAddSubFxInstrument::play( sampleFrame * _buf ) m_plugin->processAudio( _buf ); } m_pluginMutex.unlock(); - instrumentTrack()->processAudioBuffer( _buf, Engine::audioEngine()->framesPerPeriod(), nullptr ); } diff --git a/src/core/InstrumentPlayHandle.cpp b/src/core/InstrumentPlayHandle.cpp index e1a9d9d65..30f5fde38 100644 --- a/src/core/InstrumentPlayHandle.cpp +++ b/src/core/InstrumentPlayHandle.cpp @@ -24,7 +24,10 @@ #include "InstrumentPlayHandle.h" +#include "Instrument.h" #include "InstrumentTrack.h" +#include "Engine.h" +#include "AudioEngine.h" namespace lmms { @@ -37,5 +40,41 @@ InstrumentPlayHandle::InstrumentPlayHandle( Instrument * instrument, InstrumentT setAudioPort( instrumentTrack->audioPort() ); } +void InstrumentPlayHandle::play( sampleFrame * _working_buffer ) +{ + InstrumentTrack * instrumentTrack = m_instrument->instrumentTrack(); + + // ensure that all our nph's have been processed first + ConstNotePlayHandleList nphv = NotePlayHandle::nphsOfInstrumentTrack(instrumentTrack, true ); + + bool nphsLeft; + do + { + nphsLeft = false; + for( const NotePlayHandle * constNotePlayHandle : nphv ) + { + NotePlayHandle * notePlayHandle = const_cast( constNotePlayHandle ); + if( notePlayHandle->state() != ThreadableJob::ProcessingState::Done && + !notePlayHandle->isFinished()) + { + nphsLeft = true; + notePlayHandle->process(); + } + } + } + while( nphsLeft ); + + m_instrument->play( _working_buffer ); + + // Process the audio buffer that the instrument has just worked on... + const fpp_t frames = Engine::audioEngine()->framesPerPeriod(); + instrumentTrack->processAudioBuffer(_working_buffer, frames, nullptr); +} + +bool InstrumentPlayHandle::isFromTrack( const Track* _track ) const +{ + return m_instrument->isFromTrack( _track ); +} + } // namespace lmms \ No newline at end of file diff --git a/src/tracks/InstrumentTrack.cpp b/src/tracks/InstrumentTrack.cpp index a4de188a5..c9b87a4f2 100644 --- a/src/tracks/InstrumentTrack.cpp +++ b/src/tracks/InstrumentTrack.cpp @@ -579,6 +579,13 @@ void InstrumentTrack::playNote( NotePlayHandle* n, sampleFrame* workingBuffer ) { // all is done, so now lets play the note! m_instrument->playNote( n, workingBuffer ); + + if (workingBuffer != nullptr) + { + const fpp_t frames = n->framesLeftForCurrentPeriod(); + const f_cnt_t offset = n->noteOffset(); + this->processAudioBuffer(workingBuffer, frames + offset, n); + } } } From a6b6565687d658350fe84b448676ace5a1c466c7 Mon Sep 17 00:00:00 2001 From: Michael Gregorius Date: Thu, 14 Sep 2023 23:35:16 +0200 Subject: [PATCH 2/5] Remove unused variable Remove the unused variable `frames` in `VestigeInstrument::play` to fix the stricter GitHub build. --- plugins/Vestige/Vestige.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/plugins/Vestige/Vestige.cpp b/plugins/Vestige/Vestige.cpp index aff5b7fdc..a696a4b2d 100644 --- a/plugins/Vestige/Vestige.cpp +++ b/plugins/Vestige/Vestige.cpp @@ -399,8 +399,6 @@ void VestigeInstrument::play( sampleFrame * _buf ) { if (!m_pluginMutex.tryLock(Engine::getSong()->isExporting() ? -1 : 0)) {return;} - const fpp_t frames = Engine::audioEngine()->framesPerPeriod(); - if( m_plugin == nullptr ) { m_pluginMutex.unlock(); From 73f9f36c9afcf65150ae8b1f47d359f2472d3eae Mon Sep 17 00:00:00 2001 From: Michael Gregorius Date: Sat, 16 Sep 2023 13:35:15 +0200 Subject: [PATCH 3/5] Code review changes Remove whitespace in methods and add it to the parenthesis of some statements. Remove underscores from parameter names. Remove usage of `this` pointer. Add `auto` keyword to shorten line length in `InstrumentPlayHandle::play`. Move `const_cast` of `NotePlayHandle` closer to the `process` method as this method is the reason that the cast is needed in the first place. One might also add a version of `nphsOfInstrumentTrack` that returns a non-const result but it seems to be a general problem of a missing clear responsibility so I keep it as is. --- include/InstrumentPlayHandle.h | 9 ++++----- src/core/InstrumentPlayHandle.cpp | 32 +++++++++++++++---------------- src/tracks/InstrumentTrack.cpp | 2 +- 3 files changed, 21 insertions(+), 22 deletions(-) diff --git a/include/InstrumentPlayHandle.h b/include/InstrumentPlayHandle.h index 1455134e6..dc744b4ff 100644 --- a/include/InstrumentPlayHandle.h +++ b/include/InstrumentPlayHandle.h @@ -37,23 +37,22 @@ class InstrumentTrack; class LMMS_EXPORT InstrumentPlayHandle : public PlayHandle { public: - InstrumentPlayHandle( Instrument * instrument, InstrumentTrack* instrumentTrack ); + InstrumentPlayHandle(Instrument * instrument, InstrumentTrack* instrumentTrack); ~InstrumentPlayHandle() override = default; - void play( sampleFrame * _working_buffer ) override; + void play(sampleFrame * working_buffer) override; bool isFinished() const override { return false; } - bool isFromTrack( const Track* _track ) const override; + bool isFromTrack(const Track* track) const override; private: Instrument* m_instrument; -} ; - +}; } // namespace lmms diff --git a/src/core/InstrumentPlayHandle.cpp b/src/core/InstrumentPlayHandle.cpp index 30f5fde38..097719ad8 100644 --- a/src/core/InstrumentPlayHandle.cpp +++ b/src/core/InstrumentPlayHandle.cpp @@ -33,48 +33,48 @@ namespace lmms { -InstrumentPlayHandle::InstrumentPlayHandle( Instrument * instrument, InstrumentTrack* instrumentTrack ) : - PlayHandle( Type::InstrumentPlayHandle ), - m_instrument( instrument ) +InstrumentPlayHandle::InstrumentPlayHandle(Instrument * instrument, InstrumentTrack* instrumentTrack) : + PlayHandle(Type::InstrumentPlayHandle), + m_instrument(instrument) { - setAudioPort( instrumentTrack->audioPort() ); + setAudioPort(instrumentTrack->audioPort()); } -void InstrumentPlayHandle::play( sampleFrame * _working_buffer ) +void InstrumentPlayHandle::play(sampleFrame * working_buffer) { InstrumentTrack * instrumentTrack = m_instrument->instrumentTrack(); // ensure that all our nph's have been processed first - ConstNotePlayHandleList nphv = NotePlayHandle::nphsOfInstrumentTrack(instrumentTrack, true ); + auto nphv = NotePlayHandle::nphsOfInstrumentTrack(instrumentTrack, true); bool nphsLeft; do { nphsLeft = false; - for( const NotePlayHandle * constNotePlayHandle : nphv ) + for (const NotePlayHandle * constNotePlayHandle : nphv) { - NotePlayHandle * notePlayHandle = const_cast( constNotePlayHandle ); - if( notePlayHandle->state() != ThreadableJob::ProcessingState::Done && - !notePlayHandle->isFinished()) + if (constNotePlayHandle->state() != ThreadableJob::ProcessingState::Done && + !constNotePlayHandle->isFinished()) { nphsLeft = true; + NotePlayHandle * notePlayHandle = const_cast(constNotePlayHandle); notePlayHandle->process(); } } } - while( nphsLeft ); + while (nphsLeft); - m_instrument->play( _working_buffer ); + m_instrument->play(working_buffer); // Process the audio buffer that the instrument has just worked on... const fpp_t frames = Engine::audioEngine()->framesPerPeriod(); - instrumentTrack->processAudioBuffer(_working_buffer, frames, nullptr); + instrumentTrack->processAudioBuffer(working_buffer, frames, nullptr); } -bool InstrumentPlayHandle::isFromTrack( const Track* _track ) const +bool InstrumentPlayHandle::isFromTrack(const Track* track) const { - return m_instrument->isFromTrack( _track ); + return m_instrument->isFromTrack(track); } -} // namespace lmms \ No newline at end of file +} // namespace lmms diff --git a/src/tracks/InstrumentTrack.cpp b/src/tracks/InstrumentTrack.cpp index c9b87a4f2..0ffde61bc 100644 --- a/src/tracks/InstrumentTrack.cpp +++ b/src/tracks/InstrumentTrack.cpp @@ -584,7 +584,7 @@ void InstrumentTrack::playNote( NotePlayHandle* n, sampleFrame* workingBuffer ) { const fpp_t frames = n->framesLeftForCurrentPeriod(); const f_cnt_t offset = n->noteOffset(); - this->processAudioBuffer(workingBuffer, frames + offset, n); + processAudioBuffer(workingBuffer, frames + offset, n); } } } From 6c3ae30c8937173bd69ec3f12ccd5996189c3342 Mon Sep 17 00:00:00 2001 From: Michael Gregorius Date: Sat, 16 Sep 2023 19:19:37 +0200 Subject: [PATCH 4/5] Add comments about nullptr guard Add some comments which describe why we need to guard agains nullptr. --- src/tracks/InstrumentTrack.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/tracks/InstrumentTrack.cpp b/src/tracks/InstrumentTrack.cpp index 0ffde61bc..5dee1f3a5 100644 --- a/src/tracks/InstrumentTrack.cpp +++ b/src/tracks/InstrumentTrack.cpp @@ -570,6 +570,10 @@ f_cnt_t InstrumentTrack::beatLen( NotePlayHandle * _n ) const void InstrumentTrack::playNote( NotePlayHandle* n, sampleFrame* workingBuffer ) { + // Note: under certain circumstances the working buffer is a nullptr. + // These cases are triggered in PlayHandle::doProcessing when the play method is called with a nullptr. + // TODO: Find out if we can skip processing at a higher level if the buffer is nullptr. + // arpeggio- and chord-widget has to do its work -> adding sub-notes // for chords/arpeggios m_noteStacking.processNote( n ); @@ -580,6 +584,8 @@ void InstrumentTrack::playNote( NotePlayHandle* n, sampleFrame* workingBuffer ) // all is done, so now lets play the note! m_instrument->playNote( n, workingBuffer ); + // Calling processAudioBuffer with a nullptr leads to crashes when checking if the buffer represents silence. + // Therefore we must guard against a nullptr here. if (workingBuffer != nullptr) { const fpp_t frames = n->framesLeftForCurrentPeriod(); From b6c80bd7366a789cab3f5a6a017636faf80a95ee Mon Sep 17 00:00:00 2001 From: Michael Gregorius Date: Thu, 21 Sep 2023 19:24:06 +0200 Subject: [PATCH 5/5] Replace check for nullptr Replace the check for a `nullptr` buffer with a check that checks if the NotePlayHandle uses a buffer. This is effectively the same condition that's used by `PlayHandle::doProcessing` to decide if it should call play with a `nullptr` in the first place. Hence it should be the most fitting check. --- src/tracks/InstrumentTrack.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/tracks/InstrumentTrack.cpp b/src/tracks/InstrumentTrack.cpp index 5dee1f3a5..29fda075e 100644 --- a/src/tracks/InstrumentTrack.cpp +++ b/src/tracks/InstrumentTrack.cpp @@ -584,9 +584,9 @@ void InstrumentTrack::playNote( NotePlayHandle* n, sampleFrame* workingBuffer ) // all is done, so now lets play the note! m_instrument->playNote( n, workingBuffer ); - // Calling processAudioBuffer with a nullptr leads to crashes when checking if the buffer represents silence. - // Therefore we must guard against a nullptr here. - if (workingBuffer != nullptr) + // This is effectively the same as checking if workingBuffer is not a nullptr. + // Calling processAudioBuffer with a nullptr leads to crashes. Hence the check. + if (n->usesBuffer()) { const fpp_t frames = n->framesLeftForCurrentPeriod(); const f_cnt_t offset = n->noteOffset();