From 3bc15103aea1dd2fdfc01b833b62f34171c82e81 Mon Sep 17 00:00:00 2001 From: deadbeef Date: Thu, 20 Apr 2017 19:25:07 -0700 Subject: [PATCH] Fix RtpReceiver.GetParameters when SSRCs aren't signaled. When SSRCs aren't signaled, an SSRC of 0 is used internally to mean "the default receive stream". But this wasn't working with the implementation of GetRtpReceiveParameters in the audio/video engines. This was breaking RtpReceiver.GetParameters in this situation, as well as the new getStats implementation (which relies on GetParameters). The new implementation will fail if *no* default receive stream is configured (meaning no default sink is set), and otherwise will return a default RtpEncodingParameters (later it will be filled with relevant SDP parameters as they're implemented). BUG=webrtc:6971 Review-Url: https://codereview.webrtc.org/2806173002 Cr-Commit-Position: refs/heads/master@{#17803} --- webrtc/media/base/mediachannel.h | 12 ++++- webrtc/media/engine/webrtcvideoengine2.cc | 50 ++++++++++++++----- webrtc/media/engine/webrtcvideoengine2.h | 3 ++ .../engine/webrtcvideoengine2_unittest.cc | 43 ++++++++++++++++ webrtc/media/engine/webrtcvoiceengine.cc | 49 +++++++++++++----- .../engine/webrtcvoiceengine_unittest.cc | 37 ++++++++++++++ 6 files changed, 167 insertions(+), 27 deletions(-) diff --git a/webrtc/media/base/mediachannel.h b/webrtc/media/base/mediachannel.h index ca02c72fcd..6f14b7aa2e 100644 --- a/webrtc/media/base/mediachannel.h +++ b/webrtc/media/base/mediachannel.h @@ -973,6 +973,11 @@ class VoiceMediaChannel : public MediaChannel { virtual bool SetRtpSendParameters( uint32_t ssrc, const webrtc::RtpParameters& parameters) = 0; + // Get the receive parameters for the incoming stream identified by |ssrc|. + // If |ssrc| is 0, retrieve the receive parameters for the default receive + // stream, which is used when SSRCs are not signaled. Note that calling with + // an |ssrc| of 0 will return encoding parameters with an unset |ssrc| + // member. virtual webrtc::RtpParameters GetRtpReceiveParameters( uint32_t ssrc) const = 0; virtual bool SetRtpReceiveParameters( @@ -1053,6 +1058,11 @@ class VideoMediaChannel : public MediaChannel { virtual bool SetRtpSendParameters( uint32_t ssrc, const webrtc::RtpParameters& parameters) = 0; + // Get the receive parameters for the incoming stream identified by |ssrc|. + // If |ssrc| is 0, retrieve the receive parameters for the default receive + // stream, which is used when SSRCs are not signaled. Note that calling with + // an |ssrc| of 0 will return encoding parameters with an unset |ssrc| + // member. virtual webrtc::RtpParameters GetRtpReceiveParameters( uint32_t ssrc) const = 0; virtual bool SetRtpReceiveParameters( @@ -1070,7 +1080,7 @@ class VideoMediaChannel : public MediaChannel { const VideoOptions* options, rtc::VideoSourceInterface* source) = 0; // Sets the sink object to be used for the specified stream. - // If SSRC is 0, the renderer is used for the 'default' stream. + // If SSRC is 0, the sink is used for the 'default' stream. virtual bool SetSink(uint32_t ssrc, rtc::VideoSinkInterface* sink) = 0; // Gets quality stats for the channel. diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc index 087fc4e011..e92598d408 100644 --- a/webrtc/media/engine/webrtcvideoengine2.cc +++ b/webrtc/media/engine/webrtcvideoengine2.cc @@ -865,20 +865,33 @@ bool WebRtcVideoChannel2::SetRtpSendParameters( webrtc::RtpParameters WebRtcVideoChannel2::GetRtpReceiveParameters( uint32_t ssrc) const { + webrtc::RtpParameters rtp_params; rtc::CritScope stream_lock(&stream_crit_); - auto it = receive_streams_.find(ssrc); - if (it == receive_streams_.end()) { - LOG(LS_WARNING) << "Attempting to get RTP receive parameters for stream " - << "with ssrc " << ssrc << " which doesn't exist."; - return webrtc::RtpParameters(); + // SSRC of 0 represents an unsignaled receive stream. + if (ssrc == 0) { + if (!default_unsignalled_ssrc_handler_.GetDefaultSink()) { + LOG(LS_WARNING) << "Attempting to get RTP parameters for the default, " + "unsignaled video receive stream, but not yet " + "configured to receive such a stream."; + return rtp_params; + } + rtp_params.encodings.emplace_back(); + } else { + auto it = receive_streams_.find(ssrc); + if (it == receive_streams_.end()) { + LOG(LS_WARNING) << "Attempting to get RTP receive parameters for stream " + << "with SSRC " << ssrc << " which doesn't exist."; + return webrtc::RtpParameters(); + } + // TODO(deadbeef): Return stream-specific parameters, beyond just SSRC. + rtp_params.encodings.emplace_back(); + rtp_params.encodings[0].ssrc = it->second->GetFirstPrimarySsrc(); } - // TODO(deadbeef): Return stream-specific parameters. - webrtc::RtpParameters rtp_params = CreateRtpParametersWithOneEncoding(); + // Add codecs, which any stream is prepared to receive. for (const VideoCodec& codec : recv_params_.codecs) { rtp_params.codecs.push_back(codec.ToCodecParameters()); } - rtp_params.encodings[0].ssrc = it->second->GetFirstPrimarySsrc(); return rtp_params; } @@ -887,11 +900,22 @@ bool WebRtcVideoChannel2::SetRtpReceiveParameters( const webrtc::RtpParameters& parameters) { TRACE_EVENT0("webrtc", "WebRtcVideoChannel2::SetRtpReceiveParameters"); rtc::CritScope stream_lock(&stream_crit_); - auto it = receive_streams_.find(ssrc); - if (it == receive_streams_.end()) { - LOG(LS_ERROR) << "Attempting to set RTP receive parameters for stream " - << "with ssrc " << ssrc << " which doesn't exist."; - return false; + + // SSRC of 0 represents an unsignaled receive stream. + if (ssrc == 0) { + if (!default_unsignalled_ssrc_handler_.GetDefaultSink()) { + LOG(LS_WARNING) << "Attempting to set RTP parameters for the default, " + "unsignaled video receive stream, but not yet " + "configured to receive such a stream."; + return false; + } + } else { + auto it = receive_streams_.find(ssrc); + if (it == receive_streams_.end()) { + LOG(LS_WARNING) << "Attempting to set RTP receive parameters for stream " + << "with SSRC " << ssrc << " which doesn't exist."; + return false; + } } webrtc::RtpParameters current_parameters = GetRtpReceiveParameters(ssrc); diff --git a/webrtc/media/engine/webrtcvideoengine2.h b/webrtc/media/engine/webrtcvideoengine2.h index 0a2f4a5d86..2c7d36ccb5 100644 --- a/webrtc/media/engine/webrtcvideoengine2.h +++ b/webrtc/media/engine/webrtcvideoengine2.h @@ -83,6 +83,9 @@ class DefaultUnsignalledSsrcHandler : public UnsignalledSsrcHandler { rtc::VideoSinkInterface* GetDefaultSink() const; void SetDefaultSink(VideoMediaChannel* channel, rtc::VideoSinkInterface* sink); + + uint32_t default_recv_ssrc() const { return default_recv_ssrc_; } + virtual ~DefaultUnsignalledSsrcHandler() = default; private: diff --git a/webrtc/media/engine/webrtcvideoengine2_unittest.cc b/webrtc/media/engine/webrtcvideoengine2_unittest.cc index 37e227f931..40d1a3f5ab 100644 --- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc +++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc @@ -4136,6 +4136,49 @@ TEST_F(WebRtcVideoChannel2Test, SetAndGetRtpReceiveParameters) { EXPECT_EQ(initial_params, channel_->GetRtpReceiveParameters(last_ssrc_)); } +// Test that GetRtpReceiveParameters returns parameters correctly when SSRCs +// aren't signaled. It should always return an empty "RtpEncodingParameters", +// even after a packet is received and the unsignaled SSRC is known. +TEST_F(WebRtcVideoChannel2Test, GetRtpReceiveParametersWithUnsignaledSsrc) { + // Call necessary methods to configure receiving a default stream as + // soon as it arrives. + cricket::VideoRecvParameters parameters; + parameters.codecs.push_back(GetEngineCodec("VP8")); + parameters.codecs.push_back(GetEngineCodec("VP9")); + EXPECT_TRUE(channel_->SetRecvParameters(parameters)); + + // Call GetRtpReceiveParameters before configured to receive an unsignaled + // stream. Should return nothing. + EXPECT_EQ(webrtc::RtpParameters(), channel_->GetRtpReceiveParameters(0)); + + // Set a sink for an unsignaled stream. + cricket::FakeVideoRenderer renderer; + // Value of "0" means "unsignaled stream". + EXPECT_TRUE(channel_->SetSink(0, &renderer)); + + // Call GetRtpReceiveParameters before the SSRC is known. Value of "0" + // in this method means "unsignaled stream". + webrtc::RtpParameters rtp_parameters = channel_->GetRtpReceiveParameters(0); + ASSERT_EQ(1u, rtp_parameters.encodings.size()); + EXPECT_FALSE(rtp_parameters.encodings[0].ssrc); + + // Receive VP8 packet. + uint8_t data[kMinRtpPacketLen]; + cricket::RtpHeader rtpHeader; + rtpHeader.payload_type = GetEngineCodec("VP8").id; + rtpHeader.seq_num = rtpHeader.timestamp = 0; + rtpHeader.ssrc = kIncomingUnsignalledSsrc; + cricket::SetRtpHeader(data, sizeof(data), rtpHeader); + rtc::CopyOnWriteBuffer packet(data, sizeof(data)); + rtc::PacketTime packet_time; + channel_->OnPacketReceived(&packet, packet_time); + + // The |ssrc| member should still be unset. + rtp_parameters = channel_->GetRtpReceiveParameters(0); + ASSERT_EQ(1u, rtp_parameters.encodings.size()); + EXPECT_FALSE(rtp_parameters.encodings[0].ssrc); +} + void WebRtcVideoChannel2Test::TestReceiverLocalSsrcConfiguration( bool receiver_first) { EXPECT_TRUE(channel_->SetSendParameters(send_parameters_)); diff --git a/webrtc/media/engine/webrtcvoiceengine.cc b/webrtc/media/engine/webrtcvoiceengine.cc index 4aa97e049e..2384ac28f1 100644 --- a/webrtc/media/engine/webrtcvoiceengine.cc +++ b/webrtc/media/engine/webrtcvoiceengine.cc @@ -1755,19 +1755,31 @@ bool WebRtcVoiceMediaChannel::SetRtpSendParameters( webrtc::RtpParameters WebRtcVoiceMediaChannel::GetRtpReceiveParameters( uint32_t ssrc) const { RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); - auto it = recv_streams_.find(ssrc); - if (it == recv_streams_.end()) { - LOG(LS_WARNING) << "Attempting to get RTP receive parameters for stream " - << "with ssrc " << ssrc << " which doesn't exist."; - return webrtc::RtpParameters(); + webrtc::RtpParameters rtp_params; + // SSRC of 0 represents the default receive stream. + if (ssrc == 0) { + if (!default_sink_) { + LOG(LS_WARNING) << "Attempting to get RTP parameters for the default, " + "unsignaled audio receive stream, but not yet " + "configured to receive such a stream."; + return rtp_params; + } + rtp_params.encodings.emplace_back(); + } else { + auto it = recv_streams_.find(ssrc); + if (it == recv_streams_.end()) { + LOG(LS_WARNING) << "Attempting to get RTP receive parameters for stream " + << "with ssrc " << ssrc << " which doesn't exist."; + return webrtc::RtpParameters(); + } + rtp_params.encodings.emplace_back(); + // TODO(deadbeef): Return stream-specific parameters. + rtp_params.encodings[0].ssrc = rtc::Optional(ssrc); } - // TODO(deadbeef): Return stream-specific parameters. - webrtc::RtpParameters rtp_params = CreateRtpParametersWithOneEncoding(); for (const AudioCodec& codec : recv_codecs_) { rtp_params.codecs.push_back(codec.ToCodecParameters()); } - rtp_params.encodings[0].ssrc = rtc::Optional(ssrc); return rtp_params; } @@ -1775,11 +1787,21 @@ bool WebRtcVoiceMediaChannel::SetRtpReceiveParameters( uint32_t ssrc, const webrtc::RtpParameters& parameters) { RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); - auto it = recv_streams_.find(ssrc); - if (it == recv_streams_.end()) { - LOG(LS_WARNING) << "Attempting to set RTP receive parameters for stream " - << "with ssrc " << ssrc << " which doesn't exist."; - return false; + // SSRC of 0 represents the default receive stream. + if (ssrc == 0) { + if (!default_sink_) { + LOG(LS_WARNING) << "Attempting to set RTP parameters for the default, " + "unsignaled audio receive stream, but not yet " + "configured to receive such a stream."; + return false; + } + } else { + auto it = recv_streams_.find(ssrc); + if (it == recv_streams_.end()) { + LOG(LS_WARNING) << "Attempting to set RTP receive parameters for stream " + << "with ssrc " << ssrc << " which doesn't exist."; + return false; + } } webrtc::RtpParameters current_parameters = GetRtpReceiveParameters(ssrc); @@ -2320,6 +2342,7 @@ int WebRtcVoiceMediaChannel::GetOutputLevel() { bool WebRtcVoiceMediaChannel::SetOutputVolume(uint32_t ssrc, double volume) { RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); std::vector ssrcs(1, ssrc); + // SSRC of 0 represents the default receive stream. if (ssrc == 0) { default_recv_volume_ = volume; ssrcs = unsignaled_recv_ssrcs_; diff --git a/webrtc/media/engine/webrtcvoiceengine_unittest.cc b/webrtc/media/engine/webrtcvoiceengine_unittest.cc index f9a631c539..75b6320166 100644 --- a/webrtc/media/engine/webrtcvoiceengine_unittest.cc +++ b/webrtc/media/engine/webrtcvoiceengine_unittest.cc @@ -1238,6 +1238,43 @@ TEST_F(WebRtcVoiceEngineTestFake, SetAndGetRtpReceiveParameters) { EXPECT_EQ(initial_params, channel_->GetRtpReceiveParameters(kSsrcX)); } +// Test that GetRtpReceiveParameters returns parameters correctly when SSRCs +// aren't signaled. It should return an empty "RtpEncodingParameters" when +// configured to receive an unsignaled stream and no packets have been received +// yet, and start returning the SSRC once a packet has been received. +TEST_F(WebRtcVoiceEngineTestFake, GetRtpReceiveParametersWithUnsignaledSsrc) { + ASSERT_TRUE(SetupChannel()); + // Call necessary methods to configure receiving a default stream as + // soon as it arrives. + cricket::AudioRecvParameters parameters; + parameters.codecs.push_back(kIsacCodec); + parameters.codecs.push_back(kPcmuCodec); + EXPECT_TRUE(channel_->SetRecvParameters(parameters)); + + // Call GetRtpReceiveParameters before configured to receive an unsignaled + // stream. Should return nothing. + EXPECT_EQ(webrtc::RtpParameters(), channel_->GetRtpReceiveParameters(0)); + + // Set a sink for an unsignaled stream. + std::unique_ptr fake_sink(new FakeAudioSink()); + // Value of "0" means "unsignaled stream". + channel_->SetRawAudioSink(0, std::move(fake_sink)); + + // Call GetRtpReceiveParameters before the SSRC is known. Value of "0" + // in this method means "unsignaled stream". + webrtc::RtpParameters rtp_parameters = channel_->GetRtpReceiveParameters(0); + ASSERT_EQ(1u, rtp_parameters.encodings.size()); + EXPECT_FALSE(rtp_parameters.encodings[0].ssrc); + + // Receive PCMU packet (SSRC=1). + DeliverPacket(kPcmuFrame, sizeof(kPcmuFrame)); + + // The |ssrc| member should still be unset. + rtp_parameters = channel_->GetRtpReceiveParameters(0); + ASSERT_EQ(1u, rtp_parameters.encodings.size()); + EXPECT_FALSE(rtp_parameters.encodings[0].ssrc); +} + // Test that we apply codecs properly. TEST_F(WebRtcVoiceEngineTestFake, SetSendCodecs) { EXPECT_TRUE(SetupSendStream());