Guarantee a thread safety when accessing source location info in pannerNode.

Source/modules/webaudio/PannerNode.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 40 matching lines @@
     , m_panningModel(Panner::PanningModelHRTF)
     , m_position(0, 0, 0)
     , m_orientation(1, 0, 0)
     , m_velocity(0, 0, 0)
     , m_cachedPosition(0, 0, 0)
     , m_cachedOrientation(1, 0, 0)
     , m_cachedVelocity(0, 0, 0)
     , m_lastGain(-1.0)
     , m_cachedAzimuth(0)
     , m_cachedElevation(0)
-    , m_cachedDistanceConeGain(0)

[KhNo, 2014/03/13 09:45:11]

Generally, default gain value should be 1.0f, Not 0.
It is too small nit to divide CL.
So, it is included this patch.

+    , m_cachedDistanceConeGain(1.0f)
     , m_cachedDopplerRate(1)
     , m_connectionCount(0)
 {
     // Load the HRTF database asynchronously so we don't block the Javascript thread while creating the HRTF database.
     // The HRTF panner will return zeroes until the database is loaded.
     m_hrtfDatabaseLoader = HRTFDatabaseLoader::createAndLoadAsynchronouslyIfNecessary(context->sampleRate());
 
     ScriptWrappable::init(this);
     addInput(adoptPtr(new AudioNodeInput(this)));
     addOutput(adoptPtr(new AudioNodeOutput(this, 2)));
@@ skipping 44 matching lines @@
         destination->zero();
         return;
     }
 
     AudioBus* source = input(0)->bus();
     if (!source) {
         destination->zero();
         return;
     }
 
-    // HRTFDatabase should be loaded before proceeding for offline audio context when panningModel() is "HRTF".
-    if (panningModel() == "HRTF" && !m_hrtfDatabaseLoader->isLoaded()) {
-        if (context()->isOfflineContext()) {
-            m_hrtfDatabaseLoader->waitForLoaderThreadCompletion();
-        } else {
-            destination->zero();
-            return;
+    // The audio thread can't block on this lock, so we call tryLock() instead.
+    MutexTryLocker tryLocker(m_processLock);
+    if (tryLocker.locked()) {
+        // HRTFDatabase should be loaded before proceeding for offline audio context when panningModel() is "HRTF".
+        if (panningModel() == "HRTF" && !m_hrtfDatabaseLoader->isLoaded()) {
+            if (context()->isOfflineContext()) {
+                m_hrtfDatabaseLoader->waitForLoaderThreadCompletion();
+            } else {
+                destination->zero();
+                return;
+            }
         }
-    }
 
-    // The audio thread can't block on this lock, so we call tryLock() instead.
-    MutexTryLocker tryLocker(m_pannerLock);
-    if (tryLocker.locked()) {
         // Apply the panning effect.
         double azimuth;
         double elevation;
         azimuthElevation(&azimuth, &elevation);
 
         m_panner->pan(azimuth, elevation, source, destination, framesToProcess);
 
         // Get the distance and cone gain.
         float totalGain = distanceConeGain();
 
         // Snap to desired gain at the beginning.
         if (m_lastGain == -1.0)
             m_lastGain = totalGain;
 
         // Apply gain in-place with de-zippering.
         destination->copyWithGainFrom(*destination, &m_lastGain, totalGain);
 
         // Update the cached listener in case listener has moved.
         updateCachedListener();
         // Now update the cached source location in case the source has changed.
         updateCachedSourceLocationInfo();
     } else {
-        // Too bad - The tryLock() failed. We must be in the middle of changing the panner.
+        // Too bad - The tryLock() failed.
+        // We must be in the middle of changing the one of attributes which must be locked during process.

[Raymond Toy, 2014/03/13 21:37:46]

This comment is not very helpful. Be more explicit about what might be changing.

[KhNo, 2014/03/14 10:39:58]

Done.

         destination->zero();
     }
 }
 
 void PannerNode::initialize()
 {
     if (isInitialized())
         return;
 
     m_panner = Panner::create(m_panningModel, sampleRate(), m_hrtfDatabaseLoader.get());
@@ skipping 38 matching lines @@
         ASSERT_NOT_REACHED();
 }
 
 bool PannerNode::setPanningModel(unsigned model)
 {
     switch (model) {
     case EQUALPOWER:
     case HRTF:
         if (!m_panner.get() || model != m_panningModel) {
             // This synchronizes with process().
-            MutexLocker processLocker(m_pannerLock);
+            MutexLocker processLocker(m_processLock);
 
             OwnPtr<Panner> newPanner = Panner::create(model, sampleRate(), m_hrtfDatabaseLoader.get());
             m_panner = newPanner.release();
             m_panningModel = model;
         }
         break;
     default:
         return false;
     }
 
     return true;
 }
 
 void PannerNode::setPosition(float x, float y, float z)
 {
-    // FIXME : consider thread safety about m_position in audio thread.
-    // See http://crbugs.com/350583.
     FloatPoint3D position = FloatPoint3D(x, y, z);
 
     if (m_position == position)
         return;
 
+    // This synchronizes with process().
+    MutexLocker processLocker(m_processLock);
+
     m_position = position;
 }
 
 void PannerNode::setOrientation(float x, float y, float z)
 {
-    // FIXME : consider thread safety about m_orientation in audio thread.
-    // See http://crbugs.com/350583.
     FloatPoint3D orientation = FloatPoint3D(x, y, z);
 
     if (m_orientation == orientation)
         return;
 
+    // This synchronizes with process().
+    MutexLocker processLocker(m_processLock);
+
     m_orientation = orientation;
 }
 
 void PannerNode::setVelocity(float x, float y, float z)
 {
-    // FIXME : consider thread safety about m_velocity in audio thread.
-    // See http://crbugs.com/350583.
     FloatPoint3D velocity = FloatPoint3D(x, y, z);
 
     if (m_velocity == velocity)
         return;
 
+    // This synchronizes with process().
+    MutexLocker processLocker(m_processLock);
+
     m_velocity = velocity;
 }
 
 String PannerNode::distanceModel() const
 {
     switch (const_cast<PannerNode*>(this)->m_distanceEffect.model()) {
     case DistanceEffect::ModelLinear:
         return "linear";
     case DistanceEffect::ModelInverse:
         return "inverse";
@@ skipping 16 matching lines @@
     else
         ASSERT_NOT_REACHED();
 }
 
 bool PannerNode::setDistanceModel(unsigned model)
 {
     switch (model) {
     case DistanceEffect::ModelLinear:
     case DistanceEffect::ModelInverse:
     case DistanceEffect::ModelExponential:
-        m_distanceEffect.setModel(static_cast<DistanceEffect::ModelType>(model), true);
+        if (model != m_distanceEffect.model()) {
+            // This synchronizes with process().
+            MutexLocker processLocker(m_processLock);
+            m_distanceEffect.setModel(static_cast<DistanceEffect::ModelType>(model), true);
+        }
         break;
     default:
         return false;
     }
 
     return true;
 }
 
 void PannerNode::calculateAzimuthElevation(double* outAzimuth, double* outElevation)
 {
@@ skipping 50 matching lines @@
         elevation = 180.0 - elevation;
     else if (elevation < -90.0)
         elevation = -180.0 - elevation;
 
     if (outAzimuth)
         *outAzimuth = azimuth;
     if (outElevation)
         *outElevation = elevation;
 }
 
-
 double PannerNode::calculateDopplerRate()
 {
     double dopplerShift = 1.0;
     double dopplerFactor = listener()->dopplerFactor();
 
     if (dopplerFactor > 0.0) {
         double speedOfSound = listener()->speedOfSound();
 
         const FloatPoint3D &sourceVelocity = m_velocity;
         const FloatPoint3D &listenerVelocity = listener()->velocity();
@@ skipping 44 matching lines @@
 
     double coneGain = m_coneEffect.gain(m_position, m_orientation, listenerPosition);
 
     m_coneGain->setValue(static_cast<float>(coneGain));
 
     return float(distanceGain * coneGain);
 }
 
 void PannerNode::azimuthElevation(double* outAzimuth, double* outElevation)
 {
+    ASSERT(context()->isAudioThread());
+
     if (isAzimuthElevationDirty())
         calculateAzimuthElevation(&m_cachedAzimuth, &m_cachedElevation);
 
     *outAzimuth = m_cachedAzimuth;
     *outElevation = m_cachedElevation;
 }
 
 double PannerNode::dopplerRate()
 {
+    ASSERT(context()->isAudioThread());
+
     if (isDopplerRateDirty())
         m_cachedDopplerRate = calculateDopplerRate();
 
     return m_cachedDopplerRate;
 }
 
 float PannerNode::distanceConeGain()
 {
+    ASSERT(context()->isAudioThread());
+
     if (isDistanceConeGainDirty())
         m_cachedDistanceConeGain = calculateDistanceConeGain();
 
     return m_cachedDistanceConeGain;
 }
 
 bool PannerNode::isAzimuthElevationDirty()
 {
     // Do a quick test and return if possible.
     if (m_cachedPosition != m_position)
@@ skipping 61 matching lines @@
                     visitedNodes.set(connectedNode, true);
                     notifyAudioSourcesConnectedToNode(connectedNode, visitedNodes); // recurse
                 }
             }
         }
     }
 }
 
 void PannerNode::updateCachedListener()
 {
+    ASSERT(context()->isAudioThread());
+
     m_cachedListener->setPosition(listener()->position());
     m_cachedListener->setOrientation(listener()->orientation());
     m_cachedListener->setUpVector(listener()->upVector());
     m_cachedListener->setVelocity(listener()->velocity());
     m_cachedListener->setDopplerFactor(listener()->dopplerFactor());
     m_cachedListener->setSpeedOfSound(listener()->speedOfSound());
 }
 
 void PannerNode::updateCachedSourceLocationInfo()
 {
+    ASSERT(context()->isAudioThread());
+
     m_cachedPosition = m_position;
     m_cachedOrientation = m_orientation;
     m_cachedVelocity = m_velocity;
 }
 
 } // namespace WebCore
 
 #endif // ENABLE(WEB_AUDIO)