From 3c39c0137afa274d1d524b150b50304b38a2847b Mon Sep 17 00:00:00 2001 From: nisse Date: Mon, 4 Sep 2017 04:22:15 -0700 Subject: [PATCH] Revert of Use RtxReceiveStream. (patchset #5 id:80001 of https://codereview.webrtc.org/3008773002/ ) Reason for revert: A few perf tests broken, including RampUpTest.UpDownUpAbsSendTimeSimulcastRedRtx RampUpTest.UpDownUpTransportSequenceNumberRtx RampUpTest.UpDownUpTransportSequenceNumberPacketLoss Original issue's description: > Use RtxReceiveStream. > > This also has the beneficial side-effect that when a media stream > which is protected by FlexFEC receives an RTX retransmission, the > retransmitted media packet is passed into the FlexFEC machinery, > which should improve its ability to recover packets via FEC. > > BUG=webrtc:7135 > > Review-Url: https://codereview.webrtc.org/3008773002 > Cr-Commit-Position: refs/heads/master@{#19649} > Committed: https://chromium.googlesource.com/external/webrtc/+/5c0f6c62ea3b1d2c43f8fc152961af27033475f7 TBR=brandtr@webrtc.org,danilchap@webrtc.org,stefan@webrtc.org,magjed@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=webrtc:7135 Review-Url: https://codereview.webrtc.org/3010983002 Cr-Commit-Position: refs/heads/master@{#19653} --- webrtc/call/rampup_tests.cc | 5 +- webrtc/call/video_receive_stream.h | 5 -- webrtc/media/engine/webrtcvideoengine.cc | 5 -- .../engine/webrtcvideoengine_unittest.cc | 54 +++++-------------- webrtc/video/BUILD.gn | 3 -- webrtc/video/end_to_end_tests.cc | 4 +- webrtc/video/rtp_video_stream_receiver.cc | 38 ++++++++++++- webrtc/video/rtp_video_stream_receiver.h | 2 + webrtc/video/video_receive_stream.cc | 11 ++-- webrtc/video/video_receive_stream.h | 2 - 10 files changed, 57 insertions(+), 72 deletions(-) diff --git a/webrtc/call/rampup_tests.cc b/webrtc/call/rampup_tests.cc index 2b85452ac3..ed44dad5e5 100644 --- a/webrtc/call/rampup_tests.cc +++ b/webrtc/call/rampup_tests.cc @@ -207,9 +207,8 @@ void RampUpTester::ModifyVideoConfigs( recv_config.rtp.ulpfec.ulpfec_payload_type = send_config->rtp.ulpfec.ulpfec_payload_type; if (rtx_) { - recv_config.rtp.rtx_associated_payload_types - [send_config->rtp.ulpfec.red_rtx_payload_type] = - send_config->rtp.ulpfec.red_payload_type; + recv_config.rtp.ulpfec.red_rtx_payload_type = + send_config->rtp.ulpfec.red_rtx_payload_type; } } diff --git a/webrtc/call/video_receive_stream.h b/webrtc/call/video_receive_stream.h index 542027e953..ef1c715c73 100644 --- a/webrtc/call/video_receive_stream.h +++ b/webrtc/call/video_receive_stream.h @@ -165,11 +165,6 @@ class VideoReceiveStream { NackConfig nack; // See UlpfecConfig for description. - // TODO(nisse): UlpfecConfig includes the field red_rtx_payload_type, - // which duplicates info in the rtx_associated_payload_types mapping. So - // delete the use of UlpfecConfig here, and replace by the values which - // make sense in this context, likely those are ulpfec_payload_type_ and - // red_payload_type_. UlpfecConfig ulpfec; // SSRC for retransmissions. diff --git a/webrtc/media/engine/webrtcvideoengine.cc b/webrtc/media/engine/webrtcvideoengine.cc index cd34e6b478..a9dc11e421 100644 --- a/webrtc/media/engine/webrtcvideoengine.cc +++ b/webrtc/media/engine/webrtcvideoengine.cc @@ -2188,11 +2188,6 @@ void WebRtcVideoChannel::WebRtcVideoReceiveStream::ConfigureCodecs( config_.rtp.nack.rtp_history_ms = HasNack(recv_codecs.begin()->codec) ? kNackHistoryMs : 0; - if (config_.rtp.ulpfec.red_rtx_payload_type != -1) { - config_.rtp - .rtx_associated_payload_types[config_.rtp.ulpfec.red_rtx_payload_type] = - config_.rtp.ulpfec.red_payload_type; - } } void WebRtcVideoChannel::WebRtcVideoReceiveStream::ConfigureFlexfecCodec( diff --git a/webrtc/media/engine/webrtcvideoengine_unittest.cc b/webrtc/media/engine/webrtcvideoengine_unittest.cc index 1306082abf..c6adfd3976 100644 --- a/webrtc/media/engine/webrtcvideoengine_unittest.cc +++ b/webrtc/media/engine/webrtcvideoengine_unittest.cc @@ -83,32 +83,6 @@ bool HasRtxCodec(const std::vector& codecs, return false; } -// TODO(nisse): Duplicated in call.cc. -const int* FindKeyByValue(const std::map& m, int v) { - for (const auto& kv : m) { - if (kv.second == v) - return &kv.first; - } - return nullptr; -} - -bool HasRtxReceiveAssociation( - const webrtc::VideoReceiveStream::Config& config, - int payload_type) { - return FindKeyByValue(config.rtp.rtx_associated_payload_types, - payload_type) != nullptr; -} - -// Check that there's an Rtx payload type for each decoder. -bool VerifyRtxReceiveAssociations( - const webrtc::VideoReceiveStream::Config& config) { - for (const auto& decoder : config.decoders) { - if (!HasRtxReceiveAssociation(config, decoder.payload_type)) - return false; - } - return true; -} - rtc::scoped_refptr CreateBlackFrameBuffer( int width, int height) { @@ -138,6 +112,15 @@ cricket::MediaConfig GetMediaConfig() { return media_config; } +// TODO(nisse): Duplicated in call.cc. +const int* FindKeyByValue(const std::map& m, int v) { + for (const auto& kv : m) { + if (kv.second == v) + return &kv.first; + } + return nullptr; +} + } // namespace namespace cricket { @@ -1333,12 +1316,9 @@ TEST_F(WebRtcVideoChannelTest, RecvStreamWithSimAndRtx) { cricket::CreateSimWithRtxStreamParams("cname", ssrcs, rtx_ssrcs)); EXPECT_FALSE( recv_stream->GetConfig().rtp.rtx_associated_payload_types.empty()); - EXPECT_TRUE(VerifyRtxReceiveAssociations(recv_stream->GetConfig())) + EXPECT_EQ(recv_stream->GetConfig().decoders.size(), + recv_stream->GetConfig().rtp.rtx_associated_payload_types.size()) << "RTX should be mapped for all decoders/payload types."; - EXPECT_TRUE(HasRtxReceiveAssociation(recv_stream->GetConfig(), - GetEngineCodec("red").id)) - << "RTX should be mapped for the RED payload type"; - EXPECT_EQ(rtx_ssrcs[0], recv_stream->GetConfig().rtp.rtx_ssrc); } @@ -1349,12 +1329,6 @@ TEST_F(WebRtcVideoChannelTest, RecvStreamWithRtx) { params.AddFidSsrc(kSsrcs1[0], kRtxSsrcs1[0]); FakeVideoReceiveStream* recv_stream = AddRecvStream(params); EXPECT_EQ(kRtxSsrcs1[0], recv_stream->GetConfig().rtp.rtx_ssrc); - - EXPECT_TRUE(VerifyRtxReceiveAssociations(recv_stream->GetConfig())) - << "RTX should be mapped for all decoders/payload types."; - EXPECT_TRUE(HasRtxReceiveAssociation(recv_stream->GetConfig(), - GetEngineCodec("red").id)) - << "RTX should be mapped for the RED payload type"; } TEST_F(WebRtcVideoChannelTest, RecvStreamNoRtx) { @@ -3822,11 +3796,9 @@ TEST_F(WebRtcVideoChannelTest, DefaultReceiveStreamReconfiguresToUseRtx) { recv_stream = fake_call_->GetVideoReceiveStreams()[0]; EXPECT_FALSE( recv_stream->GetConfig().rtp.rtx_associated_payload_types.empty()); - EXPECT_TRUE(VerifyRtxReceiveAssociations(recv_stream->GetConfig())) + EXPECT_EQ(recv_stream->GetConfig().decoders.size(), + recv_stream->GetConfig().rtp.rtx_associated_payload_types.size()) << "RTX should be mapped for all decoders/payload types."; - EXPECT_TRUE(HasRtxReceiveAssociation(recv_stream->GetConfig(), - GetEngineCodec("red").id)) - << "RTX should be mapped also for the RED payload type"; EXPECT_EQ(rtx_ssrcs[0], recv_stream->GetConfig().rtp.rtx_ssrc); } diff --git a/webrtc/video/BUILD.gn b/webrtc/video/BUILD.gn index 29f509767c..f3bba0ebf4 100644 --- a/webrtc/video/BUILD.gn +++ b/webrtc/video/BUILD.gn @@ -60,9 +60,6 @@ rtc_static_library("video") { "../call:call_interfaces", "../call:rtp_interfaces", "../call:video_stream_api", - - # For RtxReceiveStream. - "../call:rtp_receiver", "../common_video", "../logging:rtc_event_log_api", "../media:rtc_media_base", diff --git a/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc index 107aed8346..a30b08a320 100644 --- a/webrtc/video/end_to_end_tests.cc +++ b/webrtc/video/end_to_end_tests.cc @@ -1184,9 +1184,7 @@ void EndToEndTest::DecodesRetransmittedFrame(bool enable_rtx, bool enable_red) { send_config->rtp.rtx.payload_type = kSendRtxPayloadType; (*receive_configs)[0].rtp.rtx_ssrc = kSendRtxSsrcs[0]; (*receive_configs)[0] - .rtp.rtx_associated_payload_types[(payload_type_ == kRedPayloadType) - ? kRtxRedPayloadType - : kSendRtxPayloadType] = + .rtp.rtx_associated_payload_types[kSendRtxPayloadType] = payload_type_; } // Configure encoding and decoding with VP8, since generic packetization diff --git a/webrtc/video/rtp_video_stream_receiver.cc b/webrtc/video/rtp_video_stream_receiver.cc index 51171d1aa2..0dbb2f7da0 100644 --- a/webrtc/video/rtp_video_stream_receiver.cc +++ b/webrtc/video/rtp_video_stream_receiver.cc @@ -105,6 +105,7 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver( rtp_receive_statistics_(ReceiveStatistics::Create(clock_)), ulpfec_receiver_(UlpfecReceiver::Create(config->rtp.remote_ssrc, this)), receiving_(false), + restored_packet_in_use_(false), last_packet_log_ms_(-1), rtp_rtcp_(CreateRtpRtcpModule(rtp_receive_statistics_.get(), transport, @@ -145,8 +146,12 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver( rtp_receive_statistics_->SetMaxReorderingThreshold(max_reordering_threshold); if (config_.rtp.rtx_ssrc) { - // Needed for rtp_payload_registry_.RtxEnabled(). rtp_payload_registry_.SetRtxSsrc(config_.rtp.rtx_ssrc); + + for (const auto& kv : config_.rtp.rtx_associated_payload_types) { + RTC_DCHECK_NE(kv.first, 0); + rtp_payload_registry_.SetRtxPayloadType(kv.first, kv.second); + } } if (IsUlpfecEnabled()) { @@ -163,6 +168,11 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver( strncpy(red_codec.plName, "red", sizeof(red_codec.plName)); red_codec.plType = config_.rtp.ulpfec.red_payload_type; RTC_CHECK(AddReceiveCodec(red_codec)); + if (config_.rtp.ulpfec.red_rtx_payload_type != -1) { + rtp_payload_registry_.SetRtxPayloadType( + config_.rtp.ulpfec.red_rtx_payload_type, + config_.rtp.ulpfec.red_payload_type); + } } if (config_.rtp.rtcp_xr.receiver_reference_time_report) @@ -485,7 +495,31 @@ void RtpVideoStreamReceiver::ParseAndHandleEncapsulatingHeader( } ulpfec_receiver_->ProcessReceivedFec(); } else if (rtp_payload_registry_.IsRtx(header)) { - LOG(LS_WARNING) << "Unexpected RTX packet on media ssrc"; + if (header.headerLength + header.paddingLength == packet_length) { + // This is an empty packet and should be silently dropped before trying to + // parse the RTX header. + return; + } + // Remove the RTX header and parse the original RTP header. + if (packet_length < header.headerLength) + return; + if (packet_length > sizeof(restored_packet_)) + return; + if (restored_packet_in_use_) { + LOG(LS_WARNING) << "Multiple RTX headers detected, dropping packet."; + return; + } + if (!rtp_payload_registry_.RestoreOriginalPacket( + restored_packet_, packet, &packet_length, config_.rtp.remote_ssrc, + header)) { + LOG(LS_WARNING) << "Incoming RTX packet: Invalid RTP header ssrc: " + << header.ssrc << " payload type: " + << static_cast(header.payloadType); + return; + } + restored_packet_in_use_ = true; + OnRecoveredPacket(restored_packet_, packet_length); + restored_packet_in_use_ = false; } } diff --git a/webrtc/video/rtp_video_stream_receiver.h b/webrtc/video/rtp_video_stream_receiver.h index 86e2de7d19..15af2e60ee 100644 --- a/webrtc/video/rtp_video_stream_receiver.h +++ b/webrtc/video/rtp_video_stream_receiver.h @@ -185,6 +185,8 @@ class RtpVideoStreamReceiver : public RtpData, rtc::SequencedTaskChecker worker_task_checker_; bool receiving_ GUARDED_BY(worker_task_checker_); + uint8_t restored_packet_[IP_PACKET_SIZE] GUARDED_BY(worker_task_checker_); + bool restored_packet_in_use_ GUARDED_BY(worker_task_checker_); int64_t last_packet_log_ms_ GUARDED_BY(worker_task_checker_); const std::unique_ptr rtp_rtcp_; diff --git a/webrtc/video/video_receive_stream.cc b/webrtc/video/video_receive_stream.cc index 9f5528bb2f..6750c9e0de 100644 --- a/webrtc/video/video_receive_stream.cc +++ b/webrtc/video/video_receive_stream.cc @@ -17,7 +17,6 @@ #include #include "webrtc/call/rtp_stream_receiver_controller_interface.h" -#include "webrtc/call/rtx_receive_stream.h" #include "webrtc/common_types.h" #include "webrtc/common_video/h264/profile_level_id.h" #include "webrtc/common_video/libyuv/include/webrtc_libyuv.h" @@ -33,7 +32,6 @@ #include "webrtc/rtc_base/location.h" #include "webrtc/rtc_base/logging.h" #include "webrtc/rtc_base/optional.h" -#include "webrtc/rtc_base/ptr_util.h" #include "webrtc/rtc_base/trace_event.h" #include "webrtc/system_wrappers/include/clock.h" #include "webrtc/system_wrappers/include/field_trial.h" @@ -122,7 +120,7 @@ VideoReceiveStream::VideoReceiveStream( decoder_payload_types.insert(decoder.payload_type); } - video_receiver_.SetRenderDelay(config_.render_delay_ms); + video_receiver_.SetRenderDelay(config.render_delay_ms); jitter_estimator_.reset(new VCMJitterEstimator(clock_)); frame_buffer_.reset(new video_coding::FrameBuffer( @@ -133,12 +131,9 @@ VideoReceiveStream::VideoReceiveStream( // Register with RtpStreamReceiverController. media_receiver_ = receiver_controller->CreateReceiver( config_.rtp.remote_ssrc, &rtp_video_stream_receiver_); - if (config_.rtp.rtx_ssrc) { - rtx_receive_stream_ = rtc::MakeUnique( - &rtp_video_stream_receiver_, config.rtp.rtx_associated_payload_types, - config_.rtp.remote_ssrc); + if (config.rtp.rtx_ssrc) { rtx_receiver_ = receiver_controller->CreateReceiver( - config_.rtp.rtx_ssrc, rtx_receive_stream_.get()); + config_.rtp.rtx_ssrc, &rtp_video_stream_receiver_); } } diff --git a/webrtc/video/video_receive_stream.h b/webrtc/video/video_receive_stream.h index 2f29c90170..833e189fb1 100644 --- a/webrtc/video/video_receive_stream.h +++ b/webrtc/video/video_receive_stream.h @@ -38,7 +38,6 @@ class ProcessThread; class RTPFragmentationHeader; class RtpStreamReceiverInterface; class RtpStreamReceiverControllerInterface; -class RtxReceiveStream; class VCMTiming; class VCMJitterEstimator; @@ -142,7 +141,6 @@ class VideoReceiveStream : public webrtc::VideoReceiveStream, std::unique_ptr frame_buffer_; std::unique_ptr media_receiver_; - std::unique_ptr rtx_receive_stream_; std::unique_ptr rtx_receiver_; // Whenever we are in an undecodable state (stream has just started or due to