1130010 - Implement the new AudioNode.disconnect methods

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P1)

Description

[Paul Adenot (:padenot)]

Those methods allows authors to disconnect a particular node:

https://github.com/WebAudio/web-audio-api/issues/6

Comment 1

[u538564]

Attachment: First draft

Comment 2

[Paul Adenot (:padenot)]

Comment on attachment 8610608 [details] [diff] [review]
First draft

Review of attachment 8610608 [details] [diff] [review]:
-----------------------------------------------------------------

Looks nice, even if a bit verbose and repetitive, maybe we can factor out the repetitive loops somehow ?

Ehsan, this changes the AudioNode's interface so authors can disconnect nodes with more flexibility, can you have a look?

::: dom/media/webaudio/AudioNode.cpp
@@ +334,4 @@
>    private:
>      nsRefPtr<AudioNode> mNode;
>    };
> +  

nit: trailing space.

@@ +336,5 @@
>    };
> +  
> +  InputNode& input = aDestination.mInputNodes[aInputIndex];
> +  if (input.mInputNode != this)
> +    return false;

Always brace your ifs.

@@ +350,5 @@
> +AudioNode::DisconnectFromParamIfConnected(AudioParam& aDestination, uint32_t aOutputIndex, uint32_t aInputIndex)
> +{
> +  const InputNode& input = aDestination.InputNodes()[aInputIndex];
> +  if (input.mInputNode != this)
> +    return false;

Always brace your ifs.

@@ -350,5 @@
> -        // RunAfterPendingUpdates() call below.
> -        dest->mInputNodes.RemoveElementAt(j);
> -        // 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.

Can you put those comments back? They are useful, and you dropped them while moving this bit of code to its own function.

@@ +365,5 @@
> +    for (int32_t inputIndex = dest->mInputNodes.Length() - 1; inputIndex >= 0; --inputIndex) {
> +      DisconnectFromOutputIfConnected(*dest, outputIndex, inputIndex);
> +    }
> +  }
> +  

nit: trailing space.

@@ +372,5 @@
> +    for (int32_t inputIndex = dest->InputNodes().Length() - 1; inputIndex >= 0; --inputIndex) {
> +      DisconnectFromParamIfConnected(*dest, outputIndex, inputIndex);
> +    }
> +  }
> +  

nit: trailing space.

@@ -368,5 @@
> -      if (input.mInputNode == this && input.mOutputPort == aOutput) {
> -        dest->RemoveInputNode(j);
> -        // 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.

Same, put the comments back in.

::: dom/media/webaudio/AudioNode.h
@@ +101,5 @@
> +  
> +  virtual void Disconnect(AudioNode& aDestination, ErrorResult& aRv);
> +  
> +  virtual void Disconnect(AudioNode& aDestination, uint32_t aOutput, ErrorResult& aRv);
> +  

nit: trailing spaces

::: dom/media/webaudio/test/test_disconnectFromAudioNodeAndOutputAndInput.html
@@ +46,5 @@
> +            data[j] = 1;
> +          else if (i == 2)
> +            data[j] = 2;
> +          else if (i == 3)
> +            data[j] = 0;

braces around if() please, even with only one statement in the if.

Comment 3

[(no longer active)]

Comment on attachment 8610608 [details] [diff] [review]
First draft

Review of attachment 8610608 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with both of our comments addressed.  :-)

Thanks a lot, Thomas, for your patches!

::: dom/media/webaudio/AudioNode.cpp
@@ -317,2 @@
>  {
> -  if (aOutput >= NumberOfOutputs()) {

Please MOZ_ASSERT this condition here.  Also, please add a similar MOZ_ASSERT for aInputIndex.

@@ +482,5 @@
>        }
>      }
> +    if (isConnected) {
> +      break;
> +    }

Why do you have to do this here, but not in the previous overload in this file?

::: dom/media/webaudio/test/test_disconnectAll.html
@@ +45,5 @@
> +      createExpectedBuffers: function(context) {
> +        expectedBuffer = context.createBuffer(1, 256, context.sampleRate);
> +        for (var i = 0; i < 256; ++i) {
> +          expectedBuffer.getChannelData(0)[i] = 0;
> +        }

Buffers are created as silent by default, you don't need to do this manually.

::: dom/media/webaudio/test/test_disconnectExceptions.html
@@ +2,5 @@
> +<html>
> +  <head>
> +    <title>Test whether we can disconnect an AudioNode</title>
> +    <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +    <script type="text/javascript" src="webaudio.js"></script>

It doesn't look like this test is using this helper script at all...

@@ +8,5 @@
> +  </head>
> +  <body>
> +    <pre id="test">
> +    <script class="testbody" type="text/javascript">
> +      SimpleTest.waitForExplicitFinish();

Since this test is synchronous (i.e., it finished when all of the JS code here has finished running), you can remove the above line and the SimpleTest.finish() call at the end.

::: dom/media/webaudio/test/test_disconnectFromAudioNode.html
@@ +34,5 @@
> +      gain2.connect(merger);
> +      gain3.connect(merger);
> +      source.start();
> +
> +      source.disconnect(gain2);

It would be nice if you added a similar version of this test where the source would be connected to an AudioParam as well, and verified that the AudioParam connection is left intact.

::: dom/media/webaudio/test/test_disconnectFromAudioNodeAndOutputAndInput.html
@@ +46,5 @@
> +            data[j] = 1;
> +          else if (i == 2)
> +            data[j] = 2;
> +          else if (i == 3)
> +            data[j] = 0;

Or, rewrite this as:

  data[i] == (i == 3) ? 0 : i;

::: dom/webidl/AudioNode.webidl
@@ +35,5 @@
> +    void disconnect(AudioNode destination);
> +    [Throws]
> +    void disconnect(AudioNode destination, unsigned long output);
> +    [Throws]
> +    void disconnect(AudioNode destination, unsigned long output, unsigned long input);

This is a bit weird, although this is an issue with <https://github.com/WebAudio/web-audio-api/pull/479> really, so I'm not going to block this patch on it.  But I expect the following IDL to provide the exact same functionality:

void disconnect(optional unsigned long output = 0);
void disconnect(AudioNode destination, optional unsigned long output, optional unsigned long input);

In the second overload, we can know what the numeric arguments mean by looking at how many of them were passed.

Also, is there a bug on file for implementing the AudioParam versions?  Since this is not easily feature detectible, it would be nice to implement those as well in the same release as the one where we ship this one.

Comment 4

[u538564]

Attachment: 1130010.txt

Thanks for your feedback!

This patch contains all the requested corrections and implements the AudioParam versions (+ tests).

Comment 5

[Paul Adenot (:padenot)]

Comment on attachment 8615192 [details] [diff] [review]
1130010.txt

Review of attachment 8615192 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but this patch adds a lot of warnings:

> 0:31.92 In file included from /home/padenot/src/trees/mozilla-inbound/dom/media/webaudio/ConvolverNode.h:10:0,
> 0:31.92                  from /home/padenot/src/trees/mozilla-inbound/dom/media/webaudio/ConvolverNode.cpp:7,
> 0:31.93                  from /home/padenot/src/trees/mozilla-inbound/obj-x86_64-unknown-linux-gnu/dom/media/webaudio/Unified_cpp_dom_media_webaudio1.cpp:2:
> 0:31.93 Warning: -Woverloaded-virtual in /home/padenot/src/trees/mozilla-inbound/dom/media/webaudio/AudioNode.h: ‘virtual void mozilla::dom::AudioNode::Disconnect(mozilla::ErrorResult&)’ was hidden
> 0:31.93 /home/padenot/src/trees/mozilla-inbound/dom/media/webaudio/AudioNode.h:98:16: warning: ‘virtual void mozilla::dom::AudioNode::Disconnect(mozilla::ErrorResult&)’ was hidden [-Woverloaded-virtual]
> 0:31.93    virtual void Disconnect(ErrorResult& aRv);
> 0:31.93                 ^
> 0:31.93 In file included from /home/padenot/src/trees/mozilla-inbound/dom/media/webaudio/ScriptProcessorNode.cpp:7:0,
> 0:31.93                  from /home/padenot/src/trees/mozilla-inbound/obj-x86_64-unknown-linux-gnu/dom/media/webaudio/Unified_cpp_dom_media_webaudio1.cpp:128:
> 0:31.93 Warning: -Woverloaded-virtual in /home/padenot/src/trees/mozilla-inbound/dom/media/webaudio/ScriptProcessorNode.h:   by ‘virtual void mozilla::dom::ScriptProcessorNode::Disconnect(uint32_t, mozilla::ErrorResult&)’
> 0:31.93 /home/padenot/src/trees/mozilla-inbound/dom/media/webaudio/ScriptProcessorNode.h:51:16: warning:   by ‘virtual void mozilla::dom::ScriptProcessorNode::Disconnect(uint32_t, mozilla::ErrorResult&)’ [-Woverloaded-virtual]
> 0:31.93    virtual void Disconnect(uint32_t aOutput, ErrorResult& aRv) override
> 0:31.93                 ^

We need this fixed, otherwise, it will error out when pushed, because dom/media/webaudio has `FAIL_ON_WARNINGS = True` in its moz.build. Also, we need to make sure this warning is not revealing an issue. Is the right method called for ScriptProcessorNode ?

Comment 6

[u538564]

Attachment: patch

Fix warnings

Comment 7

[Paul Adenot (:padenot)]

Comment on attachment 8615898 [details] [diff] [review]
patch

Review of attachment 8615898 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/webaudio/ScriptProcessorNode.h
@@ +101,5 @@
> +    AudioNode::Disconnect(aDestination, aOutput, aRv);
> +    if (!aRv.Failed() && OutputNodes().IsEmpty() && OutputParams().IsEmpty()) {
> +      MarkInactive();
> +    }
> +  }

Looks like this could use some factoring of the methods, they share almost all of their bodies.

Comment 8

[u538564]

Attachment: 1130010_factoring1.patch

Comment 9

[Paul Adenot (:padenot)]

This is causing a number of test failures (crashes and assert failures), partly caused by the optimizations for source node where we now delete the stream when the processing has ended to safe CPU.

I've fixed a couple things, but it's still crashing and I don't have time right now to finish this.

Comment 10

[Maire Reavy [:mreavy]]

Paul: is this still p1, or should we move it to p2?   thanks

Comment 11

[Paul Adenot (:padenot)]

It's pretty important, but I can't find time to finish it...

Comment 12

[Paul Adenot (:padenot)]

Attachment: rebased/somewhat fixed patch

Some new tests are failing, I'll have a look tomorrow

MozReview-Commit-ID: 5k3NXSF2q5C

Comment 13

[Paul Adenot (:padenot)]

Attachment: Bug 1130010: Implement the new AudioNode.disconnect methods.

Initial patch by Thomas Escalon <[e-mail]>.

Review commit: https://reviewboard.mozilla.org/r/63464/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63464/

Comment 14

[Paul Adenot (:padenot)]

Comment on attachment 8769684 [details]
Bug 1130010: Implement the new AudioNode.disconnect methods.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63464/diff/1-2/

Comment 15

[Paul Adenot (:padenot)]

Attachment: Bug 1130010 - Add tests for the new {AudioParam,AudioNode}.disconnect() methods.

Initial patch by Thomas Escalon <[e-mail]>.

Review commit: https://reviewboard.mozilla.org/r/63466/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63466/

Comment 16

[Dan Minor [:dminor]]

Comment on attachment 8769684 [details]
Bug 1130010: Implement the new AudioNode.disconnect methods.

https://reviewboard.mozilla.org/r/63464/#review60638

Looks good to me.

::: dom/media/webaudio/AudioNode.cpp:412
(Diff revision 2)
> +}
> +
> +void
> +AudioNode::Disconnect(AudioNode& aDestination, ErrorResult& aRv)
> +{
> +  bool isConnected = false;

I think we should call this "wasConnected" rather than "isConnected" since it tracks whether there was a connection prior to the disconnect attempt.

Same for the other Disconnect methods.

::: dom/media/webaudio/AudioNode.cpp:422
(Diff revision 2)
> +    return;
> +  }
> +  for (int32_t inputIndex = aDestination.mInputNodes.Length() - 1; inputIndex >= 0; --inputIndex) {
> +    isConnected |= DisconnectFromOutputIfConnected(aDestination, outputIndex, inputIndex);
> +  }
> +  MOZ_ASSERT(isConnected);

Rather than an assertion this should throw an InvalidAccessError like the other Disconnect methods below.

::: dom/media/webaudio/ScriptProcessorNode.h:90
(Diff revision 2)
> +  void Disconnect(AudioNode& aDestination, uint32_t aOutput, uint32_t aInput, ErrorResult& aRv) override
> +  {
> +    AudioNode::Disconnect(aDestination, aOutput, aInput, aRv);
> +    UpdateConnectedStatus();
> +  }
> +virtual void Disconnect(AudioParam& aDestination, ErrorResult& aRv) override

nit: indent is off here, also according to the style guide, we should omit virtual and just have override.

::: dom/media/webaudio/ScriptProcessorNode.h:96
(Diff revision 2)
> +  {
> +    AudioNode::Disconnect(aDestination, aRv);
> +    UpdateConnectedStatus();
> +  }
> +
> +  virtual void Disconnect(AudioParam& aDestination, uint32_t aOutput, ErrorResult& aRv) override

nit: according to the style guide, we should remove virtual and just have override here.

::: dom/webidl/AudioNode.webidl:32
(Diff revision 2)
>      AudioNode connect(AudioNode destination, optional unsigned long output = 0, optional unsigned long input = 0);
>      [Throws]
>      void connect(AudioParam destination, optional unsigned long output = 0);
>      [Throws]
> -    void disconnect(optional unsigned long output = 0);
> +    void disconnect();
> +    [Throws]

Be careful when you land this as MozReview won't know about Ehsan's review in Bugzilla and you will hit a precommit hook if you try to use autoland :/

Comment 17

[Dan Minor [:dminor]]

Comment on attachment 8769691 [details]
Bug 1130010 - Add tests for the new  {AudioParam,AudioNode}.disconnect() methods.

https://reviewboard.mozilla.org/r/63466/#review60652

Looks like we have basic coverage for each of the methods added.

::: dom/media/webaudio/test/test_disconnectAudioParamFromOutput.html:16
(Diff revision 1)
> +      <script class="testbody" type="text/javascript">
> +      var gTest = {
> +        length: 256,
> +        numberOfChannels: 2,
> +        createGraph: function(context) {
> +          var sourceBuffer = context.createBuffer(2, 256, context.sampleRate);        

nit: trailing whitespace

::: dom/media/webaudio/test/test_disconnectExceptions.html:38
(Diff revision 1)
> +      splitter.connect(merger, 1, 1);
> +      gain2.connect(gain3);
> +      gain3.connect(ctx.destination);
> +      merger.connect(ctx.destination);
> +
> +      try {

We should probably use expectException / expectNoException here for consistency with the other tests.

::: dom/media/webaudio/test/test_disconnectExceptions.html:94
(Diff revision 1)
> +        splitter.disconnect(merger, 3, 0);
> +        ok(false, 'Should get IndexSizeError exception');
> +      } catch(e) {
> +        is(e.name, 'IndexSizeError', 'Get correct exception');
> +      }
> +      </script>   

nit: trailing whitespace

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac56ced6aba2
Implement the new AudioNode.disconnect methods. r=dminor,ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/6acd35b64dc9
Add tests for the new  {AudioParam,AudioNode}.disconnect() methods. r=dminor

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d10afcbed4e74d3a1246bb2477cb5282226491bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/05ce71421b6910fee9cf0b4451c4cd371d62bd83

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6acd35b64dc936c0e5fed7b041a81508518e324a

/builds/slave/m-in-m64-000000000000000000000/build/src/dom/media/webaudio/ScriptProcessorNode.h:53:8: error: 'Disconnect' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/178517f7c736
Implement the new AudioNode.disconnect methods. r=dminor,ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/162c0295a120
Add tests for the new  {AudioParam,AudioNode}.disconnect() methods. r=dminor

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/178517f7c736
https://hg.mozilla.org/mozilla-central/rev/162c0295a120

Comment 22

[Karl Tomlinson (:karlt)]

Comment on attachment 8769684 [details]
Bug 1130010: Implement the new AudioNode.disconnect methods.

https://reviewboard.mozilla.org/r/63464/#review69394

::: dom/media/webaudio/AudioNode.cpp
(Diff revision 2)
> -        // Destroying the InputNode here sends a message to the graph thread
> -        // to disconnect the streams, which should be sent before the
> -        // RunAfterPendingUpdates() call below.
> +  // RunAfterPendingUpdates() call below.

I assume modifying the comment was unintentional.

::: dom/media/webaudio/AudioNode.cpp
(Diff revision 2)
> -        // Remove one instance of 'dest' from mOutputNodes. There could be
> +  // Remove one instance of 'dest' from mOutputNodes. There could be
> -        // others, and it's not correct to remove them all since some of them
> +  // others, and it's not correct to remove them all since some of them
> -        // could be for different output ports.
> +  // could be for different output ports.
> -        RefPtr<AudioNode> output = mOutputNodes[i].forget();
> -        mOutputNodes.RemoveElementAt(i);
> +  RefPtr<AudioNode> output = mOutputNodes[aOutputIndex].forget();
> +  mOutputNodes.RemoveElementAt(aOutputIndex);
> -        output->NotifyInputsChanged();

I assume this change was unintentional?

Comment 23

[Dan Minor [:dminor]]

Attachment: Backout

Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=992e6d399513e1b5dec27832a6e5eb177c350315&selectedJob=25868480

Comment 24

[Maire Reavy [:mreavy]]

Comment on attachment 8781961 [details] [diff] [review]
Backout

lgtm, but I'm not a web audio peer.  I'm adding karlt as reviewer to bless the backout.

Comment 25

[Karl Tomlinson (:karlt)]

Comment on attachment 8781961 [details] [diff] [review]
Backout

Thanks.  Sorry to request the backout, but I think this is going to be easier than addressing a number of issues on 50.

Comment 26

[Dan Minor [:dminor]]

Comment on attachment 8781961 [details] [diff] [review]
Backout

This backout touches DOM so we should also have review from a DOM peer.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8781961 [details] [diff] [review]
Backout

r+, assuming this is then backed out from all the branches this is in.

Comment 28

[Dan Minor [:dminor]]

Back out pushed to: https://hg.mozilla.org/integration/mozilla-inbound/rev/beeeafe552b3729d2994e8ac0131db01a36774be

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/beeeafe552b3

Comment 30

[Ryan VanderMeulen [:RyanVM]]

This landed with the wrong bug # in the commit message. Also, I assume this needs uplift to Aurora at some point?

Comment 31

[Dan Minor [:dminor]]

Attachment: Backout (with fixed bug number)

Approval Request Comment
[Feature/regressing bug #]: 1130010
[User impact if declined]: Crashes and potential security problems.
[Describe test coverage new/current, TreeHerder]: This is a backout.
[Risks and why]: We're reverting to older code, so this should be low risk.
[String/UUID change made/needed]: None.

This fixes the bug number in the previous version of this patch.

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8782903 [details] [diff] [review]
Backout (with fixed bug number)

Backout to avoid more crashiness, Aurora50+

Comment 33

[Eric Shepherd [:sheppy]]

Do we expect these to return in 50 or are they postponed to 51 or later? This is on my to-do list for documentation work.

Comment 34

[Eric Shepherd [:sheppy]]

I've added the new versions of disconnect() to the documentation, listed as not compatible with Firefox at this time. Will update once this lands and sticks.

https://developer.mozilla.org/en-US/docs/Web/API/AudioNode/disconnect

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1514a037b750

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/38ae3688a00b
Implement the new AudioNode.disconnect methods. r=dminor,ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b34fb6f5b5f
Add tests for the new  {AudioParam,AudioNode}.disconnect() methods. r=dminor

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/38ae3688a00b
https://hg.mozilla.org/mozilla-central/rev/1b34fb6f5b5f

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This is showing up in my uplift queries because of the old patch's approval flags. Can we either drop the a+ from the patch or mark 50/51 as wontfix or something if there's no plans to uplift this?