Bug 1291702 - Remove the aDestination parameter in DisconnectFromOutputIfConnected and IterateOnDestinationInputs. r=karlt
authorPaul Adenot <paul@paul.cx>
Thu, 20 Oct 2016 14:34:27 +0200
changeset 318838 2db3f77ab4516bb7d7fb22ae14ffc1f5b226d60a
parent 318837 33318a218d2d0f7c06b1da17456ea6cc80cbb0cd
child 318839 d40e7403de4630eb7af5aff4f40d6047850e75aa
push id30854
push userryanvm@gmail.com
push dateFri, 21 Oct 2016 21:08:02 +0000
treeherdermozilla-central@806054dd12bd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskarlt
bugs1291702
milestone52.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1291702 - Remove the aDestination parameter in DisconnectFromOutputIfConnected and IterateOnDestinationInputs. r=karlt We do this by abstracting away getting the input for AudioNodes and AudioParams with templates. This prevents removing the wrong connection when disconnecting nodes with multiple inputs. MozReview-Commit-ID: 3fzUKthRM1f
dom/media/webaudio/AudioNode.cpp
dom/media/webaudio/AudioNode.h
dom/media/webaudio/test/mochitest.ini
dom/media/webaudio/test/test_disconnectFromAudioNodeMultipleConnection.html
--- a/dom/media/webaudio/AudioNode.cpp
+++ b/dom/media/webaudio/AudioNode.cpp
@@ -290,26 +290,28 @@ void
 AudioNode::SendChannelMixingParametersToStream()
 {
   if (mStream) {
     mStream->SetChannelMixingParameters(mChannelCount, mChannelCountMode,
                                         mChannelInterpretation);
   }
 }
 
+template<>
 bool
-AudioNode::DisconnectFromOutputIfConnected(AudioNode& aDestination,
-                                           uint32_t aOutputNodeIndex,
-                                           uint32_t aInputIndex)
+AudioNode::DisconnectFromOutputIfConnected<AudioNode>(uint32_t aOutputNodeIndex,
+                                                      uint32_t aInputIndex)
 {
   WEB_AUDIO_API_LOG("%f: %s %u Disconnect()", Context()->CurrentTime(),
                     NodeType(), Id());
 
+  AudioNode* destination = mOutputNodes[aOutputNodeIndex];
+
   MOZ_ASSERT(aOutputNodeIndex < mOutputNodes.Length());
-  MOZ_ASSERT(aInputIndex < aDestination.InputNodes().Length());
+  MOZ_ASSERT(aInputIndex < destination->InputNodes().Length());
 
   // An upstream node may be starting to play on the graph thread, and the
   // engine for a downstream node may be sending a PlayingRefChangeHandler
   // ADDREF message to this (main) thread.  Wait for a round trip before
   // releasing nodes, to give engines receiving sound now time to keep their
   // nodes alive.
   class RunnableRelease final : public Runnable
   {
@@ -321,145 +323,164 @@ AudioNode::DisconnectFromOutputIfConnect
     {
       mNode = nullptr;
       return NS_OK;
     }
   private:
     RefPtr<AudioNode> mNode;
   };
 
-  InputNode& input = aDestination.mInputNodes[aInputIndex];
+  InputNode& input = destination->mInputNodes[aInputIndex];
   if (input.mInputNode != this) {
     return false;
   }
 
-  // Destroying the InputNode here sends a message to the graph thread
-  // to disconnect the streams, which should be sent before the
-  // RunAfterPendingUpdates() call below.
-  aDestination.mInputNodes.RemoveElementAt(aInputIndex);
   // Remove one instance of 'dest' from mOutputNodes. There could be
   // others, and it's not correct to remove them all since some of them
   // could be for different output ports.
   RefPtr<AudioNode> output = mOutputNodes[aOutputNodeIndex].forget();
   mOutputNodes.RemoveElementAt(aOutputNodeIndex);
+  // Destroying the InputNode here sends a message to the graph thread
+  // to disconnect the streams, which should be sent before the
+  // RunAfterPendingUpdates() call below.
+  destination->mInputNodes.RemoveElementAt(aInputIndex);
   output->NotifyInputsChanged();
   if (mStream) {
     nsCOMPtr<nsIRunnable> runnable = new RunnableRelease(output.forget());
     mStream->RunAfterPendingUpdates(runnable.forget());
   }
   return true;
 }
 
+template<>
 bool
-AudioNode::DisconnectFromOutputIfConnected(AudioParam& aDestination,
-                                           uint32_t aOutputParamIndex,
-                                           uint32_t aInputIndex)
+AudioNode::DisconnectFromOutputIfConnected<AudioParam>(uint32_t aOutputParamIndex,
+                                                       uint32_t aInputIndex)
 {
   MOZ_ASSERT(aOutputParamIndex < mOutputParams.Length());
-  MOZ_ASSERT(aInputIndex < aDestination.InputNodes().Length());
+
+  AudioParam* destination = mOutputParams[aOutputParamIndex];
 
-  const InputNode& input = aDestination.InputNodes()[aInputIndex];
+  MOZ_ASSERT(aInputIndex < destination->InputNodes().Length());
+
+  const InputNode& input = destination->InputNodes()[aInputIndex];
   if (input.mInputNode != this) {
     return false;
   }
-  aDestination.RemoveInputNode(aInputIndex);
+  destination->RemoveInputNode(aInputIndex);
   // Remove one instance of 'dest' from mOutputParams. There could be
   // others, and it's not correct to remove them all since some of them
   // could be for different output ports.
   mOutputParams.RemoveElementAt(aOutputParamIndex);
   return true;
 }
 
+template<>
+const nsTArray<AudioNode::InputNode>&
+AudioNode::InputsForDestination<AudioNode>(uint32_t aOutputNodeIndex) const {
+  return mOutputNodes[aOutputNodeIndex]->InputNodes();
+}
+
+template<>
+const nsTArray<AudioNode::InputNode>&
+AudioNode::InputsForDestination<AudioParam>(uint32_t aOutputNodeIndex) const {
+  return mOutputParams[aOutputNodeIndex]->InputNodes();
+}
+
 template<typename DestinationType, typename Predicate>
 bool
-AudioNode::DisconnectMatchingDestinationInputs(DestinationType& aDestination,
-                                               uint32_t aDestinationIndex,
+AudioNode::DisconnectMatchingDestinationInputs(uint32_t aDestinationIndex,
                                                Predicate aPredicate)
 {
   bool wasConnected = false;
-  for (int32_t inputIndex = aDestination.InputNodes().Length() - 1;
-       inputIndex >= 0; --inputIndex) {
-    const InputNode& input = aDestination.InputNodes()[inputIndex];
+  uint32_t inputCount =
+    InputsForDestination<DestinationType>(aDestinationIndex).Length();
+
+  for (int32_t inputIndex = inputCount - 1; inputIndex >= 0; --inputIndex) {
+    const InputNode& input =
+      InputsForDestination<DestinationType>(aDestinationIndex)[inputIndex];
     if (aPredicate(input)) {
-      if (DisconnectFromOutputIfConnected(aDestination,
-                                          aDestinationIndex,
-                                          inputIndex)) {
+      if (DisconnectFromOutputIfConnected<DestinationType>(aDestinationIndex,
+                                                           inputIndex)) {
         wasConnected = true;
         break;
       }
     }
   }
   return wasConnected;
 }
 
 void
 AudioNode::Disconnect(ErrorResult& aRv)
 {
   for (int32_t outputIndex = mOutputNodes.Length() - 1;
        outputIndex >= 0; --outputIndex) {
-    AudioNode* dest = mOutputNodes[outputIndex];
-    DisconnectMatchingDestinationInputs(*dest, outputIndex,
-                                        [](const InputNode&) { return true; });
+    DisconnectMatchingDestinationInputs<AudioNode>(outputIndex,
+                                                   [](const InputNode&) {
+                                                     return true;
+                                                   });
   }
 
   for (int32_t outputIndex = mOutputParams.Length() - 1;
        outputIndex >= 0; --outputIndex) {
-    AudioParam* dest = mOutputParams[outputIndex];
-    DisconnectMatchingDestinationInputs(*dest, outputIndex,
-                                        [](const InputNode&) { return true; });
+    DisconnectMatchingDestinationInputs<AudioParam>(outputIndex,
+                                                    [](const InputNode&) {
+                                                      return true;
+                                                    });
   }
 
   // This disconnection may have disconnected a panner and a source.
   Context()->UpdatePannerSource();
 }
 
 void
 AudioNode::Disconnect(uint32_t aOutput, ErrorResult& aRv)
 {
   if (aOutput >= NumberOfOutputs()) {
     aRv.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
     return;
   }
 
   for (int32_t outputIndex = mOutputNodes.Length() - 1;
        outputIndex >= 0; --outputIndex) {
-    AudioNode* dest = mOutputNodes[outputIndex];
-    DisconnectMatchingDestinationInputs(*dest, outputIndex,
-                                        [aOutput](const InputNode& aInputNode) {
-                                          return aInputNode.mOutputPort == aOutput;
-                                        });
+    DisconnectMatchingDestinationInputs<AudioNode>(
+        outputIndex,
+        [aOutput](const InputNode& aInputNode) {
+          return aInputNode.mOutputPort == aOutput;
+        });
   }
 
   for (int32_t outputIndex = mOutputParams.Length() - 1;
        outputIndex >= 0; --outputIndex) {
-    AudioParam* dest = mOutputParams[outputIndex];
-    DisconnectMatchingDestinationInputs(*dest, outputIndex,
-                                        [aOutput](const InputNode& aInputNode) {
-                                          return aInputNode.mOutputPort == aOutput;
-                                        });
+    DisconnectMatchingDestinationInputs<AudioParam>(
+        outputIndex,
+        [aOutput](const InputNode& aInputNode) {
+          return aInputNode.mOutputPort == aOutput;
+        });
   }
 
   // This disconnection may have disconnected a panner and a source.
   Context()->UpdatePannerSource();
 }
 
 void
 AudioNode::Disconnect(AudioNode& aDestination, ErrorResult& aRv)
 {
   bool wasConnected = false;
 
-  size_t outputIndex = mOutputNodes.IndexOf(&aDestination);
-  if (outputIndex == nsTArray<InputNode>::NoIndex) {
-    aRv.Throw(NS_ERROR_DOM_INVALID_ACCESS_ERR);
-    return;
-  }
-  for (int32_t inputIndex = aDestination.mInputNodes.Length() - 1;
-       inputIndex >= 0; --inputIndex) {
+  for (int32_t outputIndex = mOutputNodes.Length() - 1;
+       outputIndex >= 0; --outputIndex) {
+    if (mOutputNodes[outputIndex] != &aDestination) {
+      continue;
+    }
     wasConnected |=
-      DisconnectFromOutputIfConnected(aDestination, outputIndex, inputIndex);
+      DisconnectMatchingDestinationInputs<AudioNode>(outputIndex,
+                                                     [](const InputNode&) {
+                                                       return true;
+                                                     });
   }
 
   if (!wasConnected) {
     aRv.Throw(NS_ERROR_DOM_INVALID_ACCESS_ERR);
     return;
   }
 
   // This disconnection may have disconnected a panner and a source.
@@ -475,19 +496,22 @@ AudioNode::Disconnect(AudioNode& aDestin
     aRv.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
     return;
   }
 
   bool wasConnected = false;
 
   for (int32_t outputIndex = mOutputNodes.Length() - 1;
        outputIndex >= 0; --outputIndex) {
+    if (mOutputNodes[outputIndex] != &aDestination) {
+      continue;
+    }
     wasConnected |=
-      DisconnectMatchingDestinationInputs(
-          aDestination, outputIndex,
+      DisconnectMatchingDestinationInputs<AudioNode>(
+          outputIndex,
           [aOutput](const InputNode& aInputNode) {
             return aInputNode.mOutputPort == aOutput;
           });
   }
 
   if (!wasConnected) {
     aRv.Throw(NS_ERROR_DOM_INVALID_ACCESS_ERR);
     return;
@@ -512,19 +536,22 @@ AudioNode::Disconnect(AudioNode& aDestin
     aRv.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
     return;
   }
 
   bool wasConnected = false;
 
   for (int32_t outputIndex = mOutputNodes.Length() - 1;
        outputIndex >= 0; --outputIndex) {
+    if (mOutputNodes[outputIndex] != &aDestination) {
+      continue;
+    }
     wasConnected |=
-      DisconnectMatchingDestinationInputs(
-          aDestination, outputIndex,
+      DisconnectMatchingDestinationInputs<AudioNode>(
+          outputIndex,
           [aOutput, aInput](const InputNode& aInputNode) {
             return aInputNode.mOutputPort == aOutput &&
                    aInputNode.mInputPort == aInput;
           });
   }
 
   if (!wasConnected) {
     aRv.Throw(NS_ERROR_DOM_INVALID_ACCESS_ERR);
@@ -537,21 +564,24 @@ AudioNode::Disconnect(AudioNode& aDestin
 
 void
 AudioNode::Disconnect(AudioParam& aDestination, ErrorResult& aRv)
 {
   bool wasConnected = false;
 
   for (int32_t outputIndex = mOutputParams.Length() - 1;
        outputIndex >= 0; --outputIndex) {
+    if (mOutputParams[outputIndex] != &aDestination) {
+      continue;
+    }
     wasConnected |=
-      DisconnectMatchingDestinationInputs(aDestination, outputIndex,
-                                          [](const InputNode&) {
-                                            return true;
-                                          });
+      DisconnectMatchingDestinationInputs<AudioParam>(outputIndex,
+                                                      [](const InputNode&) {
+                                                        return true;
+                                                      });
   }
 
   if (!wasConnected) {
     aRv.Throw(NS_ERROR_DOM_INVALID_ACCESS_ERR);
     return;
   }
 }
 
@@ -564,21 +594,25 @@ AudioNode::Disconnect(AudioParam& aDesti
     aRv.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
     return;
   }
 
   bool wasConnected = false;
 
   for (int32_t outputIndex = mOutputParams.Length() - 1;
        outputIndex >= 0; --outputIndex) {
+    if (mOutputParams[outputIndex] != &aDestination) {
+      continue;
+    }
     wasConnected |=
-      DisconnectMatchingDestinationInputs(aDestination, outputIndex,
-                                          [aOutput](const InputNode& aInputNode) {
-                                            return aInputNode.mOutputPort == aOutput;
-                                          });
+      DisconnectMatchingDestinationInputs<AudioParam>(
+          outputIndex,
+          [aOutput](const InputNode& aInputNode) {
+            return aInputNode.mOutputPort == aOutput;
+          });
   }
 
   if (!wasConnected) {
     aRv.Throw(NS_ERROR_DOM_INVALID_ACCESS_ERR);
     return;
   }
 }
 
--- a/dom/media/webaudio/AudioNode.h
+++ b/dom/media/webaudio/AudioNode.h
@@ -196,16 +196,20 @@ public:
   {
     return mOutputNodes;
   }
   const nsTArray<RefPtr<AudioParam> >& OutputParams() const
   {
     return mOutputParams;
   }
 
+  template<typename T>
+  const nsTArray<InputNode>&
+  InputsForDestination(uint32_t aOutputIndex) const;
+
   void RemoveOutputParam(AudioParam* aParam);
 
   // MarkActive() asks the context to keep the AudioNode alive until the
   // context is finished.  This takes care of "playing" references and
   // "tail-time" references.
   void MarkActive() { Context()->RegisterActiveNode(this); }
   // Active nodes call MarkInactive() when they have finished producing sound
   // for the foreseeable future.
@@ -222,34 +226,34 @@ public:
   virtual const char* NodeType() const = 0;
 
 private:
   // Given:
   //
   // - a DestinationType, that can be an AudioNode or an AudioParam ;
   // - a Predicate, a function that takes an InputNode& and returns a bool ;
   //
-  // This method iterates on the InputNodes() of aDestination, and calls
-  // `DisconnectFromOutputIfConnected` with this input node, if aPredicate
-  // returns true.
+  // This method iterates on the InputNodes() of the node at the index
+  // aDestinationIndex, and calls `DisconnectFromOutputIfConnected` with this
+  // input node, if aPredicate returns true.
   template<typename DestinationType, typename Predicate>
-  bool DisconnectMatchingDestinationInputs(DestinationType& aDestination,
-                                           uint32_t aDestinationIndex,
+  bool DisconnectMatchingDestinationInputs(uint32_t aDestinationIndex,
                                            Predicate aPredicate);
 
   virtual void LastRelease() override
   {
     // We are about to be deleted, disconnect the object from the graph before
     // the derived type is destroyed.
     DisconnectFromGraph();
   }
   // Callers must hold a reference to 'this'.
   void DisconnectFromGraph();
-  bool DisconnectFromOutputIfConnected(AudioNode& aDestination, uint32_t aOutputIndex, uint32_t aInputIndex);
-  bool DisconnectFromOutputIfConnected(AudioParam& aDestination, uint32_t aOutputIndex, uint32_t aInputIndex);
+
+  template<typename DestinationType>
+  bool DisconnectFromOutputIfConnected(uint32_t aOutputIndex, uint32_t aInputIndex);
 
 protected:
   // Helpers for sending different value types to streams
   void SendDoubleParameterToStream(uint32_t aIndex, double aValue);
   void SendInt32ParameterToStream(uint32_t aIndex, int32_t aValue);
   void SendThreeDPointParameterToStream(uint32_t aIndex, const ThreeDPoint& aValue);
   void SendChannelMixingParametersToStream();
 
--- a/dom/media/webaudio/test/mochitest.ini
+++ b/dom/media/webaudio/test/mochitest.ini
@@ -127,16 +127,17 @@ skip-if = toolkit == 'android' # bug 105
 [test_delayNodeWithGain.html]
 [test_disconnectAll.html]
 [test_disconnectAudioParam.html]
 [test_disconnectAudioParamFromOutput.html]
 [test_disconnectExceptions.html]
 [test_disconnectFromAudioNode.html]
 [test_disconnectFromAudioNodeAndOutput.html]
 [test_disconnectFromAudioNodeAndOutputAndInput.html]
+[test_disconnectFromAudioNodeMultipleConnection.html]
 [test_disconnectFromOutput.html]
 [test_dynamicsCompressorNode.html]
 [test_dynamicsCompressorNodePassThrough.html]
 [test_dynamicsCompressorNodeWithGain.html]
 [test_gainNode.html]
 [test_gainNodeInLoop.html]
 [test_gainNodePassThrough.html]
 [test_iirFilterNodePassThrough.html]
new file mode 100644
--- /dev/null
+++ b/dom/media/webaudio/test/test_disconnectFromAudioNodeMultipleConnection.html
@@ -0,0 +1,56 @@
+<!DOCTYPE HTML>
+<html>
+  <head>
+    <title>
+      Test whether we can disconnect all outbound connection of an AudioNode
+    </title>
+    <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+    <script type="text/javascript" src="webaudio.js"></script>
+    <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+  </head>
+  <body>
+    <pre id="test">
+      <script class="testbody" type="text/javascript">
+      var gTest = {
+        length: 256,
+        numberOfChannels: 2,
+        createGraph: function(context) {
+          var sourceBuffer = context.createBuffer(1, 256, context.sampleRate);
+          var data = sourceBuffer.getChannelData(0);
+          for (var j = 0; j < data.length; j++) {
+              data[j] = 1;
+          }
+
+          var source = context.createBufferSource();
+          source.buffer = sourceBuffer;
+
+          var merger = context.createChannelMerger(2);
+          var gain = context.createGain();
+
+          source.connect(merger, 0, 0);
+          source.connect(gain);
+          source.connect(merger, 0, 1);
+
+          source.disconnect(merger);
+
+          source.start();
+
+          return merger;
+        },
+        createExpectedBuffers: function(context) {
+          expectedBuffer = context.createBuffer(2, 256, context.sampleRate);
+          for (var channel = 0; channel < 2; channel++) {
+            for (var i = 0; i < 256; ++i) {
+              expectedBuffer.getChannelData(0)[i] = 0;
+            }
+          }
+
+          return expectedBuffer;
+        }
+      };
+
+      runTest();
+      </script>
+    </pre>
+  </body>
+</html>