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

NaN introduced with sample-exact fix in 1.0.91 #3685

Closed
zonkmachine opened this issue Jul 5, 2017 · 14 comments
Closed

NaN introduced with sample-exact fix in 1.0.91 #3685

zonkmachine opened this issue Jul 5, 2017 · 14 comments

Comments

@zonkmachine
Copy link
Member

zonkmachine commented Jul 5, 2017

NaN in a project file from this comment in PR #3313 submitted by @Umcaruje
Edit: I edited down the original song to a minimal project.
issue3685.mmp.zip

The project goes silent just as the BitInvader sounds for the first time in bar 5.
<lmms-project version="1.0" creator="LMMS" creatorversion="1.0.0" type="song">
It was created with lmms 1.0.0 and the project breaks in lmms-1.0.91

If you turn the volume knob on the BitInvader track or if you render the project before you hit play the bug isn't triggered. I've been trying to spot something like an undefined variable in there but have come up with nothing so far.

The bug was introduced in 23433a7

23433a70b577bc67f2836c0ae98a4dc48ae40f93 is the first bad commit
commit 23433a70b577bc67f2836c0ae98a4dc48ae40f93
Author: Vesa <contact.diizy@nbl.fi>
Date:   Mon Jun 30 01:59:18 2014 +0300

    Sample-exact models: improve
    - Remove the redundant hasSampleExactData() function. Instead, signal lack of s.ex.data by returning a NULL in valueBuffer()
    - Cache s.ex.buffers and only update them once per period
    - Make valueBuffer() in AutomatableModel threadsafe so that it can be used for NPH's sharing the same model
    - Add sample-exactness to instrumenttrack's vol & pan knobs
@musikBear
Copy link

There is more to this
Try this:

  • Open the FX-mixer.
  • Focus Master
  • rightclick and select 'remove unused channels'

Not only does the project sounds different, the issue with BitInvader is also gone. Somehow effects in unused channels are still influencing the playback. These channels are for presets thats no longer in the project !

issue3685_unusedChannelRemoved.zip

@zonkmachine
Copy link
Member Author

zonkmachine commented Jul 6, 2017

I don't see this on my side. The project issue3685_unusedChannelRemoved.zip still has this issue on my setup. I can add that turning the volume down a bit and saving/reloading or turning normalization off on the BitInvader, also removes the glitch.

@musikBear
Copy link

@zonkmachine

issue3685_unusedChannelRemoved.zip still has this issue on my setup

Interesting! could be that selected backend is a factor, mine is SDL.
Otherwise the bug could be random, that would indeed be bad. I will make a 10 times repeat test with your orr. file, and the 'remove-channel' change.

@musikBear
Copy link

No it is not a backend thing. I found a peculiarity that explain why i have sound where you had non.
First, here are the combos

cuts sound
Lim+plate
Lim+HP
Lim+plate(W/D = 0
Lim+HP (W/D = 0
Lim+ plate OFF
Lim+ HP OFF
Lim (W/D = .67 +HP+Plate
Lim (W/D = .0 +HP+Plate

no sound cut
Lim OFF +HP+Plate
Lim +HP+Plate After one play with Lim OFF!

Its the last one
Lim +HP+Plate After one play with Lim OFF!
that leads to the confusing results
I can reproduce this outcome every time, so i am sure that this is correct.
So
IF you start by disabling (OFF in mixer ) the Calf-limiter, and let the BITINWADER play through once, then after that one play-through, you can re-invoke the Calf-limiter (ON in mixer), and now the whole track can be played, without sound-cut -Eg NaN will not occur.
However!
If you save the file in this 'play-through-state' it will not preserve this ability. The sound will again cut-off at next project-reload. Eg the state is not saveable.
My first erroneous result simply came from a play-through with sound..
I cant say i understand why we have that behaviour, but maby someone can..

@zonkmachine
Copy link
Member Author

The actual NaN happens in the Limiter, yes... but that's not the issue. The limiter wasn't changed in the actual commit. Somewhere up-stream from the Limiter we are producing a value the limiter just can't handle very well and it goes bananas. It's the up-stream event I'm interested in here, not the limiter. The limiter has issues but I've failed to nail it down. It's an old package that isn't developed any more and probably our best bet there is to just wait for LV2 and the newer Calf packages to come true on LMMS.
However, the actual issue here more likely originates from our envelopes.

@zonkmachine
Copy link
Member Author

@michaelgregorius I suspect this may be related to what you comment on here.

@michaelgregorius
Copy link
Contributor

I have added the pull request #3687 to provide an option to debug floating point exception. Please refer to the pull request for more details.

@michaelgregorius
Copy link
Contributor

@zonkmachine That might be the case. These exception really need to be weeded out.

zonkmachine added a commit to zonkmachine/lmms that referenced this issue Jul 18, 2017
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: LMMS#1048, LMMS#3685
zonkmachine added a commit to zonkmachine/lmms that referenced this issue Jul 23, 2017
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: LMMS#1048, LMMS#3685
zonkmachine added a commit to zonkmachine/lmms that referenced this issue Aug 9, 2017
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: LMMS#1048, LMMS#3685
@PhysSong
Copy link
Member

PhysSong commented Aug 11, 2017

This behavior depends on too many arguments: playhead position before press play button, the wave shape in BitInvader, effects in the instrument, limiter's lookahead parameter, etc. So bisect may give wrong result for this.
Furthermore, if the bug was really introduced in 23433a7, it doesn't mean that commit is the direct cause of this bug.

There's nothing wrong with the input signal of the limiter in CH10, and its output contains a few number of(4~9 in my case) NaNs. In CH10, there are plate reverb and high pass filter. After these effects, the output signal becomes "the sea of NaN".
I tested it by FPE debugging and cross checked using manually inserted printf() so I think this result is reliable.

NaN is introduced here. If _peak has value 0, divide by 0 occurs.
I think it is a bug of calf limiter, not from LMMS core. With other limiter, there's no NaN anymore.

So I guess the bisect result is wrong and the real reason of this issue is a bug of calf limiter which occurs rarely.

@musikBear
Copy link

I think it is a bug of calf limiter, not from LMMS core. With other limiter, there's no NaN anymore.

@PhysSong -Good find! 👍

@zonkmachine
Copy link
Member Author

Closing this as a duplicate of #3859.

@zonkmachine
Copy link
Member Author

zonkmachine commented Oct 29, 2017

NaN is introduced here. If _peak has value 0, divide by 0 occurs.

@PhysSong Right. I hacked _peak above that line with: _peak = fmaxf( 0.0000001, _peak ); and that of course fixed the limiter right up. I think we should fix that for 1.2 . How about introducing an LMMS_MIN constant to use when you need a fmaxf or qMax( LMMS_MIN, Something ) to prevent 0? I've had use for it before and the FLT_MIN constants in float.h is a bit brutal. 1E-37 or smaller has no business in audio applications.

Edit: std::max is used in other places in audio_fx.cpp

@zonkmachine
Copy link
Member Author

How about introducing an LMMS_MIN

Here: https://github.com/LMMS/lmms/blob/stable-1.2/include/lmms_constants.h#L51

@zonkmachine
Copy link
Member Author

NaN is introduced here. If _peak has value 0, divide by 0 occurs.

I fixed that up as part of #3927

zonkmachine added a commit to zonkmachine/lmms that referenced this issue Nov 21, 2017
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: LMMS#1048, LMMS#3685
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants