Reland of Use RtxReceiveStream. (patchset #1 id:1 of https://codereview.webrtc.org/3007303002/ )

Reason for revert:
Identified a configuration problem in the video quality tests. Intend to fix and reland.

Original issue's description:
> Revert of Use RtxReceiveStream. (patchset #5 id:320001 of https://codereview.webrtc.org/3006063002/ )
>
> Reason for revert:
> This change appears to break ulpfec, with severe regressions, e.g., for webrtc_perf_test FullStackTest.ForemanCifPlr5Ulpfec
>
> Original issue's description:
> > Reland of Use RtxReceiveStream. (patchset #1 id:1 of https://codereview.webrtc.org/3010983002/ )
> >
> > Reason for revert:
> > Intend to fix perf failures and reland.
> >
> > Original issue's description:
> > > 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: 5c0f6c62ea
> > >
> > > 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}
> > > Committed: 3c39c0137a
> >
> > 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
> > BUG=webrtc:7135
> >
> > Review-Url: https://codereview.webrtc.org/3006063002
> > Cr-Commit-Position: refs/heads/master@{#19715}
> > Committed: 35713eaf56
>
> TBR=brandtr@webrtc.org,danilchap@webrtc.org,stefan@webrtc.org,magjed@webrtc.org
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG=webrtc:7135
>
> Review-Url: https://codereview.webrtc.org/3007303002
> Cr-Commit-Position: refs/heads/master@{#19744}
> Committed: 8e7eee0351

TBR=brandtr@webrtc.org,danilchap@webrtc.org,stefan@webrtc.org,magjed@webrtc.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=webrtc:7135

Review-Url: https://codereview.webrtc.org/3012963002
Cr-Commit-Position: refs/heads/master@{#19765}
This commit is contained in:
nisse
2017-09-11 02:32:16 -07:00
committed by Commit Bot
parent 8412fd333d
commit ca5706d8b5
14 changed files with 120 additions and 68 deletions

View File

@ -207,8 +207,9 @@ void RampUpTester::ModifyVideoConfigs(
recv_config.rtp.ulpfec.ulpfec_payload_type =
send_config->rtp.ulpfec.ulpfec_payload_type;
if (rtx_) {
recv_config.rtp.ulpfec.red_rtx_payload_type =
send_config->rtp.ulpfec.red_rtx_payload_type;
recv_config.rtp.rtx_associated_payload_types
[send_config->rtp.ulpfec.red_rtx_payload_type] =
send_config->rtp.ulpfec.red_payload_type;
}
}

View File

@ -11,17 +11,21 @@
#include <utility>
#include "webrtc/call/rtx_receive_stream.h"
#include "webrtc/modules/rtp_rtcp/include/receive_statistics.h"
#include "webrtc/modules/rtp_rtcp/source/rtp_packet_received.h"
#include "webrtc/rtc_base/logging.h"
namespace webrtc {
RtxReceiveStream::RtxReceiveStream(RtpPacketSinkInterface* media_sink,
std::map<int, int> associated_payload_types,
uint32_t media_ssrc)
RtxReceiveStream::RtxReceiveStream(
RtpPacketSinkInterface* media_sink,
std::map<int, int> associated_payload_types,
uint32_t media_ssrc,
ReceiveStatistics* rtp_receive_statistics /* = nullptr */)
: media_sink_(media_sink),
associated_payload_types_(std::move(associated_payload_types)),
media_ssrc_(media_ssrc) {
media_ssrc_(media_ssrc),
rtp_receive_statistics_(rtp_receive_statistics) {
if (associated_payload_types_.empty()) {
LOG(LS_WARNING)
<< "RtxReceiveStream created with empty payload type mapping.";
@ -31,6 +35,12 @@ RtxReceiveStream::RtxReceiveStream(RtpPacketSinkInterface* media_sink,
RtxReceiveStream::~RtxReceiveStream() = default;
void RtxReceiveStream::OnRtpPacket(const RtpPacketReceived& rtx_packet) {
if (rtp_receive_statistics_) {
RTPHeader header;
rtx_packet.GetHeader(&header);
rtp_receive_statistics_->IncomingPacket(header, rtx_packet.size(),
false /* retransmitted */);
}
rtc::ArrayView<const uint8_t> payload = rtx_packet.payload();
if (payload.size() < kRtxHeaderSize) {

View File

@ -17,13 +17,20 @@
namespace webrtc {
class ReceiveStatistics;
// This class is responsible for RTX decapsulation. The resulting media packets
// are passed on to a sink representing the associated media stream.
class RtxReceiveStream : public RtpPacketSinkInterface {
public:
RtxReceiveStream(RtpPacketSinkInterface* media_sink,
std::map<int, int> associated_payload_types,
uint32_t media_ssrc);
uint32_t media_ssrc,
// TODO(nisse): Delete this argument, and
// corresponding member variable, by moving the
// responsibility for rtcp feedback to
// RtpStreamReceiverController.
ReceiveStatistics* rtp_receive_statistics = nullptr);
~RtxReceiveStream() override;
// RtpPacketSinkInterface.
void OnRtpPacket(const RtpPacketReceived& packet) override;
@ -35,6 +42,7 @@ class RtxReceiveStream : public RtpPacketSinkInterface {
// TODO(nisse): Ultimately, the media receive stream shouldn't care about the
// ssrc, and we should delete this.
const uint32_t media_ssrc_;
ReceiveStatistics* const rtp_receive_statistics_;
};
} // namespace webrtc

View File

@ -167,6 +167,11 @@ 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.

View File

@ -2187,6 +2187,11 @@ 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(

View File

@ -83,6 +83,32 @@ bool HasRtxCodec(const std::vector<cricket::VideoCodec>& codecs,
return false;
}
// TODO(nisse): Duplicated in call.cc.
const int* FindKeyByValue(const std::map<int, int>& 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<webrtc::VideoFrameBuffer> CreateBlackFrameBuffer(
int width,
int height) {
@ -112,15 +138,6 @@ cricket::MediaConfig GetMediaConfig() {
return media_config;
}
// TODO(nisse): Duplicated in call.cc.
const int* FindKeyByValue(const std::map<int, int>& m, int v) {
for (const auto& kv : m) {
if (kv.second == v)
return &kv.first;
}
return nullptr;
}
} // namespace
namespace cricket {
@ -1316,9 +1333,12 @@ TEST_F(WebRtcVideoChannelTest, RecvStreamWithSimAndRtx) {
cricket::CreateSimWithRtxStreamParams("cname", ssrcs, rtx_ssrcs));
EXPECT_FALSE(
recv_stream->GetConfig().rtp.rtx_associated_payload_types.empty());
EXPECT_EQ(recv_stream->GetConfig().decoders.size(),
recv_stream->GetConfig().rtp.rtx_associated_payload_types.size())
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";
EXPECT_EQ(rtx_ssrcs[0], recv_stream->GetConfig().rtp.rtx_ssrc);
}
@ -1329,6 +1349,12 @@ 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) {
@ -3796,9 +3822,11 @@ TEST_F(WebRtcVideoChannelTest, DefaultReceiveStreamReconfiguresToUseRtx) {
recv_stream = fake_call_->GetVideoReceiveStreams()[0];
EXPECT_FALSE(
recv_stream->GetConfig().rtp.rtx_associated_payload_types.empty());
EXPECT_EQ(recv_stream->GetConfig().decoders.size(),
recv_stream->GetConfig().rtp.rtx_associated_payload_types.size())
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 also for the RED payload type";
EXPECT_EQ(rtx_ssrcs[0], recv_stream->GetConfig().rtp.rtx_ssrc);
}

View File

@ -61,6 +61,9 @@ 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",

View File

@ -1184,7 +1184,9 @@ 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[kSendRtxPayloadType] =
.rtp.rtx_associated_payload_types[(payload_type_ == kRedPayloadType)
? kRtxRedPayloadType
: kSendRtxPayloadType] =
payload_type_;
}
// Configure encoding and decoding with VP8, since generic packetization

View File

@ -86,6 +86,7 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver(
RtcpRttStats* rtt_stats,
PacketRouter* packet_router,
const VideoReceiveStream::Config* config,
ReceiveStatistics* rtp_receive_statistics,
ReceiveStatisticsProxy* receive_stats_proxy,
ProcessThread* process_thread,
NackSender* nack_sender,
@ -102,12 +103,11 @@ RtpVideoStreamReceiver::RtpVideoStreamReceiver(
this,
this,
&rtp_payload_registry_)),
rtp_receive_statistics_(ReceiveStatistics::Create(clock_)),
rtp_receive_statistics_(rtp_receive_statistics),
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(),
rtp_rtcp_(CreateRtpRtcpModule(rtp_receive_statistics_,
transport,
rtt_stats,
receive_stats_proxy,
@ -146,12 +146,8 @@ 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()) {
@ -168,11 +164,6 @@ 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)
@ -495,31 +486,7 @@ void RtpVideoStreamReceiver::ParseAndHandleEncapsulatingHeader(
}
ulpfec_receiver_->ProcessReceivedFec();
} else if (rtp_payload_registry_.IsRtx(header)) {
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<int>(header.payloadType);
return;
}
restored_packet_in_use_ = true;
OnRecoveredPacket(restored_packet_, packet_length);
restored_packet_in_use_ = false;
LOG(LS_WARNING) << "Unexpected RTX packet on media ssrc";
}
}

View File

@ -72,6 +72,7 @@ class RtpVideoStreamReceiver : public RtpData,
RtcpRttStats* rtt_stats,
PacketRouter* packet_router,
const VideoReceiveStream::Config* config,
ReceiveStatistics* rtp_receive_statistics,
ReceiveStatisticsProxy* receive_stats_proxy,
ProcessThread* process_thread,
NackSender* nack_sender,
@ -180,13 +181,11 @@ class RtpVideoStreamReceiver : public RtpData,
const std::unique_ptr<RtpHeaderParser> rtp_header_parser_;
const std::unique_ptr<RtpReceiver> rtp_receiver_;
const std::unique_ptr<ReceiveStatistics> rtp_receive_statistics_;
ReceiveStatistics* const rtp_receive_statistics_;
std::unique_ptr<UlpfecReceiver> ulpfec_receiver_;
rtc::SequencedTaskChecker worker_task_checker_;
bool receiving_ RTC_GUARDED_BY(worker_task_checker_);
uint8_t restored_packet_[IP_PACKET_SIZE] RTC_GUARDED_BY(worker_task_checker_);
bool restored_packet_in_use_ RTC_GUARDED_BY(worker_task_checker_);
int64_t last_packet_log_ms_ RTC_GUARDED_BY(worker_task_checker_);
const std::unique_ptr<RtpRtcp> rtp_rtcp_;

View File

@ -126,11 +126,14 @@ class RtpVideoStreamReceiverTest : public testing::Test {
process_thread_(ProcessThread::Create("TestThread")) {}
void SetUp() {
rtp_video_stream_receiver_.reset(new RtpVideoStreamReceiver(
rtp_receive_statistics_ =
rtc::WrapUnique(ReceiveStatistics::Create(Clock::GetRealTimeClock()));
rtp_video_stream_receiver_ = rtc::MakeUnique<RtpVideoStreamReceiver>(
&mock_transport_, nullptr, &packet_router_, &config_,
nullptr, process_thread_.get(), &mock_nack_sender_,
rtp_receive_statistics_.get(), nullptr, process_thread_.get(),
&mock_nack_sender_,
&mock_key_frame_request_sender_, &mock_on_complete_frame_callback_,
&timing_));
&timing_);
}
WebRtcRTPHeader GetDefaultPacket() {
@ -196,6 +199,7 @@ class RtpVideoStreamReceiverTest : public testing::Test {
PacketRouter packet_router_;
VCMTiming timing_;
std::unique_ptr<ProcessThread> process_thread_;
std::unique_ptr<ReceiveStatistics> rtp_receive_statistics_;
std::unique_ptr<RtpVideoStreamReceiver> rtp_video_stream_receiver_;
};

View File

@ -1555,6 +1555,9 @@ void VideoQualityTest::SetupVideo(Transport* send_transport,
video_send_config_.rtp.ulpfec.ulpfec_payload_type;
it->rtp.ulpfec.red_rtx_payload_type =
video_send_config_.rtp.ulpfec.red_rtx_payload_type;
it->rtp.rtx_associated_payload_types[video_send_config_.rtp.ulpfec
.red_rtx_payload_type] =
video_send_config_.rtp.ulpfec.red_payload_type;
}
} else {
video_receive_configs_[params_.ss.selected_stream]
@ -1566,6 +1569,10 @@ void VideoQualityTest::SetupVideo(Transport* send_transport,
video_receive_configs_[params_.ss.selected_stream]
.rtp.ulpfec.red_rtx_payload_type =
video_send_config_.rtp.ulpfec.red_rtx_payload_type;
video_receive_configs_[params_.ss.selected_stream]
.rtp.rtx_associated_payload_types[video_send_config_.rtp.ulpfec
.red_rtx_payload_type] =
video_send_config_.rtp.ulpfec.red_payload_type;
}
}
}

View File

@ -18,6 +18,7 @@
#include "webrtc/api/optional.h"
#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"
@ -32,6 +33,7 @@
#include "webrtc/rtc_base/checks.h"
#include "webrtc/rtc_base/location.h"
#include "webrtc/rtc_base/logging.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"
@ -88,6 +90,7 @@ VideoReceiveStream::VideoReceiveStream(
"DecodingThread",
rtc::kHighestPriority),
call_stats_(call_stats),
rtp_receive_statistics_(ReceiveStatistics::Create(clock_)),
timing_(new VCMTiming(clock_)),
video_receiver_(clock_, nullptr, this, timing_.get(), this, this),
stats_proxy_(&config_, clock_),
@ -95,6 +98,7 @@ VideoReceiveStream::VideoReceiveStream(
call_stats_->rtcp_rtt_stats(),
packet_router,
&config_,
rtp_receive_statistics_.get(),
&stats_proxy_,
process_thread_,
this, // NackSender
@ -120,7 +124,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(
@ -131,9 +135,12 @@ VideoReceiveStream::VideoReceiveStream(
// Register with RtpStreamReceiverController.
media_receiver_ = receiver_controller->CreateReceiver(
config_.rtp.remote_ssrc, &rtp_video_stream_receiver_);
if (config.rtp.rtx_ssrc) {
if (config_.rtp.rtx_ssrc) {
rtx_receive_stream_ = rtc::MakeUnique<RtxReceiveStream>(
&rtp_video_stream_receiver_, config.rtp.rtx_associated_payload_types,
config_.rtp.remote_ssrc, rtp_receive_statistics_.get());
rtx_receiver_ = receiver_controller->CreateReceiver(
config_.rtp.rtx_ssrc, &rtp_video_stream_receiver_);
config_.rtp.rtx_ssrc, rtx_receive_stream_.get());
}
}

View File

@ -38,6 +38,7 @@ class ProcessThread;
class RTPFragmentationHeader;
class RtpStreamReceiverInterface;
class RtpStreamReceiverControllerInterface;
class RtxReceiveStream;
class VCMTiming;
class VCMJitterEstimator;
@ -125,6 +126,10 @@ class VideoReceiveStream : public webrtc::VideoReceiveStream,
CallStats* const call_stats_;
// Shared by media and rtx stream receivers, since the latter has no RtpRtcp
// module of its own.
const std::unique_ptr<ReceiveStatistics> rtp_receive_statistics_;
std::unique_ptr<VCMTiming> timing_; // Jitter buffer experiment.
vcm::VideoReceiver video_receiver_;
std::unique_ptr<rtc::VideoSinkInterface<VideoFrame>> incoming_video_stream_;
@ -141,6 +146,7 @@ class VideoReceiveStream : public webrtc::VideoReceiveStream,
std::unique_ptr<video_coding::FrameBuffer> frame_buffer_;
std::unique_ptr<RtpStreamReceiverInterface> media_receiver_;
std::unique_ptr<RtxReceiveStream> rtx_receive_stream_;
std::unique_ptr<RtpStreamReceiverInterface> rtx_receiver_;
// Whenever we are in an undecodable state (stream has just started or due to