Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Move AEC from WebRTC code to getUserMedia

RESOLVED FIXED in mozilla31

Status

()

Core
WebRTC: Audio/Video
P1
normal
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

(Blocks: 4 bugs)

Trunk
mozilla31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:-)

Details

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

Attachments

(7 attachments, 23 obsolete attachments)

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
(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

5 years ago
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-]
(Assignee)

Updated

5 years ago
Depends on: 818670
(Assignee)

Comment 2

5 years ago
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
(Assignee)

Updated

4 years ago
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-]
(Assignee)

Updated

4 years ago
Blocks: 894820
Assignee: nobody → rjesup
Target Milestone: --- → mozilla28
(Assignee)

Updated

4 years ago
Blocks: 916331
(Assignee)

Updated

4 years ago
Depends on: 929138

Comment 3

4 years ago
Plus this one for v1.3 since it blocks the webRTC committed feature.
blocking-b2g: --- → 1.3+

Updated

4 years ago
Whiteboard: [getUserMedia] [blocking-gum-] → [getUserMedia] [blocking-gum-][ft:webrtc]
(Assignee)

Comment 4

4 years ago
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? → -
(Assignee)

Comment 6

4 years ago
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 → ---
(Assignee)

Comment 7

4 years ago
Created attachment 8341101 [details] [diff] [review]
WIP patch

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

Comment 9

4 years ago
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)

Updated

4 years ago
Blocks: 970426
No longer blocks: 970426
(Assignee)

Comment 11

4 years ago
Created attachment 8374512 [details] [diff] [review]
WIP patch to move AEC to getUserMedia()
(Assignee)

Comment 12

4 years ago
Created attachment 8374513 [details] [diff] [review]
WIP patch for adding output observers to inject mixed audio data into the AEC
(Assignee)

Updated

4 years ago
Attachment #8341101 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Depends on: 818822
(Assignee)

Updated

4 years ago
Depends on: 970715
(Assignee)

Comment 13

3 years ago
Created attachment 8391695 [details] [diff] [review]
Patch 1 - WIP to move AEC to getUserMedia()
(Assignee)

Updated

3 years ago
Attachment #8374512 - Attachment is obsolete: true
(Assignee)

Comment 14

3 years ago
Created attachment 8391881 [details] [diff] [review]
Patch 2 - WIP for adding output observers to inject mixed audio data into the AEC
(Assignee)

Updated

3 years ago
Attachment #8374513 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8391695 - Attachment description: WIP patch to move AEC to getUserMedia() → Patch 1 - WIP to move AEC to getUserMedia()
(Assignee)

Comment 15

3 years ago
Created attachment 8391882 [details] [diff] [review]
Patch 3 - Convert AEC-in-gum to use output mixer in MSG

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
(Assignee)

Comment 16

3 years ago
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.
(Assignee)

Updated

3 years ago
Target Milestone: --- → mozilla31
(Assignee)

Comment 17

3 years ago
Created attachment 8393670 [details] [diff] [review]
Patch 1: Add farend input to webrtc.org upstream
(Assignee)

Comment 18

3 years ago
Created attachment 8393671 [details] [diff] [review]
Patch 2: modifications to webrtc.org single_rw_fifo
(Assignee)

Comment 19

3 years ago
Created attachment 8393673 [details] [diff] [review]
Patch 3 - WIP patch to add far-end output observers
(Assignee)

Comment 20

3 years ago
Created attachment 8393674 [details] [diff] [review]
Patch 4 - Add audio playout delay config var
(Assignee)

Comment 21

3 years ago
Created attachment 8393675 [details] [diff] [review]
Patch 5 - WIP enable AEC in getUserMedia
(Assignee)

Updated

3 years ago
Attachment #8391695 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8391881 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8391882 - Attachment is obsolete: true
(Assignee)

Comment 22

3 years ago
Created attachment 8393691 [details] [diff] [review]
Patch 3 - WIP patch to add far-end output observers
(Assignee)

Updated

3 years ago
Attachment #8393673 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Blocks: 985714
(Assignee)

Comment 23

3 years ago
Created attachment 8394347 [details] [diff] [review]
Patch 2: modifications to webrtc.org single_rw_fifo
(Assignee)

Updated

3 years ago
Attachment #8393671 - Attachment is obsolete: true
(Assignee)

Comment 24

3 years ago
Created attachment 8394349 [details] [diff] [review]
Patch 3 - WIP patch to add far-end output observers
(Assignee)

Updated

3 years ago
Attachment #8393691 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Blocks: 970426
(Assignee)

Comment 25

3 years ago
Created attachment 8394952 [details] [diff] [review]
Patch 5 - enable AEC in getUserMedia instead of PeerConnection
(Assignee)

Updated

3 years ago
Attachment #8393675 - Attachment is obsolete: true
(Assignee)

Comment 26

3 years ago
Created attachment 8394953 [details] [diff] [review]
Patch 3 - WIP patch to add far-end output observers
(Assignee)

Updated

3 years ago
Attachment #8394349 - Attachment is obsolete: true
(Assignee)

Comment 27

3 years ago
Created attachment 8395048 [details] [diff] [review]
Patch 1: Add farend input to webrtc.org upstream
(Assignee)

Updated

3 years ago
Attachment #8393670 - Attachment is obsolete: true
(Assignee)

Comment 28

3 years ago
Created attachment 8395049 [details] [diff] [review]
Patch 2: modifications to webrtc.org single_rw_fifo
(Assignee)

Updated

3 years ago
Attachment #8394347 - Attachment is obsolete: true
(Assignee)

Comment 29

3 years ago
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)
(Assignee)

Comment 30

3 years ago
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)
(Assignee)

Comment 31

3 years ago
Created attachment 8395156 [details] [diff] [review]
Patch 1: Add farend input to webrtc.org upstream
(Assignee)

Updated

3 years ago
Attachment #8395048 - Attachment is obsolete: true
(Assignee)

Comment 32

3 years ago
Created attachment 8395158 [details] [diff] [review]
Patch 1: Add farend input to webrtc.org upstream
(Assignee)

Updated

3 years ago
Attachment #8395156 - Attachment is obsolete: true
(Assignee)

Comment 33

3 years ago
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)
(Assignee)

Comment 34

3 years ago
Created attachment 8395160 [details] [diff] [review]
Patch 3 - add far-end output observers to getUserMedia
(Assignee)

Updated

3 years ago
Attachment #8394953 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8395160 - Attachment description: Patch 3 - WIP patch to add far-end output observers → Patch 3 - add far-end output observers to getUserMedia
(Assignee)

Comment 35

3 years ago
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)
(Assignee)

Comment 36

3 years ago
Created attachment 8395165 [details] [diff] [review]
Patch 4 - Add audio playout delay config var
(Assignee)

Updated

3 years ago
Attachment #8393674 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8395165 - Flags: review?(paul)
(Assignee)

Updated

3 years ago
Attachment #8394952 - Flags: review?(paul)
(Assignee)

Updated

3 years ago
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+
(Assignee)

Comment 41

3 years ago
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.
(Assignee)

Comment 42

3 years ago
Created attachment 8398371 [details] [diff] [review]
Patch 6 - Only enable AEC when directly hooked into a PeerConnection/MediaPipeline
(Assignee)

Comment 43

3 years ago
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)
(Assignee)

Comment 44

3 years ago
Created attachment 8398684 [details] [diff] [review]
Patch 3 - WIP patch to add far-end output observers

move Clear() into the output observer to reduce upstream changes
(Assignee)

Updated

3 years ago
Attachment #8395160 - Attachment is obsolete: true
Attachment #8395160 - Flags: review?(paul)
(Assignee)

Comment 45

3 years ago
Created attachment 8398685 [details] [diff] [review]
Patch 2: modifications to webrtc.org single_rw_fifo
(Assignee)

Updated

3 years ago
Attachment #8395049 - Attachment is obsolete: true
Attachment #8395049 - Flags: review?(ted)
(Assignee)

Comment 46

3 years ago
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)
(Assignee)

Updated

3 years ago
Attachment #8398685 - Flags: review?(ted)
(Assignee)

Updated

3 years ago
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)

Updated

3 years ago
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+
(Assignee)

Comment 49

3 years ago
(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.
(Assignee)

Comment 50

3 years ago
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)
(Assignee)

Comment 51

3 years ago
Created attachment 8400074 [details] [diff] [review]
interdiffs for patch 3 (far-end observers)
(Assignee)

Comment 52

3 years ago
Created attachment 8400075 [details] [diff] [review]
Patch 3 - WIP patch to add far-end output observers

with interdiffs folded in
(Assignee)

Updated

3 years ago
Attachment #8398684 - Attachment is obsolete: true
(Assignee)

Comment 53

3 years ago
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)
(Assignee)

Comment 54

3 years ago
Created attachment 8400086 [details] [diff] [review]
Patch 6 - Only enable AEC when directly hooked into a PeerConnection/MediaPipeline

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
(Assignee)

Updated

3 years ago
Attachment #8398371 - Attachment is obsolete: true
Attachment #8398371 - Flags: review?(jib)
(Assignee)

Updated

3 years ago
Attachment #8400086 - Flags: review?(jib)
(Assignee)

Comment 55

3 years ago
Created attachment 8400126 [details] [diff] [review]
Patch 3 - WIP patch to add far-end output observers
(Assignee)

Updated

3 years ago
Attachment #8400075 - Attachment is obsolete: true
Attachment #8400075 - Flags: review?(paul)
(Assignee)

Comment 56

3 years ago
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+
(Assignee)

Comment 58

3 years ago
Created attachment 8400452 [details] [diff] [review]
Patch 3 - WIP patch to add far-end output observers

Forgot to qref and pick up the ConvertAudioSamples change
(Assignee)

Updated

3 years ago
Attachment #8400126 - Attachment is obsolete: true
Attachment #8400126 - Flags: review?(paul)
(Assignee)

Updated

3 years ago
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+
(Assignee)

Comment 60

3 years ago
(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.
(Assignee)

Updated

3 years ago
Blocks: 991125
(Assignee)

Comment 61

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8cb15625567
https://hg.mozilla.org/integration/mozilla-inbound/rev/34bc46be6f67
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e4db991bf74
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe548073933e
https://hg.mozilla.org/integration/mozilla-inbound/rev/03402caf2023
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ce56c66a536
(Assignee)

Comment 62

3 years ago
Backed out for perma-orange on b2g emulator M10:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=cb894b5d342f
(likely the MSG changes)

http://hg.mozilla.org/integration/mozilla-inbound/rev/c0eaa844f2f0
https://hg.mozilla.org/mozilla-central/rev/f8cb15625567
https://hg.mozilla.org/mozilla-central/rev/34bc46be6f67
https://hg.mozilla.org/mozilla-central/rev/1e4db991bf74
https://hg.mozilla.org/mozilla-central/rev/fe548073933e
https://hg.mozilla.org/mozilla-central/rev/03402caf2023
https://hg.mozilla.org/mozilla-central/rev/5ce56c66a536
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 64

3 years ago
backed out of central:
https://hg.mozilla.org/mozilla-central/rev/ac6cbaa47f34
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 65

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/89a615263614
https://hg.mozilla.org/integration/mozilla-inbound/rev/6922b1261595
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3664615ecbf
https://hg.mozilla.org/integration/mozilla-inbound/rev/74e5c32c6fa2
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dc08e9fc7e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cf98d1c6b75
(Assignee)

Comment 66

3 years ago
backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4cc87b3eeac
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e5f6bc7cdec
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ae7d42531c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/20aea86b3432
https://hg.mozilla.org/integration/mozilla-inbound/rev/63be52cd09c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/df77827fc918
(Assignee)

Comment 67

3 years ago
Third time's the charm!
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a27b8086916
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f5de7e21de8
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb7e248e9ccf
https://hg.mozilla.org/integration/mozilla-inbound/rev/263d6539e0a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/38cdf299eda8
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c233a3ad819
https://hg.mozilla.org/mozilla-central/rev/4a27b8086916
https://hg.mozilla.org/mozilla-central/rev/3f5de7e21de8
https://hg.mozilla.org/mozilla-central/rev/cb7e248e9ccf
https://hg.mozilla.org/mozilla-central/rev/263d6539e0a9
https://hg.mozilla.org/mozilla-central/rev/38cdf299eda8
https://hg.mozilla.org/mozilla-central/rev/3c233a3ad819
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED

Updated

3 years ago
Depends on: 996433

Comment 69

3 years ago
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.

Comment 71

3 years ago
(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
(Assignee)

Updated

3 years ago
Blocks: 1001272

Updated

3 years ago
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.