roc knows what needs to be done here. The summary should sort of match the reality I hope. :-)

roc, can you please attach the patches that you have so far here? Thanks!

I have the basics working --- AudioBufferSourceNodes playing audio. I'll clean up my patches and post them on Monday.

This compensation by adding blocked time isn't really useful as far as I can tell, and it complicated things for AudioNodeStreams that we'll want to treat as never blocked.

Part 5 lets us implement AudioNodeStreams in their own file. We could later split up MediaStreamGraph.cpp some more too.

This is the interface between WebAudio and the MediaStreamGraph.

With these patches and the patches in bug 830707, this testcase works (plays a series of rising tones).

Comment on attachment 704434 [details] [diff] [review] Part 8: Create AudioNodeEngine and AudioNodeStream Review of attachment 704434 [details] [diff] [review]: ----------------------------------------------------------------- I think you forgot to add AudioNodeStream.cpp to the patch. Clearing the flag for that. This patch was a lot more straightforward to understand than I would have guessed! :-) ::: content/media/AudioNodeEngine.cpp @@ +28,5 @@ > + > +void > +WriteZeroesToAudioBlock(AudioChunk* aChunk, uint32_t aStart, uint32_t aLength) > +{ > + NS_ASSERTION(aStart + aLength <= WEBAUDIO_BLOCK_SIZE, "Bad buffer end"); MOZ_ASSERT? @@ +65,5 @@ > + for (uint32_t i = 0; i < WEBAUDIO_BLOCK_SIZE; ++i) { > + aOutput[i] = aInput[i]*aScale; > + } > + } > +} Hmm, these two functions are unused, as far as I can tell. Also, if aScale comes from the result of a computation, is the equality check here really safe? ::: content/media/AudioNodeEngine.h @@ +27,5 @@ > +public: > + /** > + * Construct with null data. > + */ > + ThreadSharedFloatArrayBufferList(uint32_t aCount) Nit: explicit. @@ +41,5 @@ > + } > + ~Storage() { free(mDataToFree); } > + void* mDataToFree; > + const float* mSampleData; > + }; I'd make this a private member. Outside code should not be messing with Storage objects, right? @@ +43,5 @@ > + void* mDataToFree; > + const float* mSampleData; > + }; > + > + uint32_t GetChannels() { return mContents.Length(); } Nit: please make this const. @@ +47,5 @@ > + uint32_t GetChannels() { return mContents.Length(); } > + /** > + * This may return null if we had an OOM during a copy. > + */ > + const float* GetData(uint32_t aIndex) const { return mContents[aIndex].mSampleData; } Might be worth adding a MOZ_ASSERT to ensure the correct length of mContents. @@ +51,5 @@ > + const float* GetData(uint32_t aIndex) const { return mContents[aIndex].mSampleData; } > + > + void SetData(uint32_t aIndex, void* aDataToFree, const float* aData) > + { > + Storage* s = &mContents[aIndex]; Same here. @@ +88,5 @@ > + float aOutput[WEBAUDIO_BLOCK_SIZE]); > + > +class AudioNodeEngine { > +public: > + AudioNodeEngine() {} This class requires a virtual dtor, doesn't it? ::: content/media/AudioNodeStream.h @@ +6,5 @@ > +#ifndef MOZILLA_AUDIONODESTREAM_H_ > +#define MOZILLA_AUDIONODESTREAM_H_ > + > +#include "MediaStreamGraph.h" > +#include "AudioNodeEngine.h" Hmm, I think you can just forward-declare AudioNodeEngine here. @@ +33,5 @@ > +class AudioNodeStream : public ProcessedMediaStream { > +public: > + enum { AUDIO_TRACK = 1 }; > + > + AudioNodeStream(AudioNodeEngine* aEngine) Nit: please make this explicit. Also please document that this transfers the ownership of AudioNodeEngine (although this is already documented in MediaStreamGraph::CreateAudioNodeStream). ::: content/media/MediaStreamGraph.cpp @@ +907,5 @@ > + } > + } > + t = next; > + } > + NS_ASSERTION(t == aTo, "Something went wrong with rounding to block boundaries"); Can this happen in practice? If not, let's make this a MOZ_ASSERT. @@ +1949,5 @@ > +{ > + AudioNodeStream* stream = new AudioNodeStream(aEngine); > + NS_ADDREF(stream); > + static_cast<MediaStreamGraphImpl*>(this)->AppendMessage(new CreateMessage(stream)); > + return stream; I'd write this using nsRefPtr... But doesn't matter much.

Comment on attachment 704435 [details] [diff] [review] Part 9: Update WebAudio implementation to integrate with AudioNodeStream Review of attachment 704435 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/AudioBuffer.cpp @@ +117,5 @@ > +StealJSArrayDataIntoThreadSharedFloatArrayBufferList(JSContext* aJSContext, > + const nsTArray<JSObject*>& aJSArrays) > +{ > + nsRefPtr<ThreadSharedFloatArrayBufferList> result = > + new ThreadSharedFloatArrayBufferList(aJSArrays.Length()); I think the allocation here and in GetThreadSharedChannelForRate should all be fallible, since the number of channels comes from content. @@ +126,5 @@ > + uint8_t* stolenData; > + if (JS_StealArrayBufferContents(aJSContext, arrayBuffer, &dataToFree, > + &stolenData)) { > + result->SetData(i, dataToFree, reinterpret_cast<float*>(stolenData)); > + } Hmm, if one of these JS_StrealArrayBufferContents calls fails, the returned data structure contains "holes". Would that be handled in a sane way? ::: content/media/webaudio/AudioBuffer.h @@ +28,5 @@ > > +/** > + * An AudioBuffer keeps its data either in the mJSChannels objects, which > + * are Float32Arrays, or in mSharedChannels if the mJSChannels objects have > + * been neutered according to mJSChannelsNeutered. mJSChannelsNeutered does not exist. ::: content/media/webaudio/AudioBufferSourceNode.cpp @@ +20,5 @@ > > NS_IMPL_ADDREF_INHERITED(AudioBufferSourceNode, AudioSourceNode) > NS_IMPL_RELEASE_INHERITED(AudioBufferSourceNode, AudioSourceNode) > > +class AudioBufferSourceNodeEngine : public AudioNodeEngine Do we have guarantees on which thread calls the member functions of AudioBufferSourceNodeEngine? If yes, it would be great if we can add assertions to that effect. ::: content/media/webaudio/AudioContext.cpp @@ +28,5 @@ > > NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(AudioContext, AddRef) > NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(AudioContext, Release) > > +static uint8_t gWebAudioOutputKey; Is this key just treated as an opaque void*? If that is the case, I wonder if this could be implemented as a static enum or something? (Definitely outside of the scope of this patch though.) ::: content/media/webaudio/AudioContext.h @@ +37,5 @@ > class PannerNode; > > +/** > + * An AudioContext listens for main-thread events on its mDestination node's > + * stream. Is this really true? Does this comment belong to AudioBufferSourceNode? @@ +99,5 @@ > > + uint32_t GetRate() { return IdealAudioRate(); } > + > + MediaStreamGraph* Graph(); > + MediaStream* DestinationStream(); Please make these const if possible. ::: content/media/webaudio/AudioDestinationNode.h @@ +23,3 @@ > { > public: > + explicit AudioDestinationNode(AudioContext* aContext, MediaStreamGraph* aGraph); Please drop explicit. ::: content/media/webaudio/AudioNode.cpp @@ +51,5 @@ > +AudioNode::~AudioNode() > +{ > + DestroyMediaStream(); > + NS_ASSERTION(mInputNodes.IsEmpty(), "Inputs still connected?"); > + NS_ASSERTION(mOutputNodes.IsEmpty(), "Outputs still connected?"); I'm a fan of using more MOZ_ASSERTs as you can probably tell by now. ;-) @@ +166,5 @@ > + input->mInputPort = aInput; > + input->mOutputPort = aOutput; > + // Connect streams in the MediaStreamGraph > + NS_ASSERTION(aDestination.mStream->AsProcessedStream(), > + "Destination stream has max inputs > 0 but is not a ProcessedMediaStream"); For the cast below to be safe, this has definitely got to be a MOZ_ASSERT. ::: content/media/webaudio/AudioNode.h @@ +62,5 @@ > > NS_DECL_CYCLE_COLLECTING_ISUPPORTS > + NS_DECL_CYCLE_COLLECTION_CLASS(AudioNode) > + > + void JSBindingFinalized() This is meant to be called in the future, right? (Just sanity checking.) @@ +64,5 @@ > + NS_DECL_CYCLE_COLLECTION_CLASS(AudioNode) > + > + void JSBindingFinalized() > + { > + mJSBindingFinalized = true; We should probably MOZ_ASSERT to make sure this is not called multiple times on the same node. @@ +92,4 @@ > > + // This could possibly delete 'this'. > + void UpdateOutputEnded(); > + bool IsOutputEnded() { return mOutputEnded; } Nit: const. @@ +138,5 @@ > + nsTArray<InputNode> mInputNodes; > + // For every mOutputNode entry, there is a corresponding entry in mInputNodes > + // of the mOutputNode entry. We won't necessarily be able to identify the > + // exact matching entry, since mOutputNodes doesn't include the port > + // identifiers and the same node could be connected on multiple ports. Hmm, I wonder if this is fine... Can't think of why it wouldn't be right now, and I guess we can revisit this in the future if needed. ::: dom/webidl/AudioBufferSourceNode.webidl @@ +15,5 @@ > > + //const unsigned short UNSCHEDULED_STATE = 0; > + //const unsigned short SCHEDULED_STATE = 1; > + //const unsigned short PLAYING_STATE = 2; > + //const unsigned short FINISHED_STATE = 3; Why did you comment these out? @@ +28,5 @@ > //attribute boolean loop; > > + [Throws] > + void start(optional double when = 0, optional double grainOffset = 0, > + optional double grainDuration); This doesn't match the spec -- |when| should not be optional. @@ +30,5 @@ > + [Throws] > + void start(optional double when = 0, optional double grainOffset = 0, > + optional double grainDuration); > + [Throws] > + void stop(optional double when = 0); Ditto.

(In reply to :Ehsan Akhgari from comment #15) > @@ +65,5 @@ > > + for (uint32_t i = 0; i < WEBAUDIO_BLOCK_SIZE; ++i) { > > + aOutput[i] = aInput[i]*aScale; > > + } > > + } > > +} > > Hmm, these two functions are unused, as far as I can tell. They're used in AudioNodeStream.cpp. > Also, if aScale comes from the result of a computation, is the equality > check here really safe? Most of the time it will not be the result of a computation. > @@ +41,5 @@ > > + } > > + ~Storage() { free(mDataToFree); } > > + void* mDataToFree; > > + const float* mSampleData; > > + }; > > I'd make this a private member. Outside code should not be messing with > Storage objects, right? There is no way to get access to the Storage elements of a ThreadSharedFloatArrayBufferList so it's more convenient to make these fields public. > @@ +47,5 @@ > > + uint32_t GetChannels() { return mContents.Length(); } > > + /** > > + * This may return null if we had an OOM during a copy. > > + */ > > + const float* GetData(uint32_t aIndex) const { return mContents[aIndex].mSampleData; } > > Might be worth adding a MOZ_ASSERT to ensure the correct length of mContents. nsTArray already does a bounds check assertion. > ::: content/media/MediaStreamGraph.cpp > @@ +907,5 @@ > > + } > > + } > > + t = next; > > + } > > + NS_ASSERTION(t == aTo, "Something went wrong with rounding to block boundaries"); > > Can this happen in practice? If not, let's make this a MOZ_ASSERT. I don't think it can, but if I'm wrong then I don't think it's worth dying over.

(In reply to :Ehsan Akhgari from comment #16) > ::: content/media/webaudio/AudioBuffer.cpp > @@ +117,5 @@ > > +StealJSArrayDataIntoThreadSharedFloatArrayBufferList(JSContext* aJSContext, > > + const nsTArray<JSObject*>& aJSArrays) > > +{ > > + nsRefPtr<ThreadSharedFloatArrayBufferList> result = > > + new ThreadSharedFloatArrayBufferList(aJSArrays.Length()); > > I think the allocation here and in GetThreadSharedChannelForRate should all > be fallible, since the number of channels comes from content. I think, given it's the allocation of the internal array that can fail, I will just make ThreadSharedFloatArrayBufferList have no channels in that case and handle that in consumers by treating it as silence. > @@ +126,5 @@ > > + uint8_t* stolenData; > > + if (JS_StealArrayBufferContents(aJSContext, arrayBuffer, &dataToFree, > > + &stolenData)) { > > + result->SetData(i, dataToFree, reinterpret_cast<float*>(stolenData)); > > + } > > Hmm, if one of these JS_StrealArrayBufferContents calls fails, the returned > data structure contains "holes". Would that be handled in a sane way? Probably not. I'll fix that to go into the same "no channels" state as for OOM. > ::: content/media/webaudio/AudioBufferSourceNode.cpp > @@ +20,5 @@ > > > > NS_IMPL_ADDREF_INHERITED(AudioBufferSourceNode, AudioSourceNode) > > NS_IMPL_RELEASE_INHERITED(AudioBufferSourceNode, AudioSourceNode) > > > > +class AudioBufferSourceNodeEngine : public AudioNodeEngine > > Do we have guarantees on which thread calls the member functions of > AudioBufferSourceNodeEngine? If yes, it would be great if we can add > assertions to that effect. All methods of AudioNodeEngine and its subclasses are called on the MediaStreamGraph thread. Unfortunately there is no easy way to assert that without giving the AudioNodeEngine a reference to the MediaStreamGraph, which would add complexity to shutdown. > ::: content/media/webaudio/AudioContext.cpp > @@ +28,5 @@ > > > > NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(AudioContext, AddRef) > > NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(AudioContext, Release) > > > > +static uint8_t gWebAudioOutputKey; > > Is this key just treated as an opaque void*? If that is the case, I wonder > if this could be implemented as a static enum or something? It just has to be a unique key. This approach is simple, safe, and avoids having to have a centralized enum that new clients need to add to. > ::: content/media/webaudio/AudioContext.h > @@ +37,5 @@ > > class PannerNode; > > > > +/** > > + * An AudioContext listens for main-thread events on its mDestination node's > > + * stream. > > Is this really true? Does this comment belong to AudioBufferSourceNode? It's not true; that comment is obsolete. Removed. > ::: content/media/webaudio/AudioNode.cpp > @@ +51,5 @@ > > +AudioNode::~AudioNode() > > +{ > > + DestroyMediaStream(); > > + NS_ASSERTION(mInputNodes.IsEmpty(), "Inputs still connected?"); > > + NS_ASSERTION(mOutputNodes.IsEmpty(), "Outputs still connected?"); > > I'm a fan of using more MOZ_ASSERTs as you can probably tell by now. ;-) I think it should depend on the severity of the situation when the assertion fails. I've made these MOZ_ASSERT since if they were to fail, we'd crash. > @@ +166,5 @@ > > + input->mInputPort = aInput; > > + input->mOutputPort = aOutput; > > + // Connect streams in the MediaStreamGraph > > + NS_ASSERTION(aDestination.mStream->AsProcessedStream(), > > + "Destination stream has max inputs > 0 but is not a ProcessedMediaStream"); > > For the cast below to be safe, this has definitely got to be a MOZ_ASSERT. Fair enough. > ::: content/media/webaudio/AudioNode.h > @@ +62,5 @@ > > > > NS_DECL_CYCLE_COLLECTING_ISUPPORTS > > + NS_DECL_CYCLE_COLLECTION_CLASS(AudioNode) > > + > > + void JSBindingFinalized() > > This is meant to be called in the future, right? (Just sanity checking.) Yeah. We need to implement one of the intermediate nodes (e.g. GainNode), and land Peter's patch, before this becomes relevant. > @@ +64,5 @@ > > + NS_DECL_CYCLE_COLLECTION_CLASS(AudioNode) > > + > > + void JSBindingFinalized() > > + { > > + mJSBindingFinalized = true; > > We should probably MOZ_ASSERT to make sure this is not called multiple times > on the same node. I think it should be NS_ASSERTION, since the consequences of it being called twice are negligible. > ::: dom/webidl/AudioBufferSourceNode.webidl > @@ +15,5 @@ > > > > + //const unsigned short UNSCHEDULED_STATE = 0; > > + //const unsigned short SCHEDULED_STATE = 1; > > + //const unsigned short PLAYING_STATE = 2; > > + //const unsigned short FINISHED_STATE = 3; > > Why did you comment these out? Because I think the playbackState attribute is stupid, and until/if we support it, these constants are useless. > @@ +28,5 @@ > > //attribute boolean loop; > > > > + [Throws] > > + void start(optional double when = 0, optional double grainOffset = 0, > > + optional double grainDuration); > > This doesn't match the spec -- |when| should not be optional. I suggested on the mailing list that it should default to 0, and Chris agreed, so I think we should just do it even though he hasn't updated the spec yet.

Updated patch. Everything not mentiond in my comments above I just fixed.

Updated patch. Everything not mentioned in my comments above I just fixed.

> Hmm, I think you can just forward-declare AudioNodeEngine here. Actually we can't; clang barfs.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #17) > > @@ +41,5 @@ > > > + } > > > + ~Storage() { free(mDataToFree); } > > > + void* mDataToFree; > > > + const float* mSampleData; > > > + }; > > > > I'd make this a private member. Outside code should not be messing with > > Storage objects, right? > > There is no way to get access to the Storage elements of a > ThreadSharedFloatArrayBufferList so it's more convenient to make these > fields public. I was talking about the Storage object itself. I agree that its members should remain public. > > @@ +47,5 @@ > > > + uint32_t GetChannels() { return mContents.Length(); } > > > + /** > > > + * This may return null if we had an OOM during a copy. > > > + */ > > > + const float* GetData(uint32_t aIndex) const { return mContents[aIndex].mSampleData; } > > > > Might be worth adding a MOZ_ASSERT to ensure the correct length of mContents. > > nsTArray already does a bounds check assertion. Ah right.

Comment on attachment 705195 [details] [diff] [review] Part 8 v2 Review of attachment 705195 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/AudioNodeStream.cpp @@ +1,1 @@ > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-*/ Most of the stuff here I don't quite understand just now. I'll read this more, but Randell, please review these bits as if nobody else has. Thanks! @@ +26,5 @@ > +void > +AudioNodeStream::SetStreamTimeParameter(uint32_t aIndex, MediaStream* aRelativeToStream, > + double aStreamTime) > +{ > + class Message : public ControlMessage { I seem to recall somebody telling me that classes inside functions create problems when setting breakpoints (perhaps in VC?). I don't personally have any problem with this, and I can't seem to find what that complaint was about now... @@ +135,5 @@ > + nsAutoTArray<AudioChunk*,250> inputChunks; > + for (uint32_t i = 0; i < inputCount; ++i) { > + MediaStream* s = mInputs[i]->GetSource(); > + AudioNodeStream* a = s->AsAudioNodeStream(); > + NS_ASSERTION(a, "Non-AudioNodeStream inputs not supported"); MOZ_ASSERT? :-) @@ +172,5 @@ > + channels.AppendElements(chunk->mChannelData); > + if (channels.Length() < outputChannelCount) { > + AudioChannelsUpMix(&channels, outputChannelCount, nullptr); > + NS_ASSERTION(outputChannelCount == channels.Length(), > + "We called GetAudioChannelsSuperset to avoid this"); Hmm, can this happen in practice?

(In reply to :Ehsan Akhgari from comment #23) > I seem to recall somebody telling me that classes inside functions create > problems when setting breakpoints (perhaps in VC?). I don't personally have > any problem with this, and I can't seem to find what that complaint was > about now... I've used VC a lot with this code and haven't had more problems than usual, so I think it's OK. > @@ +135,5 @@ > > + nsAutoTArray<AudioChunk*,250> inputChunks; > > + for (uint32_t i = 0; i < inputCount; ++i) { > > + MediaStream* s = mInputs[i]->GetSource(); > > + AudioNodeStream* a = s->AsAudioNodeStream(); > > + NS_ASSERTION(a, "Non-AudioNodeStream inputs not supported"); > > MOZ_ASSERT? :-) OK. > @@ +172,5 @@ > > + channels.AppendElements(chunk->mChannelData); > > + if (channels.Length() < outputChannelCount) { > > + AudioChannelsUpMix(&channels, outputChannelCount, nullptr); > > + NS_ASSERTION(outputChannelCount == channels.Length(), > > + "We called GetAudioChannelsSuperset to avoid this"); > > Hmm, can this happen in practice? Only if we have a bug.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18) > (In reply to :Ehsan Akhgari from comment #16) > > ::: content/media/webaudio/AudioBuffer.cpp > > @@ +117,5 @@ > > > +StealJSArrayDataIntoThreadSharedFloatArrayBufferList(JSContext* aJSContext, > > > + const nsTArray<JSObject*>& aJSArrays) > > > +{ > > > + nsRefPtr<ThreadSharedFloatArrayBufferList> result = > > > + new ThreadSharedFloatArrayBufferList(aJSArrays.Length()); > > > > I think the allocation here and in GetThreadSharedChannelForRate should all > > be fallible, since the number of channels comes from content. > > I think, given it's the allocation of the internal array that can fail, I > will just make ThreadSharedFloatArrayBufferList have no channels in that > case and handle that in consumers by treating it as silence. That's a good idea. > > @@ +126,5 @@ > > > + uint8_t* stolenData; > > > + if (JS_StealArrayBufferContents(aJSContext, arrayBuffer, &dataToFree, > > > + &stolenData)) { > > > + result->SetData(i, dataToFree, reinterpret_cast<float*>(stolenData)); > > > + } > > > > Hmm, if one of these JS_StrealArrayBufferContents calls fails, the returned > > data structure contains "holes". Would that be handled in a sane way? > > Probably not. I'll fix that to go into the same "no channels" state as for > OOM. Great! > > ::: content/media/webaudio/AudioBufferSourceNode.cpp > > @@ +20,5 @@ > > > > > > NS_IMPL_ADDREF_INHERITED(AudioBufferSourceNode, AudioSourceNode) > > > NS_IMPL_RELEASE_INHERITED(AudioBufferSourceNode, AudioSourceNode) > > > > > > +class AudioBufferSourceNodeEngine : public AudioNodeEngine > > > > Do we have guarantees on which thread calls the member functions of > > AudioBufferSourceNodeEngine? If yes, it would be great if we can add > > assertions to that effect. > > All methods of AudioNodeEngine and its subclasses are called on the > MediaStreamGraph thread. Unfortunately there is no easy way to assert that > without giving the AudioNodeEngine a reference to the MediaStreamGraph, > which would add complexity to shutdown. The comment you added there is fine then. Thanks! > > ::: content/media/webaudio/AudioContext.cpp > > @@ +28,5 @@ > > > > > > NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(AudioContext, AddRef) > > > NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(AudioContext, Release) > > > > > > +static uint8_t gWebAudioOutputKey; > > > > Is this key just treated as an opaque void*? If that is the case, I wonder > > if this could be implemented as a static enum or something? > > It just has to be a unique key. This approach is simple, safe, and avoids > having to have a centralized enum that new clients need to add to. Hmm ok. No big deal! > > ::: content/media/webaudio/AudioNode.h > > @@ +62,5 @@ > > > > > > NS_DECL_CYCLE_COLLECTING_ISUPPORTS > > > + NS_DECL_CYCLE_COLLECTION_CLASS(AudioNode) > > > + > > > + void JSBindingFinalized() > > > > This is meant to be called in the future, right? (Just sanity checking.) > > Yeah. We need to implement one of the intermediate nodes (e.g. GainNode), > and land Peter's patch, before this becomes relevant. Cool, that's what I figured. > > ::: dom/webidl/AudioBufferSourceNode.webidl > > @@ +15,5 @@ > > > > > > + //const unsigned short UNSCHEDULED_STATE = 0; > > > + //const unsigned short SCHEDULED_STATE = 1; > > > + //const unsigned short PLAYING_STATE = 2; > > > + //const unsigned short FINISHED_STATE = 3; > > > > Why did you comment these out? > > Because I think the playbackState attribute is stupid, and until/if we > support it, these constants are useless. That's a good reason! > > @@ +28,5 @@ > > > //attribute boolean loop; > > > > > > + [Throws] > > > + void start(optional double when = 0, optional double grainOffset = 0, > > > + optional double grainDuration); > > > > This doesn't match the spec -- |when| should not be optional. > > I suggested on the mailing list that it should default to 0, and Chris > agreed, so I think we should just do it even though he hasn't updated the > spec yet. OK, cool.

BTW, I want to start working on ScriptProcessorNode, and I want some of the input/output changes you made here... I'll see if I can disentangle those bits into a separate patch that I can land here. If I can do that, I will attach a new patch here for you to land later.

(In reply to :Ehsan Akhgari from comment #26) > BTW, I want to start working on ScriptProcessorNode, and I want some of the > input/output changes you made here... I'll see if I can disentangle those > bits into a separate patch that I can land here. If I can do that, I will > attach a new patch here for you to land later. Landed as https://hg.mozilla.org/integration/mozilla-inbound/rev/89c0d45b70c8

Here's the remainder of part 9. Should apply cleanly on mozilla-inbound. I also adjusted the commit message so that you can import it verbatim.

Comment on attachment 705195 [details] [diff] [review] Part 8 v2 Review of attachment 705195 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/AudioNodeEngine.cpp @@ +59,5 @@ > + float aScale, > + float aOutput[WEBAUDIO_BLOCK_SIZE]) > +{ > + if (aScale == 1.0f) { > + memcpy(aOutput, aInput, sizeof(aInput)); The size argument here is incorrect. What you want is: memcpy(aOutput, aInput, sizeof(*aInput) * WEBAUDIO_BLOCK_SIZE); (Thanks to clang's excellent diagnostics!)

Comment on attachment 705195 [details] [diff] [review] Part 8 v2 Review of attachment 705195 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/AudioNodeEngine.h @@ +104,5 @@ > + * MediaStreamGraph thread. > + */ > +class AudioNodeEngine { > +public: > + AudioNodeEngine() {} Please add a virtual dtor for this class.

Comment on attachment 705654 [details] [diff] [review] Part 9 v3 Review of attachment 705654 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/AudioBuffer.cpp @@ +170,5 @@ > + return mResampledChannels; > + } > + > + for (uint32_t i = 0; i < NumberOfChannels(); ++i) { > + const float* inputData = mSharedChannels->GetData(i); inputData is not used, which breaks builds with --enable-warnings-as-errors.

Comment on attachment 704421 [details] [diff] [review] Part 1.5: Clean up main-thread MediaStream event listeners Review of attachment 704421 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/MediaDecoder.h @@ +342,5 @@ > // captureStream(UntilEnded). Seeking creates a new source stream, as does > // replaying after the input as ended. In the latter case, the new source is > // not connected to streams created by captureStreamUntilEnded. > > + struct DecodedStreamData : public MainThreadMediaStreamListener { Please mark this class as MOZ_FINAL. The virtual function that you've added below makes clang unhappy in builds with --enable-warnings-as-errors.

Comment on attachment 705654 [details] [diff] [review] Part 9 v3 Review of attachment 705654 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/AudioBuffer.cpp @@ -51,5 @@ > } > > AudioBuffer::~AudioBuffer() > { > - mChannels.Clear(); Hmm, I don't think this is safe. This was added in bug 811206 by smaug...

Smaug, please see comment 33.

So there is currently a problem with the fact that not all node types have been hooked up to the MSG yet. This causes us to sometimes create a port with a null input, which crashes on the MSG thread. This patch is sort of unfortunate, but it's the only way that I can think of to land this patch without waiting for all of the other bits of the MSG integration. What do you think, roc? (Note that once all of the implemented nodes are hooked up to the MSG, we can just remove the SupportsMediaStreams function.)

(In reply to :Ehsan Akhgari from comment #33) > > AudioBuffer::~AudioBuffer() > > { > > - mChannels.Clear(); > > Hmm, I don't think this is safe. This was added in bug 811206 by smaug... Not technically unsafe, but assertion may fire if mChannels is not empty when DROP is called. http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCJSRuntime.cpp?rev=c5d438850b86&mark=325-333#323

(In reply to comment #37) > (In reply to :Ehsan Akhgari from comment #33) > > > AudioBuffer::~AudioBuffer() > > > { > > > - mChannels.Clear(); > > > > Hmm, I don't think this is safe. This was added in bug 811206 by smaug... > Not technically unsafe, but assertion may fire if mChannels is not empty when > DROP is called. > > http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCJSRuntime.cpp?rev=c5d438850b86&mark=325-333#323 Yeah, I was seeing this assertion in my local builds.

(In reply to :Ehsan Akhgari from comment #29) > The size argument here is incorrect. What you want is: > > memcpy(aOutput, aInput, sizeof(*aInput) * WEBAUDIO_BLOCK_SIZE); > > (Thanks to clang's excellent diagnostics!) Yes, I've fixed that locally.

As well as the other clang issues you pointed out. Sorry that I didn't update the patches, but that's a pain in Bugzilla.

(In reply to comment #40) > As well as the other clang issues you pointed out. Sorry that I didn't update > the patches, but that's a pain in Bugzilla. Yeah, no worries. I just hope that I didn't miss any of the changes I made locally... It would be great if we can get this stuff landed sooner. Randell, when do you expect to be able to review these patches?

Comment on attachment 704429 [details] [diff] [review] Part 4: Move MediaStreamGraphImpl to its own header file Review of attachment 704429 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/MediaStreamGraph.cpp @@ +1,1 @@ > /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-*/ I assume that splinter is confused, and this really is MediaStreamGraphImpl.h, and not a second MediaStreamGraph.cpp

Comment on attachment 704431 [details] [diff] [review] Part 5: Add MediaStream::GraphTimeToStreamTimeOptimistic and MediaStream::StreamTimeToGraphTime Review of attachment 704431 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/MediaStreamGraph.cpp @@ +223,5 @@ > } > t = end; > } > return std::max<StreamTime>(0, s); > } existing trailing space

Comment on attachment 705195 [details] [diff] [review] Part 8 v2 Review of attachment 705195 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/AudioNodeEngine.cpp @@ +11,5 @@ > +void > +AllocateAudioBlock(uint32_t aChannelCount, AudioChunk* aChunk) > +{ > + // XXX for SIMD purposes we should do something here to make sure the > + // channel buffers are 16-byte aligned. You may want to create a master/meta bug for SIMD/etc optimizing this code where you can point things like this, and collect info on what's available to optimize (and point someone with that knowledge at). @@ +22,5 @@ > + aChunk->mChannelData[i] = data + i*WEBAUDIO_BLOCK_SIZE; > + } > + aChunk->mBuffer = buffer.forget(); > + aChunk->mVolume = 1.0f; > + aChunk->mBufferFormat = AUDIO_FORMAT_FLOAT32; I assume since we're saying it's FLOAT32 when it's an array of floats that there's something that asserts (at build time) that sizeof(float) = 4 @@ +42,5 @@ > +AudioBlockAddChannelWithScale(const float aInput[WEBAUDIO_BLOCK_SIZE], > + float aScale, > + float aOutput[WEBAUDIO_BLOCK_SIZE]) > +{ > + if (aScale == 1.0f) { How safe/portable is a floating-point equality comparison like this? So long as it's a direct 'flag', it should be ok. if it's a calculation, you'll need some type of epsilon. I believe this is only used as a flag, so it's ok. ::: content/media/AudioNodeStream.cpp @@ +26,5 @@ > +void > +AudioNodeStream::SetStreamTimeParameter(uint32_t aIndex, MediaStream* aRelativeToStream, > + double aStreamTime) > +{ > + class Message : public ControlMessage { I think ehsan's thinking of anonymous classes @@ +121,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()); does this need to be added before the NotifyQueuedTrackChanges? @@ +131,5 @@ > +AudioNodeStream::ObtainInputBlock(AudioChunk* aTmpChunk) > +{ > + uint32_t inputCount = mInputs.Length(); > + uint32_t outputChannelCount = 0; > + nsAutoTArray<AudioChunk*,250> inputChunks; what's the source of the 250 constant? @@ +167,5 @@ > + AllocateAudioBlock(outputChannelCount, aTmpChunk); > + > + for (uint32_t i = 0; i < inputChunkCount; ++i) { > + AudioChunk* chunk = inputChunks[i]; > + nsAutoTArray<const void*,2> channels; should this be a define like it is elsewhere? ::: content/media/MediaStreamGraph.h @@ +833,5 @@ > class MediaStreamGraph { > public: > + // We ensure that the graph current time advances in multiples of > + // IdealAudioBlockSize()/IdealAudioRate(). A stream that never blocks > + // and has a track with the ideal audio rate will produce audio Seems like this comment isn't finished...

(In reply to Randell Jesup [:jesup] from comment #44) > I assume since we're saying it's FLOAT32 when it's an array of floats that > there's something that asserts (at build time) that sizeof(float) = 4 I suppose we could, but really, sizeof(float)==4 is one of the fundamental truths of the universe. > @@ +42,5 @@ > > +AudioBlockAddChannelWithScale(const float aInput[WEBAUDIO_BLOCK_SIZE], > > + float aScale, > > + float aOutput[WEBAUDIO_BLOCK_SIZE]) > > +{ > > + if (aScale == 1.0f) { > > How safe/portable is a floating-point equality comparison like this? So > long as it's a direct 'flag', it should be ok. if it's a calculation, > you'll need some type of epsilon. I believe this is only used as a flag, so > it's ok. We could have an epsilon but for now I think this is fine. There are certainly plenty of cases (e.g. playing sample buffers, results of mixing multiple channels) where the value is going to be exactly 1.0. > @@ +121,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()); > > does this need to be added before the NotifyQueuedTrackChanges? No. > @@ +131,5 @@ > > +AudioNodeStream::ObtainInputBlock(AudioChunk* aTmpChunk) > > +{ > > + uint32_t inputCount = mInputs.Length(); > > + uint32_t outputChannelCount = 0; > > + nsAutoTArray<AudioChunk*,250> inputChunks; > > what's the source of the 250 constant? Nothing. I just made it up. It could be any value that's not too large. > @@ +167,5 @@ > > + AllocateAudioBlock(outputChannelCount, aTmpChunk); > > + > > + for (uint32_t i = 0; i < inputChunkCount; ++i) { > > + AudioChunk* chunk = inputChunks[i]; > > + nsAutoTArray<const void*,2> channels; > > should this be a define like it is elsewhere? I suppose so. > ::: content/media/MediaStreamGraph.h > @@ +833,5 @@ > > class MediaStreamGraph { > > public: > > + // We ensure that the graph current time advances in multiples of > > + // IdealAudioBlockSize()/IdealAudioRate(). A stream that never blocks > > + // and has a track with the ideal audio rate will produce audio > > Seems like this comment isn't finished... Will fix.

Comment on attachment 709878 [details] [diff] [review] Part 7.5: Make Web Audio tests context-rate-independent (disabling some decodeAudioData tests) Review of attachment 709878 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/test/test_decodeAudioData.html @@ +257,5 @@ > +if (cx.sampleRate == 44100) { > + // Now, let's get real! > + runNextTest(); > +} else { > + todo(false, "Decoded data tests disabled; context sampleRate " + cx.sampleRate + " not supported"); Please file a follow-up bug to re-enable this test and assign it to me. Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/000b88ac40a7 https://hg.mozilla.org/integration/mozilla-inbound/rev/53d5ed687612 https://hg.mozilla.org/integration/mozilla-inbound/rev/372322f3264d https://hg.mozilla.org/integration/mozilla-inbound/rev/e6254d8997ab https://hg.mozilla.org/integration/mozilla-inbound/rev/11f4d740cd6c https://hg.mozilla.org/integration/mozilla-inbound/rev/1d7ec5adc49f https://hg.mozilla.org/integration/mozilla-inbound/rev/5477b87ff03e https://hg.mozilla.org/integration/mozilla-inbound/rev/d0772aba503c https://hg.mozilla.org/integration/mozilla-inbound/rev/0ad6f67a95f9 https://hg.mozilla.org/integration/mozilla-inbound/rev/e6ef90038007 https://hg.mozilla.org/integration/mozilla-inbound/rev/0e3f20927c50 https://hg.mozilla.org/integration/mozilla-inbound/rev/00f86870931c https://hg.mozilla.org/integration/mozilla-inbound/rev/3de2271ad47f https://hg.mozilla.org/integration/mozilla-inbound/rev/80e8530f06ea Filed bug 838065.

Backed out because of Android M2 crashes: https://hg.mozilla.org/integration/mozilla-inbound/rev/58ea26c5a810 https://tbpl.mozilla.org/php/getParsedLog.php?id=19442026&tree=Mozilla-Inbound&full=1

The crash stack: 0 libdvm.so + 0x41f10 r4 = 0x808a23f4 r5 = 0x808a23f4 r6 = 0x00000000 r7 = 0x00000000 r8 = 0x00000000 r9 = 0x00000001 r10 = 0x0000000f fp = 0x5c3bfe2c sp = 0x5c3bfd58 lr = 0xafd1588f pc = 0x80841f10 Found by: given as instruction pointer in context 1 libdvm.so + 0x33359 sp = 0x5c3bfd60 pc = 0x8083335b Found by: stack scanning 2 libdvm.so + 0x3347f sp = 0x5c3bfd68 pc = 0x80833481 Found by: stack scanning 3 2 (deleted) + 0x174b56 sp = 0x5c3bfd74 pc = 0x484fab58 Found by: stack scanning 4 libdvm.so + 0x85f7a sp = 0x5c3bfd78 pc = 0x80885f7c Found by: stack scanning 5 libdvm.so + 0x85f7a sp = 0x5c3bfd80 pc = 0x80885f7c Found by: stack scanning 6 2 (deleted) + 0x13a136 sp = 0x5c3bfd88 pc = 0x484c0138 Found by: stack scanning 7 dalvik-LinearAlloc (deleted) + 0x70d1a sp = 0x5c3bfd8c pc = 0x4313cd1c Found by: stack scanning 8 libdvm.so + 0x3d291 sp = 0x5c3bfd90 pc = 0x8083d293 Found by: stack scanning 9 libdvm.so + 0x85f7a sp = 0x5c3bfd94 pc = 0x80885f7c Found by: stack scanning 10 libdvm.so + 0x469b7 sp = 0x5c3bfd98 pc = 0x808469b9 Found by: stack scanning 11 2 (deleted) + 0x1ac37e sp = 0x5c3bfda4 pc = 0x48532380 Found by: stack scanning 12 libxul.so!sa_stream_destroy [sydney_audio_android.c:80e8530f06ea : 272 + 0x16] sp = 0x5c3bfdb8 pc = 0x54b33648 Found by: stack scanning 13 0x16 r4 = 0x4313cd1c r5 = 0x8083d22d r6 = 0x4e8c5b80 sp = 0x5c3bfdc8 pc = 0x00000018 Found by: call frame info 14 libxul.so!mozilla::NativeAudioStream::Shutdown() [AudioStream.cpp:80e8530f06ea : 345 + 0x2] sp = 0x5c3bfdd0 pc = 0x5432d2c0 Found by: stack scanning 15 libxul.so!mozilla::MediaStream::DestroyImpl() [MediaStreamGraph.cpp:80e8530f06ea : 1378 + 0xa] r4 = 0x56f5a420 sp = 0x5c3bfdd8 pc = 0x54319dd4 Found by: call frame info 16 libxul.so + 0x93ffb2 r4 = 0x5706d888 r5 = 0x00000002 r6 = 0x5910d3c0 sp = 0x5c3bfde8 pc = 0x54319fb4 Found by: call frame info 17 libxul.so!mozilla::MediaStreamGraphImpl::RunThread() [MediaStreamGraph.cpp:80e8530f06ea : 937 + 0xa] r4 = 0x00000000 r5 = 0x00000002 r6 = 0x5910d3c0 sp = 0x5c3bfdf0 pc = 0x5431cb30 Found by: call frame info 18 libxul.so!mozilla::::MediaStreamGraphThreadRunnable::Run [MediaStreamGraph.cpp:80e8530f06ea : 1097 + 0x6] r4 = 0x4e8e1700 r5 = 0x00000000 r6 = 0x5c3bfe84 r7 = 0x00000001 r8 = 0x00000000 r9 = 0x4e8e172c r10 = 0x5c3bfeb7 fp = 0x5355ba50 sp = 0x5c3bfe68 pc = 0x5431cfd4 Found by: call frame info 19 libxul.so!nsThread::ProcessNextEvent(bool, bool*) [nsThread.cpp:80e8530f06ea : 627 + 0xa] r4 = 0x4e8e1700 r5 = 0x00000000 r6 = 0x5c3bfe84 r7 = 0x00000001 r8 = 0x00000000 r9 = 0x4e8e172c r10 = 0x5c3bfeb7 fp = 0x5355ba50 sp = 0x5c3bfe70 pc = 0x548e7fb0 Found by: call frame info 20 libxul.so!NS_ProcessNextEvent_P(nsIThread*, bool) [nsThreadUtils.cpp:80e8530f06ea : 238 + 0x12] r4 = 0x00000001 r5 = 0x5c3bfec4 r6 = 0x553d5834 r7 = 0x00000000 r8 = 0x00100000 r9 = 0x4e4cb448 r10 = 0x5c2c0000 fp = 0x5355ba50 sp = 0x5c3bfeb0 pc = 0x548b7118 Found by: call frame info 21 libxul.so!nsThread::ThreadFunc(void*) [nsThread.cpp:80e8530f06ea : 265 + 0xa] r4 = 0x4e8e1700 r5 = 0x5c3bfec4 r6 = 0x553d5834 r7 = 0x00000000 r8 = 0x00100000 r9 = 0x4e4cb448 r10 = 0x5c2c0000 fp = 0x5355ba50 sp = 0x5c3bfec0 pc = 0x548e7c10 Found by: call frame info 22 libnspr4.so!_pt_root [ptthread.c:80e8530f06ea : 156 + 0xa] So looks like we crash shutting down the audio stream after playing Web Audio during one of the Web Audio tests.

It looks like the line in sydney_audio_android.c is: (*jenv)->CallVoidMethod(jenv, s->output_unit, at.flush); Ho hum, that doesn't really help.

I guess this has something to do with it: 02-04 22:10:20.189 W/dalvikvm( 1817): JNI WARNING: JNI method called with exception raised 02-04 22:10:20.189 W/dalvikvm( 1817): in Ldalvik/system/NativeStart;.run ()V (CallVoidMethod) 02-04 22:10:20.189 W/dalvikvm( 1817): Pending exception is: 02-04 22:10:20.189 E/dalvikvm( 1817): VM aborting I don't know exactly what this means :-).

Snorp, any hints on comments #51-#53?

roc, I believe the call immediately before the flush (stop) [1] threw [2] because the AudioTrack was uninitialized, and we do not catch the exception at this location in the JNI code. There is examples in the same file on how to catch an exception. [1]: http://mxr.mozilla.org/mozilla-central/source/media/libsydneyaudio/src/sydney_audio_android.c#271 [2]: https://github.com/android/platform_frameworks_base/blob/master/media/java/android/media/AudioTrack.java#L903

Yeah, gotta handle the exceptions or the VM gets upset. I don't see it here for some reason, but typically the VM will cause a segfault at 0xdeadd00d when it aborts like this.

(In reply to :Ehsan Akhgari (Away 2/7-2/15) from comment #57) > Created attachment 710183 [details] [diff] [review] > Handle AudioTrack.stop exceptions This patch is running on the try server: https://tbpl.mozilla.org/?tree=Try&rev=1d5e097580d5

Comment on attachment 710183 [details] [diff] [review] Handle AudioTrack.stop exceptions Review of attachment 710183 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the one thing fixed ::: media/libsydneyaudio/src/sydney_audio_android.c @@ +273,5 @@ > + jthrowable exception = (*jenv)->ExceptionOccurred(jenv); > + if (exception) { > + (*jenv)->ExceptionDescribe(jenv); > + (*jenv)->ExceptionClear(jenv); > + (*jenv)->PopLocalFrame(jenv, NULL); You don't want this, as PushLocalFrame was never called. @@ +281,2 @@ > (*jenv)->CallVoidMethod(jenv, s->output_unit, at.flush); > (*jenv)->CallVoidMethod(jenv, s->output_unit, at.release); flush() and release() don't throw, at least according to the docs, so it's fine that we don't check for exceptions there.

Comment on attachment 705654 [details] [diff] [review] Part 9 v3 Review of attachment 705654 [details] [diff] [review]: ----------------------------------------------------------------- Bug 804837 isn't this bug.

(In reply to :Ehsan Akhgari (Away 2/7-2/15) from comment #58) > (In reply to :Ehsan Akhgari (Away 2/7-2/15) from comment #57) > > Created attachment 710183 [details] [diff] [review] > > Handle AudioTrack.stop exceptions > > This patch is running on the try server: > https://tbpl.mozilla.org/?tree=Try&rev=1d5e097580d5 Got 02-05 07:48:04.337 W/System.err( 1788): java.lang.IllegalStateException: stop() called on uninitialized AudioTrack. 02-05 07:48:04.337 W/System.err( 1788): at android.media.AudioTrack.stop(AudioTrack.java:780) 02-05 07:48:04.337 W/System.err( 1788): at dalvik.system.NativeStart.run(Native Method) 02-05 07:48:04.337 W/dalvikvm( 1788): JNI WARNING: too many PopLocalFrame calls 02-05 07:48:04.337 W/dalvikvm( 1788): JNI WARNING: JNI method called with exception raised 02-05 07:48:04.337 W/dalvikvm( 1788): in Ldalvik/system/NativeStart;.run ()V (CallVoidMethod) 02-05 07:48:04.337 W/dalvikvm( 1788): Pending exception is: 02-05 07:48:04.337 E/dalvikvm( 1788): VM aborting Sort of as expected. Pushed to try again: https://tbpl.mozilla.org/?tree=Try&rev=19a6f37c42ac

Relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/b1533f8a8e12 https://hg.mozilla.org/integration/mozilla-inbound/rev/5a403ead198d https://hg.mozilla.org/integration/mozilla-inbound/rev/3664bcfb874a https://hg.mozilla.org/integration/mozilla-inbound/rev/6164f84ebbde https://hg.mozilla.org/integration/mozilla-inbound/rev/74f62ac34bac https://hg.mozilla.org/integration/mozilla-inbound/rev/7faccaa6b26b https://hg.mozilla.org/integration/mozilla-inbound/rev/2543f19d08e1 https://hg.mozilla.org/integration/mozilla-inbound/rev/9c057247fe22 https://hg.mozilla.org/integration/mozilla-inbound/rev/29c81f9c039f https://hg.mozilla.org/integration/mozilla-inbound/rev/84fdaad17284 https://hg.mozilla.org/integration/mozilla-inbound/rev/bfb6df78ae0e https://hg.mozilla.org/integration/mozilla-inbound/rev/7d65d8548d3d https://hg.mozilla.org/integration/mozilla-inbound/rev/7b6f1f18c052 https://hg.mozilla.org/integration/mozilla-inbound/rev/5c617b881053 https://hg.mozilla.org/integration/mozilla-inbound/rev/390d38eb966d

https://hg.mozilla.org/mozilla-central/rev/b1533f8a8e12 https://hg.mozilla.org/mozilla-central/rev/5a403ead198d https://hg.mozilla.org/mozilla-central/rev/3664bcfb874a https://hg.mozilla.org/mozilla-central/rev/6164f84ebbde https://hg.mozilla.org/mozilla-central/rev/74f62ac34bac https://hg.mozilla.org/mozilla-central/rev/7faccaa6b26b https://hg.mozilla.org/mozilla-central/rev/2543f19d08e1 https://hg.mozilla.org/mozilla-central/rev/9c057247fe22 https://hg.mozilla.org/mozilla-central/rev/29c81f9c039f https://hg.mozilla.org/mozilla-central/rev/84fdaad17284 https://hg.mozilla.org/mozilla-central/rev/bfb6df78ae0e https://hg.mozilla.org/mozilla-central/rev/7d65d8548d3d https://hg.mozilla.org/mozilla-central/rev/7b6f1f18c052 https://hg.mozilla.org/mozilla-central/rev/5c617b881053 https://hg.mozilla.org/mozilla-central/rev/390d38eb966d

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