Closed Bug 694814 Opened 13 years ago Closed 10 years ago

Move AEC from WebRTC code to getUserMedia

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla31
blocking-b2g -

People

(Reporter: jesup, Assigned: jesup)

References

(Blocks 1 open bug)

Details

(Whiteboard: [getUserMedia] [blocking-gum-][p=13, 1.5:p1, ft:webrtc])

Attachments

(7 files, 23 obsolete files)

8.36 KB, patch
padenot
: review+
Details | Diff | Splinter Review
11.22 KB, patch
padenot
: review+
Details | Diff | Splinter Review
12.09 KB, patch
padenot
: review+
Details | Diff | Splinter Review
7.12 KB, patch
glandium
: review+
ted
: review+
Details | Diff | Splinter Review
10.12 KB, patch
Details | Diff | Splinter Review
11.31 KB, patch
jib
: review+
Details | Diff | Splinter Review
13.57 KB, patch
padenot
: review+
Details | Diff | Splinter Review
Move AEC from WebRTC code to our audio drivers, so that we can both echo-cancel any other audio we're generating from the browser, and to allow us to use a system or hardware echo-canceller, if they exist.

-> kinetik based on discussions at All-Hands

Not immediately critical; primarily to improve the performance and user-experience.
Putting it in the audio drivers is critical if we're going to allow any audio processing (especially webaudio) between the output of the PeerConnection and the speakers.
Priority: -- → P1
Whiteboard: [WebRTC] [blocking-webrtc-]
Depends on: 818670
So, as an outgrowth of bug 818670 where we're enabling the AEC, here are some of our options:

In increasing order of complexity and utility:

1) do AEC on all PeerConnections and streams in a peerconnection instead of just individual ones
   We pull audio data from separate channels, but that means the audio far-end reference data for each one is separate.  With two PC's, each would have it's own echo cancelled, but not the echo of the other (and you use N times the CPU resource to do the AEC, which is expensive).

   We could use as a far-end signal the output to cubeb of MediaStreamGraph.  That would cancel all MSG-sourced echoes.

2) Combined with 1), the cancellation can be moved entirely to occur at the microphone input in getUserMedia().  This avoids mis-cancelling when the audio source isn't a microphone, and also provides cancelling for non-peerconnection use-cases (gUM audio loopback, though it's not a super-useful usecase).

3) A better version of this would combine all MSG-sourced audio with all other audio output to the system audio drivers by the browser; this would cancel any other sources of audio that don't go through MSG (Youtube videos, etc).

4) Hooking into system-provided AECs, and/or finding a way to get access to all system audio, not just the browser's.  This may only be possible on B2G, though.


Note that some minor API changes will need to be made to allow feeding the reference signal into the webrtc audio channel from the 'outside'.  We'll want to do them in a way that will allow the changes to be upstreamed to webrtc.org.
Assignee: kinetik → nobody
Component: Video/Audio → WebRTC: Audio/Video
Summary: Move AEC from WebRTC code to our audio drivers → Move AEC from WebRTC code to getUserMedia
Whiteboard: [WebRTC] [blocking-webrtc-] → [getUserMedia] [blocking-gum-]
Blocks: 894820
Assignee: nobody → rjesup
Target Milestone: --- → mozilla28
Blocks: 916331
Depends on: 929138
Plus this one for v1.3 since it blocks the webRTC committed feature.
blocking-b2g: --- → 1.3+
Whiteboard: [getUserMedia] [blocking-gum-] → [getUserMedia] [blocking-gum-][ft:webrtc]
We are planning to do this for 28, to improve AEC performance, especially in cases where we're in a mesh conference.  Since landing bug 884365, the input latency is far more stable, and gives the AEC a chance to function.  However, there are still issues with cancelling in mesh conferences, and on mobile we don't want to run multiple copies of the AEC if we can avoid it.  Also, if we combine WebAudio with WebRTC, it will only work (outside of a few simple cases) if the AEC is before insertion to the MediaStream.

Audio-only 1-1 peerconnection calls on mobile should work without this.  This may slightly improve them.  The big improvements will be be for other cases, per above.  I would not block 1.3 on this, though I believe it will make 28/1.3.
blocking-b2g: 1.3+ → 1.3?
(In reply to Randell Jesup [:jesup] from comment #4)
> We are planning to do this for 28, to improve AEC performance, especially in
> cases where we're in a mesh conference.  Since landing bug 884365, the input
> latency is far more stable, and gives the AEC a chance to function. 
> However, there are still issues with cancelling in mesh conferences, and on
> mobile we don't want to run multiple copies of the AEC if we can avoid it. 
> Also, if we combine WebAudio with WebRTC, it will only work (outside of a
> few simple cases) if the AEC is before insertion to the MediaStream.
> 
> Audio-only 1-1 peerconnection calls on mobile should work without this. 
> This may slightly improve them.  The big improvements will be be for other
> cases, per above.  I would not block 1.3 on this, though I believe it will
> make 28/1.3.

Sounds good. Moving to blocking- then.
blocking-b2g: 1.3? → -
On reviewing cubeb, I find that there's no mixed stream available currently (some OS's can provide a 'loopback' copy of what did play out (modulo issues with protected content), but not all, and we don't implement that currently).

Either we'll need to mix in cubeb, or we'll need to get audio back from the OS, or we'll need to use OS AEC capabilities, if they're good enough (and don't glitch enabling and disabling, etc).

The fallback will be to use AEC in the peerconnection as we do today, and cancel only one stream.

:-(
Target Milestone: mozilla28 → ---
Attached patch WIP patch (obsolete) — Splinter Review
WIP patch that uses an AudioStream for the remote source.  Note: assumes there is only ONE audiostream in use!  Note also some SDP logging stuff is mixed in, sorry
If it made sense as part of this patch to make mixed audio (from the OS or otherwise) available to web content, that would be tremendously useful for writing functional tests of a whole variety of audio-using apps.

If not, I can file a separate bug.
The APIs here wouldn't directly lend themselves to web content getting access - but you could write something to use these APIs to provide that.  I'd say file a separate bug, and we'll keep that in mind designing this API/patch
Flags: needinfo?(dmose)
Thanks, Jesup.  Bug 957725 filed on exposing mixed audio somehow.
Flags: needinfo?(dmose)
Blocks: 970426
No longer blocks: 970426
Attachment #8341101 - Attachment is obsolete: true
Depends on: 818822
Depends on: 970715
Attachment #8374512 - Attachment is obsolete: true
Attachment #8374513 - Attachment is obsolete: true
Attachment #8391695 - Attachment description: WIP patch to move AEC to getUserMedia() → Patch 1 - WIP to move AEC to getUserMedia()
This assumed a patch queue of: Bug 818822, Bug 982490 (3 patches including my fixes), then these 3.  One would probably also want to flip the aec debug setting in build/gyp.mozbuild
FYI, while there are plenty of rough edges and hacks to deal with in this, this does appear to work and cancel in GUM (use http://mozilla.github.com/webrtc-landing/gum_test.html to test).  The Audio test here is a cruel test, as it loops back gUM audio to the speakers, which normally would lead to an uncontrolled feedback loop (and not something someone would normally do - to note, this test does NOT need to work well; it's not a real usecase).  Real usecases are a normal call in speakerphone mode, especially at higher volumes.  Note also that very high volumes may cause rattling, clipping or other non-linear effects which an AEC cannot correct for.
Target Milestone: --- → mozilla31
Attachment #8391695 - Attachment is obsolete: true
Attachment #8391881 - Attachment is obsolete: true
Attachment #8391882 - Attachment is obsolete: true
Attachment #8393673 - Attachment is obsolete: true
Blocks: 985714
Attachment #8393671 - Attachment is obsolete: true
Attachment #8393691 - Attachment is obsolete: true
Blocks: 970426
Attachment #8393675 - Attachment is obsolete: true
Attachment #8394349 - Attachment is obsolete: true
Attachment #8393670 - Attachment is obsolete: true
Attachment #8394347 - Attachment is obsolete: true
Comment on attachment 8395049 [details] [diff] [review]
Patch 2: modifications to webrtc.org single_rw_fifo

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

ted for build side, bsmedberg for the memory barrier.  I'm not certain if I could hoist the MemoryBarrier out of the loop, but since Clear is almost never called I wanted to be safe.
Attachment #8395049 - Flags: review?(ted)
Attachment #8395049 - Flags: review?(benjamin)
Comment on attachment 8395049 [details] [diff] [review]
Patch 2: modifications to webrtc.org single_rw_fifo

Switching to glandium for review; he did the previous change to this file (__sync_synchronize)
Attachment #8395049 - Flags: review?(benjamin) → review?(mh+mozilla)
Attachment #8395048 - Attachment is obsolete: true
Attachment #8395156 - Attachment is obsolete: true
Comment on attachment 8395158 [details] [diff] [review]
Patch 1: Add farend input to webrtc.org upstream

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

note: for reference, the actual analysis is done in AudioProcessingImpl::AnalyzeReverseStream() in audio_processing_impl.cc
Attachment #8395158 - Flags: review?(paul)
Attachment #8394953 - Attachment is obsolete: true
Attachment #8395160 - Attachment description: Patch 3 - WIP patch to add far-end output observers → Patch 3 - add far-end output observers to getUserMedia
Comment on attachment 8395160 [details] [diff] [review]
Patch 3 - add far-end output observers to getUserMedia

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

Note that this uses a singleton global observer in place of the TBD API in MSG to register (the XXX comments are where it needs insertion)
Attachment #8395160 - Flags: review?(paul)
Attachment #8393674 - Attachment is obsolete: true
Attachment #8395165 - Flags: review?(paul)
Attachment #8394952 - Flags: review?(paul)
Attachment #8394952 - Attachment description: Patch 5 - WIP enable AEC in getUserMedia → Patch 5 - enable AEC in getUserMedia instead of PeerConnection
Comment on attachment 8395049 [details] [diff] [review]
Patch 2: modifications to webrtc.org single_rw_fifo

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

::: media/webrtc/trunk/webrtc/modules/audio_device/android/single_rw_fifo.cc
@@ +83,5 @@
> +  while (size() > 0) {
> +    // Memory barrier ensures that |size_| is updated after the size has changed.
> +    subtle::MemoryBarrier();
> +    --size_; // can't do size_ = 0
> +  }

Seems to me you should use CompareExchange instead of decreasing size one by one.
Attachment #8395049 - Flags: review?(mh+mozilla) → review-
Comment on attachment 8395158 [details] [diff] [review]
Patch 1: Add farend input to webrtc.org upstream

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

This looks like it could work, but I'm not familiar with this code. rs=padenot
Attachment #8395158 - Flags: review?(paul) → review+
Comment on attachment 8395165 [details] [diff] [review]
Patch 4 - Add audio playout delay config var

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

Did you forget to set a sensible default in all.js? Or maybe if you pass in 0 to the AEC, it will do something smart?
Comment on attachment 8394952 [details] [diff] [review]
Patch 5 - enable AEC in getUserMedia instead of PeerConnection

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

Same thing, I'm not familiar with this code, but it seems correct. rs=padenot
Attachment #8394952 - Flags: review?(paul) → review+
Comment on attachment 8395165 [details] [diff] [review]
Patch 4 - Add audio playout delay config var

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

0 should be ok (we could use 10 as well), but likely we'll need to add platform-specific output delays similar to current "capture_delay" settings, until we add a followup to get the delay from cubeb.

I'll add a separate patch to set the defaults.
Comment on attachment 8398371 [details] [diff] [review]
Patch 6 - Only enable AEC when directly hooked into a PeerConnection/MediaPipeline

r? to jib, as mostly this is plumbing changes
Attachment #8398371 - Flags: review?(jib)
move Clear() into the output observer to reduce upstream changes
Attachment #8395160 - Attachment is obsolete: true
Attachment #8395160 - Flags: review?(paul)
Attachment #8395049 - Attachment is obsolete: true
Attachment #8395049 - Flags: review?(ted)
Comment on attachment 8398685 [details] [diff] [review]
Patch 2: modifications to webrtc.org single_rw_fifo

Removed the Clear() function; setting up a cross-platform solution for that was a bit of a pain and might not be taken upstream; I made the caller instead just Pop() until empty.
Attachment #8398685 - Flags: review?(mh+mozilla)
Attachment #8398685 - Flags: review?(ted)
Attachment #8398684 - Flags: review?(paul)
Attachment #8398685 - Flags: review?(ted) → review+
Comment on attachment 8398371 [details] [diff] [review]
Patch 6 - Only enable AEC when directly hooked into a PeerConnection/MediaPipeline

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

Some questions.

::: dom/media/MediaManager.cpp
@@ +450,5 @@
>        mSourceStream->AddDirectListener(aListener);
> +      // Tell GUM to start echo-cancelling
> +      // XXX Only do if the app didn't specify on or off
> +      if (mEchoOn) {
> +        RUN_ON_THREAD(MediaManager::GetThread(),

Should this be #ifdef MOZ_WEBRTC ?

I don't have enough context, I'm just guessing from similar code in MediaManager.h

@@ +591,5 @@
> +    int32_t aec = (int32_t) webrtc::kEcUnchanged;
> +    int32_t agc = (int32_t) webrtc::kAgcUnchanged;
> +    int32_t noise = (int32_t) webrtc::kNsUnchanged;
> +    bool aec_on = false, agc_on = false, noise_on = false;
> +    int32_t playout_delay = 0;

These could be located closer to their use.

@@ +669,5 @@
>        new TracksAvailableCallback(mManager, mSuccess, mWindowID, trackunion);
>  
>  #ifdef MOZ_WEBRTC
> +    // leave AEC off until it's connected to a PeerConnection
> +    mListener->AudioConfig(false, (uint32_t) aec,

Previously, AudioConfig() was only called here if both do_GetService() and do_QueryInterface() succeeded, and only #ifdef MOZ_WEBRTC, whereas now it is called all the time. Is this intentional?

::: modules/libpref/src/init/all.js
@@ +284,5 @@
> +pref("media.getusermedia.capture_delay", 10);
> +#endif
> +// Adjustments for OS-specific AudioStream+cubeb+output delay (lower bound)
> +#if defined(XP_MACOSX)
> +pref("media.getusermedia.playout_delay", 10);

Just for readability, maybe lump playout_delay in with capture_delay to cut the number of #ifdefs in half? The structure seems identical.
Flags: needinfo?(rjesup)
Attachment #8395165 - Flags: review?(paul) → review+
Comment on attachment 8398684 [details] [diff] [review]
Patch 3 - WIP patch to add far-end output observers

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

::: content/media/MediaStreamGraph.cpp
@@ +1191,5 @@
>    // xxx
> +  if (aFrames > 0 && aChannels > 0) {
> +    // XXX need Observer base class and registration API
> +    if (gFarendObserver) {
> +      gFarendObserver->InsertFarEnd(aMixedBuffer, aFrames, 44100, aChannels, AUDIO_FORMAT_FLOAT32);

s/44100/IdealAudioRate()/, so it works on all configurations.
s/AUDIO_FORMAT_FLOAT32/aFormat/, because the buffer will be integers on mobile (and we can save a float -> int16 conversion).

::: content/media/webrtc/AudioOutputObserver.h
@@ +12,5 @@
> +}
> +
> +namespace mozilla {
> +
> +// Fifo for cubeb to dump data into that's sent to the speakers.

This is not accurate anymore, since we take the MSG output rather than cubeb's.

@@ +25,5 @@
> +
> +  void Clear();
> +  // XXX Needs a "underran" boolean
> +  void InsertFarEnd(const void *aBuffer, uint32_t aSamples,
> +                    int aFreq, int aChannels, AudioSampleFormat aFormat);

The msg can certainly tell you if the input side it too slow at feeding data (because the MediaStream is blocked).

@@ +27,5 @@
> +  // XXX Needs a "underran" boolean
> +  void InsertFarEnd(const void *aBuffer, uint32_t aSamples,
> +                    int aFreq, int aChannels, AudioSampleFormat aFormat);
> +
> +  int8_t *Pop();

Pop() should return short*, to make it clear that the consumer will get 16-bits integer audio samples.

@@ +31,5 @@
> +  int8_t *Pop();
> +  uint32_t Size();
> +
> +  uint32_t mPlayoutFreq;
> +  uint32_t mPlayoutChannels;

No need for this to be public, right? Just write two inline members that get the value for you. That way, we can also make sure the values are valid when getting them.

@@ +38,5 @@
> +  nsAutoPtr<webrtc::SingleRwFifo> mPlayoutFifo;
> +  uint32_t mChunkSize;
> +
> +  // chunking to 10ms support
> +  nsAutoPtr<int16_t> mSaved;

nsTArray, please. Also, we tend to use "short" (and not int16_t) for integer audio samples in content/media.

::: content/media/webrtc/MediaEngineWebRTCAudio.cpp
@@ +20,5 @@
>  #define SAMPLE_LENGTH ((SAMPLE_FREQUENCY*10)/1000)
>  
> +// FIX! get from cubeb
> +#define MAX_CHANNELS 2
> +#define MAX_SAMPLING_FREQ 48000 // Hz - multiple of 100

Either this restriction is arbitrary, and it should not exist, or it comes from the AEC code, and we should get the values from there. Also, we should not make assumptions regarding the ideal sample rate of the MSG: The MSG get the information for the OS. If the user has set a samplerate of 96kHz, say in the little windows configuration panel, then it'll be the MediaStreamGraph::IdealSampleRate(), and this will assert.

@@ +21,5 @@
>  
> +// FIX! get from cubeb
> +#define MAX_CHANNELS 2
> +#define MAX_SAMPLING_FREQ 48000 // Hz - multiple of 100
> +#define MAX_AEC_FIFO_DEPTH 200 // ms - multiple of 10

MOZ_STATIC_ASSERT(!(MAX_AEC_FIFO_DEPTH % 10), "MAX_AEC_FIFO_DEPTH should be a multiple of 10");

@@ +47,5 @@
> +
> +AudioOutputObserver::AudioOutputObserver() : mPlayoutFreq(0),
> +                                             mPlayoutChannels(0),
> +                                             mChunkSize(0),
> +                                             mSamplesSaved(0)

We usually put the initializer lists at column 1.

@@ +97,5 @@
> +    }
> +  } else {
> +    MOZ_ASSERT(aFreq <= MAX_SAMPLING_FREQ);
> +    mPlayoutFreq = aFreq;
> +    mChunkSize = aFreq/100; // 10ms

MOZ_ASSERT(!(aFreq % 100), "Sampling rate for far end data should be multiple of 100.");

Also, spaces around binary operators.

@@ +112,5 @@
> +  const float *src_float = (const float *) aBuffer;
> +
> +  while (aSamples) {
> +    if (!mSaved) {
> +      mSaved = new int16_t[1 + mChunkSize * aChannels];

I'd be more comfortable with nsTArrays, here, so you don't need to do this little dance with the length.

@@ +117,5 @@
> +      // put the length in the first two bytes as an uint16_t
> +      mSaved[0] = mChunkSize;
> +    }
> +    uint32_t to_copy = mSaved[0] - mSamplesSaved;
> +    if (to_copy > mChunkSize - mSamplesSaved)

Always brace your ifs, and prefer camelCase, please.

@@ +138,5 @@
> +        break;
> +      default:
> +        MOZ_ASSERT(false, "Unsupported audio format for AEC far end");
> +        break;
> +    }

content/media/AudioSampleFormat.h so we don't reinvent the wheel here. You can even drop the switch case because of the template magic.

@@ +149,5 @@
> +    aSamples -= to_copy;
> +    mSamplesSaved += to_copy;
> +
> +    if (mSamplesSaved >= mChunkSize) {
> +      int free_slots = mPlayoutFifo->capacity() - mPlayoutFifo->size();

camelCase

@@ +508,5 @@
> +  // input code with "old" audio.
> +  if (!mStarted) {
> +    mStarted  = true;
> +    while (gFarendObserver->Size() > 1) {
> +      int8_t *buffer = gFarendObserver->Pop(); // only call if size() > 0

MOZ_ASSERT(gFarendObserver->Size > 0);
Attachment #8398684 - Flags: review?(paul) → review-
Attachment #8398685 - Flags: review?(mh+mozilla) → review+
(In reply to Paul Adenot (:padenot) from comment #48)
> Comment on attachment 8398684 [details] [diff] [review]
> Patch 3 - WIP patch to add far-end output observers
> 
> Review of attachment 8398684 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +38,5 @@
> > +  nsAutoPtr<webrtc::SingleRwFifo> mPlayoutFifo;
> > +  uint32_t mChunkSize;
> > +
> > +  // chunking to 10ms support
> > +  nsAutoPtr<int16_t> mSaved;
> 
> nsTArray, please. Also, we tend to use "short" (and not int16_t) for integer
> audio samples in content/media.

mSaved is a single buffer, not an array.  And we need to switch to int16_t at some point in this process as that's what this feed into in the WebRTC code.

> ::: content/media/webrtc/MediaEngineWebRTCAudio.cpp
> @@ +20,5 @@
> >  #define SAMPLE_LENGTH ((SAMPLE_FREQUENCY*10)/1000)
> >  
> > +// FIX! get from cubeb
> > +#define MAX_CHANNELS 2
> > +#define MAX_SAMPLING_FREQ 48000 // Hz - multiple of 100
> 
> Either this restriction is arbitrary, and it should not exist, or it comes
> from the AEC code, and we should get the values from there. Also, we should
> not make assumptions regarding the ideal sample rate of the MSG: The MSG get
> the information for the OS. If the user has set a samplerate of 96kHz, say
> in the little windows configuration panel, then it'll be the
> MediaStreamGraph::IdealSampleRate(), and this will assert.

The WebRTC AEC code doesn't handle >48000Hz currently.  Also, I believe it handles at most stereo, but I'll check.

> 
> @@ +21,5 @@
> >  
> > +// FIX! get from cubeb
> > +#define MAX_CHANNELS 2
> > +#define MAX_SAMPLING_FREQ 48000 // Hz - multiple of 100
> > +#define MAX_AEC_FIFO_DEPTH 200 // ms - multiple of 10
> 
> MOZ_STATIC_ASSERT(!(MAX_AEC_FIFO_DEPTH % 10), "MAX_AEC_FIFO_DEPTH should be
> a multiple of 10");

ok
 
> @@ +47,5 @@
> > +
> > +AudioOutputObserver::AudioOutputObserver() : mPlayoutFreq(0),
> > +                                             mPlayoutChannels(0),
> > +                                             mChunkSize(0),
> > +                                             mSamplesSaved(0)
> 
> We usually put the initializer lists at column 1.

All the WebRTC code I think has it indented, but I'll check.

> 
> @@ +97,5 @@
> > +    }
> > +  } else {
> > +    MOZ_ASSERT(aFreq <= MAX_SAMPLING_FREQ);
> > +    mPlayoutFreq = aFreq;
> > +    mChunkSize = aFreq/100; // 10ms
> 
> MOZ_ASSERT(!(aFreq % 100), "Sampling rate for far end data should be
> multiple of 100.");

ok

> 
> Also, spaces around binary operators.

The rest of the file's style is no spaces (again I'll check).  Mixing styles in a file is bad, and restyling generally to be avoided (messes up blame, etc).

> 
> @@ +112,5 @@
> > +  const float *src_float = (const float *) aBuffer;
> > +
> > +  while (aSamples) {
> > +    if (!mSaved) {
> > +      mSaved = new int16_t[1 + mChunkSize * aChannels];
> 
> I'd be more comfortable with nsTArrays, here, so you don't need to do this
> little dance with the length.

We need to pass these as int16_t *'s to the AEC code.  What I *really* want to do is replace the lockless-rw-fifo of pointers with a lockless-rw-circular-buffer of data to avoid having to allocate for every 10ms chunk, so switching to nsTArray will simply make that harder.

> 
> @@ +117,5 @@
> > +      // put the length in the first two bytes as an uint16_t
> > +      mSaved[0] = mChunkSize;
> > +    }
> > +    uint32_t to_copy = mSaved[0] - mSamplesSaved;
> > +    if (to_copy > mChunkSize - mSamplesSaved)
> 
> Always brace your ifs, and prefer camelCase, please.

Bracing - sure, missed that one I guess.  local_var style in this file isn't camelCase.

> 
> @@ +138,5 @@
> > +        break;
> > +      default:
> > +        MOZ_ASSERT(false, "Unsupported audio format for AEC far end");
> > +        break;
> > +    }
> 
> content/media/AudioSampleFormat.h so we don't reinvent the wheel here. You
> can even drop the switch case because of the template magic.

I'll check that.
jib: I'll revise that patch; I think we're not going to need the "enable AEC once connected to PeerConnection part of it" (though this adds more pressure to be able to enable/disable it on the fly)
Flags: needinfo?(rjesup)
with interdiffs folded in
Attachment #8398684 - Attachment is obsolete: true
Comment on attachment 8400075 [details] [diff] [review]
Patch 3 - WIP patch to add far-end output observers

Interdiffs are uploaded as a separate patch
Attachment #8400075 - Flags: review?(paul)
back to turning on AEC by default.  Changed config names to reduce future confusion.  Keeps access to change AEC settings in-call, though we haven't hooked that up yet
Attachment #8398371 - Attachment is obsolete: true
Attachment #8398371 - Flags: review?(jib)
Attachment #8400086 - Flags: review?(jib)
Attachment #8400075 - Attachment is obsolete: true
Attachment #8400075 - Flags: review?(paul)
Comment on attachment 8400126 [details] [diff] [review]
Patch 3 - WIP patch to add far-end output observers

Added a 'break' if the rw_fifo is full to avoid ilooping (this may be fallout of the restructuring I just did).
Attachment #8400126 - Flags: review?(paul)
Comment on attachment 8400086 [details] [diff] [review]
Patch 6 - Only enable AEC when directly hooked into a PeerConnection/MediaPipeline

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

r=me.

::: dom/media/MediaManager.cpp
@@ +599,5 @@
> +#ifdef MOZ_WEBRTC
> +    // Right now these configs are only of use if webrtc is available
> +    nsresult rv;
> +    nsCOMPtr<nsIPrefService> prefs = do_GetService("@mozilla.org/preferences-service;1", &rv);
> +    if (NS_SUCCEEDED(rv)) {

One point from comment 47 is left unaddressed: if this fails...

@@ +623,5 @@
>        LOG(("Returning error for getUserMedia() - no stream"));
>        error->OnError(NS_LITERAL_STRING("NO_STREAM"));
>        return NS_OK;
>      }
> +    trackunion->AudioConfig(aec_on, (uint32_t) aec,

...you now set AudioConfig to hard-coded values instead of values from about:config. This is a change, but it seems reasonable.
Attachment #8400086 - Flags: review?(jib) → review+
Forgot to qref and pick up the ConvertAudioSamples change
Attachment #8400126 - Attachment is obsolete: true
Attachment #8400126 - Flags: review?(paul)
Attachment #8400452 - Flags: review?(paul)
Comment on attachment 8400452 [details] [diff] [review]
Patch 3 - WIP patch to add far-end output observers

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

::: content/media/webrtc/MediaEngineWebRTCAudio.cpp
@@ +121,5 @@
> +  // samples per call.  Annoying...
> +  while (aSamples) {
> +    if (!mSaved) {
> +      mSaved = (FarEndAudioChunk *) moz_xmalloc(sizeof(FarEndAudioChunk) +
> +                                                (mChunkSize * aChannels - 1)*sizeof(int16_t));

remove the -1, right?

@@ +122,5 @@
> +  while (aSamples) {
> +    if (!mSaved) {
> +      mSaved = (FarEndAudioChunk *) moz_xmalloc(sizeof(FarEndAudioChunk) +
> +                                                (mChunkSize * aChannels - 1)*sizeof(int16_t));
> +      // put the length in the first two bytes as an uint16_t

this is obsolete

@@ +127,5 @@
> +      mSaved->mSamples = mChunkSize;
> +      mSaved->mOverrun = aOverran;
> +      aOverran = false;
> +    }
> +    uint32_t to_copy = mChunkSize - mSamplesSaved;

camelCase
Attachment #8400452 - Flags: review?(paul) → review+
(In reply to Paul Adenot (:padenot) from comment #59)
> Comment on attachment 8400452 [details] [diff] [review]
> Patch 3 - WIP patch to add far-end output observers
> 
> Review of attachment 8400452 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/webrtc/MediaEngineWebRTCAudio.cpp
> @@ +121,5 @@
> > +  // samples per call.  Annoying...
> > +  while (aSamples) {
> > +    if (!mSaved) {
> > +      mSaved = (FarEndAudioChunk *) moz_xmalloc(sizeof(FarEndAudioChunk) +
> > +                                                (mChunkSize * aChannels - 1)*sizeof(int16_t));
> 
> remove the -1, right?

Nope.  mData[1].  (Last I recall we can't use [0])


> @@ +127,5 @@
> > +      mSaved->mSamples = mChunkSize;
> > +      mSaved->mOverrun = aOverran;
> > +      aOverran = false;
> > +    }
> > +    uint32_t to_copy = mChunkSize - mSamplesSaved;
> 
> camelCase

The rest of the file isn't camelCase for locals.
Blocks: 991125
backed out of central:
https://hg.mozilla.org/mozilla-central/rev/ac6cbaa47f34
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 996433
Can somebody confirm this does break b2g build, when WEBRTC is disabled? I am on an older version of code, and just backported this patches, but I think it should break anyway, as webrtc:: code inside dom/media/MediaManager.cpp is not wrapped with ifdefs.
Flags: needinfo?(rjesup)
(In reply to Nikola from comment #69)
> Can somebody confirm this does break b2g build, when WEBRTC is disabled? I
> am on an older version of code, and just backported this patches, but I
> think it should break anyway, as webrtc:: code inside
> dom/media/MediaManager.cpp is not wrapped with ifdefs.

This breaks all builds with webrtc disabled, as reported in 996433.
(In reply to Landry Breuil (:gaston) from comment #70)
> (In reply to Nikola from comment #69)
> > Can somebody confirm this does break b2g build, when WEBRTC is disabled? I
> > am on an older version of code, and just backported this patches, but I
> > think it should break anyway, as webrtc:: code inside
> > dom/media/MediaManager.cpp is not wrapped with ifdefs.
> 
> This breaks all builds with webrtc disabled, as reported in 996433.

Sorry, did not see that one. Thank you for your prompt reply.
Flags: needinfo?(rjesup)
Depends on: 1000539
Blocks: 1001272
Whiteboard: [getUserMedia] [blocking-gum-][ft:webrtc] → [getUserMedia] [blocking-gum-][ft:webrtc][s=fx32]
Whiteboard: [getUserMedia] [blocking-gum-][ft:webrtc][s=fx32] → [getUserMedia] [blocking-gum-][p=13;ft:webrtc][s=fx32]
Whiteboard: [getUserMedia] [blocking-gum-][p=13;ft:webrtc][s=fx32] → [getUserMedia] [blocking-gum-][p=13, 1.5:p1, ft:webrtc][s=fx32]
Whiteboard: [getUserMedia] [blocking-gum-][p=13, 1.5:p1, ft:webrtc][s=fx32] → [getUserMedia] [blocking-gum-][p=13, 1.5:p1, ft:webrtc]
You need to log in before you can comment on or make changes to this bug.