Initialize value since calculateFinalValues may fail to do so.

Initialize value since calculateFinalValues may fail to do so.

Fix threading issue where updateCoefficientsIfNecessary was not always
called from the audio thread. This causes the value not to be
initialized.

Thus,

o Initialize the variable to some value, just in case.
o Split updateCoefficientsIfNecessary into two functions with the code
  that sets the coefficients pulled out in to the new function
  updateCoefficients.
o Simplify updateCoefficientsIfNecessary since useSmoothing was always
  true, and forceUpdate is not longer needed.
o Add process lock to prevent the audio thread from updating the
  coefficients while they are being read in the main thread. The audio
  thread will update them the next time around.
o Make getFrequencyResponse set the lock while reading the
  coefficients of the biquad in preparation for computing the
  frequency response.

BUG=389219
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177250

[Raymond Toy, 2014-06-30 16:51:05]

PTAL

[Ken Russell (switch to Gerrit), 2014-06-30 17:48:56]

https://codereview.chromium.org/354213002/diff/1/Source/modules/webaudio/Biqu...
File Source/modules/webaudio/BiquadDSPKernel.cpp (right):

https://codereview.chromium.org/354213002/diff/1/Source/modules/webaudio/Biqu...
Source/modules/webaudio/BiquadDSPKernel.cpp:173:
updateCoefficients(cutoffFrequency, Q, gain, detune);
Does this call to updateCoefficients need to be covered by m_processLock? In
process(), above, the call to updateCoefficientsIfNecessary and from there to
updateCoefficients is covered by the lock.

[Raymond Toy, 2014-06-30 18:06:11]

https://codereview.chromium.org/354213002/diff/1/Source/modules/webaudio/Biqu...
File Source/modules/webaudio/BiquadDSPKernel.cpp (right):

https://codereview.chromium.org/354213002/diff/1/Source/modules/webaudio/Biqu...
Source/modules/webaudio/BiquadDSPKernel.cpp:173:
updateCoefficients(cutoffFrequency, Q, gain, detune);
On 2014/06/30 17:48:55, Ken Russell wrote:
> Does this call to updateCoefficients need to be covered by m_processLock? In
> process(), above, the call to updateCoefficientsIfNecessary and from there to
> updateCoefficients is covered by the lock.

It shouldn't be needed because updateCoefficients here is updating the biquad
used for the freqency response, which is not the same as the one in process().

[Ken Russell (switch to Gerrit), 2014-06-30 18:26:38]

https://codereview.chromium.org/354213002/diff/1/Source/modules/webaudio/Biqu...
File Source/modules/webaudio/BiquadDSPKernel.cpp (right):

https://codereview.chromium.org/354213002/diff/1/Source/modules/webaudio/Biqu...
Source/modules/webaudio/BiquadDSPKernel.cpp:173:
updateCoefficients(cutoffFrequency, Q, gain, detune);
On 2014/06/30 18:06:11, Raymond Toy wrote:
> On 2014/06/30 17:48:55, Ken Russell wrote:
> > Does this call to updateCoefficients need to be covered by m_processLock? In
> > process(), above, the call to updateCoefficientsIfNecessary and from there
to
> > updateCoefficients is covered by the lock.
> 
> It shouldn't be needed because updateCoefficients here is updating the biquad
> used for the freqency response, which is not the same as the one in process().

Both BiquadDSPKernel::getFrequencyResponse and BiquadDSPKernel::process end up
calling BiquadDSPKernel::updateCoefficients which mutates
BiquadDSPKernel::m_biquad. What am I missing?

[Raymond Toy, 2014-06-30 18:44:52]

https://codereview.chromium.org/354213002/diff/1/Source/modules/webaudio/Biqu...
File Source/modules/webaudio/BiquadDSPKernel.cpp (right):

https://codereview.chromium.org/354213002/diff/1/Source/modules/webaudio/Biqu...
Source/modules/webaudio/BiquadDSPKernel.cpp:173:
updateCoefficients(cutoffFrequency, Q, gain, detune);
On 2014/06/30 18:26:38, Ken Russell wrote:
> On 2014/06/30 18:06:11, Raymond Toy wrote:
> > On 2014/06/30 17:48:55, Ken Russell wrote:
> > > Does this call to updateCoefficients need to be covered by m_processLock?
In
> > > process(), above, the call to updateCoefficientsIfNecessary and from there
> to
> > > updateCoefficients is covered by the lock.
> > 
> > It shouldn't be needed because updateCoefficients here is updating the
biquad
> > used for the freqency response, which is not the same as the one in
process().
> 
> Both BiquadDSPKernel::getFrequencyResponse and BiquadDSPKernel::process end up
> calling BiquadDSPKernel::updateCoefficients which mutates
> BiquadDSPKernel::m_biquad. What am I missing?

They are different m_biquad objects.  The m_biquad for getFrequencyResponse is
specially created in BiquadProcessor::getFrequencyResponse. The m_biquad in
process() is the m_biquad actually associated with the biquad in
BiquadFilterNode.
(Yes, I find this confusing too.)

[Ken Russell (switch to Gerrit), 2014-06-30 19:09:54]

LGTM

https://codereview.chromium.org/354213002/diff/1/Source/modules/webaudio/Biqu...
File Source/modules/webaudio/BiquadDSPKernel.cpp (right):

https://codereview.chromium.org/354213002/diff/1/Source/modules/webaudio/Biqu...
Source/modules/webaudio/BiquadDSPKernel.cpp:163: // The biquad object here is
not the same as the one being processed because a new biquad is
Please point out it's actually both the BiquadDSPKernel and its contained Biquad
objects that are different from the one being processed.

https://codereview.chromium.org/354213002/diff/1/Source/modules/webaudio/Biqu...
Source/modules/webaudio/BiquadDSPKernel.cpp:173:
updateCoefficients(cutoffFrequency, Q, gain, detune);
On 2014/06/30 18:44:51, Raymond Toy wrote:
> On 2014/06/30 18:26:38, Ken Russell wrote:
> > On 2014/06/30 18:06:11, Raymond Toy wrote:
> > > On 2014/06/30 17:48:55, Ken Russell wrote:
> > > > Does this call to updateCoefficients need to be covered by
m_processLock?
> In
> > > > process(), above, the call to updateCoefficientsIfNecessary and from
there
> > to
> > > > updateCoefficients is covered by the lock.
> > > 
> > > It shouldn't be needed because updateCoefficients here is updating the
> biquad
> > > used for the freqency response, which is not the same as the one in
> process().
> > 
> > Both BiquadDSPKernel::getFrequencyResponse and BiquadDSPKernel::process end
up
> > calling BiquadDSPKernel::updateCoefficients which mutates
> > BiquadDSPKernel::m_biquad. What am I missing?
> 
> They are different m_biquad objects.  The m_biquad for getFrequencyResponse is
> specially created in BiquadProcessor::getFrequencyResponse. The m_biquad in
> process() is the m_biquad actually associated with the biquad in
> BiquadFilterNode.
> (Yes, I find this confusing too.)

It's not only different m_biquad (Biquad) objects, it's different
BiquadDSPKernel objects, both pointing to the same BiquadProcessor. One is for
actually processing and one is for querying the frequency response. Could you
please clarify this in the comment above?

It would be good if this could be made less confusing.

[Raymond Toy, 2014-06-30 20:45:40]

Thanks for the review!

https://codereview.chromium.org/354213002/diff/1/Source/modules/webaudio/Biqu...
File Source/modules/webaudio/BiquadDSPKernel.cpp (right):

https://codereview.chromium.org/354213002/diff/1/Source/modules/webaudio/Biqu...
Source/modules/webaudio/BiquadDSPKernel.cpp:163: // The biquad object here is
not the same as the one being processed because a new biquad is
On 2014/06/30 19:09:54, Ken Russell wrote:
> Please point out it's actually both the BiquadDSPKernel and its contained
Biquad
> objects that are different from the one being processed.

Done.

https://codereview.chromium.org/354213002/diff/1/Source/modules/webaudio/Biqu...
Source/modules/webaudio/BiquadDSPKernel.cpp:173:
updateCoefficients(cutoffFrequency, Q, gain, detune);
On 2014/06/30 19:09:54, Ken Russell wrote:
> On 2014/06/30 18:44:51, Raymond Toy wrote:
> > On 2014/06/30 18:26:38, Ken Russell wrote:
> > > On 2014/06/30 18:06:11, Raymond Toy wrote:
> > > > On 2014/06/30 17:48:55, Ken Russell wrote:
> > > > > Does this call to updateCoefficients need to be covered by
> m_processLock?
> > In
> > > > > process(), above, the call to updateCoefficientsIfNecessary and from
> there
> > > to
> > > > > updateCoefficients is covered by the lock.
> > > > 
> > > > It shouldn't be needed because updateCoefficients here is updating the
> > biquad
> > > > used for the freqency response, which is not the same as the one in
> > process().
> > > 
> > > Both BiquadDSPKernel::getFrequencyResponse and BiquadDSPKernel::process
end
> up
> > > calling BiquadDSPKernel::updateCoefficients which mutates
> > > BiquadDSPKernel::m_biquad. What am I missing?
> > 
> > They are different m_biquad objects.  The m_biquad for getFrequencyResponse
is
> > specially created in BiquadProcessor::getFrequencyResponse. The m_biquad in
> > process() is the m_biquad actually associated with the biquad in
> > BiquadFilterNode.
> > (Yes, I find this confusing too.)
> 
> It's not only different m_biquad (Biquad) objects, it's different
> BiquadDSPKernel objects, both pointing to the same BiquadProcessor. One is for
> actually processing and one is for querying the frequency response. Could you
> please clarify this in the comment above?
> 
> It would be good if this could be made less confusing.

Clarified this in the comments.

Yes, it shouldn't this confusing:  crbug.com/390266.

[Raymond Toy, 2014-06-30 20:46:36]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2014-06-30 20:47:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rtoy@chromium.org/354213002/20001

[commit-bot: I haz the power, 2014-06-30 21:49:20]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/14667)

[commit-bot: I haz the power, 2014-06-30 23:29:04]

Change committed as 177250