Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Metronome starts skipping at fast tempo. #4038

Closed
EtornaZ opened this issue Dec 6, 2017 · 13 comments · Fixed by #7483
Closed

Metronome starts skipping at fast tempo. #4038

EtornaZ opened this issue Dec 6, 2017 · 13 comments · Fixed by #7483

Comments

@EtornaZ
Copy link

EtornaZ commented Dec 6, 2017

I am using LMMS 1.2.0 RC4.
The metronome in LMMS will start skipping if the tempo is faster than 218 BPM, in about 15-20 seconds. If the tempo is sped up to around 230 BPM, it starts skipping sooner, more like 5-7 seconds. When it's at 250 the metronome will immediately start skipping. I restarted LMMS just in case it was a temporary thing, but the bug still persisted.
This is all of the information I have, but if I find out more about this bug I'll let you all know.

@PhysSong
Copy link
Member

PhysSong commented Dec 6, 2017

Confirmed. Metronome is handled in Mixer::renderNextBuffer().

lmms/src/core/Mixer.cpp

Lines 359 to 375 in dd4a73e

if( playModeSupportsMetronome && m_metronomeActive && !song->isExporting() &&
p != last_metro_pos &&
// Stop crash with metronome if empty project
Engine::getSong()->countTracks() )
{
tick_t ticksPerTact = MidiTime::ticksPerTact();
if ( p.getTicks() % (ticksPerTact / 1 ) == 0 )
{
addPlayHandle( new SamplePlayHandle( "misc/metronome02.ogg" ) );
}
else if ( p.getTicks() % (ticksPerTact /
song->getTimeSigModel().getNumerator() ) == 0 )
{
addPlayHandle( new SamplePlayHandle( "misc/metronome01.ogg" ) );
}
last_metro_pos = p;
}

With 4/4 time signature, one tick is 1.25/(tempo in BPM) seconds. It's about 5.79 ms if the tempo is 216BPM. And the buffer is about 5.80 ms long with 44100Hz, 256 frames. When one tick is shorter than one mixer period, this logic breaks down.

There are some possible solutions:

  1. Do a better calculation in the same function. Might be a little bit tricky?
  2. Move the metronome logic to Song::processNextBuffer(). It could be a better alternative because this function adds play handles for ordinary tracks.
  3. Introduce a new sort of track, named "Metronome track". This is simple because metronome could be treated as a normal track. It means Mixer::renderNextBuffer() and/or Song::processNextBuffer() don't need any hacks for metronome.

@gi0e5b06
Copy link

gi0e5b06 commented Dec 9, 2017

@PhysSong The best design would be the third one but that would involve too many changes. If the core got redesigned one day, that should be the best choice. You have a better understanding than me on how things work internally so just chose what works best among the two first ones.
Until then, I fixed the problem. The code for the metronome is now frame-exact and 100 times faster. It works fine up to 999 bpm (I haven't tested over).
https://github.com/gi0e5b06/lmms/blob/master/src/core/Mixer.cpp#L344

@PhysSong
Copy link
Member

PhysSong commented Dec 9, 2017

@gi0e5b06 Thank you for the suggestion. That makes sense, but looks inefficient. I will try to find a better way.

@gi0e5b06
Copy link

gi0e5b06 commented Dec 9, 2017

@PhysSong The old code was inefficient because of the instantiation of the metronome samples at each beat. I removed that. The fact that it still uses NotePlayHandles is no big deal. But as a user, there is something much more problematic: the metronome is affected by the effects. Ideally it should be the last thing to be mixed, after everything else (so at the end of the master mix).

@PhysSong
Copy link
Member

PhysSong commented Dec 9, 2017

The old code was inefficient because of the instantiation of the metronome samples at each beat.

That's a problem too, but it looks like a separate issue. I meant the loop logic when I said "looks inefficient". It could be replaced by simpler and faster way.

@frank-a-i
Copy link
Contributor

frank-a-i commented Sep 1, 2021

@PhysSong is there already a solution in the pipeline or does it make sense to take a look into it?

@zonkmachine
Copy link
Member

@frank-ihle I don't know of any fix for this but the one from @gi0e5b06 which I can't find in his link above. Please look into it!

@frank-a-i
Copy link
Contributor

@frank-ihle I don't know of any fix for this but the one from @gi0e5b06 which I can't find in his link above. Please look into it!

From my peek into the code I would come to a similar conclusion as the 3 solutions mentioned above, maye there's a better way that's what I'd like to spent the time on. However I'm a little bit concerned that deep down, this might be OS/Scheduling related and I'm curious for opinions on this thought.

@zonkmachine
Copy link
Member

I think this behavior is reported by just about everyone and the result is the same. If it was related to anything deeper into the OS, wouldn't it differ between systems and/or number of cores?

@frank-a-i
Copy link
Contributor

frank-a-i commented Sep 4, 2021

Okay I've figured out the root cause and I also have a fix. However I'm not sure if I like it. Let me point out with a code-walkabout

The problem

In extension to @PhysSong analytics #4038 (comment) solution number 2 and 3 should still be affected, here is why:

if (ticks % (ticksPerBar / 1) == 0)

is the line that figures out when to play the tick sound, it relies on

tick_t ticks = song->getPlayPos(currentPlayMode).getTicks();

the modulo operation mentioned above is working with a comparably, and maybe unnecessary-extremly high precision and so it comes if getTicks() didn't manage to return a value that is simply an increment to the previous call, the modulo cannot become 0 anymore and thus (has to) skip(s) a tick.

An obivous solution

Play the sound whenever the right time spot has been passed (be it exact or shortly afterwards), like

tick_t curHighTickDivison = ticks / (ticksPerBar / 1) ;
tick_t curLowTickDivision = ticks / (ticksPerBar / numerator) ;
if (int(currentHighTickDivison) >= lastHighTickDivison)  // <--- whenever the number before the floating point gets incremented, it is the time to play ticks
{
	addPlayHandle(new SamplePlayHandle("misc/metronome02.ogg"));
}
else if (int(curLowTickDivision) >= lastLowTickDivison)
{
	addPlayHandle(new SamplePlayHandle("misc/metronome01.ogg"));
}
lastLowTickDivision = curLowTickDivision;
lastHighTickDivision = curHighTickDivision;

I have verified this approach on my low-latency Ubuntu kernel.

However, what I don't like is, LMMS will always be a little off beat - not that much but still. In fact this already affects other playable samples too. I'm yet not sure if this is caused by developer manipulable threading or a restricted os-scheduling, but to fix that in turn I currently believe we would have to rethink how we want to realize the overall merging of the individual audio samples

@zonkmachine
Copy link
Member

However, what I don't like is, LMMS will always be a little off beat

I don't notice this, it's a clear improvement.

There is an issue with #6149 where the first metronome tick is being skipped. Either when looping or when starting from the position where the song is at, i.e. the song position doesn't have to skip from whithout the loop before playing in which case the first sound is played. It kind of reminds me of #3028

@frank-a-i
Copy link
Contributor

frank-a-i commented Sep 6, 2021

However, what I don't like is, LMMS will always be a little off beat

I don't notice this, it's a clear improvement.

Yep on my machine and from what I saw it was just 1 tick, I doubt that humans can even process such a speed. But with my LMMS experience I cannot extrapolate the severity for other situations, e.g. if one is running a complex song or simply struggles with poor machine resources

There is an issue with #6149 where the first metronome tick is being skipped. Either when looping or when starting from the position where the song is at, i.e. the song position doesn't have to skip from whithout the loop before playing in which case the first sound is played. It kind of reminds me of #3028

Good ears, I wouldn't have noticed, if you hadn't pointed that out, thanks - published the fix

@zonkmachine
Copy link
Member

Ok. Now the first tick plays. But not after you've stopped it once. Start lmms with the default project. Activate the metronome. tick, tock, tock, tock, tick... etc
Stop and start again. The first tick is now omitted. This is also the case with the first tick in a loop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants