Add to rendering orphan handlers when destination is running.

Continue to add to the rendering orphan handlers list if the
destination is still rendering.

If the destination is requested to stop, set a flag so that the
rendering won't pull on the audio graph anymore, even if the
audio device is still running and requesting data.

In the test case from the bug, we were not adding the handlers to the
list because the context was closed.  However, the destination was
still running because the audio device wasn't stopped yet. In the test,
the handlers were being destroyed at the same time as the audio thread
was accessing the handlers to render audio.

Bug: 1003807, 1017961
Change-Id: I87052c8504c5698d71f041e6d4305cf42851f067
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1879784Reviewed-by: Hongchan Choi <hongchan@chromium.org>
Commit-Queue: Raymond Toy <rtoy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712666}

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

@@ -257,6 +257,21 @@ ScriptPromise AudioContext::resumeContext(ScriptState* script_state) {
   return promise;
 }
 
+bool AudioContext::IsPullingAudioGraph() const {
+  DCHECK(IsMainThread());
+
+  if (!destination())
+    return false;
+
+  RealtimeAudioDestinationHandler& destination_handler =
+      static_cast<RealtimeAudioDestinationHandler&>(
+          destination()->GetAudioDestinationHandler());
+
+  // The realtime context is pulling on the audio graph if the realtime
+  // destination allows it.
+  return destination_handler.IsPullingAudioGraphAllowed();
+}
+
 AudioTimestamp* AudioContext::getOutputTimestamp(
     ScriptState* script_state) const {
   AudioTimestamp* result = AudioTimestamp::Create();

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

@@ -56,6 +56,8 @@ class MODULES_EXPORT AudioContext : public BaseAudioContext {
 
   bool HasRealtimeConstraint() final { return true; }
 
+  bool IsPullingAudioGraph() const final;
+
   AudioTimestamp* getOutputTimestamp(ScriptState*) const;
   double baseLatency() const;
 

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

@@ -609,25 +609,15 @@ void AudioNode::Dispose() {
   BaseAudioContext::GraphAutoLocker locker(context());
   Handler().Dispose();
 
-  if (context()->HasRealtimeConstraint()) {
-    // Add the handler to the orphan list if the context is not
-    // uninitialized (Nothing will clean up the orphan list if the context
-    // is uninitialized.)  These will get cleaned up in the post render task
-    // if audio thread is running or when the context is colleced (in
-    // the worst case).
-    if (!context()->IsContextClosed()) {
-      context()->GetDeferredTaskHandler().AddRenderingOrphanHandler(
-          std::move(handler_));
-    }
-  } else {
-    // For an offline context, only need to save the handler when the
-    // context is running.  The change in the context state is
-    // synchronous with the main thread (even though the offline
-    // thread is not synchronized to the main thread).
-    if (context()->ContextState() == BaseAudioContext::kRunning) {
-      context()->GetDeferredTaskHandler().AddRenderingOrphanHandler(
-          std::move(handler_));
-    }
+  // Add the handler to the orphan list if the context is pulling on the audio
+  // graph.  This keeps the handler alive until it can be deleted at a safe
+  // point (in pre/post handler task).  If graph isn't being pulled, we can
+  // delete the handler now since nothing on the audio thread will be touching
+  // it.
+  DCHECK(context());
+  if (context()->IsPullingAudioGraph()) {
+    context()->GetDeferredTaskHandler().AddRenderingOrphanHandler(
+        std::move(handler_));
   }
 
   // Notify the inspector that this node is going away. The actual clean up

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

@@ -142,6 +142,10 @@ class MODULES_EXPORT BaseAudioContext
   // does nothing useful because the context is closed.
   void WarnForConnectionIfContextClosed() const;
 
+  // Return true if the destination is pulling on the audio graph.  Otherwise
+  // return false.
+  virtual bool IsPullingAudioGraph() const = 0;
+
   AudioBuffer* createBuffer(uint32_t number_of_channels,
                             uint32_t number_of_frames,
                             float sample_rate,

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

@@ -479,6 +479,15 @@ void OfflineAudioContext::RejectPendingResolvers() {
   RejectPendingDecodeAudioDataResolvers();
 }
 
+bool OfflineAudioContext::IsPullingAudioGraph() const {
+  DCHECK(IsMainThread());
+
+  // For an offline context, we're rendering only while the context is running.
+  // Unlike an AudioContext, there's no audio device that keeps pulling on graph
+  // after the context has finished rendering.
+  return ContextState() == BaseAudioContext::kRunning;
+}
+
 bool OfflineAudioContext::ShouldSuspend() {
   DCHECK(IsAudioThread());
 

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

@@ -70,6 +70,8 @@ class MODULES_EXPORT OfflineAudioContext final : public BaseAudioContext {
 
   bool HasRealtimeConstraint() final { return false; }
 
+  bool IsPullingAudioGraph() const final;
+
   DEFINE_ATTRIBUTE_EVENT_LISTENER(complete, kComplete)
 
   // Fire completion event when the rendering is finished.

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

@@ -52,7 +52,8 @@ RealtimeAudioDestinationHandler::RealtimeAudioDestinationHandler(
     base::Optional<float> sample_rate)
     : AudioDestinationHandler(node),
       latency_hint_(latency_hint),
-      sample_rate_(sample_rate) {
+      sample_rate_(sample_rate),
+      allow_pulling_audio_graph_(false) {
   // Node-specific default channel count and mixing rules.
   channel_count_ = 2;
   SetInternalChannelCountMode(kExplicit);
@@ -194,19 +195,26 @@ void RealtimeAudioDestinationHandler::Render(
 
   context->HandlePreRenderTasks(&output_position, &metric);
 
-  // Renders the graph by pulling all the input(s) to this node. This will in
-  // turn pull on their input(s), all the way backwards through the graph.
-  AudioBus* rendered_bus = Input(0).Pull(destination_bus, number_of_frames);
-
-  DCHECK(rendered_bus);
-  if (!rendered_bus) {
-    // AudioNodeInput might be in the middle of destruction. Then the internal
-    // summing bus will return as nullptr. Then zero out the output.
+  // Only pull on the audio graph if we have not stopped the destination.  It
+  // takes time for the destination to stop, but we want to stop pulling before
+  // the destination has actually stopped.
+  if (IsPullingAudioGraphAllowed()) {
+    // Renders the graph by pulling all the input(s) to this node. This will in
+    // turn pull on their input(s), all the way backwards through the graph.
+    AudioBus* rendered_bus = Input(0).Pull(destination_bus, number_of_frames);
+
+    DCHECK(rendered_bus);
+    if (!rendered_bus) {
+      // AudioNodeInput might be in the middle of destruction. Then the internal
+      // summing bus will return as nullptr. Then zero out the output.
+      destination_bus->Zero();
+    } else if (rendered_bus != destination_bus) {
+      // In-place processing was not possible. Copy the rendererd result to the
+      // given |destination_bus| buffer.
+      destination_bus->CopyFrom(*rendered_bus);
+    }
+  } else {
     destination_bus->Zero();
-  } else if (rendered_bus != destination_bus) {
-    // In-place processing was not possible. Copy the rendererd result to the
-    // given |destination_bus| buffer.
-    destination_bus->CopyFrom(*rendered_bus);
   }
 
   // Processes "automatic" nodes that are not connected to anything. This can
@@ -243,6 +251,8 @@ void RealtimeAudioDestinationHandler::CreatePlatformDestination() {
 }
 
 void RealtimeAudioDestinationHandler::StartPlatformDestination() {
+  DCHECK(IsMainThread());
+
   if (platform_destination_->IsPlaying()) {
     return;
   }
@@ -258,9 +268,19 @@ void RealtimeAudioDestinationHandler::StartPlatformDestination() {
   } else {
     platform_destination_->Start();
   }
+
+  // Allow the graph to be pulled once the destination actually starts
+  // requesting data.
+  EnablePullingAudioGraph();
 }
 
 void RealtimeAudioDestinationHandler::StopPlatformDestination() {
+  DCHECK(IsMainThread());
+
+  // Stop pulling on the graph, even if the destination is still requesting data
+  // for a while. (It may take a bit of time for the destination to stop.)
+  DisablePullingAudioGraph();
+
   if (platform_destination_->IsPlaying()) {
     platform_destination_->Stop();
   }

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

@@ -26,6 +26,7 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_MODULES_WEBAUDIO_REALTIME_AUDIO_DESTINATION_NODE_H_
 #define THIRD_PARTY_BLINK_RENDERER_MODULES_WEBAUDIO_REALTIME_AUDIO_DESTINATION_NODE_H_
 
+#include <atomic>
 #include <memory>
 #include "third_party/blink/public/platform/web_audio_latency_hint.h"
 #include "third_party/blink/renderer/modules/webaudio/audio_destination_node.h"
@@ -80,6 +81,10 @@ class RealtimeAudioDestinationHandler final : public AudioDestinationHandler,
   // Returns a given frames-per-buffer size from audio infra.
   int GetFramesPerBuffer() const;
 
+  bool IsPullingAudioGraphAllowed() const {
+    return allow_pulling_audio_graph_.load(std::memory_order_acquire);
+  }
+
  private:
   explicit RealtimeAudioDestinationHandler(AudioNode&,
                                            const WebAudioLatencyHint&,
@@ -89,12 +94,32 @@ class RealtimeAudioDestinationHandler final : public AudioDestinationHandler,
   void StartPlatformDestination();
   void StopPlatformDestination();
 
+  // Should only be called from StartPlatformDestination.
+  void EnablePullingAudioGraph() {
+    allow_pulling_audio_graph_.store(true, std::memory_order_release);
+  }
+
+  // Should only be called from StopPlatformDestination.
+  void DisablePullingAudioGraph() {
+    allow_pulling_audio_graph_.store(false, std::memory_order_release);
+  }
+
   const WebAudioLatencyHint latency_hint_;
 
   // Holds the audio device thread that runs the real time audio context.
   scoped_refptr<AudioDestination> platform_destination_;
 
   base::Optional<float> sample_rate_;
+
+  // If true, the audio graph will be pulled to get new data.  Otherwise, the
+  // graph is not pulled, even if the audio thread is still running and
+  // requesting data.
+  //
+  // Must be modified only in StartPlatformDestination (via
+  // EnablePullingAudioGraph) or StopPlatformDestination (via
+  // DisablePullingAudioGraph) .  This is modified only by the main threda and
+  // the audio thread only reads this.
+  std::atomic_bool allow_pulling_audio_graph_;
 };
 
 // -----------------------------------------------------------------------------