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

Improve handling of nan/inf #4743

Merged
merged 3 commits into from Jan 20, 2019

Conversation

Projects
None yet
5 participants
@zonkmachine
Copy link
Member

zonkmachine commented Dec 26, 2018

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.

@zonkmachine zonkmachine changed the title Baddata2 Improve handling of nan/inf Dec 26, 2018

@@ -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] ) ) )

This comment has been minimized.

@PhysSong

PhysSong Dec 26, 2018

Member

Please note that unlikely is incompatible with MSVC.

This comment has been minimized.

@zonkmachine

zonkmachine Dec 26, 2018

Author Member

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 .

This comment has been minimized.

@zonkmachine

zonkmachine Dec 26, 2018

Author Member

Fixed!

@PhysSong

This comment has been minimized.

Copy link
Member

PhysSong commented Dec 26, 2018

I wonder if clearing whole buffer will cause some noise like #4625 (comment) , though I guess the possibility is low.

@zonkmachine zonkmachine force-pushed the zonkmachine:baddata2 branch from 76065c0 to 2239dc3 Dec 26, 2018

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Dec 26, 2018

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.
What I've seen when testing this PR on top of rc2 is that the nan/inf mostly happens already in the beginning (first or second sample) of the buffer then that run is mostly corrupted so the end result is not different from the earlier way with the whole buffer becoming cleared out. The difference is when the corruption happens somewhere in the middle with extreme level signals in front of it. Here we just sneak back and cut that part out.
I think the best idea going forward would be to somehow bypass a unit once it creates bad data and signal with the LED color like is done for that FX units that aren't found . The noise source is contained and the user informed.

@jasp00

This comment has been minimized.

Copy link
Member

jasp00 commented Dec 26, 2018

Sanitizing values should not depend on buffer size. Overall, MixHelpers::sanitize() should not be executed by default. This behavior should be available as an effect plug-in.

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Dec 26, 2018

Sanitizing values should not depend on buffer size.

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.

Overall, MixHelpers::sanitize() should not be executed by default.

A switch in Settings > Performance Settings ?

Do you know how sanitizing is handled in other DAWs?

@jasp00

This comment has been minimized.

Copy link
Member

jasp00 commented Dec 26, 2018

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.

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Dec 26, 2018

Another issue is that NaN and infinite are not the same. Setting infinite to zero is wrong.

Why? What else can you do with an infinite value?

This decision should be left to the user through effect plug-ins, not hard coded neither through settings.

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?

@jasp00

This comment has been minimized.

Copy link
Member

jasp00 commented Dec 27, 2018

What else can you do with an infinite value?

There are positive and negative infinities.

one effect unit on one instrument is sending NaN. Are you supposed to trouble shoot stuff like this?

As a user, I could use an anti-NaN effect before the bug is fixed.

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.

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Dec 27, 2018

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). 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.

@Spekular

This comment has been minimized.

Copy link
Contributor

Spekular commented Dec 27, 2018

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Dec 27, 2018

What if the first sample in the buffer is NaN?

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.

As a user, I could use an anti-NaN effect before the bug is fixed.

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'm still not convinced that this is the wrong way to go about things.

@jasp00

This comment has been minimized.

Copy link
Member

jasp00 commented Dec 27, 2018

I will add a hidden setting to disable forced sanitizing, okay?

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Dec 27, 2018

I will add a hidden setting to disable forced sanitizing, okay?

Sure!

@softrabbit

This comment has been minimized.

Copy link
Member

softrabbit commented Dec 28, 2018

I think the best idea going forward would be to somehow bypass a unit once it creates bad data and signal with the LED color like is done for that FX units that aren't found . The noise source is contained and the user informed.

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.

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Dec 30, 2018

to disable forced sanitizing, okay?

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.

@zonkmachine zonkmachine force-pushed the zonkmachine:baddata2 branch from 38215c8 to 91a4128 Jan 3, 2019

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jan 3, 2019

Reverted the increased clipping level and here is the file that made me take that decision.
vynilnoise.mmp.zip (Warning! It's loud.)
#3170

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.

@zonkmachine

This comment has been minimized.

Copy link
Member Author

zonkmachine commented Jan 19, 2019

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 .
The reason is mainly lack of samples because it's hard to replicate these bugs.

I'll merge this tomorrow if there are no voices against it.

@zonkmachine zonkmachine merged commit dd99f3a into LMMS:stable-1.2 Jan 20, 2019

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@zonkmachine zonkmachine deleted the zonkmachine:baddata2 branch Jan 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.