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

Reland "Make RTCP cumulative_lost be a signed value"

Browse Source 
Instead of modifying the API, we'll add a new function to return
the true value, and have a shim that returns what other code expects.

> This reverts commit 4c34f435db2b921b82b8be19ee5c1746f46cb188.
>
> Reason for revert: Broke internal projects. Type mismatch.
>
> Original change's description:
> > Make RTCP cumulative_lost be a signed value
> >
> > This is formally defined as a signed 24-bit value in RFC 3550 section 6.4.1.
> > See RFC 3550 Appendix A.3 for the reason why it may turn negative.
> >
> > Noticed on discuss-webrtc mailing list.
> >
> > BUG=webrtc:8626
> >
> > Change-Id: I7317f73e9490a876e8445bd3d6b66095ce53ca0a
> > Reviewed-on: https://webrtc-review.googlesource.com/30901
> > Reviewed-by: Stefan Holmer <stefan@webrtc.org>
> > Commit-Queue: Harald Alvestrand <hta@webrtc.org>
> > Cr-Commit-Position: refs/heads/master@{#21142}
>
> TBR=stefan@webrtc.org,hta@webrtc.org
>
> Change-Id: I544f7979d584cfb72a2d0d526f4fef84aebeecb3
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: webrtc:8626
> Reviewed-on: https://webrtc-review.googlesource.com/31040
> Reviewed-by: Zhi Huang <zhihuang@webrtc.org>
> Commit-Queue: Zhi Huang <zhihuang@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#21144}

Change-Id: I95c8c248f4f85c4d1aa2a47424d8c4d954d4ae7a
Bug: webrtc:8626
Reviewed-on: https://webrtc-review.googlesource.com/31220
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Stefan Holmer <stefan@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#21154}
This commit is contained in:
Harald Alvestrand 2017-12-08 08:59:07 +01:00 committed by Commit Bot
parent e08cf3a615
commit 70206d6608
7 changed files with 45 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
modules/rtp_rtcp/include/rtp_rtcp_defines.h
View File

@ -179,7 +179,7 @@ struct RTCPReportBlock {
RTCPReportBlock(uint32_t sender_ssrc,
uint32_t source_ssrc,
uint8_t fraction_lost,
uint32_t packets_lost,
int32_t packets_lost,
uint32_t extended_highest_sequence_number,
uint32_t jitter,
uint32_t last_sender_report_timestamp,

4
modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc
View File

@ -27,7 +27,7 @@ namespace {
const uint32_t kSenderSsrc = 0x12345678;
const uint32_t kRemoteSsrc = 0x23456789;
const uint8_t kFractionLost = 55;
const uint32_t kCumulativeLost = 0x111213;
const int32_t kCumulativeLost = 0x111213;
const uint32_t kExtHighestSeqNum = 0x22232425;
const uint32_t kJitter = 0x33343536;
const uint32_t kLastSr = 0x44454647;
@ -51,7 +51,7 @@ TEST(RtcpPacketReceiverReportTest, ParseWithOneReportBlock) {
const ReportBlock& rb = parsed.report_blocks().front();
EXPECT_EQ(kRemoteSsrc, rb.source_ssrc());
EXPECT_EQ(kFractionLost, rb.fraction_lost());
EXPECT_EQ(kCumulativeLost, rb.cumulative_lost());
EXPECT_EQ(kCumulativeLost, rb.cumulative_lost_signed());
EXPECT_EQ(kExtHighestSeqNum, rb.extended_high_seq_num());
EXPECT_EQ(kJitter, rb.jitter());
EXPECT_EQ(kLastSr, rb.last_sr());

18
modules/rtp_rtcp/source/rtcp_packet/report_block.cc
View File

@ -65,19 +65,21 @@ bool ReportBlock::Parse(const uint8_t* buffer, size_t length) {
void ReportBlock::Create(uint8_t* buffer) const {
// Runtime check should be done while setting cumulative_lost.
RTC_DCHECK_LT(cumulative_lost(), (1 << 24)); // Have only 3 bytes for it.
RTC_DCHECK_LT(cumulative_lost_signed(),
(1 << 23)); // Have only 3 bytes for it.
ByteWriter<uint32_t>::WriteBigEndian(&buffer[0], source_ssrc());
ByteWriter<uint8_t>::WriteBigEndian(&buffer[4], fraction_lost());
ByteWriter<uint32_t, 3>::WriteBigEndian(&buffer[5], cumulative_lost());
ByteWriter<int32_t, 3>::WriteBigEndian(&buffer[5], cumulative_lost_signed());
ByteWriter<uint32_t>::WriteBigEndian(&buffer[8], extended_high_seq_num());
ByteWriter<uint32_t>::WriteBigEndian(&buffer[12], jitter());
ByteWriter<uint32_t>::WriteBigEndian(&buffer[16], last_sr());
ByteWriter<uint32_t>::WriteBigEndian(&buffer[20], delay_since_last_sr());
}
bool ReportBlock::SetCumulativeLost(uint32_t cumulative_lost) {
if (cumulative_lost >= (1u << 24)) { // Have only 3 bytes to store it.
bool ReportBlock::SetCumulativeLost(int32_t cumulative_lost) {
// We have only 3 bytes to store it, and it's a signed value.
if (cumulative_lost >= (1 << 23) || cumulative_lost < -(1 << 23)) {
RTC_LOG(LS_WARNING)
<< "Cumulative lost is too big to fit into Report Block";
return false;
@ -86,5 +88,13 @@ bool ReportBlock::SetCumulativeLost(uint32_t cumulative_lost) {
return true;
}
uint32_t ReportBlock::cumulative_lost() const {
if (cumulative_lost_ < 0) {
RTC_LOG(LS_VERBOSE) << "Ignoring negative value of cumulative_lost";
return 0;
}
return cumulative_lost_;
}
} // namespace rtcp
} // namespace webrtc

22
modules/rtp_rtcp/source/rtcp_packet/report_block.h
View File

@ -17,6 +17,8 @@
namespace webrtc {
namespace rtcp {
// A ReportBlock represents the Sender Report packet from
// RFC 3550 section 6.4.1.
class ReportBlock {
public:
static const size_t kLength = 24;
@ -34,7 +36,7 @@ class ReportBlock {
void SetFractionLost(uint8_t fraction_lost) {
fraction_lost_ = fraction_lost;
}
bool SetCumulativeLost(uint32_t cumulative_lost);
bool SetCumulativeLost(int32_t cumulative_lost);
void SetExtHighestSeqNum(uint32_t ext_highest_seq_num) {
extended_high_seq_num_ = ext_highest_seq_num;
}
@ -46,20 +48,22 @@ class ReportBlock {
uint32_t source_ssrc() const { return source_ssrc_; }
uint8_t fraction_lost() const { return fraction_lost_; }
uint32_t cumulative_lost() const { return cumulative_lost_; }
int32_t cumulative_lost_signed() const { return cumulative_lost_; }
// Deprecated - returns max(0, cumulative_lost_), not negative values.
uint32_t cumulative_lost() const;
uint32_t extended_high_seq_num() const { return extended_high_seq_num_; }
uint32_t jitter() const { return jitter_; }
uint32_t last_sr() const { return last_sr_; }
uint32_t delay_since_last_sr() const { return delay_since_last_sr_; }
private:
uint32_t source_ssrc_;
uint8_t fraction_lost_;
uint32_t cumulative_lost_;
uint32_t extended_high_seq_num_;
uint32_t jitter_;
uint32_t last_sr_;
uint32_t delay_since_last_sr_;
uint32_t source_ssrc_; // 32 bits
uint8_t fraction_lost_; // 8 bits representing a fixed point value 0..1
int32_t cumulative_lost_; // Signed 24-bit value
uint32_t extended_high_seq_num_; // 32 bits
uint32_t jitter_; // 32 bits
uint32_t last_sr_; // 32 bits
uint32_t delay_since_last_sr_; // 32 bits, units of 1/65536 seconds
};
} // namespace rtcp

15
modules/rtp_rtcp/source/rtcp_packet/report_block_unittest.cc
View File

@ -23,7 +23,7 @@ namespace {
const uint32_t kRemoteSsrc = 0x23456789;
const uint8_t kFractionLost = 55;
// Use values that are streamed differently LE and BE.
const uint32_t kCumulativeLost = 0x111213;
const int32_t kCumulativeLost = 0x111213;
const uint32_t kExtHighestSeqNum = 0x22232425;
const uint32_t kJitter = 0x33343536;
const uint32_t kLastSr = 0x44454647;
@ -68,7 +68,7 @@ TEST(RtcpPacketReportBlockTest, ParseMatchCreate) {
EXPECT_EQ(kRemoteSsrc, parsed.source_ssrc());
EXPECT_EQ(kFractionLost, parsed.fraction_lost());
EXPECT_EQ(kCumulativeLost, parsed.cumulative_lost());
EXPECT_EQ(kCumulativeLost, parsed.cumulative_lost_signed());
EXPECT_EQ(kExtHighestSeqNum, parsed.extended_high_seq_num());
EXPECT_EQ(kJitter, parsed.jitter());
EXPECT_EQ(kLastSr, parsed.last_sr());
@ -76,10 +76,19 @@ TEST(RtcpPacketReportBlockTest, ParseMatchCreate) {
}
TEST(RtcpPacketReportBlockTest, ValidateCumulativeLost) {
const uint32_t kMaxCumulativeLost = 0xffffff;
// CumulativeLost is a signed 24-bit integer.
// However, existing code expects it to be an unsigned integer.
// The case of negative values should be unusual; we return 0
// when caller wants an unsigned integer.
const int32_t kMaxCumulativeLost = 0x7fffff;
const int32_t kMinCumulativeLost = -0x800000;
ReportBlock rb;
EXPECT_FALSE(rb.SetCumulativeLost(kMaxCumulativeLost + 1));
EXPECT_TRUE(rb.SetCumulativeLost(kMaxCumulativeLost));
EXPECT_FALSE(rb.SetCumulativeLost(kMinCumulativeLost - 1));
EXPECT_TRUE(rb.SetCumulativeLost(kMinCumulativeLost));
EXPECT_EQ(kMinCumulativeLost, rb.cumulative_lost_signed());
EXPECT_EQ(0u, rb.cumulative_lost());
}
} // namespace

3
modules/rtp_rtcp/source/rtcp_receiver.cc
View File

@ -461,7 +461,8 @@ void RTCPReceiver::HandleReportBlock(const ReportBlock& report_block,
report_block_info->report_block.sender_ssrc = remote_ssrc;
report_block_info->report_block.source_ssrc = report_block.source_ssrc();
report_block_info->report_block.fraction_lost = report_block.fraction_lost();
report_block_info->report_block.packets_lost = report_block.cumulative_lost();
report_block_info->report_block.packets_lost =
report_block.cumulative_lost_signed();
if (report_block.extended_high_seq_num() >
report_block_info->report_block.extended_highest_sequence_number) {
// We have successfully delivered new RTP packets to the remote side after

2
modules/rtp_rtcp/source/rtcp_sender_unittest.cc
View File

@ -358,7 +358,7 @@ TEST_F(RtcpSenderTest, SendRrWithOneReportBlock) {
const rtcp::ReportBlock& rb = parser()->receiver_report()->report_blocks()[0];
EXPECT_EQ(kRemoteSsrc, rb.source_ssrc());
EXPECT_EQ(0U, rb.fraction_lost());
EXPECT_EQ(0U, rb.cumulative_lost());
EXPECT_EQ(0, rb.cumulative_lost_signed());
EXPECT_EQ(kSeqNum, rb.extended_high_seq_num());
}