Refactor / Cleanup

[sevonj]

A separate thread to continue discussing cleaning up unused variables and other improvements to the code. This isn't a specific task or a set of tasks that can be completed, so it may be better to convert this issue into a discussion instead.

It's better to discuss ideas before actually building them.

List:

• ModulationEnvelope::Process() returns a bool that's never used. Remove return value.
• VolumeEnvelope::Process() returns a bool that is used, but it's hard to understand what for. This should probably be documented, or even better, delivered in a way that's obvious just from reading the code.
• Message::common1() and Message::common2() look like they can be merged. In each use, behavior of common1() is identical to calling common2() with 0 as the second param.
• This loop type mapping logic in common2() would make more sense under impl MidiFileLoopType.
  #43
• It would be great if the repository contained the files required to run tests.
  • This would make continuous integration possible; test results automatically shown in the pull request.
• Moving tests to rustysynth crate would give access to private types and fields and therefore allow building better tests.
• TinyAudio example can be moved from separate branch to example_tinyaudio/ on main branch.
• ~~Voice's sample_rate is unused...wonder if that's an issue.~~.
• Weird differences between mod and vol envelopes

[dsgallups]

https://github.com/dsgallups/midix/pull/39/commits/b04e0cb9f6ab9e4878d59542dc3ac1ed3fe8a0ee this commit could be useful

edit: there's more to it here. This might be a good testing ground to scope out different ideas. only difference prior to this linked PR is reorganization of files

[dsgallups]

• RegionPair::get_start_address_offset, get_end_address_offset, get_start_loop_address_offset, and get_end_loop_address_offset appear to be unused

seems like instances of those methods are found in the sanity check for Region.

• Voice's sample_rate is unused...wonder if that's an issue.

• There are many structs that should have constructors and start methods combined. i.e. Voice::new and Voice::start. The initial values are instantly written over and kept in bank if unused in VoiceCollection. Those voices need not be allocated until required, foregoing the need for allocation of all voices up to the maximum_polyphony value in the settings. Additionally, they can be dropped when unused. The reasoning behind using this foregoes a lot of the runtime checks that occur to ensure that voices exist. I'll demonstrate what I mean after 💤

• Lfo::get_value is ALWAYS called after Lfo::process. We can remove get_value entirely, and return the f32 directly.

[dsgallups]

• Seems like VolumeEnvelope and ModulationEnvelope behave identically, with the exception that VolumeEnvelope holds a priority. They're only found in Voice. Might be wrong here, but I think these structs could be combined as well.

Note I'm just throwing out observations, definitely don't want to cause any uncomfortability if there's a preference to keep what I've noticed out of the concept of a refactor :)

[dsgallups]

VolumeEnvelope::Process() returns a bool that is used, but it's hard to understand what for.

See VoiceCollection::process. It appears that if a voice has returned true, and i is equal to the active voice count, VoiceCollection.process returns. However, if the voice can't process, then it "sits" in the vector, in an unreachable place (now that active_voice count is behind by 1).

That is to say that a voice returns true if it has successfully processed while generating a block.

This is relevant to point 3.

I have replaced all this code (removed VoiceCollection, replaced with Vec<Voice>) by replacing the first part of the Synthesizer::render_block to be

        self.voices.retain_mut(|voice| {
            voice.process(&self.sound_font.wave_data, &self.channels)
        });

However, I'm quite ignorant about why the voices in the original collection had priorities, or why the structs are reused (i.e. exclusive classes for voices). I can't see much documentation on these purposes. There may be a good reason why voices are kept around, even if they're not currently being used. not sure.

[dsgallups]

Could I get some context about why the numerator for decay_slope and release_slope are different for ModulationEnvelope and VolumeEnvelope please 😄

After I removed VoiceCollection, I catch these differences with other differences in params in RegionEx. Unsure if the variables for ModulationEnvelope and VolumeEnvelope actually mean the same thing.

Slopes

https://github.com/sinshu/rustysynth/blob/4ce0b8ccf04ffe52ce4845d7ce4c3532b4ca2a4c/rustysynth/src/volume_envelope.rs#L61-L62 vs https://github.com/sinshu/rustysynth/blob/4ce0b8ccf04ffe52ce4845d7ce4c3532b4ca2a4c/rustysynth/src/modulation_envelope.rs#L61-L62

Decay logic (and Release logic)

https://github.com/sinshu/rustysynth/blob/4ce0b8ccf04ffe52ce4845d7ce4c3532b4ca2a4c/rustysynth/src/volume_envelope.rs#L121-L125 vs https://github.com/sinshu/rustysynth/blob/4ce0b8ccf04ffe52ce4845d7ce4c3532b4ca2a4c/rustysynth/src/modulation_envelope.rs#L120-L122

[sinshu]

@sevonj Thanks as always for the insightful PRs. I always learn a lot from them 👍

That said, I am not too keen on moving the MIDI event loop logic into MidiFileLoopType. The Message struct is an implementation detail that is only known to MidiFile and MidiFileSequencer, and I would prefer to keep it hidden not only from outside the crate but also from most of the internal code. This might just be a matter of personal preference, but that is how I feel.

[sinshu]

@dsgallups VolumeEnvelop and ModulationEnvelope are indeed similar, but there are subtle differences between them. Based on my judgment at the time of development, I decided to implement them as two separate pieces of code. As for why these subtle differences exist, it is simply because the original implementation in TinySoundFont, which RustySynth is based on, was written that way. That said, I do not fully understand why TinySoundFont made those specific implementation choices either.

The reason Vec<Voice> is pre-allocated and reused during playback is to follow the principle that memory allocation should not occur on the audio thread. If you are interested, I recommend looking up an article titled "Four common mistakes in audio development."

[sevonj]

@sinshu - MidiFileLoopType

There's a couple different ways to solve that. It could be moved back into midifile.rs as inline or a function. I like having the implementation next to the explanation, though.

Another option is to try using a more granular visibility setting. https://doc.rust-lang.org/reference/visibility-and-privacy.html

I'll read the rest of the thread soon-ish.

[dsgallups]

I recommend looking up an article titled "Four common mistakes in audio development."

That's an awesome article man, thanks!! That totally makes sense! (I literally did 3 of these 4 yesterday, shows how nooby I am).

IG then, @sinshu if you initialize your vec with Vec::with_capacity(settings.maximum_polyphony), doesn't that means that no memory allocations will occur after the initialization of the synth? In other words, malloc not be called after Synthesizer::new, but data will be pushed into that memory space...

Separately, thank you so much for teaching me about all this stuff. means a lot.

I really liked how you used a static array and index into it with consts (GeneratorType), made me think about if you could replace Vec<Voice> with a [Voice; MAX_POLYPH] and make Synthesizer Synthesizer<const MAX_POLYPH: usize>. Then you're guaranteed to never allocate again. These values will also be on the stack.

[sinshu]

About with_capacity, I'm not really sure 🤔 I think no allocation would occur, but I can't say for certain with my current knowledge of Rust...

[sevonj]

Lfo::get_value is ALWAYS called after Lfo::process. We can remove get_value entirely, and return the f32 directly.

I'm not a huge fan this idea. It's more compact, but not necessarily more readable.

Regarding getters: use of "get_" prefix is discouraged. It's a minor thing, but should we change this or keep using the current style? let _ = voice.block() is in my opinion nicer than let _ = voice.get_block() anyway.

"Four common mistakes in audio development."

That was a good read, thanks. I've been planning to build more audio stuff.

Envelopes

I think we should determine what the practical difference is, if any and merge the envelopes if they're the same?

Voice's sample_rate is unused...wonder if that's an issue.

min_voice_length is a multiple of sample rate. I'd first assume something named _length to be in seconds, though.

with_capacity

Indeed. Vec::with_capacity(n) is guaranteed to not allocate again if length <= n.

[dsgallups]

Definitely. @sinshu, let me know if you consider anything worth tackling, and I'd be happy to give back! I've really had fun rewriting most of this crate (along with all the bugs I've created).

I am super happy with you either

1. Listing out priorities/things you find need work or improvement
2. Saying "rustysynth is perfect, no need to change anything" because so far, it's been extremely reliable, and I'm still a complete novice at audio engineering in general.

Nonetheless, on #39

I don't think it's an abject failure yet. However, I think it's scary that, in #44, I couldn't figure out the problem for several hours. More importantly, that the root cause was a direct consequence of my PR. What I'm going to do is figure out some way to

• Take the rustysynth main branch
• Take #39
• Take midix_synth

and compare the output of all three in a non-flaky fashion. However, I'll probably need to add #[derive(PartialEq, Eq)] on a lot of the structs. Since the synthesizer appears to be deterministic, I can think of a few avenues to do this.

[sevonj]

@dsgallups - #39

I think this would be better done in smaller increments and with extensive automated unit tests and manual listening tests built beforehand. There's a lot of potential for subtle bugs.

getter naming

For public API, this is an option:

    pub fn value(&self) -> usize {
        self.value
    }

    #[deprecated = "renamed to value()"]
    pub fn get_value(&self) -> usize {
        self.value
    }

[sinshu]

Regarding Envelope, I think it's a matter of preference whether to handle something that is mostly similar but differs in key parts using if-statements or by implementing separate code. In my case, an issue related to the modulation envelope was reported in the original C# version, and since there is a possibility that I may further modify the modulation envelope because of that, I would prefer to keep the current split implementation for the time being.

@dsgallups
I'm glad if you enjoyed this code 😁 As for future plans, while I’m currently satisfied with the existing functionality, I do believe it's important to follow Rust conventions (including issue #39), so I’d like to continue refactoring. Also, if possible, I’d like to SIMD-optimize ArrayMath::multiply_add. In the C# version, doing so significantly improved performance. However, I’m still not sure if it’s possible to introduce SIMD while keeping the library dependency free.

Regarding testing, for changes that are not expected to affect sound quality, I always verify that the rendered output of flourish.mid is exactly the same before and after the change.

[sinshu]

There are quite a few public API methods with a get_ prefix, but I feel like most of them aren’t really used. The majority of users are only interested in things like note_on or process_midi_message. If we’re planning a major update, I don’t think we need to worry too much about maintaining compatibility in this area 🤔