811695 - Stop using local socket transport to loopback captured audio

Status: VERIFIED FIXED

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

Description

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

Currently, capturing audio involves:

a) capturing it from the OS
b) pre-processing it (AGC if turned on, mute, replace-with-file, typing detection, etc)
c) "encoding" it in L16 and wrapping in RTP
d) sending it to a 127.0.0.1 socket (triggers security alert on Windows)
e) receiving data on socket
f) decoding RTP and "decoding" L16 data
g) stuffing into MediaStreamGraph

You'll note that b-f are things we either don't want, or are totally useless to us and at best just add processing and memory use (and maybe delay). (AGC and the like can be dealt with when we feed this stream into the channel again before sending it.)

If we register a 'NullConduit' as an external transport, we could cut out the socket (huge plus on windows), but all the processing and RTP would remain.  A better solution may involve modifying the webrtc/trunk code to allow us to bypass most or all the processing and the RTP encode/decode by registering a sink.

(blocking-gum+ because of the Windows security issue)

Comment 1

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

Attachment: disable internal socket transports

Comment 2

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

Comment on attachment 681830 [details] [diff] [review]
disable internal socket transports

derf or anant on this one (though both is better)

Turns out we were *totally* throwing away the RTP audio data we looped through the socket to ourselves!  Instead, MediaEngineWebRTCAudio was collecting the actual data using an existing external callback after AGC and other APM processing is done, before packetization.

So, I disabled all internal socket code (which allows it to 'send' without setting a destination), and added a define to allow it to believe the data has been 'sent' correctly if there's no transport.  I could avoid touching the imported code at all by adding a /dev/null transport that throws away the data.  It's a bit more code, though the performance should be almost the same.

Note there's also a separate bug on how gUM selects input devices I'm about to file.

Comment 4

[Anant Narayanan [:anant]]

Comment on attachment 681830 [details] [diff] [review]
disable internal socket transports

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

Looks good!

Comment 5

[Timothy B. Terriberry (:derf)]

Comment on attachment 681830 [details] [diff] [review]
disable internal socket transports

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

I think it would be better to make an external Transport object that just throws the packet away. Then we wouldn't have to patch upstream code, and we wouldn't have to worry about the other _transportPtr==NULL checks in SendRawPacket and SendRTCPPacket.

Comment 6

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

Attachment: disable internal socket transports

Comment 7

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

Comment on attachment 682156 [details] [diff] [review]
disable internal socket transports

Now with more transport goodness!

Comment 8

[Timothy B. Terriberry (:derf)]

Comment on attachment 682156 [details] [diff] [review]
disable internal socket transports

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

::: content/media/webrtc/MediaEngineWebRTCAudio.cpp
@@ +211,5 @@
>  void
>  MediaEngineWebRTCAudioSource::Shutdown()
>  {
>    if (!mInitDone) {
> +    // duplicate these here in case we failed during Init()

This is okay for the logic you are changing, but I think this code is already busted. If, e.g., the CreateChannel() call fails, we'll still have a non-NULL mVoERender, but we never call mVoERender->Release(). Same goes for mVoEBase, etc.

The fix for that probably belongs in a different bug, but I think we can do it without duplicating all the code for the !mInitDone case.

::: media/webrtc/signaling/src/common/NullTransport.h
@@ +34,5 @@
> +
> +  virtual ~NullTransport() {};
> +
> +private:
> +  NullTransport(const NullTransport& other) MOZ_DELETE;

EKR still hasn't managed to get a single macro for this into MFBT?

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/045367d6644e

Comment 10

[Eric Rescorla (:ekr)]

It's not in MFBT but check out m_cpp_utils.h in mtransport.

(In reply to Timothy B. Terriberry (:derf) from comment #8)
> Comment on attachment 682156 [details] [diff] [review]
> disable internal socket transports
> 
> Review of attachment 682156 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/webrtc/MediaEngineWebRTCAudio.cpp
> @@ +211,5 @@
> >  void
> >  MediaEngineWebRTCAudioSource::Shutdown()
> >  {
> >    if (!mInitDone) {
> > +    // duplicate these here in case we failed during Init()
> 
> This is okay for the logic you are changing, but I think this code is
> already busted. If, e.g., the CreateChannel() call fails, we'll still have a
> non-NULL mVoERender, but we never call mVoERender->Release(). Same goes for
> mVoEBase, etc.
> 
> The fix for that probably belongs in a different bug, but I think we can do
> it without duplicating all the code for the !mInitDone case.
> 
> ::: media/webrtc/signaling/src/common/NullTransport.h
> @@ +34,5 @@
> > +
> > +  virtual ~NullTransport() {};
> > +
> > +private:
> > +  NullTransport(const NullTransport& other) MOZ_DELETE;
> 
> EKR still hasn't managed to get a single macro for this into MFBT?

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/045367d6644e

Comment 12

[Jason Smith [:jsmith]]

Verified using original test case from dup.

Comment 13

[Jason Smith [:jsmith]]

If we could fake the devices, we might be able to automate this. Although as it stands in the current framework, we don't have this ability. Putting in-testsuite- for now, although if device faking becomes possible at some point, then we should re-evaluate this one for a test.