simplewebrtc / SimpleWebRTC

Simplest WebRTC ever
Other
4.65k stars 1.19k forks source link

Audio issues when localVideo config properties aren't set explicitly. #214

Open jlengstorf opened 9 years ago

jlengstorf commented 9 years ago

When starting video at a later time with webrtc.startLocalVideo(), I'm seeing an issue where the local video doesn't mute. It also doesn't respond to webrtc.mute().

I'm working on a modified copy of SimpleWebRTC, so there is a possibility that I've introduced this bug, but I haven't touched any of the actual RTC code.

The way I've set up the app is:

  1. The admin loads the app and immediately shares video, which goes to anyone who's connected to the room (this works without issue)
  2. The admin can then request video from anyone in the room, which calls webrtc.startLocalVideo() on their browser

The video loads fine, and from the admin side there is no issue.

However, on the user side, the video is not muted, causing echo and feedback. webrtc.config.localVideo.muted is true.

In addition, calling webrtc.mute() on the user's computer has no effect. Adding the muted attribute to the video element also doesn't fix the issue. The only way to get the audio to stop — as far as I can tell, at least — is to actually delete the video element from the DOM.

I'm not sure if any of the below is helpful, but this is what the admin's browser received for setRemoteDescription according to chrome://webrtc-internals/:

type: offer, sdp: v=0
o=- 996824012314804915 2 IN IP4 127.0.0.1
s=-
t=0 0
a=group:BUNDLE audio video data
a=msid-semantic: WMS
m=audio 9 RTP/SAVPF 111 103 104 9 0 8 106 105 13 126
c=IN IP4 0.0.0.0
a=rtcp:9 IN IP4 0.0.0.0
a=ice-ufrag:d3QW36YPPz0i/lbT
a=ice-pwd:8TJZZT4cbA9vC+hTGGoiaAKT
a=ice-options:google-ice
a=fingerprint:sha-256 0A:A2:2C:D9:46:E3:D5:73:E3:0C:CC:1D:04:F5:26:9A:94:08:82:16:77:06:14:58:3C:E4:42:59:9B:9D:9F:A8
a=setup:actpass
a=mid:audio
a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level
a=extmap:3 http://www.webrtc.org/experiments/rtp-hdrext/abs-send-time
a=recvonly
a=rtcp-mux
a=rtpmap:111 opus/48000/2
a=fmtp:111 minptime=10
a=rtpmap:103 ISAC/16000
a=rtpmap:104 ISAC/32000
a=rtpmap:9 G722/8000
a=rtpmap:0 PCMU/8000
a=rtpmap:8 PCMA/8000
a=rtpmap:106 CN/32000
a=rtpmap:105 CN/16000
a=rtpmap:13 CN/8000
a=rtpmap:126 telephone-event/8000
a=maxptime:60
m=video 9 RTP/SAVPF 100 116 117 96
c=IN IP4 0.0.0.0
a=rtcp:9 IN IP4 0.0.0.0
a=ice-ufrag:d3QW36YPPz0i/lbT
a=ice-pwd:8TJZZT4cbA9vC+hTGGoiaAKT
a=ice-options:google-ice
a=fingerprint:sha-256 0A:A2:2C:D9:46:E3:D5:73:E3:0C:CC:1D:04:F5:26:9A:94:08:82:16:77:06:14:58:3C:E4:42:59:9B:9D:9F:A8
a=setup:actpass
a=mid:video
a=extmap:2 urn:ietf:params:rtp-hdrext:toffset
a=extmap:3 http://www.webrtc.org/experiments/rtp-hdrext/abs-send-time
a=recvonly
a=rtcp-mux
a=rtpmap:100 VP8/90000
a=rtcp-fb:100 ccm fir
a=rtcp-fb:100 nack
a=rtcp-fb:100 nack pli
a=rtcp-fb:100 goog-remb
a=rtpmap:116 red/90000
a=rtpmap:117 ulpfec/90000
a=rtpmap:96 rtx/90000
a=fmtp:96 apt=100
m=application 9 DTLS/SCTP 5000
c=IN IP4 0.0.0.0
a=ice-ufrag:d3QW36YPPz0i/lbT
a=ice-pwd:8TJZZT4cbA9vC+hTGGoiaAKT
a=ice-options:google-ice
a=fingerprint:sha-256 0A:A2:2C:D9:46:E3:D5:73:E3:0C:CC:1D:04:F5:26:9A:94:08:82:16:77:06:14:58:3C:E4:42:59:9B:9D:9F:A8
a=setup:actpass
a=mid:data
a=sctpmap:5000 webrtc-datachannel 1024

Please let me know if I can add any additional info here to help. I'm not familiar enough with WebRTC to get too far into this, so any help is greatly appreciated.

Thanks!

fippo commented 9 years ago

This sounds odd... the local video should never be automatically attached to anything but the element designated by localVideoEl. How are you creating the connection back to the admin? The normal way would be to add it to the existing connection but since renegotiation is not supported yet...

jlengstorf commented 9 years ago

Sorry, it is attached to the element designated by localVideoEl — I modified those functions to select different elements based on context, but it's still using the correct elements. That all behaves normally.

The weird part is that audio controls are completely dead for some reason. (Especially the fact that I can't directly target the video element with vanilla JS using the muted element or volume settings.)

In case it helps, here's how I modified getLocalVideoContainer():

SimpleWebRTC.prototype.getLocalVideoContainer = function () {
    var cfg = this.config;

    // Configures which element should hold the local video
    var whichEl = cfg.videoElements.local;

    // Admin needs to feed local into the admin element
    if (cfg.isAdmin) {
        whichEl = cfg.videoElements.admin;
    }

    // The attendee selected for solo needs local fed into that element
    if (cfg.isSoloAttendee) {
        whichEl = cfg.videoElements.solo;
    }

    this.logger.info('isAdmin: ', cfg.isAdmin);
    this.logger.info('Local Video Container: ', whichEl);

    var el = this.getEl(whichEl);
    if (el && el.tagName === 'VIDEO') {
        el.oncontextmenu = function () { return false; };
        return el;
    } else if (el) {
        var video = document.createElement('video');
        video.oncontextmenu = function () { return false; };
        el.appendChild(video);
        return video;
    } else {
        return;
    }
};
jlengstorf commented 9 years ago

After posting the last comment, I realized I wasn't dumping the config.localVideo options to check them.

I was passing mirror: false and leaving everything else to defaults. I just explicitly added the other settings:

        localVideo: {
            autoplay: true,
            mirror: false, // We do this manually because otherwise it messes with styles
            muted: true
        }

And now it behaves as expected, including responding to the mute() and unmute() commands.

So it appears that the issue is with passing config to attachMediaStream() and, somehow, causing the audio control methods to fail when all config settings aren't explicitly set.

Am I wrong in assuming that these have default settings? Or am I breaking it by overriding the config object with just one property?

fippo commented 9 years ago

that sounds really odd... attachMediaStream should use it's own defaults that just override in https://github.com/HenrikJoreteg/attachMediaStream/blob/master/attachmediastream.js#L12-16

The only thing I can imagine that goes wrong is attachMediaStream creating a new element and then the rest of the things being confused about which element to mute.

jlengstorf commented 9 years ago

It does sound odd, but explicitly setting those config options removed the issue — that wouldn't do anything to tell the app which element should be muted that wasn't already being done, I imagine.

Looking at the code, though, it doesn't seem possible to leave defaults blank, so... magic?