Stuck VU meter on muted mixer channel

[softrabbit]

Bug Summary

Stuck VU meter on muted mixer channel, pretty sure nobody has filed an issue on this.

Steps to reproduce

Play sound, mute mixer channel while sound is playing.

Expected behavior

The meter on the muted channel should drop to 0.

Actual behavior

The meter on the muted channel stays up.

Screenshot

Affected LMMS versions

Version 1.3.0-alpha.1.33+g4f74151 (Linux/x86_64, Qt 5.6.3, GCC 5.4.0 20160609). AppImage from lmms.io Master pulled from git on 2021-03-08 and compiled on Ubuntu 20.04

Logs

Click to expand

Carla does not appear to be installed.  That's OK, please ignore any related library errors.
Jack appears to be installed on this system, so we'll use it.
Gtk-Message: 09:22:25.628: Failed to load module "overlay-scrollbar"
Gtk-Message: 09:22:25.647: Failed to load module "atk-bridge"
Fontconfig warning: "/etc/fonts/conf.avail/65-ttf-droid-sans-fonts.conf", line 61: Having multiple values in  isn't supported and may not work as expected
Fontconfig warning: "/etc/fonts/conf.avail/65-ttf-droid-sans-fonts.conf", line 96: Having multiple values in  isn't supported and may not work as expected
Fontconfig warning: "/etc/fonts/conf.avail/65-ttf-droid-sans-fonts.conf", line 61: Having multiple values in  isn't supported and may not work as expected
Fontconfig warning: "/etc/fonts/conf.avail/65-ttf-droid-sans-fonts.conf", line 96: Having multiple values in  isn't supported and may not work as expected
VST sync support disabled in your configuration
Lv2 plugin SUMMARY: 0 of 5  loaded in 4 msecs.
For details about not loaded plugins, please set
  environment variable "LMMS_LV2_DEBUG" to nonempty.
"Cannot load library /usr/lib/ladspa/autotalent.so: (/usr/lib/ladspa/autotalent.so: undefined symbol: __exp_finite)"
"Cannot load library /usr/lib/ladspa/autotalent.so: (/usr/lib/ladspa/autotalent.so: undefined symbol: __exp_finite)"
Cannot load library /tmp/.mount_lmms-1AYlJU1/usr/lib/lmms/libcarlabase.so: (libcarla_native-plugin.so: cannot open shared object file: No such file or directory)
Cannot load library /tmp/.mount_lmms-1AYlJU1/usr/lib/lmms/libcarlapatchbay.so: (libcarla_native-plugin.so: cannot open shared object file: No such file or directory)
Cannot load library /tmp/.mount_lmms-1AYlJU1/usr/lib/lmms/libcarlarack.so: (libcarla_native-plugin.so: cannot open shared object file: No such file or directory)

[softrabbit]

Did some bisecting, 499d425b4b9ebe0d1c13038cd9acf4777af94320 is probably the first bad commit. Just guessing here, but could it be that comparing floats for equality is a bad idea?

[DomClark]

Comparing floats for equality is in general a bad idea, but in this case I don't think it's a problem. The code assigns -1 and checks for the exact same value, which will succeed, and no other nearby values are assigned (all other assignments to those variables are non-negative).

The issue is that a -1 peak value is introduced as a "don't update this fader" setinel. This is usually overwritten in the next buffer, except if the channel is muted, since muted channels aren't processed.

@DouglasDGI What's the purpose of this special value? The old code didn't use it.

[softrabbit]

Dropping a note to myself here after reading the function a few times, something for me to try later on the dev box. 100% untested coffee break quality code. ☕ (Try this on one channel to compare the falloff behavior visually!)

// Select what to show, precompensate the peak for falloff
const float opl = qMax( m->effectChannel(i)->m_peakLeft*fallOff,  m_fxChannelViews[i]->m_fader->getPeak_L() ); 
// Update counter, with falloff
m_fxChannelViews[i]->m_fader->setPeak_L(  opl/fallOff );
// Might be a bad idea to always reset the peak?
m->effectChannel(i)->m_peakLeft = 0; 

[softrabbit]

@DouglasDGI What's the purpose of this special value? The old code didn't use it.

The comment "Set to -1 so later we'll know if this value has been refreshed yet." hints at it being used to prevent the falloff being applied if there hasn't been a refresh on the peak values since the previous periodicUpdate signal, i.e. in 1/60 s. That is 735 samples at 44100 Hz, so a buffer size > 512 will trigger the bug and the meter will fall too fast, maybe even be a bit jerky?

I didn't find an issue that fits my reasoning or what the PR is supposed to fix: "a display bug that is present with large fader falloff speeds.", though.

[DomClark]

The comment "Set to -1 so later we'll know if this value has been refreshed yet." hints at it being used to prevent the falloff being applied if there hasn't been a refresh on the peak values since the previous periodicUpdate signal, i.e. in 1/60 s. That is 735 samples at 44100 Hz, so a buffer size > 512 will trigger the bug and the meter will fall too fast, maybe even be a bit jerky?

If the peak values haven't been refreshed, then it doesn't matter if falloff is applied, because the peak value will be greater than the subsequent fader value, and so the fader value will be reset to the peak value. Also, LMMS currently has a maximum internal buffer size of 256, so this would require a very low sample rate to occur (15000Hz or below).

I didn't find an issue that fits my reasoning or what the PR is supposed to fix: "a display bug that is present with large fader falloff speeds.", though.

My guess is that this is what dividing by the falloff in the peak comparison is for. Previously, the peak value was compared to the old value pre-falloff, so it was possible for the fader to fall below the peak value if the old value were above the peak but the falloff value below it.