From ef1b4fb0e1133f749d5433bc9366c2a434a8c591 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Cervi=C3=B1o?= Date: Mon, 7 Jun 2021 20:30:06 +0200 Subject: [PATCH] Fix various issues related to unencrypted transport, stream priority and multiple publishers (#1719) --- erizo/src/erizo/SdpInfo.cpp | 78 ++++++------- erizo/src/erizo/WebRtcConnection.cpp | 19 ++-- .../erizo/rtp/BandwidthEstimationHandler.cpp | 12 +- erizo/src/erizo/rtp/QualityFilterHandler.cpp | 9 +- erizo/src/erizo/rtp/RtpTrackMuteHandler.cpp | 2 +- .../common/semanticSdp/SDPInfo.js | 7 +- .../erizoJS/models/SessionDescription.js | 5 +- .../erizoJS/models/WebRtcConnection.js | 2 +- .../test/erizoJS/FullRtcPeerConnection.js | 103 ++++++++++++++++-- 9 files changed, 169 insertions(+), 68 deletions(-) diff --git a/erizo/src/erizo/SdpInfo.cpp b/erizo/src/erizo/SdpInfo.cpp index 779d63de83..f1b7ce624b 100644 --- a/erizo/src/erizo/SdpInfo.cpp +++ b/erizo/src/erizo/SdpInfo.cpp @@ -143,20 +143,22 @@ namespace erizo { payloadVector = internalPayloadVector_; } - this->isBundle = bundle; - this->isRtcpMux = true; - if (videoEnabled) - this->videoSdpMLine = 0; - if (audioEnabled) - this->audioSdpMLine = 0; - - this->videoDirection = SENDRECV; - this->audioDirection = SENDRECV; + isBundle = bundle; + isRtcpMux = true; + if (videoEnabled) { + videoSdpMLine = 0; + } + if (audioEnabled) { + audioSdpMLine = 0; + } + + videoDirection = SENDRECV; + audioDirection = SENDRECV; ELOG_DEBUG("Setting Offer SDP"); } - void SdpInfo::copyInfoFromSdp(std::shared_ptr offerSdp) { - for (const auto &payload : offerSdp->payloadVector) { + void SdpInfo::copyInfoFromSdp(std::shared_ptr offer_sdp) { + for (const auto &payload : offer_sdp->payloadVector) { bool payload_exists = false; for (const auto &local_payload : payloadVector) { if (local_payload == payload) { @@ -168,7 +170,7 @@ namespace erizo { } } - for (const auto &ext_map : offerSdp->extMapVector) { + for (const auto &ext_map : offer_sdp->extMapVector) { bool ext_map_exists = false; for (const auto &local_ext_map : extMapVector) { if (local_ext_map == ext_map) { @@ -184,54 +186,53 @@ namespace erizo { extMapVector.size(), payloadVector.size()); } - void SdpInfo::setOfferSdp(std::shared_ptr offerSdp) { - this->payloadVector = offerSdp->payloadVector; - this->isBundle = offerSdp->isBundle; - this->profile = offerSdp->profile; - this->isRtcpMux = offerSdp->isRtcpMux; - this->videoSdpMLine = offerSdp->videoSdpMLine; - this->audioSdpMLine = offerSdp->audioSdpMLine; - this->inOutPTMap = offerSdp->inOutPTMap; - this->outInPTMap = offerSdp->outInPTMap; - this->bundleTags = offerSdp->bundleTags; - this->extMapVector = offerSdp->extMapVector; - this->rids_ = offerSdp->rids(); - this->google_conference_flag_set = offerSdp->google_conference_flag_set; + void SdpInfo::setOfferSdp(std::shared_ptr offer_sdp) { + isBundle = offer_sdp->isBundle; + profile = offer_sdp->profile; + isRtcpMux = offer_sdp->isRtcpMux; + videoSdpMLine = offer_sdp->videoSdpMLine; + audioSdpMLine = offer_sdp->audioSdpMLine; + inOutPTMap = offer_sdp->inOutPTMap; + outInPTMap = offer_sdp->outInPTMap; + bundleTags = offer_sdp->bundleTags; + copyInfoFromSdp(offer_sdp); + rids_ = offer_sdp->rids(); + google_conference_flag_set = offer_sdp->google_conference_flag_set; for (auto& rid : rids_) { rid.direction = reverse(rid.direction); } - switch (offerSdp->videoDirection) { + switch (offer_sdp->videoDirection) { case SENDONLY: - this->videoDirection = RECVONLY; + videoDirection = RECVONLY; break; case RECVONLY: - this->videoDirection = SENDONLY; + videoDirection = SENDONLY; break; case SENDRECV: - this->videoDirection = SENDRECV; + videoDirection = SENDRECV; break; case INACTIVE: - this->videoDirection = INACTIVE; + videoDirection = INACTIVE; break; default: - this->videoDirection = SENDRECV; + videoDirection = SENDRECV; break; } - switch (offerSdp->audioDirection) { + switch (offer_sdp->audioDirection) { case SENDONLY: - this->audioDirection = RECVONLY; + audioDirection = RECVONLY; break; case RECVONLY: - this->audioDirection = SENDONLY; + audioDirection = SENDONLY; break; case SENDRECV: - this->audioDirection = SENDRECV; + audioDirection = SENDRECV; break; case INACTIVE: - this->audioDirection = INACTIVE; + audioDirection = INACTIVE; break; default: - this->audioDirection = SENDRECV; + audioDirection = SENDRECV; break; } ELOG_DEBUG("Offer SDP successfully set"); @@ -502,8 +503,7 @@ namespace erizo { } bool operator==(const RtpMap& lhs, const RtpMap& rhs) { - return lhs.payload_type == rhs.payload_type && - lhs.encoding_name == rhs.encoding_name && + return lhs.encoding_name == rhs.encoding_name && lhs.clock_rate == rhs.clock_rate && lhs.media_type == rhs.media_type && lhs.channels == rhs.channels; diff --git a/erizo/src/erizo/WebRtcConnection.cpp b/erizo/src/erizo/WebRtcConnection.cpp index 1ad5cc3656..a4739741e8 100644 --- a/erizo/src/erizo/WebRtcConnection.cpp +++ b/erizo/src/erizo/WebRtcConnection.cpp @@ -767,14 +767,15 @@ boost::future WebRtcConnection::processRemoteSdp() { detectNewTransceiversInRemoteSdp(); + local_sdp_->setOfferSdp(remote_sdp_); + extension_processor_.setSdpInfo(local_sdp_); + local_sdp_->updateSupportedExtensionMap(extension_processor_.getSupportedExtensionMap()); + if (first_remote_sdp_processed_) { return setRemoteSdpsToMediaStreams(); } bundle_ = remote_sdp_->isBundle; - local_sdp_->setOfferSdp(remote_sdp_); - extension_processor_.setSdpInfo(local_sdp_); - local_sdp_->updateSupportedExtensionMap(extension_processor_.getSupportedExtensionMap()); if (remote_sdp_->profile == SAVPF) { ELOG_DEBUG("%s message: creating encrypted transports", toLog()); @@ -885,7 +886,6 @@ bool WebRtcConnection::addRemoteCandidateSync(std::string mid, int mLineIndex, C return false; } MediaType theType; - std::string theMid; // TODO(pedro) check if this works with video+audio and no bundle if (mLineIndex == -1) { @@ -900,10 +900,8 @@ bool WebRtcConnection::addRemoteCandidateSync(std::string mid, int mLineIndex, C if ((!mid.compare("video")) || (mLineIndex == remote_sdp_->videoSdpMLine)) { theType = VIDEO_TYPE; - theMid = "video"; } else { theType = AUDIO_TYPE; - theMid = "audio"; } bool res = false; candidate_list.push_back(candidate); @@ -1041,15 +1039,22 @@ void WebRtcConnection::read(std::shared_ptr packet) { extension_processor_.processRtpExtensions(packet); std::string mid = packet->mid; extension_processor_.removeMidAndRidExtensions(packet); - forEachMediaStream([packet, transport, ssrc, &mid] (const std::shared_ptr &media_stream) { + bool sent = false; + forEachMediaStream([packet, transport, ssrc, &mid, &sent] (const std::shared_ptr &media_stream) { if (!mid.empty()) { if (media_stream->getVideoMid() == mid || media_stream->getAudioMid() == mid) { + sent = true; media_stream->onTransportData(packet, transport); } } else if (media_stream->isSourceSSRC(ssrc) || media_stream->isSinkSSRC(ssrc)) { + sent = true; media_stream->onTransportData(packet, transport); } }); + if (!sent) { + ELOG_DEBUG("Packet does not belong to a known stream, ssrc: %u, length: %d, mid: %s, rid: %s", + ssrc, packet->length, packet->mid, packet->rid); + } } } diff --git a/erizo/src/erizo/rtp/BandwidthEstimationHandler.cpp b/erizo/src/erizo/rtp/BandwidthEstimationHandler.cpp index 43f826c023..5d561c0700 100644 --- a/erizo/src/erizo/rtp/BandwidthEstimationHandler.cpp +++ b/erizo/src/erizo/rtp/BandwidthEstimationHandler.cpp @@ -137,9 +137,17 @@ void BandwidthEstimationHandler::updateExtensionMap(bool is_video, std::arraytype != VIDEO_PACKET) { return; } - if (packet->belongsToTemporalLayer(1) || packet->belongsToSpatialLayer(1)) { + if (packet->rid != "0" || + packet->belongsToTemporalLayer(1) || + packet->belongsToSpatialLayer(1) || + packet->belongsToSpatialLayer(1)) { is_scalable_ = true; quality_manager_->enable(); } @@ -160,7 +163,7 @@ void QualityFilterHandler::write(Context *ctx, std::shared_ptr packe int picture_id = packet->picture_id; uint8_t tl0_pic_idx = packet->tl0_pic_idx; - if (last_ssrc_received_ != 0 && ssrc != last_ssrc_received_) { + if (packet->rid != "0" && !receiving_multiple_ssrc_) { receiving_multiple_ssrc_ = true; } @@ -221,7 +224,7 @@ void QualityFilterHandler::write(Context *ctx, std::shared_ptr packe updateTL0PicIdx(packet, tl0_pic_idx_sent); // removeVP8OptionalPayload(packet); // TODO(javier): uncomment this line in case of issues with pictureId - /* TODO(pedro): Disabled as part of the hack to reduce audio drift + /* TODO(pedro): Disabled as part of the hack to reduce audio drift * We will have to go back here and fix timestamp updates } else if (is_scalable_ && enabled_ && chead->isRtcp() && chead->isSenderReport()) { uint32_t ssrc = chead->getSSRC(); diff --git a/erizo/src/erizo/rtp/RtpTrackMuteHandler.cpp b/erizo/src/erizo/rtp/RtpTrackMuteHandler.cpp index 9ce846f53a..1e0eb7f852 100644 --- a/erizo/src/erizo/rtp/RtpTrackMuteHandler.cpp +++ b/erizo/src/erizo/rtp/RtpTrackMuteHandler.cpp @@ -114,7 +114,7 @@ void RtpTrackMuteHandler::handlePacket(Context *ctx, TrackMuteInfo *info, std::s uint16_t packet_seq_num = rtp_header->getSeqNumber(); maybeUpdateHighestSeqNum(info, packet_seq_num); SequenceNumber sequence_number_info = info->translator.get(packet_seq_num, should_skip_packet); - if (!should_skip_packet) { + if (!should_skip_packet && sequence_number_info.type == SequenceNumberType::Valid) { setPacketSeqNumber(packet, sequence_number_info.output); ctx->fireWrite(std::move(packet)); } diff --git a/erizo_controller/common/semanticSdp/SDPInfo.js b/erizo_controller/common/semanticSdp/SDPInfo.js index f4683b6707..6526082d57 100755 --- a/erizo_controller/common/semanticSdp/SDPInfo.js +++ b/erizo_controller/common/semanticSdp/SDPInfo.js @@ -253,6 +253,8 @@ class SDPInfo { md.rtcpMux = 'rtcp-mux'; + // TODO(javier): Enable RTCP-RSIZE: md.rtcpRsize = 'rtcp-rsize'; + md.connection = media.getConnection(); md.xGoogleFlag = media.getXGoogleFlag(); @@ -313,8 +315,7 @@ class SDPInfo { md.setup = Setup.toString(dtls.getSetup()); } - md.protocol = dtls ? 'UDP/TLS/RTP/SAVPF' : 'RTP/AVPF'; - + md.protocol = dtls.getFingerprint() ? 'UDP/TLS/RTP/SAVPF' : 'RTP/AVPF'; if (media.setup) { md.setup = Setup.toString(media.setup); } @@ -614,7 +615,7 @@ function getSimulcastDir(index, md, simulcast) { const simulcastList = md.simulcast[`list${index}`]; if (simulcastDir) { const direction = DirectionWay.byValue(simulcastDir); - const list = SDPTransform.parseSimulcastStreamList(simulcastList); + const list = SDPTransform.parseSimulcastStreamList(`${simulcastList}`); list.forEach((stream) => { const alternatives = []; stream.forEach((entry) => { diff --git a/erizo_controller/erizoJS/models/SessionDescription.js b/erizo_controller/erizoJS/models/SessionDescription.js index dea005e821..10383200dd 100644 --- a/erizo_controller/erizoJS/models/SessionDescription.js +++ b/erizo_controller/erizoJS/models/SessionDescription.js @@ -321,11 +321,12 @@ class SessionDescription { if (media.protocol === 'UDP/TLS/RTP/SAVPF') { info.setProfile('SAVPF'); - this.profole = 'SAVPF'; + this.profile = 'SAVPF'; } else { info.setProfile('AVPF'); - this.profole = 'AVPF'; + this.profile = 'AVPF'; } + info.addBundleTag(media.getId(), media.getType()); const candidates = media.getCandidates(); diff --git a/erizo_controller/erizoJS/models/WebRtcConnection.js b/erizo_controller/erizoJS/models/WebRtcConnection.js index 72dfcb92f6..8a7ad76a0e 100644 --- a/erizo_controller/erizoJS/models/WebRtcConnection.js +++ b/erizo_controller/erizoJS/models/WebRtcConnection.js @@ -426,7 +426,7 @@ class WebRtcConnection extends EventEmitter { } const mediaStream = new erizo.MediaStream(this.threadPool, this.wrtc, id, options.label, isPublisher, - options.audio, options.video, options.priority, handlers); + options.audio, options.video, handlers, options.priority); mediaStream.id = id; mediaStream.label = options.label; mediaStream.isPublisher = isPublisher; diff --git a/erizo_controller/test/erizoJS/FullRtcPeerConnection.js b/erizo_controller/test/erizoJS/FullRtcPeerConnection.js index 9f0526f8a4..b594b4fe68 100644 --- a/erizo_controller/test/erizoJS/FullRtcPeerConnection.js +++ b/erizo_controller/test/erizoJS/FullRtcPeerConnection.js @@ -49,6 +49,17 @@ const timeout = utils.promisify(setTimeout); const promisifyOnce = (connection, event) => new Promise(resolve => connection.once(event, resolve)); + +const expectMediaWith = (media, { id, port, type, direction, setup }) => { + id && expect(media.id).to.be.equals(id); + port && expect(media.port).to.be.equals(port); + type && expect(media.type).to.be.equals(type); + direction && expect(media.getDirectionString()).to.be.equals(direction); + setup && expect(Setup.toString(media.getSetup())).to.be.equals(setup); + expect(media.extensions.size).to.be.greaterThan(0); + expect(media.codecs.size).to.be.greaterThan(0); +}; + describeTest('RTCPeerConnection with WebRtcConnection', () => { let connection; let erizo; @@ -57,6 +68,7 @@ describeTest('RTCPeerConnection with WebRtcConnection', () => { let threadPool; let ioThreadPool; let webRtcConnectionConfiguration; + let SessionDescription; beforeEach('Mock Process', () => { this.originalExit = process.exit; @@ -91,6 +103,8 @@ describeTest('RTCPeerConnection with WebRtcConnection', () => { erizo = require('./../../../erizoAPI/build/Release/addonDebug'); // eslint-disable-next-line global-require RTCPeerConnection = require('./../../erizoJS/models/RTCPeerConnection'); + // eslint-disable-next-line global-require + SessionDescription = require('./../../erizoJS/models/SessionDescription'); threadPool = new erizo.ThreadPool(5); threadPool.start(); @@ -610,6 +624,67 @@ describeTest('RTCPeerConnection with WebRtcConnection', () => { expect(connection.negotiationNeeded).to.be.false; }); + it('should negotiate two publishers (Audio+Video after Video Only)', async () => { + const config = Object.assign({}, webRtcConnectionConfiguration); + connection = new RTCPeerConnection(config); + connection.init(); + let label = 'stream1'; + + await connection.onInitialized; + + await connection.addStream('stream1', { label, audio: false, video: true }, true); + + expect(connection.negotiationNeeded).to.be.false; + + await connection.setRemoteDescription({ + type: 'offer', + sdp: sdpUtils.getChromePublisherSdp([ + { mid: 0, label, kind: 'video', direction: 'sendrecv', setup: 'actpass' }, + ]), + }); + + expect(connection.signalingState).to.be.equals('have-remote-offer'); + await connection.setLocalDescription(); + let answer = connection.localDescription; + let sdpInfo = SemanticSdp.SDPInfo.processString(answer); + + expect(sdpInfo.medias.length).to.be.equals(1); + + (sdpInfo.medias[0], { id: 0, port: 9, type: 'video', direction: 'recvonly', setup: 'active' }); + + expect(connection.signalingState).to.be.equals('stable'); + + expect(connection.negotiationNeeded).to.be.false; + + label = 'stream2'; + + await connection.addStream('stream2', { label, audio: true, video: true }, true); + + expect(connection.negotiationNeeded).to.be.false; + expect(connection.signalingState).to.be.equals('stable'); + await connection.setRemoteDescription({ + type: 'offer', + sdp: sdpUtils.getChromePublisherSdp([ + { mid: 0, label: 'stream1', kind: 'video', direction: 'sendrecv', setup: 'active' }, + { mid: 1, label, kind: 'audio', direction: 'sendrecv', setup: 'actpass' }, + { mid: 2, label, kind: 'video', direction: 'sendrecv', setup: 'actpass' }, + ]), + }); + + expect(connection.signalingState).to.be.equals('have-remote-offer'); + + await connection.setLocalDescription(); + answer = connection.localDescription; + sdpInfo = SemanticSdp.SDPInfo.processString(answer); + expect(sdpInfo.medias.length).to.be.equals(3); + expectMediaWith(sdpInfo.medias[0], { id: 0, port: 9, type: 'video', direction: 'recvonly', setup: 'active' }); + expectMediaWith(sdpInfo.medias[1], { id: 1, port: 9, type: 'audio', direction: 'recvonly', setup: 'active' }); + expectMediaWith(sdpInfo.medias[2], { id: 2, port: 9, type: 'video', direction: 'recvonly', setup: 'active' }); + + expect(connection.signalingState).to.be.equals('stable'); + expect(connection.negotiationNeeded).to.be.false; + }); + it('should negotiate audio only reusing senders', async () => { const config = Object.assign({ }, webRtcConnectionConfiguration); connection = new RTCPeerConnection(config); @@ -693,14 +768,6 @@ describeTest('RTCPeerConnection with WebRtcConnection', () => { expect(connection.negotiationNeeded).to.be.false; }); - const expectMediaWith = (media, { id, port, type, direction, setup }) => { - id && expect(media.id).to.be.equals(id); - port && expect(media.port).to.be.equals(port); - type && expect(media.type).to.be.equals(type); - direction && expect(media.getDirectionString()).to.be.equals(direction); - setup && expect(Setup.toString(media.getSetup())).to.be.equals(setup); - }; - it('should negotiate video only reusing senders', async () => { const config = Object.assign({ }, webRtcConnectionConfiguration); connection = new RTCPeerConnection(config); @@ -786,6 +853,11 @@ describeTest('RTCPeerConnection with WebRtcConnection', () => { it('should negotiate AV reusing senders', async () => { const config = Object.assign({}, webRtcConnectionConfiguration); + const publisherSdpInfo = SemanticSdp.SDPInfo.processString(sdpUtils.getChromePublisherSdp([ + { mid: 0, label: '1', kind: 'audio', direction: 'recvonly', setup: 'active' }, + { mid: 1, label: '1', kind: 'video', direction: 'recvonly', setup: 'active' }, + ])); + const publisherDescription = new SessionDescription(publisherSdpInfo, 'default'); connection = new RTCPeerConnection(config); connection.init(); const label = 'stream10'; @@ -794,6 +866,8 @@ describeTest('RTCPeerConnection with WebRtcConnection', () => { let onNegotiationNeededPromise = promisifyOnce(connection, 'negotiationneeded'); await connection.addStream('1', { label, audio: true, video: true }, false); + connection.internalConnection.wrtc + .copySdpToLocalDescription(publisherDescription.connectionDescription); await expect(onNegotiationNeededPromise).to.be.eventually.fulfilled; expect(connection.negotiationNeeded).to.be.true; @@ -839,7 +913,8 @@ describeTest('RTCPeerConnection with WebRtcConnection', () => { onNegotiationNeededPromise = promisifyOnce(connection, 'negotiationneeded'); await connection.addStream('2', { label, audio: true, video: true }, false); - + connection.internalConnection.wrtc + .copySdpToLocalDescription(publisherDescription.connectionDescription); await expect(onNegotiationNeededPromise).to.be.eventually.fulfilled; expect(connection.negotiationNeeded).to.be.true; @@ -847,7 +922,6 @@ describeTest('RTCPeerConnection with WebRtcConnection', () => { await connection.setLocalDescription(); offer = connection.localDescription; sdpInfo = SemanticSdp.SDPInfo.processString(offer); - expect(sdpInfo.medias.length).to.be.equals(3); expectMediaWith(sdpInfo.medias[0], { id: 0, port: 9, type: 'audio', direction: 'inactive', setup: 'passive' }); expectMediaWith(sdpInfo.medias[1], { id: 2, port: 9, type: 'audio', direction: 'sendrecv', setup: 'actpass' }); @@ -1170,6 +1244,11 @@ describeTest('RTCPeerConnection with WebRtcConnection', () => { it('should negotiate AV with multiple receiving streams', async () => { const kNumberOfSubscribers = 10; const config = Object.assign({}, webRtcConnectionConfiguration); + const publisherSdpInfo = SemanticSdp.SDPInfo.processString(sdpUtils.getChromePublisherSdp([ + { mid: 0, label: '1', kind: 'audio', direction: 'recvonly', setup: 'active' }, + { mid: 1, label: '1', kind: 'video', direction: 'recvonly', setup: 'active' }, + ])); + const publisherDescription = new SessionDescription(publisherSdpInfo, 'default'); connection = new RTCPeerConnection(config); const originalCreateOffer = connection.internalConnection.wrtc .createOffer.bind(connection.internalConnection.wrtc); @@ -1196,6 +1275,8 @@ describeTest('RTCPeerConnection with WebRtcConnection', () => { let onNegotiationNeededPromise = promisifyOnce(connection, 'negotiationneeded'); await connection.addStream('1', { label, audio: true, video: true }, false); + connection.internalConnection.wrtc + .copySdpToLocalDescription(publisherDescription.connectionDescription); await expect(onNegotiationNeededPromise).to.be.eventually.fulfilled; expect(connection.negotiationNeeded).to.be.true; @@ -1209,6 +1290,8 @@ describeTest('RTCPeerConnection with WebRtcConnection', () => { setTimeout(() => { connection.addStream(index, { label: label2, audio: true, video: true }, false) .then(() => { + connection.internalConnection.wrtc + .copySdpToLocalDescription(publisherDescription.connectionDescription); resolve(); }); }, 30);