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?
I think both should be using MinDecibels.
But there's a minDecibels attribute. Shouldn't we use the value the user wants?
(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?
(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?
(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.
(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.
Created attachment 127448 [details] Patch
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
Created attachment 127450 [details] Patch
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 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 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.
Created attachment 127508 [details] Patch
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 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
Created attachment 127598 [details] Patch
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.
(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.
(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?
(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 on attachment 127598 [details] Patch Clearing flags on attachment: 127598 Committed r108110: <http://trac.webkit.org/changeset/108110>
All reviewed patches have been landed. Closing bug.