Closed Bug 1130010 Opened 5 years ago Closed 3 years ago

Implement the new AudioNode.disconnect methods

Categories

(Core :: Web Audio, defect, P1)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: padenot, Assigned: padenot)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [spec])

Attachments

(4 files, 5 obsolete files)

Those methods allows authors to disconnect a particular node:

https://github.com/WebAudio/web-audio-api/issues/6
Priority: -- → P1
Attached patch First draft (obsolete) — Splinter Review
Attachment #8610608 - Flags: review?(padenot)
Attachment #8610608 - Attachment is patch: true
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.
Attachment #8610608 - Flags: review?(padenot)
Attachment #8610608 - Flags: review?(ehsan)
Attachment #8610608 - Flags: review+
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.
Attachment #8610608 - Flags: review?(ehsan) → review+
Attached patch 1130010.txt (obsolete) — Splinter Review
Thanks for your feedback!

This patch contains all the requested corrections and implements the AudioParam versions (+ tests).
Attachment #8615192 - Flags: review?(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 ?
Attachment #8615192 - Flags: review?(padenot)
Attached patch patch (obsolete) — Splinter Review
Fix warnings
Attachment #8610608 - Attachment is obsolete: true
Attachment #8615192 - Attachment is obsolete: true
Attachment #8615898 - Flags: review?(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.
Attachment #8615898 - Flags: review?(padenot)
Attached patch 1130010_factoring1.patch (obsolete) — Splinter Review
Attachment #8616594 - Flags: review?(padenot)
Attachment #8616594 - Flags: review?(padenot) → review+
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.
Paul: is this still p1, or should we move it to p2?   thanks
Rank: 15
Flags: needinfo?(padenot)
Whiteboard: [spec]
It's pretty important, but I can't find time to finish it...
Flags: needinfo?(padenot)
Assignee: nobody → padenot
Some new tests are failing, I'll have a look tomorrow

MozReview-Commit-ID: 5k3NXSF2q5C
Attachment #8615898 - Attachment is obsolete: true
Attachment #8616594 - Attachment is obsolete: true
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/
Attachment #8769684 - Attachment description: Bug 1130010 - Add tests for the new {AudioParam,AudioNode}.disconnect() methods. → Bug 1130010: Implement the new AudioNode.disconnect methods.
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 :/
Attachment #8769684 - Flags: review?(dminor) → review+
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
Attachment #8769691 - Flags: review?(dminor) → review+
Pushed by paul@paul.cx:
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
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]
Flags: needinfo?(padenot)
Pushed by paul@paul.cx:
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
Flags: needinfo?(padenot)
https://hg.mozilla.org/mozilla-central/rev/178517f7c736
https://hg.mozilla.org/mozilla-central/rev/162c0295a120
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1291702
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8781961 - Flags: review?(mreavy)
Attachment #8781961 - Flags: review?(karlt)
Attachment #8781961 - Flags: review+
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.
Attachment #8781961 - Flags: review?(karlt) → review+
Comment on attachment 8781961 [details] [diff] [review]
Backout

This backout touches DOM so we should also have review from a DOM peer.
Attachment #8781961 - Flags: review?(bugs)
Comment on attachment 8781961 [details] [diff] [review]
Backout

r+, assuming this is then backed out from all the branches this is in.
Attachment #8781961 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/beeeafe552b3
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: mozilla50 → mozilla51
This landed with the wrong bug # in the commit message. Also, I assume this needs uplift to Aurora at some point?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla51 → ---
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.
Attachment #8781961 - Attachment is obsolete: true
Attachment #8782903 - Flags: approval-mozilla-aurora?
Comment on attachment 8782903 [details] [diff] [review]
Backout (with fixed bug number)

Backout to avoid more crashiness, Aurora50+
Attachment #8782903 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1294492
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.
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
Pushed by paul@paul.cx:
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
https://hg.mozilla.org/mozilla-central/rev/38ae3688a00b
https://hg.mozilla.org/mozilla-central/rev/1b34fb6f5b5f
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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?
Flags: needinfo?(padenot)
Flags: needinfo?(padenot)
You need to log in before you can comment on or make changes to this bug.