Here's the uninteresting goop. Next I'll look at integrating the media stream properly.

This is nearly ready, but I'm stuck on how to avoid this single assertion after the test finishes: > 0:19.59 -1808828672[7f5398e85120]: Removing media stream 7f53998353a0 from the graph > 0:19.59 -1808828672[7f5398e85120]: Removing media stream 7f5399835540 from the graph > 0:19.59 -1808828672[7f5398e85120]: MediaStream 7f539958f7d0 will finish > 0:19.59 ###!!! ASSERTION: Ran out of data but track not ended?: 'endTicks == sliceEnd || track->IsEnded()', file /run/media/jdm/ssd/mozilla-central/content/media/MediaStreamGraph.cpp, line 797 > 0:19.96 mozilla::MediaStreamGraphImpl::PlayAudio(mozilla::MediaStream*, long, long) (/run/media/jdm/ssd/mozilla-central/content/media/MediaStreamGraph.cpp:798) > 0:19.96 mozilla::MediaStreamGraphImpl::RunThread() (/run/media/jdm/ssd/mozilla-central/content/media/MediaStreamGraph.cpp:1032) > 0:19.96 Run (/run/media/jdm/ssd/mozilla-central/content/media/MediaStreamGraph.cpp:1164) > 0:20.11 nsThread::ProcessNextEvent(bool, bool*) (/run/media/jdm/ssd/mozilla-central/xpcom/threads/nsThread.cpp:627) > 0:20.11 NS_ProcessNextEvent(nsIThread*, bool) (/run/media/jdm/ssd/mozilla-central/obj-x86_64-unknown-linux-gnu/xpcom/build/nsThreadUtils.cpp:238) > 0:20.11 nsThread::ThreadFunc(void*) (/run/media/jdm/ssd/mozilla-central/xpcom/threads/nsThread.cpp:264) > 0:20.12 _pt_root (/run/media/jdm/ssd/mozilla-central/nsprpub/pr/src/pthreads/ptthread.c:207) > 0:20.13 start_thread (/usr/src/debug/glibc-2.15-a316c1f/nptl/pthread_create.c:309) > 0:20.16 clone+0x0000006D (/usr/src/debug////////glibc-2.15-a316c1f/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:117) > 0:20.16 UNKNOWN (nil) The PassthroughMediaStream got rid of the endless stream of identical assertions that used to follow it, but apparently we briefly hit a state where no audio is appended to the PassthroughMediaStream but the input stream doesn't count as finished yet.

As mentioned on IRC, I think you need to call SetEnded() on your track when you're done.

All leaks and assertions are fixed.

Comment on attachment 752310 [details] [diff] [review] Implement MediaStreamAudioDestinationNode. Review of attachment 752310 [details] [diff] [review]: ----------------------------------------------------------------- Minusing because of the comments in MSADN.cpp for the most part. Also, roc should review the MSG related changes too. ::: content/media/MediaStreamGraph.h @@ +912,5 @@ > * media data, such as a camera) can write to. > */ > SourceMediaStream* CreateSourceStream(DOMMediaStream* aWrapper); > /** > + * Create a stream which doesn't modify its input. This comment doesn't make much sense. ProcessedMediaStreams generate output using their input. Actually I liked the PassthroughMediaStream name better, it reflected what this stream does a lot better. ::: content/media/webaudio/MediaStreamAudioDestinationNode.cpp @@ +8,5 @@ > +#include "mozilla/dom/MediaStreamAudioDestinationNodeBinding.h" > +#include "DOMMediaStream.h" > +#include "AudioNodeEngine.h" > +#include "AudioNodeStream.h" > +#include "../AudioStreamTrack.h" AudioStreamTrack.h is exported to mozilla/dom, no need to look at the parent dir here. @@ +21,5 @@ > + > +NS_IMPL_ADDREF_INHERITED(MediaStreamAudioDestinationNode, AudioNode) > +NS_IMPL_RELEASE_INHERITED(MediaStreamAudioDestinationNode, AudioNode) > + > +static const int AUDIO_NODE_STREAM_TRACK_ID = 1; AUDIO_NODE_STREAM_TRACK_ID is reserved for AudioNodeStreams. Please use a different ID. @@ +43,5 @@ > + bool finished; > + { > + MutexAutoLock lock(NodeMutex()); > + const nsTArray<AudioNode::InputNode>& inputs = Node()->InputNodes(); > + finished = inputs.Length() == 0 || inputs[0].mInputNode->Stream()->IsFinishedOnGraphThread(); Hmm, you can't actually do this here, since JS might keep a reference to this node and add more inputs to it in the future. Which means that you can never set *aFinished to true in a meaningful way, until the engine is destroyed. You might want to do the stuff in the |if(finished)| block below at that time. @@ +61,5 @@ > + ProcessedMediaStream* mOutputStream; > +}; > + > +MediaStreamAudioDestinationNode::MediaStreamAudioDestinationNode(AudioContext* aContext) > + : AudioNode(aContext, 2, ChannelCountMode::Explicit, ChannelInterpretation::Speakers) Not: please adjust the whitespace here similar to other existing node types. ::: content/media/webaudio/MediaStreamAudioDestinationNode.h @@ +29,5 @@ > + { > + return 0; > + } > + > + virtual void DestroyMediaStream(); Nit: MOZ_OVERRIDE. @@ +31,5 @@ > + } > + > + virtual void DestroyMediaStream(); > + > + DOMMediaStream* Stream() const; This hides AudioNode::Stream, which is bad. I also cannot see where it's being used. ::: content/media/webaudio/test/test_mediaStreamAudioDestinationNode.html @@ +30,5 @@ > + var elem = document.getElementById('audioelem'); > + elem.mozSrcObject = dest.stream; > + elem.onloadedmetadata = function() { > + ok(true, "got metadata event"); > + elem.play(); You're calling play() twice. Intentional? @@ +34,5 @@ > + elem.play(); > + setTimeout(function() { > + is(elem.played.length, 1); > + is(elem.played.start(0), 0); > + isnot(elem.played.end(0), 0); Please add a third argument for these checks. @@ +37,5 @@ > + is(elem.played.start(0), 0); > + isnot(elem.played.end(0), 0); > + SpecialPowers.clearUserPref("media.webaudio.enabled"); > + SimpleTest.finish(); > + }, 2000); Hmm, is there not an event that you can use instead of this timer?

Comment on attachment 752310 [details] [diff] [review] Implement MediaStreamAudioDestinationNode. Robert, if you could look over the MediaStreamGraph changes that would be swell.

Comment on attachment 752310 [details] [diff] [review] Implement MediaStreamAudioDestinationNode. Review of attachment 752310 [details] [diff] [review]: ----------------------------------------------------------------- Why do we need to create an extra ProcessedMediaStream here? Why can't we just return the AudioNodeStream from the node's 'stream' attribute?

Robert, here's a version that uses a single stream. You're right that it's possible, but there are stream ownership complications (that are addressed), and it causes crashes in HTMLMediaElement, which assumes that DOMMediaStreams will always have a non-null internal stream. Which direction do you prefer, this new one, or the original patch which has clearer ownership semantics?

Comment on attachment 752310 [details] [diff] [review] Implement MediaStreamAudioDestinationNode. Review of attachment 752310 [details] [diff] [review]: ----------------------------------------------------------------- I prefer the ownership model in your original patch --- you're right about that. However, I think you should make the DOM stream a TrackUnionStream that is fed by the original stream --- that would simplify your changes here, I think. You wouldn't need to call EnsureTrack on it in that case.

Comment on attachment 753889 [details] [diff] [review] Implement MediaStreamAudioDestinationNode. Robert, do you have any idea how to avoid the need for EnsureTrack in the audio engine here? I can't figure out how to use the AudioChunk properly; don't do any explicit Append operation, I get silence. When I append it to track 1 (the audio stream one), the audio seems to play at half speed (maybe because there are two identical audio chunks in a row now?). When I append to a track 2 it sounds fine.

Comment on attachment 753874 [details] [diff] [review] Implement MediaStreamAudioDestinationNode. Review of attachment 753874 [details] [diff] [review]: ----------------------------------------------------------------- Nice! Minusing because I think some of the comments that I have on MediaStreamAudioDestinationNode.cpp are serious, and I would like to look at another version of the patch, but this is really close. ::: content/media/AudioNodeStream.cpp @@ +241,4 @@ > MediaStreamListener::TRACK_EVENT_CREATED, > *segment); > } > + track = &mBuffer.AddTrack(aTrackId, IdealAudioRate(), 0, segment.forget()); Please rebase this on top of bug 873553 before landing. ::: content/media/AudioNodeStream.h @@ +108,5 @@ > > // Any thread > AudioNodeEngine* Engine() { return mEngine; } > > + StreamBuffer::Track* EnsureTrack(TrackID aTrackId); Nit: newline before protected. ::: content/media/webaudio/MediaStreamAudioDestinationNode.cpp @@ +13,5 @@ > + > +namespace mozilla { > +namespace dom { > + > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(MediaStreamAudioDestinationNode, AudioNode) This will turn into a bug if the node gets collected through the CC, since the AudioNode unlink method will null out mStream. Please use NS_IMPL_CYCLE_COLLECTION_UNLINK and NS_IMPL_CYCLE_COLLECTION_END_INHERITED. Also, you've forgotten to unlink mDOMStream here. @@ +14,5 @@ > +namespace mozilla { > +namespace dom { > + > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(MediaStreamAudioDestinationNode, AudioNode) > + tmp->FinishStream(); Thinking more about this, can FinishStream here be called _without_ DestroyMediaStream being called first? @@ +27,5 @@ > + > +NS_IMPL_ADDREF_INHERITED(MediaStreamAudioDestinationNode, AudioNode) > +NS_IMPL_RELEASE_INHERITED(MediaStreamAudioDestinationNode, AudioNode) > + > +static const int MEDIA_DESTINATION_TRACK_ID = 2; Please add a comment here and in AudioNodeStream.cpp saying that these values must not be set to the same thing. @@ +31,5 @@ > +static const int MEDIA_DESTINATION_TRACK_ID = 2; > + > +class MediaStreamDestinationEngine : public AudioNodeEngine { > +public: > + MediaStreamDestinationEngine(AudioNode* aNode) Nit: explicit. @@ +39,5 @@ > + > + virtual void ProduceAudioBlock(AudioNodeStream* aStream, > + const AudioChunk& aInput, > + AudioChunk* aOutput, > + bool* aFinished) MOZ_OVERRIDE. @@ +42,5 @@ > + AudioChunk* aOutput, > + bool* aFinished) > + { > + *aOutput = aInput; > + AudioNodeStream* as = aStream->AsAudioNodeStream(); aStream is already an AudioNodeStream. :-) @@ +50,5 @@ > + } > +}; > + > +MediaStreamAudioDestinationNode::MediaStreamAudioDestinationNode(AudioContext* aContext) > + : AudioNode(aContext, 2, ChannelCountMode::Explicit, ChannelInterpretation::Speakers) Nit: whitespace. :-) @@ +52,5 @@ > + > +MediaStreamAudioDestinationNode::MediaStreamAudioDestinationNode(AudioContext* aContext) > + : AudioNode(aContext, 2, ChannelCountMode::Explicit, ChannelInterpretation::Speakers) > +{ > + mDOMStream = DOMMediaStream::CreateTrackUnionStream(GetOwner()); Please do this in the initializer list. That should help remove some of the clutter here. @@ +55,5 @@ > +{ > + mDOMStream = DOMMediaStream::CreateTrackUnionStream(GetOwner()); > + MediaStreamDestinationEngine* engine = new MediaStreamDestinationEngine(this); > + mStream = aContext->Graph()->CreateAudioNodeStream(engine, MediaStreamGraph::INTERNAL_STREAM); > + ProcessedMediaStream* ps = mDOMStream->GetStream()->AsProcessedStream(); static_cast, like below. @@ +64,5 @@ > +MediaStreamAudioDestinationNode::DestroyMediaStream() > +{ > + FinishStream(); > + AudioNode::DestroyMediaStream(); > + if (mPort) { How can mPort be null here? @@ +73,5 @@ > + > +void > +MediaStreamAudioDestinationNode::FinishStream() > +{ > + if (mStream) { mStream should never be null here, please MOZ_ASSERT that. If it is, then we have a bug. @@ +74,5 @@ > +void > +MediaStreamAudioDestinationNode::FinishStream() > +{ > + if (mStream) { > + AudioNodeStream* as = mStream->AsAudioNodeStream(); Here we know that mStream is an AudioNodeStream. Please just do a static_cast<>, and MOZ_ASSERT(as == mStream->AsAudioNodeStream()) in order to avoid the virtual function call in opt builds, but still do a sanity check in debug builds. @@ +76,5 @@ > +{ > + if (mStream) { > + AudioNodeStream* as = mStream->AsAudioNodeStream(); > + StreamBuffer::Track* track = as->EnsureTrack(MEDIA_DESTINATION_TRACK_ID); > + track->SetEnded(); I'm not 100% sure if this is safe to do on the main thread. Please check with roc. @@ +81,5 @@ > + } > + > + if (mDOMStream) { > + if (NS_IsMainThread()) { > + mDOMStream->GetStream()->AsProcessedStream()->Finish(); You should be able to static_cast here, with a similar MOZ_ASSERT check like above. @@ +83,5 @@ > + if (mDOMStream) { > + if (NS_IsMainThread()) { > + mDOMStream->GetStream()->AsProcessedStream()->Finish(); > + } else { > + mDOMStream->GetStream()->FinishOnGraphThread(); How can this ever be called not on the main thread? That would be a bug. Please MOZ_ASSERT the correct thread at the beginning of this function, and remove this condition check. @@ +90,5 @@ > + } > +} > + > +DOMMediaStream* > +MediaStreamAudioDestinationNode::Stream() const I still think this function is unused. Am I missing something? ::: content/media/webaudio/MediaStreamAudioDestinationNode.h @@ +31,5 @@ > + } > + > + virtual void DestroyMediaStream() MOZ_OVERRIDE; > + > + DOMMediaStream* Stream() const; Rename this to DOMStream() or something. Also, newline before private. ::: content/media/webaudio/test/test_mediaStreamAudioDestinationNode.html @@ +36,5 @@ > + is(elem.played.start(0), 0, "should have played audio from the start"); > + isnot(elem.played.end(0), 0, "should have played audio for a non-zero time interval"); > + SpecialPowers.clearUserPref("media.webaudio.enabled"); > + SimpleTest.finish(); > + }, 2000); Did you check to see if there is an event which you can use here instead of the timeout? ::: dom/webidl/MediaStreamAudioDestinationNode.webidl @@ +12,5 @@ > + > +[PrefControlled] > +interface MediaStreamAudioDestinationNode : AudioNode { > + > + readonly attribute MediaStream stream; Please mark this as not-AddRefed in Bindings.conf.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11) > However, I think you should make the DOM stream a TrackUnionStream > that is fed by the original stream --- that would simplify your changes > here, I think. You wouldn't need to call EnsureTrack on it in that case. I don't think TrackUnionStream will work here. Note that for all non-destination AudioNodeStreams in the Web Audio graph, we just put silence in the track. If we connect the AudioNodeStream for this node to a TrackUnionStream underlying the DOMMediaStream, all we will see in the TrackUnionStream is going to be silence, unless we hack up MediaStreamGraphImpl::CreateOrDestroyAudioStreams in a way to not create an AudioStream for such AudioNodeStream tracks, but that seems aweful. Besides that, I don't see how we can use TrackUnionStream here.

OK.

Comment on attachment 754511 [details] [diff] [review] Implement MediaStreamAudioDestinationNode. Review of attachment 754511 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the below either addressed or explained. ::: content/media/AudioNodeStream.cpp @@ -240,5 @@ > - l->NotifyQueuedTrackChanges(Graph(), AUDIO_NODE_STREAM_TRACK_ID, IdealAudioRate(), 0, > - MediaStreamListener::TRACK_EVENT_CREATED, > - *segment); > - } > - track = &mBuffer.AddTrack(AUDIO_NODE_STREAM_TRACK_ID, IdealAudioRate(), 0, segment.forget()); Reminder about rebasing this on top of bug 873553. ::: content/media/webaudio/MediaStreamAudioDestinationNode.cpp @@ +14,5 @@ > +namespace mozilla { > +namespace dom { > + > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(MediaStreamAudioDestinationNode, AudioNode) > + tmp->FinishStream(); Thinking more about this, can FinishStream here be called _without_ DestroyMediaStream being called first? @@ +28,5 @@ > + > +NS_IMPL_ADDREF_INHERITED(MediaStreamAudioDestinationNode, AudioNode) > +NS_IMPL_RELEASE_INHERITED(MediaStreamAudioDestinationNode, AudioNode) > + > +static const int MEDIA_STREAM_DEST_TRACK_ID = 1; Please add a comment here and in AudioNodeStream.cpp saying that these values must not be set to the same thing. (And change the value here!) @@ +32,5 @@ > +static const int MEDIA_STREAM_DEST_TRACK_ID = 1; > + > +class MediaStreamDestinationEngine : public AudioNodeEngine { > +public: > + explicit MediaStreamDestinationEngine(AudioNode* aNode, ProcessedMediaStream* aOutputStream) Nit: drop explicit. @@ +55,5 @@ > + ProcessedMediaStream* mOutputStream; > +}; > + > +MediaStreamAudioDestinationNode::MediaStreamAudioDestinationNode(AudioContext* aContext) > + : AudioNode(aContext, 2, ChannelCountMode::Explicit, ChannelInterpretation::Speakers) Nit: whitespace. :-) @@ +57,5 @@ > + > +MediaStreamAudioDestinationNode::MediaStreamAudioDestinationNode(AudioContext* aContext) > + : AudioNode(aContext, 2, ChannelCountMode::Explicit, ChannelInterpretation::Speakers) > +{ > + mDOMStream = DOMMediaStream::CreatePassthroughStream(GetOwner(), DOMMediaStream::HINT_CONTENTS_AUDIO); Please do this in the initializer list. That should help remove some of the clutter here. @@ +72,5 @@ > + if (!mDOMStream || !mDOMStream->GetStream()) { > + return; > + } > + StreamBuffer::Track* track = mDOMStream->GetStream()->EnsureTrack(MEDIA_STREAM_DEST_TRACK_ID); > + track->SetEnded(); Is this method safe to get called multiple times? Also, as I said before I'm not sure if doing this from the main thread is ok. @@ +89,5 @@ > + > +DOMMediaStream* > +MediaStreamAudioDestinationNode::DOMStream() const > +{ > + return mDOMStream; Nit: please inline this method.

Comment on attachment 754511 [details] [diff] [review] Implement MediaStreamAudioDestinationNode. Review of attachment 754511 [details] [diff] [review]: ----------------------------------------------------------------- This is pretty good. However, I think we need the DOMMediaStream to keep the MediaStreamAudioDestinationNode alive. Otherwise, the MediaStreamAudioDestinationNode can be cycle-collected, causing the DOMMediaStream to finish, which is observable by Web content --- and Web content should not be able to detect when GC happens.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20) > Comment on attachment 754511 [details] [diff] [review] > Implement MediaStreamAudioDestinationNode. > > Review of attachment 754511 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is pretty good. However, I think we need the DOMMediaStream to keep the > MediaStreamAudioDestinationNode alive. Otherwise, the > MediaStreamAudioDestinationNode can be cycle-collected, causing the > DOMMediaStream to finish, which is observable by Web content --- and Web > content should not be able to detect when GC happens. Very good point!

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

Josh, I'm wrapping up the Web Audio work. If you can't land this today, do you mind if I do it for you?

Robert, could you clarify if it's safe to call StreamBuffer::Track::SetEnded from the main thread? I have a function that sends a message to the graph thread, but I don't see any way of guaranteeing that the track will still exist when the message is processed.

See comment 24.

Still waiting on Robert's feedback about SetEnded, but this version includes the potentially-unsafe replacement.

(In reply to Josh Matthews [:jdm] from comment #24) > Robert, could you clarify if it's safe to call StreamBuffer::Track::SetEnded > from the main thread? That's not safe. > I have a function that sends a message to the graph > thread, but I don't see any way of guaranteeing that the track will still > exist when the message is processed. TrackIDs are unique for the lifetime of the stream. So pass the track ID and fetch the Track on the graph thread. If it doesn't exist, you'll know.

Excellent suggestion, thanks.

Comment on attachment 759971 [details] [diff] [review] Implement MediaStreamAudioDestinationNode. Review of attachment 759971 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/StreamBuffer.cpp @@ +98,5 @@ > + > + virtual void Run() > + { > + StreamBuffer::Track* track = mStream->EnsureTrack(mTrackId, mSampleRate); > + track->SetEnded(); Check track for null!!! ::: content/media/webaudio/MediaStreamAudioDestinationNode.cpp @@ +73,5 @@ > + if (mDOMStream && mDOMStream->GetStream()) { > + MediaStream* stream = mDOMStream->GetStream(); > + StreamBuffer::Track* track = stream->EnsureTrack(MEDIA_STREAM_DEST_TRACK_ID, > + Context()->SampleRate()); > + track->SetEndedMainThread(stream); Actually, having to end the track this way is pretty ugly. Sorry, but I think we need to revisit using TrackUnionStream for the DOMStream, which would do this for you automatically.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #16) > I don't think TrackUnionStream will work here. Note that for all > non-destination AudioNodeStreams in the Web Audio graph, we just put silence > in the track. If we connect the AudioNodeStream for this node to a > TrackUnionStream underlying the DOMMediaStream, all we will see in the > TrackUnionStream is going to be silence, unless we hack up > MediaStreamGraphImpl::CreateOrDestroyAudioStreams in a way to not create an > AudioStream for such AudioNodeStream tracks, but that seems aweful. Besides > that, I don't see how we can use TrackUnionStream here. I think actually we should be using TrackUnionStream for the mDOMStream. We simply need to make MediaStreamDestinationEngine::ProduceAudioBlock put the block in its stream's buffer, then mDOMStream will pick it up. When mDOMStream's input is disconnected, the audio track is automatically removed.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #16) > > I don't think TrackUnionStream will work here. Note that for all > > non-destination AudioNodeStreams in the Web Audio graph, we just put silence > > in the track. If we connect the AudioNodeStream for this node to a > > TrackUnionStream underlying the DOMMediaStream, all we will see in the > > TrackUnionStream is going to be silence, unless we hack up > > MediaStreamGraphImpl::CreateOrDestroyAudioStreams in a way to not create an > > AudioStream for such AudioNodeStream tracks, but that seems aweful. Besides > > that, I don't see how we can use TrackUnionStream here. > > I think actually we should be using TrackUnionStream for the mDOMStream. We > simply need to make MediaStreamDestinationEngine::ProduceAudioBlock put the > block in its stream's buffer, then mDOMStream will pick it up. When > mDOMStream's input is disconnected, the audio track is automatically removed. OK, if that would work, then it sounds fine I guess.

This passes the test and doesn't output unexpected sound in my manual tests.

Comment on attachment 761167 [details] [diff] [review] Implement MediaStreamAudioDestinationNode. Review of attachment 761167 [details] [diff] [review]: ----------------------------------------------------------------- Looks great! ::: content/media/webaudio/MediaStreamAudioDestinationNode.cpp @@ +73,5 @@ > + AudioNode::DestroyMediaStream(); > + if (mPort) { > + mPort->Destroy(); > + mPort = nullptr; > + } Please call the base class function after this.

Comment on attachment 761167 [details] [diff] [review] Implement MediaStreamAudioDestinationNode. Review of attachment 761167 [details] [diff] [review]: ----------------------------------------------------------------- Much improved, thanks! Using a separate track was a good idea. ::: content/media/DOMMediaStream.cpp @@ +33,5 @@ > NS_IMPL_CYCLE_COLLECTION_UNLINK_END > NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(DOMMediaStream) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mWindow) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mTracks) > + NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mStreamDestinationNode) Can we use a subclass of DOMMediaStream so not all DOMMediaStreams need this field, please? ::: content/media/webaudio/MediaStreamAudioDestinationNode.cpp @@ +63,5 @@ > + MOZ_ASSERT(ps == mDOMStream->GetStream()->AsProcessedStream()); > + > + MediaStreamDestinationEngine* engine = new MediaStreamDestinationEngine(this, ps); > + mStream = aContext->Graph()->CreateAudioNodeStream(engine, MediaStreamGraph::INTERNAL_STREAM); > + mPort = ps->AllocateInputPort(mStream, MediaInputPort::FLAG_BLOCK_INPUT); So, mDOMStream is going to get both tracks of mStream. That's a problem. It should only get one track. We need to add something to TrackUnionStream to support filtering by track ID. I suggest a method SetTrackIDFilter that takes an object representing a callback function that checks track information and returns true if the track should be allowed to pass through to the output. Also I don't think we currently want FLAG_BLOCK_INPUT here, at least not by default, since currently Web Audio should not be blocked. So just pass 0 for the flags.

Just confirming that this is the filtering you desired.

Comment on attachment 761603 [details] [diff] [review] Implement MediaStreamAudioDestinationNode. Review of attachment 761603 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/TrackUnionStream.h @@ +108,5 @@ > } > > + // Consumers may specify a filtering callback to apply to every input track. > + // Returns true to allow the data to pass through unaltered; false to replace it > + // with silence instead. False should mean that the track doesn't even get added to the output stream, not that it gets converted to silence/null.

In that case, this seems to do the trick.

Comment on attachment 762252 [details] [diff] [review] Implement MediaStreamAudioDestinationNode. Review of attachment 762252 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/TrackUnionStream.h @@ +117,2 @@ > protected: > + TrackIDFilterCallback mFilterCallback; This needs to be initialized to null in the constructor!!!