Initialize value since calculateFinalValues may fail to do so.

Source/modules/webaudio/BiquadDSPKernel.cpp

 /*
  * Copyright (C) 2010, Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1.  Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 22 matching lines @@
 #include "wtf/Vector.h"
 
 namespace WebCore {
 
 // FIXME: As a recursive linear filter, depending on its parameters, a biquad filter can have
 // an infinite tailTime. In practice, Biquad filters do not usually (except for very high resonance values)
 // have a tailTime of longer than approx. 200ms. This value could possibly be calculated based on the
 // settings of the Biquad.
 static const double MaxBiquadDelayTime = 0.2;
 
-void BiquadDSPKernel::updateCoefficientsIfNecessary(bool useSmoothing, bool forceUpdate)
+void BiquadDSPKernel::updateCoefficientsIfNecessary()
 {
-    if (forceUpdate || biquadProcessor()->filterCoefficientsDirty()) {
-        double value1;
-        double value2;
+    if (biquadProcessor()->filterCoefficientsDirty()) {
+        double cutoffFrequency;
+        double Q;
         double gain;
         double detune; // in Cents
 
         if (biquadProcessor()->hasSampleAccurateValues()) {
-            value1 = biquadProcessor()->parameter1()->finalValue();
-            value2 = biquadProcessor()->parameter2()->finalValue();
+            cutoffFrequency = biquadProcessor()->parameter1()->finalValue();
+            Q = biquadProcessor()->parameter2()->finalValue();
             gain = biquadProcessor()->parameter3()->finalValue();
             detune = biquadProcessor()->parameter4()->finalValue();
-        } else if (useSmoothing) {
-            value1 = biquadProcessor()->parameter1()->smoothedValue();
-            value2 = biquadProcessor()->parameter2()->smoothedValue();
+        } else {
+            cutoffFrequency = biquadProcessor()->parameter1()->smoothedValue();
+            Q = biquadProcessor()->parameter2()->smoothedValue();
             gain = biquadProcessor()->parameter3()->smoothedValue();
             detune = biquadProcessor()->parameter4()->smoothedValue();
-        } else {
-            value1 = biquadProcessor()->parameter1()->value();
-            value2 = biquadProcessor()->parameter2()->value();
-            gain = biquadProcessor()->parameter3()->value();
-            detune = biquadProcessor()->parameter4()->value();
         }
 
-        // Convert from Hertz to normalized frequency 0 -> 1.
-        double nyquist = this->nyquist();
-        double normalizedFrequency = value1 / nyquist;
-
-        // Offset frequency by detune.
-        if (detune)
-            normalizedFrequency *= pow(2, detune / 1200);
-
-        // Configure the biquad with the new filter parameters for the appropriate type of filter.
-        switch (biquadProcessor()->type()) {
-        case BiquadProcessor::LowPass:
-            m_biquad.setLowpassParams(normalizedFrequency, value2);
-            break;
-
-        case BiquadProcessor::HighPass:
-            m_biquad.setHighpassParams(normalizedFrequency, value2);
-            break;
-
-        case BiquadProcessor::BandPass:
-            m_biquad.setBandpassParams(normalizedFrequency, value2);
-            break;
-
-        case BiquadProcessor::LowShelf:
-            m_biquad.setLowShelfParams(normalizedFrequency, gain);
-            break;
-
-        case BiquadProcessor::HighShelf:
-            m_biquad.setHighShelfParams(normalizedFrequency, gain);
-            break;
-
-        case BiquadProcessor::Peaking:
-            m_biquad.setPeakingParams(normalizedFrequency, value2, gain);
-            break;
-
-        case BiquadProcessor::Notch:
-            m_biquad.setNotchParams(normalizedFrequency, value2);
-            break;
-
-        case BiquadProcessor::Allpass:
-            m_biquad.setAllpassParams(normalizedFrequency, value2);
-            break;
-        }
+        updateCoefficients(cutoffFrequency, Q, gain, detune);
     }
 }
 
+void BiquadDSPKernel::updateCoefficients(double cutoffFrequency, double Q, double gain, double detune)
+{
+    // Convert from Hertz to normalized frequency 0 -> 1.
+    double nyquist = this->nyquist();
+    double normalizedFrequency = cutoffFrequency / nyquist;
+
+    // Offset frequency by detune.
+    if (detune)
+        normalizedFrequency *= pow(2, detune / 1200);
+
+    // Configure the biquad with the new filter parameters for the appropriate type of filter.
+    switch (biquadProcessor()->type()) {
+    case BiquadProcessor::LowPass:
+        m_biquad.setLowpassParams(normalizedFrequency, Q);
+        break;
+
+    case BiquadProcessor::HighPass:
+        m_biquad.setHighpassParams(normalizedFrequency, Q);
+        break;
+
+    case BiquadProcessor::BandPass:
+        m_biquad.setBandpassParams(normalizedFrequency, Q);
+        break;
+
+    case BiquadProcessor::LowShelf:
+        m_biquad.setLowShelfParams(normalizedFrequency, gain);
+        break;
+
+    case BiquadProcessor::HighShelf:
+        m_biquad.setHighShelfParams(normalizedFrequency, gain);
+        break;
+
+    case BiquadProcessor::Peaking:
+        m_biquad.setPeakingParams(normalizedFrequency, Q, gain);
+        break;
+
+    case BiquadProcessor::Notch:
+        m_biquad.setNotchParams(normalizedFrequency, Q);
+        break;
+
+    case BiquadProcessor::Allpass:
+        m_biquad.setAllpassParams(normalizedFrequency, Q);
+        break;
+    }
+}
+
 void BiquadDSPKernel::process(const float* source, float* destination, size_t framesToProcess)
 {
     ASSERT(source && destination && biquadProcessor());
 
     // Recompute filter coefficients if any of the parameters have changed.
     // FIXME: as an optimization, implement a way that a Biquad object can simply copy its internal filter coefficients from another Biquad object.
     // Then re-factor this code to only run for the first BiquadDSPKernel of each BiquadProcessor.
 
-    updateCoefficientsIfNecessary(true, false);
+
+    // The audio thread can't block on this lock; skip updating the coefficients for this block if
+    // necessary. We'll get them the next time around.
+    {
+        MutexTryLocker tryLocker(m_processLock);
+        if (tryLocker.locked())
+            updateCoefficientsIfNecessary();
+    }
 
     m_biquad.process(source, destination, framesToProcess);
 }
 
 void BiquadDSPKernel::getFrequencyResponse(int nFrequencies,
                                            const float* frequencyHz,
                                            float* magResponse,
                                            float* phaseResponse)
 {
     bool isGood = nFrequencies > 0 && frequencyHz && magResponse && phaseResponse;
     ASSERT(isGood);
     if (!isGood)
         return;
 
     Vector<float> frequency(nFrequencies);
 
     double nyquist = this->nyquist();
 
     // Convert from frequency in Hz to normalized frequency (0 -> 1),
     // with 1 equal to the Nyquist frequency.
     for (int k = 0; k < nFrequencies; ++k)
         frequency[k] = narrowPrecisionToFloat(frequencyHz[k] / nyquist);
 
-    // We want to get the final values of the coefficients and compute
-    // the response from that instead of some intermediate smoothed
-    // set. Forcefully update the coefficients even if they are not
-    // dirty.
+    double cutoffFrequency;
+    double Q;
+    double gain;
+    double detune; // in Cents
 
-    updateCoefficientsIfNecessary(false, true);
+    {
+        // Get a copy of the current biquad filter coefficients so we can update the biquad with
+        // these values. We need to synchronize with process() to prevent process() from updating
+        // the filter coefficients while we're trying to access them. The process will update it
+        // next time around.
+        //
+        // The biquad object here is not the same as the one being processed because a new biquad is

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

Please point out it's actually both the BiquadDSPKernel and its contained Biquad
objects that are different from the one being processed.

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

Done.

+        // created in BiquadProcessor::getFrequencyResponse.
+        MutexLocker processLocker(m_processLock);
+
+        cutoffFrequency = biquadProcessor()->parameter1()->value();
+        Q = biquadProcessor()->parameter2()->value();
+        gain = biquadProcessor()->parameter3()->value();
+        detune = biquadProcessor()->parameter4()->value();
+    }
+
+    updateCoefficients(cutoffFrequency, Q, gain, detune);

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

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]

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]

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:51]

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]

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]

Clarified this in the comments.

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

 
     m_biquad.getFrequencyResponse(nFrequencies, frequency.data(), magResponse, phaseResponse);
 }
 
 double BiquadDSPKernel::tailTime() const
 {
     return MaxBiquadDelayTime;
 }
 
 double BiquadDSPKernel::latencyTime() const
 {
     return 0;
 }
 
 } // namespace WebCore
 
 #endif // ENABLE(WEB_AUDIO)