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 bbf53d16c21..1455134e640 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 @@ class LMMS_EXPORT InstrumentPlayHandle : public PlayHandle ~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 a941e773f25..6671022070c 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 98ef1e97cf1..4ea73dc7116 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 faff94b570b..819736e928b 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 6450253ee45..f2dc95699ed 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 c2e155a20c5..0713d310038 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 ef1d623c1a1..e6418e2da5b 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 b8fff2c0b8a..ee49442d5d4 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 1e45f4e919e..32f81d23c25 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 f588d6b786d..2201e4ed90b 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 a530ac19b3b..47122a0c602 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 64f60999576..d90d5f343a4 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 f8a2b0d135c..a70da642156 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 a2b829940a4..668c7f461e9 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 d46af5e4f6e..79bd4b97686 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 fc39ea0fa28..e79b8e2adbe 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 f663c3b6977..143003f9803 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 4fb077de5bc..b746e949120 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 f2340d3d609..5b8f6e8ade6 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 dd8e9cbef3b..aff5b7fdc46 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 3ed51fe79d7..ad6a3942af4 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 7603a9c1be6..8e49942e16a 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 b1a17a1ce76..babc372317a 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 2ec86459281..be38bcb7978 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 e1a9d9d65fd..30f5fde38a1 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 a4de188a5d1..c9b87a4f2ed 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 aff5b7fdc46..a696a4b2ded 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 1455134e640..dc744b4ffdb 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 30f5fde38a1..097719ad83d 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 c9b87a4f2ed..0ffde61bcea 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 0ffde61bcea..5dee1f3a537 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 5dee1f3a537..29fda075e9c 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();