From f1bb47605098f3e7c6583ff935f8570b3bb55a28 Mon Sep 17 00:00:00 2001 From: brandtr Date: Mon, 7 Nov 2016 03:05:06 -0800 Subject: [PATCH] Simplify {,Set}UlpfecConfig interface. Prior to this change, we signalled that ULPFEC was disabled through a bool, but that RED was disabled by setting its payload type to -1. The latter is consistent with how we disable RED/ULPFEC in the config, so this CL removes the ULPFEC bool from the {,Set}UlpfecConfig chain of member functions. BUG=webrtc:5654 Review-Url: https://codereview.webrtc.org/2460533002 Cr-Commit-Position: refs/heads/master@{#14944} --- webrtc/modules/rtp_rtcp/include/rtp_rtcp.h | 13 ++++-- webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h | 6 +-- .../modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 5 +-- .../modules/rtp_rtcp/source/rtp_rtcp_impl.h | 4 +- webrtc/modules/rtp_rtcp/source/rtp_sender.cc | 10 ++--- webrtc/modules/rtp_rtcp/source/rtp_sender.h | 6 +-- .../rtp_rtcp/source/rtp_sender_unittest.cc | 4 +- .../rtp_rtcp/source/rtp_sender_video.cc | 41 +++++++++++-------- .../rtp_rtcp/source/rtp_sender_video.h | 24 ++++++----- webrtc/video/rtp_stream_receiver.cc | 6 ++- webrtc/video/video_send_stream.cc | 22 +++++----- 11 files changed, 74 insertions(+), 67 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/include/rtp_rtcp.h b/webrtc/modules/rtp_rtcp/include/rtp_rtcp.h index be3d0f453b..68429c6748 100644 --- a/webrtc/modules/rtp_rtcp/include/rtp_rtcp.h +++ b/webrtc/modules/rtp_rtcp/include/rtp_rtcp.h @@ -434,9 +434,16 @@ class RtpRtcp : public Module { // Video // ************************************************************************** - // Turn on/off ULPFEC. - virtual void SetUlpfecConfig(bool enabled, - int red_payload_type, + // Set RED and ULPFEC payload types. A payload type of -1 means that the + // corresponding feature is turned off. Note that we DO NOT support enabling + // ULPFEC without enabling RED. However, we DO support enabling RED without + // enabling ULPFEC. This is due to an RED/RTX workaround, where the receiver + // assumes that RTX packets carry RED if RED has been configured in the SDP, + // regardless of what RTX payload type mapping was negotiated in the SDP. + // TODO(brandtr): Update this comment when we have removed the RED/RTX + // send-side workaround, i.e., when we do not support enabling RED without + // enabling ULPFEC. + virtual void SetUlpfecConfig(int red_payload_type, int ulpfec_payload_type) = 0; virtual int32_t SetFecParameters(const FecProtectionParams* delta_params, diff --git a/webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h b/webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h index 4f396e7242..ff8d90f6db 100644 --- a/webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h +++ b/webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h @@ -186,10 +186,8 @@ 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(SetUlpfecConfig, - void(bool ulpfec_enabled, - int red_payload_type, - int fec_payload_type)); + MOCK_METHOD2(SetUlpfecConfig, + void(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 9f4e165fdd..2a2bf0df4b 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -789,10 +789,9 @@ int32_t ModuleRtpRtcpImpl::SendRTCPSliceLossIndication( GetFeedbackState(), kRtcpSli, 0, 0, false, picture_id); } -void ModuleRtpRtcpImpl::SetUlpfecConfig(bool enabled, - int red_payload_type, +void ModuleRtpRtcpImpl::SetUlpfecConfig(int red_payload_type, int ulpfec_payload_type) { - rtp_sender_.SetUlpfecConfig(enabled, red_payload_type, ulpfec_payload_type); + rtp_sender_.SetUlpfecConfig(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 c89de9b0ff..f055bbe001 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -277,9 +277,7 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp { // Send a request for a keyframe. int32_t RequestKeyFrame() override; - void SetUlpfecConfig(bool enabled, - int red_payload_type, - int ulpfec_payload_type) override; + void SetUlpfecConfig(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 f0e323a4b2..a3593c604e 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc @@ -789,10 +789,10 @@ bool RTPSender::IsFecPacket(const RtpPacketToSend& packet) const { if (!video_) { return false; } - bool fec_enabled; int pt_red; int pt_fec; - video_->GetUlpfecConfig(&fec_enabled, &pt_red, &pt_fec); + video_->GetUlpfecConfig(&pt_red, &pt_fec); + const bool fec_enabled = (pt_fec != -1); return fec_enabled && static_cast(packet.PayloadType()) == pt_red && static_cast(packet.payload()[0]) == pt_fec; } @@ -1131,11 +1131,9 @@ RtpVideoCodecTypes RTPSender::VideoCodecType() const { return video_->VideoCodecType(); } -void RTPSender::SetUlpfecConfig(bool enabled, - int red_payload_type, - int ulpfec_payload_type) { +void RTPSender::SetUlpfecConfig(int red_payload_type, int ulpfec_payload_type) { RTC_DCHECK(!audio_configured_); - video_->SetUlpfecConfig(enabled, red_payload_type, ulpfec_payload_type); + video_->SetUlpfecConfig(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 75132b2799..dafddc59f1 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.h @@ -183,10 +183,8 @@ class RTPSender { uint32_t MaxConfiguredBitrateVideo() const; - // FEC. - void SetUlpfecConfig(bool enabled, - int red_payload_type, - int ulpfec_payload_type); + // ULPFEC. + void SetUlpfecConfig(int red_payload_type, int ulpfec_payload_type); int32_t SetFecParameters(const FecProtectionParams *delta_params, const FecProtectionParams *key_params); diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 16bc2716d7..fa8db8a4c4 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -1123,8 +1123,8 @@ TEST_F(RtpSenderTestWithoutPacer, StreamDataCountersCallbacks) { expected.transmitted.packets = 3; callback.Matches(ssrc, expected); - // Send FEC. - rtp_sender_->SetUlpfecConfig(true, kRedPayloadType, kUlpfecPayloadType); + // Send ULPFEC. + rtp_sender_->SetUlpfecConfig(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 dd19bb2fc7..2450a43d48 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc @@ -50,9 +50,8 @@ RTPSenderVideo::RTPSenderVideo(Clock* clock, RTPSender* rtp_sender) video_type_(kRtpVideoGeneric), retransmission_settings_(kRetransmitBaseLayer), last_rotation_(kVideoRotation_0), - fec_enabled_(false), red_payload_type_(-1), - fec_payload_type_(-1), + ulpfec_payload_type_(-1), delta_fec_params_{0, 1, kFecMaskRandom}, key_fec_params_{0, 1, kFecMaskRandom}, fec_bitrate_(1000, RateStatistics::kBpsScale), @@ -139,7 +138,7 @@ void RTPSenderVideo::SendVideoPacketAsRed( uint16_t first_fec_sequence_number = rtp_sender_->AllocateSequenceNumber(num_fec_packets); fec_packets = ulpfec_generator_.GetUlpfecPacketsAsRed( - red_payload_type_, fec_payload_type_, first_fec_sequence_number, + red_payload_type_, ulpfec_payload_type_, first_fec_sequence_number, media_packet->headers_size()); RTC_DCHECK_EQ(num_fec_packets, fec_packets.size()); if (retransmission_settings_ & kRetransmitFECPackets) @@ -179,36 +178,42 @@ void RTPSenderVideo::SendVideoPacketAsRed( } } -void RTPSenderVideo::SetUlpfecConfig(bool enabled, - int red_payload_type, +void RTPSenderVideo::SetUlpfecConfig(int red_payload_type, int ulpfec_payload_type) { - RTC_DCHECK(!enabled || red_payload_type > 0); + // Sanity check. Per the definition of UlpfecConfig (see config.h), + // a payload type of -1 means that the corresponding feature is + // turned off. + RTC_DCHECK_GE(red_payload_type, -1); RTC_DCHECK_LE(red_payload_type, 127); + RTC_DCHECK_GE(ulpfec_payload_type, -1); RTC_DCHECK_LE(ulpfec_payload_type, 127); rtc::CritScope cs(&crit_); - fec_enabled_ = enabled; red_payload_type_ = red_payload_type; - fec_payload_type_ = ulpfec_payload_type; + ulpfec_payload_type_ = ulpfec_payload_type; + + // Must not enable ULPFEC without RED. + // TODO(brandtr): We currently support enabling RED without ULPFEC. Change + // this when we have removed the RED/RTX send-side workaround, so that we + // ensure that RED and ULPFEC are only enabled together. + RTC_DCHECK(red_enabled() || !ulpfec_enabled()); // Reset FEC rates. delta_fec_params_ = FecProtectionParams{0, 1, kFecMaskRandom}; key_fec_params_ = FecProtectionParams{0, 1, kFecMaskRandom}; } -void RTPSenderVideo::GetUlpfecConfig(bool* enabled, - int* red_payload_type, +void RTPSenderVideo::GetUlpfecConfig(int* red_payload_type, int* ulpfec_payload_type) const { rtc::CritScope cs(&crit_); - *enabled = fec_enabled_; *red_payload_type = red_payload_type_; - *ulpfec_payload_type = fec_payload_type_; + *ulpfec_payload_type = ulpfec_payload_type_; } size_t RTPSenderVideo::FecPacketOverhead() const { rtc::CritScope cs(&crit_); size_t overhead = 0; - if (red_payload_type_ != -1) { + if (red_enabled()) { // 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 @@ -217,7 +222,7 @@ size_t RTPSenderVideo::FecPacketOverhead() const { return ulpfec_generator_.MaxPacketOverhead() + kRedForFecHeaderLength + (rtp_sender_->RtpHeaderLength() - kRtpHeaderSize); } - if (fec_enabled_) + if (ulpfec_enabled()) overhead += ulpfec_generator_.MaxPacketOverhead(); return overhead; } @@ -227,7 +232,7 @@ void RTPSenderVideo::SetFecParameters(const FecProtectionParams* delta_params, rtc::CritScope cs(&crit_); RTC_DCHECK(delta_params); RTC_DCHECK(key_params); - if (fec_enabled_) { + if (ulpfec_enabled()) { delta_fec_params_ = *delta_params; key_fec_params_ = *key_params; } @@ -283,7 +288,7 @@ bool RTPSenderVideo::SendVideo(RtpVideoCodecTypes video_type, video_header ? &(video_header->codecHeader) : nullptr, frame_type)); StorageType storage; - int red_payload_type; + bool red_enabled; bool first_frame = first_frame_sent_(); { rtc::CritScope cs(&crit_); @@ -291,7 +296,7 @@ bool RTPSenderVideo::SendVideo(RtpVideoCodecTypes video_type, frame_type == kVideoFrameKey ? &key_fec_params_ : &delta_fec_params_; ulpfec_generator_.SetFecParameters(fec_params); storage = packetizer->GetStorageType(retransmission_settings_); - red_payload_type = red_payload_type_; + red_enabled = this->red_enabled(); } // TODO(changbin): we currently don't support to configure the codec to @@ -318,7 +323,7 @@ bool RTPSenderVideo::SendVideo(RtpVideoCodecTypes video_type, if (!rtp_sender_->AssignSequenceNumber(packet.get())) return false; - if (red_payload_type != -1) { + if (red_enabled) { 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 56be39debd..2cb40e3397 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender_video.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender_video.h @@ -59,14 +59,9 @@ class RTPSenderVideo { void SetVideoCodecType(RtpVideoCodecTypes type); - // FEC - void SetUlpfecConfig(bool enabled, - int red_payload_type, - int ulpfec_payload_type); - - void GetUlpfecConfig(bool* enabled, - int* red_payload_type, - int* ulpfec_payload_type) const; + // ULPFEC. + void SetUlpfecConfig(int red_payload_type, int ulpfec_payload_type); + void GetUlpfecConfig(int* red_payload_type, int* ulpfec_payload_type) const; void SetFecParameters(const FecProtectionParams* delta_params, const FecProtectionParams* key_params); @@ -85,6 +80,14 @@ class RTPSenderVideo { StorageType media_packet_storage, bool protect); + bool red_enabled() const EXCLUSIVE_LOCKS_REQUIRED(crit_) { + return red_payload_type_ >= 0; + } + + bool ulpfec_enabled() const EXCLUSIVE_LOCKS_REQUIRED(crit_) { + return ulpfec_payload_type_ >= 0; + } + RTPSender* const rtp_sender_; Clock* const clock_; @@ -96,10 +99,9 @@ class RTPSenderVideo { int32_t retransmission_settings_ GUARDED_BY(crit_); VideoRotation last_rotation_ GUARDED_BY(encoder_checker_); - // FEC - bool fec_enabled_ GUARDED_BY(crit_); + // RED/ULPFEC. int red_payload_type_ GUARDED_BY(crit_); - int fec_payload_type_ GUARDED_BY(crit_); + int ulpfec_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_); diff --git a/webrtc/video/rtp_stream_receiver.cc b/webrtc/video/rtp_stream_receiver.cc index cf108e1345..576543bef6 100644 --- a/webrtc/video/rtp_stream_receiver.cc +++ b/webrtc/video/rtp_stream_receiver.cc @@ -178,8 +178,10 @@ RtpStreamReceiver::RtpStreamReceiver( config_.rtp.ulpfec.red_rtx_payload_type, config_.rtp.ulpfec.red_payload_type); } - - rtp_rtcp_->SetUlpfecConfig(true, config_.rtp.ulpfec.red_payload_type, + // TODO(brandtr): It doesn't seem that |rtp_rtcp_| is used for sending any + // RTP packets. Investigate if this is the case, and if so, remove this + // call, since there are no RTP packets to protect with RED+ULPFEC. + rtp_rtcp_->SetUlpfecConfig(config_.rtp.ulpfec.red_payload_type, config_.rtp.ulpfec.ulpfec_payload_type); } diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc index 14fd72fd6c..98baf05677 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -934,7 +934,8 @@ void VideoSendStreamImpl::ConfigureProtection() { RTC_DCHECK_RUN_ON(worker_queue_); // Enable NACK, FEC or both. const bool enable_protection_nack = config_->rtp.nack.rtp_history_ms > 0; - bool enable_protection_fec = config_->rtp.ulpfec.ulpfec_payload_type != -1; + const int red_payload_type = config_->rtp.ulpfec.red_payload_type; + int ulpfec_payload_type = config_->rtp.ulpfec.ulpfec_payload_type; // 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 @@ -945,7 +946,7 @@ void VideoSendStreamImpl::ConfigureProtection() { LOG(LS_WARNING) << "Transmitting payload type without picture ID using" "NACK+FEC is a waste of bandwidth since FEC packets " "also have to be retransmitted. Disabling FEC."; - enable_protection_fec = false; + ulpfec_payload_type = -1; } // TODO(brandtr): Remove the workaround described below. @@ -961,13 +962,13 @@ void VideoSendStreamImpl::ConfigureProtection() { // using the wrong payload type. // 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); + if (red_payload_type != -1) { + RTC_DCHECK_GE(red_payload_type, 0); + RTC_DCHECK_LE(red_payload_type, 127); } - 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); + if (ulpfec_payload_type != -1) { + RTC_DCHECK_GE(ulpfec_payload_type, 0); + RTC_DCHECK_LE(ulpfec_payload_type, 127); } for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) { @@ -977,12 +978,11 @@ void VideoSendStreamImpl::ConfigureProtection() { kMinSendSidePacketHistorySize); // Set FEC. for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) { - rtp_rtcp->SetUlpfecConfig(enable_protection_fec, - config_->rtp.ulpfec.red_payload_type, - config_->rtp.ulpfec.ulpfec_payload_type); + rtp_rtcp->SetUlpfecConfig(red_payload_type, ulpfec_payload_type); } } + const bool enable_protection_fec = (ulpfec_payload_type != -1); protection_bitrate_calculator_.SetProtectionMethod(enable_protection_fec, enable_protection_nack); }