Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AudioNode.disconnect() needs to be able to disconnect only one connection #6

Closed
olivierthereaux opened this issue Sep 11, 2013 · 16 comments

Comments

@olivierthereaux
Copy link
Contributor

commented Sep 11, 2013

Originally reported on W3C Bugzilla ISSUE-17793 Tue, 17 Jul 2012 18:30:24 GMT
Reported by Chris Wilson
Assigned to

(Summary of email conversation in list)

There is currently no way to disconnect node A's connection to node B without disconnecting all connections from node A to other nodes. This makes it impossible to disconnect node B from the graph without potential side effects, as you have to:

  • call disconnect() on node A (which disconnects all its outputs)
  • reconnect every connection that node A used to have, EXCEPT the connection to node B.

Not only is this cumbersome, it will be problematic in the future when we solve the related issue of unconnected streams - which is currently exhibiting incorrect behavior in Chrome (it pauses the audio stream), but is underspecified in the spec today. (filing separate bug). Disconnecting then reconnecting would have to have no side effects. (It works okay today, but not ideal - can click.)

Recommended solution:

  • there should be a way to remove a single connection (by supplying the destination node to be disconnected, since there can only be one connection to a given destination node [tested]).

E.g.: the IDL for disconnect should read:

    void disconnect(in [Optional] AudioNode destination, in [Optional] unsigned long output = 0)
        raises(DOMException);

this lets us keep most compatibility - node.disconnect() will still remove all connections.

@olivierthereaux

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2013

Original comment by Chris Rogers on W3C Bugzilla. Mon, 30 Jul 2012 20:16:50 GMT

(In reply to comment #0)

(Summary of email conversation in list)

There is currently no way to disconnect node A's connection to node B without
disconnecting all connections from node A to other nodes. This makes it
impossible to disconnect node B from the graph without potential side effects,
as you have to:

  • call disconnect() on node A (which disconnects all its outputs)
  • reconnect every connection that node A used to have, EXCEPT the connection to
    node B.

Not only is this cumbersome, it will be problematic in the future when we solve
the related issue of unconnected streams - which is currently exhibiting
incorrect behavior in Chrome (it pauses the audio stream), but is
underspecified in the spec today. (filing separate bug). Disconnecting then
reconnecting would have to have no side effects. (It works okay today, but not
ideal - can click.)

Recommended solution:

  • there should be a way to remove a single connection (by supplying the
    destination node to be disconnected, since there can only be one connection to
    a given destination node [tested]).

E.g.: the IDL for disconnect should read:

    void disconnect(in [Optional] AudioNode destination, in [Optional]

unsigned long output = 0)
raises(DOMException);

this lets us keep most compatibility - node.disconnect() will still remove all
connections.

I agree we need to extend disconnect() with more optional parameters to allow disconnecting specific connections.

I think we'll also need to add an "input" parameter similar to the connect() method to be able to say which specific input to disconnect:

void disconnect(in AudioNode destination, in [Optional] unsigned long output = 0, in [Optional] unsigned long input = 0)
raises(DOMException);

This would make the API look the same as connect(), and allow disconnecting an exact connection.

@cwilso cwilso self-assigned this May 28, 2014
@cwilso cwilso added this to the V1 milestone May 28, 2014
@cwilso cwilso modified the milestones: V1, Web Audio v.1 Oct 17, 2014
@mdjp mdjp added the V1 (TPAC 2014) label Oct 20, 2014
@cwilso

This comment has been minimized.

Copy link
Contributor

commented Oct 27, 2014

"We should just do it."

@adelespinasse

This comment has been minimized.

Copy link

commented Oct 28, 2014

I don't think this really needs to be a breaking change, does it? It can be an overloaded function with three forms:

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

The first form is the old one, which disconnects all connections from the given output (at least, I think that's what it does; the spec isn't clear). The other two correspond exactly to the connect methods, and only remove one connection. Since their first arguments have no default and aren't numeric, there's no ambiguity.

The old form can be deprecated, but shouldn't need to be removed right away.

I've been wanting this for years, and am glad to see it on the table.

@cwilso

This comment has been minimized.

Copy link
Contributor

commented Oct 28, 2014

It's not a breaking change. (Although it may be challenging to use until everyone implements the new one.)

@cwilso

This comment has been minimized.

Copy link
Contributor

commented Oct 28, 2014

Related: #343.

@cwilso cwilso modified the milestones: Web Audio v 1.0, Web Audio Last Call 1 Oct 29, 2014
@hoch

This comment has been minimized.

Copy link
Member

commented Feb 5, 2015

From the teleconference and the discussion after, I would like to propose:

  1. For invalid input/output indices, we should throw IndexSizeError.
  2. For invalid destination AudioNode or AudioParam, we should throw InvalidAccessError.

http://www.w3.org/TR/dom/#domerror

@cwilso @padenot What do you think?

@cwilso

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2015

LGTM.

On Thu, Feb 5, 2015 at 10:34 AM, Hongchan Choi notifications@github.com
wrote:

From the teleconference and the discussion after, I would like to propose:

  1. For invalid input/output indices, we should throw IndexSizeError.
  2. For invalid destination AudioNode or AudioParam, we should throw
    InvalidAccessError.

http://www.w3.org/TR/dom/#domerror

@cwilso https://github.com/cwilso @padenot https://github.com/padenot
What do you think?


Reply to this email directly or view it on GitHub
#6 (comment)
.

@padenot

This comment has been minimized.

Copy link
Member

commented Feb 6, 2015

Yep, LGTM as well.

hoch added a commit to hoch/web-audio-api that referenced this issue Feb 6, 2015
@hoch

This comment has been minimized.

Copy link
Member

commented Feb 6, 2015

@hoch

This comment has been minimized.

Copy link
Member

commented Mar 2, 2015

The proposal above at #6 (comment) was not really well-defined for all possible cases. Please refer the IDL in the pull-request #479 I created.

cwilso added a commit that referenced this issue Mar 3, 2015
#6 Support selective disconnection
dstockwell pushed a commit to dstockwell/blink that referenced this issue Mar 4, 2015
Currently AudioNode.disconnect() disconnects all outputs of the AudioNode. However, it is useful to be able to disconnect just one of the connections between the AudioNode output and another input.

See discussion: 
WebAudio/web-audio-api#6
WebAudio/web-audio-api#479

Audio WG agreed to add this feature to Web Audio API.

BUG=448071

Review URL: https://codereview.chromium.org/886173004

git-svn-id: svn://svn.chromium.org/blink/trunk@191235 bbb929c8-8fbe-4397-9dbb-9b2b20218538
@sebpiq

This comment has been minimized.

Copy link

commented Mar 20, 2015

This is great to have this ...

A little concern though : is there any (sane) way to automatically detect that this is indeed implemented? Cause if you use it while not implemented, it's going to fail silently, and just disconnect all your connections (which you probably didn't expect).

@rtoy

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2015

One possible way: node1.disconnect(node2) where you know node1 wasn't
connected to node2. This is required to throw an error now. I don't think
that happened before. At least in Chrome 42 beta, no error is thrown. I
didn't try any other browser.

On Fri, Mar 20, 2015 at 2:00 AM, Sebastien Piquemal <
notifications@github.com> wrote:

This is great to have this ...

A little concern though : is there any (sane) way to automatically detect
that this is indeed implemented? Cause if you use it while not implemented,
it's going to fail silently, and just disconnect all your connections
(which you probably didn't expect).


Reply to this email directly or view it on GitHub
#6 (comment)
.

Ray

@sebpiq

This comment has been minimized.

Copy link

commented Mar 20, 2015

Yeah but if an error is not raised, that would not really tell me that you can disconnect a single connection ... instead of doing that, I might as well test the browser versions!

The other hack I was thinking of is using an OfflineAudioContext, but it sounds like going through a lot of trouble ...

@hoch

This comment has been minimized.

Copy link
Member

commented Mar 20, 2015

@sebpiq Yes. I should have thought about that. I agree that we don't have a 'sane' way of sniffing this. For the time being, the browser version check is the most concise solution.

Here's the snippet based on @rtoy's idea:

function hasSelectiveDisconnect() {
  var c = new OfflineAudioContext(1,1,44100);
  try {
    c.createGain().disconnect(c.destination);
    return false;
  } catch (error) { 
    return true;  
  }
}

console.log(hasSelectiveDisconnect());

For this case, OfflineAudioContext is slightly better than the real-time one, because the browser might have a limitation on the number of real-time contexts can be running or constructed.

@sebpiq

This comment has been minimized.

Copy link

commented Mar 21, 2015

Yeah ... I was doing something more verbose like so :

var _hasSelectiveDisconnect = function() {
  var context = new OfflineAudioContext(1, 1, 44100)
    , bufferNode = context.createBufferSource(), gain = context.createGain()
    , buffer = context.createBuffer(1, 1, 44100)
  buffer.getChannelData(0)[0] = 1
  bufferNode.buffer = buffer
  bufferNode.connect(gain)
  bufferNode.connect(context.destination)
  bufferNode.start(0)
  gain.connect(context.destination)
  bufferNode.disconnect(gain)
  context.oncomplete = function(event) {
    _hasSelectiveDisconnectResult = (!!event.renderedBuffer.getChannelData(0)[0])
  }
  context.startRendering()
}, _hasSelectiveDisconnectResult = null
_hasSelectiveDisconnect()
@cwilso

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2015

Fixed.

@cwilso cwilso closed this Jun 2, 2015
dstockwell pushed a commit to dstockwell/chromium that referenced this issue Sep 23, 2015
Currently AudioNode.disconnect() disconnects all outputs of the AudioNode. However, it is useful to be able to disconnect just one of the connections between the AudioNode output and another input.

See discussion: 
WebAudio/web-audio-api#6
WebAudio/web-audio-api#479

Audio WG agreed to add this feature to Web Audio API.

BUG=448071

Review URL: https://codereview.chromium.org/886173004

git-svn-id: svn://svn.chromium.org/blink/trunk@191235 bbb929c8-8fbe-4397-9dbb-9b2b20218538
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.