This can help with latency and underrun issues. Please see the wiki page for more information on the problem definition and the proposed solution.

Paul, is this something you can work on?

So, I started thinking about that again. There are three main goals here: (1) lower the latency when using the MSG: the BufferedAudioStream internal buffer adds latency (exactly the length of the buffer in frames divided by the samplerate). Either we want to avoid this buffering step completely (1a) or limit it drastically (1b). (2) make the MSG clock be the audio output clock when available (and not a TimeStamp-based clock) ; (3) teach the callback to request some new data from the graph, so it can recover from underruns ; (1) for a wide variety of application, we need to have a low latency (real time communication, games, musical application with real time user input, etc.). The MSG writes to a BufferedAudioStream, that has an internal buffer, that adds latency. We need to either: (1a) stop using the BufferedAudioStream with the MSG and interface the MSG to cubeb directly ; (1b) use a latency in the BufferedAudioStream roughtly equal to the delay between two cubeb callbacks divided by the samplerate, plus some slack (which would be in the order of samplerate/100, that is, on graph iteration). This means most of the AudioStream internal buffer would be consumed each time, effectively removing the latency brought by the AudioStream buffer. (1a) seem to be harder and provide no advantages over (1b) (we need some kind of buffering because we are interfacing a push based system with a pull based system), so I'll go that route, if I'm not missing anything. The main problem is that we are more likely to underrun if we do that. Also, we need to speed up the initial creation of the stream (because that can buffer up some data, waiting for the callback to start firing regularly). This can be done using lightweight stream, but I had good result starting the stream right after creation, and letting it underrun until the first write. This is probably good enough until we have lightweight stream. To do (2), we need to figure out what should be the exact time cubeb (or the AudioStream) should report to the graph for a particular stream. It appears that it is: clock_graph = (pos_cubeb + stream_buffer_len + backend_latency) / samplerate where: - clock_graph is the output clock in seconds ; - pos_cubeb is the position reported by cubeb_stream_get_position, in frame; - stream_buffer_len is the number of frames in the AudioStream buffer (mBuffer.Length()) ; - backend_latency is the number of frames in the backends buffers ; - samplerate is the number of frames per seconds for this stream ; Indeed, this is the time at which the first frame of the next iteration will be heard. I have a patch to make |backend_latency| available, we already have the other things we need. If we have (2), we have more control over the timing issues, because we know what frame is being outputed on the speakers (are are not estimating it using TimeStamp). To do (3), we need to give the callback the ability to run an iteration of the graph so it can refill the buffer in case of underrun. If we have more that one audio output, this means we can write in other cubeb streams during a streams callback. Then, we can move the meat of MediaStreamGraphImpl::RunThread (the whole function minus the loop and scheduling parts) in its own function, put a lock around it, and add a public method to the MSG to run an iteration. Then, we need to pass the graph to the AudioStreams (iff they are used with a MSG). I'm not sure yet what problem I should expect from trying to run the MSG on two threads. Any thoughts?

(In reply to Paul Adenot (:padenot) from comment #3) > I'm > not sure yet what problem I should expect from trying to run the MSG on two > threads. So, running an iteration of the MSG will call NotifyPull() for all the pull-based streams. As you can see in, e.g., MediaPipelineReceiveAudio::PipelineListener::NotifyPull() in media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp, these listeners use a lot of variables without locking under the assumption that they will only be accessed from the MSG. So even if you add locking to the MSG, you'd have to add locking to all of these listeners as well. I think a better solution would be to dispatch a request to the MSG thread to run an iteration.

Yes, what tim said. (still reading the rest, may comment more later) Also, the suppliers of the data may use the pulls to adjust timing - NetEQ in webrtc will adjust jitter buffer depths and Timebase corrections on the assumption that the pull means 10ms of time has gone by (in the output stream). And if you're pulling, in a way it has since the data is now in MSG - but if you pull "early" too much you may force 5-10ms of delay into the NetEQ jitter buffer. An occasional early pull (especially if not ~10ms early) may not hurt much, but even that could in some instances add 5-10ms of delay. And pulling from video early may cause a frame skip, though it's not clear that would actually happen or that it wouldn't happen just once when the delay bumped up.

And I see why you need to run an iteration; you're underflowing, so pushing NetEQ to get you more data (even if it forces NetEQ to start deepening the jitter buffer) is reasonable.

(In reply to Paul Adenot (:padenot) from comment #3) > (1a) seem to be harder and provide no advantages over (1b) (we need some > kind of > buffering because we are interfacing a push based system with a pull based > system), so I'll go that route, if I'm not missing anything. I don't understand this. If we ran all MSG processing during the libcubeb callback then that makes MSG a pull-based system. And I think we should do that if the libcubeb callback happens frequently enough (period of 10ms or less). We wouldn't need "some kind of buffering". If the cubeb callback doesn't run frequently enough, then we need to run some MSG iterations "early" and buffer those outputs so it doesn't take too long to do the rest of the MSG processing when the libcubeb callback finally arrives. I think this should be really simple; it's like faking libcubeb callbacks every 10ms if one hasn't arrived in that time, and accumulating the output of these fake callbacks in a buffer that is copied into the output buffer of the real libcubeb callback when it arrives. This should subsume step 3. > Also, we need to speed up the initial creation of the stream (because that > can > buffer up some data, waiting for the callback to start firing regularly). > This > can be done using lightweight stream, but I had good result starting the > stream > right after creation, and letting it underrun until the first write. This is > probably good enough until we have lightweight stream. I think that the MSG should have a single libcubeb stream and resample and mix together all its outputs to write into that stream.

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

Paul (padenot) and Ben (bkelly) believe that this bug is the root cause for Bug 917193 (which is a 1.3 blocker). Reading bug 917193, it sounds like the Dialer group is going to work around Bug 917193 by not using web audio. I'd feel better if we had some confirmation of the theory that this bug is the underlying root cause for Bug 917193. Ben -- Are you seeing bug 917193 on multiple devices or just the Buri? Can you check to see if other web audio apps or tests that play simple tones using web audio (like the Dialer keypad) have issues with broken/choppy sound on the Buri? Paul, Roc -- Does it make sense to ask Ben to look for underflows (or another error message) in the logs when he experiences problems (to confirm this bug)? If so, should he use a debug build with logging enabled or should he use a custom Opt build that logs overflows and other errors -- or does testing with Opt vs debug not matter?

Maire, unfortunately I only have the buri building for v1.3 at the moment. I don't have another device handy to compare it to at that version. I'm going to make a video of what I am seeing (hearing) so that Paul can try to determine if the popping is indeed this bug or not. Also, I suggesting trying the media files as a backup plan, but I don't know that the communications team agrees with that or not. At this point its just an idea.

With NSPR_LOG_MODULES=AudioStream:5 in a debug build (or any build with PRlogging enabled), whenever there's an underrun in the cubeb callback we'll get "AudioStream %p lost %d frames" in the log.

Has anyone tried just boosting the priority of the MSG thread to see if that helps? Using sched_setscheduler(0, SCHED_RR) with priority 1 would be worth trying. Of course, the process needs to be root or have CAP_SYS_NICE (or do something with rlimit and RLIMIT_RTPRIO) for that to work.

I forgot to mention here that I posted a video over in bug 917193 comment 16. I would buy cpu/priority being an issue. There are a ton of sync reflows that occur when touching a number in the dialer. Also, the fact that the glitch is more likely to occur on the first press after opening the app suggests something like jit is interfering.

FWIW - I can reproduce the blocking bug on Buri 1.3, but I can't reproduce it on a TCL Soul device. So yeah, I think the hardware is going to influence if the blocking bug reproduces or not.

roc has answered already.

This [1] is the current plan to do this, updated as we write the code. [1]: https://webrtc.etherpad.mozilla.org/mediastream-refactor

This is not finished yet (it does not look good enough), but is green and has more or less the functionnality we need. Maire asked me to upload the patch before the weekend so here it is. tl;dr, the aim of this patch is to turn the graph into a passive class, that gets woken up by something, gets asked to render some audio or video, does it, and goes back to sleep. The patch may seem big, but it's not complex. The main goal here is to isolate the clocks, iteration scheduling and threads from the MediaStreamGraphImpl class into another class, to be able to easily add another mecanism to run a graph. It's just a refactoring, the code will be functionnaly equivalent, but is necessary to continue the plan. Two new classes: Driver and DriverHolder - A Driver is something that can take a graph and run it: maybe using the system clock and a monitor to wait between iterations, maybe running it offline as fast as possible. The final goal here is to have a third Driver that schedules MediaStreamGraph iterations using an audio callback. It is implemented using an abstract base class (Driver), and two concrete classes, SystemClockDriver and OfflineClockDriver. - A DriverHolder is something that can switch between multiple drivers: the MSG will always start using a SystemClockDriver (because the graph might not have audio, ever: we always have a system clock, but having an audio clock is not guaranteed), but if it finds a MediaStream that wants to output audio, it will switch to an audio callback based Driver. It serves as a indirection layer to be able to have a consistent clock domain, despite switching to new clocks that start at 0 after the MediaStreamGraph has run for some time. What has been taken out of the MediaStreamGraphImpl, and why: - mStateComputedTime, mCurrentTime, now in the driver. The current names for mCurrentTime has been changed to mIntervalEnd, and there is a new mIntervalStart, which basically correspond to aFrom and aTo. I might replace mInterval{Start,End} by mIteration{Start,End}. Since this is the core of the clock, it has to go out of the MSG. Since I have plans to use the term "current time" for something else (the actual current time, in an interation, to make sure we can make callback deadlines, and check CPU load, etc.), I needed to use another term. - Graph wait/wakeups/EnsureNextIteration: iteration will, in the future, come from something else than a for loop (namely, callback from the system), so we need to isolate those parts from the graph itself. For example, the graph wait period with an audio callback will simply be the time between the callbacks. When we need to make the graph wait indefinitly, we will just pause the audio stream (hence receive no callbacks), etc. mWaitState is tied to this, so it had to go in the driver as well. - The monitor, because the driver will now do the thread management. - Other adjustments based on the fact that offline graphs and realtime graphs are more clearly separated. What is not done yet (will be on tuesday), is properly put the thread in the driver and not in the graph, fix not-so-nice things (like the initial stack allocated messageblock array that is passed from functions to functions way too much), and give a new interface to the MSG, something like: > void MediaStreamGraphImpl::Render(nsTArray<MessageBlock>& aMessages, GraphTime aFrom, GraphTime aTo); that would basically be one iteration.

New version of the first patch.

This puts the thread management in the driver instead of the graph. Some factoring, cleanups and comments are really missing, this is not reviewable, but is green locally.

All this is green on try, and don't change anything in terms of functionality. Next up, mix down all audio for the graph, and actually use the audio callback to do the processing.

Updating this to WebRTC:Audio/Video since WebRTC is the current driver for this work.

So, at this point, if everything is applied, in the order of the bug (grab the queue at [1] to make this easy), we have: - glitch-less switching between system clock and audio clock (basically, if there is audio in the graph, we use an audio clock, otherwise, we don't, and the graph dynamically switches between the two when needed). - processing happens in the audio callback, to ensure minimal buffering, and the lowest latency possible on the system (I haven't measured, but it's extremely clear by ear that the latency is reduced, testing a regular nightly and a build with those patches, using gUM). What needs to happen, now: - Maybe find a way to schedule video frames, because the audio callback might not be called often enough (worst case is a 60fps video, that needs to have a frame scheduled per 16.6ms, and this might not happen on mobile). roc, you told me in Taipei you had an idea for that, but I can't remember it. - Allow Gecko to run a MediaStreamGraph per AudioChannelType, so that we can route audio properly on mobile. This will certainly help for when we decide to support multiple outputs. This need to be discussed, as we will need to make sure cross-graph streams are working fine. - Test on more platforms (this is developed on Linux/Pulse, which is the highest latency backend we have on desktop) This patch queue builds and works at any point, is really is iterative, so some stuff is not-so-nice, but is fixed in the next patch, etc. There is some room for cleanups, I think, especially by the end. I tried to explain what I was doing in the commit messages, but I want to do a patch by patch write up so that the reviewer understands the approach. Finally, this is usable to test (I can do gUM, play with Web Audio demos, etc.), but I would expect a full test run to not be green. [1]: http://hg.mozilla.org/users/paul_paul.cx/patches/, the patches posted here are from revision 69aa59465b33

(In reply to Paul Adenot (:padenot) from comment #39) > - Test on more platforms (this is developed on Linux/Pulse, which is the > highest latency backend we have on desktop) We have a horrible workaround in Firefox OS' dialer application that prevents an audible glitch when we first play a sound. I can try to disable it on a device with your patch applied and see if the sound is smooth.

You can try, but it probably won't work. I plan to test FxOS later this week or next week if everything goes according to the plan.

(In reply to Paul Adenot (:padenot) from comment #39) > - glitch-less switching between system clock and audio clock (basically, if > there is audio in the graph, we use an audio clock, otherwise, we don't, and > the graph dynamically switches between the two when needed). > - processing happens in the audio callback, to ensure minimal buffering, and > the lowest latency possible on the system (I haven't measured, but it's > extremely clear by ear that the latency is reduced, testing a regular > nightly and a build with those patches, using gUM). Sounds great! > What needs to happen, now: > - Maybe find a way to schedule video frames, because the audio callback > might not be called often enough (worst case is a 60fps video, that needs to > have a frame scheduled per 16.6ms, and this might not happen on mobile). > roc, you told me in Taipei you had an idea for that, but I can't remember > it. The real fix is to propagate video sinks up to the source streams, let the source streams set the video frames directly on sinks every time they produce frames, and stop propagating video frames through the MSG. But I haven't been able to make time to work on that. But maybe we can not worry about this for now? > - Allow Gecko to run a MediaStreamGraph per AudioChannelType, so that we can > route audio properly on mobile. This will certainly help for when we decide > to support multiple outputs. This need to be discussed, as we will need to > make sure cross-graph streams are working fine. Cross-graph streams sounds really problematic. Why do we need an MSG per AudioChannelType?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #42) > (In reply to Paul Adenot (:padenot) from comment #39) > > What needs to happen, now: > > - Maybe find a way to schedule video frames, because the audio callback > > might not be called often enough (worst case is a 60fps video, that needs to > > have a frame scheduled per 16.6ms, and this might not happen on mobile). > > roc, you told me in Taipei you had an idea for that, but I can't remember > > it. > > The real fix is to propagate video sinks up to the source streams, let the > source streams set the video frames directly on sinks every time they > produce frames, and stop propagating video frames through the MSG. But I > haven't been able to make time to work on that. > > But maybe we can not worry about this for now? I think this can be postponed a bit. On Linux, Windows, OSX (which are the platforms I have tested an work), the callbacks fire _in the worst case_ (Linux/Pulse, default configuration) every 10ms. Per Nyquist & Shannon, to ensure that frames are displayed properly, and in the worst case (60fps video stream), we need to wake up every (1000ms / 60fps / 2) = 8.33ms. I haven't tested the callback frequency of b2g/android, but I think it's around 25/30ms or something, depending on the hardware, for low end devices (i.e. worst case). Considering that the videos that go in the graph are, most of the time, below 30fps (because they are from camera, local or remote), we can try just dropping a frame now and then, and then decide if it is acceptable. Then implement it properly when we have time. > > - Allow Gecko to run a MediaStreamGraph per AudioChannelType, so that we can > > route audio properly on mobile. This will certainly help for when we decide > > to support multiple outputs. This need to be discussed, as we will need to > > make sure cross-graph streams are working fine. > > Cross-graph streams sounds really problematic. Why do we need an MSG per Because this patch queue does a mixdown of the graph to a single cubeb_stream so we have only one callback firing and this saves us from worrying about concurrency issues in the graph. Basically, this pushes the concurrency issues at the edges of the graph, rather than having them in the middle of the processing path. Then some other reasons: - at least on Android (and b2g), the channel type cannot changed after the stream has been created. - for a given channel, the sound might be routed to a specific output (e.g. earpiece, headphones, loudspeaker), depending on the use case, so we can't have a single cubeb_stream there. - being able to have high latency graphs is going to be useful (people are building media player applications that need web audio-like features), so we would need a way to run the graph with deep buffers: render a lot of audio in each graph iteration, have a high latency audio stream at the end. That said, cross-graph streams are going to be edge cases, another reason why we should not slow the rendering path with it: - connecting a stream from an MSG routed to the earpiece to an MSG routed to the headphones - having an AudioContext with a "background" channel type connected to a AudioContext with a "normal" type We discussed about that with jesup, and Google people at a WebRTC workweek, and the general agreement was that it was okay to have some latency at graph boundaries to make things simpler. I think that the immense majority of applications are not going to need this, but it should work anyway. Also, note that because of the B2G one-app-per-process model, and because we have an MSG per process, and because this is only needed on mobile for now, this is somewhat mitigated.

I think we should clean up, review and land what we already have for this bug and file follow up bugs for the two issues discussed in Comment 43 ((1) direct connections of video sources with their sinks and (2) the ability to run one MediaStreamGraph per AudioChannelType). In the two follow up bugs we should detail out the use cases because that will help us determine when we need them to be implemented. Based on my understanding of the use cases and the discussion so far, it seems like we don't need them immediately (for Fx 32/v2.0). Does that sounds reasonable?

This is the only patch for now that changes the behavior: the output are mixed and only one cubeb stream is used. Try is still green up to that point.

Moving things around so that cubeb can be used from elsewhere than AudioStream.{cpp,h}.

Get rid of the MessageQueue that is on the stack, because cubeb will directly call back into our main function and we won't have a stack location that will persist accross calls.

A couple utility classes + tests that are going to be used when the AudioDriver is going to be implemented.

Comment on attachment 8430302 [details] [diff] [review] Factor out clocks and scheduling of MSG iterations. r= Review of attachment 8430302 [details] [diff] [review]: ----------------------------------------------------------------- Basic design looks good, but I'd like to see the finished documentation before r+ ::: content/media/GraphDriver.h @@ +3,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef CLOCKPROVIDER_H_ > +#define CLOCKPROVIDER_H_ GRAPHDRIVER_H_ @@ +27,5 @@ > + * A driver is responsible for the scheduling of the processing, the thread > + * management, and give the different clocks to a MediaStreamGraph. This is an > + * abstract base class. A MediaStreamGraph can be driven by an > + * OfflineClockDriver, if the graph is offline, or a SystemClockDriver, if the > + * graph is real time. Document that the MediaStreamGraph holds an owning reference to its driver. @@ +29,5 @@ > + * abstract base class. A MediaStreamGraph can be driven by an > + * OfflineClockDriver, if the graph is offline, or a SystemClockDriver, if the > + * graph is real time. > + */ > +class Driver class GraphDriver. Or maybe even MSGDriver. Either one. @@ +42,5 @@ > + GraphTime& aTo) = 0; > + virtual GraphTime GetCurrentTime() = 0; > + virtual void DoIteration(nsTArray<MessageBlock>& aMessageQueue) = 0; > + virtual void WaitForNextIteration() = 0; > + virtual void WakeUp() = 0; Can you document these please? ::: content/media/MediaStreamGraphImpl.h @@ +187,4 @@ > */ > + bool OneIteration(nsTArray<MessageBlock>& aMessageQueue); > + /* > + * Contains the finish comment! @@ +191,3 @@ > */ > + void DoIteration(nsTArray<MessageBlock>& aMessageQueue); > + GraphTime IterationEnd(); Document this.

Comment on attachment 8430303 [details] [diff] [review] Put the thread management in the driver. r= Review of attachment 8430303 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/GraphDriver.h @@ +43,5 @@ > + /** > + * Runs main control loop on the graph thread. Normally a single invocation > + * of this runs for the entire lifetime of the graph thread. > + */ > + virtual void RunThread() = 0; Document these! ::: content/media/MediaStreamGraph.cpp @@ +1341,5 @@ > stream->mWrapper->NotifyStreamStateChanged(); > } > for (int32_t i = stream->mMainThreadListeners.Length() - 1; i >= 0; --i) { > stream->mMainThreadListeners[i]->NotifyMainThreadStateChanged(); > +u } get rid of this 'u'!

Comment on attachment 8430304 [details] [diff] [review] Separate interval time calculation and actual processing, and give an audio-callback compatible interface to the Process function. r= Review of attachment 8430304 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/GraphDriver.cpp @@ +123,5 @@ > NS_ASSERTION(!messageQueue.IsEmpty(), > "Shouldn't have started a graph with empty message queue!"); > > + bool stillProcessing = true; > + while(stillProcessing) { space before ( @@ +129,5 @@ > + GetIntervalForIteration(prevCurrentTime, nextCurrentTime); > + > + GraphTime nextStateComputedTime = mGraphImpl->RoundUpToNextAudioBlock(IterationEnd() + MillisecondsToMediaTime(AUDIO_TARGET_MS)); > + > + stillProcessing = mGraphImpl->OneIteration(prevCurrentTime, nextCurrentTime, StateComputedTime(), nextStateComputedTime, messageQueue); Break line

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #57) Seems like making these changes to the first patch will result in a number of difficult-to-resolve merge conflicts. In that case, feel free to apply the changes as a separate patch somewhere in your queue.

Comment on attachment 8430306 [details] [diff] [review] Mix down all audio and only output a single stream. r= Review of attachment 8430306 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/AudioMixer.h @@ +19,5 @@ > AudioSampleFormat aFormat, > uint32_t aChannels, > uint32_t aFrames, > uint32_t aSampleRate) = 0; > +}; trailing whitespace! ::: content/media/MediaStreamGraph.cpp @@ -1674,5 @@ > amount += mConsumers.SizeOfExcludingThis(aMallocSizeOf); > - amount += mAudioOutputStreams.SizeOfExcludingThis(aMallocSizeOf); > - for (size_t i = 0; i < mAudioOutputStreams.Length(); i++) { > - amount += mAudioOutputStreams[i].SizeOfExcludingThis(aMallocSizeOf); > - } Shouldn't we call SizeOfExcludingThis on mAudioStream here? ::: content/media/MediaStreamGraphImpl.h @@ +363,5 @@ > * to the audio output stream. Returns the number of frames played. > */ > TrackTicks PlayAudio(MediaStream* aStream, GraphTime aFrom, GraphTime aTo); > + > + virtual void MixerCallback(AudioDataValue* aMixedBuffer, Document this @@ +608,2 @@ > */ > + nsRefPtr<AudioStream> mAudioStream; Can we call this mMixedAudioOutput? I just got confused between this and a MediaStream.

Comment on attachment 8430309 [details] [diff] [review] Put cubeb-related static functions in their own file. r= Review of attachment 8430309 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/GraphDriver.cpp @@ +136,5 @@ > GetIntervalForIteration(prevCurrentTime, nextCurrentTime); > > GraphTime nextStateComputedTime = mGraphImpl->RoundUpToNextAudioBlock(IterationEnd() + MillisecondsToMediaTime(AUDIO_TARGET_MS)); > > + printf("%ld %ld // %ld %ld\n", prevCurrentTime, nextCurrentTime, StateComputedTime(), nextStateComputedTime); You probably don't want this

Comment on attachment 8430311 [details] [diff] [review] Get rid of the weird stack allocated message queue. r= Review of attachment 8430311 [details] [diff] [review]: ----------------------------------------------------------------- Can you explain, in a commit message preferably, more about why this change is wanted? ::: content/media/CubebUtils.cpp @@ +9,1 @@ > #include "CubebUtils.h" This probably belongs in the previous patch

Comment on attachment 8430313 [details] [diff] [review] Implement classes to ensure safe audio buffer manipulation. r= Review of attachment 8430313 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/AudioBufferUtils.h @@ +75,5 @@ > + /** > + * Check that the buffer is completly filled, and reset internal state so this > + * instance can be reused. > + */ > + void EnsureBufferFilled() { Call this BufferComplete() since EnsureBufferFilled sounds like it won't have side effects like clearing the state. @@ +97,5 @@ > + uint32_t mSampleWriteOffset; > +}; > + > +/** > + * This is a class that interfaces with above's classes, and is responsible for there's only one class above

With this refactoring, it is possible to call into a WASAPI function from a thread that does not have COM initialized, namely when the MediaStreamGraph switches from a system clock to an audio clock, and initializes the audio stream on the thread that was previously running the graph.

This is needed because the audio callback can now call deep into WebRTC code, and they seem to use a bunch of stack-allocated memory. Without this change, a simple WebRTC call see the threads' stack overflow. Making the stack four times bigger seem to work fine.

Comment on attachment 8441471 [details] [diff] [review] Make sure COM is initialized when calling into WASAPI functions. r= (In reply to Paul Adenot (:padenot) from comment #65) > Created attachment 8441471 [details] [diff] [review] > Make sure COM is initialized when calling into WASAPI functions. r= > > With this refactoring, it is possible to call into a WASAPI function from a > thread that does not have COM initialized, namely when the MediaStreamGraph > switches from a system clock to an audio clock, and initializes the audio > stream > on the thread that was previously running the graph. You've only converted some of the libcubeb API calls to use auto_com. What about the others? I'm also not sure how sane it is to initialize and then uninitialize COM on every API call...

Comment on attachment 8441473 [details] [diff] [review] Increase the size of the stack for the audio threads on Windows. r= Review of attachment 8441473 [details] [diff] [review]: ----------------------------------------------------------------- Make it 256 * 1024 to match cubeb_alsa.

No big changes, mainly more comments.

(Adding ordering number in commit messages because there are quite a few patches coming).

No changes, carrying r+ forward.

No changes, carrying r+ forward.

No changes appart from review comments, carrying r+ forward.

carrying r+ forward.

In the next patches, when the AudioDriver will be implemented, the audio backend thread (that we don't control), will return from the stack frame where the nsTArray that allows the MSG thread to exchange message queues in a efficient manner with the main thread. We put it in the MediaStreamGraph to avoid adding an allocation/deallocation per iteration on the MSG thread. In addition, the graph will be able to run on different threads during its lifetime, so we can't guarantee a stable stack frame to allocate things on anymore. The array are renamed with meaningful names, explaining the double-buffering pattern: the back queue is filled by the main thread, and is swapped with the front queue that is processed by the MSG thread. Arrays accesses are synchronized using the driver's monitor.

Addressed comments, carrying r+ forward.

This is useful because some platform have rather slow audio stream initialization time, especially the first time an audio stream is created for the process. We put in telemetry probes to measure that: First stream opened for the process: http://telemetry.mozilla.org/#filter=nightly%2F33%2FAUDIOSTREAM_FIRST_OPEN_MS&aggregates=multiselect-all!Submissions!Mean!5th%20percentile!25th%20percentile!median!75th%20percentile!95th%20percentile&evoOver=Builds&locked=true&sanitize=true&renderhistogram=Graph Subsequent streams: http://telemetry.mozilla.org/#filter=nightly%2F33%2FAUDIOSTREAM_LATER_OPEN_MS&aggregates=multiselect-all!Submissions!Mean!5th%20percentile!25th%20percentile!median!75th%20percentile!95th%20percentile&evoOver=Builds&locked=true&sanitize=true&renderhistogram=Graph

This is the interesting patch. Note that in this patch, some cubeb calls can be performed on the main thread. These are later moved off-main-thread, but I thought this patch was big and complicated enough alread, so I kept the async cubeb operations in another patch. Of course the two patches will land at the same time, because we've seen time like 8.5 _seconds_ to open a stream. Anyway, here is how this works: - As soon as the should output audio, everything is running inside the audio callback. This brings the latency of the graph down to the platform's minimal acceptable latency: 12.5ms on osx, 40ms on linux, 10-30ms on windows, 12.5-80ms on android/b2g (depending on hardware). - Switching to another driver happens at graph iteration boundaries: the graph detects that it should run on another driver (for example, we just added an audio output to a MediaStream, the graph now needs to output audio, it will switch to an AudioCallbackDriver). At the end of the iteration, the control of the graph is passed to the other driver. At the beginning of the next iteration, the driver that has taken control of the graph will clean up the previous driver. There are some complicated stuff going on when an AudioCallbackDriver is started because of the prefill, but it's commented in the code. - The AudioCallbackDriver is a normal AudioMixer consummer, it gets called back when the audio is mixed down, and fills the callback buffer, at the same time we feed the audio data back to the AEC and the like, so we are really sure we write the same data in the speaker and back to the AEC. - The tricky bit is the clock time calculation. The state time is decided by the number of frames the callback needs (we can't change that). We need to derive a reasonnable current time out of that. I'm not quite sure of the formula I put it, but it works well in practice. - Putting the monitor in the driver was in fact a terrible design choice (because you don't know which monitor to trust when switching), so it's back in the MSG from now on.

(forgot numbering in the patch message)

This seem to be the behavior of some other backend, no?

For some reason, this is needed since Karl's GraphTime unit refactoring, and only breaks osx.

I only converted the function where it's needed. If we have crazy needs after, we can add some more calls. It will blow up very explicitly on the call anyways. Also, it's sane to init/deinit COM multiple times, according to MSDN. Looking at other WASAPI backends, they do exactly this ("autocom" class on API calls).

This is just paranoia, but was kind of useful during development.

This should work, but I can't remember how to trigger the stream pause. I've tried a bunch of stuff, but I could not make it work, even without this patch queue.

This is kind of broken because we need to have a graph per channel type for routing.

Addressed review comments.

This is mostly green on try. I figured I'd fix the emulator issues while this is getting reviewed, as they are simply because the emulator is too slow, I think. https://tbpl.mozilla.org/?tree=Try&rev=ed39ffb29bce

Comment on attachment 8450286 [details] [diff] [review] Part 1 - Factor out clocks and scheduling of MSG iterations. r= Review of attachment 8450286 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/GraphDriver.h @@ +91,5 @@ > + * Call this to indicate that another iteration of the control loop is > + * required immediately. The monitor must already be held. > + */ > + void > + EnsureImmediateWakeUpLocked() { void on same line as function name, here and below

Comment on attachment 8450298 [details] [diff] [review] Part 8 - Allow to pass in hints when getting a reference to a MediaStreamGraph to get the right driver started as soon as possible. r= Review of attachment 8450298 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure this is a good idea. In later patches you make video-only streams set up a MediaStreamGraph with the system clock as the driver. But that means if the first stream contains no audio but lots of others do, we don't get the benefit of using the audio clock. I think we need to not specify these hints but instead always use the audio clock driver. or, possibly, dynamically switch drivers based on whether there are any streams playing audio. ::: content/media/MediaStreamGraph.cpp @@ +2514,3 @@ > mDriverHolder.Switch(new SystemClockDriver(this)); > } else { > + printf("New Graph, using a OfflineClockDriver %p\n", this); Remove these printfs

Comment on attachment 8450298 [details] [diff] [review] Part 8 - Allow to pass in hints when getting a reference to a MediaStreamGraph to get the right driver started as soon as possible. r= Review of attachment 8450298 [details] [diff] [review]: ----------------------------------------------------------------- Never mind, I should have read more closely.

Comment on attachment 8450314 [details] [diff] [review] Part 9 - Add a MediaStreamGraph driver based on an audio callback. r= Review of attachment 8450314 [details] [diff] [review]: ----------------------------------------------------------------- Generally looks really good ::: content/media/GraphDriver.cpp @@ +64,5 @@ > + GraphTime aLastSwitchNextIterationEnd, > + GraphTime aLastSwitchStateComputedTime, > + GraphTime aLastSwitchNextStateComputedTime) > +{ > + // We set mIterationEnd here, because the first thing a driver when he does an when *it* does @@ +561,5 @@ > + // audio buffers), it can be that we don't have a message here (because this > + // driver is the first one for this graph), and the graph would exit. Simply > + // return here until we have messages. > + if (!mGraphImpl->MessagesQueued()) { > + return aFrames; Shouldn't we zero the output buffer here? @@ +591,5 @@ > + mGraphImpl->RoundUpToNextAudioBlock(mStateComputedTime + mBuffer.Available()); > + > + mIterationStart = mIterationEnd; > + GraphTime inGraph = mStateComputedTime - mIterationStart; > + mIterationEnd = mIterationStart + 0.8 * inGraph; Can you explain this? @@ +618,5 @@ > + > + > + if (mOtherDriver && stillProcessing) { > + { > + // If we the audio stream has not been started by the previous driver or delete "we" @@ +619,5 @@ > + > + if (mOtherDriver && stillProcessing) { > + { > + // If we the audio stream has not been started by the previous driver or > + // the graph itself, keep it alive. I don't understand this comment. Keep what alive? It looks like we don't switch to mOtherDriver until this driver has started, why is that? is it because mOtherDriver is the previous driver in that case? If so I think the code would benefit by separating mOtherDriver into mPrevDriver/mNextDriver. ::: content/media/GraphDriver.h @@ +236,5 @@ > public: > DriverHolder(MediaStreamGraphImpl* aGraphImpl); > GraphTime GetCurrentTime(); > > + // Immediatly switch to another driver. Immediately @@ +258,4 @@ > // The lifetime of this pointer is equal to the lifetime of the graph, so it > // will never be null. > MediaStreamGraphImpl* mGraphImpl; > + // XXX What were you going to say here? @@ +330,5 @@ > // Time, in GraphTime, for each iteration > GraphTime mSlice; > }; > > +class AudioCallbackDriver : public GraphDriver, needs a comment for documentation ::: content/media/MediaStreamGraph.cpp @@ +2548,5 @@ > + mDriverHolder.Switch(new SystemClockDriver(this)); > + } > + } else { > + mDriverHolder.Switch(new OfflineClockDriver(this, MEDIA_GRAPH_TARGET_PERIOD_MS)); > + } Fix indent ::: content/media/MediaStreamGraphImpl.h @@ +418,5 @@ > GraphDriver* CurrentDriver() { > return mDriverHolder.GetDriver(); > } > > + void SetCurrentDriver(GraphDriver* aDriver) { Please document when it's safe to call this. Presumably it's almost never safe to call... @@ +457,5 @@ > + // mMonitor guards the data below. > + // MediaStreamGraph normally does its work without holding mMonitor, so it is > + // not safe to just grab mMonitor from some thread and start monkeying with > + // the graph. Instead, communicate with the graph thread using provided > + // mechanisms such as the ControlMessage queue. This comment doesn't explain when you *should* use mMonitor.

Comment on attachment 8450329 [details] [diff] [review] Part 13 - Add an RAII class to ensure another thread is not in the audio callback when shutting down. r= Review of attachment 8450329 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/GraphDriver.h @@ +416,5 @@ > * driver back to a SystemClockDriver). > * This is synchronized by the Graph's monitor. > * */ > bool mStarted; > + bool mInCallback; Document threading constraints around access to mInCallback. Actually it looks like you just access it racily, but if so you should document that.

Comment on attachment 8450332 [details] [diff] [review] Part 14 - Run all blocking cubeb operations off-main-thread. r= Review of attachment 8450332 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/GraphDriver.cpp @@ +145,5 @@ > MOZ_ASSERT(NS_IsMainThread()); > + // We can't release an audio driver on the main thread, because it can be > + // blocking. > + if (mDriver->AsAudioCallbackDriver()) { > + printf("Releasing audio driver off main thread.\n"); remove printf @@ +424,5 @@ > +AsyncCubebTask::Run() > +{ > + MOZ_ASSERT(mThread); > + if (NS_IsMainThread()) { > + mThread->Shutdown(); // can't shutdown from the thread itself, darn We're just not doing the operation we're supposed to do in this case, right? If so, let's assert here.

Comment on attachment 8450348 [details] [diff] [review] Part 20 - Remove the now useless DriverHolder class. r= Review of attachment 8450348 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/MediaStreamGraphImpl.h @@ +429,5 @@ > + * switch occur, previous driver is either deleted, or it's ownership is > + * passed to a event that will take care of the asynchronous cleanup, as > + * audio stream can take some time to shut down. > + */ > + GraphDriver* mDriver; can we make this an nsRefPtr to make things clearer?

Addressed comments.

Addressed comments