1428392 - Remove AudioOutputObserver

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, enhancement, P2)

Description

[Paul Adenot (:padenot)]

Because we now use duplex audio streams unconditionally, we can simply feed the reverse-stream (i.e. the mixed audio output of the MSG) to the AEC directly, skipping a bunch of copies,  allocations, and lifetime management stuff. It also removes an ad-hoc packtizer and replaces it by our AudioPacketizer<> class.

Comment 1

[Paul Adenot (:padenot)]

Attachment: Bug 1428392 - Rename the MediaEngineWebRTCMicrophoneSource packetizer to indicate it's packetizing the input data (microphone).

Review commit: https://reviewboard.mozilla.org/r/210564/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/210564/

Comment 2

[Paul Adenot (:padenot)]

Attachment: Bug 1428392 - Remove a leftover VoE pointer in MediaEngineWebRTC.h

Review commit: https://reviewboard.mozilla.org/r/210566/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/210566/

Comment 3

[Paul Adenot (:padenot)]

Attachment: Bug 1428392 - Remove AudioOutputObserver, and feed the reverse-stream directly to the AEC.

Review commit: https://reviewboard.mozilla.org/r/210568/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/210568/

Comment 4

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8940267 [details]
Bug 1428392 - Rename the MediaEngineWebRTCMicrophoneSource packetizer to indicate it's packetizing the input data (microphone).

https://reviewboard.mozilla.org/r/210564/#review216650

Comment 5

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8940268 [details]
Bug 1428392 - Remove a leftover VoE pointer in MediaEngineWebRTC.h

https://reviewboard.mozilla.org/r/210566/#review216652

Comment 6

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8940269 [details]
Bug 1428392 - Remove AudioOutputObserver, and feed the reverse-stream directly to the AEC.

https://reviewboard.mozilla.org/r/210568/#review216656

Looks good, and great cleanup!

One issue with a removed channel count, the rest is comment nits.

::: dom/media/TrackUnionStream.cpp
(Diff revision 1)
> -/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-*/

See "Mode line" on https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:641
(Diff revision 1)
> -    StreamConfig inputConfig(mAudioOutputObserver->PlayoutFrequency(),
> +    StreamConfig inputConfig(aRate);
> -                             channelCountFarend,

Why don't we need the channel count in the config anymore?

Looks to like this will lead to an error: https://searchfox.org/mozilla-central/rev/f42618c99dcb522fb674221acfbc68c2d92e7936/media/webrtc/trunk/webrtc/modules/audio_processing/audio_processing_impl.cc#1340

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:673
(Diff revision 1)
> +  size_t offset = 0;
> +
> +  if (!mPacketizerInput ||
> +      mPacketizerInput->PacketSize() != aRate/100u ||
> +      mPacketizerInput->Channels() != aChannels) {
> +    // It's ok to drop the audio still in the packetizer here.

Mention *why* it's ok.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:679
(Diff revision 1)
> +    mPacketizerInput =
> +      new AudioPacketizer<AudioDataValue, float>(aRate/100, aChannels);
> +  }
> +
> +  // On initial capture, throw away all far-end data except the most recent sample
> +  // since it's already irrelevant and we want to keep avoid confusing the AEC far-end

s/keep // ?

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:685
(Diff revision 1)
> +  // input code with "old" audio.
> +  if (!mStarted) {
> +    mStarted  = true;
> +  }
> +
> +  // Feed the far-end audio data (speakers) to the feedback input of the AEC.

Isn't far-end data handled in NotifyOutputData with ProcessReverseStream?

Comment 7

[Paul Adenot (:padenot)]

Comment on attachment 8940269 [details]
Bug 1428392 - Remove AudioOutputObserver, and feed the reverse-stream directly to the AEC.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210568/diff/1-2/

Comment 8

[Paul Adenot (:padenot)]

Comment on attachment 8940269 [details]
Bug 1428392 - Remove AudioOutputObserver, and feed the reverse-stream directly to the AEC.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210568/diff/2-3/

Comment 9

[Paul Adenot (:padenot)]

Comment on attachment 8940269 [details]
Bug 1428392 - Remove AudioOutputObserver, and feed the reverse-stream directly to the AEC.

https://reviewboard.mozilla.org/r/210568/#review216656

> Why don't we need the channel count in the config anymore?
> 
> Looks to like this will lead to an error: https://searchfox.org/mozilla-central/rev/f42618c99dcb522fb674221acfbc68c2d92e7936/media/webrtc/trunk/webrtc/modules/audio_processing/audio_processing_impl.cc#1340

A plain mistake. I've made returning an error here a fatal assert.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/209f16a94427
Rename the MediaEngineWebRTCMicrophoneSource packetizer to indicate it's packetizing the input data (microphone). r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/a638a0a5874c
Remove a leftover VoE pointer in MediaEngineWebRTC.h r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/07859f6fa8ce
Remove AudioOutputObserver, and feed the reverse-stream directly to the AEC. r=pehrsons

Comment 11

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/209f16a94427
https://hg.mozilla.org/mozilla-central/rev/a638a0a5874c
https://hg.mozilla.org/mozilla-central/rev/07859f6fa8ce