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

dBV is actually mislabeled dBFS #3095

Merged
merged 3 commits into from Nov 7, 2016
Merged

dBV is actually mislabeled dBFS #3095

merged 3 commits into from Nov 7, 2016

Conversation

Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Oct 28, 2016

So, after some looking around, I realised LMMS already used dBFS values but labeled them as dBV. according to wikipedia, dBV is a decibel that's relative to 1V of voltage, which does not quite fit in software.

dBFS on the other hand is relative to the maximum value of a digital full scale, which in our case is 1 in the linear scale, so the conclusion is that we are indeed actually using dBFS in LMMS.

Also @michaelgregorius also came to this conclusion in #2672 but probably never got around to this rename job.

@BaraMGB
Copy link
Contributor

BaraMGB commented Oct 28, 2016

Agreed!

qa->setCheckable( true );
qa->setChecked( ConfigManager::inst()->value( "app", "displaydbv" ).toInt() );
qa->setChecked( ConfigManager::inst()->value( "app", "displaydbfs" ).toInt() );
Copy link
Member

@tresf tresf Oct 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this going to need an upgrade()?

Copy link
Member Author

@Umcaruje Umcaruje Oct 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we even do upgrades for .lmmsrc.xml? If so, yes I can write an upgrade.

Copy link
Member

@tresf tresf Oct 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Albeit rather recently, yes we do.

Copy link
Member Author

@Umcaruje Umcaruje Oct 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll look into it.

Copy link
Member Author

@Umcaruje Umcaruje Oct 31, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tresf done per 5a7acd3

Copy link
Member

@tresf tresf Nov 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@tresf
Copy link
Member

tresf commented Nov 1, 2016

I don't know enough about dbfs to merge, but otherwise the PR is good to merge. I did notice some inconsistent whitespace formatting that this commit introduces. We should probably decide eventually which one of these is correct, but this PR isn't the place for that decision.

     if (foo) {
           // foo
+          whitepsace option 1
+whitespace option 2
           // bar
     }

@BaraMGB
Copy link
Contributor

BaraMGB commented Nov 1, 2016

For what I've learned in Wikipedia dB FS is the unit used by all digital audio signals. For analog signals is dbv used (voltage).

@Umcaruje
Copy link
Member Author

Umcaruje commented Nov 3, 2016

@tresf please comment on the code where you see that issue. This is a simple find/replace job, and I didn't touch the whitespace.

@@ -144,14 +154,19 @@ void ConfigManager::upgrade()
{
return;
}

Copy link
Member

@tresf tresf Nov 3, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace

Copy link
Member Author

@Umcaruje Umcaruje Nov 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, ok, I accidentaly added whitespace there. I'll remove it.

@tresf tresf merged commit c961896 into LMMS:master Nov 7, 2016
@Umcaruje Umcaruje deleted the dbvtodbfs branch Nov 10, 2016
int peak_R = height * rightSpan * fullSpanReciprocal;
QRect const drawRectR( center, height - peak_R, width, peak_R ); // Source and target are identical
painter.drawPixmap( drawRectR, *m_leds, drawRectR );

float const persistentRightPeakDBFS = ampToDbv(m_persistentPeak_R);
float const persistentRightPeakDBFS = ampToDbfs(m_persistentPeak_R);
Copy link
Member

@zonkmachine zonkmachine Mar 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ampToDbfs( qMax<float>( 0.0001, m_persistentPeak_R ) ) ?
@Umcaruje ampToDbfs( Must_be_greater_than_zero! )
I'm debugging with a new secret weapon and you've got a floating point error here. I triggered it by just starting lmms in gdb with @michaelgregorius super hack and when it loaded I just launched the Empty template. Boom! Stuck the qMax code in there and the issue went away. Care to take a look at this?

Program received signal SIGFPE, Arithmetic exception.
0x00007ffff47d51ac in feraiseexcept (__excepts=4) at ../sysdeps/x86/fpu/bits/fenv.h:135
135	../sysdeps/x86/fpu/bits/fenv.h: No such file or directory.
(gdb) bt
#0  0x00007ffff47d51ac in feraiseexcept (__excepts=4) at ../sysdeps/x86/fpu/bits/fenv.h:135
#1  __log10f (x=0) at w_log10f.c:32
#2  0x00000000005ec737 in ampToDbfs (amp=0) at /home/zonkmachine/builds/lmms/lmms/include/lmms_math.h:273
#3  0x00000000005ee1eb in Fader::paintDBFSLevels (this=0x1650ef0, ev=0x7fffffffb5f0, painter=...)
    at /home/zonkmachine/builds/lmms/lmms/src/gui/widgets/Fader.cpp:409

Copy link
Member Author

@Umcaruje Umcaruje Mar 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use safeAmpToDbfs here then? That has built in protection for zeores. I'll take a more thorough look soon.

Copy link
Member

@zonkmachine zonkmachine Mar 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use safeAmpToDbfs here then?

Yes. This looks a bit odd however: https://github.com/LMMS/lmms/blob/master/include/lmms_math.h#L251
amp == 0.0f There is no test here for negative input. I'm not up to date on the math here but this reference hints at an error for negative arguments.
http://www.cplusplus.com/reference/cmath/log10/

If x is negative, it causes a domain error.

Copy link
Member Author

@Umcaruje Umcaruje Mar 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but we never use negative values for linear amplitude though, so it shouldn't really be even considered. scratches head and the variables are initialized with the value of zero: https://github.com/Umcaruje/lmms/blob/e3aad60661b9581f12e3d91bfc2158618e1ca571/src/gui/widgets/Fader.cpp#L73-L74

Though you loaded the Empty template, and there are no instruments there. Does this happen with the default template?

Copy link
Member

@zonkmachine zonkmachine Mar 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm failing to reproduce that right now as I just get the crash right away. I think I might have mistaken and just thought I managed to launch something while it had already closed down business. Anyway.
Line 387 reads: float const persistentLeftPeakDBFS = ampToDbfs(m_persistentPeak_L);
The bt full readings for line 387 reads:

#3  0x00000000005ee070 in Fader::paintDBFSLevels (this=0x16556b0, ev=0x7fffffffbb90, painter=...)
    at /home/zonkmachine/builds/lmms/lmms/src/gui/widgets/Fader.cpp:387
        peak_L = 0
        persistentLeftPeakDBFS = -nan(0x7fbba4)
        width = 11
        center = 12
        maxDB = 9
        leftSpan = 0
        rightSpan = 4.59163468e-41
        drawRectR = {x1 = 0, y1 = 0, x2 = 0, y2 = 0}
        persistentPeak_R = 32767
        peak_R = -18464
        persistentRightPeakDBFS = 4.59163468e-41
        height = 116
        minDB = -42.0000038
        fullSpanReciprocal = 0.019607842
        drawRectL = {x1 = 0, y1 = 116, x2 = 10, y2 = 115}
        persistentPeak_L = -17520

Copy link
Member

@zonkmachine zonkmachine Mar 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int persistentPeak_L = height * (1 - (persistentLeftPeakDBFS - minDB) ...

It's overflowing.
unsigned int persistentPeak_L seem to be a fix. Not tested this a lot though.

Copy link
Member

@zonkmachine zonkmachine Mar 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I start up and press play directly it's cool. If I wait a couple of seconds and do nothing it crashes.

Copy link
Contributor

@michaelgregorius michaelgregorius Mar 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we rather open a new issue for this one?

Copy link
Member

@zonkmachine zonkmachine Mar 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right.

Copy link
Member

@zonkmachine zonkmachine Mar 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Continued here: #3420

@unfa
Copy link
Contributor

unfa commented Mar 8, 2017

I think that in the FX Mixer, it'd make more sense to label the fader positions using "dB" units not "dBFS", because then +6 dBFS sounds weird. +6dB is perfectly fine though.

sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
* Relabel "dBV" to "dBFS" in function names and GUI
* Write a ConfigManager upgrade for older versions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants