hydrogen-music / hydrogen

The advanced drum machine for Linux, macOS, and Windows
http://www.hydrogen-music.org
GNU General Public License v2.0
1.04k stars 173 forks source link

Sampler: account for note length during rendering #1872

Closed theGreatWhiteShark closed 12 months ago

theGreatWhiteShark commented 12 months ago

it seems the custom note length defined by the user never made it into the Sampler. At least properly.

Previously, the remaining frames of a samples were always calculated from the original length of the sample. Not the user defined one. The original length was also the one added to the sample position of the selected layer being rendered. At the end of the user defined length this caused the sample position of the selected layer to be beyond the note length and the note end provided to ADSR::applyADSR() to be negative. The latter than ended rendering.

theGreatWhiteShark commented 12 months ago

I have to admit that looking at Sampler::renderNoteResample() and Sampler::renderNoteNoResample() does not make me too confident about that the Sampler is doing fine. They are mostly the same but differ at a couple of places where I can't shake the feeling that just one of them is right.

cme commented 12 months ago

Yeah, I keep meaning to refactor this to avoid the duplication. I think the NoResample variant only exists as an optimisation of the common case, because the resample loop is hideously inefficient. There are far better ways to tackle that though.

I'm apprehensive about the correctness because the logic has diverged and the more complex case, the Resample case, is the one that should be retained and optimised, but because it's more commonly used, NoResample has had more testing exposure :/

theGreatWhiteShark commented 12 months ago

It's a bit of a shame these very essential parts of the code did grew so naturally. The Timeline was another example. Neither AudioEngine nor Sampler were properly adapted and before rewriting the AudioEngine we basically had glitches every single time a tempo marker was passed or a manual tempo change occurred.

NoResample has had more testing exposure

I'm not sure about that. E.g. the samples of the GMRockKit have a sample rate of 44100 while my JACK server is running on 48000. This requires 100% resampling.