Bug 78729 - RealtimeAnalyserNode does not consistently respect .minDecibels
Summary: RealtimeAnalyserNode does not consistently respect .minDecibels
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Raymond Toy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-15 12:26 PST by Chris Rogers
Modified: 2022-02-28 00:06 PST (History)
2 users (show)

See Also:


Attachments
Patch (5.78 KB, patch)
2012-02-16 14:50 PST, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (5.70 KB, patch)
2012-02-16 15:07 PST, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (5.72 KB, patch)
2012-02-16 20:32 PST, Raymond Toy
no flags Details | Formatted Diff | Diff
Patch (6.73 KB, patch)
2012-02-17 08:54 PST, Raymond Toy
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Rogers 2012-02-15 12:26:23 PST
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?
Comment 1 Raymond Toy 2012-02-15 13:33:20 PST
I think both should be using MinDecibels.
Comment 2 Chris Rogers 2012-02-15 17:33:46 PST
But there's a minDecibels attribute.  Shouldn't we use the value the user wants?
Comment 3 Raymond Toy 2012-02-15 21:16:44 PST
(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?
Comment 4 Chris Rogers 2012-02-15 22:41:21 PST
(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?
Comment 5 Raymond Toy 2012-02-16 08:47:30 PST
(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.
Comment 6 Chris Rogers 2012-02-16 11:47:56 PST
(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.
Comment 7 Raymond Toy 2012-02-16 14:50:18 PST
Created attachment 127448 [details]
Patch
Comment 8 Chris Rogers 2012-02-16 14:56:34 PST
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
Comment 9 Raymond Toy 2012-02-16 15:07:05 PST
Created attachment 127450 [details]
Patch
Comment 10 Raymond Toy 2012-02-16 15:07:50 PST
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.
Comment 11 WebKit Review Bot 2012-02-16 18:34:18 PST
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
Comment 12 Chris Rogers 2012-02-16 19:27:21 PST
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.
Comment 13 Raymond Toy 2012-02-16 20:32:22 PST
Created attachment 127508 [details]
Patch
Comment 14 Raymond Toy 2012-02-16 20:36:51 PST
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.
Comment 15 Chris Rogers 2012-02-16 22:14:56 PST
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
Comment 16 Raymond Toy 2012-02-17 08:54:48 PST
Created attachment 127598 [details]
Patch
Comment 17 Chris Rogers 2012-02-17 10:35:12 PST
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.
Comment 18 Raymond Toy 2012-02-17 10:50:40 PST
(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.
Comment 19 Raymond Toy 2012-02-17 10:53:49 PST
(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?
Comment 20 Chris Rogers 2012-02-17 11:57:07 PST
(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.
Comment 21 WebKit Review Bot 2012-02-17 12:57:18 PST
Comment on attachment 127598 [details]
Patch

Clearing flags on attachment: 127598

Committed r108110: <http://trac.webkit.org/changeset/108110>
Comment 22 WebKit Review Bot 2012-02-17 12:57:23 PST
All reviewed patches have been landed.  Closing bug.