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

Allways remove infs/nans #3706

Merged
merged 3 commits into from Apr 15, 2018

Conversation

Projects
None yet
5 participants
@zonkmachine
Member

zonkmachine commented Jul 18, 2017

Brief

Testing to switch the inf/nan tests that are performed on export to be 'on' allways.
This seem to work well. For some 2.5% increase in cycles most of the NaN oriented bugs can be transformed from a project cutting out all sound at a specific point or from start, to just a moderate glitch.
I currently lack real test cases to assess this.


Verbose

I split the original PR into two separate commits and added a new one.

2fc1802 Turn on sanitizers in the FX-Mixer. This effectively prevents the whole projects to go silent because of one channel with a bad signal.

a85b02c Turn on sanitizers in the effect chains. This prevents bad data from shutting down the channel. Basically all NaN ( Warning! Will get loud without 47ac277 )

47ac277 As of the previious commit some dangerously large signals can come through the mix.

... has some amazing new noise going on with this PR

Fixed. Clamp values to, for the time being, +/-1.0f . I understand introducing hard limiting like this could trigger some but I see no reason to allow overly large signals. Don't other projects perform some form of clipping?
At this level a glitch will propagate through a reverb with long decay more like a distinct tick. A slight performance hit. +~0.2%.


Profiling

Test Project: Skiessi - Onion
Result: ~2.5% is spent in MixHelpers::sanitize() and MixHelpers::addSanitizedMultiplied()routines.

$ operf ./lmms

zonkmachine@zonkmachine:~/builds/lmms/lmms/build$ opreport --callgraph
CPU: AMD64 family15h, speed 3.4e+06 MHz (estimated)
Counted CPU_CLK_UNHALTED events (CPU Clocks not Halted) with a unit mask of 0x00 (No unit mask) count 100000
samples  %        image name               symbol name
-------------------------------------------------------------------------------
433771   14.1108  caps.so                  void Clip::one_cycle<&(store_func(float*, int, float, float))>(int)
  433771   100.000  caps.so                  void Clip::one_cycle<&(store_func(float*, int, float, float))>(int) [self]
-------------------------------------------------------------------------------
390387   12.6995  libQtGui.so.4.8.6        /usr/lib/x86_64-linux-gnu/libQtGui.so.4.8.6
  390387   100.000  libQtGui.so.4.8.6        /usr/lib/x86_64-linux-gnu/libQtGui.so.4.8.6 [self]

/////

61433     1.9870  lmms                     MixHelpers::sanitize(float (*) [2], int)
  61433    100.000  lmms                     MixHelpers::sanitize(float (*) [2], int) [self]
8048      0.2603  lmms                     MixHelpers::addSanitizedMultiplied(float (*) [2], float const (*) [2], float, int)
  8048     100.000  lmms                     MixHelpers::addSanitizedMultiplied(float (*) [2], float const (*) [2], float, int) [self]

fixes #1048 #3685

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Jul 18, 2017

The demo project in #3408, envelopeNaN.mmp.zip has some amazing new noise going on with this PR.

What happens is probably something like this: Some faults give you infinite values and these propagated through the sound chain will metamorphose into NaN. The NaN will make lmms go silent.
When we add the sanitizer the NaN/inf values are sorted out but some crap will get through in the form of short glitches around where the infs. would have been, because you'll have values leading up to/from the infinites. These will now pass straight through and if you have a reverb in there... Magic!!! ( Edit: turn volume down... )

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Jul 18, 2017

If you only turn on the Sanitizers in the FX-Mixer the sound will cut out on just the troubled channel and not affect the whole mix. This could be a better option.

@PhysSong

This comment has been minimized.

Member

PhysSong commented Aug 11, 2017

I see no reason to allow overly large signals.

Some projects use them as a intermediate signals. If we clip them, some projects may give different sound output.

And I have a question: what if an instrument generates NaN and its output goes to master FX channel directly? If there is something like master reverb, the result will be worse.

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Aug 11, 2017

Some projects use them as a intermediate signals. If we clip them, some projects may give different sound output.

Yes. This is a destructive PR in that sense. You could have a switch to allow higher signals. I don't know how many projects would be affected though. It would probably be more user dependent, the ones that has the habit of driving a large signal through the FX channel.

what if an instrument generates NaN and its output goes to master FX channel directly? If there is something like master reverb, the result will be worse.

Not worse but equally shitty I think. I've kept an eye open on the master channel but so far I haven't seen it acting up because of this in any way. And I've hit it with NaN intentionally. I'll try and come up with a more angry test... :)

@tresf

This comment has been minimized.

Member

tresf commented Nov 15, 2017

If this fixes the notorious bug where the mixer from goes silent, this comes with a lot of value. @zonkmachine @PhysSong Can we make a decision on this and merge?

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Nov 15, 2017

If this fixes the notorious bug where the mixer from goes silent

It does. I think it should be merged and I think perhaps 1.2 is a better target than master.

@PhysSong

This comment has been minimized.

Member

PhysSong commented Nov 17, 2017

A sample file may contain inf/NaN stuffs. In that case, this PR won't help to fix such issues.
I guess PlayHandle is also a candidate for sanitization, but I don't know how it will affect performance.

Anyway, Clipping signal at +/- 1.0f doesn't sound good for me. I think it should be some appropriate larger level(ex. 4.0f~10.0f) for intermediate signals and 1.0f(or slightly larger value) for master FX channel.

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Nov 17, 2017

I think it should be some appropriate larger level(ex. 4.0f~10.0f) for intermediate signal

Yes, I started out with 4.0f in the first version I pushed and in testing this 10.0f was about where the glitches would start to be too loud.

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Nov 17, 2017

A sample file may contain inf/NaN stuffs.

Interesting, I didn't think of this possibility. I went looking for some audio test suites containing various defective sound files for further testing but I couldn't find any. There are functions macros to generate problematic/borked floats like http://en.cppreference.com/w/cpp/numeric/math/nan so I may try to make some. Qt5 has functions to generate non-finite doubles but I haven't made the switch yet.

@PhysSong

This comment has been minimized.

Member

PhysSong commented Nov 17, 2017

@zonkmachine I have a .wav file with NaN value. I can provide it if you want.

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Nov 18, 2017

Yes please!

zonkmachine added some commits Jul 18, 2017

Allways remove infs/nans
When exporting a project lmms performs extra tests for bad data.
The tests are for infs and nans. Switching these tests on for all
occasions as the extra performance hit would be in the order of
only ~2% and the problems, when it hits the end user, are hard to
debug and/or work around.
Turn on effect chain sanity checks
This will check for infs/nans in between the effect units and will clear
up most cases where projects with bad data would simply cut out all sound
at one point or be all silent. It comes with a slight performance hit of
some 2% more cycles. The down side is that now instead of cutting out
at the point of bad data some large signals will get through that could
potentially hurt equipment.

Resolves: #1048, #3685

@zonkmachine zonkmachine changed the base branch from master to stable-1.2 Nov 21, 2017

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Nov 21, 2017

Changing base for the time being as my master branch isn't anywhere near sane.

Clip signal at +/-4.0f
Currently we shut out defunct signal values like inf/nan but we apart
from that we allow signals of any size. This will let large but short
shot noise through that can trigger especially reverb to dangerously
large sound levels even though they would pass largely unnoticed
without a feedback loop.
After testing for inf/nan we clamp the sound to +/-4.0f .
@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Nov 21, 2017

Anyway, Clipping signal at +/- 1.0f doesn't sound good for me. I think it should be some appropriate larger level(ex. 4.0f~10.0f) for intermediate signals

Changed to +/-4.0f

@PhysSong

This comment has been minimized.

Member

PhysSong commented Nov 22, 2017

nan2.wav.zip

@zonkmachine Here is a .wav file full with NaN. I made it using hex editor.

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Nov 22, 2017

@zonkmachine Here is a .wav file full with NaN. I made it using hex editor.

Super! Can you fix one with infinites too? :)

As you suspected an audio file to Master will sneak past the checks. As will any sound that doesn't have an FX unit in it's path.

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Nov 22, 2017

As will any sound that doesn't have an FX unit in it's path.

Actually, sound via a mixer channel is catching NaN even without an FX unit. It's only direct to Master that's an issue.

@unfa

This comment has been minimized.

Contributor

unfa commented Nov 23, 2017

I guess no one would want to use Infs/NaNs in a creative way. One might use a very loud intermediate signal to do something crazy and break effects in creative ways with that, but invalid float samples could probably be sanitized safely, without hurting creativity.

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Nov 24, 2017

One might use a very loud intermediate signal to do something crazy and break effects in creative ways

But how much louder? What levels do other studios, like Ardour and Bitwig, use for clipping?
With my hardware a 10.0 clipping level is a bit too audible for my taste when the glitches come. I think the VST specification use +/-1.0 for audio signals but I don't know if that also apply to hosts and if developers really stay within those levels.

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Nov 27, 2017

One might use a very loud intermediate signal to do something crazy and break effects in creative ways with that, but invalid float samples could probably be sanitized safely, without hurting creativity.

@unfa Here's the thing. The NaN's will cut the sound for a channel or even a whole mix. If you remove nan's/infinites, there will instead be an audible glitch in it's place where the sound is on it's way to/from infinitely high. This needs to be limited or it will give you a wall of noise when the glitch is followed by a reverb unit. Right now the suggested level for limiting is +/-4.0 . I've tried to trigger some of the glitches we've seen with ladspa pluggins in audacity and if they have a limit it seem to be much higher than that and I think we should err on being less generous in lmms.

Have you seen any similar glitches in other daw's? Ardour?

@zonkmachine zonkmachine removed the in progress label Dec 7, 2017

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Feb 23, 2018

Anyway, Clipping signal at +/- 1.0f doesn't sound good for me. I think it should be some appropriate larger level(ex. 4.0f~10.0f) for intermediate signals and 1.0f(or slightly larger value) for master FX channel.

Right now the suggested level for limiting is +/-4.0

As I've understood it, exporting will clip at +/-1.0 anyway so I think this should be the clipping level. I believe this is the signal level limit for VST's too.

A sample file may contain inf/NaN stuffs. In that case, this PR won't help to fix such issues.

I think this is a separate issue and should probably be fixed in SampleBuffer.cpp

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Mar 1, 2018

@zonkmachine Here is a .wav file full with NaN. I made it using hex editor.

OK. I made a project with nan2.wav

  1. As a Sample Track playing at the very beginning.
  2. A note playing at the 5th measure.

nanwav.mmp.zip

Without this fix the sound will cut out from when the nan ridden file starts sounding and you need to reload the project or bypass the reverb to get the sound back. With this fix the sound will cut out with the bad sound file playing but it will come back again as soon as the sample is done playing. So, a bit of an improvement but it still needs fixing on the sample side. SampleBuffer.cpp...

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Apr 13, 2018

Export issue: https://lmms.io/forum/viewtopic.php?f=7&t=27199
I think this should be merged. The last commit would take care of the issue above.

@tresf

This comment has been minimized.

Member

tresf commented Apr 14, 2018

I don't have much to offer this because I don't know the DSP code well enough to understand what this will break.

My initial concerns were that stuff like C*CLIP would have problems, but it doesn't appear to. I'm sure this is simply a lack of understanding where sanitize is being called. Anyway, I did two tracks with BEFORE and AFTER... In both cases, the tracks play fine. Both tracks make heavy use of effects plugins and to the naked ear, I can't hear a difference.

There are slight visual differences, but I believe these are directly attributed to nondeterministic plugins and effects, which will create the differences regardless of this PR.

@zonkmachine I think we can merge this, but it would be nice to have a real-world track in the wild to unit-test against (rather than the one that as made intentionally to test this scenario).

screen shot 2018-04-14 at 11 07 20 am

(I know this is not what unfa-Spoken normally looks like, the track has some playback issues on Mac that are unrelated to this PR)
image

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Apr 14, 2018

I don't have much to offer this because I don't know the DSP code well enough to understand what this will break.

We already do this sanitizing but only on export. The only difference you'd see should be:

  • No more NaN issues when not exporting.
  • No more noises from feeding delay/reverb with extreme signals.
  • Possible side effect from the forced clipping at +/-4.0 .

I think we can merge this

I'll go about it tomorrow.

@tresf

This comment has been minimized.

Member

tresf commented Apr 14, 2018

We already do this sanitizing but only on export.

That makes sense. I thought the code was already there for export. Ok, yes, in that case we definitely need to merge this.

@zonkmachine zonkmachine merged commit 07a23c4 into LMMS:stable-1.2 Apr 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@zonkmachine zonkmachine deleted the zonkmachine:sanitize branch Apr 15, 2018

@PhysSong

This comment has been minimized.

Member

PhysSong commented Apr 18, 2018

Possible side effect from the forced clipping at +/-4.0 .

It seems to cause clipping for one of our demo projects, Jousboxx - BuzzerBeater. I guess some channels in the project reach the level 5.0~6.0 and are affected.

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Apr 20, 2018

Jousboxx - BuzzerBeater

I don't know what to look for with this one as it's clipping with or without this PR. I exported as tracks and 11 out of 56 tracks was clipping but the rendered tracks are also clipped in the render process to +/-1.0 so it needs some more probing into which ones go above +/-4.0.
Do you think we should do something with this prior to rc6. Bump up the clip level to +/-10 ?

@Sawuare

This comment has been minimized.

Member

Sawuare commented Apr 21, 2018

Since MixHelpers::sanitize() is called in EffectChain::processAudioBuffer() when an effect chain is enabled, I think effect chains should always be disabled when they are empty, to avoid unneeded calls to MixHelpers::sanitize().

Edit: A better approach would be checking for m_effects.isEmpty() in:

if( m_enabledModel.value() == false )

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