diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc index 0bf95b7fe4..e10b5b2eda 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc @@ -36,8 +36,8 @@ RTPSenderVideo::RTPSenderVideo(Clock* clock, RTPSenderInterface* rtpSender) // Generic FEC fec_(), fec_enabled_(false), - red_payload_type_(-1), - fec_payload_type_(-1), + red_payload_type_(0), + fec_payload_type_(0), delta_fec_params_(), key_fec_params_(), producer_fec_(&fec_), @@ -191,16 +191,19 @@ void RTPSenderVideo::GenericFECStatus(bool* enable, size_t RTPSenderVideo::FECPacketOverhead() const { rtc::CritScope cs(&crit_); - if (fec_enabled_) { + size_t overhead = 0; + if (red_payload_type_ != 0) { // Overhead is FEC headers plus RED for FEC header plus anything in RTP // header beyond the 12 bytes base header (CSRC list, extensions...) // This reason for the header extensions to be included here is that // from an FEC viewpoint, they are part of the payload to be protected. // (The base RTP header is already protected by the FEC header.) - return ForwardErrorCorrection::PacketOverhead() + REDForFECHeaderLength + - (_rtpSender.RTPHeaderLength() - kRtpHeaderSize); + overhead = REDForFECHeaderLength + (_rtpSender.RTPHeaderLength() - + kRtpHeaderSize); } - return 0; + if (fec_enabled_) + overhead += ForwardErrorCorrection::PacketOverhead(); + return overhead; } void RTPSenderVideo::SetFecParameters(const FecProtectionParams* delta_params, @@ -208,8 +211,10 @@ void RTPSenderVideo::SetFecParameters(const FecProtectionParams* delta_params, rtc::CritScope cs(&crit_); RTC_DCHECK(delta_params); RTC_DCHECK(key_params); - delta_fec_params_ = *delta_params; - key_fec_params_ = *key_params; + if (fec_enabled_) { + delta_fec_params_ = *delta_params; + key_fec_params_ = *key_params; + } } int32_t RTPSenderVideo::SendVideo(const RtpVideoCodecTypes videoType, @@ -230,7 +235,7 @@ int32_t RTPSenderVideo::SendVideo(const RtpVideoCodecTypes videoType, video_header ? &(video_header->codecHeader) : nullptr, frameType)); StorageType storage; - bool fec_enabled; + int red_payload_type; bool first_frame = first_frame_sent_(); { rtc::CritScope cs(&crit_); @@ -238,7 +243,7 @@ int32_t RTPSenderVideo::SendVideo(const RtpVideoCodecTypes videoType, frameType == kVideoFrameKey ? &key_fec_params_ : &delta_fec_params_; producer_fec_.SetFecParameters(fec_params, 0); storage = packetizer->GetStorageType(_retransmissionSettings); - fec_enabled = fec_enabled_; + red_payload_type = red_payload_type_; } // Register CVO rtp header extension at the first time when we receive a frame @@ -301,7 +306,7 @@ int32_t RTPSenderVideo::SendVideo(const RtpVideoCodecTypes videoType, _rtpSender.UpdateVideoRotation(dataBuffer, packetSize, rtp_header, video_header->rotation); } - if (fec_enabled) { + if (red_payload_type != 0) { SendVideoPacketAsRed(dataBuffer, payload_bytes_in_packet, rtp_header_length, _rtpSender.SequenceNumber(), captureTimeStamp, capture_time_ms, storage, diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc index 6af7ff6303..d1811d2ced 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -670,7 +670,7 @@ int32_t VideoSendStream::Encoded(const EncodedImage& encoded_image, void VideoSendStream::ConfigureProtection() { // Enable NACK, FEC or both. const bool enable_protection_nack = config_.rtp.nack.rtp_history_ms > 0; - bool enable_protection_fec = config_.rtp.fec.red_payload_type != -1; + bool enable_protection_fec = config_.rtp.fec.ulpfec_payload_type != -1; // Payload types without picture ID cannot determine that a stream is complete // without retransmitting FEC, so using FEC + NACK for H.264 (for instance) is // a waste of bandwidth since FEC packets still have to be transmitted. Note @@ -687,22 +687,25 @@ void VideoSendStream::ConfigureProtection() { // Set to valid uint8_ts to be castable later without signed overflows. uint8_t payload_type_red = 0; uint8_t payload_type_fec = 0; + // TODO(changbin): Should set RTX for RED mapping in RTP sender in future. // Validate payload types. If either RED or FEC payload types are set then // both should be. If FEC is enabled then they both have to be set. - if (enable_protection_fec || config_.rtp.fec.red_payload_type != -1 || - config_.rtp.fec.ulpfec_payload_type != -1) { + if (config_.rtp.fec.red_payload_type != -1) { RTC_DCHECK_GE(config_.rtp.fec.red_payload_type, 0); - RTC_DCHECK_GE(config_.rtp.fec.ulpfec_payload_type, 0); RTC_DCHECK_LE(config_.rtp.fec.red_payload_type, 127); - RTC_DCHECK_LE(config_.rtp.fec.ulpfec_payload_type, 127); + // TODO(holmer): We should only enable red if ulpfec is also enabled, but + // but due to an incompatibility issue with previous versions the receiver + // assumes rtx packets are containing red if it has been configured to + // receive red. Remove this in a few versions once the incompatibility + // issue is resolved (M53 timeframe). payload_type_red = static_cast(config_.rtp.fec.red_payload_type); + } + if (config_.rtp.fec.ulpfec_payload_type != -1) { + RTC_DCHECK_GE(config_.rtp.fec.ulpfec_payload_type, 0); + RTC_DCHECK_LE(config_.rtp.fec.ulpfec_payload_type, 127); payload_type_fec = static_cast(config_.rtp.fec.ulpfec_payload_type); - } else { - // Payload types unset. - RTC_DCHECK_EQ(config_.rtp.fec.red_payload_type, -1); - RTC_DCHECK_EQ(config_.rtp.fec.ulpfec_payload_type, -1); } for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) { diff --git a/webrtc/video/video_send_stream_tests.cc b/webrtc/video/video_send_stream_tests.cc index 9c43da2700..87abfea08c 100644 --- a/webrtc/video/video_send_stream_tests.cc +++ b/webrtc/video/video_send_stream_tests.cc @@ -308,11 +308,13 @@ class FecObserver : public test::EndToEndTest { FecObserver(bool header_extensions_enabled, bool use_nack, bool expect_red, + bool expect_fec, const std::string& codec) : EndToEndTest(VideoSendStreamTest::kDefaultTimeoutMs), payload_name_(codec), use_nack_(use_nack), expect_red_(expect_red), + expect_fec_(expect_fec), send_count_(0), received_media_(false), received_fec_(false), @@ -375,6 +377,7 @@ class FecObserver : public test::EndToEndTest { if (encapsulated_payload_type != -1) { if (encapsulated_payload_type == VideoSendStreamTest::kUlpfecPayloadType) { + EXPECT_TRUE(expect_fec_); received_fec_ = true; } else { received_media_ = true; @@ -382,7 +385,7 @@ class FecObserver : public test::EndToEndTest { } if (send_count_ > 100 && received_media_) { - if (received_fec_ || !expect_red_) + if (received_fec_ || !expect_fec_) observation_complete_.Set(); } @@ -442,6 +445,7 @@ class FecObserver : public test::EndToEndTest { const std::string payload_name_; const bool use_nack_; const bool expect_red_; + const bool expect_fec_; int send_count_; bool received_media_; bool received_fec_; @@ -450,12 +454,12 @@ class FecObserver : public test::EndToEndTest { }; TEST_F(VideoSendStreamTest, SupportsFecWithExtensions) { - FecObserver test(true, false, true, "VP8"); + FecObserver test(true, false, true, true, "VP8"); RunBaseTest(&test); } TEST_F(VideoSendStreamTest, SupportsFecWithoutExtensions) { - FecObserver test(false, false, true, "VP8"); + FecObserver test(false, false, true, true, "VP8"); RunBaseTest(&test); } @@ -463,25 +467,25 @@ TEST_F(VideoSendStreamTest, SupportsFecWithoutExtensions) { // since we'll still have to re-request FEC packets, effectively wasting // bandwidth since the receiver has to wait for FEC retransmissions to determine // that the received state is actually decodable. -TEST_F(VideoSendStreamTest, DoesNotUtilizeRedForH264WithNackEnabled) { - FecObserver test(false, true, false, "H264"); +TEST_F(VideoSendStreamTest, DoesNotUtilizeFecForH264WithNackEnabled) { + FecObserver test(false, true, true, false, "H264"); RunBaseTest(&test); } // Without retransmissions FEC for H264 is fine. TEST_F(VideoSendStreamTest, DoesUtilizeRedForH264WithoutNackEnabled) { - FecObserver test(false, false, true, "H264"); + FecObserver test(false, false, true, true, "H264"); RunBaseTest(&test); } TEST_F(VideoSendStreamTest, DoesUtilizeRedForVp8WithNackEnabled) { - FecObserver test(false, true, true, "VP8"); + FecObserver test(false, true, true, true, "VP8"); RunBaseTest(&test); } #if !defined(RTC_DISABLE_VP9) TEST_F(VideoSendStreamTest, DoesUtilizeRedForVp9WithNackEnabled) { - FecObserver test(false, true, true, "VP9"); + FecObserver test(false, true, true, true, "VP9"); RunBaseTest(&test); } #endif // !defined(RTC_DISABLE_VP9)