Clean-up: MediaStreamAudioSourceNode/Handler

This cleans up MediaStreamAudioNode/Handler classes.

- Reorganizes the code order.
- Clarifies the constructor step based on the spec algorithm.
- Simplifies if() branches and the lock scope in SetFormat().

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

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

@@ -62,30 +62,43 @@ MediaStreamAudioSourceHandler::~MediaStreamAudioSourceHandler() {
 
 void MediaStreamAudioSourceHandler::SetFormat(uint32_t number_of_channels,
                                               float source_sample_rate) {
-  MutexLocker locker(process_lock_);
-  if (number_of_channels != source_number_of_channels_ ||
-      source_sample_rate != Context()->sampleRate()) {
-    // The sample-rate must be equal to the context's sample-rate.
-    if (!number_of_channels ||
-        number_of_channels > BaseAudioContext::MaxNumberOfChannels() ||
-        source_sample_rate != Context()->sampleRate()) {
-      // process() will generate silence for these uninitialized values.
-      DLOG(ERROR) << "setFormat(" << number_of_channels << ", "
-                  << source_sample_rate << ") - unhandled format change";
-      source_number_of_channels_ = 0;
+  DCHECK(IsMainThread());
+
+  {
+    MutexLocker locker(process_lock_);
+    TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("webaudio.audionode"),
+                "MediaStreamAudioSourceHandler::SetFormat under lock");
+
+    // If the channel count and the sample rate match, nothing to do here.
+    if (number_of_channels == source_number_of_channels_ &&
+        source_sample_rate == Context()->sampleRate()) {
       return;
     }
 
-    source_number_of_channels_ = number_of_channels;
-
-    {
-      // The context must be locked when changing the number of output channels.
-      BaseAudioContext::GraphAutoLocker context_locker(Context());
+    // Checks for invalid channel count.
+    if (number_of_channels == 0 ||
+        number_of_channels > BaseAudioContext::MaxNumberOfChannels()) {
+      source_number_of_channels_ = 0;
+      DLOG(ERROR) << "MediaStreamAudioSourceHandler::setFormat - "
+                  << "invalid channel count requested ("
+                  << number_of_channels << ")";
+      return;
+    }
 
-      // Do any necesssary re-configuration to the output's number of channels.
-      Output(0).SetNumberOfChannels(number_of_channels);
+    // Checks for invalid sample rate.
+    if (source_sample_rate != Context()->sampleRate()) {
+      source_number_of_channels_ = 0;
+      DLOG(ERROR) << "MediaStreamAudioSourceHandler::setFormat - "
+                  << "invalid sample rate requested ("
+                  << source_sample_rate << ")";
+      return;
     }
+
+    source_number_of_channels_ = number_of_channels;
   }
+
+  BaseAudioContext::GraphAutoLocker graph_locker(Context());
+  Output(0).SetNumberOfChannels(number_of_channels);
 }
 
 void MediaStreamAudioSourceHandler::Process(uint32_t number_of_frames) {
@@ -94,28 +107,23 @@ void MediaStreamAudioSourceHandler::Process(uint32_t number_of_frames) {
 
   AudioBus* output_bus = Output(0).Bus();
 
-  if (!GetAudioSourceProvider()) {
-    output_bus->Zero();
-    return;
-  }
-
-  // Use a tryLock() to avoid contention in the real-time audio thread.
-  // If we fail to acquire the lock then the MediaStream must be in the middle
-  // of a format change, so we output silence in this case.
   MutexTryLocker try_locker(process_lock_);
   if (try_locker.Locked()) {
     if (source_number_of_channels_ != output_bus->NumberOfChannels()) {
       output_bus->Zero();
       return;
     }
-    GetAudioSourceProvider()->ProvideInput(output_bus, number_of_frames);
+    audio_source_provider_.get()->ProvideInput(output_bus, number_of_frames);
   } else {
-    // We failed to acquire the lock.
+    TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("webaudio.audionode"),
+                 "MediaStreamAudioSourceHandler::Process TryLock failed");
+    // If we fail to acquire the lock, it means setFormat() is running. So
+    // output silence.
     output_bus->Zero();
   }
 }
 
-// ----------------------------------------------------------------
+// -----------------------------------------------------------------------------
 
 MediaStreamAudioSourceNode::MediaStreamAudioSourceNode(
     AudioContext& context,
@@ -140,6 +148,10 @@ MediaStreamAudioSourceNode* MediaStreamAudioSourceNode::Create(
   if (!context.CheckExecutionContextAndThrowIfNecessary(exception_state))
     return nullptr;
 
+  // The constructor algorithm:
+  // https://webaudio.github.io/web-audio-api/#mediastreamaudiosourcenode
+
+  // 1.24.1. Step 1 & 2.
   MediaStreamTrackVector audio_tracks = media_stream.getAudioTracks();
   if (audio_tracks.IsEmpty()) {
     exception_state.ThrowDOMException(DOMExceptionCode::kInvalidStateError,
@@ -147,28 +159,29 @@ MediaStreamAudioSourceNode* MediaStreamAudioSourceNode::Create(
     return nullptr;
   }
 
-  // Find the first track, which is the track whose id comes first given a
-  // lexicographic ordering of the code units of the track id.
+  // 1.24.1. Step 3: Sort the elements in tracks based on their id attribute
+  // using an ordering on sequences of code unit values.
+  // (See: https://infra.spec.whatwg.org/#code-unit)
   MediaStreamTrack* audio_track = audio_tracks[0];
   for (auto track : audio_tracks) {
-    if (CodeUnitCompareLessThan(track->id(), audio_track->id())) {
+    if (CodeUnitCompareLessThan(track->id(), audio_track->id()))
       audio_track = track;
-    }
   }
+
+  // 1.24.1. Step 5: The step is out of order because the constructor needs
+  // this provider, which is [[input track]] from the spec.
   std::unique_ptr<AudioSourceProvider> provider =
       audio_track->CreateWebAudioSource(context.sampleRate());
 
+  // 1.24.1. Step 4.
   MediaStreamAudioSourceNode* node =
       MakeGarbageCollected<MediaStreamAudioSourceNode>(
           context, media_stream, audio_track, std::move(provider));
 
-  if (!node)
-    return nullptr;
-
-  // TODO(hongchan): Only stereo streams are supported right now. We should be
-  // able to accept multi-channel streams.
+  // Initializes the node with the stereo output channel.
   node->SetFormat(2, context.sampleRate());
-  // context keeps reference until node is disconnected
+
+  // Lets the context know this source node started.
   context.NotifySourceNodeStartedProcessing(node);
 
   return node;
@@ -181,33 +194,12 @@ MediaStreamAudioSourceNode* MediaStreamAudioSourceNode::Create(
   return Create(*context, *options->mediaStream(), exception_state);
 }
 
-void MediaStreamAudioSourceNode::Trace(Visitor* visitor) const {
-  visitor->Trace(audio_track_);
-  visitor->Trace(media_stream_);
-  AudioSourceProviderClient::Trace(visitor);
-  AudioNode::Trace(visitor);
-}
-
-MediaStreamAudioSourceHandler&
-MediaStreamAudioSourceNode::GetMediaStreamAudioSourceHandler() const {
-  return static_cast<MediaStreamAudioSourceHandler&>(Handler());
-}
-
-MediaStream* MediaStreamAudioSourceNode::getMediaStream() const {
-  return media_stream_;
-}
-
 void MediaStreamAudioSourceNode::SetFormat(uint32_t number_of_channels,
                                            float source_sample_rate) {
   GetMediaStreamAudioSourceHandler().SetFormat(number_of_channels,
                                                source_sample_rate);
 }
 
-bool MediaStreamAudioSourceNode::HasPendingActivity() const {
-  // As long as the context is running, this node has activity.
-  return (context()->ContextState() == BaseAudioContext::kRunning);
-}
-
 void MediaStreamAudioSourceNode::ReportDidCreate() {
   GraphTracer().DidCreateAudioNode(this);
 }
@@ -216,4 +208,23 @@ void MediaStreamAudioSourceNode::ReportWillBeDestroyed() {
   GraphTracer().WillDestroyAudioNode(this);
 }
 
+bool MediaStreamAudioSourceNode::HasPendingActivity() const {
+  // The node stays alive as long as the context is running. It also will not
+  // be collected until the context is suspended or stopped.
+  // (See https://crbug.com/937231)
+  return context()->ContextState() == BaseAudioContext::kRunning;
+}
+
+void MediaStreamAudioSourceNode::Trace(Visitor* visitor) const {
+  visitor->Trace(audio_track_);
+  visitor->Trace(media_stream_);
+  AudioSourceProviderClient::Trace(visitor);
+  AudioNode::Trace(visitor);
+}
+
+MediaStreamAudioSourceHandler&
+MediaStreamAudioSourceNode::GetMediaStreamAudioSourceHandler() const {
+  return static_cast<MediaStreamAudioSourceHandler&>(Handler());
+}
+
 }  // namespace blink

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

@@ -49,27 +49,19 @@ class MediaStreamAudioSourceHandler final : public AudioHandler {
   // AudioHandler
   void Process(uint32_t frames_to_process) override;
 
-  // AudioNode
-  double TailTime() const override { return 0; }
-  double LatencyTime() const override { return 0; }
-
-  // A helper for AudioSourceProviderClient implementation of
-  // MediaStreamAudioSourceNode.
   void SetFormat(uint32_t number_of_channels, float sample_rate);
 
+  double TailTime() const override { return 0; }
+  double LatencyTime() const override { return 0; }
   bool RequiresTailProcessing() const final { return false; }
 
  private:
   MediaStreamAudioSourceHandler(AudioNode&,
                                 std::unique_ptr<AudioSourceProvider>);
 
-  // As an audio source, we will never propagate silence.
+  // AudioHandler: MediaStreamAudioSourceNode never propagates silence.
   bool PropagatesSilence() const override { return false; }
 
-  AudioSourceProvider* GetAudioSourceProvider() const {
-    return audio_source_provider_.get();
-  }
-
   std::unique_ptr<AudioSourceProvider> audio_source_provider_;
 
   // Protects |source_number_of_channels_|.
@@ -78,6 +70,8 @@ class MediaStreamAudioSourceHandler final : public AudioHandler {
   unsigned source_number_of_channels_ = 0;
 };
 
+// -----------------------------------------------------------------------------
+
 class MediaStreamAudioSourceNode final
     : public AudioNode,
       public AudioSourceProviderClient,
@@ -88,27 +82,28 @@ class MediaStreamAudioSourceNode final
   static MediaStreamAudioSourceNode* Create(AudioContext&,
                                             MediaStream&,
                                             ExceptionState&);
-  static MediaStreamAudioSourceNode*
-  Create(AudioContext*, const MediaStreamAudioSourceOptions*, ExceptionState&);
+  static MediaStreamAudioSourceNode* Create(
+      AudioContext*, const MediaStreamAudioSourceOptions*, ExceptionState&);
 
   MediaStreamAudioSourceNode(AudioContext&,
                              MediaStream&,
                              MediaStreamTrack*,
                              std::unique_ptr<AudioSourceProvider>);
 
-  void Trace(Visitor*) const override;
-
-  MediaStream* getMediaStream() const;
+  // V8 binding
+  MediaStream* getMediaStream() const { return media_stream_; }
 
-  // AudioSourceProviderClient functions:
+  // AudioSourceProviderClient
   void SetFormat(uint32_t number_of_channels, float sample_rate) override;
 
-  bool HasPendingActivity() const final;
-
   // InspectorHelperMixin
   void ReportDidCreate() final;
   void ReportWillBeDestroyed() final;
 
+  // GC
+  bool HasPendingActivity() const final;
+  void Trace(Visitor*) const override;
+
  private:
   MediaStreamAudioSourceHandler& GetMediaStreamAudioSourceHandler() const;