Closed Bug 804387 Opened 12 years ago Closed 11 years ago

Make the mediastreams backend awesome enough for us to hook the initial web audio APIs into

Categories

(Core :: Web Audio, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla21

People

(Reporter: ehsan.akhgari, Assigned: roc)

References

Details

Attachments

(16 files, 3 obsolete files)

Part 1: Make AllocateInputPort addref the returned port
2.54 KB, patch
jesup
: review+
Details | Diff | Splinter Review
Part 1.5: Clean up main-thread MediaStream event listeners
14.07 KB, patch
jesup
: review+
Details | Diff | Splinter Review
Part 2: Fix misleading parameter name; these are NOT exactly microseconds
1.96 KB, patch
jesup
: review+
Details | Diff | Splinter Review
Part 3: When a global underrun happens, don't insert blocked time, just cut that time out of the entire MediaStreamGraph timeline instead
1.57 KB, patch
jesup
: review+
Details | Diff | Splinter Review
Part 4: Move MediaStreamGraphImpl to its own header file
82.78 KB, patch
jesup
: review+
Details | Diff | Splinter Review
Part 5: Add MediaStream::GraphTimeToStreamTimeOptimistic and MediaStream::StreamTimeToGraphTime
4.48 KB, patch
jesup
: review+
Details | Diff | Splinter Review
Part 6: ChangeExplicitBlockerCountImpl takes a GraphTime, not a StreamTime
1.03 KB, patch
jesup
: review+
Details | Diff | Splinter Review
Part 7: Use a static_cast instead of dynamic check when processing ProcessedMediaStream message
928 bytes, patch
jesup
: review+
Details | Diff | Splinter Review
testcase/demo
689 bytes, text/html
Details
Part 8 v2
30.66 KB, patch
ehsan.akhgari
: review+
jesup
: feedback+
Details | Diff | Splinter Review
Part 9 v3
39.12 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Part 10: Add a mechanism for ensuring that we don't attempt to allocate an input port if the input node is not hooked up to MSG yet
3.30 KB, patch
roc
: review+
Details | Diff | Splinter Review
Correct the expected sample rate in test_AudioContext.html
827 bytes, patch
roc
: review+
Details | Diff | Splinter Review
Part 7.2: Let BiquadFilterNode use the actual context sample rate
1.07 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Part 7.5: Make Web Audio tests context-rate-independent (disabling some decodeAudioData tests)
9.25 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Handle AudioTrack.stop exceptions
1.27 KB, patch
snorp
: review+
Details | Diff | Splinter Review 
roc knows what needs to be done here.  The summary should sort of match the reality I hope.  :-)
Blocks: webaudio
roc, can you please attach the patches that you have so far here?  Thanks!
Assignee: roc → ehsan
Depends on: 812617
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.
Attachment #704426 - Flags: review?(rjesup)
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.
Attachment #704434 - Flags: review?(ehsan)
Attachment #704434 - Flags: feedback?(rjesup)
OS: Mac OS X → All
Attached file testcase/demo — 
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.
Attachment #704434 - Flags: review?(ehsan)
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.
Attachment #704435 - Flags: review?(ehsan) → review+
(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.
Attached patch Part 8 v2 — — Splinter Review 
Updated patch. Everything not mentiond in my comments above I just fixed.
Attachment #704434 - Attachment is obsolete: true
Attachment #704434 - Flags: feedback?(rjesup)
Attachment #705195 - Flags: review?(ehsan)
Attachment #705195 - Flags: feedback?(rjesup)
Attached patch Part 9 v2 (obsolete) — — Splinter Review 
Updated patch. Everything not mentioned in my comments above I just fixed.
Attachment #704435 - Attachment is obsolete: true
Attachment #705197 - Flags: review+
> 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?
Attachment #705195 - Flags: review?(ehsan) → review+
(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
Whiteboard: [leave open]
Attached patch Part 9 v3 — — Splinter Review 
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.
Attachment #705197 - Attachment is obsolete: true
Attachment #705654 - Flags: review+
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.)
Attachment #706578 - Flags: review?(roc)
(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.
Attachment #706578 - Flags: review?(roc) → review+
Attachment #706581 - Flags: review?(roc) → review+
(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?
Attachment #704349 - Flags: review?(rjesup) → review+
Attachment #704421 - Flags: review?(rjesup) → review+
Attachment #704422 - Flags: review?(rjesup) → review+
Attachment #704426 - Flags: review?(rjesup) → review+
Blocks: 836076
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
Attachment #704429 - Flags: review?(rjesup) → review+
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
Attachment #704431 - Flags: review?(rjesup) → review+
Attachment #704432 - Flags: review?(rjesup) → review+
Attachment #704433 - Flags: review?(rjesup) → review+
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...
Attachment #705195 - Flags: feedback?(rjesup) → feedback+
(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.
Attachment #709877 - Flags: review?(ehsan) → review+
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!
Attachment #709878 - Flags: review?(ehsan) → review+
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.

        Attachment #710183 -
        Flags: review?(snorp)

        Attachment #710183 -
        Flags: review?(blassey.bugs)

    
    

    
  
(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.

        Attachment #710183 -
        Flags: review?(snorp) → review+

    
  

        Attachment #710183 -
        Flags: review?(blassey.bugs)

    
    

    
  
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

    
  
Depends on: 849652

    
    

    
  
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: