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

Fix wrong value interpolation #3929

Merged
merged 1 commit into from Nov 3, 2017
Merged

Fix wrong value interpolation #3929

merged 1 commit into from Nov 3, 2017

Conversation

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Nov 1, 2017

Fixes #3859.

@PhysSong PhysSong requested a review from zonkmachine Nov 2, 2017
@PhysSong PhysSong added this to In Progress in Release LMMS 1.2.0-RC5 Nov 2, 2017
@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Nov 2, 2017

A first test. I'm not quite sure if this test tells anything but I cherry-picked 600e361 into master and tested it with the fpe debug option. The project I tested it with had a calf limiter just as the one in #3685/#3859. We had two separate fixes that will allow the project to play as normally, patching audio_fx.cpp or setting m_valueOld to '0' for new value buffers, and now this third proper fix for the value buffer. They all work and makes the failing projects pass but when you test for floating point errors, they will all come out crashing. The only difference is that with this latest effort it will play for two bars, but they will all crash with a similar backtrace.

master with 600e361
Crash after 2 bars
test project: fpeonstart.mmp.zip

Program received signal SIGFPE, Arithmetic exception.
[Switching to Thread 0x7fffb0d84700 (LWP 680)]
0x00007fffd96d433e in LadspaEffect::processAudioBuffer (this=0x10000f0020, _buf=0x1000011df0, _frames=256)
    at /home/zonkmachine/builds/lmms/lmms/plugins/LadspaEffect/LadspaEffect.cpp:245
245							out_sum += _buf[frame][channel] * _buf[frame][channel];
(gdb) bt full
#0  0x00007fffd96d433e in LadspaEffect::processAudioBuffer (this=0x10000f0020, _buf=0x1000011df0, _frames=256)
    at /home/zonkmachine/builds/lmms/lmms/plugins/LadspaEffect/LadspaEffect.cpp:245
        frame = 114
        pp = 0x19f9780
        port = 2
        proc = 0 '\000'
        frames = 256
        o_buf = 0x0
        sBuf = 0x7fffb0d83280
        channel = 0 '\000'
        w = 0,469999999
        is_running = 127
        out_sum = 0,00017636669005594019
        d = 0,529999971
#1  0x00000000005178d9 in EffectChain::processAudioBuffer (this=0x19ba3a0, _buf=0x1000011df0, _frames=256, hasInputNoise=true)
    at /home/zonkmachine/builds/lmms/lmms/src/core/EffectChain.cpp:216
        it = 0x19c4550
        exporting = false
        moreEffects = false
#2  0x0000000000582411 in AudioPort::processEffects (this=0x10000d0398) at /home/zonkmachine/builds/lmms/lmms/src/core/audio/AudioPort.cpp:98
        more = false
#3  0x0000000000582dd3 in AudioPort::doProcessing (this=0x10000d0398) at /home/zonkmachine/builds/lmms/lmms/src/core/audio/AudioPort.cpp:221
        fpp = 256
        me = false
@PhysSong
Copy link
Member Author

@PhysSong PhysSong commented Nov 2, 2017

Yes. I had the same issue before. That's an underflow, which seems harmless there.

@PhysSong
Copy link
Member Author

@PhysSong PhysSong commented Nov 2, 2017

You may remove disable the underflow check and test it again. Underflow can cause problems, but it isn't harmful for some situations such as level checking. In LMMS, inf and NaN are more important than denormals.

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Nov 2, 2017

That worked. Interesting, what shows that there are denormals involved here?

@PhysSong
Copy link
Member Author

@PhysSong PhysSong commented Nov 2, 2017

Interesting, what shows that there are denormals involved here?

I inserted printf to check the value of _buf[frame][channel]. It was small enough to generate denormals when squared.

Copy link
Member

@zonkmachine zonkmachine left a comment

I can't comment on the code, especially the valueBuffer() stuff, but it's been sweating in the debugger and works fine.

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Nov 3, 2017

In LMMS, inf and NaN are more important than denormals.

Maybe we should have the underflow check disabled by default for now then. The debugger is a bit too nervous right now.

@PhysSong
Copy link
Member Author

@PhysSong PhysSong commented Nov 3, 2017

The debugger is a bit too nervous right now.

Totally agree. In my case, LMMS fails to start with FPE debugging for some reason.

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Nov 3, 2017

Also, we do have some tweak to keep denormals under control.
disable_denormals(); as included in denormals.h
As I understand it numbers of this size are simply substituted with '0'.

LMMS fails to start with FPE debugging for some reason.

gdb gdb ?

@PhysSong PhysSong merged commit a3c7328 into LMMS:stable-1.2 Nov 3, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@PhysSong PhysSong deleted the PhysSong:initvaluebug branch Nov 3, 2017
@zonkmachine zonkmachine moved this from In Progress to Done in Release LMMS 1.2.0-RC5 Nov 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.