admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Revert of Don't send FEC for H.264 with NACK enabled. (patchset #5 id:80001 of https://codereview.webrtc.org/1687303002/ )

Browse Source 
Reason for revert:
Broke the VerifyHistogramStatsWithRed test on the Windows DrMemory Full bot and Linux Memcheck bot. Please fix the test and reland.

Original issue's description:
> Don't send FEC for H.264 with NACK enabled.
>
> The H.264 does not contain picture IDs and are not sufficient to
> determine that a packet may be skipped. This causes retransmission
> requests for FEC that are currently dropped by the sender (since they
> should be redundant).
>
> The receiver is then unable to continue without having the packet gap
> filled (unlike VP8/VP9 which moves on since it has a consecutive stream
> of picture IDs).
>
> Even if FEC retransmission did work it's a huge waste of bandwidth,
> since it just adds additional overhead that has to be unconditionally
> transmitted. This bandwidth is better used to send higher-quality
> frames.
>
> BUG=webrtc:5264
> R=stefan@webrtc.org
>
> Committed: https://crrev.com/25558ad819b4df41ba51537e26a77480ace1e525
> Cr-Commit-Position: refs/heads/master@{#11601}

TBR=stefan@webrtc.org,pbos@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=webrtc:5264

Review URL: https://codereview.webrtc.org/1692783005

Cr-Commit-Position: refs/heads/master@{#11607}
This commit is contained in:
deadbeef 2016-02-12 12:00:42 -08:00 committed by Commit bot
parent 5e7834e151
commit 29ffdc1a15
4 changed files with 49 additions and 161 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

32
webrtc/video/end_to_end_tests.cc
View File

@ -76,7 +76,7 @@ class EndToEndTest : public test::CallTest {
}
};
void DecodesRetransmittedFrame(bool enable_rtx, bool enable_red);
void DecodesRetransmittedFrame(bool use_rtx, bool use_red);
void ReceivesPliAndRecovers(int rtp_history_ms);
void RespectsRtcpMode(RtcpMode rtcp_mode);
void TestXrReceiverReferenceTimeReport(bool enable_rrtr);
@ -701,20 +701,18 @@ TEST_F(EndToEndTest, DISABLED_ReceivedFecPacketsNotNacked) {
// This test drops second RTP packet with a marker bit set, makes sure it's
// retransmitted and renders. Retransmission SSRCs are also checked.
void EndToEndTest::DecodesRetransmittedFrame(bool enable_rtx, bool enable_red) {
void EndToEndTest::DecodesRetransmittedFrame(bool use_rtx, bool use_red) {
// Must be set high enough to allow the bitrate probing to finish.
static const int kMinProbePackets = 30;
static const int kDroppedFrameNumber = kMinProbePackets + 1;
class RetransmissionObserver : public test::EndToEndTest,
public I420FrameCallback {
public:
RetransmissionObserver(bool enable_rtx, bool enable_red)
explicit RetransmissionObserver(bool use_rtx, bool use_red)
: EndToEndTest(kDefaultTimeoutMs),
payload_type_(GetPayloadType(false, enable_red)),
retransmission_ssrc_(enable_rtx ? kSendRtxSsrcs[0]
: kVideoSendSsrcs[0]),
retransmission_payload_type_(GetPayloadType(enable_rtx, enable_red)),
encoder_(VideoEncoder::Create(VideoEncoder::EncoderType::kVp8)),
payload_type_(GetPayloadType(false, use_red)),
retransmission_ssrc_(use_rtx ? kSendRtxSsrcs[0] : kVideoSendSsrcs[0]),
retransmission_payload_type_(GetPayloadType(use_rtx, use_red)),
marker_bits_observed_(0),
num_packets_observed_(0),
retransmitted_timestamp_(0),
@ -792,12 +790,6 @@ void EndToEndTest::DecodesRetransmittedFrame(bool enable_rtx, bool enable_red) {
(*receive_configs)[0].rtp.rtx[payload_type_].payload_type =
kSendRtxPayloadType;
}
// Configure encoding and decoding with VP8, since generic packetization
// doesn't support FEC with NACK.
RTC_DCHECK_EQ(1u, (*receive_configs)[0].decoders.size());
send_config->encoder_settings.encoder = encoder_.get();
send_config->encoder_settings.payload_name = "VP8";
(*receive_configs)[0].decoders[0].payload_name = "VP8";
}
void PerformTest() override {
@ -820,13 +812,11 @@ void EndToEndTest::DecodesRetransmittedFrame(bool enable_rtx, bool enable_red) {
const int payload_type_;
const uint32_t retransmission_ssrc_;
const int retransmission_payload_type_;
rtc::scoped_ptr<VideoEncoder> encoder_;
const std::string payload_name_;
int marker_bits_observed_;
int num_packets_observed_;
uint32_t retransmitted_timestamp_ GUARDED_BY(&crit_);
bool frame_retransmitted_;
} test(enable_rtx, enable_red);
} test(use_rtx, use_red);
RunBaseTest(&test);
}
@ -2098,10 +2088,6 @@ void EndToEndTest::VerifyHistogramStats(bool use_rtx,
use_rtx_(use_rtx),
use_red_(use_red),
screenshare_(screenshare),
// This test uses NACK, so to send FEC we can't use a fake encoder.
vp8_encoder_(
use_red ? VideoEncoder::Create(VideoEncoder::EncoderType::kVp8)
: nullptr),
sender_call_(nullptr),
receiver_call_(nullptr),
start_runtime_ms_(-1) {}
@ -2141,9 +2127,6 @@ void EndToEndTest::VerifyHistogramStats(bool use_rtx,
if (use_red_) {
send_config->rtp.fec.ulpfec_payload_type = kUlpfecPayloadType;
send_config->rtp.fec.red_payload_type = kRedPayloadType;
send_config->encoder_settings.encoder = vp8_encoder_.get();
send_config->encoder_settings.payload_name = "VP8";
(*receive_configs)[0].decoders[0].payload_name = "VP8";
(*receive_configs)[0].rtp.fec.red_payload_type = kRedPayloadType;
(*receive_configs)[0].rtp.fec.ulpfec_payload_type = kUlpfecPayloadType;
}
@ -2173,7 +2156,6 @@ void EndToEndTest::VerifyHistogramStats(bool use_rtx,
const bool use_rtx_;
const bool use_red_;
const bool screenshare_;
const rtc::scoped_ptr<VideoEncoder> vp8_encoder_;
Call* sender_call_;
Call* receiver_call_;
int64_t start_runtime_ms_;

54
webrtc/video/video_send_stream.cc
View File

@ -107,35 +107,6 @@ std::string VideoSendStream::Config::ToString() const {
namespace {
VideoCodecType PayloadNameToCodecType(const std::string& payload_name) {
if (payload_name == "VP8")
return kVideoCodecVP8;
if (payload_name == "VP9")
return kVideoCodecVP9;
if (payload_name == "H264")
return kVideoCodecH264;
return kVideoCodecGeneric;
}
bool PayloadTypeSupportsSkippingFecPackets(const std::string& payload_name) {
switch (PayloadNameToCodecType(payload_name)) {
case kVideoCodecVP8:
case kVideoCodecVP9:
return true;
case kVideoCodecH264:
case kVideoCodecGeneric:
return false;
case kVideoCodecI420:
case kVideoCodecRED:
case kVideoCodecULPFEC:
case kVideoCodecUnknown:
RTC_NOTREACHED();
return false;
}
RTC_NOTREACHED();
return false;
}
CpuOveruseOptions GetCpuOveruseOptions(bool full_overuse_time) {
CpuOveruseOptions options;
if (full_overuse_time) {
@ -244,19 +215,7 @@ VideoSendStream::VideoSendStream(
// Enable NACK, FEC or both.
const bool enable_protection_nack = config_.rtp.nack.rtp_history_ms > 0;
bool enable_protection_fec = config_.rtp.fec.red_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 &&
!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.";
enable_protection_fec = false;
}
const bool enable_protection_fec = config_.rtp.fec.red_payload_type != -1;
// TODO(changbin): Should set RTX for RED mapping in RTP sender in future.
vie_channel_.SetProtectionMode(enable_protection_nack, enable_protection_fec,
config_.rtp.fec.red_payload_type,
@ -361,8 +320,15 @@ bool VideoSendStream::ReconfigureVideoEncoder(
VideoCodec video_codec;
memset(&video_codec, 0, sizeof(video_codec));
video_codec.codecType =
PayloadNameToCodecType(config_.encoder_settings.payload_name);
if (config_.encoder_settings.payload_name == "VP8") {
video_codec.codecType = kVideoCodecVP8;
} else if (config_.encoder_settings.payload_name == "VP9") {
video_codec.codecType = kVideoCodecVP9;
} else if (config_.encoder_settings.payload_name == "H264") {
video_codec.codecType = kVideoCodecH264;
} else {
video_codec.codecType = kVideoCodecGeneric;
}
switch (config.content_type) {
case VideoEncoderConfig::ContentType::kRealtimeVideo:

118
webrtc/video/video_send_stream_tests.cc
View File

@ -308,55 +308,48 @@ class FakeReceiveStatistics : public NullReceiveStatistics {
StatisticianMap stats_map_;
};
class FecObserver : public test::EndToEndTest {
class FecObserver : public test::SendTest {
public:
FecObserver(bool header_extensions_enabled,
bool use_nack,
bool expect_red,
const std::string& codec)
: EndToEndTest(VideoSendStreamTest::kDefaultTimeoutMs),
payload_name_(codec),
use_nack_(use_nack),
expect_red_(expect_red),
explicit FecObserver(bool header_extensions_enabled)
: SendTest(VideoSendStreamTest::kDefaultTimeoutMs),
send_count_(0),
received_media_(false),
received_fec_(false),
header_extensions_enabled_(header_extensions_enabled) {
if (codec == "H264") {
encoder_.reset(new test::FakeH264Encoder(Clock::GetRealTimeClock()));
} else if (codec == "VP8") {
encoder_.reset(VideoEncoder::Create(VideoEncoder::EncoderType::kVp8));
} else if (codec == "VP9") {
encoder_.reset(VideoEncoder::Create(VideoEncoder::EncoderType::kVp9));
} else {
RTC_NOTREACHED();
}
}
header_extensions_enabled_(header_extensions_enabled) {}
private:
Action OnSendRtp(const uint8_t* packet, size_t length) override {
RTPHeader header;
EXPECT_TRUE(parser_->Parse(packet, length, &header));
++send_count_;
// Send lossy receive reports to trigger FEC enabling.
if (send_count_++ % 2 != 0) {
// Receive statistics reporting having lost 50% of the packets.
FakeReceiveStatistics lossy_receive_stats(
VideoSendStreamTest::kVideoSendSsrcs[0], header.sequenceNumber,
send_count_ / 2, 127);
RTCPSender rtcp_sender(false, Clock::GetRealTimeClock(),
&lossy_receive_stats, nullptr, nullptr,
transport_adapter_.get());
rtcp_sender.SetRTCPStatus(RtcpMode::kReducedSize);
rtcp_sender.SetRemoteSSRC(VideoSendStreamTest::kVideoSendSsrcs[0]);
RTCPSender::FeedbackState feedback_state;
EXPECT_EQ(0, rtcp_sender.SendRTCP(feedback_state, kRtcpRr));
}
int encapsulated_payload_type = -1;
if (header.payloadType == VideoSendStreamTest::kRedPayloadType) {
EXPECT_TRUE(expect_red_);
encapsulated_payload_type = static_cast<int>(packet[header.headerLength]);
if (encapsulated_payload_type !=
VideoSendStreamTest::kFakeVideoSendPayloadType) {
VideoSendStreamTest::kFakeVideoSendPayloadType)
EXPECT_EQ(VideoSendStreamTest::kUlpfecPayloadType,
encapsulated_payload_type);
}
} else {
EXPECT_EQ(VideoSendStreamTest::kFakeVideoSendPayloadType,
header.payloadType);
if (static_cast<size_t>(header.headerLength + header.paddingLength) <
length) {
// Not padding-only, media received outside of RED.
EXPECT_FALSE(expect_red_);
received_media_ = true;
}
}
if (header_extensions_enabled_) {
@ -386,27 +379,14 @@ class FecObserver : public test::EndToEndTest {
}
}
if (send_count_ > 100 && received_media_) {
if (received_fec_ || !expect_red_)
observation_complete_.Set();
}
if (received_media_ && received_fec_ && send_count_ > 100)
observation_complete_.Set();
prev_header_ = header;
return SEND_PACKET;
}
test::PacketTransport* CreateSendTransport(Call* sender_call) override {
// At low RTT (< kLowRttNackMs) -> NACK only, no FEC.
// Configure some network delay.
const int kNetworkDelayMs = 100;
FakeNetworkPipe::Config config;
config.loss_percent = 50;
config.queue_delay_ms = kNetworkDelayMs;
return new test::PacketTransport(sender_call, this,
test::PacketTransport::kSender, config);
}
void ModifyVideoConfigs(
VideoSendStream::Config* send_config,
std::vector<VideoReceiveStream::Config>* receive_configs,
@ -414,17 +394,10 @@ class FecObserver : public test::EndToEndTest {
transport_adapter_.reset(
new internal::TransportAdapter(send_config->send_transport));
transport_adapter_->Enable();
if (use_nack_) {
send_config->rtp.nack.rtp_history_ms =
(*receive_configs)[0].rtp.nack.rtp_history_ms =
VideoSendStreamTest::kNackRtpHistoryMs;
}
send_config->encoder_settings.encoder = encoder_.get();
send_config->encoder_settings.payload_name = payload_name_;
send_config->rtp.fec.red_payload_type =
VideoSendStreamTest::kRedPayloadType;
VideoSendStreamTest::kRedPayloadType;
send_config->rtp.fec.ulpfec_payload_type =
VideoSendStreamTest::kUlpfecPayloadType;
VideoSendStreamTest::kUlpfecPayloadType;
if (header_extensions_enabled_) {
send_config->rtp.extensions.push_back(RtpExtension(
RtpExtension::kAbsSendTime, test::kAbsSendTimeExtensionId));
@ -432,10 +405,6 @@ class FecObserver : public test::EndToEndTest {
RtpExtension(RtpExtension::kTransportSequenceNumber,
test::kTransportSequenceNumberExtensionId));
}
(*receive_configs)[0].rtp.fec.red_payload_type =
send_config->rtp.fec.red_payload_type;
(*receive_configs)[0].rtp.fec.ulpfec_payload_type =
send_config->rtp.fec.ulpfec_payload_type;
}
void PerformTest() override {
@ -443,10 +412,6 @@ class FecObserver : public test::EndToEndTest {
}
rtc::scoped_ptr<internal::TransportAdapter> transport_adapter_;
rtc::scoped_ptr<VideoEncoder> encoder_;
const std::string payload_name_;
const bool use_nack_;
const bool expect_red_;
int send_count_;
bool received_media_;
bool received_fec_;
@ -455,37 +420,14 @@ class FecObserver : public test::EndToEndTest {
};
TEST_F(VideoSendStreamTest, SupportsFecWithExtensions) {
FecObserver test(true, false, true, "VP8");
FecObserver test(true);
RunBaseTest(&test);
}
TEST_F(VideoSendStreamTest, SupportsFecWithoutExtensions) {
FecObserver test(false, false, true, "VP8");
RunBaseTest(&test);
}
FecObserver test(false);
// The FEC scheme used is not efficient for H264, so we should not use RED/FEC
// since we'll still have to re-request FEC packets, effectively wasting
// bandwidth since the receiver has to wait for FEC retransmissions to determine
// that the received state is actually decodable.
TEST_F(VideoSendStreamTest, DoesNotUtilizeRedForH264WithNackEnabled) {
FecObserver test(false, true, false, "H264");
RunBaseTest(&test);
}
// Without retransmissions FEC for H264 is fine.
TEST_F(VideoSendStreamTest, DoesUtilizeRedForH264WithoutNackEnabled) {
FecObserver test(false, false, true, "H264");
RunBaseTest(&test);
}
TEST_F(VideoSendStreamTest, DoesUtilizeRedForVp8WithNackEnabled) {
FecObserver test(false, true, true, "VP8");
RunBaseTest(&test);
}
TEST_F(VideoSendStreamTest, DoesUtilizeRedForVp9WithNackEnabled) {
FecObserver test(false, true, true, "VP9");
RunBaseTest(&test);
}

6
webrtc/video/vie_channel.cc
View File

@ -414,15 +414,13 @@ void ViEChannel::SetProtectionMode(bool enable_nack,
bool enable_fec,
int payload_type_red,
int payload_type_fec) {
// 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.
if (enable_fec || payload_type_red != -1 || payload_type_fec != -1) {
// Validate payload types.
if (enable_fec) {
RTC_DCHECK_GE(payload_type_red, 0);
RTC_DCHECK_GE(payload_type_fec, 0);
RTC_DCHECK_LE(payload_type_red, 127);
RTC_DCHECK_LE(payload_type_fec, 127);
} else {
// Payload types unset.
RTC_DCHECK_EQ(payload_type_red, -1);
RTC_DCHECK_EQ(payload_type_fec, -1);
// Set to valid uint8_ts to be castable later without signed overflows.