diff --git a/webrtc/modules/rtp_rtcp/include/rtp_rtcp.h b/webrtc/modules/rtp_rtcp/include/rtp_rtcp.h index 0c36117cea..be3d0f453b 100644 --- a/webrtc/modules/rtp_rtcp/include/rtp_rtcp.h +++ b/webrtc/modules/rtp_rtcp/include/rtp_rtcp.h @@ -434,15 +434,10 @@ class RtpRtcp : public Module { // Video // ************************************************************************** - // Turn on/off generic FEC. - virtual void SetGenericFECStatus(bool enable, - uint8_t payload_type_red, - uint8_t payload_type_fec) = 0; - - // Get generic FEC setting. - virtual void GenericFECStatus(bool* enable, - uint8_t* payload_type_red, - uint8_t* payload_type_fec) = 0; + // Turn on/off ULPFEC. + virtual void SetUlpfecConfig(bool enabled, + int red_payload_type, + int ulpfec_payload_type) = 0; virtual int32_t SetFecParameters(const FecProtectionParams* delta_params, const FecProtectionParams* key_params) = 0; diff --git a/webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h b/webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h index 25d78c0589..4f396e7242 100644 --- a/webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h +++ b/webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h @@ -186,14 +186,10 @@ class MockRtpRtcp : public RtpRtcp { int32_t(bool enable, uint8_t id)); MOCK_METHOD1(SetAudioLevel, int32_t(uint8_t level_dbov)); MOCK_METHOD1(SetTargetSendBitrate, void(uint32_t bitrate_bps)); - MOCK_METHOD3(SetGenericFECStatus, - void(bool enable, - uint8_t payload_type_red, - uint8_t payload_type_fec)); - MOCK_METHOD3(GenericFECStatus, - void(bool* enable, - uint8_t* payload_type_red, - uint8_t* payload_type_fec)); + MOCK_METHOD3(SetUlpfecConfig, + void(bool ulpfec_enabled, + int red_payload_type, + int fec_payload_type)); MOCK_METHOD2(SetFecParameters, int32_t(const FecProtectionParams* delta_params, const FecProtectionParams* key_params)); diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index c219cb1e00..9f4e165fdd 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -789,17 +789,10 @@ int32_t ModuleRtpRtcpImpl::SendRTCPSliceLossIndication( GetFeedbackState(), kRtcpSli, 0, 0, false, picture_id); } -void ModuleRtpRtcpImpl::SetGenericFECStatus( - const bool enable, - const uint8_t payload_type_red, - const uint8_t payload_type_fec) { - rtp_sender_.SetGenericFECStatus(enable, payload_type_red, payload_type_fec); -} - -void ModuleRtpRtcpImpl::GenericFECStatus(bool* enable, - uint8_t* payload_type_red, - uint8_t* payload_type_fec) { - rtp_sender_.GenericFECStatus(enable, payload_type_red, payload_type_fec); +void ModuleRtpRtcpImpl::SetUlpfecConfig(bool enabled, + int red_payload_type, + int ulpfec_payload_type) { + rtp_sender_.SetUlpfecConfig(enabled, red_payload_type, ulpfec_payload_type); } int32_t ModuleRtpRtcpImpl::SetFecParameters( diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h index 482cfb09d4..c89de9b0ff 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -277,13 +277,9 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp { // Send a request for a keyframe. int32_t RequestKeyFrame() override; - void SetGenericFECStatus(bool enable, - uint8_t payload_type_red, - uint8_t payload_type_fec) override; - - void GenericFECStatus(bool* enable, - uint8_t* payload_type_red, - uint8_t* payload_type_fec) override; + void SetUlpfecConfig(bool enabled, + int red_payload_type, + int ulpfec_payload_type) override; int32_t SetFecParameters(const FecProtectionParams* delta_params, const FecProtectionParams* key_params) override; diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc index f3fac77d54..f0e323a4b2 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc @@ -790,11 +790,11 @@ bool RTPSender::IsFecPacket(const RtpPacketToSend& packet) const { return false; } bool fec_enabled; - uint8_t pt_red; - uint8_t pt_fec; - video_->GenericFECStatus(&fec_enabled, &pt_red, &pt_fec); - return fec_enabled && packet.PayloadType() == pt_red && - packet.payload()[0] == pt_fec; + int pt_red; + int pt_fec; + video_->GetUlpfecConfig(&fec_enabled, &pt_red, &pt_fec); + return fec_enabled && static_cast(packet.PayloadType()) == pt_red && + static_cast(packet.payload()[0]) == pt_fec; } size_t RTPSender::TimeToSendPadding(size_t bytes, int probe_cluster_id) { @@ -1131,18 +1131,11 @@ RtpVideoCodecTypes RTPSender::VideoCodecType() const { return video_->VideoCodecType(); } -void RTPSender::SetGenericFECStatus(bool enable, - uint8_t payload_type_red, - uint8_t payload_type_fec) { +void RTPSender::SetUlpfecConfig(bool enabled, + int red_payload_type, + int ulpfec_payload_type) { RTC_DCHECK(!audio_configured_); - video_->SetGenericFECStatus(enable, payload_type_red, payload_type_fec); -} - -void RTPSender::GenericFECStatus(bool* enable, - uint8_t* payload_type_red, - uint8_t* payload_type_fec) const { - RTC_DCHECK(!audio_configured_); - video_->GenericFECStatus(enable, payload_type_red, payload_type_fec); + video_->SetUlpfecConfig(enabled, red_payload_type, ulpfec_payload_type); } int32_t RTPSender::SetFecParameters( diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.h b/webrtc/modules/rtp_rtcp/source/rtp_sender.h index f31dde9961..75132b2799 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.h @@ -184,13 +184,9 @@ class RTPSender { uint32_t MaxConfiguredBitrateVideo() const; // FEC. - void SetGenericFECStatus(bool enable, - uint8_t payload_type_red, - uint8_t payload_type_fec); - - void GenericFECStatus(bool* enable, - uint8_t* payload_type_red, - uint8_t* payload_type_fec) const; + void SetUlpfecConfig(bool enabled, + int red_payload_type, + int ulpfec_payload_type); int32_t SetFecParameters(const FecProtectionParams *delta_params, const FecProtectionParams *key_params); @@ -271,7 +267,7 @@ class RTPSender { int64_t last_capture_time_ms_sent_; rtc::CriticalSection send_critsect_; - Transport *transport_; + Transport* transport_; bool sending_media_ GUARDED_BY(send_critsect_); size_t max_payload_length_; diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 8fa21d931d..16bc2716d7 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -1124,7 +1124,7 @@ TEST_F(RtpSenderTestWithoutPacer, StreamDataCountersCallbacks) { callback.Matches(ssrc, expected); // Send FEC. - rtp_sender_->SetGenericFECStatus(true, kRedPayloadType, kUlpfecPayloadType); + rtp_sender_->SetUlpfecConfig(true, kRedPayloadType, kUlpfecPayloadType); FecProtectionParams fec_params; fec_params.fec_mask_type = kFecMaskRandom; fec_params.fec_rate = 1; diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc index 76fc42bdba..dd19bb2fc7 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc @@ -47,6 +47,14 @@ void BuildRedPayload(const RtpPacketToSend& media_packet, RTPSenderVideo::RTPSenderVideo(Clock* clock, RTPSender* rtp_sender) : rtp_sender_(rtp_sender), clock_(clock), + video_type_(kRtpVideoGeneric), + retransmission_settings_(kRetransmitBaseLayer), + last_rotation_(kVideoRotation_0), + fec_enabled_(false), + red_payload_type_(-1), + fec_payload_type_(-1), + delta_fec_params_{0, 1, kFecMaskRandom}, + key_fec_params_{0, 1, kFecMaskRandom}, fec_bitrate_(1000, RateStatistics::kBpsScale), video_bitrate_(1000, RateStatistics::kBpsScale) { encoder_checker_.Detach(); @@ -171,31 +179,36 @@ void RTPSenderVideo::SendVideoPacketAsRed( } } -void RTPSenderVideo::SetGenericFECStatus(bool enable, - uint8_t payload_type_red, - uint8_t payload_type_fec) { - RTC_DCHECK(!enable || payload_type_red > 0); +void RTPSenderVideo::SetUlpfecConfig(bool enabled, + int red_payload_type, + int ulpfec_payload_type) { + RTC_DCHECK(!enabled || red_payload_type > 0); + RTC_DCHECK_LE(red_payload_type, 127); + RTC_DCHECK_LE(ulpfec_payload_type, 127); + rtc::CritScope cs(&crit_); - fec_enabled_ = enable; - red_payload_type_ = payload_type_red; - fec_payload_type_ = payload_type_fec; + fec_enabled_ = enabled; + red_payload_type_ = red_payload_type; + fec_payload_type_ = ulpfec_payload_type; + + // Reset FEC rates. delta_fec_params_ = FecProtectionParams{0, 1, kFecMaskRandom}; key_fec_params_ = FecProtectionParams{0, 1, kFecMaskRandom}; } -void RTPSenderVideo::GenericFECStatus(bool* enable, - uint8_t* payload_type_red, - uint8_t* payload_type_fec) const { +void RTPSenderVideo::GetUlpfecConfig(bool* enabled, + int* red_payload_type, + int* ulpfec_payload_type) const { rtc::CritScope cs(&crit_); - *enable = fec_enabled_; - *payload_type_red = red_payload_type_; - *payload_type_fec = fec_payload_type_; + *enabled = fec_enabled_; + *red_payload_type = red_payload_type_; + *ulpfec_payload_type = fec_payload_type_; } size_t RTPSenderVideo::FecPacketOverhead() const { rtc::CritScope cs(&crit_); size_t overhead = 0; - if (red_payload_type_ != 0) { + if (red_payload_type_ != -1) { // 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 @@ -305,7 +318,7 @@ bool RTPSenderVideo::SendVideo(RtpVideoCodecTypes video_type, if (!rtp_sender_->AssignSequenceNumber(packet.get())) return false; - if (red_payload_type != 0) { + if (red_payload_type != -1) { SendVideoPacketAsRed(std::move(packet), storage, packetizer->GetProtectionType() == kProtectedPacket); } else { diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.h b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.h index 003b79f0b8..56be39debd 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.h @@ -60,13 +60,13 @@ class RTPSenderVideo { void SetVideoCodecType(RtpVideoCodecTypes type); // FEC - void SetGenericFECStatus(bool enable, - uint8_t payload_type_red, - uint8_t payload_type_fec); + void SetUlpfecConfig(bool enabled, + int red_payload_type, + int ulpfec_payload_type); - void GenericFECStatus(bool* enable, - uint8_t* payload_type_red, - uint8_t* payload_type_fec) const; + void GetUlpfecConfig(bool* enabled, + int* red_payload_type, + int* ulpfec_payload_type) const; void SetFecParameters(const FecProtectionParams* delta_params, const FecProtectionParams* key_params); @@ -92,18 +92,16 @@ class RTPSenderVideo { rtc::CriticalSection crit_; rtc::SequencedTaskChecker encoder_checker_; - RtpVideoCodecTypes video_type_ = kRtpVideoGeneric; - int32_t retransmission_settings_ GUARDED_BY(crit_) = kRetransmitBaseLayer; - VideoRotation last_rotation_ GUARDED_BY(encoder_checker_) = kVideoRotation_0; + RtpVideoCodecTypes video_type_; + int32_t retransmission_settings_ GUARDED_BY(crit_); + VideoRotation last_rotation_ GUARDED_BY(encoder_checker_); // FEC - bool fec_enabled_ GUARDED_BY(crit_) = false; - int8_t red_payload_type_ GUARDED_BY(crit_) = 0; - int8_t fec_payload_type_ GUARDED_BY(crit_) = 0; - FecProtectionParams delta_fec_params_ GUARDED_BY(crit_) = FecProtectionParams{ - 0, 1, kFecMaskRandom}; - FecProtectionParams key_fec_params_ GUARDED_BY(crit_) = FecProtectionParams{ - 0, 1, kFecMaskRandom}; + bool fec_enabled_ GUARDED_BY(crit_); + int red_payload_type_ GUARDED_BY(crit_); + int fec_payload_type_ GUARDED_BY(crit_); + FecProtectionParams delta_fec_params_ GUARDED_BY(crit_); + FecProtectionParams key_fec_params_ GUARDED_BY(crit_); UlpfecGenerator ulpfec_generator_ GUARDED_BY(crit_); rtc::CriticalSection stats_crit_; diff --git a/webrtc/video/rtp_stream_receiver.cc b/webrtc/video/rtp_stream_receiver.cc index b826ed5b02..cf108e1345 100644 --- a/webrtc/video/rtp_stream_receiver.cc +++ b/webrtc/video/rtp_stream_receiver.cc @@ -179,8 +179,8 @@ RtpStreamReceiver::RtpStreamReceiver( config_.rtp.ulpfec.red_payload_type); } - rtp_rtcp_->SetGenericFECStatus(true, config_.rtp.ulpfec.red_payload_type, - config_.rtp.ulpfec.ulpfec_payload_type); + rtp_rtcp_->SetUlpfecConfig(true, config_.rtp.ulpfec.red_payload_type, + config_.rtp.ulpfec.ulpfec_payload_type); } if (config_.rtp.rtcp_xr.receiver_reference_time_report) diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc index fdb8ccc7aa..14fd72fd6c 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -948,29 +948,26 @@ void VideoSendStreamImpl::ConfigureProtection() { enable_protection_fec = false; } - // 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(brandtr): Remove the workaround described below. + // + // In theory, we should enable RED if and only if ULPFEC is also enabled, + // and vice versa. (We only support ULPFEC over RED, not multiplexed in any + // other way.) However, due to the RED/RTX workaround introduced here: + // https://codereview.webrtc.org/1649493004, we need to send media over RED + // (even if ULPFEC is disabled), whenever RED has been negotiated in the SDP. + // This is due to the associated payload type is hardcoded to be RED in the + // receiver, whenever RED appears in the SDP. If we would not send media over + // RED in this case, the RTX receiver would recover retransmitted packets + // using the wrong payload type. - // 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. + // Verify validity of provided payload types. if (config_->rtp.ulpfec.red_payload_type != -1) { RTC_DCHECK_GE(config_->rtp.ulpfec.red_payload_type, 0); RTC_DCHECK_LE(config_->rtp.ulpfec.red_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.ulpfec.red_payload_type); } if (config_->rtp.ulpfec.ulpfec_payload_type != -1) { RTC_DCHECK_GE(config_->rtp.ulpfec.ulpfec_payload_type, 0); RTC_DCHECK_LE(config_->rtp.ulpfec.ulpfec_payload_type, 127); - payload_type_fec = - static_cast(config_->rtp.ulpfec.ulpfec_payload_type); } for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) { @@ -980,8 +977,9 @@ void VideoSendStreamImpl::ConfigureProtection() { kMinSendSidePacketHistorySize); // Set FEC. for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) { - rtp_rtcp->SetGenericFECStatus(enable_protection_fec, payload_type_red, - payload_type_fec); + rtp_rtcp->SetUlpfecConfig(enable_protection_fec, + config_->rtp.ulpfec.red_payload_type, + config_->rtp.ulpfec.ulpfec_payload_type); } }