Reimplemented fix for bogus RTP timestamp in RTCP packet created before RTP packet.
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}
diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc
index 7113807..d4c1cd1 100644
--- a/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc
@@ -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 @@
}
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 @@
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 @@
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 @@
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.
diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender.h b/webrtc/modules/rtp_rtcp/source/rtcp_sender.h
index 6f94de5..58f19b0 100644
--- a/webrtc/modules/rtp_rtcp/source/rtcp_sender.h
+++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender.h
@@ -156,8 +156,7 @@
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,
diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc
index a4d6e59..ec83308 100644
--- a/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender_unittest.cc
@@ -221,6 +221,8 @@
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 @@
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 @@
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 @@
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, 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 @@
// 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 @@
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);
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
index dbd919d..40e73eb 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
@@ -207,13 +207,8 @@
}
}
- // 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
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc
index 1e2cc61..9dfcc13 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc
@@ -297,7 +297,9 @@
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 @@
// 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 @@
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 @@
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 @@
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));
diff --git a/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc
index 21a6654..1f4ed77 100644
--- a/webrtc/video/end_to_end_tests.cc
+++ b/webrtc/video/end_to_end_tests.cc
@@ -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 @@
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 @@
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 @@
}
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 @@
}
}
- 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 @@
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 @@
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 @@
}
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) {
