Closed Bug 1428392 Opened 2 years ago Closed 2 years ago

Remove AudioOutputObserver

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: padenot, Assigned: padenot)

References

Details

Attachments

(3 files)

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.
Attachment #8940267 - Flags: review?(apehrson)
Attachment #8940268 - Flags: review?(apehrson)
Attachment #8940269 - Flags: review?(apehrson)
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
Attachment #8940267 - Flags: review?(apehrson) → review+
Comment on attachment 8940268 [details]
Bug 1428392 - Remove a leftover VoE pointer in MediaEngineWebRTC.h

https://reviewboard.mozilla.org/r/210566/#review216652
Attachment #8940268 - Flags: review?(apehrson) → review+
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?
Attachment #8940269 - Flags: review?(apehrson) → review+
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.
Pushed by paul@paul.cx:
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
You need to log in before you can comment on or make changes to this bug.