792649 - Make the simplest of Web Audio test cases work (but not play any audio yet)

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[(no longer active)]

This is the goal here:

var context = new mozAudioContext();

function playSound(buffer) {
  var source = context.createBufferSource(); // creates a sound source
  source.buffer = buffer;                    // tell the source which sound to play
  source.connect(context.destination);       // connect the source to the context's destination (the speakers)
  source.noteOn(0);                          // play the source now
}

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 665188 [details] [diff] [review]
Patch (v1)

>+++ b/content/media/webaudio/AudioBufferSourceNode.cpp
>-NS_IMPL_ISUPPORTS_INHERITED0(AudioBufferSourceNode, AudioSourceNode)
>+NS_IMPL_CYCLE_COLLECTION_CLASS(AudioBufferSourceNode)

I wonder whether it's worth it adding INHERITED versions of NS_IMPL_CYCLE_COLLECTION_1 and friends.



>+++ b/content/media/webaudio/AudioNode.cpp

>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSTARRAY_OF_NSCOMPTR(mInputs)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSTARRAY_OF_NSCOMPTR(mOutputs)

So this depends on two things:

1)  The entries of those arrays implenting a get() that returns AudioNode*
2)  The CC macros using .get()

right?

That seems kinda fragile to me, but I guess it's OK if we add a comment here _and_ in the CC macro.

>+AudioNode::Connect(AudioNode& aDestination, uint32_t aOutput,
>+  if (Context() != aDestination.Context()) {
>+    aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
>+    return;

This is not mentioned in the spec.  Perhaps it should be?

What _is_ mentioned in the spec is cycle detection.  Should we be doing that?


>+AudioNode::Disconnect(uint32_t aOutput, ErrorResult& aRv)

>+  const OutputSlot output = mOutputs[aOutput];
>+  const InputSlot input = output.mDestination->mInputs[output.mInputSlot];

But what if "output" is not initialized?  So say someone connect()s output #1 but then tries to disconnect() output #0?  I think we want to be making sure "output" is actually connected.  And the lack of such a check in the spec looks like a spec bug, at first glance.  Unless disconnect() is meant to be idempotent?

And in particular, it seems like disconnect(0) should do the same thing whether connect(1) was called or not, no?

>+  if (!output || !input ||
>+      Context() != output.mDestination->Context()) {

Can we not assert the context equality?

If not, again it needs to be in the spec.

You should also assert aOutput == input.mOutputSlot, since you depend on it below.

>+++ b/content/media/webaudio/AudioNode.h
>+  struct OutputSlot {

I would name this struct "Output".

>+    nsRefPtr<AudioNode> mDestination;
>+    const uint32_t mInputSlot;

And maybe mInput?  And either way, document that it's an index into mDestination->mInputs.

>+  struct InputSlot {

And here, "Input"

>+    nsRefPtr<AudioNode> mSource;
>+    const uint32_t mOutputSlot;

And here, maybe mOutput and certainly a comment about this being an index into mSource.mOutputs.

>+++ b/dom/webidl/AudioDestinationNode.webidl
>+    readonly attribute AudioContext context;

Why is that needed?

>+++ b/dom/webidl/AudioNode.webidl
>+    readonly attribute unsigned long numberOfInputs;
>+    readonly attribute unsigned long numberOfOutputs;

I don't understand these, per spec.  Your impl is such that connecting up input #1 will say we have 2 inputs, even though input #0 is not connected up.  Is that the behavior the spec calls for?

Comment 3

[(no longer active)]

(In reply to Boris Zbarsky (:bz) from comment #2)
> Comment on attachment 665188 [details] [diff] [review]
> Patch (v1)
> 
> >+++ b/content/media/webaudio/AudioBufferSourceNode.cpp
> >-NS_IMPL_ISUPPORTS_INHERITED0(AudioBufferSourceNode, AudioSourceNode)
> >+NS_IMPL_CYCLE_COLLECTION_CLASS(AudioBufferSourceNode)
> 
> I wonder whether it's worth it adding INHERITED versions of
> NS_IMPL_CYCLE_COLLECTION_1 and friends.

I actually thought about that but I was too lazy to do that (and wanted to avoid a full rebuild...) but none of those are good reasons, so let me know if you want me to do that here.

> >+++ b/content/media/webaudio/AudioNode.cpp
> 
> >+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSTARRAY_OF_NSCOMPTR(mInputs)
> >+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSTARRAY_OF_NSCOMPTR(mOutputs)
> 
> So this depends on two things:
> 
> 1)  The entries of those arrays implenting a get() that returns AudioNode*
> 2)  The CC macros using .get()
> 
> right?
> 
> That seems kinda fragile to me, but I guess it's OK if we add a comment here
> _and_ in the CC macro.

Yeah, that's probably not worth it.  I'll just change this to not use the macros at all.

> >+AudioNode::Connect(AudioNode& aDestination, uint32_t aOutput,
> >+  if (Context() != aDestination.Context()) {
> >+    aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
> >+    return;
> 
> This is not mentioned in the spec.  Perhaps it should be?

Yes.  I will be maintaining my own version of the spec where I would make these kinds of fixes and will merge them back to the main spec from time to time.  I'll drop you a link to that when I set it up.

> What _is_ mentioned in the spec is cycle detection.  Should we be doing that?

Yes, I'm planning on doing that in the future.  We also need to make sure that we implement the lifetime management details that the spec requires.  This patch is by no means ship quality yet.  :-)

> >+AudioNode::Disconnect(uint32_t aOutput, ErrorResult& aRv)
> 
> >+  const OutputSlot output = mOutputs[aOutput];
> >+  const InputSlot input = output.mDestination->mInputs[output.mInputSlot];
> 
> But what if "output" is not initialized?  So say someone connect()s output
> #1 but then tries to disconnect() output #0?  I think we want to be making
> sure "output" is actually connected.  And the lack of such a check in the
> spec looks like a spec bug, at first glance.  Unless disconnect() is meant
> to be idempotent?

I agree on both.  I should fix the spec to say that disconnect would throw in that case, and detect (and test) it.

> And in particular, it seems like disconnect(0) should do the same thing
> whether connect(1) was called or not, no?

That is my understanding.

> >+  if (!output || !input ||
> >+      Context() != output.mDestination->Context()) {
> 
> Can we not assert the context equality?

No.

> If not, again it needs to be in the spec.

For sure.

> You should also assert aOutput == input.mOutputSlot, since you depend on it
> below.

Good point.

> >+++ b/content/media/webaudio/AudioNode.h
> >+  struct OutputSlot {
> 
> I would name this struct "Output".

OK, will do.

> >+    nsRefPtr<AudioNode> mDestination;
> >+    const uint32_t mInputSlot;
> 
> And maybe mInput?  And either way, document that it's an index into
> mDestination->mInputs.

Sure.  Note that this setup is probably too simplistic so it will most likely be overhauled in the future.

> >+  struct InputSlot {
> 
> And here, "Input"
> 
> >+    nsRefPtr<AudioNode> mSource;
> >+    const uint32_t mOutputSlot;
> 
> And here, maybe mOutput and certainly a comment about this being an index
> into mSource.mOutputs.

Will do.

> >+++ b/dom/webidl/AudioDestinationNode.webidl
> >+    readonly attribute AudioContext context;
> 
> Why is that needed?

It's not, my bad.

> >+++ b/dom/webidl/AudioNode.webidl
> >+    readonly attribute unsigned long numberOfInputs;
> >+    readonly attribute unsigned long numberOfOutputs;
> 
> I don't understand these, per spec.  Your impl is such that connecting up
> input #1 will say we have 2 inputs, even though input #0 is not connected
> up.  Is that the behavior the spec calls for?

The spec doesn't really call for anything...  This is sort of what WebKit does, and I need to make more sense out of this (and potentially change this in the future), and definitely spec whatever makes sense here.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> let me know if you want me to do that here.

That would be great.

> I will be maintaining my own version of the spec where I would make these kinds of fixes

Ok, great.  As long as they make it back to the spec eventually!

> Yes, I'm planning on doing that in the future.

OK.  An XXX comment is probably ok for now.

> I should fix the spec to say that disconnect would throw in that case, and detect (and
> test) it.

Sounds good.

> No.

Why not?  I mean, we're connected to output.mDestination, right?  How could we get connected if we have different contexts?

> This is sort of what WebKit does

So connecting up input #1 will say we have 2 inputs in webkit, even though input #0 is not connected to anything?

That's just screwed up.  :(

Comment 5

[(no longer active)]

(In reply to Ehsan Akhgari [:ehsan] from comment #3)
> > >+AudioNode::Disconnect(uint32_t aOutput, ErrorResult& aRv)
> > 
> > >+  const OutputSlot output = mOutputs[aOutput];
> > >+  const InputSlot input = output.mDestination->mInputs[output.mInputSlot];
> > 
> > But what if "output" is not initialized?  So say someone connect()s output
> > #1 but then tries to disconnect() output #0?  I think we want to be making
> > sure "output" is actually connected.  And the lack of such a check in the
> > spec looks like a spec bug, at first glance.  Unless disconnect() is meant
> > to be idempotent?
> 
> I agree on both.  I should fix the spec to say that disconnect would throw
> in that case, and detect (and test) it.

Actually I'm already doing that check -- just need to take out the MOZ_ASSERTs.

Comment 6

[(no longer active)]

(In reply to Boris Zbarsky (:bz) from comment #4)
> > No.
> 
> Why not?  I mean, we're connected to output.mDestination, right?  How could
> we get connected if we have different contexts?

You're right, my bad!

> > This is sort of what WebKit does
> 
> So connecting up input #1 will say we have 2 inputs in webkit, even though
> input #0 is not connected to anything?
> 
> That's just screwed up.  :(

Yeah.  I'll bring it up on public-audio, and will fix our implementation once that's cleared.

Comment 7

[(no longer active)]

Attachment: Patch (v2)

I think this should address all of your comments.

Comment 8

[(no longer active)]

Attachment: Patch (v3)

The only change in this version of the patch is that it uses the ConvertibleToBool idiom.  This would have caught one bug that I found when working on this locally.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 667243 [details] [diff] [review]
Patch (v3)

>+++ b/content/media/webaudio/AudioNode.cpp
>+AudioNode::Disconnect(uint32_t aOutput, ErrorResult& aRv)
>+  const Input input = output.mDestination->mInputs[output.mInput];

This is still busted if "output" is not initialized.  We need to check for that before doing this.

>+      aOutput != input.mOutput) {

How could that test false, given how we got input?  I was suggesting we MOZ_ASSERT that it's true...

r=me with those dealt with

Comment 10

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a4d62a24e05

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7a4d62a24e05

Comment 12

[(no longer active)]

Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.