Bug 78729 – RealtimeAnalyserNode does not consistently respect .minDecibels

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Chris Rogers]

Reported by Christian Corti:

Also a small inconsistency with the code:
 
Line 216:
    double dbMag = !linearValue ? MinDecibels : AudioUtilities::linearToDecibels(linearValue);
 
Line 242:
    double dbMag = !linearValue ? m_minDecibels : AudioUtilities::linearToDecibels(linearValue);
 
shouldn’t you use in both cases m_minDecibels instead of the getter?

[Raymond Toy]

I think both should be using MinDecibels.

[Chris Rogers]

But there's a minDecibels attribute.  Shouldn't we use the value the user wants?

[Raymond Toy]

(In reply to comment #2)
> But there's a minDecibels attribute.  Shouldn't we use the value the user wants?

Line 208 says 

const double MinDecibels = m_minDecibels.  

Isn't m_minDecibels the value the user wants?

[Chris Rogers]

(In reply to comment #3)
> (In reply to comment #2)
> > But there's a minDecibels attribute.  Shouldn't we use the value the user wants?
> 
> Line 208 says 
> 
> const double MinDecibels = m_minDecibels.  
> 
> Isn't m_minDecibels the value the user wants?

Sorry, I didn't look as closely as I should have.  I guess it's more of a consistency/style issue then.  I'd recommend changing to directly use m_minDecibels instead of having the unneeded MinDecibels...

It seems like there are some other style problems with this file (I noticed use of 0.0) so maybe we can fix this inconsistency and do some general style cleanup?

[Raymond Toy]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > But there's a minDecibels attribute.  Shouldn't we use the value the user wants?
> > 
> > Line 208 says 
> > 
> > const double MinDecibels = m_minDecibels.  
> > 
> > Isn't m_minDecibels the value the user wants?
> 
> Sorry, I didn't look as closely as I should have.  I guess it's more of a consistency/style issue then.  I'd recommend changing to directly use m_minDecibels instead of having the unneeded MinDecibels...

That's fine.  I had assumed line 208 existed so that the loop didn't have to look up m_minDecibels from the object every time.  But perhaps compilers are smart enough to know that m_minDecibels can't change inside the loop.

> 
> It seems like there are some other style problems with this file (I noticed use of 0.0) so maybe we can fix this inconsistency and do some general style cleanup?

Ok.

[Chris Rogers]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > But there's a minDecibels attribute.  Shouldn't we use the value the user wants?
> > > 
> > > Line 208 says 
> > > 
> > > const double MinDecibels = m_minDecibels.  
> > > 
> > > Isn't m_minDecibels the value the user wants?
> > 
> > Sorry, I didn't look as closely as I should have.  I guess it's more of a consistency/style issue then.  I'd recommend changing to directly use m_minDecibels instead of having the unneeded MinDecibels...
> 
> That's fine.  I had assumed line 208 existed so that the loop didn't have to look up m_minDecibels from the object every time.  But perhaps compilers are smart enough to know that m_minDecibels can't change inside the loop.

No, the compiler can never make that assumption (because another thread might be changing that value), so it will be slightly slower.  So for efficiency's sake, caching in a local variable is better.  In any case, they should be made consistent.

And just a style nit (my fault since I originally authored that code -- but know better now):

We use a lowercase character to start a constant, so:

    const double MinDecibels = m_minDecibels;

should be:

    const double minDecibels = m_minDecibels;

and same with "RangeScaleFactor" and any other such local constants.


> 
> > 
> > It seems like there are some other style problems with this file (I noticed use of 0.0) so maybe we can fix this inconsistency and do some general style cleanup?
> 
> Ok.

[Raymond Toy]

Created attachment 127448 [details]
Patch

[Chris Rogers]

Comment on attachment 127448 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127448&action=review

> Source/WebCore/webaudio/RealtimeAnalyser.cpp:237
> +        const double rangeScaleFactor = m_maxDecibels == minDecibels ? 1.0 : 1.0 / (m_maxDecibels - minDecibels);

This looks really peculiar and inconsistent:    (m_maxDecibels - minDecibels)
it seemed better as it was (m_maxDecibels - m_minDecibels)

we only need the efficiency inside the loop, so you could move the "minDecibels" definition down past this line

[Raymond Toy]

Created attachment 127450 [details]
Patch

[Raymond Toy]

Comment on attachment 127448 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127448&action=review

>> Source/WebCore/webaudio/RealtimeAnalyser.cpp:237
>> +        const double rangeScaleFactor = m_maxDecibels == minDecibels ? 1.0 : 1.0 / (m_maxDecibels - minDecibels);
> 
> This looks really peculiar and inconsistent:    (m_maxDecibels - minDecibels)
> it seemed better as it was (m_maxDecibels - m_minDecibels)
> 
> we only need the efficiency inside the loop, so you could move the "minDecibels" definition down past this line

Done.

[WebKit Review Bot]

Comment on attachment 127450 [details]
Patch

Rejecting attachment 127450 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ebKit/chromium/third_party/yasm/source/patched-yasm --revision 73761 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
47>At revision 73761.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/11540301

[Chris Rogers]

Comment on attachment 127450 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127450&action=review

> Source/WebCore/ChangeLog:12
> +        No new tests. (OOPS!)

Ray, please either remove this, or say these are just cosmetic changes and no new tests needed.

[Raymond Toy]

Created attachment 127508 [details]
Patch

[Raymond Toy]

Comment on attachment 127450 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127450&action=review

>> Source/WebCore/ChangeLog:12
>> +        No new tests. (OOPS!)
> 
> Ray, please either remove this, or say these are just cosmetic changes and no new tests needed.

Fixed.

[Chris Rogers]

Comment on attachment 127508 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127508&action=review

> Source/WebCore/webaudio/RealtimeAnalyser.cpp:236
> +        const double rangeScaleFactor = m_maxDecibels == m_minDecibels ? 1.0 : 1.0 / (m_maxDecibels - m_minDecibels);

Sorry, I just noticed 1.0

Please fix this and any other similar style issues

[Raymond Toy]

Created attachment 127598 [details]
Patch

[Chris Rogers]

Comment on attachment 127598 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127598&action=review

> Source/WebCore/webaudio/RealtimeAnalyser.cpp:181
> +    const double magnitudeScale = 1.0 / DefaultFFTSize;

You missed one here.

[Raymond Toy]

(In reply to comment #17)
> (From update of attachment 127598 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127598&action=review
> 
> > Source/WebCore/webaudio/RealtimeAnalyser.cpp:181
> > +    const double magnitudeScale = 1.0 / DefaultFFTSize;
> 
> You missed one here.

No, that would be incorrect since DefaultFFTSize is an integer.

[Raymond Toy]

(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 127598 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=127598&action=review
> > 
> > > Source/WebCore/webaudio/RealtimeAnalyser.cpp:181
> > > +    const double magnitudeScale = 1.0 / DefaultFFTSize;
> > 
> > You missed one here.
> 
> No, that would be incorrect since DefaultFFTSize is an integer.

Oh.  Should that be fftSize instead of DefaultFFTSize since it looks like we're doing FFTs of size fftSize and not DefaultFFTSize?

[Chris Rogers]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > (From update of attachment 127598 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=127598&action=review
> > > 
> > > > Source/WebCore/webaudio/RealtimeAnalyser.cpp:181
> > > > +    const double magnitudeScale = 1.0 / DefaultFFTSize;
> > > 
> > > You missed one here.
> > 
> > No, that would be incorrect since DefaultFFTSize is an integer.
> 
> Oh.  Should that be fftSize instead of DefaultFFTSize since it looks like we're doing FFTs of size fftSize and not DefaultFFTSize?

I see.  Yeah, I think this is a bug, but probably better to land this and fix this bug in a separate patch since it's an unrelated change.

[WebKit Review Bot]

Comment on attachment 127598 [details]
Patch

Clearing flags on attachment: 127598

Committed r108110: <http://trac.webkit.org/changeset/108110>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.