Lb302 cleanup

[rubiefawn]

Makes the following changes to the Lb302 plugin for increased maintainability and readability:

• Use non-static data member initializers where possible
• Use std::numbers::pi instead of M_PI
• Use std::array and std::unique_ptr instead of manual memory management
• Change plain-old-data classes with only public members to structs
• Change some ints to f_cnt_t, fpp_t, etc. to better communicate their purpose and avoid weird implicit casting nonsense
• Change floats to sample_t where appropriate to better communicate their purpose
• Find suitable homes for loose constants
• Prefix standard library math function calls with std::
• Ensure all floating-point literals intended to be floats end in f and are not doubles to appease MSVC
• Change uses of SIGNAL and SLOT macros to whatever they should be post-Qt6 upgrade
• Remove cursed mutex in favor of a real-time safe queue (TODO: possible generalization for Monophonic mode on instrument plugins; portamento #6516?)
• Optimize struct/class member layout for better locality
• Completely conform to current code conventions

The above list is not comprehensive and is destined to expand.

[rubiefawn]

Figured I'd run some un-rigorous perf stat benchmarks of release builds as a sanity check to make sure I wasn't destroying performance by changing the mutex for a bunch of atomic operations and busy-wait loops.

Updated for commit 03cca1b

Before, with mutex/dynamically allocated vector:

Performance counter stats for './lmms render /home/fawn/Documents/lmms/projects/torture.mmpz -a -f flac':

   103,084,983,781      task-clock                       #    2.979 CPUs utilized             
           695,800      context-switches                 #    6.750 K/sec                     
               388      cpu-migrations                   #    3.764 /sec                      
             6,173      page-faults                      #   59.883 /sec                      
   267,717,430,944      instructions                     #    1.17  insn per cycle            
   229,289,575,609      cycles                           #    2.224 GHz                       
    61,653,273,269      branches                         #  598.082 M/sec                     
       496,591,833      branch-misses                    #    0.81% of all branches           

      34.602878156 seconds time elapsed

      93.334079000 seconds user
       9.660192000 seconds sys

After, with atomic ringbuffer/statically allocated array:

Performance counter stats for './lmms render /home/fawn/Documents/lmms/projects/torture.mmpz -a -f flac':

   104,963,456,247      task-clock                       #    3.066 CPUs utilized             
           708,920      context-switches                 #    6.754 K/sec                     
                67      cpu-migrations                   #    0.638 /sec                      
             4,237      page-faults                      #   40.366 /sec                      
   267,503,252,259      instructions                     #    1.15  insn per cycle            
   233,160,958,544      cycles                           #    2.221 GHz                       
    61,493,266,240      branches                         #  585.854 M/sec                     
       511,104,882      branch-misses                    #    0.83% of all branches           

      34.239441489 seconds time elapsed

      95.604870000 seconds user
       9.199340000 seconds sys

Project file used for said benchmarks: torture.mmp.xml

[rubiefawn]

CACHELINE and busy_wait_hint probably don't belong inside Lb302. Where is the best place for me to move these to?

[messmerd]

CACHELINE and busy_wait_hint probably don't belong inside Lb302. Where is the best place for me to move these to?

Maybe add them to a new LmmsPolyfill.h header. And rename CACHELINE to lmms::hardware_destructive_interference_size to match the name the standard library uses.

[messmerd]

I would use the host architecture macros from lmmsconfig.h instead

[rubiefawn]

Mostly resolved as of 6fccf95 (MSVC is a pain in the ass as usual)

[messmerd]

Same here

[rubiefawn]

Ugh, I just noticed a bug I must have introduced in this PR with slide behavior. Notes continue to slide even after the toggle is disabled, but the slide-from note is whatever the last note was before the toggle was disabled.

Edit: Bug introduced in 1b563ed, is fixed by 0fc7d88. Hooray for git bisect

[bratpeki]

Why do this?

[rubiefawn]

GCC complains and I need it to not do that

[rubiefawn]

@sakertooth from #8280:

Since the actual processing only gets ran in Lb302::play (called by InstrumentPlayHandle::play), and Lb302::playNote does nothing but just track the notes, I wonder if you can remove m_notes and just pass the list of notes within Lb302::play as a parameter, and move the release_frame calculation into Lb302Synth::play as well (or maybe just leave it there if its needed earlier than when the play function will be called).

Its really odd to me. The per-note processing occurs in processNote, which only happens after the InstrumentPlayHandle processes all the notes.

Replying here since discussion of this PR belongs in this PR.

Since the actual processing only gets ran in Lb302::play (called by InstrumentPlayHandle::play), and Lb302::playNote does nothing but just track the notes, I wonder if you can remove m_notes and just pass the list of notes within Lb302::play as a parameter,

Lb302Synth::play() and Lb302Synth::playNote() are overrides of Instrument::play() and Instrument::playNote(), respectively. Changing these would require a refactor of everything that inherits Instrument. I don't understand this suggestion.

// Instrument.h:76
	// if the plugin doesn't play each note, it can create an instrument-
	// play-handle and re-implement this method, so that it mixes its
	// output buffer only once per audio engine period
	virtual void play( SampleFrame* _working_buffer );

	// to be implemented by actual plugin
	virtual void playNote( NotePlayHandle * /* _note_to_play */,
					SampleFrame* /* _working_buf */ )
	{
	}

and move the release_frame calculation into Lb302Synth::play as well (or maybe just leave it there if its needed earlier than when the play function will be called).

This is a good suggestion. As of 6aec972, I've moved the release_frame calculation into Lb302Synth::processNote(), which is a helper only ever called from inside Lb302Synth::play(). Now that it is only ever read or written to by one thread, I've made it a normal non-atomic f_cnt_t.

[sakertooth]

Lb302Synth::play() and Lb302Synth::playNote() are overrides of Instrument::play() and Instrument::playNote(), respectively. Changing these would require a refactor of everything that inherits Instrument. I don't understand this suggestion.

Each call to Lb302Synth::playNote doesn't actually do any per note processing. It just adds it to a internal list of NotePlayHandle objects. But the InstrumentPlayHandle associated with the instrument already keeps track of all the notes that were supposed to run for the given track and then calls Instrument::play.

My idea was that maybe you can remove the m_notes list in Lb302Synth, and pass the list of notes that the InstrumentPlayHandle already manages into Lb302Synth::play as an extra parameter. The other instruments wont be impacted (though there function signatures might change) because you can just pass a empty list as a default parameter. The other instruments will just ignore that extra parameter while Lb302Synth is the start of where this migration will happen. The only difference is that the handle list managed by InstrumentPlayHandle may not respect the ordering requirements of the handles themselves (i.e., NotePlayHandle objects where totalFramesPlayed() == 0 should be played last)

This is somewhat hard to explain (and also might not work), but this is just a idea I had that sprung into my mind to remove that seemingly redundant m_notes list in Lb302Synth. Consider this mostly as a thought experiment, not something you have to implement or anything.

---

void InstrumentPlayHandle::play(SampleFrame* working_buffer)
{
	InstrumentTrack * instrumentTrack = m_instrument->instrumentTrack();

	// ensure that all our nph's have been processed first
	auto nphv = NotePlayHandle::nphsOfInstrumentTrack(instrumentTrack, true);

	bool nphsLeft;
	do
	{
		nphsLeft = false;
		for (const auto& handle : nphv)
		{
			if (handle->state() != ThreadableJob::ProcessingState::Done && !handle->isFinished())
			{
				nphsLeft = true;
				const_cast<NotePlayHandle*>(handle)->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);
}

nphv already keeps track of all the notes that were meant to be played by this instrument, so I was confused why in Lb302Synth there is a m_notes list with Lb302Synth::playNote doing the same thing but just putting the notes in a different order.

[sakertooth]

There's probably a better approach but would probably mean a larger refactor, but its rather plausible to think that the m_notes list in Lb302Synth (and probably the other instruments) is redundant tracking.

[rubiefawn]

But the InstrumentPlayHandle associated with the instrument already keeps track of all the notes that were supposed to run for the given track and then calls Instrument::play.

I wasn't aware of this! That's good to know.

The queue (m_notes) is merely a safer replacement for the old behavior, which was a QList protected by a mutex (yuck!!). Getting rid of the mutex was one of the main motivators for making this PR. I assumed there wasn't an existing way to keep track of notes and that I should just make the current method safer, since if there were, Lb302 would already be using it, right? ... right? 😱

If this can be done without m_notes that sounds like an even better upgrade. I'm still iffy on the idea of changing program-wide function signatures to accommodate one plugin, but there might be a way to avoid that too. I'll take a look at InstrumentPlayHandle.

Edit:

nphsOfInstrumentTrack() gets a list of every single NotePlayHandle, filters them by checking if they belong to the instrumentTrack parameter, and pushes them to a QList. It doesn't actually keep track of which NotePlayHandle belongs where, and use of QList is sketchy because of dynamic allocations in what is supposed to be a realtime audio callback function. That's pretty bad.

To fix this, I'd want to refactor nphsOfInstrumentTrack() or even InstrumentPlayHandle. That's beyond the scope of this PR.

[sakertooth]

I'm still iffy on the idea of changing program-wide function signatures to accommodate one plugin, but there might be a way to avoid that too.

Yeah I would recommend not doing anything like this here (might not work and also blurs the responsibilities of this PR), but its something to think about.

We might consider also removing InstrumentPlayHandle in the future (among some of the others since they don't do any actual processing on their own, like what is PresetPreviewPlayHandle even doing here?) after figuring out a more correct design, so the proposed idea may not be applicable.