I believe it is worth trying to remove this because there is reason to believe a performance hit occurs when doing these operations.

Please, no. It's a good thing to try and optimize the code but this is not one of those cases. We still have cases where a delay/reverb is hit with a loud signal with a loud crash sound as a result as a reminder to what will come if we remove this. Complaints on loud noises will increase but that's not the worst. It's the users who can't figure out the various odd effects and just will drop out of using lmms, and perhaps music software completely. I've been that user who couldn't figure out why the mix went completely silent and just dropped out of using lmms for some two years.

The results are based on when I was playing the demo, not on idle. I also did not run into any odd experiences with Infs/NaNs. More testing may be necessary.

And the test results for the sanitizing disabled as is already possible? The setting is a hidden one but maybe we can make it a proper option in settings?

Please, no. It's a good thing to try and optimize the code but this is one of those cases. We still have cases where a delay/reverb is hit with a loud signal with a loud crash sound as a result as a reminder to what will come if we remove this. Complaints on loud noises will increase but that's not the worst. It's the users who can't figure out the various odd effects and just will drop out of using lmms, and perhaps music software completely. I've been that user who couldn't figure out why the mix went completely silent and just dropped out of using lmms for some two years.

I personally think if there's a issue with loud bangs or loud silence and it relates to NaNs popping up somewhere, we should fix it at the source, not put some wrapper function around it like a band-aid and move on. #7141 was a step in that direction. The right direction really. We should keep looking out for issues like these and continue fixing them when we can.

Another important thing to mention: Even with the sanitation in place, users STILL reported issues involving the mixer going silent. The EQ was muting output because of NaNs in the filters (reported at #7131). How do we fix that? We fix it at the root. This is what should've happened initially, and this sanitation really has no reason to be here if we fixed the root cause of the problem in the beginning.

(I just noticed that the issue I talked about where the EQ was muting output might've been the reason for why your mix was going slient. You should test this now. It might've been actually fixed this time around. The sanitation here would cause the mix to be muted if a NaN or Inf was found, which could have been the problem).

And the test results for the sanitizing disabled as is already possible? The setting is a hidden one but maybe we can make it a proper option in settings?

Not sure what you mean, but I removed this sanitation completely. It has no reason to be here if we fix what needs to be fixed.

We still have cases where a delay/reverb is hit with a loud signal with a loud crash sound as a result as a reminder to what will come if we remove this.

Also important to note that in #7098, a user reports of a loud bang. So, we have this sanitation in place, and the intention was to have that functionality to avoid situations like this. But the problem exists even with this in place, so it makes you wonder what does this really fix?

But the problem exists even with this in place, so it makes you wonder what does this really fix?

Many of the instances of floating point errors will hand you a really large signal before reaching inf/NaN. This is why I opted to just nuke the whole buffer in these cases. It will handle most issues but not all. Also, some of the loud bangs aren't inf/NaN but just some other loud artifact, for these we have clipping of the signal to 1000.f.

else
{
	src[f][c] = std::clamp(src[f][c], -1000.0f, 1000.0f);
}

Why not allow both sanitizing the signal and then let the sound run free for the young and brave?

Many of the instances of floating point errors will hand you a really large signal before reaching inf/NaN. This is why I opted to just nuke the whole buffer in these cases. It will handle most issues but not all. Also, some of the loud bangs aren't inf/NaN but just some other loud artifact, for these we have clipping of the signal to 1000.f.

Okay, but I'm trying to tell you is that these aren't solutions. They are bandages covering the real problem. Setting the clipping of the signal to 1000.0f is a bandage (which is odd since for digital audio it goes from -1.0f to 1.0f). Nuking the whole buffer is a bandage (and also ended up being a problem itself). They simply cover up the real problem we need to address somewhere else entirely, even if its not because of an Inf or NaN.

Why not allow both sanitizing the signal and then let the sound run free for the young and brave?

Because sanitizing the buffer makes the processing slower, even if its a bit. If we can make this program's audio processing faster in any way, I see that as an immediate benefit.

Because sanitizing the buffer makes the processing slower, even if its a bit. I

It's quite a bit slower, yes. But my suggestion was, to instead make the nanhandler setting visible and known. Surely a single test is a reasonable trade off?

They are bandages

Right, I have heard those talking points before. They are not bandages but just a result of the final product meeting the real world. Things are simply messy out there.

They simply cover up the real problem we need to address somewhere else entirely, even if its not because of an Inf or NaN.

It's quite a lot of plugins that has glitches like that and this is the reason other hosts do sanitize the signal. Some plugins sanitize the signal too, especially delay based ones since these are the ones that allow the occasional large transients to loop around and grow to a bang.

which is odd since for digital audio it goes from -1.0f to 1.0f

I also found this odd but as it turns out, while the final sound file is mostly clipped to +/-1.0f, people like to break those numbers routinely. I think vst is mostly +/-1.0 but that may be wrong too.

Ps. This is related code that should be deleted if the nanhandler is nuked: https://github.com/LMMS/lmms/pull/4787/files

Let's ask @jasp00 . I think he's on your side by the way! :)

(I just noticed that the issue I talked about where the EQ was muting output might've been the reason for why your mix was going slient.

No. This was some twelve years ago but I think it was the Calf compressor that got me. It got me good though.

I tested this on some demos, results are shown below. The results are based on when I was playing the demo, not on idle. I also did not run into any odd experiences with Infs/NaNs. More testing may be necessary.

Q - What method did you use to find out the load?
Q - Can you complement this list with Master: Effects % and Mixer % with nanhandler off? nanhandler="0"
The setting is somewhere in the top of your lmmsrc.xml. This is what the line looks like by default on my machine.

  <app displaydbfs="0" configured="1" nommpz="0" loopmarkermode="dual" nanhandler="1" language="en" disablebackup="0" 

This bypasses the sanitizer.

Q - What method did you use to find out the load?

I just used the CPU meter tooltip.

Can you complement this list with Master: Effects % and Mixer % with nanhandler off? nanhandler="0"
The setting is somewhere in the top of your lmmsrc.xml. This is what the line looks like by default on my machine.

Sure. I'll let you know when I'm done. Or, if you want to see for yourself, you can also try.

How about making the sanitation faster? A little something I threw together over my morning coffee, but I think it might optimize better, and at the very least be faster for the common case, i.e. the buffer not containing any Inf/Nan.

finite = false
for(i=0; i<frames*channels; ++i) {
   finite &= std::isfinite(src[i];
   src[i] = std::clamp(src[i], -1000.0f, 1000.0f);
}
if(!finite) {
  // memset or whatever the buffer to 0 
 return true
}
return false

How about making the sanitation faster? A little something I threw together over my morning coffee, but I think it might optimize better, and at the very least be faster for the common case, i.e. the buffer not containing any Inf/Nan.

We can most certainly make it faster, but its even more faster to do nothing at all. I think doing nothing at all is the way to go in Release builds, even if we decide we don't want to remove it entirely, but only enable the checks for debugging (suggestion by @rdrpenguin04 on Discord).

Only enabling them debugging saves us the performance hit, but we also get the safety checks if we want to take a look more closely. Of course, if we intend on keeping sanitation, we should definitely optimize that function, no doubt.

Still, this suggestion still has some issues though. We still have the problem of checking if Infs and NaNs are being generated elsewhere in the code besides the mixing stage, so this sanitation wouldn't be able to cover those cases.

The better solution is to still remove sanitation completely, but use other tools out there to debug an Inf or NaN situation in the event that we suspect we are dealing with them. This will help to actually get to resolving the bug, no performance hit for regular users that did not ask for it, less code debt, etc.

There are tools such as MemorySanitizer and Valgrind. From what I've seen, most of the time we ran into an Inf or Nan was because we did not initialize our variables, which is what these two can help with. For example, in Valgrind's case, it will report issues involving unitialized variables and actually take us to the problem directly in the code through its stacktrace.

TLDR: Still remove sanitation, but use debugging tools to actually fix the root cause.

Edit: There is also UndefinedBehaviorSanitizer for more specific UB checks that can help find an Inf or NaN.

Got around doing the nanhandler=0 test for master @zonkmachine:

So basically no difference from a performance standpoint.

Changing this PR per this comment.

Try this drum track from #7340: compressor-sigfpe.mmp.txt

Clipping loud signals has artifacts. It looks nice when the LED lights up though... :)

Try this drum track from #7340: compressor-sigfpe.mmp.txt

Clipping loud signals has artifacts. It looks nice when the LED lights up though... :)

Yeah, the feature seems to be working perfectly for me here. This will be amazing for debugging.

Edit: Oh wait, I see the issue now. I'll take a look soon.

Interesting, the artifacts looks like the ones in #6934

This PR also changes the nanhandler to be exposed and was renamed to muteinvalidoutput for more accuracy as to what the option does.

@sakertooth Can you help explain how this name is more accurate?

Existing behavior:

• With nanhandler off, it implicitly and unobviously mutes the entire mixer due to math.
• With nanhandler on, it silently sanitizes the values and plays properly, no warning or notice to the user (except, perhaps, in the logs) that the issue occurs.
• The name is slightly ambiguous but accurate.

With the new behavior, does the muteinvalidoutput change the nanhandler behavior to mute the problematic plugin/mixer channel?

Edit: 1: The video does show the plugin getting muted, so I think that's what happens.

If so, that may not be desired. I also feel that this name may add further ambiguity as to the symptom being similar to the """"bug"""".

Edit: 2 I forgot to say "Thanks". Thanks!

With nanhandler on, it silently sanitizes the values and plays properly, no warning or notice to the user (except, perhaps, in the logs) that the issue occurs.

Your choice of words don't sit right with me here. Nothing about playing/muting a corrupted buffer should be considered "the project is playing properly". In fact, the existing behavior I believe mutes the entire buffer when invalid output is found in it. So are you sure you aren't just hearing the rest of the song? That isn't it playing properly at all.

The name is slightly ambiguous but accurate.

Feel free to suggest a better name. I like it over nanhandler at least because it acts as a verb, while nanhandler is a noun. And I think its clear from its name what the option will do. I also don't see an issue with this option being separate from the old nanhandler option.

With the new behavior, does the muteinvalidoutput change the nanhandler behavior to mute the problematic plugin/mixer channel?

What happens here is that the mixer channel processes its audio normally, then when everything is said and done, it has the option (if the user permits) to look for invalid output. If invalid output is found, it mutes the buffer and emits a signal to the UI to flash its mute button red, saying that it was forcefully muted internally because of invalid output.

The current behavior achieves the same thing (besides the visual indicator), but with a lot more work. It checks the audio input first for invalid values, and checks the buffer after processing each effect. If at any point it finds an Inf or NaN, it mutes the buffer and continues processing. There is no reason to continue the processing after muting the buffer, because that effectively would mute the entire effect chain from the issue (whether the issue is a faulty plugin or some audio input that was fed into the mixer channel) onwards. Simply a waste of CPU time.

TLDR: They both mute the mixer channel, but the current behavior does so in a suboptimal way, and doesn't provide the visual indicator of course.

NB: I also want to clarify that this invalid output handling should not be considered a fix for anything, but a tool to help find and eventually fix Infs or NaNs appearing in the signal chain. It seems as if the current sanitation was implemented as a fix, when it very evidently isn't. Resolving the issue for why the Inf or NaN appears and removing it is a fix.

I forgot to say "Thanks". Thanks!

No problem. 👍

In fact, the existing behavior I believe mutes the entire buffer when invalid output is found in it

The code chooses a "sane" value when an invalid value is found. In testing, this sounds rather fine. These may be audible under certain circumstances, but with the zita-reverb plug-in, they sound fine enough for casual listening. The modified buffers are not noticable, at least not to my ear on first listen. The reverb is clearly audible and "working enough" for the ear to think it's working. This makes sense because the bug report was marked as resolved, consistent with my findings.

This PR, as I understand it, mutes the plugin, breaking the sound.

I don't want to minimize the importance of notifying a user when a plugin is misbehaving, I hope that's not misconstrued, but muting the plugin is a regression to the user -- the person -- without knowledge of how to fix this plugin ( because projects in older versions of LMMS could play and sound "normal" prior to this patch ) don't just magically patch old broken plugins because they're muted. Developers may choose to, but this has still proven to be a rabbit hole. In the case of zita-reverb, I'm not even certain where the upstream code is considered to live to file such a bug report.

I've been thinking how to represent this to the user interface and I believe there should be some type of notification and the plugin should go to an intermediate state (e.g. not green, not red, but something else perhaps purple or yellow) and the user can chose -- if they wish -- to remove, mute or replace the problematic plugin.

With regards to the name of the feature (verb, noun or otherwise) it will be better defined if we can agree on what it should be doing, which still seems to be in disagreement.

@sakertooth thanks for all the work on this. This does exactly as described. I consider this an improvement over existing behavior.

An enhancement to this functionality would be to display some type of alert when the issue occurs, but the logs do show some helpful information, so I would consider this ready.

An enhancement to this functionality would be to display some type of alert when the issue occurs, but the logs do show some helpful information, so I would consider this ready.

It seems like I did away with the logging recently (not exactly sure why). I'll add it back since it is helpful and think of merging after if no one objects.

There's an issue with the connections not applying to the right channels, causing some channels to report having invalid output when there isn't. I'll have to fix this issue before merge.

Updated this PR. Mixer channels can detect invalid output (inf/nan), set a flag, and then that flag is checked at a fixed interval on the main thread, reporting to stderr if the flag has been set by the audio thread, Overall same idea as before, but different approach.

sakertooth closed this 4 hours ago

Why?

Why?

Not too sure. Its kind of random why I did close. I was feeling pretty bad at the time with how the codebase was moving and started trying to relinquish myself from it all. I'm back though (lol) so I've reopened it.

Disregarding that however, I think I have to change how this PR works to avoid the propagation of NaNs, because it can still propagate within the internal plugin buffers and stay there forever, causing permanent corruption. So the user would have to reload the faulty plugin still.

@tresf I couldn't keep the original behavior proposed in this PR because once a NaN hits a plugin, it will stay there permanently, so we need to sanitize at the effect boundaries to avoid this. The onus is mostly on the plugin developers themselves to ensure that inf and nan values don't end up in the signal chain. Also, I changed the sanitation to only replace the invalid inf/nan values with silence instead of the whole buffer. That will at least avoid having all of the plugin's useful output disappear.

There also isn't a nice way to report the error too because once it happens, it will just result in constant spam in the logs. Alternatively, I might try to mark the effect's I am sanitizing with the red mute button, like I did with the mixer channels. I do like that feature, because it gives the user feedback.

I see nothing wrong with spamming the logs. Red LED is fine too, just might be hard to find/notice.