From e6f98c7a379aae970e7570ac3cf99e2a21f256c0 Mon Sep 17 00:00:00 2001 From: brandtr Date: Fri, 11 Nov 2016 03:28:30 -0800 Subject: [PATCH] Remove RED/RTX workaround from sender/receiver and VideoEngine2. In older Chrome versions, the associated payload type in the RTX header of retransmitted packets was always set to be the original media payload type, regardless of the actual payload type of the packet. This meant that packets encapsulated with RED headers had incorrect payload type information in the RTX header. Due to an assumption in the receiver, this incorrect payload type information would effectively be undone, leading to a working system. Albeit working, this behaviour was undesired, and thus removed. In the interim, several workarounds were introduced to not destroy interop between old and new Chrome versions: (1) https://codereview.webrtc.org/1649493004 - If no payload type mapping existed for RED over RTX, the payload type of the underlying media would be used. - If RED had been negotiated, received RTX packets would always be assumed to contain RED. (2) https://codereview.webrtc.org/1964473002 - If RED was removed from the remote description answer, it would be disabled in the local receiver as well. (3) https://codereview.webrtc.org/2033763002 - If RED was negotiated in the SDP, it would always be used, regardless if ULPFEC was negotiated and used, or not. Since the Chrome versions that exhibited the original bug now are very old, this CL removes the workarounds from (1) and (2). In particular, after this change, we will have the following behaviour: - We assume that a payload type mapping for RED over RTX always is set. If this is not the case, the RTX packet is not sent. - The associated payload type of received RTX packets will always be obeyed. - The (non)-existence of RED in the remote description does not affect the local receiver. The workaround in (3) still needs to exist, in order to interop with receivers that did not have the workarounds in (1) and (2) removed. The change in (3) can be removed in a couple of Chrome versions. TESTED=Using AppRTC between patched Chrome (connected to ethernet) and standard Chrome M54 (connected to lossy internal Google WiFi), with and without FEC turned off using AppRTC flag. Also using "Munge SDP" sample on patched Chrome over loopback interface, with 100ms delay and 5% packet loss simulated using tc. BUG=webrtc:6650 Review-Url: https://codereview.webrtc.org/2469093003 Cr-Commit-Position: refs/heads/master@{#15038} --- webrtc/media/engine/webrtcvideoengine2.cc | 34 +--------- webrtc/media/engine/webrtcvideoengine2.h | 13 +--- .../engine/webrtcvideoengine2_unittest.cc | 12 +--- .../rtp_rtcp/include/rtp_payload_registry.h | 23 ++----- .../rtp_rtcp/source/rtp_payload_registry.cc | 29 ++++---- .../source/rtp_payload_registry_unittest.cc | 20 ------ webrtc/modules/rtp_rtcp/source/rtp_sender.cc | 12 ++-- webrtc/video/rtp_stream_receiver.cc | 23 ++----- webrtc/video/rtp_stream_receiver.h | 1 + webrtc/video/video_send_stream.cc | 66 ++++++++++--------- webrtc/video/video_send_stream_tests.cc | 6 +- webrtc/video_receive_stream.h | 5 -- 12 files changed, 79 insertions(+), 165 deletions(-) diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc index 3cacc7da82..e275e5e628 100644 --- a/webrtc/media/engine/webrtcvideoengine2.cc +++ b/webrtc/media/engine/webrtcvideoengine2.cc @@ -677,7 +677,6 @@ WebRtcVideoChannel2::WebRtcVideoChannel2( external_encoder_factory_(external_encoder_factory), external_decoder_factory_(external_decoder_factory), default_send_options_(options), - red_disabled_by_remote_side_(false), last_stats_log_ms_(-1) { RTC_DCHECK(thread_checker_.CalledOnValidThread()); @@ -848,19 +847,6 @@ bool WebRtcVideoChannel2::SetSendParameters(const VideoSendParameters& params) { : webrtc::RtcpMode::kCompound); } } - if (changed_params.codec) { - bool red_was_disabled = red_disabled_by_remote_side_; - red_disabled_by_remote_side_ = - changed_params.codec->ulpfec.red_payload_type == -1; - if (red_was_disabled != red_disabled_by_remote_side_) { - for (auto& kv : receive_streams_) { - // In practice VideoChannel::SetRemoteContent appears to most of the - // time also call UpdateRemoteStreams, which recreates the receive - // streams. If that's always true this call isn't needed. - kv.second->SetUlpfecDisabledRemotely(red_disabled_by_remote_side_); - } - } - } } send_params_ = params; return true; @@ -1240,7 +1226,7 @@ bool WebRtcVideoChannel2::AddRecvStream(const StreamParams& sp, receive_streams_[ssrc] = new WebRtcVideoReceiveStream( call_, sp, std::move(config), external_decoder_factory_, default_stream, - recv_codecs_, red_disabled_by_remote_side_); + recv_codecs_); return true; } @@ -2134,14 +2120,12 @@ WebRtcVideoChannel2::WebRtcVideoReceiveStream::WebRtcVideoReceiveStream( webrtc::VideoReceiveStream::Config config, WebRtcVideoDecoderFactory* external_decoder_factory, bool default_stream, - const std::vector& recv_codecs, - bool red_disabled_by_remote_side) + const std::vector& recv_codecs) : call_(call), stream_params_(sp), stream_(NULL), default_stream_(default_stream), config_(std::move(config)), - red_disabled_by_remote_side_(red_disabled_by_remote_side), external_decoder_factory_(external_decoder_factory), sink_(NULL), first_frame_timestamp_(-1), @@ -2344,13 +2328,7 @@ void WebRtcVideoChannel2::WebRtcVideoReceiveStream::RecreateWebRtcStream() { if (stream_ != NULL) { call_->DestroyVideoReceiveStream(stream_); } - webrtc::VideoReceiveStream::Config config = config_.Copy(); - if (red_disabled_by_remote_side_) { - config.rtp.ulpfec.red_payload_type = -1; - config.rtp.ulpfec.ulpfec_payload_type = -1; - config.rtp.ulpfec.red_rtx_payload_type = -1; - } - stream_ = call_->CreateVideoReceiveStream(std::move(config)); + stream_ = call_->CreateVideoReceiveStream(config_.Copy()); stream_->Start(); } @@ -2457,12 +2435,6 @@ WebRtcVideoChannel2::WebRtcVideoReceiveStream::GetVideoReceiverInfo( return info; } -void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetUlpfecDisabledRemotely( - bool disable) { - red_disabled_by_remote_side_ = disable; - RecreateWebRtcStream(); -} - WebRtcVideoChannel2::VideoCodecSettings::VideoCodecSettings() : rtx_payload_type(-1) {} diff --git a/webrtc/media/engine/webrtcvideoengine2.h b/webrtc/media/engine/webrtcvideoengine2.h index ef893d2e98..74a1311ea1 100644 --- a/webrtc/media/engine/webrtcvideoengine2.h +++ b/webrtc/media/engine/webrtcvideoengine2.h @@ -389,8 +389,7 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport { webrtc::VideoReceiveStream::Config config, WebRtcVideoDecoderFactory* external_decoder_factory, bool default_stream, - const std::vector& recv_codecs, - bool red_disabled_by_remote_side); + const std::vector& recv_codecs); ~WebRtcVideoReceiveStream(); const std::vector& GetSsrcs() const; @@ -411,14 +410,6 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport { VideoReceiverInfo GetVideoReceiverInfo(bool log_stats); - // Used to disable RED/FEC when the remote description doesn't contain those - // codecs. This is needed to be able to work around an RTX bug which is only - // happening if the remote side doesn't send RED, but the local side is - // configured to receive RED. - // TODO(holmer): Remove this after a couple of Chrome versions, M53-54 - // time frame. - void SetUlpfecDisabledRemotely(bool disable); - private: struct AllocatedDecoder { AllocatedDecoder(webrtc::VideoDecoder* decoder, @@ -448,7 +439,6 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport { webrtc::VideoReceiveStream* stream_; const bool default_stream_; webrtc::VideoReceiveStream::Config config_; - bool red_disabled_by_remote_side_; WebRtcVideoDecoderFactory* const external_decoder_factory_; std::vector allocated_decoders_; @@ -521,7 +511,6 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport { VideoSendParameters send_params_; VideoOptions default_send_options_; VideoRecvParameters recv_params_; - bool red_disabled_by_remote_side_; int64_t last_stats_log_ms_; }; diff --git a/webrtc/media/engine/webrtcvideoengine2_unittest.cc b/webrtc/media/engine/webrtcvideoengine2_unittest.cc index 71156a6db0..d42afcefb3 100644 --- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc +++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc @@ -2641,7 +2641,7 @@ TEST_F(WebRtcVideoChannel2Test, SetRecvCodecsWithoutFecDisablesFec) { << "SetSendCodec without FEC should disable current FEC."; } -TEST_F(WebRtcVideoChannel2Test, SetSendParamsWithoutFecDisablesReceivingFec) { +TEST_F(WebRtcVideoChannel2Test, SetSendParamsWithFecEnablesFec) { FakeVideoReceiveStream* stream = AddRecvStream(); EXPECT_EQ(kUlpfecCodec.id, stream->GetConfig().rtp.ulpfec.ulpfec_payload_type); @@ -2654,22 +2654,16 @@ TEST_F(WebRtcVideoChannel2Test, SetSendParamsWithoutFecDisablesReceivingFec) { stream = fake_call_->GetVideoReceiveStreams()[0]; ASSERT_TRUE(stream != NULL); EXPECT_EQ(kUlpfecCodec.id, stream->GetConfig().rtp.ulpfec.ulpfec_payload_type) - << "FEC should be enabled on the recieve stream."; + << "FEC should be enabled on the receive stream."; cricket::VideoSendParameters send_parameters; send_parameters.codecs.push_back(kVp8Codec); - ASSERT_TRUE(channel_->SetSendParameters(send_parameters)); - stream = fake_call_->GetVideoReceiveStreams()[0]; - EXPECT_EQ(-1, stream->GetConfig().rtp.ulpfec.ulpfec_payload_type) - << "FEC should have been disabled when we know the other side won't do " - "FEC."; - send_parameters.codecs.push_back(kRedCodec); send_parameters.codecs.push_back(kUlpfecCodec); ASSERT_TRUE(channel_->SetSendParameters(send_parameters)); stream = fake_call_->GetVideoReceiveStreams()[0]; EXPECT_EQ(kUlpfecCodec.id, stream->GetConfig().rtp.ulpfec.ulpfec_payload_type) - << "FEC should be enabled on the recieve stream."; + << "FEC should be enabled on the receive stream."; } TEST_F(WebRtcVideoChannel2Test, SetSendCodecsRejectDuplicateFecPayloads) { diff --git a/webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h b/webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h index b0975a05bd..fea2293b85 100644 --- a/webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h +++ b/webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h @@ -13,8 +13,10 @@ #include #include +#include #include "webrtc/base/criticalsection.h" +#include "webrtc/base/deprecation.h" #include "webrtc/modules/rtp_rtcp/source/rtp_receiver_strategy.h" #include "webrtc/modules/rtp_rtcp/source/rtp_utility.h" @@ -87,7 +89,7 @@ class RTPPayloadRegistry { const uint8_t* packet, size_t* packet_length, uint32_t original_ssrc, - const RTPHeader& header) const; + const RTPHeader& header); bool IsRed(const RTPHeader& header) const; @@ -137,15 +139,7 @@ class RTPPayloadRegistry { return last_received_media_payload_type_; } - bool use_rtx_payload_mapping_on_restore() const { - rtc::CritScope cs(&crit_sect_); - return use_rtx_payload_mapping_on_restore_; - } - - void set_use_rtx_payload_mapping_on_restore(bool val) { - rtc::CritScope cs(&crit_sect_); - use_rtx_payload_mapping_on_restore_ = val; - } + RTC_DEPRECATED void set_use_rtx_payload_mapping_on_restore(bool val) {} private: // Prunes the payload type map of the specific payload type, if it exists. @@ -167,15 +161,12 @@ class RTPPayloadRegistry { int8_t last_received_payload_type_; int8_t last_received_media_payload_type_; bool rtx_; - // TODO(changbin): Remove rtx_payload_type_ once interop with old clients that - // only understand one RTX PT is no longer needed. - int rtx_payload_type_; // Mapping rtx_payload_type_map_[rtx] = associated. std::map rtx_payload_type_map_; - // When true, use rtx_payload_type_map_ when restoring RTX packets to get the - // correct payload type. - bool use_rtx_payload_mapping_on_restore_; uint32_t ssrc_rtx_; + // Only warn once per payload type, if an RTX packet is received but + // no associated payload type found in |rtx_payload_type_map_|. + std::set payload_types_with_suppressed_warnings_ GUARDED_BY(crit_sect_); }; } // namespace webrtc diff --git a/webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc b/webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc index 283c2846e1..84b200ccc9 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc @@ -23,7 +23,6 @@ RTPPayloadRegistry::RTPPayloadRegistry(RTPPayloadStrategy* rtp_payload_strategy) last_received_payload_type_(-1), last_received_media_payload_type_(-1), rtx_(false), - rtx_payload_type_(-1), ssrc_rtx_(0) {} RTPPayloadRegistry::~RTPPayloadRegistry() { @@ -234,7 +233,7 @@ bool RTPPayloadRegistry::RestoreOriginalPacket(uint8_t* restored_packet, const uint8_t* packet, size_t* packet_length, uint32_t original_ssrc, - const RTPHeader& header) const { + const RTPHeader& header) { if (kRtxHeaderSize + header.headerLength + header.paddingLength > *packet_length) { return false; @@ -258,21 +257,22 @@ bool RTPPayloadRegistry::RestoreOriginalPacket(uint8_t* restored_packet, if (!rtx_) return true; - int associated_payload_type; auto apt_mapping = rtx_payload_type_map_.find(header.payloadType); - if (apt_mapping == rtx_payload_type_map_.end()) + if (apt_mapping == rtx_payload_type_map_.end()) { + // No associated payload type found. Warn, unless we have already done so. + if (payload_types_with_suppressed_warnings_.find(header.payloadType) == + payload_types_with_suppressed_warnings_.end()) { + LOG(LS_WARNING) + << "No RTX associated payload type mapping was available; " + "not able to restore original packet from RTX packet " + "with payload type: " + << static_cast(header.payloadType) << ". " + << "Suppressing further warnings for this payload type."; + payload_types_with_suppressed_warnings_.insert(header.payloadType); + } return false; - associated_payload_type = apt_mapping->second; - if (red_payload_type_ != -1) { - // Assume red will be used if it's configured. - // This is a workaround for a Chrome sdp bug where rtx is associated - // with the media codec even though media is sent over red. - // TODO(holmer): Remove once the Chrome versions exposing this bug are - // old enough, which should be once Chrome Stable reaches M53 as this - // work-around went into M50. - associated_payload_type = red_payload_type_; } - restored_packet[1] = static_cast(associated_payload_type); + restored_packet[1] = static_cast(apt_mapping->second); if (header.markerBit) { restored_packet[1] |= kRtpMarkerBitMask; // Marker bit is set. } @@ -301,7 +301,6 @@ void RTPPayloadRegistry::SetRtxPayloadType(int payload_type, rtx_payload_type_map_[payload_type] = associated_payload_type; rtx_ = true; - rtx_payload_type_ = payload_type; } bool RTPPayloadRegistry::IsRed(const RTPHeader& header) const { diff --git a/webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc index fa2b3a811e..abb2ae91c4 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc @@ -350,26 +350,6 @@ TEST_F(RtpPayloadRegistryTest, InvalidRtxConfiguration) { TestRtxPacket(rtp_payload_registry_.get(), 106, 0, false); } -TEST_F(RtpPayloadRegistryTest, AssumeRtxWrappingRed) { - rtp_payload_registry_->SetRtxSsrc(100); - // Succeeds when the mapping is used, but fails for the implicit fallback. - rtp_payload_registry_->SetRtxPayloadType(105, 95); - // Set the incoming payload type to 96, which we assume is red. - RTPHeader header; - header.payloadType = 96; - header.ssrc = 1; - rtp_payload_registry_->SetIncomingPayloadType(header); - // Recovers with correct, but unexpected, payload type since we haven't - // configured red. - TestRtxPacket(rtp_payload_registry_.get(), 105, 95, true); - bool created_new_payload; - rtp_payload_registry_->RegisterReceivePayload( - "RED", header.payloadType, 90000, 1, 0, &created_new_payload); - // Now that red is configured we expect to get the red payload type back on - // recovery because of the workaround to always recover red when configured. - TestRtxPacket(rtp_payload_registry_.get(), 105, header.payloadType, true); -} - INSTANTIATE_TEST_CASE_P(TestDynamicRange, RtpPayloadRegistryGenericTest, testing::Range(96, 127 + 1)); diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc index 36915c53af..0a244d13e9 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc @@ -1158,16 +1158,12 @@ std::unique_ptr RTPSender::BuildRtxPacket( rtc::CritScope lock(&send_critsect_); if (!sending_media_) return nullptr; - // Replace payload type, if a specific type is set for RTX. - auto kv = rtx_payload_type_map_.find(packet.PayloadType()); - // Use rtx mapping associated with media codec if we can't find one, - // assume it's red. - // TODO(holmer): Remove once old Chrome versions don't rely on this. + // Replace payload type. + auto kv = rtx_payload_type_map_.find(packet.PayloadType()); if (kv == rtx_payload_type_map_.end()) - kv = rtx_payload_type_map_.find(payload_type_); - if (kv != rtx_payload_type_map_.end()) - rtx_packet->SetPayloadType(kv->second); + return nullptr; + rtx_packet->SetPayloadType(kv->second); // Replace sequence number. rtx_packet->SetSequenceNumber(sequence_number_rtx_++); diff --git a/webrtc/video/rtp_stream_receiver.cc b/webrtc/video/rtp_stream_receiver.cc index 576543bef6..138ed84583 100644 --- a/webrtc/video/rtp_stream_receiver.cc +++ b/webrtc/video/rtp_stream_receiver.cc @@ -152,22 +152,15 @@ RtpStreamReceiver::RtpStreamReceiver( kv.first); } - // If set to true, the RTX payload type mapping supplied in - // |SetRtxPayloadType| will be used when restoring RTX packets. Without it, - // RTX packets will always be restored to the last non-RTX packet payload type - // received. - // TODO(holmer): When Chrome no longer depends on this being false by default, - // always use the mapping and remove this whole codepath. - rtp_payload_registry_.set_use_rtx_payload_mapping_on_restore( - config_.rtp.use_rtx_payload_mapping_on_restore); - if (IsFecEnabled()) { VideoCodec ulpfec_codec = {}; ulpfec_codec.codecType = kVideoCodecULPFEC; strncpy(ulpfec_codec.plName, "ulpfec", sizeof(ulpfec_codec.plName)); ulpfec_codec.plType = config_.rtp.ulpfec.ulpfec_payload_type; RTC_CHECK(SetReceiveCodec(ulpfec_codec)); + } + if (IsRedEnabled()) { VideoCodec red_codec = {}; red_codec.codecType = kVideoCodecRED; strncpy(red_codec.plName, "red", sizeof(red_codec.plName)); @@ -178,11 +171,6 @@ RtpStreamReceiver::RtpStreamReceiver( config_.rtp.ulpfec.red_rtx_payload_type, 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); } if (config_.rtp.rtcp_xr.receiver_reference_time_report) @@ -339,8 +327,11 @@ int32_t RtpStreamReceiver::SliceLossIndicationRequest( } bool RtpStreamReceiver::IsFecEnabled() const { - return config_.rtp.ulpfec.red_payload_type != -1 && - config_.rtp.ulpfec.ulpfec_payload_type != -1; + return config_.rtp.ulpfec.ulpfec_payload_type != -1; +} + +bool RtpStreamReceiver::IsRedEnabled() const { + return config_.rtp.ulpfec.red_payload_type != -1; } bool RtpStreamReceiver::IsRetransmissionsEnabled() const { diff --git a/webrtc/video/rtp_stream_receiver.h b/webrtc/video/rtp_stream_receiver.h index 0a3b3159a1..7f0f3722e9 100644 --- a/webrtc/video/rtp_stream_receiver.h +++ b/webrtc/video/rtp_stream_receiver.h @@ -126,6 +126,7 @@ class RtpStreamReceiver : public RtpData, public RtpFeedback, bool IsPacketRetransmitted(const RTPHeader& header, bool in_order) const; void UpdateHistograms(); void EnableReceiveRtpHeaderExtension(const std::string& extension, int id); + bool IsRedEnabled() const; Clock* const clock_; // Ownership of this object lies with VideoReceiveStream, which owns |this|. diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc index 26082abf36..aac73aba98 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -942,59 +942,65 @@ EncodedImageCallback::Result VideoSendStreamImpl::OnEncodedImage( 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; - const int red_payload_type = config_->rtp.ulpfec.red_payload_type; + + const bool nack_enabled = config_->rtp.nack.rtp_history_ms > 0; + int red_payload_type = config_->rtp.ulpfec.red_payload_type; int ulpfec_payload_type = config_->rtp.ulpfec.ulpfec_payload_type; + + // Shorthands. + auto IsRedEnabled = [&]() { return red_payload_type >= 0; }; + auto IsUlpfecEnabled = [&]() { return ulpfec_payload_type >= 0; }; + auto DisableUlpfec = [&]() { 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 - // that this is not the case with FLEXFEC. - if (enable_protection_nack && + // without retransmitting FEC, so using ULPFEC + NACK for H.264 (for instance) + // is a waste of bandwidth since FEC packets still have to be transmitted. + // Note that this is not the case with FlexFEC. + if (nack_enabled && IsUlpfecEnabled() && !PayloadTypeSupportsSkippingFecPackets( config_->encoder_settings.payload_name)) { - 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."; - ulpfec_payload_type = -1; + LOG(LS_WARNING) + << "Transmitting payload type without picture ID using " + "NACK+ULPFEC is a waste of bandwidth since ULPFEC packets " + "also have to be retransmitted. Disabling ULPFEC."; + DisableUlpfec(); } - // TODO(brandtr): Remove the workaround described below. + // Verify payload types. // - // 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. - - // Verify validity of provided payload types. - if (red_payload_type != -1) { + // Due to how old receivers work, we need to always send RED if it has been + // negotiated. This is a remnant of an old RED/RTX workaround, see + // https://codereview.webrtc.org/2469093003. + // TODO(brandtr): This change went into M56, so we can remove it in ~M59. + // At that time, we can disable RED whenever ULPFEC is disabled, as there is + // no point in using RED without ULPFEC. + if (IsRedEnabled()) { RTC_DCHECK_GE(red_payload_type, 0); RTC_DCHECK_LE(red_payload_type, 127); } - if (ulpfec_payload_type != -1) { + if (IsUlpfecEnabled()) { RTC_DCHECK_GE(ulpfec_payload_type, 0); RTC_DCHECK_LE(ulpfec_payload_type, 127); + if (!IsRedEnabled()) { + LOG(LS_WARNING) + << "ULPFEC is enabled but RED is disabled. Disabling ULPFEC."; + DisableUlpfec(); + } } for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) { // Set NACK. rtp_rtcp->SetStorePacketsStatus( - enable_protection_nack || congestion_controller_->pacer(), + nack_enabled || congestion_controller_->pacer(), kMinSendSidePacketHistorySize); - // Set FEC. + // Set RED/ULPFEC information. for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) { 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); + protection_bitrate_calculator_.SetProtectionMethod(IsUlpfecEnabled(), + nack_enabled); } void VideoSendStreamImpl::ConfigureSsrcs() { diff --git a/webrtc/video/video_send_stream_tests.cc b/webrtc/video/video_send_stream_tests.cc index a601a10f60..fc805cbd57 100644 --- a/webrtc/video/video_send_stream_tests.cc +++ b/webrtc/video/video_send_stream_tests.cc @@ -518,18 +518,18 @@ TEST_F(VideoSendStreamTest, DoesNotUtilizeUlpfecForH264WithNackEnabled) { } // Without retransmissions FEC for H264 is fine. -TEST_F(VideoSendStreamTest, DoesUtilizeRedForH264WithoutNackEnabled) { +TEST_F(VideoSendStreamTest, DoesUtilizeUlpfecForH264WithoutNackEnabled) { UlpfecObserver test(false, false, true, true, "H264"); RunBaseTest(&test); } -TEST_F(VideoSendStreamTest, DoesUtilizeRedForVp8WithNackEnabled) { +TEST_F(VideoSendStreamTest, DoesUtilizeUlpfecForVp8WithNackEnabled) { UlpfecObserver test(false, true, true, true, "VP8"); RunBaseTest(&test); } #if !defined(RTC_DISABLE_VP9) -TEST_F(VideoSendStreamTest, DoesUtilizeRedForVp9WithNackEnabled) { +TEST_F(VideoSendStreamTest, DoesUtilizeUlpfecForVp9WithNackEnabled) { UlpfecObserver test(false, true, true, true, "VP9"); RunBaseTest(&test); } diff --git a/webrtc/video_receive_stream.h b/webrtc/video_receive_stream.h index 7049afc48f..848267fc49 100644 --- a/webrtc/video_receive_stream.h +++ b/webrtc/video_receive_stream.h @@ -152,11 +152,6 @@ class VideoReceiveStream { typedef std::map RtxMap; RtxMap rtx; - // If set to true, the RTX payload type mapping supplied in |rtx| will be - // used when restoring RTX packets. Without it, RTX packets will always be - // restored to the last non-RTX packet payload type received. - bool use_rtx_payload_mapping_on_restore = false; - // RTP header extensions used for the received stream. std::vector extensions; } rtp;