zqz/platform-external-webrtc
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Reimplemented fix for bogus RTP timestamp in RTCP packet created before RTP packet.

Browse Source 
Now it check if rtp timestamp can be calculating instead of checking number of rtp packets. This way it works for reconfigured streams too.

It also moved deeper into rtcp_sender class to prevent SR no matter the reason it need to be genereated. This way it prevents creating compound rtcp packets that have to start with Sender Report and Sender Reports as response to (mostly theoretical) sr-request rtcp packet.

BUG=webrtc:1600
R=pbos@webrtc.org, stefan@webrtc.org

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

Cr-Commit-Position: refs/heads/master@{#13503}
This commit is contained in:
Danil Chapovalov
2016-07-20 15:26:59 +02:00
parent f39f7d931c
commit 70ffead256
6 changed files with 143 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files

38
webrtc/modules/rtp_rtcp/source/rtcp_sender.cc
View File

@ -12,6 +12,8 @@
#include <string.h> // memcpy
#include <utility>
#include "webrtc/base/checks.h"
#include "webrtc/base/constructormagic.h"
#include "webrtc/base/logging.h"
@ -435,6 +437,8 @@ bool RTCPSender::TimeToSendRTCPReport(bool sendKeyframeBeforeRTP) const {
}
std::unique_ptr<rtcp::RtcpPacket> RTCPSender::BuildSR(const RtcpContext& ctx) {
// Timestamp shouldn't be estimated before first media frame.
RTC_DCHECK_GE(last_frame_capture_time_ms_, 0);
// The timestamp of this RTCP packet should be estimated as the timestamp of
// the frame being captured at this moment. We are calculating that
// timestamp as the last frame's timestamp + the time since the last frame
@ -766,6 +770,28 @@ int32_t RTCPSender::SendCompoundRTCP(
LOG(LS_WARNING) << "Can't send rtcp if it is disabled.";
return -1;
}
// Add all flags as volatile. Non volatile entries will not be overwritten.
// All new volatile flags added will be consumed by the end of this call.
SetFlags(packet_types, true);
// Prevent sending streams to send SR before any media has been sent.
const bool can_calculate_rtp_timestamp = (last_frame_capture_time_ms_ >= 0);
if (!can_calculate_rtp_timestamp) {
bool consumed_sr_flag = ConsumeFlag(kRtcpSr);
bool consumed_report_flag = sending_ && ConsumeFlag(kRtcpReport);
bool sender_report = consumed_report_flag || consumed_sr_flag;
if (sender_report && AllVolatileFlagsConsumed()) {
// This call was for Sender Report and nothing else.
return 0;
}
if (sending_ && method_ == RtcpMode::kCompound) {
// Not allowed to send any RTCP packet without sender report.
return -1;
}
}
if (packet_type_counter_.first_packet_time_ms == -1)
packet_type_counter_.first_packet_time_ms = clock_->TimeInMilliseconds();
// We need to send our NTP even if we haven't received any reports.
uint32_t ntp_sec;
@ -774,7 +800,7 @@ int32_t RTCPSender::SendCompoundRTCP(
RtcpContext context(feedback_state, nack_size, nack_list, repeat, pictureID,
ntp_sec, ntp_frac);
PrepareReport(packet_types, feedback_state);
PrepareReport(feedback_state);
std::unique_ptr<rtcp::RtcpPacket> packet_bye;
@ -818,15 +844,7 @@ int32_t RTCPSender::SendCompoundRTCP(
return bytes_sent == 0 ? -1 : 0;
}
void RTCPSender::PrepareReport(const std::set<RTCPPacketType>& packetTypes,
const FeedbackState& feedback_state) {
// Add all flags as volatile. Non volatile entries will not be overwritten
// and all new volatile flags added will be consumed by the end of this call.
SetFlags(packetTypes, true);
if (packet_type_counter_.first_packet_time_ms == -1)
packet_type_counter_.first_packet_time_ms = clock_->TimeInMilliseconds();
void RTCPSender::PrepareReport(const FeedbackState& feedback_state) {
bool generate_report;
if (IsFlagPresent(kRtcpSr) || IsFlagPresent(kRtcpRr)) {
// Report type already explicitly set, don't automatically populate.

3
webrtc/modules/rtp_rtcp/source/rtcp_sender.h
View File

@ -156,8 +156,7 @@ class RTCPSender {
class RtcpContext;
// Determine which RTCP messages should be sent and setup flags.
void PrepareReport(const std::set<RTCPPacketType>& packetTypes,
const FeedbackState& feedback_state)
void PrepareReport(const FeedbackState& feedback_state)
EXCLUSIVE_LOCKS_REQUIRED(critical_section_rtcp_sender_);
bool AddReportBlock(const FeedbackState& feedback_state,

43
webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc
View File

@ -221,6 +221,8 @@ class TestTransport : public Transport,
namespace {
static const uint32_t kSenderSsrc = 0x11111111;
static const uint32_t kRemoteSsrc = 0x22222222;
static const uint32_t kStartRtpTimestamp = 0x34567;
static const uint32_t kRtpTimestamp = 0x45678;
}
class RtcpSenderTest : public ::testing::Test {
@ -238,6 +240,8 @@ class RtcpSenderTest : public ::testing::Test {
nullptr, nullptr, &test_transport_));
rtcp_sender_->SetSSRC(kSenderSsrc);
rtcp_sender_->SetRemoteSSRC(kRemoteSsrc);
rtcp_sender_->SetStartTimestamp(kStartRtpTimestamp);
rtcp_sender_->SetLastRtpTime(kRtpTimestamp, clock_.TimeInMilliseconds());
}
void InsertIncomingPacket(uint32_t remote_ssrc, uint16_t seq_num) {
@ -285,6 +289,7 @@ TEST_F(RtcpSenderTest, SendSr) {
const uint32_t kOctetCount = 0x23456;
rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize);
RTCPSender::FeedbackState feedback_state = rtp_rtcp_impl_->GetFeedbackState();
rtcp_sender_->SetSendingStatus(feedback_state, true);
feedback_state.packets_sent = kPacketCount;
feedback_state.media_bytes_sent = kOctetCount;
uint32_t ntp_secs;
@ -297,9 +302,43 @@ TEST_F(RtcpSenderTest, SendSr) {
EXPECT_EQ(ntp_frac, parser()->sender_report()->NtpFrac());
EXPECT_EQ(kPacketCount, parser()->sender_report()->PacketCount());
EXPECT_EQ(kOctetCount, parser()->sender_report()->OctetCount());
EXPECT_EQ(kStartRtpTimestamp + kRtpTimestamp,
parser()->sender_report()->RtpTimestamp());
EXPECT_EQ(0, parser()->report_block()->num_packets());
}
TEST_F(RtcpSenderTest, DoNotSendSrBeforeRtp) {
rtcp_sender_.reset(new RTCPSender(false, &clock_, receive_statistics_.get(),
nullptr, nullptr, &test_transport_));
rtcp_sender_->SetSSRC(kSenderSsrc);
rtcp_sender_->SetRemoteSSRC(kRemoteSsrc);
rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize);
rtcp_sender_->SetSendingStatus(feedback_state(), true);
// Sender Report shouldn't be send as an SR nor as a Report.
rtcp_sender_->SendRTCP(feedback_state(), kRtcpSr);
EXPECT_EQ(0, parser()->sender_report()->num_packets());
rtcp_sender_->SendRTCP(feedback_state(), kRtcpReport);
EXPECT_EQ(0, parser()->sender_report()->num_packets());
// Other packets (e.g. Pli) are allowed, even if useless.
EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state(), kRtcpPli));
EXPECT_EQ(1, parser()->pli()->num_packets());
}
TEST_F(RtcpSenderTest, DoNotSendCompundBeforeRtp) {
rtcp_sender_.reset(new RTCPSender(false, &clock_, receive_statistics_.get(),
nullptr, nullptr, &test_transport_));
rtcp_sender_->SetSSRC(kSenderSsrc);
rtcp_sender_->SetRemoteSSRC(kRemoteSsrc);
rtcp_sender_->SetRTCPStatus(RtcpMode::kCompound);
rtcp_sender_->SetSendingStatus(feedback_state(), true);
// In compound mode no packets are allowed (e.g. Pli) because compound mode
// should start with Sender Report.
EXPECT_EQ(-1, rtcp_sender_->SendRTCP(feedback_state(), kRtcpPli));
EXPECT_EQ(0, parser()->pli()->num_packets());
}
TEST_F(RtcpSenderTest, SendRr) {
rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize);
EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state(), kRtcpRr));
@ -688,6 +727,7 @@ TEST_F(RtcpSenderTest, TmmbrIncludedInCompoundPacketIfEnabled) {
TEST_F(RtcpSenderTest, SendTmmbn) {
rtcp_sender_->SetRTCPStatus(RtcpMode::kCompound);
rtcp_sender_->SetSendingStatus(feedback_state(), true);
std::vector<rtcp::TmmbItem> bounding_set;
const uint32_t kBitrateKbps = 32768;
const uint32_t kPacketOh = 40;
@ -714,6 +754,7 @@ TEST_F(RtcpSenderTest, SendTmmbn) {
// situation where this caused confusion.
TEST_F(RtcpSenderTest, SendsTmmbnIfSetAndEmpty) {
rtcp_sender_->SetRTCPStatus(RtcpMode::kCompound);
rtcp_sender_->SetSendingStatus(feedback_state(), true);
std::vector<rtcp::TmmbItem> bounding_set;
rtcp_sender_->SetTMMBN(&bounding_set);
EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state(), kRtcpSr));
@ -768,6 +809,8 @@ TEST_F(RtcpSenderTest, ByeMustBeLast) {
nullptr, nullptr, &mock_transport));
rtcp_sender_->SetSSRC(kSenderSsrc);
rtcp_sender_->SetRemoteSSRC(kRemoteSsrc);
rtcp_sender_->SetStartTimestamp(kStartRtpTimestamp);
rtcp_sender_->SetLastRtpTime(kRtpTimestamp, clock_.TimeInMilliseconds());
// Set up XR VoIP metric to be included with BYE
rtcp_sender_->SetRTCPStatus(RtcpMode::kCompound);

9
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
View File

@ -207,13 +207,8 @@ void ModuleRtpRtcpImpl::Process() {
}
}
// For sending streams, make sure to not send a SR before media has been sent.
if (rtcp_sender_.TimeToSendRTCPReport()) {
RTCPSender::FeedbackState state = GetFeedbackState();
// Prevent sending streams to send SR before any media has been sent.
if (!rtcp_sender_.Sending() || state.packets_sent > 0)
rtcp_sender_.SendRTCP(state, kRtcpReport);
}
if (rtcp_sender_.TimeToSendRTCPReport())
rtcp_sender_.SendRTCP(GetFeedbackState(), kRtcpReport);
if (UpdateRTCPReceiveInformationTimers()) {
// A receiver has timed out

12
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc
View File

@ -297,7 +297,9 @@ TEST_F(RtpRtcpImplTest, Rtt) {
header.headerLength = 12;
receiver_.receive_statistics_->IncomingPacket(header, 100, false);
// Sender module should send a SR.
// Send Frame before sending an SR.
SendFrame(&sender_, kBaseLayerTid);
// Sender module should send an SR.
EXPECT_EQ(0, sender_.impl_->SendRTCP(kRtcpReport));
// Receiver module should send a RR with a response to the last received SR.
@ -343,6 +345,8 @@ TEST_F(RtpRtcpImplTest, RttForReceiverOnly) {
// Sender module should send a response to the last received RTRR (DLRR).
clock_.AdvanceTimeMilliseconds(1000);
// Send Frame before sending a SR.
SendFrame(&sender_, kBaseLayerTid);
EXPECT_EQ(0, sender_.impl_->SendRTCP(kRtcpReport));
// Verify RTT.
@ -462,6 +466,8 @@ TEST_F(RtpRtcpImplTest, SendsInitialNackList) {
const uint16_t kNackLength = 1;
uint16_t nack_list[kNackLength] = {123};
EXPECT_EQ(0U, sender_.RtcpSent().nack_packets);
// Send Frame before sending a compound RTCP that starts with SR.
SendFrame(&sender_, kBaseLayerTid);
EXPECT_EQ(0, sender_.impl_->SendNACK(nack_list, kNackLength));
EXPECT_EQ(1U, sender_.RtcpSent().nack_packets);
EXPECT_THAT(sender_.LastNackListSent(), ElementsAre(123));
@ -472,6 +478,8 @@ TEST_F(RtpRtcpImplTest, SendsExtendedNackList) {
const uint16_t kNackLength = 1;
uint16_t nack_list[kNackLength] = {123};
EXPECT_EQ(0U, sender_.RtcpSent().nack_packets);
// Send Frame before sending a compound RTCP that starts with SR.
SendFrame(&sender_, kBaseLayerTid);
EXPECT_EQ(0, sender_.impl_->SendNACK(nack_list, kNackLength));
EXPECT_EQ(1U, sender_.RtcpSent().nack_packets);
EXPECT_THAT(sender_.LastNackListSent(), ElementsAre(123));
@ -495,6 +503,8 @@ TEST_F(RtpRtcpImplTest, ReSendsNackListAfterRttMs) {
const uint16_t kNackLength = 2;
uint16_t nack_list[kNackLength] = {123, 125};
EXPECT_EQ(0U, sender_.RtcpSent().nack_packets);
// Send Frame before sending a compound RTCP that starts with SR.
SendFrame(&sender_, kBaseLayerTid);
EXPECT_EQ(0, sender_.impl_->SendNACK(nack_list, kNackLength));
EXPECT_EQ(1U, sender_.RtcpSent().nack_packets);
EXPECT_THAT(sender_.LastNackListSent(), ElementsAre(123, 125));

80
webrtc/video/end_to_end_tests.cc
View File

@ -25,6 +25,7 @@
#include "webrtc/modules/include/module_common_types.h"
#include "webrtc/modules/rtp_rtcp/include/rtp_rtcp.h"
#include "webrtc/modules/rtp_rtcp/source/byte_io.h"
#include "webrtc/modules/rtp_rtcp/source/rtcp_packet/rapid_resync_request.h"
#include "webrtc/modules/rtp_rtcp/source/rtcp_utility.h"
#include "webrtc/modules/rtp_rtcp/source/rtp_utility.h"
#include "webrtc/modules/video_coding/codecs/h264/include/h264.h"
@ -111,7 +112,7 @@ class EndToEndTest : public test::CallTest {
void RespectsRtcpMode(RtcpMode rtcp_mode);
void TestXrReceiverReferenceTimeReport(bool enable_rrtr);
void TestSendsSetSsrcs(size_t num_ssrcs, bool send_single_ssrc_first);
void TestRtpStatePreservation(bool use_rtx);
void TestRtpStatePreservation(bool use_rtx, bool provoke_rtcpsr_before_rtp);
void VerifyHistogramStats(bool use_rtx, bool use_red, bool screenshare);
void VerifyNewVideoSendStreamsRespectNetworkState(
MediaType network_to_bring_down,
@ -2965,7 +2966,8 @@ TEST_F(EndToEndTest, DISABLED_RedundantPayloadsTransmittedOnAllSsrcs) {
RunBaseTest(&test);
}
void EndToEndTest::TestRtpStatePreservation(bool use_rtx) {
void EndToEndTest::TestRtpStatePreservation(bool use_rtx,
bool provoke_rtcpsr_before_rtp) {
class RtpSequenceObserver : public test::RtpRtcpObserver {
public:
explicit RtpSequenceObserver(bool use_rtx)
@ -2985,6 +2987,28 @@ void EndToEndTest::TestRtpStatePreservation(bool use_rtx) {
}
private:
void ValidateTimestampGap(uint32_t ssrc,
uint32_t timestamp,
bool only_padding)
EXCLUSIVE_LOCKS_REQUIRED(crit_) {
static const int32_t kMaxTimestampGap = kDefaultTimeoutMs * 90;
auto timestamp_it = last_observed_timestamp_.find(ssrc);
if (timestamp_it == last_observed_timestamp_.end()) {
EXPECT_FALSE(only_padding);
last_observed_timestamp_[ssrc] = timestamp;
} else {
// Verify timestamps are reasonably close.
uint32_t latest_observed = timestamp_it->second;
// Wraparound handling is unnecessary here as long as an int variable
// is used to store the result.
int32_t timestamp_gap = timestamp - latest_observed;
EXPECT_LE(std::abs(timestamp_gap), kMaxTimestampGap)
<< "Gap in timestamps (" << latest_observed << " -> " << timestamp
<< ") too large for SSRC: " << ssrc << ".";
timestamp_it->second = timestamp;
}
}
Action OnSendRtp(const uint8_t* packet, size_t length) override {
RTPHeader header;
EXPECT_TRUE(parser_->Parse(packet, length, &header));
@ -3021,24 +3045,9 @@ void EndToEndTest::TestRtpStatePreservation(bool use_rtx) {
}
}
static const int32_t kMaxTimestampGap = kDefaultTimeoutMs * 90;
auto timestamp_it = last_observed_timestamp_.find(ssrc);
if (timestamp_it == last_observed_timestamp_.end()) {
EXPECT_FALSE(only_padding);
last_observed_timestamp_[ssrc] = timestamp;
} else {
// Verify timestamps are reasonably close.
uint32_t latest_observed = timestamp_it->second;
// Wraparound handling is unnecessary here as long as an int variable
// is used to store the result.
int32_t timestamp_gap = timestamp - latest_observed;
EXPECT_LE(std::abs(timestamp_gap), kMaxTimestampGap)
<< "Gap in timestamps (" << latest_observed << " -> "
<< timestamp << ") too large for SSRC: " << ssrc << ".";
timestamp_it->second = timestamp;
}
rtc::CritScope lock(&crit_);
ValidateTimestampGap(ssrc, timestamp, only_padding);
// Wait for media packets on all ssrcs.
if (!ssrc_observed_[ssrc] && !only_padding) {
ssrc_observed_[ssrc] = true;
@ -3049,6 +3058,19 @@ void EndToEndTest::TestRtpStatePreservation(bool use_rtx) {
return SEND_PACKET;
}
Action OnSendRtcp(const uint8_t* packet, size_t length) override {
test::RtcpPacketParser rtcp_parser;
rtcp_parser.Parse(packet, length);
if (rtcp_parser.sender_report()->num_packets() > 0) {
uint32_t ssrc = rtcp_parser.sender_report()->Ssrc();
uint32_t rtcp_timestamp = rtcp_parser.sender_report()->RtpTimestamp();
rtc::CritScope lock(&crit_);
ValidateTimestampGap(ssrc, rtcp_timestamp, false);
}
return SEND_PACKET;
}
SequenceNumberUnwrapper seq_numbers_unwrapper_;
std::map<uint32_t, std::list<int64_t>> last_observed_seq_numbers_;
std::map<uint32_t, uint32_t> last_observed_timestamp_;
@ -3118,6 +3140,15 @@ void EndToEndTest::TestRtpStatePreservation(bool use_rtx) {
video_send_stream_ =
sender_call_->CreateVideoSendStream(video_send_config_, one_stream);
video_send_stream_->Start();
if (provoke_rtcpsr_before_rtp) {
// Rapid Resync Request forces sending RTCP Sender Report back.
// Using this request speeds up this test because then there is no need
// to wait for a second for periodic Sender Report.
rtcp::RapidResyncRequest force_send_sr_back_request;
rtc::Buffer packet = force_send_sr_back_request.Build();
static_cast<webrtc::test::DirectTransport&>(receive_transport)
.SendRtcp(packet.data(), packet.size());
}
CreateFrameGeneratorCapturer();
frame_generator_capturer_->Start();
@ -3150,13 +3181,18 @@ void EndToEndTest::TestRtpStatePreservation(bool use_rtx) {
}
TEST_F(EndToEndTest, RestartingSendStreamPreservesRtpState) {
TestRtpStatePreservation(false);
TestRtpStatePreservation(false, false);
}
// This test is flaky. See:
// These tests are flaky. See:
// https://bugs.chromium.org/p/webrtc/issues/detail?id=4332
TEST_F(EndToEndTest, DISABLED_RestartingSendStreamPreservesRtpStatesWithRtx) {
TestRtpStatePreservation(true);
TestRtpStatePreservation(true, false);
}
TEST_F(EndToEndTest,
DISABLED_RestartingSendStreamKeepsRtpAndRtcpTimestampsSynced) {
TestRtpStatePreservation(true, true);
}
TEST_F(EndToEndTest, RespectsNetworkState) {