Closed Bug 811695 Opened 7 years ago Closed 7 years ago

Stop using local socket transport to loopback captured audio

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla19

People

(Reporter: jesup, Assigned: jesup)

References

Details

(Whiteboard: [getUserMedia], [blocking-gum+])

Attachments

(1 file, 1 obsolete file)

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 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.
Attachment #681830 - Flags: review?(tterribe)
Attachment #681830 - Flags: review?(anant)
Duplicate of this bug: 811011
Comment on attachment 681830 [details] [diff] [review]
disable internal socket transports

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

Looks good!
Attachment #681830 - Flags: review?(anant) → review+
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.
Attachment #681830 - Flags: review?(tterribe) → review-
Attachment #681830 - Attachment is obsolete: true
Comment on attachment 682156 [details] [diff] [review]
disable internal socket transports

Now with more transport goodness!
Attachment #682156 - Flags: review?(tterribe)
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?
Attachment #682156 - Flags: review?(tterribe) → review+
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?
https://hg.mozilla.org/mozilla-central/rev/045367d6644e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Keywords: verifyme
Verified using original test case from dup.
Status: RESOLVED → VERIFIED
Keywords: verifyme
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.
Flags: in-testsuite-
Depends on: 815002
You need to log in before you can comment on or make changes to this bug.