Improve handling of nan/inf #4743
Conversation
src/core/MixHelpers.cpp
Outdated
| @@ -77,7 +77,7 @@ bool sanitize( sampleFrame * src, int frames ) | |||
| { | |||
| for( int c = 0; c < 2; ++c ) | |||
| { | |||
| if( isinff( src[f][c] ) || isnanf( src[f][c] ) ) | |||
| if( unlikely( isinff( src[f][c] ) || isnanf( src[f][c] ) ) ) | |||
There was a problem hiding this comment.
Please note that unlikely is incompatible with MSVC.
There was a problem hiding this comment.
Right. As I understand it this should work in master then where Q_UNLIKELY(x) is used instead of builtin_expect directly. I'll revert this for stable-1.2 .
|
I wonder if clearing whole buffer will cause some noise like #4625 (comment) , though I guess the possibility is low. |
zonkmachine
force-pushed
the
baddata2
branch
from
76065c0
to
2239dc3
Compare
|
I find it hard to guess with the nan stuff. If you have a situation where the signal is going into/out of forbidden regions (inf/nan) you would probably get a squarish, digital noise kind of signal, clipped at +/-100 . Not an altogether appealing scenario. |
|
Sanitizing values should not depend on buffer size. Overall, |
Why not? It is a wee bit aggressive, yes, but the problem area (in my opinion) is before and after the nan/inf samples and this was the easiest way to get to them. I haven't tested this with different buffer size setting yet. Are you OK with clearing out samples around the nan/inf samples if it just isn't the whole buffer? Such as 16 bits before/after. It would no longer depend on the buffer size.
A switch in Do you know how sanitizing is handled in other DAWs? |
|
Sanitizing values should not depend on buffer size because it does not fix your problem. What if the first sample in the buffer is NaN? To accomplish your goal, you need to apply a window, a filter. This decision should be left to the user through effect plug-ins, not hard coded neither through settings. Another issue is that NaN and infinite are not the same. Setting infinite to zero is wrong. Clipping should also be left to the user through effect plug-ins. I have not looked at other DAWs, I do not think it is necessary in this case. |
Why? What else can you do with an infinite value?
If you have little experience in lmms and make a track with some 20+ channels and suddely one day the whole thing goes all silent because of one effect unit on one instrument is sending NaN. Are you supposed to trouble shoot stuff like this? I've never experienced this on any DAW. We had sanitizing code on export and I find it completely rational to turn it on when we're not exporting too. Do you mean some of this should be fixed for 1.2 already like the option to turn the sanitizing on/off? |
There are positive and negative infinities.
As a user, I could use an anti-NaN effect before the bug is fixed.
Your call. My choice for 1.2 would be a LADSPA effect. |
To fix what? The sanitize() function used to only run when exporting but now it's been turned on for regular use too (see #3706). Do you mean this should be reverted and an effect unit used instead? I'm not against it but I can't do it myself. |
|
I still haven't seen any reason provided for why the sanitizing should be
an effect, and it seems absurd to me. I can't imagine any other DAW would
do this. In an ideal world, the user wouldn't have to know or care about
the possibility of a bug existing, so why would you intentionally make this
optional, off by default, and implemented via effect?
Release builds don't provide a "crash instantly on exceptions" toggle in
the settings, so why would they include a "let audio output completely
break" setting?
…On Thu, Dec 27, 2018, 02:58 Oskar Wallgren ***@***.*** wrote:
Do you mean some of this should be fixed for 1.2 already like the option
to turn the sanitizing on/off?
Your call. My choice for 1.2 would be a LADSPA effect.
To fix what? The sanitize() function used to only run when exporting but
now it's been turned on for regular use too (see #3706
<#3706>). Do you mean this should be
reverted and an effect unit used instead? I'm not against it but I can't do
it myself.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#4743 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AIgVmr0Mjg1uvmU5SW-cb8cmEZPQyV5Rks5u9CkqgaJpZM4ZhrdZ>
.
|
Or infinite... Then the whole buffer gets knocked out. One solution for all cases. It's not about making that channel sound good but about solving an issue that is an extreme case and to try and remove the possibility of sound artifacts that can be annoyingly/dangerously loud.
Or, I could observe an error message that informs me that a pluggin is acting up and that lmms has taken action to protect 1. my sanity 2. my ears 3. my equipment 4. the rest of the project. |
|
I will add a hidden setting to disable forced sanitizing, okay? |
Sure! |
That sounds like a good solution, as it could be implemented through checking continuosly for NaN/Inf on the master channel, invoking more thorough checking only when needed to find the offending unit. |
Maybe remove it from exporting too then, to make render and playback work the same? It's not very common but there are cases where users have experienced noise in the exported track where there is none in playback or silence in playback but exported track works fine. |
|
Reverted the increased clipping level and here is the file that made me take that decision. It works pretty poor with or without sanitizing. It still works worst without sanitizing though, but not with a ground breaking lead. The Vynil effect is one that receives quite a lot of remarks of making unwanted noise. It simply makes loud beeps spontaneously. |
|
I've listened to noise for some 2 days now to get to grips with the best levels to clip it. I was working towards the original idea to increase it to +/-100.0f but am not settling with the more cautious +/-10.0f . I'll merge this tomorrow if there are no voices against it. |
* If we find NaN/inf, we declare the whole buffer bad and set it to 0.0f. This is because the noise leading up to, or coming from, an infinite or NaN value is often very large and will create problems later in the sound chain. Especially if it hits a delay based fx with feedback. * We bump the clipping level to +/-10.0f.
If we find nan/inf, we declare the whole buffer bad and set it to 0.0f. I'm still clipping the signal, for now at least, but set the level at
+/-100.0f+/-10.0f.Fixes #4665
None of the example projects used to test nan/inf works any longer as each of their specific issues has been fixed. To test this PR properly you need to cherry-pick it on top of an older version of lmms like rc2.