848954 - Call the MediaStreamGraph's processing code from the cubeb callback when needed

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

[(no longer active)]

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

Comment 2

[(no longer active)]

Paul, is this something you can work on?

Comment 3

[Paul Adenot (:padenot)]

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?

Comment 4

[Timothy B. Terriberry (:derf)]

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

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 8

[(no longer active)]

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

Comment 11

[Maire Reavy [:mreavy]]

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?

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 16

[Jason Smith [:jsmith]]

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.

Comment 17

[Paul Adenot (:padenot)]

roc has answered already.

Comment 18

[Paul Adenot (:padenot)]

This [1] is the current plan to do this, updated as we write the code.

[1]: https://webrtc.etherpad.mozilla.org/mediastream-refactor

Comment 19

[Paul Adenot (:padenot)]

Attachment: factor-out-clock

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.

Comment 20

[Paul Adenot (:padenot)]

Attachment: Factor out clocks and scheduling of MSG iterations. r=

New version of the first patch.

Comment 21

[Paul Adenot (:padenot)]

Attachment: Put the thread management in the driver. r=

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.

Comment 22

[Paul Adenot (:padenot)]

Attachment: Factor out clocks and scheduling of MSG iterations. r=

Comment 23

[Paul Adenot (:padenot)]

Attachment: Put the thread management in the driver. r=

Comment 24

[Paul Adenot (:padenot)]

Attachment: Separate interval time calculation and actual processing, and give an audio-callback compatible interface to the Process function. r=

Comment 25

[Paul Adenot (:padenot)]

Attachment: Factor behavior for drivers that manage their thread. r=

Comment 26

[Paul Adenot (:padenot)]

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.

Comment 27

[Maire Reavy [:mreavy]]

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

Comment 28

[Paul Adenot (:padenot)]

Attachment: Factor out clocks and scheduling of MSG iterations. r=

Comment 29

[Paul Adenot (:padenot)]

Attachment: Put the thread management in the driver. r=

Comment 30

[Paul Adenot (:padenot)]

Attachment: Separate interval time calculation and actual processing, and give an audio-callback compatible interface to the Process function. r=

Comment 31

[Paul Adenot (:padenot)]

Attachment: Factor behavior for drivers that manage their thread. r=

Comment 32

[Paul Adenot (:padenot)]

Attachment: Mix down all audio and only output a single stream. r=

Comment 33

[Paul Adenot (:padenot)]

Attachment: Put cubeb-related static functions in their own file. r=

Comment 34

[Paul Adenot (:padenot)]

Attachment: Get rid of the weird stack allocated message queue. r=

Comment 35

[Paul Adenot (:padenot)]

Attachment: Implement classes to ensure safe audio buffer manipulation. r=

Comment 36

[Paul Adenot (:padenot)]

Attachment: Allow to pass in hints when getting a reference to a MediaStreamGraph to get the right driver started as soon as possible. r=

Comment 37

[Paul Adenot (:padenot)]

Attachment: Add a MediaStreamGraph driver based on an audio callback

Comment 38

[Paul Adenot (:padenot)]

Attachment: Allow switching between drivers. r=

Comment 39

[Paul Adenot (:padenot)]

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

Comment 40

[Gabriele Svelto [:gsvelto] (PTO)]

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

Comment 41

[Paul Adenot (:padenot)]

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.

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 43

[Paul Adenot (:padenot)]

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

Comment 44

[Maire Reavy [:mreavy]]

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?

Comment 45

[Paul Adenot (:padenot)]

Attachment: Factor out clocks and scheduling of MSG iterations. r=

Comment 46

[Paul Adenot (:padenot)]

Attachment: Put the thread management in the driver. r=

Comment 47

[Paul Adenot (:padenot)]

Attachment: Separate interval time calculation and actual processing, and give an audio-callback compatible interface to the Process function. r=

Comment 48

[Paul Adenot (:padenot)]

Attachment: Factor behavior for drivers that manage their thread. r=

Comment 49

[Paul Adenot (:padenot)]

Attachment: Mix down all audio and only output a single stream. r=

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.

Comment 50

[Paul Adenot (:padenot)]

Attachment: Put cubeb-related static functions in their own file. r=

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

Comment 51

[Paul Adenot (:padenot)]

Attachment: Get rid of the weird stack allocated message queue. r=

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.

Comment 52

[Paul Adenot (:padenot)]

Attachment: Implement classes to ensure safe audio buffer manipulation. r=

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

Comment 53

[Paul Adenot (:padenot)]

Attachment: Allow to pass in hints when getting a reference to a MediaStreamGraph to get the right driver started as soon as possible. r=

Comment 54

[Paul Adenot (:padenot)]

Attachment: Add a MediaStreamGraph driver based on an audio callback

Comment 55

[Paul Adenot (:padenot)]

Attachment: Allow switching between drivers. r=

Comment 56

[Paul Adenot (:padenot)]

Attachment: Rework the shutdown sequence because it's broken

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 58

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 60

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 62

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 63

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 64

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 65

[Paul Adenot (:padenot)]

Attachment: 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.

Comment 66

[Paul Adenot (:padenot)]

Attachment: Increase the size of the stack for the audio threads on Windows. r=

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 67

[Matthew Gregan [:kinetik]]

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 68

[Matthew Gregan [:kinetik]]

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.

Comment 69

[Paul Adenot (:padenot)]

Attachment: Factor out clocks and scheduling of MSG iterations. r=

No big changes, mainly more comments.

Comment 70

[Paul Adenot (:padenot)]

Attachment: Put the thread management in the driver. r=

Comment 71

[Paul Adenot (:padenot)]

Attachment: Part 1 - Factor out clocks and scheduling of MSG iterations. r=

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

Comment 72

[Paul Adenot (:padenot)]

Attachment: Part 2 - Put the thread management in the driver. r=

No changes, carrying r+ forward.

Comment 73

[Paul Adenot (:padenot)]

Attachment: Part 3 - Factor behavior for drivers that manage their thread. r=

No changes, carrying r+ forward.

Comment 74

[Paul Adenot (:padenot)]

Attachment: Mix down all audio and only output a single stream. r=

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

Comment 75

[Paul Adenot (:padenot)]

Attachment: Part 4 - Mix down all audio and only output a single stream. r=

Comment 76

[Paul Adenot (:padenot)]

Attachment: Part 5 - Put cubeb-related static functions in their own file. r=

carrying r+ forward.

Comment 77

[Paul Adenot (:padenot)]

Attachment: Part 6 - Get rid of the stack allocated message queue. r=

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.

Comment 78

[Paul Adenot (:padenot)]

Attachment: Part 7 - Implement classes to ensure safe audio buffer manipulation. r=

Addressed comments, carrying r+ forward.

Comment 79

[Paul Adenot (:padenot)]

Attachment: 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=

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

Comment 80

[Paul Adenot (:padenot)]

Attachment: Add a MediaStreamGraph driver based on an audio callback. r=

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.

Comment 81

[Paul Adenot (:padenot)]

Attachment: Part 9 - Add a MediaStreamGraph driver based on an audio callback. r=

(forgot numbering in the patch message)

Comment 82

[Paul Adenot (:padenot)]

Attachment: Part 10 - Allow the alsa backend to be shutdown while draining. r=

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

Comment 83

[Paul Adenot (:padenot)]

Attachment: Part 11 - When deciding to block a MediaStream due to a speculative data underrun, don't block if we have exactly the right number of frames buffered. r=

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

Comment 84

[Paul Adenot (:padenot)]

Attachment: Make sure COM is initialized when calling into WASAPI functions. r=

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

Comment 85

[Paul Adenot (:padenot)]

Attachment: Add an RAII class to ensure another thread is not in the audio callback when shutting down. r=

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

Comment 86

[Paul Adenot (:padenot)]

Attachment: Part 12 - Make sure COM is initialized when calling into WASAPI functions. r=

Comment 87

[Paul Adenot (:padenot)]

Attachment: Part 13 - Add an RAII class to ensure another thread is not in the audio callback when shutting down. r=

Comment 88

[Paul Adenot (:padenot)]

Attachment: Part 13 - Run all blocking cubeb operations off-main-thread. r=

Comment 89

[Paul Adenot (:padenot)]

Attachment: Part 14 - Run all blocking cubeb operations off-main-thread. r=

Comment 90

[Paul Adenot (:padenot)]

Attachment: Part 14 - Allow an AudioCallbackDriver to sleep to save power. r=

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.

Comment 91

[Paul Adenot (:padenot)]

Attachment: Part 15 - Allow an AudioCallbackDriver to sleep to save power. r=

Comment 92

[Paul Adenot (:padenot)]

Attachment: Part 16 - Make AudioCallbackDriver respect the AudioChannelType. r=

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

Comment 93

[Paul Adenot (:padenot)]

Attachment: 17 - Increase the size of the stack for the audio threads on Windows. r=

Addressed review comments.

Comment 94

[Paul Adenot (:padenot)]

Attachment: Part 18 - Start the thread after having processed the messages to make sure there are messages in the queue when processing starts. r=

Comment 95

[Paul Adenot (:padenot)]

Attachment: Part 19 - Delete the mixer callbacks on removal. r=

Comment 96

[Paul Adenot (:padenot)]

Attachment: Part 20 - Remove the now useless DriverHolder class. r=

Comment 97

[Paul Adenot (:padenot)]

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 98

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 99

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 100

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 101

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 102

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 103

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 104

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 105

[Paul Adenot (:padenot)]

Attachment: Part 9 - Add a MediaStreamGraph driver based on an audio callback. r=

Addressed comments.

Comment 106

[Paul Adenot (:padenot)]

Attachment: Part 20 - Remove the now useless DriverHolder class. r=

Addressed comments

Comment 107

[Paul Adenot (:padenot)]

Attachment: Part 21 - Fix sleep/wake up code for the MSG. r=

The sleep/wakeup logic was not really thread safe, and it should have been.

Comment 108

[Paul Adenot (:padenot)]

Attachment: Part 22 - Port the osx panning code to live in the AudioCallbackDriver. r=

Move the code to the new location, basically a copy/paste.

Comment 109

[Paul Adenot (:padenot)]

Attachment: Part 23 - Refcount the AudioOutputObserver. r=

This is now necessary, because I sometimes saw the MSG outliving the
MediaManager, and it crased.

Comment 110

[Paul Adenot (:padenot)]

Attachment: Part 24 - Clean up the clock getter, because the right thing is now done by the GraphDriver. r=

Comment 111

[Paul Adenot (:padenot)]

Attachment: Part 25 - Add documentation on some driver's members. r=

Comment 112

[Paul Adenot (:padenot)]

Attachment: Part 26 - Prevent a race when the MSG is going to sleep mode. r=

Comment 113

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8472440 [details] [diff] [review]
Part 9 - Add a MediaStreamGraph driver based on an audio callback. r=

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

::: content/media/GraphDriver.h
@@ +223,5 @@
> +  nsRefPtr<GraphDriver> mPreviousDriver;
> +  // This is non-null only when this driver has recently switched from an other
> +  // driver, and has not cleaned it up yet (for example because the audio stream
> +  // is currently calling the callback during initialization).
> +  nsRefPtr<GraphDriver> mNextDriver;

Shouldn't the comment for mNextDriver apply to mPreviousDriver and vice versa?

@@ +352,5 @@
> + *   sometimes hardware components are involved and need to be warmed up)
> + * - We have no control on how much audio we generate, we have to return exactly
> + *   the number of frames asked for by the callback. Since for the Web Audio
> + *   API, we have to do block processing at 128 frames per block, we need to
> + *   keep a little spill buffer to store the excedentary frames.

"the extra frames"

Comment 114

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8472441 [details] [diff] [review]
Part 20 - Remove the now useless DriverHolder class. r=

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

::: content/media/MediaStreamGraphImpl.h
@@ +426,5 @@
>    // Data members
> +  //
> +  /**
> +   * Graphs own owning references to their driver, until shutdown. When a driver
> +   * switch occur, previous driver is either deleted, or it's ownership is

"the previous driver is either deleted, or its ownership"

Comment 115

[Paul Adenot (:padenot)]

Attachment: Part 27 - Only attempt to sleep if the graph is still running. r=

(this will be disabled anyways in a later patch)

Comment 116

[Paul Adenot (:padenot)]

Attachment: Part 28 - Allow to set a driver twice per iteration iff the second driver is an audio driver. r=

This should not happen, but does. Somehow, we detect that there is no track with
audio in the graph, during the graph updates (after having ran the messages),
and then, during CreateOrDestroyAudioStream, there is a track with audio.

The real fix is to have only one location where we decide whether we need to
change driver or not.

Comment 117

[Paul Adenot (:padenot)]

Attachment: Part 28 - Properly stop the driver when shutting down the graph. r=

(this was originaly present, probably caused by a bad rebase)

Comment 118

[Paul Adenot (:padenot)]

Attachment: Part 29 - Make sure to clear the right boolean flag when dispatching a stable state event

Comment 119

[Paul Adenot (:padenot)]

Attachment: Part 29 - Make the MSG in stable state event ref counted

Comment 120

[Paul Adenot (:padenot)]

Attachment: Part 31 - Temporarily disable a web audio api test

This will be reenabled in bug 1058586

Comment 121

[Paul Adenot (:padenot)]

Attachment: Part 31 - Temporarily disable a web audio api test

This will be reenabled in bug 1058586

Comment 122

[Paul Adenot (:padenot)]

Attachment: Part 32 - Disable MSG sleep

This has a race somewhere, so we disable it for now.

The real fix will come from the Web Audio API Suspend API [0]

[0]: https://github.com/WebAudio/web-audio-api/issues/317

Comment 123

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8479018 [details] [diff] [review]
Part 28 - Allow to set a driver twice per iteration iff the second driver is an audio driver. r=

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

::: content/media/GraphDriver.cpp
@@ +68,5 @@
>  void GraphDriver::SwitchAtNextIteration(GraphDriver* aNextDriver)
>  {
>  
>    STREAM_LOG(PR_LOG_DEBUG, ("Switching to new driver: %p (%s)", aNextDriver, aNextDriver->AsAudioCallbackDriver() ? "AudioCallbackDriver" : "SystemClockDriver"));
> +  MOZ_ASSERT(!mNextDriver || !mNextDriver->AsAudioCallbackDriver());

Add a comment as to why this is needed - even if the complete cause isn't clear yet

@@ +770,5 @@
>                                               mIterationEnd,
>                                               mStateComputedTime,
>                                               mNextStateComputedTime);
>  
> +  mBuffer.BufferFilled();

is this part of the issue described, or is this cleanup?

Comment 124

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8479019 [details] [diff] [review]
Part 28 - Properly stop the driver when shutting down the graph. r=

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

r+ if StopInternal is removed (or just leave Stop and adjust ASSERT if needed)

::: content/media/GraphDriver.cpp
@@ +592,5 @@
> +  }
> +}
> +
> +void
> +AudioCallbackDriver::StopInternal()

What calls StopInternal()?  We added it, but I see no calls to it in this patch.  Also, it's identical to Stop() with a LOG() and an ASSERT...  What are the thread requirements for ::Stop?  And should we just modify ::Stop() to warn if on mainthread instead?

::: content/media/GraphDriver.h
@@ +392,5 @@
>    /* Start the cubeb stream */
>    void StartStream();
>    friend class AsyncCubebTask;
>    void Init();
> +  void StopInternal();

goes away if we remove it

::: content/media/MediaStreamGraph.cpp
@@ +1449,5 @@
>    {
>      NS_ASSERTION(mGraph->mDetectedNotRunning,
>                   "We should know the graph thread control loop isn't running!");
>  
> +    STREAM_LOG(PR_LOG_DEBUG, ("Shutting drown graph %p", mGraph));

let's keep the graph away from water, shall we? ;-)  ("down")

Comment 125

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf9daaf1ef28
https://hg.mozilla.org/integration/mozilla-inbound/rev/78ad22d3b5f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/37cd9c64ce97
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d2711c7fc5c
https://hg.mozilla.org/integration/mozilla-inbound/rev/0224b5d034e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/28cd206a24fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fc0684a08e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7e57c1f224e
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f8fa852edd3
https://hg.mozilla.org/integration/mozilla-inbound/rev/1967d592df13
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec95ea99d750
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a0199460e68
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc47d39b955e
https://hg.mozilla.org/integration/mozilla-inbound/rev/be1ece8d5824
https://hg.mozilla.org/integration/mozilla-inbound/rev/57c556be8687
https://hg.mozilla.org/integration/mozilla-inbound/rev/93842687054e
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5b970976871
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8bb3bcc38eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/256f26db10f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bc52009e325
https://hg.mozilla.org/integration/mozilla-inbound/rev/792b2a9e647e
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6cc3eebcd72
https://hg.mozilla.org/integration/mozilla-inbound/rev/511915d63977
https://hg.mozilla.org/integration/mozilla-inbound/rev/887ce6c58e3d
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d90c1b64653
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e4e5d289d3a
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0f902ee9636
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdd81b782104
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed3c0bd3e979
https://hg.mozilla.org/integration/mozilla-inbound/rev/3382beb6de84
https://hg.mozilla.org/integration/mozilla-inbound/rev/abaf1dc73334
https://hg.mozilla.org/integration/mozilla-inbound/rev/e61148266735
https://hg.mozilla.org/integration/mozilla-inbound/rev/a71337656d0c

Comment 126

[Randell Jesup [:jesup] (needinfo me)]

Note there are frequent Mulet M1 OOMs in content/media/webaudio/test/test_audioBufferSourceNodeNeutered.html (typically)
Around 50-60%; and retriggering the Try shows OOM as well (we got lucky).

Not sure of the cause.

I'm told you can do Mulet builds with 
# Mulet
ac_add_options --enable-application=b2g/dev

but possibly other things are needed.

Also, with a local build made that way, on ./mach mochitest-plain content/media/webaudio/tests I get an MOZ_ASSERT() at 
0x00007ffff17fc657 in mozilla::GraphDriver::UpdateStateComputedTime (this=0x7fffc87ea890, 
    aStateComputedTime=1536) at ../../../content/media/GraphDriver.cpp:91
91	  MOZ_ASSERT(aStateComputedTime >= mStateComputedTime, "State time can't go backward.");
(mStateComputedTime = 1792, aStateComputedTime = ~1500 typically

Comment 127

[Alexandre Poirot [:ochameau]]

Attachment: Disable webaudio tests on mulet

These tests were already explicitely disabled on b2g desktop...
so it makes sense to disable them also on Mulet.

(It could be worth figuring out why it crashes, 
 it may crash on b2g desktop if we enable them.)

Comment 128

[Carsten Book [:Tomcat]]

(In reply to Alexandre Poirot [:ochameau] from comment #127)
> Created attachment 8479647 [details] [diff] [review]
> Disable webaudio tests on mulet
> 
> These tests were already explicitely disabled on b2g desktop...
> so it makes sense to disable them also on Mulet.
> 
> (It could be worth figuring out why it crashes, 
>  it may crash on b2g desktop if we enable them.)

landed as https://hg.mozilla.org/integration/mozilla-inbound/rev/1b883c791174

Comment 129

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bf9daaf1ef28
https://hg.mozilla.org/mozilla-central/rev/78ad22d3b5f3
https://hg.mozilla.org/mozilla-central/rev/37cd9c64ce97
https://hg.mozilla.org/mozilla-central/rev/6d2711c7fc5c
https://hg.mozilla.org/mozilla-central/rev/0224b5d034e4
https://hg.mozilla.org/mozilla-central/rev/28cd206a24fc
https://hg.mozilla.org/mozilla-central/rev/9fc0684a08e0
https://hg.mozilla.org/mozilla-central/rev/e7e57c1f224e
https://hg.mozilla.org/mozilla-central/rev/6f8fa852edd3
https://hg.mozilla.org/mozilla-central/rev/1967d592df13
https://hg.mozilla.org/mozilla-central/rev/ec95ea99d750
https://hg.mozilla.org/mozilla-central/rev/6a0199460e68
https://hg.mozilla.org/mozilla-central/rev/dc47d39b955e
https://hg.mozilla.org/mozilla-central/rev/be1ece8d5824
https://hg.mozilla.org/mozilla-central/rev/57c556be8687
https://hg.mozilla.org/mozilla-central/rev/93842687054e
https://hg.mozilla.org/mozilla-central/rev/b5b970976871
https://hg.mozilla.org/mozilla-central/rev/f8bb3bcc38eb
https://hg.mozilla.org/mozilla-central/rev/256f26db10f1
https://hg.mozilla.org/mozilla-central/rev/8bc52009e325
https://hg.mozilla.org/mozilla-central/rev/792b2a9e647e
https://hg.mozilla.org/mozilla-central/rev/c6cc3eebcd72
https://hg.mozilla.org/mozilla-central/rev/511915d63977
https://hg.mozilla.org/mozilla-central/rev/887ce6c58e3d
https://hg.mozilla.org/mozilla-central/rev/0d90c1b64653
https://hg.mozilla.org/mozilla-central/rev/6e4e5d289d3a
https://hg.mozilla.org/mozilla-central/rev/f0f902ee9636
https://hg.mozilla.org/mozilla-central/rev/fdd81b782104
https://hg.mozilla.org/mozilla-central/rev/ed3c0bd3e979
https://hg.mozilla.org/mozilla-central/rev/3382beb6de84
https://hg.mozilla.org/mozilla-central/rev/abaf1dc73334
https://hg.mozilla.org/mozilla-central/rev/e61148266735
https://hg.mozilla.org/mozilla-central/rev/a71337656d0c
https://hg.mozilla.org/mozilla-central/rev/1b883c791174

Comment 130

[Liz Henry (:lizzard) (relman/hg->git project)]

Is there, or should there be, extra test coverage of this? Thanks.

Comment 131

[Paul Adenot (:padenot)]

(In reply to Liz Henry :lizzard from comment #130)
> Is there, or should there be, extra test coverage of this? Thanks.

There is a lot of QA on this already, I think we're fine.