PannerNode implements custom ProcessIfNecessary

When setting the panning model, the Panner object gets replaced with
a new Panner for the new panning model.  This happens with the process
lock.  However, in AudioNode::ProcessIfNecessary, there is a call to
PropagatesSilence() that calls TailTime() and LatencyTime() in the
audio thread and this is not covered by the process lock. Hence
TailTime() may get called on a destroyed Panner object.

So let PannerNode implement its own ProcessIfNecessary method to
add a process lock to protect PropagatesSilence (and thus
TailTime/LatencyTime). This requires making ProcessIfNecessary virtual,
and requires making a few member variables protected instead of
private.


Bug: 1023817
Change-Id: Icc850d9f8b952814c40c29912cdec3d25d6f4ae2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1924718Reviewed-by: Hongchan Choi <hongchan@chromium.org>
Commit-Queue: Raymond Toy <rtoy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#717029}

third_party/blink/renderer/modules/webaudio/audio_node.cc

@@ -45,13 +45,13 @@ namespace blink {
 AudioHandler::AudioHandler(NodeType node_type,
                            AudioNode& node,
                            float sample_rate)
-    : is_initialized_(false),
+    : last_processing_time_(-1),
+      last_non_silent_time_(0),
+      is_initialized_(false),
       node_type_(kNodeTypeUnknown),
       node_(&node),
       context_(node.context()),
       deferred_task_handler_(&context_->GetDeferredTaskHandler()),
-      last_processing_time_(-1),
-      last_non_silent_time_(0),
       connection_ref_count_(0),
       is_disabled_(false),
       channel_count_(2) {

third_party/blink/renderer/modules/webaudio/audio_node.h

@@ -180,7 +180,7 @@ class MODULES_EXPORT AudioHandler : public ThreadSafeRefCounted<AudioHandler> {
   // will only process once per rendering time quantum even if it's called
   // repeatedly.  This handles the case of "fanout" where an output is connected
   // to multiple AudioNode inputs.  Called from context's audio thread.
-  void ProcessIfNecessary(uint32_t frames_to_process);
+  virtual void ProcessIfNecessary(uint32_t frames_to_process);
 
   // Called when a new connection has been made to one of our inputs or the
   // connection number of channels has changed.  This potentially gives us
@@ -267,6 +267,14 @@ class MODULES_EXPORT AudioHandler : public ThreadSafeRefCounted<AudioHandler> {
   // Force all inputs to take any channel interpretation changes into account.
   void UpdateChannelsForInputs();
 
+  // The last time (context time) that his handler ran its Process() method.
+  // For each render quantum, we only want to process just once to handle fanout
+  // of this handler.
+  double last_processing_time_;
+
+  // The last time (context time) when this node did not have silent inputs.
+  double last_non_silent_time_;
+
  private:
   void SetNodeType(NodeType);
 
@@ -289,9 +297,6 @@ class MODULES_EXPORT AudioHandler : public ThreadSafeRefCounted<AudioHandler> {
   Vector<std::unique_ptr<AudioNodeInput>> inputs_;
   Vector<std::unique_ptr<AudioNodeOutput>> outputs_;
 
-  double last_processing_time_;
-  double last_non_silent_time_;
-
   int connection_ref_count_;
 
   bool is_disabled_;

third_party/blink/renderer/modules/webaudio/panner_node.cc

@@ -99,6 +99,69 @@ PannerHandler::~PannerHandler() {
   Uninitialize();
 }
 
+// PannerNode needs a custom ProcessIfNecessary to get the process lock when
+// computing PropagatesSilence() to protect processing from changes happening to
+// the panning model.  This is very similar to AudioNode::ProcessIfNecessary.
+void PannerHandler::ProcessIfNecessary(uint32_t frames_to_process) {
+  DCHECK(Context()->IsAudioThread());
+
+  if (!IsInitialized())
+    return;
+
+  // Ensure that we only process once per rendering quantum.
+  // This handles the "fanout" problem where an output is connected to multiple
+  // inputs.  The first time we're called during this time slice we process, but
+  // after that we don't want to re-process, instead our output(s) will already
+  // have the results cached in their bus;
+  double current_time = Context()->currentTime();
+  if (last_processing_time_ != current_time) {
+    // important to first update this time because of feedback loops in the
+    // rendering graph.
+    last_processing_time_ = current_time;
+
+    PullInputs(frames_to_process);
+
+    bool silent_inputs = InputsAreSilent();
+
+    {
+      // Need to protect calls to PropagetesSilence (and Process) because the
+      // main threda may be changing the panning model that modifies the
+      // TailTime and LatencyTime methods called by PropagatesSilence.
+      MutexTryLocker try_locker(process_lock_);
+      if (try_locker.Locked()) {
+        if (silent_inputs && PropagatesSilence()) {
+          SilenceOutputs();
+          // AudioParams still need to be processed so that the value can be
+          // updated if there are automations or so that the upstream nodes get
+          // pulled if any are connected to the AudioParam.
+          ProcessOnlyAudioParams(frames_to_process);
+        } else {
+          // Unsilence the outputs first because the processing of the node may
+          // cause the outputs to go silent and we want to propagate that hint
+          // to the downstream nodes.  (For example, a Gain node with a gain of
+          // 0 will want to silence its output.)
+          UnsilenceOutputs();
+          Process(frames_to_process);
+        }
+      } else {
+        // We must be in the middle of changing the properties of the panner.
+        // Just output silence.
+        AudioBus* destination = Output(0).Bus();
+        destination->Zero();
+      }
+    }
+
+    if (!silent_inputs) {
+      // Update |last_non_silent_time| AFTER processing this block.
+      // Doing it before causes |PropagateSilence()| to be one render
+      // quantum longer than necessary.
+      last_non_silent_time_ =
+          (Context()->CurrentSampleFrame() + frames_to_process) /
+          static_cast<double>(Context()->sampleRate());
+    }
+  }
+}
+
 void PannerHandler::Process(uint32_t frames_to_process) {
   AudioBus* destination = Output(0).Bus();
 
@@ -114,10 +177,9 @@ void PannerHandler::Process(uint32_t frames_to_process) {
   }
 
   // The audio thread can't block on this lock, so we call tryLock() instead.
-  MutexTryLocker try_locker(process_lock_);
   MutexTryLocker try_listener_locker(Listener()->ListenerLock());
 
-  if (try_locker.Locked() && try_listener_locker.Locked()) {
+  if (try_listener_locker.Locked()) {
     if (!Context()->HasRealtimeConstraint() &&
         panning_model_ == Panner::kPanningModelHRTF) {
       // For an OfflineAudioContext, we need to make sure the HRTFDatabase

third_party/blink/renderer/modules/webaudio/panner_node.h

@@ -71,6 +71,7 @@ class PannerHandler final : public AudioHandler {
   ~PannerHandler() override;
 
   // AudioHandler
+  void ProcessIfNecessary(uint32_t frames_to_process) override;
   void Process(uint32_t frames_to_process) override;
   void ProcessSampleAccurateValues(AudioBus* destination,
                                    const AudioBus* source,