Closed Bug 881959 Opened 11 years ago Closed 11 years ago

Investigate support for cycles in the MediaStreamGraph with delay nodes present

Categories

(Core :: Web Audio, defect)

Product:
Core
Component:
Web Audio
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla27
Tracking Flags:
Tracking Status
firefox25 + fixed
firefox26 + fixed
firefox27 + fixed

People

(Reporter: ehsan.akhgari, Assigned: padenot)

References

Details

(Whiteboard: [blocking-webaudio+])

Attachments

(7 files, 3 obsolete files)

Testcase
324.14 KB, application/x-gzip
Details
Mute WebAudio nodes that are part of a cycle that contains no DelayNode, and make cycle work. r=
13.28 KB, patch
ehsan.akhgari
: review+
akeybl
: approval-mozilla-aurora+
akeybl
: approval-mozilla-beta+
Details | Diff | Splinter Review
Tell the MediaStreamGraph when changes to the graph occur. r=
3.32 KB, patch
roc
: review+
Details | Diff | Splinter Review
Warn the author when a cycle in a WebAudio graph does not contain a DelayNode. r=
5.07 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Tests for cycles in WebAudio graphs. r=
5.65 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Clamp the DelayNode.delayTime to 128/AudioContext.sampleRate when in a cycle. r=
3.38 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Handle self-connections
2.01 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review 
I've never properly tested this, but I believe that most things should work fine.  We may allow creating loops without DelayNodes in them, which is bad.

Paul, can you look into this once you get a chance, please?
Probably better to talk about cycles rather than "looping" since looping usually refers to AudioBufferSourceNodes.
Summary: Investigate looping support with the presence of delay nodes → Investigate support for cycles in the MediaStreamGraph with delay nodes present
Sure, I'll have a look and write tests.
(In reply to comment #1)
> Probably better to talk about cycles rather than "looping" since looping
> usually refers to AudioBufferSourceNodes.

Oops you're right.  English fail ;-)
Assignee: nobody → paul
Attached file Testcase — 
The situation here is full of sadness.

First, spec issue: the spec does not say what should happen if we have a cycle that contains a delay node, but with a delayTime param of zero. We need to fix this. Considering a DelayNode with delayTime = 0.0 is basically noop, I propose it should throw.

Also, what should we do if, when a graph is running, the delayTime value for a DelayNode in a cycle becomes 0.0? A couple options:
(1) throw, and stop the graph, maybe logging an informative error message in the console
(2) don't throw, and mute the subgraph portion of the graph that has a cycle, maybe logging an informative error message in the console
(3) a combination of (1) and (2)

I think (1) if a reasonable choice, because most of the time, this is a programming error. But if we have an application that allows a user to build a WebAudio graph (like [1]), there should be a way for the author to be aware of the situation and display an informative UI. I'll send an email to the list for this issue.

Then, current implementation behaviour:

The attached testcase has three different cases:
(1) graph with a cycle, with a delay node in the cycle, with delayTime = 0.0 ;
(2) graph with a cycle, with a delay node in the cycle, with delayTime = 0.5 ;
(3) graph with a cycle, without a delay node in the cycle.

In Firefox, we never output sound.

In Chrome, it outputs sound in every case, but in fact, we only hear a glitchy buzz (like what would happen if you have a delay with very short time, in fact, having played with that in pure data and the like). It does even throw, as specced, in case (3).

[1]: http://webaudioplayground.appspot.com/
(In reply to comment #4)
> Created attachment 793517 [details]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=793517&action=edit
> Testcase
> 
> The situation here is full of sadness.

That's what I expected.  :-)

> First, spec issue: the spec does not say what should happen if we have a cycle
> that contains a delay node, but with a delayTime param of zero. We need to fix
> this. Considering a DelayNode with delayTime = 0.0 is basically noop, I propose
> it should throw.

Throwing is probably the wrong thing to do, since you can first construct the cycle with non-zero delay and then change the delay to 0.  Or, manipulate the AudioParam to cause a 0 delay.  We won't know about this until we start processing the graph.

> Also, what should we do if, when a graph is running, the delayTime value for a
> DelayNode in a cycle becomes 0.0? A couple options:
> (1) throw, and stop the graph, maybe logging an informative error message in
> the console
> (2) don't throw, and mute the subgraph portion of the graph that has a cycle,
> maybe logging an informative error message in the console
> (3) a combination of (1) and (2)
> 
> I think (1) if a reasonable choice, because most of the time, this is a
> programming error. But if we have an application that allows a user to build a
> WebAudio graph (like [1]), there should be a way for the author to be aware of
> the situation and display an informative UI. I'll send an email to the list for
> this issue.

I'd vote for (3).  I don't think we can reasonably throw here, so we should just consider no-delay loops that we can't reliably detect as silent inputs, and warn on the web console about them.

> Then, current implementation behaviour:
> 
> The attached testcase has three different cases:
> (1) graph with a cycle, with a delay node in the cycle, with delayTime = 0.0 ;
> (2) graph with a cycle, with a delay node in the cycle, with delayTime = 0.5 ;
> (3) graph with a cycle, without a delay node in the cycle.
> 
> In Firefox, we never output sound.

Hurray!  ;-)

> In Chrome, it outputs sound in every case, but in fact, we only hear a glitchy
> buzz (like what would happen if you have a delay with very short time, in fact,
> having played with that in pure data and the like). It does even throw, as
> specced, in case (3).

Actually, 0 may not be an interesting threshold here, since what we really want is a full block of audio, so perhaps we should change the threshold in the spec and our implementation to 128/sampleRate?
> I'd vote for (3).  I don't think we can reasonably throw here, so we should
> just consider no-delay loops that we can't reliably detect as silent inputs,
> and warn on the web console about them.

This is (2), then. I'll open a bug and propose this.

> Actually, 0 may not be an interesting threshold here, since what we really
> want is a full block of audio, so perhaps we should change the threshold in
> the spec and our implementation to 128/sampleRate?

Yes, 128 / samplerate makes perfect sense.
(In reply to comment #6)
> > I'd vote for (3).  I don't think we can reasonably throw here, so we should
> > just consider no-delay loops that we can't reliably detect as silent inputs,
> > and warn on the web console about them.
> 
> This is (2), then. I'll open a bug and propose this.

Hehe, yes, sorry!

> > Actually, 0 may not be an interesting threshold here, since what we really
> > want is a full block of audio, so perhaps we should change the threshold in
> > the spec and our implementation to 128/sampleRate?
> 
> Yes, 128 / samplerate makes perfect sense.

Cool...  Note that another solution here could be to manually clamp the minimum delay time in DelayNode's in a cycle to 128/sampleRate, then there is a chance that everything may "just work".
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #7)
> Cool...  Note that another solution here could be to manually clamp the
> minimum delay time in DelayNode's in a cycle to 128/sampleRate, then there
> is a chance that everything may "just work".

I think this is what we should do.
Yes, and people agree on the spec bug [1], so I'll go and amend the spec. I've started to write patches to (1) handle cycles, instead of outputting silence and (2) clamp the delayTime value to 128, and (3), throw the required exception on |connect()|. For now, it segfaults somewhere in AudioNodeStream::ProduceOutput, though.

[1]: https://www.w3.org/Bugs/Public/show_bug.cgi?id=23037
(In reply to comment #9)
> Yes, and people agree on the spec bug [1], so I'll go and amend the spec. I've
> started to write patches to (1) handle cycles, instead of outputting silence
> and (2) clamp the delayTime value to 128, and (3), throw the required exception
> on |connect()|. For now, it segfaults somewhere in
> AudioNodeStream::ProduceOutput, though.

We should probably fix that before taking your patch.  ;-)
This implements the exception when an author makes a cycle that does not contain
a delay node.
Attachment #795977 - Flags: review?(ehsan)
This is a test for the previous patch.
Attachment #795978 - Flags: review?(ehsan)
This implements the delayTime clamping when a DelayNode is in a cycle.
Attachment #795979 - Flags: review?(ehsan)
Comment on attachment 795977 [details] [diff] [review]
Throw an exception when connecing nodes in a cycle without at least one DelayNode. r=

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

So, we have cycle detection code in MediaStreamGraphImpl::UpdateStreamOrderForStream (see mInCycle) and we use it inside AudioNodeStream::ProduceOutput.  How do you reconcile that code with this?  It seems like at the very least we should remove that code in this patch.

::: content/media/webaudio/AudioNode.cpp
@@ +180,5 @@
>    }
>  
> +  // Check wether this connect() makes a cycle that does not contain at least
> +  // one DelayNode.
> +  if (!CheckCycle()) {

Hmm, I think here it's too late to check for a cycle, since for example NotifyInputConnected() may have already been called, and we would have already created a port, etc.

@@ +184,5 @@
> +  if (!CheckCycle()) {
> +    // Remove the illegal connection
> +    Disconnect(aOutput, aRv);
> +
> +    nsCOMPtr<nsPIDOMWindow> pWindow = do_QueryInterface(GetParentObject()->GetParentObject());

Nit: please use Context(), it makes the code a bit more readable.

@@ +204,5 @@
>    Context()->UpdatePannerSource();
>  }
>  
> +bool
> +AudioNode::DepthFirstTraversal(AudioNode* aNode, std::set<AudioNode*> aCycleSet)

The name of this function is very misleading, it does more than just depth-first traversal.  :-)

@@ +210,5 @@
> +  // If we find a node we already know, we have a cycle.
> +  if (aCycleSet.find(aNode) != aCycleSet.end()) {
> +    // Check that we have at least a DelayNode
> +    bool delayNodePresent = false;
> +    for (std::set<AudioNode*>::iterator it = aCycleSet.begin(); it != aCycleSet.end(); it++) {

const_iterator.

@@ +212,5 @@
> +    // Check that we have at least a DelayNode
> +    bool delayNodePresent = false;
> +    for (std::set<AudioNode*>::iterator it = aCycleSet.begin(); it != aCycleSet.end(); it++) {
> +      if ((*it)->AsDelayNode()) {
> +        delayNodePresent = true;

Can't you get rid of this variable and just return true here, and return false after the loop?

::: content/media/webaudio/AudioNode.h
@@ +132,5 @@
>    virtual AudioBufferSourceNode* AsAudioBufferSourceNode() {
>      return nullptr;
>    }
>  
> +  virtual DelayNode* AsDelayNode() {

Nit: const.

@@ +245,5 @@
>    nsRefPtr<MediaStream> mStream;
>  
>  private:
> +
> +  bool CheckCycle();

Please document the return value of this method.

@@ +246,5 @@
>  
>  private:
> +
> +  bool CheckCycle();
> +  bool DepthFirstTraversal(AudioNode* aNode, std::set<AudioNode*> cycleSet);

Hmm, I think you meant to pass the set by reference?  You can move the <set> include to the cpp file, and just forward-declare std::set in the header if you pass it by reference.

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +83,5 @@
>  WithCredentialsSyncXHRWarning=Use of XMLHttpRequest's withCredentials attribute is no longer supported in the synchronous mode in window context.
>  TimeoutSyncXHRWarning=Use of XMLHttpRequest's timeout attribute is not supported in the synchronous mode in window context.
>  JSONCharsetWarning=An attempt was made to declare a non-UTF-8 encoding for JSON retrieved using XMLHttpRequest. Only UTF-8 is supported for decoding JSON.
> +# LOCALIZATION NOTE: Do not translate DelayNode
> +AudioNodeCycleWithoutDelay=Making a cycles with nodes is only allowed if the cycle contains at least one DelayNode.

"Making a cycle".  I'm not a native speaker, but I think I'd write this as:

Cycles in the audio graph are only supported if they contain at least one DelayNode.
Attachment #795977 - Flags: review?(ehsan) → review-
Attachment #795978 - Flags: review?(ehsan) → review+
Comment on attachment 795979 [details] [diff] [review]
Clamp the DelayNode.delayTime to 128/AudioContext.sampleRate when in a cycle. r=

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

Minusing because of the comment below, but the logic here seems fine.

::: content/media/webaudio/DelayNode.cpp
@@ +123,5 @@
>      float* const* outputChannels = reinterpret_cast<float* const*>
>        (const_cast<void* const*>(aOutput->mChannelData.Elements()));
>  
> +
> +    bool inCycle = aStream->AsProcessedStream()->InCycle();

Using mInCycle here seems wasteful.  Can't we just send this information as a parameter to the AudioNodeStream for the delay nodes from the main thread?  That way we can run the cycle detection code only once, as opposed to once on the main thread and once on the MSG thread.
Attachment #795979 - Flags: review?(ehsan) → review-
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #14)
> Comment on attachment 795977 [details] [diff] [review]
> Throw an exception when connecing nodes in a cycle without at least one
> DelayNode. r=
> 
> Review of attachment 795977 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So, we have cycle detection code in
> MediaStreamGraphImpl::UpdateStreamOrderForStream (see mInCycle) and we use
> it inside AudioNodeStream::ProduceOutput.  How do you reconcile that code
> with this?  It seems like at the very least we should remove that code in
> this patch.

Well, UpdateStreamOrderForStream decides in which order the MediaStreams should be computed. The code in this patch decides if a given AudioNode graph is valid, from the point of view of the WebAudio spec.

The former can handle much more than WebAudio (say, a MediaStream from gUM feeding in a WebAudio graph, feeding to a PeerConnection), but does not know anything about DelayNodes and cycles (and probably does not want to know), the latter does not care about anything but making sure we can't make an invalid graph (and has no idea about things outside of WebAudio).

Yes, the algorithms are the same, but they are not performing it at the same level (MediaStream vs. AudioNode), same time (connect() vs. RunThread()), same thread (main thread vs. MSG thread), same constraints (need to be able to synchronously dispatch an exception for this patch).

Also, the code in AudioNodeStream::ProduceOutput obviously has to be modified for cycles to work at all. It does not work right now with those patches, it fatal-asserts somewhere in ObtainInputBlock, because mLastChunks.Length() is 1 and try to get the second value of the array (haven't solved it yet).

> @@ +246,5 @@
> >  
> >  private:
> > +
> > +  bool CheckCycle();
> > +  bool DepthFirstTraversal(AudioNode* aNode, std::set<AudioNode*> cycleSet);
> 
> Hmm, I think you meant to pass the set by reference?  You can move the <set>
> include to the cpp file, and just forward-declare std::set in the header if
> you pass it by reference.

We can do that, with some `remove` calls somewhere so the backtracking still works fine. This is just laziness from me, there.
Whiteboard: [blocking-webaudio+]
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #15)
> Comment on attachment 795979 [details] [diff] [review]
> Clamp the DelayNode.delayTime to 128/AudioContext.sampleRate when in a
> cycle. r=
> 
> Review of attachment 795979 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Minusing because of the comment below, but the logic here seems fine.
> 
> ::: content/media/webaudio/DelayNode.cpp
> @@ +123,5 @@
> >      float* const* outputChannels = reinterpret_cast<float* const*>
> >        (const_cast<void* const*>(aOutput->mChannelData.Elements()));
> >  
> > +
> > +    bool inCycle = aStream->AsProcessedStream()->InCycle();
> 
> Using mInCycle here seems wasteful.  Can't we just send this information as
> a parameter to the AudioNodeStream for the delay nodes from the main thread?
> That way we can run the cycle detection code only once, as opposed to once
> on the main thread and once on the MSG thread.

Well, we have to do a topological sort anyway to order the streams for processing. Setting a flag on the stream is not the expensive part here.

I just don't want to duplicate this state, and we are forced to set it in UpdateStreamOrder anyway.
(In reply to Paul Adenot (:padenot) from comment #16)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #14)
> > Comment on attachment 795977 [details] [diff] [review]
> > Throw an exception when connecing nodes in a cycle without at least one
> > DelayNode. r=
> > 
> > Review of attachment 795977 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > So, we have cycle detection code in
> > MediaStreamGraphImpl::UpdateStreamOrderForStream (see mInCycle) and we use
> > it inside AudioNodeStream::ProduceOutput.  How do you reconcile that code
> > with this?  It seems like at the very least we should remove that code in
> > this patch.
> 
> Well, UpdateStreamOrderForStream decides in which order the MediaStreams
> should be computed. The code in this patch decides if a given AudioNode
> graph is valid, from the point of view of the WebAudio spec.
> 
> The former can handle much more than WebAudio (say, a MediaStream from gUM
> feeding in a WebAudio graph, feeding to a PeerConnection), but does not know
> anything about DelayNodes and cycles (and probably does not want to know),
> the latter does not care about anything but making sure we can't make an
> invalid graph (and has no idea about things outside of WebAudio).

I don't think that's true.  IIRC roc added that code specifically for Web Audio cycles.  You should check with roc.

> Yes, the algorithms are the same, but they are not performing it at the same
> level (MediaStream vs. AudioNode), same time (connect() vs. RunThread()),
> same thread (main thread vs. MSG thread), same constraints (need to be able
> to synchronously dispatch an exception for this patch).
> 
> Also, the code in AudioNodeStream::ProduceOutput obviously has to be
> modified for cycles to work at all. It does not work right now with those
> patches, it fatal-asserts somewhere in ObtainInputBlock, because
> mLastChunks.Length() is 1 and try to get the second value of the array
> (haven't solved it yet).

Yep.

> > @@ +246,5 @@
> > >  
> > >  private:
> > > +
> > > +  bool CheckCycle();
> > > +  bool DepthFirstTraversal(AudioNode* aNode, std::set<AudioNode*> cycleSet);
> > 
> > Hmm, I think you meant to pass the set by reference?  You can move the <set>
> > include to the cpp file, and just forward-declare std::set in the header if
> > you pass it by reference.
> 
> We can do that, with some `remove` calls somewhere so the backtracking still
> works fine. This is just laziness from me, there.

Heh, fair enough.
Flags: needinfo?(roc)
I know the spec says to throw an exception for cycles without DelayNodes but I think that's wrong. Online cycle detection can be expensive. I think we're much better off simply saying that cycles without DelayNodes produce only silence. I thought I mentioned this on the list a while back but I can't find it.
Flags: needinfo?(roc)
I was thinking that we could do the topological sort/cycle detection only when changing the MSG topology, to limit the runtime cost. That was what ehsan did in bug 883010 but his patch got backed out for some reason. We will need a patch like this sooner or later anyway, |UpdateStreamOrder| shows up high in the profiles.

For what is worth, the same problem has been solved in a couple different ways in other graph-based audio software: Pd "throws an exception" in the form of an error message in their console, and stops the processing, while Max/MSP seem to silently mute the output.

In any case, I think we should warn the user when a connect() produces an illegal cycle: while it reasonably easy to detect a cycle looking a graphical representation of a processing graph, it can be harder when all you have is a bunch of connect() statement scattered all over the place.
Checking for cycles in every connect() call is expensive --- that's what I mean by "online cycle detection". That can lead to O(N^2) costs when constructing certain kinds of graphs. Checking for cycles one every iteration of the MSG thread is not expensive; it's O(N) in the size of the graph. However, then it's too late to synchronously throw an exception in connect(), obviously.

So I suggest on the MSG side we run UpdateStreamOrder is infrequently as possible (we can also optimize it on other ways; for example any node with no inputs can be safely placed at the beginning of the order, so creating and destroying AudioBufferSourceNodes should be cheap). When it detects cycles, they get muted. We could have it then send a message to the main thread identifying the MediaStreams in the cycle, which could be picked up by devtools to report the nodes in the cycle. Reporting the entire cycle would make it easier to figure out than just the latest connect() statement that completed the cycle --- if we can figure out how to identify nodes to the Web author. I guess devtools could list the JS expandos of the nodes and authors could add their own properties for debugging.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #21)
> Checking for cycles in every connect() call is expensive --- that's what I
> mean by "online cycle detection". That can lead to O(N^2) costs when
> constructing certain kinds of graphs. Checking for cycles one every
> iteration of the MSG thread is not expensive; it's O(N) in the size of the
> graph. However, then it's too late to synchronously throw an exception in
> connect(), obviously.

Right, I think I misunderstood the meaning of "online cycle detection" here.

> So I suggest on the MSG side we run UpdateStreamOrder is infrequently as
> possible (we can also optimize it on other ways; for example any node with
> no inputs can be safely placed at the beginning of the order, so creating
> and destroying AudioBufferSourceNodes should be cheap). When it detects
> cycles, they get muted. We could have it then send a message to the main
> thread identifying the MediaStreams in the cycle, which could be picked up
> by devtools to report the nodes in the cycle. Reporting the entire cycle
> would make it easier to figure out than just the latest connect() statement
> that completed the cycle --- if we can figure out how to identify nodes to
> the Web author. I guess devtools could list the JS expandos of the nodes and
> authors could add their own properties for debugging.

As long as the author is warned and can debug his code in some ways, it seems fine. The devtools people are here in Paris this week, I'll ask if it's easy to have clickable JS objects in the console, as I'm not sure it's something we do at the moment.
Comment 21 makes sense to me.  FWIW I think we can solve the devtools issue separately, it doesn't need to block our progress here.
Agreed. Now, we _just_ have to convince the world comment 21 is the right way to do it.
(In reply to comment #24)
> Agreed. Now, we _just_ have to convince the world comment 21 is the right way
> to do it.

Lucky us, cause you signed up to do that work, didn't you?  ;-)
I guess this is correct. I'll do a followup to roc's post to the mailing list explaining the reasoning behind his proposal, now everything's clear.
(In reply to comment #26)
> I guess this is correct. I'll do a followup to roc's post to the mailing list
> explaining the reasoning behind his proposal, now everything's clear.

Thanks a lot!  :-)
So, I've implemented the "new" way of doing this: muting the cycle when it does not contain at least one DelayNode.

It's still asserting here [1] _when the DelayNode.delayTime.value is not null_, because:

> (gdb) p a->mLastChunks.Length()
> $4 = 0
> (gdb) p i
> $5 = 1
> (gdb) p mInputs[i]->OutputNumber()
> $6 = 0
> (gdb) p inputCount
> $7 = 2

If the delayTime is null, we clamp it to 128/samplerate, and it produces a horrible noise, that I believe is expected (I haven't put damping after the delay, so it energy builds up in the cycle). When I have a delayTime like 0.5s, it crashs. Hopefully I can solve it tomorrow, but maybe someone has the solution. 

[1]: http://mxr.mozilla.org/mozilla-central/source/content/media/AudioNodeStream.cpp#289
I found the issue and explained it in a comment in the patch.
Attachment #804504 - Flags: review?(ehsan)
This ehsan's patch from bug 883010. I removed the lazy-reorder part, we can
add it later.
Attachment #804505 - Flags: review?(roc)
This is essentially the same test as last time, modified because we mute instead
of throwing, now.
Attachment #804507 - Flags: review?(ehsan)
Attachment #795978 - Attachment is obsolete: true
This is the same patch as last time. IsInCycle() seem the way to go, now.
Attachment #804508 - Flags: review?(ehsan)
Attachment #795979 - Attachment is obsolete: true
Attachment #795977 - Attachment is obsolete: true
Comment on attachment 804504 [details] [diff] [review]
Mute WebAudio nodes that are part of a cycle that contains no DelayNode, and make cycle work. r=

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

::: content/media/AudioNodeStream.cpp
@@ +290,5 @@
> +    // It is possible for mLastChunks to be empty here, because `a` might be a
> +    // AudioNodeStream that has not been scheduled yet, because it is further
> +    // down the graph _but_ as a connection to this node. Because we enforce the
> +    // presence of at least one DelayNode, with at least one block of delay, and
> +    // because the output of a DelayNode when it has been feed less that

Nit: fed.

::: content/media/webaudio/DelayNode.cpp
@@ +43,5 @@
>      , mLeftOverData(INT32_MIN)
>    {
>    }
>  
> +  virtual DelayNodeEngine* AsDelayNodeEngine()

Nit: please add MOZ_OVERRIDE here.

::: content/media/webaudio/DelayNode.h
@@ +31,5 @@
>    {
>      return mDelay;
>    }
>  
> +  const DelayNode* AsDelayNode() const

Nit: please add virtual and MOZ_OVERRIDE here.

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +83,5 @@
>  WithCredentialsSyncXHRWarning=Use of XMLHttpRequest's withCredentials attribute is no longer supported in the synchronous mode in window context.
>  TimeoutSyncXHRWarning=Use of XMLHttpRequest's timeout attribute is not supported in the synchronous mode in window context.
>  JSONCharsetWarning=An attempt was made to declare a non-UTF-8 encoding for JSON retrieved using XMLHttpRequest. Only UTF-8 is supported for decoding JSON.
> +# LOCALIZATION NOTE: Do not translate DelayNode
> +AudioNodeCycleWithoutDelay=Cycles in audio graphs are only supported if they contain at least one DelayNode.

Nit: please move this to attachment 804506 [details] [diff] [review].
Attachment #804504 - Flags: review?(ehsan) → review+
Comment on attachment 804504 [details] [diff] [review]
Mute WebAudio nodes that are part of a cycle that contains no DelayNode, and make cycle work. r=

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

::: content/media/webaudio/AudioNode.cpp
@@ +8,5 @@
>  #include "mozilla/ErrorResult.h"
>  #include "AudioNodeStream.h"
>  #include "AudioNodeEngine.h"
>  #include "mozilla/dom/AudioParam.h"
> +#include "nsIScriptError.h"

Also, this seems to be unnecessary.
Comment on attachment 804506 [details] [diff] [review]
Warn the author when a cycle in a WebAudio graph does not contain a DelayNode. r=

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

Note that since this patch will contain l10n changes, we can't uplift it without explicit approval.  I personally think this can just be in Firefox 26 and not 25, but if you want it uplifted to 25, please request approval explicitly.

::: content/media/MediaStreamGraph.cpp
@@ +496,5 @@
> +  private:
> +    MediaStream* mStream;
> +  };
> +
> +  nsCOMPtr<nsIRunnable> event = new MediaStreamGraphWarnCycleRunnable(aStream);

Please retrieve the AudioNode* here, and hold a strong reference to it inside MediaStreamGraphWarnCycleRunnable.
Attachment #804506 - Flags: review?(ehsan) → review+
Comment on attachment 804507 [details] [diff] [review]
Tests for cycles in WebAudio graphs. r=

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

::: content/media/webaudio/test/test_delayNodeCycles.html
@@ +12,5 @@
> +
> +SimpleTest.waitForExplicitFinish();
> +
> +addLoadEvent(function() {
> +function getSineBuffer(ctx) {

Nit: please indent everything in this function.
Attachment #804507 - Flags: review?(ehsan) → review+
Comment on attachment 804508 [details] [diff] [review]
Clamp the DelayNode.delayTime to 128/AudioContext.sampleRate when in a cycle. r=

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

::: content/media/MediaStreamGraph.h
@@ +942,5 @@
>     * Forward SetTrackEnabled() to the input MediaStream(s) and translate the ID
>     */
>    virtual void ForwardTrackEnabled(TrackID aOutputID, bool aEnabled) {};
>  
> +  bool InCycle() { return mInCycle; }

Nit: const.
Attachment #804508 - Flags: review?(ehsan) → review+
Thanks to test_bug875221.html, where the fuzzer did such self connection.
Attachment #805320 - Flags: review?(ehsan)
Attachment #805320 - Flags: review?(ehsan) → review+
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #36)
> ::: content/media/MediaStreamGraph.cpp
> @@ +496,5 @@
> > +  private:
> > +    MediaStream* mStream;
> > +  };
> > +
> > +  nsCOMPtr<nsIRunnable> event = new MediaStreamGraphWarnCycleRunnable(aStream);
> 
> Please retrieve the AudioNode* here, and hold a strong reference to it
> inside MediaStreamGraphWarnCycleRunnable.

AudioNode is cycle collected on the main thread.
If this is not the main thread, then a reference cannot be added on this thread and AudioNode* can only be used here while NodeMutex() is held.
See also bug 914392.

Do nodes wait for responses from their destroy message to the MSG before they delete themselves?

If not, then Run() needs to check whether the node still exists.  Run() doesn't need a strong reference because it is on the main thread.
Comment on attachment 804504 [details] [diff] [review]
Mute WebAudio nodes that are part of a cycle that contains no DelayNode, and make cycle work. r=

For Aurora, this is for all the patches in this bug. For Beta, we can't land the patch that warns the user about cycles, because it would introduce a new string.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): this is the last bit of feature work for WebAudio, planned to go in 25. I missed the uplift because the tree was closed all day (european time) yesterday.
User impact if declined: People won't have a feature complete WebAudio implementation.
Testing completed (on m-c, etc.): Has green tests, landed on inbound
Risk to taking this patch (and alternatives if risky): Tested manually, has unit tests.
String or IDL/UUID changes made by this patch: none for beta, a single js console string for Aurora.
Attachment #804504 - Flags: approval-mozilla-beta?
Attachment #804504 - Flags: approval-mozilla-aurora?
(In reply to comment #40)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #36)
> > ::: content/media/MediaStreamGraph.cpp
> > @@ +496,5 @@
> > > +  private:
> > > +    MediaStream* mStream;
> > > +  };
> > > +
> > > +  nsCOMPtr<nsIRunnable> event = new MediaStreamGraphWarnCycleRunnable(aStream);
> > 
> > Please retrieve the AudioNode* here, and hold a strong reference to it
> > inside MediaStreamGraphWarnCycleRunnable.
> 
> AudioNode is cycle collected on the main thread.
> If this is not the main thread, then a reference cannot be added on this thread
> and AudioNode* can only be used here while NodeMutex() is held.
> See also bug 914392.

Yeah, IIRC we discussed this on IRC.

> Do nodes wait for responses from their destroy message to the MSG before they
> delete themselves?

No (and that is a problem.)

> If not, then Run() needs to check whether the node still exists.  Run() doesn't
> need a strong reference because it is on the main thread.

We need to make the lifetime of the stream (and the engine) a subset of the lifetime of the node at some point.  That would make checking for the liveliness of the node unneeded.
(In reply to Paul Adenot (:padenot) from comment #43)
> Bustage fix on Windows.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/924b0619e616

+  NS_IMETHOD Run()
+  {
+    AudioNodeEngine* engine = mStream->AsAudioNodeStream()->Engine();
+    MutexAutoLock mon(engine->NodeMutex());
+    AudioNode* node = engine->Node();
+    nsCOMPtr<nsPIDOMWindow> pWindow = do_QueryInterface(node->Context()->GetParentObject());

Still need a null check on |node|.
But that's not the cause of the crashes in comment 45.
(In reply to Karl Tomlinson (:karlt) from comment #46)
> But that's not the cause of the crashes in comment 45.

Well at least I don't know how AudioNode::mContext could be at offset 0x28 and at offset 0xf4.

I don't think Context() could be null on a Node referenced by a stream, because, if AudioNode::Unlink() has been called to remove the context, then the node would have removed itself from the stream.
I'm going to reland this without the "warn the user" part, because [1] tells me it is the problem.

[1]: https://tbpl.mozilla.org/?tree=Try&rev=dae2036d994a
Attachment #804504 - Flags: approval-mozilla-beta?
Attachment #804504 - Flags: approval-mozilla-beta+
Attachment #804504 - Flags: approval-mozilla-aurora?
Attachment #804504 - Flags: approval-mozilla-aurora+
Depends on: 926619
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: