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 Overflow in Fader.cpp #3425

Merged
merged 2 commits into from Mar 15, 2017

Conversation

Projects
None yet
2 participants
@Umcaruje
Member

Umcaruje commented Mar 14, 2017

Fixes #3420

I just implemented @zonkmachine's idea. I don't have the superdebug code set up so testing is appreciated.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Mar 14, 2017

Member

:-O

(gdb) 
(gdb) bt full
#0  0x00007ffff47d51ac in feraiseexcept (__excepts=4) at ../sysdeps/x86/fpu/bits/fenv.h:135
No locals.
#1  __log10f (x=0) at w_log10f.c:32
No locals.
#2  0x00000000005eee07 in ampToDbfs (amp=0) at /home/zonkmachine/builds/lmms/lmms/include/lmms_math.h:273
No locals.
#3  0x00000000005f067f in Fader::paintDBFSLevels (this=0x1b80360, ev=0x7fffffff8e60, painter=...)
    at /home/zonkmachine/builds/lmms/lmms/src/gui/widgets/Fader.cpp:382
        peak_L = 0
        persistentLeftPeakDBFS = -nan(0x7f8e74)
        width = 11
        center = 12
...

I didn't get this before but ampToDbfs (amp=0) is because m_fPeakValue_L and m_fPeakValue_R is initialised to 0.0f , here and here... More qMax?
I think this probably depends on #3381 for testing apart form the debug code, as envelope floating point errors will shut down LMMS immediately and after this you will get the Fader issue. However, I'm not yet confident with the envelope testing/fix but I'm still surprised by this result. I'll have to poke the envelope some more and get back to you on this.

Member

zonkmachine commented Mar 14, 2017

:-O

(gdb) 
(gdb) bt full
#0  0x00007ffff47d51ac in feraiseexcept (__excepts=4) at ../sysdeps/x86/fpu/bits/fenv.h:135
No locals.
#1  __log10f (x=0) at w_log10f.c:32
No locals.
#2  0x00000000005eee07 in ampToDbfs (amp=0) at /home/zonkmachine/builds/lmms/lmms/include/lmms_math.h:273
No locals.
#3  0x00000000005f067f in Fader::paintDBFSLevels (this=0x1b80360, ev=0x7fffffff8e60, painter=...)
    at /home/zonkmachine/builds/lmms/lmms/src/gui/widgets/Fader.cpp:382
        peak_L = 0
        persistentLeftPeakDBFS = -nan(0x7f8e74)
        width = 11
        center = 12
...

I didn't get this before but ampToDbfs (amp=0) is because m_fPeakValue_L and m_fPeakValue_R is initialised to 0.0f , here and here... More qMax?
I think this probably depends on #3381 for testing apart form the debug code, as envelope floating point errors will shut down LMMS immediately and after this you will get the Fader issue. However, I'm not yet confident with the envelope testing/fix but I'm still surprised by this result. I'll have to poke the envelope some more and get back to you on this.

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Mar 14, 2017

Member

Does this still happen on the Empty template only? or in general?

Member

Umcaruje commented Mar 14, 2017

Does this still happen on the Empty template only? or in general?

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Mar 14, 2017

Member

In general. This crash was new so I haven't looked into it that much.
No crash on just loading default.mpt or Empty.mpt .
Open mixer and click 'add channel', boom! It's the initialisation to zero that's fed to ampToDbfs().
I think either we initialise it to something over 0.0f , use the safe version that hands back -inf for 0.0f, or use the same qMax fix. Go with the qMax fix. I think it's most reasonable to go with the same fix.
tested, works... :-)

float const leftSpan = ampToDbfs( qMax<float>( 0.0001, m_fPeakValue_L ) ) - minDB;

Member

zonkmachine commented Mar 14, 2017

In general. This crash was new so I haven't looked into it that much.
No crash on just loading default.mpt or Empty.mpt .
Open mixer and click 'add channel', boom! It's the initialisation to zero that's fed to ampToDbfs().
I think either we initialise it to something over 0.0f , use the safe version that hands back -inf for 0.0f, or use the same qMax fix. Go with the qMax fix. I think it's most reasonable to go with the same fix.
tested, works... :-)

float const leftSpan = ampToDbfs( qMax<float>( 0.0001, m_fPeakValue_L ) ) - minDB;

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Mar 14, 2017

Member

What happens if you change all the instances to SafeAmpToDBFS? do you get the crash?

Member

Umcaruje commented Mar 14, 2017

What happens if you change all the instances to SafeAmpToDBFS? do you get the crash?

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Mar 14, 2017

Member

If you don't have time I can do it in the PR too

Member

Umcaruje commented Mar 14, 2017

If you don't have time I can do it in the PR too

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Mar 14, 2017

Member

What happens if you change all the instances to SafeAmpToDBFS? do you get the crash?

The function hands back -inf which creates a crash in the line after.

Program received signal SIGFPE, Arithmetic exception.
0x00000000005f06b2 in Fader::paintDBFSLevels (this=0x1b866d0, ev=0x7fffffffb470, painter=...)
    at /home/zonkmachine/builds/lmms/lmms/src/gui/widgets/Fader.cpp:383
383		int peak_L = height * leftSpan * fullSpanReciprocal;
Member

zonkmachine commented Mar 14, 2017

What happens if you change all the instances to SafeAmpToDBFS? do you get the crash?

The function hands back -inf which creates a crash in the line after.

Program received signal SIGFPE, Arithmetic exception.
0x00000000005f06b2 in Fader::paintDBFSLevels (this=0x1b866d0, ev=0x7fffffffb470, painter=...)
    at /home/zonkmachine/builds/lmms/lmms/src/gui/widgets/Fader.cpp:383
383		int peak_L = height * leftSpan * fullSpanReciprocal;
@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Mar 14, 2017

Member

If you don't have time I can do it in the PR too

👍

Member

zonkmachine commented Mar 14, 2017

If you don't have time I can do it in the PR too

👍

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Mar 14, 2017

Member

Let's see, I'll make all the values initalize at something other than 0

Member

Umcaruje commented Mar 14, 2017

Let's see, I'll make all the values initalize at something other than 0

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Mar 14, 2017

Member

Let's see, I'll make all the values initalize at something other than 0

I don't think there's any real point doing this other than to show that the value can't be 0 or lower. But it wont really work as a fix since we got negative values, probably from an overflowing int., in the original thread so you'll likely end up having to use qMax anyway.

Member

zonkmachine commented Mar 14, 2017

Let's see, I'll make all the values initalize at something other than 0

I don't think there's any real point doing this other than to show that the value can't be 0 or lower. But it wont really work as a fix since we got negative values, probably from an overflowing int., in the original thread so you'll likely end up having to use qMax anyway.

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Mar 14, 2017

Member

k then, qmax it is 😄

Member

Umcaruje commented Mar 14, 2017

k then, qmax it is 😄

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Mar 14, 2017

Member

@zonkmachine done. I didn't add the code to the min and max values, they're constants defined in the FXMixerView

Member

Umcaruje commented Mar 14, 2017

@zonkmachine done. I didn't add the code to the min and max values, they're constants defined in the FXMixerView

@zonkmachine zonkmachine merged commit 7be7784 into LMMS:master Mar 15, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment