From 1cfbd6003b91dca69f485f17f03ad44212f4218a Mon Sep 17 00:00:00 2001 From: brandtr Date: Thu, 8 Dec 2016 04:17:53 -0800 Subject: [PATCH] Generalize FlexfecReceiveStream::Config. - Adding information about RTCP and RTP header extensions. - Renaming flexfec_payload_type -> payload_type and flexfec_ssrc -> remote_ssrc. BUG=webrtc:5654 R=stefan@webrtc.org, philipel@webrtc.org Review-Url: https://codereview.webrtc.org/2542413002 Cr-Commit-Position: refs/heads/master@{#15477} --- webrtc/api/call/flexfec_receive_stream.h | 39 +++++++++++++++++-- webrtc/call/call.cc | 4 +- webrtc/call/call_unittest.cc | 18 ++++----- webrtc/call/flexfec_receive_stream.cc | 32 ++++++++++++--- .../call/flexfec_receive_stream_unittest.cc | 16 ++++---- webrtc/media/engine/webrtcvideoengine2.cc | 11 +++++- .../engine/webrtcvideoengine2_unittest.cc | 13 +++---- webrtc/test/call_test.cc | 11 +++--- webrtc/video/end_to_end_tests.cc | 9 ++++- webrtc/video/video_quality_test.cc | 4 +- 10 files changed, 112 insertions(+), 45 deletions(-) diff --git a/webrtc/api/call/flexfec_receive_stream.h b/webrtc/api/call/flexfec_receive_stream.h index 5918f7730f..64b1d5903f 100644 --- a/webrtc/api/call/flexfec_receive_stream.h +++ b/webrtc/api/call/flexfec_receive_stream.h @@ -12,7 +12,9 @@ #define WEBRTC_API_CALL_FLEXFEC_RECEIVE_STREAM_H_ #include +#include +#include "webrtc/api/call/transport.h" #include "webrtc/config.h" namespace webrtc { @@ -32,10 +34,39 @@ class FlexfecReceiveStream { int flexfec_bitrate_bps; }; - // TODO(brandtr): When we add multistream protection, and thus add a - // FlexfecSendStream class, remove FlexfecConfig from config.h and add - // the appropriate configs here and in FlexfecSendStream. - using Config = FlexfecConfig; + struct Config { + std::string ToString() const; + + // Payload type for FlexFEC. + int payload_type = -1; + + // SSRC for FlexFEC stream to be received. + uint32_t remote_ssrc = 0; + + // Vector containing a single element, corresponding to the SSRC of the + // media stream being protected by this FlexFEC stream. The vector MUST have + // size 1. + // + // TODO(brandtr): Update comment above when we support multistream + // protection. + std::vector protected_media_ssrcs; + + // SSRC for RTCP reports to be sent. + uint32_t local_ssrc = 0; + + // What RTCP mode to use in the reports. + RtcpMode rtcp_mode = RtcpMode::kCompound; + + // Transport for outgoing RTCP packets. + Transport* rtcp_send_transport = nullptr; + + // |transport_cc| is true whenever the send-side BWE RTCP feedback message + // has been negotiated. This is a prerequisite for enabling send-side BWE. + bool transport_cc = false; + + // RTP header extensions that have been negotiated for this track. + std::vector extensions; + }; // Starts stream activity. // When a stream is active, it can receive and process packets. diff --git a/webrtc/call/call.cc b/webrtc/call/call.cc index 6bd33a7ab2..20daba88df 100644 --- a/webrtc/call/call.cc +++ b/webrtc/call/call.cc @@ -665,9 +665,9 @@ webrtc::FlexfecReceiveStream* Call::CreateFlexfecReceiveStream( WriteLockScoped write_lock(*receive_crit_); for (auto ssrc : config.protected_media_ssrcs) flexfec_receive_ssrcs_media_.insert(std::make_pair(ssrc, receive_stream)); - RTC_DCHECK(flexfec_receive_ssrcs_protection_.find(config.flexfec_ssrc) == + RTC_DCHECK(flexfec_receive_ssrcs_protection_.find(config.remote_ssrc) == flexfec_receive_ssrcs_protection_.end()); - flexfec_receive_ssrcs_protection_[config.flexfec_ssrc] = receive_stream; + flexfec_receive_ssrcs_protection_[config.remote_ssrc] = receive_stream; flexfec_receive_streams_.insert(receive_stream); } // TODO(brandtr): Store config in RtcEventLog here. diff --git a/webrtc/call/call_unittest.cc b/webrtc/call/call_unittest.cc index 15c642fd91..7a7e7a9ab8 100644 --- a/webrtc/call/call_unittest.cc +++ b/webrtc/call/call_unittest.cc @@ -223,8 +223,8 @@ TEST(CallTest, CreateDestroy_AssociateAudioSendReceiveStreams_SendFirst) { TEST(CallTest, CreateDestroy_FlexfecReceiveStream) { CallHelper call; FlexfecReceiveStream::Config config; - config.flexfec_payload_type = 118; - config.flexfec_ssrc = 38837212; + config.payload_type = 118; + config.remote_ssrc = 38837212; config.protected_media_ssrcs = {27273}; FlexfecReceiveStream* stream = call->CreateFlexfecReceiveStream(config); @@ -235,12 +235,12 @@ TEST(CallTest, CreateDestroy_FlexfecReceiveStream) { TEST(CallTest, CreateDestroy_FlexfecReceiveStreams) { CallHelper call; FlexfecReceiveStream::Config config; - config.flexfec_payload_type = 118; + config.payload_type = 118; std::list streams; for (int i = 0; i < 2; ++i) { for (uint32_t ssrc = 0; ssrc < 1234567; ssrc += 34567) { - config.flexfec_ssrc = ssrc; + config.remote_ssrc = ssrc; config.protected_media_ssrcs = {ssrc + 1}; FlexfecReceiveStream* stream = call->CreateFlexfecReceiveStream(config); EXPECT_NE(stream, nullptr); @@ -260,27 +260,27 @@ TEST(CallTest, CreateDestroy_FlexfecReceiveStreams) { TEST(CallTest, MultipleFlexfecReceiveStreamsProtectingSingleVideoStream) { CallHelper call; FlexfecReceiveStream::Config config; - config.flexfec_payload_type = 118; + config.payload_type = 118; config.protected_media_ssrcs = {1324234}; FlexfecReceiveStream* stream; std::list streams; - config.flexfec_ssrc = 838383; + config.remote_ssrc = 838383; stream = call->CreateFlexfecReceiveStream(config); EXPECT_NE(stream, nullptr); streams.push_back(stream); - config.flexfec_ssrc = 424993; + config.remote_ssrc = 424993; stream = call->CreateFlexfecReceiveStream(config); EXPECT_NE(stream, nullptr); streams.push_back(stream); - config.flexfec_ssrc = 99383; + config.remote_ssrc = 99383; stream = call->CreateFlexfecReceiveStream(config); EXPECT_NE(stream, nullptr); streams.push_back(stream); - config.flexfec_ssrc = 5548; + config.remote_ssrc = 5548; stream = call->CreateFlexfecReceiveStream(config); EXPECT_NE(stream, nullptr); streams.push_back(stream); diff --git a/webrtc/call/flexfec_receive_stream.cc b/webrtc/call/flexfec_receive_stream.cc index 6ab1c3ab66..61a31418cc 100644 --- a/webrtc/call/flexfec_receive_stream.cc +++ b/webrtc/call/flexfec_receive_stream.cc @@ -22,20 +22,42 @@ std::string FlexfecReceiveStream::Stats::ToString(int64_t time_ms) const { return ss.str(); } +std::string FlexfecReceiveStream::Config::ToString() const { + std::stringstream ss; + ss << "{payload_type: " << payload_type; + ss << ", remote_ssrc: " << remote_ssrc; + ss << ", local_ssrc: " << local_ssrc; + ss << ", protected_media_ssrcs: ["; + size_t i = 0; + for (; i + 1 < protected_media_ssrcs.size(); ++i) + ss << protected_media_ssrcs[i] << ", "; + if (!protected_media_ssrcs.empty()) + ss << protected_media_ssrcs[i]; + ss << "], transport_cc: " << (transport_cc ? "on" : "off"); + ss << ", extensions: ["; + i = 0; + for (; i + 1 < extensions.size(); ++i) + ss << extensions[i].ToString() << ", "; + if (!extensions.empty()) + ss << extensions[i].ToString(); + ss << "]}"; + return ss.str(); +} + namespace { // TODO(brandtr): Update this function when we support multistream protection. std::unique_ptr MaybeCreateFlexfecReceiver( const FlexfecReceiveStream::Config& config, RecoveredPacketReceiver* recovered_packet_callback) { - if (config.flexfec_payload_type < 0) { + if (config.payload_type < 0) { LOG(LS_WARNING) << "Invalid FlexFEC payload type given. " << "This FlexfecReceiveStream will therefore be useless."; return nullptr; } - RTC_DCHECK_GE(config.flexfec_payload_type, 0); - RTC_DCHECK_LE(config.flexfec_payload_type, 127); - if (config.flexfec_ssrc == 0) { + RTC_DCHECK_GE(config.payload_type, 0); + RTC_DCHECK_LE(config.payload_type, 127); + if (config.remote_ssrc == 0) { LOG(LS_WARNING) << "Invalid FlexFEC SSRC given. " << "This FlexfecReceiveStream will therefore be useless."; return nullptr; @@ -56,7 +78,7 @@ std::unique_ptr MaybeCreateFlexfecReceiver( } RTC_DCHECK_EQ(1U, config.protected_media_ssrcs.size()); return std::unique_ptr( - new FlexfecReceiver(config.flexfec_ssrc, config.protected_media_ssrcs[0], + new FlexfecReceiver(config.remote_ssrc, config.protected_media_ssrcs[0], recovered_packet_callback)); } diff --git a/webrtc/call/flexfec_receive_stream_unittest.cc b/webrtc/call/flexfec_receive_stream_unittest.cc index a41b2a54e8..68f2a2d020 100644 --- a/webrtc/call/flexfec_receive_stream_unittest.cc +++ b/webrtc/call/flexfec_receive_stream_unittest.cc @@ -20,8 +20,8 @@ namespace webrtc { TEST(FlexfecReceiveStreamTest, ConstructDestruct) { FlexfecReceiveStream::Config config; - config.flexfec_payload_type = 118; - config.flexfec_ssrc = 424223; + config.payload_type = 118; + config.remote_ssrc = 424223; config.protected_media_ssrcs = {912512}; MockRecoveredPacketReceiver callback; @@ -30,8 +30,8 @@ TEST(FlexfecReceiveStreamTest, ConstructDestruct) { TEST(FlexfecReceiveStreamTest, StartStop) { FlexfecReceiveStream::Config config; - config.flexfec_payload_type = 118; - config.flexfec_ssrc = 1652392; + config.payload_type = 118; + config.remote_ssrc = 1652392; config.protected_media_ssrcs = {23300443}; MockRecoveredPacketReceiver callback; internal::FlexfecReceiveStream receive_stream(config, &callback); @@ -42,8 +42,8 @@ TEST(FlexfecReceiveStreamTest, StartStop) { TEST(FlexfecReceiveStreamTest, DoesNotProcessPacketWhenNoMediaSsrcGiven) { FlexfecReceiveStream::Config config; - config.flexfec_payload_type = 118; - config.flexfec_ssrc = 424223; + config.payload_type = 118; + config.remote_ssrc = 424223; config.protected_media_ssrcs = {}; MockRecoveredPacketReceiver callback; internal::FlexfecReceiveStream receive_stream(config, &callback); @@ -94,8 +94,8 @@ TEST(FlexfecReceiveStreamTest, RecoversPacketWhenStarted) { constexpr size_t kFlexfecPacketLength = sizeof(kFlexfecPacket); FlexfecReceiveStream::Config config; - config.flexfec_payload_type = kFlexfecPlType; - config.flexfec_ssrc = ByteReader::ReadBigEndian(kFlexfecSsrc); + config.payload_type = kFlexfecPlType; + config.remote_ssrc = ByteReader::ReadBigEndian(kFlexfecSsrc); config.protected_media_ssrcs = { ByteReader::ReadBigEndian(kMediaSsrc)}; testing::StrictMock recovered_packet_receiver; diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc index 92ac698015..9e36697a61 100644 --- a/webrtc/media/engine/webrtcvideoengine2.cc +++ b/webrtc/media/engine/webrtcvideoengine2.cc @@ -2340,7 +2340,16 @@ void WebRtcVideoChannel2::WebRtcVideoReceiveStream::RecreateWebRtcStream() { stream_ = call_->CreateVideoReceiveStream(config_.Copy()); stream_->Start(); if (IsFlexfecFieldTrialEnabled() && flexfec_config_.IsCompleteAndEnabled()) { - flexfec_stream_ = call_->CreateFlexfecReceiveStream(flexfec_config_); + webrtc::FlexfecReceiveStream::Config config; + // Payload types and SSRCs come from the FlexFEC specific part of the SDP. + config.payload_type = flexfec_config_.flexfec_payload_type; + config.remote_ssrc = flexfec_config_.flexfec_ssrc; + config.protected_media_ssrcs = flexfec_config_.protected_media_ssrcs; + // RTCP messages and RTP header extensions apply to the entire track + // in the SDP. + config.transport_cc = config_.rtp.transport_cc; + config.extensions = config_.rtp.extensions; + flexfec_stream_ = call_->CreateFlexfecReceiveStream(config); flexfec_stream_->Start(); } } diff --git a/webrtc/media/engine/webrtcvideoengine2_unittest.cc b/webrtc/media/engine/webrtcvideoengine2_unittest.cc index b8cd213f17..b8495b0546 100644 --- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc +++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc @@ -2863,9 +2863,8 @@ TEST_F(WebRtcVideoChannel2FlexfecTest, SetRecvParamsWithoutFecDisablesFec) { ASSERT_EQ(1U, streams.size()); const FakeFlexfecReceiveStream& stream = streams.front(); - EXPECT_EQ(GetEngineCodec("flexfec-03").id, - stream.GetConfig().flexfec_payload_type); - EXPECT_EQ(kFlexfecSsrc, stream.GetConfig().flexfec_ssrc); + EXPECT_EQ(GetEngineCodec("flexfec-03").id, stream.GetConfig().payload_type); + EXPECT_EQ(kFlexfecSsrc, stream.GetConfig().remote_ssrc); ASSERT_EQ(1U, stream.GetConfig().protected_media_ssrcs.size()); EXPECT_EQ(kSsrcs1[0], stream.GetConfig().protected_media_ssrcs[0]); @@ -2918,8 +2917,8 @@ TEST_F(WebRtcVideoChannel2FlexfecTest, SetSendParamsWithFecEnablesFec) { ASSERT_EQ(1U, streams.size()); const FakeFlexfecReceiveStream& stream_with_recv_params = streams.front(); EXPECT_EQ(GetEngineCodec("flexfec-03").id, - stream_with_recv_params.GetConfig().flexfec_payload_type); - EXPECT_EQ(kFlexfecSsrc, stream_with_recv_params.GetConfig().flexfec_ssrc); + stream_with_recv_params.GetConfig().payload_type); + EXPECT_EQ(kFlexfecSsrc, stream_with_recv_params.GetConfig().remote_ssrc); EXPECT_EQ(1U, stream_with_recv_params.GetConfig().protected_media_ssrcs.size()); EXPECT_EQ(kSsrcs1[0], @@ -2932,8 +2931,8 @@ TEST_F(WebRtcVideoChannel2FlexfecTest, SetSendParamsWithFecEnablesFec) { ASSERT_EQ(1U, streams.size()); const FakeFlexfecReceiveStream& stream_with_send_params = streams.front(); EXPECT_EQ(GetEngineCodec("flexfec-03").id, - stream_with_send_params.GetConfig().flexfec_payload_type); - EXPECT_EQ(kFlexfecSsrc, stream_with_send_params.GetConfig().flexfec_ssrc); + stream_with_send_params.GetConfig().payload_type); + EXPECT_EQ(kFlexfecSsrc, stream_with_send_params.GetConfig().remote_ssrc); EXPECT_EQ(1U, stream_with_send_params.GetConfig().protected_media_ssrcs.size()); EXPECT_EQ(kSsrcs1[0], diff --git a/webrtc/test/call_test.cc b/webrtc/test/call_test.cc index aadcb8d5b8..ed8a18102a 100644 --- a/webrtc/test/call_test.cc +++ b/webrtc/test/call_test.cc @@ -16,7 +16,6 @@ #include "webrtc/config.h" #include "webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.h" #include "webrtc/modules/audio_mixer/audio_mixer_impl.h" -#include "webrtc/test/call_test.h" #include "webrtc/test/testsupport/fileutils.h" #include "webrtc/voice_engine/include/voe_base.h" @@ -276,11 +275,11 @@ void CallTest::CreateMatchingReceiveConfigs(Transport* rtcp_send_transport) { // TODO(brandtr): Update this when we support multistream protection. RTC_DCHECK(num_flexfec_streams_ <= 1); if (num_flexfec_streams_ == 1) { - FlexfecReceiveStream::Config flexfec_config; - flexfec_config.flexfec_payload_type = kFlexfecPayloadType; - flexfec_config.flexfec_ssrc = kFlexfecSendSsrc; - flexfec_config.protected_media_ssrcs = {kVideoSendSsrcs[0]}; - flexfec_receive_configs_.push_back(flexfec_config); + FlexfecReceiveStream::Config config; + config.payload_type = kFlexfecPayloadType; + config.remote_ssrc = kFlexfecSendSsrc; + config.protected_media_ssrcs = {kVideoSendSsrcs[0]}; + flexfec_receive_configs_.push_back(config); } } diff --git a/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc index 286da6c221..99f951df5b 100644 --- a/webrtc/video/end_to_end_tests.cc +++ b/webrtc/video/end_to_end_tests.cc @@ -3855,6 +3855,8 @@ void VerifyEmptyUlpfecConfig(const UlpfecConfig& config) { void VerifyEmptyFlexfecConfig(const FlexfecConfig& config) { EXPECT_EQ(-1, config.flexfec_payload_type) << "Enabling FlexFEC requires rtpmap: flexfec negotiation."; + EXPECT_EQ(0U, config.flexfec_ssrc) + << "Enabling FlexFEC requires ssrc-group: FEC-FR negotiation."; EXPECT_TRUE(config.protected_media_ssrcs.empty()) << "Enabling FlexFEC requires ssrc-group: FEC-FR negotiation."; } @@ -3893,7 +3895,12 @@ TEST_P(EndToEndTest, VerifyDefaultVideoReceiveConfigParameters) { TEST_P(EndToEndTest, VerifyDefaultFlexfecReceiveConfigParameters) { FlexfecReceiveStream::Config default_receive_config; - VerifyEmptyFlexfecConfig(default_receive_config); + EXPECT_EQ(-1, default_receive_config.payload_type) + << "Enabling FlexFEC requires rtpmap: flexfec negotiation."; + EXPECT_EQ(0U, default_receive_config.remote_ssrc) + << "Enabling FlexFEC requires ssrc-group: FEC-FR negotiation."; + EXPECT_TRUE(default_receive_config.protected_media_ssrcs.empty()) + << "Enabling FlexFEC requires ssrc-group: FEC-FR negotiation."; } TEST_P(EndToEndTest, TransportSeqNumOnAudioAndVideo) { diff --git a/webrtc/video/video_quality_test.cc b/webrtc/video/video_quality_test.cc index 38c01a7d1f..ce40c5fbb3 100644 --- a/webrtc/video/video_quality_test.cc +++ b/webrtc/video/video_quality_test.cc @@ -1073,9 +1073,9 @@ void VideoQualityTest::SetupVideo(Transport* send_transport, kVideoSendSsrcs[params_.ss.selected_stream]}; FlexfecReceiveStream::Config flexfec_receive_config; - flexfec_receive_config.flexfec_payload_type = + flexfec_receive_config.payload_type = video_send_config_.rtp.flexfec.flexfec_payload_type; - flexfec_receive_config.flexfec_ssrc = + flexfec_receive_config.remote_ssrc = video_send_config_.rtp.flexfec.flexfec_ssrc; flexfec_receive_config.protected_media_ssrcs = video_send_config_.rtp.flexfec.protected_media_ssrcs;