diff --git a/webrtc/call/call_unittest.cc b/webrtc/call/call_unittest.cc index 7a7e7a9ab8..99d6812d68 100644 --- a/webrtc/call/call_unittest.cc +++ b/webrtc/call/call_unittest.cc @@ -17,6 +17,7 @@ #include "webrtc/modules/audio_coding/codecs/mock/mock_audio_decoder_factory.h" #include "webrtc/modules/audio_mixer/audio_mixer_impl.h" #include "webrtc/test/gtest.h" +#include "webrtc/test/mock_transport.h" #include "webrtc/test/mock_voice_engine.h" namespace { @@ -222,7 +223,8 @@ TEST(CallTest, CreateDestroy_AssociateAudioSendReceiveStreams_SendFirst) { TEST(CallTest, CreateDestroy_FlexfecReceiveStream) { CallHelper call; - FlexfecReceiveStream::Config config; + MockTransport rtcp_send_transport; + FlexfecReceiveStream::Config config(&rtcp_send_transport); config.payload_type = 118; config.remote_ssrc = 38837212; config.protected_media_ssrcs = {27273}; @@ -234,7 +236,8 @@ TEST(CallTest, CreateDestroy_FlexfecReceiveStream) { TEST(CallTest, CreateDestroy_FlexfecReceiveStreams) { CallHelper call; - FlexfecReceiveStream::Config config; + MockTransport rtcp_send_transport; + FlexfecReceiveStream::Config config(&rtcp_send_transport); config.payload_type = 118; std::list streams; @@ -259,7 +262,8 @@ TEST(CallTest, CreateDestroy_FlexfecReceiveStreams) { TEST(CallTest, MultipleFlexfecReceiveStreamsProtectingSingleVideoStream) { CallHelper call; - FlexfecReceiveStream::Config config; + MockTransport rtcp_send_transport; + FlexfecReceiveStream::Config config(&rtcp_send_transport); config.payload_type = 118; config.protected_media_ssrcs = {1324234}; FlexfecReceiveStream* stream; diff --git a/webrtc/call/flexfec_receive_stream.h b/webrtc/call/flexfec_receive_stream.h index fe57643cb0..babeee0ecb 100644 --- a/webrtc/call/flexfec_receive_stream.h +++ b/webrtc/call/flexfec_receive_stream.h @@ -31,8 +31,17 @@ class FlexfecReceiveStream { }; struct Config { + explicit Config(Transport* rtcp_send_transport) + : rtcp_send_transport(rtcp_send_transport) { + RTC_DCHECK(rtcp_send_transport); + } + std::string ToString() const; + // Returns true if all RTP information is available in order to + // enable receiving FlexFEC. + bool IsCompleteAndEnabled() const; + // Payload type for FlexFEC. int payload_type = -1; diff --git a/webrtc/call/flexfec_receive_stream_impl.cc b/webrtc/call/flexfec_receive_stream_impl.cc index 13390a12c0..95bcfc14dc 100644 --- a/webrtc/call/flexfec_receive_stream_impl.cc +++ b/webrtc/call/flexfec_receive_stream_impl.cc @@ -46,6 +46,19 @@ std::string FlexfecReceiveStream::Config::ToString() const { return ss.str(); } +bool FlexfecReceiveStream::Config::IsCompleteAndEnabled() const { + // Check if FlexFEC is enabled. + if (payload_type < 0) + return false; + // Do we have the necessary SSRC information? + if (remote_ssrc == 0) + return false; + // TODO(brandtr): Update this check when we support multistream protection. + if (protected_media_ssrcs.size() != 1u) + return false; + return true; +} + namespace { // TODO(brandtr): Update this function when we support multistream protection. diff --git a/webrtc/call/flexfec_receive_stream_unittest.cc b/webrtc/call/flexfec_receive_stream_unittest.cc index c25858f02c..24026365d3 100644 --- a/webrtc/call/flexfec_receive_stream_unittest.cc +++ b/webrtc/call/flexfec_receive_stream_unittest.cc @@ -13,16 +13,32 @@ #include "webrtc/base/array_view.h" #include "webrtc/call/flexfec_receive_stream_impl.h" #include "webrtc/modules/rtp_rtcp/include/flexfec_receiver.h" -#include "webrtc/modules/rtp_rtcp/source/byte_io.h" -#include "webrtc/modules/rtp_rtcp/source/rtp_packet_received.h" #include "webrtc/modules/rtp_rtcp/mocks/mock_recovered_packet_receiver.h" +#include "webrtc/modules/rtp_rtcp/source/byte_io.h" +#include "webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h" #include "webrtc/test/gmock.h" #include "webrtc/test/gtest.h" +#include "webrtc/test/mock_transport.h" namespace webrtc { namespace { +constexpr uint8_t kFlexfecPlType = 118; +constexpr uint8_t kFlexfecSsrc[] = {0x00, 0x00, 0x00, 0x01}; +constexpr uint8_t kMediaSsrc[] = {0x00, 0x00, 0x00, 0x02}; + +FlexfecReceiveStream::Config CreateDefaultConfig( + Transport* rtcp_send_transport) { + FlexfecReceiveStream::Config config(rtcp_send_transport); + config.payload_type = kFlexfecPlType; + config.remote_ssrc = ByteReader::ReadBigEndian(kFlexfecSsrc); + config.protected_media_ssrcs = { + ByteReader::ReadBigEndian(kMediaSsrc)}; + EXPECT_TRUE(config.IsCompleteAndEnabled()); + return config; +} + RtpPacketReceived ParsePacket(rtc::ArrayView packet) { RtpPacketReceived parsed_packet(nullptr); EXPECT_TRUE(parsed_packet.Parse(packet)); @@ -31,40 +47,58 @@ RtpPacketReceived ParsePacket(rtc::ArrayView packet) { } // namespace -TEST(FlexfecReceiveStreamTest, ConstructDestruct) { - FlexfecReceiveStream::Config config; - config.payload_type = 118; - config.remote_ssrc = 424223; - config.protected_media_ssrcs = {912512}; - MockRecoveredPacketReceiver recovered_packet_receiver; +TEST(FlexfecReceiveStreamConfigTest, IsCompleteAndEnabled) { + MockTransport rtcp_send_transport; + FlexfecReceiveStream::Config config(&rtcp_send_transport); - FlexfecReceiveStreamImpl receive_stream(config, &recovered_packet_receiver); + config.local_ssrc = 18374743; + config.rtcp_mode = RtcpMode::kCompound; + config.transport_cc = true; + config.rtp_header_extensions.emplace_back(TransportSequenceNumber::kUri, 7); + EXPECT_FALSE(config.IsCompleteAndEnabled()); + + config.payload_type = 123; + EXPECT_FALSE(config.IsCompleteAndEnabled()); + + config.remote_ssrc = 238423838; + EXPECT_FALSE(config.IsCompleteAndEnabled()); + + config.protected_media_ssrcs.push_back(138989393); + EXPECT_TRUE(config.IsCompleteAndEnabled()); + + config.protected_media_ssrcs.push_back(33423423); + EXPECT_FALSE(config.IsCompleteAndEnabled()); } -TEST(FlexfecReceiveStreamTest, StartStop) { - FlexfecReceiveStream::Config config; - config.payload_type = 118; - config.remote_ssrc = 1652392; - config.protected_media_ssrcs = {23300443}; - MockRecoveredPacketReceiver recovered_packet_receiver; - FlexfecReceiveStreamImpl receive_stream(config, &recovered_packet_receiver); +class FlexfecReceiveStreamTest : public ::testing::Test { + protected: + FlexfecReceiveStreamTest() + : config_(CreateDefaultConfig(&rtcp_send_transport_)), + receive_stream_(config_, &recovered_packet_receiver_) {} - receive_stream.Start(); - receive_stream.Stop(); + FlexfecReceiveStream::Config config_; + MockRecoveredPacketReceiver recovered_packet_receiver_; + MockTransport rtcp_send_transport_; + + FlexfecReceiveStreamImpl receive_stream_; +}; + +TEST_F(FlexfecReceiveStreamTest, ConstructDestruct) {} + +TEST_F(FlexfecReceiveStreamTest, StartStop) { + receive_stream_.Start(); + receive_stream_.Stop(); } // Create a FlexFEC packet that protects a single media packet and ensure // that the callback is called. Correctness of recovery is checked in the // FlexfecReceiver unit tests. -TEST(FlexfecReceiveStreamTest, RecoversPacketWhenStarted) { - constexpr uint8_t kFlexfecPlType = 118; +TEST_F(FlexfecReceiveStreamTest, RecoversPacketWhenStarted) { constexpr uint8_t kFlexfecSeqNum[] = {0x00, 0x01}; constexpr uint8_t kFlexfecTs[] = {0x00, 0x11, 0x22, 0x33}; - constexpr uint8_t kFlexfecSsrc[] = {0x00, 0x00, 0x00, 0x01}; constexpr uint8_t kMediaPlType = 107; constexpr uint8_t kMediaSeqNum[] = {0x00, 0x02}; constexpr uint8_t kMediaTs[] = {0xaa, 0xbb, 0xcc, 0xdd}; - constexpr uint8_t kMediaSsrc[] = {0x00, 0x00, 0x00, 0x02}; // This packet mask protects a single media packet, i.e., the FlexFEC payload // is a copy of that media packet. When inserted in the FlexFEC pipeline, @@ -91,13 +125,8 @@ TEST(FlexfecReceiveStreamTest, RecoversPacketWhenStarted) { kPayloadBits, kPayloadBits, kPayloadBits, kPayloadBits}; // clang-format on - FlexfecReceiveStream::Config config; - config.payload_type = kFlexfecPlType; - config.remote_ssrc = ByteReader::ReadBigEndian(kFlexfecSsrc); - config.protected_media_ssrcs = { - ByteReader::ReadBigEndian(kMediaSsrc)}; testing::StrictMock recovered_packet_receiver; - FlexfecReceiveStreamImpl receive_stream(config, &recovered_packet_receiver); + FlexfecReceiveStreamImpl receive_stream(config_, &recovered_packet_receiver); // Do not call back before being started. receive_stream.AddAndProcessReceivedPacket(ParsePacket(kFlexfecPacket)); diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc index de0e2d5078..14e72d5574 100644 --- a/webrtc/media/engine/webrtcvideoengine2.cc +++ b/webrtc/media/engine/webrtcvideoengine2.cc @@ -1185,7 +1185,7 @@ bool WebRtcVideoChannel2::AddRecvStream(const StreamParams& sp, receive_ssrcs_.insert(used_ssrc); webrtc::VideoReceiveStream::Config config(this); - webrtc::FlexfecConfig flexfec_config; + webrtc::FlexfecReceiveStream::Config flexfec_config(this); ConfigureReceiverRtp(&config, &flexfec_config, sp); config.disable_prerenderer_smoothing = @@ -1201,7 +1201,7 @@ bool WebRtcVideoChannel2::AddRecvStream(const StreamParams& sp, void WebRtcVideoChannel2::ConfigureReceiverRtp( webrtc::VideoReceiveStream::Config* config, - webrtc::FlexfecConfig* flexfec_config, + webrtc::FlexfecReceiveStream::Config* flexfec_config, const StreamParams& sp) const { uint32_t ssrc = sp.first_ssrc(); @@ -1234,10 +1234,12 @@ void WebRtcVideoChannel2::ConfigureReceiverRtp( send_codec_ ? HasTransportCc(send_codec_->codec) : false; // TODO(brandtr): Generalize when we add support for multistream protection. - uint32_t flexfec_ssrc; - if (sp.GetFecFrSsrc(ssrc, &flexfec_ssrc)) { - flexfec_config->flexfec_ssrc = flexfec_ssrc; + if (sp.GetFecFrSsrc(ssrc, &flexfec_config->remote_ssrc)) { flexfec_config->protected_media_ssrcs = {ssrc}; + flexfec_config->local_ssrc = config->rtp.local_ssrc; + flexfec_config->rtcp_mode = config->rtp.rtcp_mode; + flexfec_config->transport_cc = config->rtp.transport_cc; + flexfec_config->rtp_header_extensions = config->rtp.extensions; } for (size_t i = 0; i < recv_codecs_.size(); ++i) { @@ -2089,7 +2091,7 @@ WebRtcVideoChannel2::WebRtcVideoReceiveStream::WebRtcVideoReceiveStream( WebRtcVideoDecoderFactory* external_decoder_factory, bool default_stream, const std::vector& recv_codecs, - const webrtc::FlexfecConfig& flexfec_config) + const webrtc::FlexfecReceiveStream::Config& flexfec_config) : call_(call), stream_params_(sp), stream_(NULL), @@ -2198,8 +2200,7 @@ void WebRtcVideoChannel2::WebRtcVideoReceiveStream::ConfigureCodecs( // TODO(pbos): Reconfigure RTX based on incoming recv_codecs. config_.rtp.ulpfec = recv_codecs.front().ulpfec; - flexfec_config_.flexfec_payload_type = - recv_codecs.front().flexfec_payload_type; + flexfec_config_.payload_type = recv_codecs.front().flexfec_payload_type; config_.rtp.nack.rtp_history_ms = HasNack(recv_codecs.begin()->codec) ? kNackHistoryMs : 0; @@ -2282,16 +2283,7 @@ void WebRtcVideoChannel2::WebRtcVideoReceiveStream::RecreateWebRtcStream() { stream_ = call_->CreateVideoReceiveStream(config_.Copy()); stream_->Start(); if (IsFlexfecFieldTrialEnabled() && flexfec_config_.IsCompleteAndEnabled()) { - 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.rtp_header_extensions = config_.rtp.extensions; - flexfec_stream_ = call_->CreateFlexfecReceiveStream(config); + flexfec_stream_ = call_->CreateFlexfecReceiveStream(flexfec_config_); flexfec_stream_->Start(); } } diff --git a/webrtc/media/engine/webrtcvideoengine2.h b/webrtc/media/engine/webrtcvideoengine2.h index b50c5a148e..2ee9bc79b8 100644 --- a/webrtc/media/engine/webrtcvideoengine2.h +++ b/webrtc/media/engine/webrtcvideoengine2.h @@ -27,6 +27,7 @@ #include "webrtc/media/base/videosinkinterface.h" #include "webrtc/media/base/videosourceinterface.h" #include "webrtc/call/call.h" +#include "webrtc/call/flexfec_receive_stream.h" #include "webrtc/media/base/mediaengine.h" #include "webrtc/media/engine/webrtcvideodecoderfactory.h" #include "webrtc/media/engine/webrtcvideoencoderfactory.h" @@ -218,9 +219,10 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport { void SetMaxSendBandwidth(int bps); - void ConfigureReceiverRtp(webrtc::VideoReceiveStream::Config* config, - webrtc::FlexfecConfig* flexfec_config, - const StreamParams& sp) const; + void ConfigureReceiverRtp( + webrtc::VideoReceiveStream::Config* config, + webrtc::FlexfecReceiveStream::Config* flexfec_config, + const StreamParams& sp) const; bool ValidateSendSsrcAvailability(const StreamParams& sp) const EXCLUSIVE_LOCKS_REQUIRED(stream_crit_); bool ValidateReceiveSsrcAvailability(const StreamParams& sp) const @@ -359,7 +361,7 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport { WebRtcVideoDecoderFactory* external_decoder_factory, bool default_stream, const std::vector& recv_codecs, - const webrtc::FlexfecConfig& flexfec_config); + const webrtc::FlexfecReceiveStream::Config& flexfec_config); ~WebRtcVideoReceiveStream(); const std::vector& GetSsrcs() const; @@ -412,7 +414,7 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport { webrtc::VideoReceiveStream* stream_; const bool default_stream_; webrtc::VideoReceiveStream::Config config_; - webrtc::FlexfecConfig flexfec_config_; + webrtc::FlexfecReceiveStream::Config flexfec_config_; webrtc::FlexfecReceiveStream* flexfec_stream_; WebRtcVideoDecoderFactory* const external_decoder_factory_; diff --git a/webrtc/test/call_test.cc b/webrtc/test/call_test.cc index 84bfab625a..2b7d2e7f08 100644 --- a/webrtc/test/call_test.cc +++ b/webrtc/test/call_test.cc @@ -275,7 +275,7 @@ 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 config; + FlexfecReceiveStream::Config config(rtcp_send_transport); config.payload_type = kFlexfecPayloadType; config.remote_ssrc = kFlexfecSendSsrc; config.protected_media_ssrcs = {kVideoSendSsrcs[0]}; diff --git a/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc index ada39c13cd..367b74ce0e 100644 --- a/webrtc/video/end_to_end_tests.cc +++ b/webrtc/video/end_to_end_tests.cc @@ -3906,7 +3906,8 @@ TEST_P(EndToEndTest, VerifyDefaultVideoReceiveConfigParameters) { } TEST_P(EndToEndTest, VerifyDefaultFlexfecReceiveConfigParameters) { - FlexfecReceiveStream::Config default_receive_config; + test::NullTransport rtcp_send_transport; + FlexfecReceiveStream::Config default_receive_config(&rtcp_send_transport); EXPECT_EQ(-1, default_receive_config.payload_type) << "Enabling FlexFEC requires rtpmap: flexfec negotiation."; EXPECT_EQ(0U, default_receive_config.remote_ssrc) diff --git a/webrtc/video/video_quality_test.cc b/webrtc/video/video_quality_test.cc index 7173e01845..e37b555f7e 100644 --- a/webrtc/video/video_quality_test.cc +++ b/webrtc/video/video_quality_test.cc @@ -1044,8 +1044,9 @@ void VideoQualityTest::SetupVideo(Transport* send_transport, if (params_.logs) trace_to_stderr_.reset(new test::TraceToStderr); - size_t num_streams = params_.ss.streams.size(); - CreateSendConfig(num_streams, 0, 0, send_transport); + size_t num_video_streams = params_.ss.streams.size(); + size_t num_flexfec_streams = params_.video.flexfec ? 1 : 0; + CreateSendConfig(num_video_streams, 0, num_flexfec_streams, send_transport); int payload_type; if (params_.video.codec == "H264") { @@ -1066,7 +1067,7 @@ void VideoQualityTest::SetupVideo(Transport* send_transport, video_send_config_.encoder_settings.payload_type = payload_type; video_send_config_.rtp.nack.rtp_history_ms = kNackRtpHistoryMs; video_send_config_.rtp.rtx.payload_type = kSendRtxPayloadType; - for (size_t i = 0; i < num_streams; ++i) + for (size_t i = 0; i < num_video_streams; ++i) video_send_config_.rtp.rtx.ssrcs.push_back(kSendRtxSsrcs[i]); video_send_config_.rtp.extensions.clear(); @@ -1098,7 +1099,7 @@ void VideoQualityTest::SetupVideo(Transport* send_transport, CreateMatchingReceiveConfigs(recv_transport); - for (size_t i = 0; i < num_streams; ++i) { + for (size_t i = 0; i < num_video_streams; ++i) { video_receive_configs_[i].rtp.nack.rtp_history_ms = kNackRtpHistoryMs; video_receive_configs_[i].rtp.rtx[payload_type].ssrc = kSendRtxSsrcs[i]; video_receive_configs_[i].rtp.rtx[payload_type].payload_type = @@ -1107,12 +1108,14 @@ void VideoQualityTest::SetupVideo(Transport* send_transport, } if (params_.video.flexfec) { - video_send_config_.rtp.flexfec.flexfec_payload_type = kFlexfecPayloadType; - video_send_config_.rtp.flexfec.flexfec_ssrc = kFlexfecSendSsrc; + // Override send config constructed by CreateSendConfig. video_send_config_.rtp.flexfec.protected_media_ssrcs = { kVideoSendSsrcs[params_.ss.selected_stream]}; - FlexfecReceiveStream::Config flexfec_receive_config; + // The matching receive config is _not_ created by + // CreateMatchingReceiveConfigs, since VideoQualityTest is not a BaseTest. + // Set up the receive config manually instead. + FlexfecReceiveStream::Config flexfec_receive_config(recv_transport); flexfec_receive_config.payload_type = video_send_config_.rtp.flexfec.flexfec_payload_type; flexfec_receive_config.remote_ssrc =