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

Refactor handling of TransportSequenceNumber in PacketRouter

Browse Source 
The use of SetTransportWideSequenceNumber() and AllocateSequenceNumber()
is gone from webrtc, but some downstream code still references them.

This means we can do some simplifications.

The member that stores the sequence number is now always accessed while
holding the modules lock, so we can just use that and don't need to add
atomic operations on top.

SetTransportWideSequenceNumber() is only used to set the start sequence
number, it would be nice to set that in the constructor instead.

AllocateSequnceNumber() is now actually only used as a getter, so this
can be replace by a proper const getter method instead.

Bug: webrtc:11036
Change-Id: I69b06e613ca3361cf24ef835b92dd0a894cbd27e
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/157167
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#29507}
This commit is contained in:
Erik Språng 2019-10-16 17:06:34 +02:00 committed by Commit Bot
parent a6d7b02824
commit 5f01bf6c8b
3 changed files with 33 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

31
modules/pacing/packet_router.cc
View File

@ -21,7 +21,6 @@
#include "modules/rtp_rtcp/include/rtp_rtcp_defines.h"
#include "modules/rtp_rtcp/source/rtcp_packet.h"
#include "modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h"
#include "rtc_base/atomic_ops.h"
#include "rtc_base/checks.h"
#include "rtc_base/logging.h"
#include "rtc_base/time_utils.h"
@ -33,14 +32,16 @@ constexpr int kRembSendIntervalMs = 200;
} // namespace
PacketRouter::PacketRouter()
PacketRouter::PacketRouter() : PacketRouter(0) {}
PacketRouter::PacketRouter(uint16_t start_transport_seq)
: last_send_module_(nullptr),
last_remb_time_ms_(rtc::TimeMillis()),
last_send_bitrate_bps_(0),
bitrate_bps_(0),
max_bitrate_bps_(std::numeric_limits<decltype(max_bitrate_bps_)>::max()),
active_remb_module_(nullptr),
transport_seq_(0) {}
transport_seq_(start_transport_seq) {}
PacketRouter::~PacketRouter() {
RTC_DCHECK(rtp_send_modules_.empty());
@ -185,25 +186,19 @@ std::vector<std::unique_ptr<RtpPacketToSend>> PacketRouter::GeneratePadding(
}
void PacketRouter::SetTransportWideSequenceNumber(uint16_t sequence_number) {
rtc::AtomicOps::ReleaseStore(&transport_seq_, sequence_number);
rtc::CritScope lock(&modules_crit_);
transport_seq_ = sequence_number;
}
uint16_t PacketRouter::AllocateSequenceNumber() {
int prev_seq = rtc::AtomicOps::AcquireLoad(&transport_seq_);
int desired_prev_seq;
int new_seq;
do {
desired_prev_seq = prev_seq;
new_seq = (desired_prev_seq + 1) & 0xFFFF;
// Note: CompareAndSwap returns the actual value of transport_seq at the
// time the CAS operation was executed. Thus, if prev_seq is returned, the
// operation was successful - otherwise we need to retry. Saving the
// return value saves us a load on retry.
prev_seq = rtc::AtomicOps::CompareAndSwap(&transport_seq_, desired_prev_seq,
new_seq);
} while (prev_seq != desired_prev_seq);
rtc::CritScope lock(&modules_crit_);
transport_seq_ = (transport_seq_ + 1) & 0xFFFF;
return transport_seq_;
}
return new_seq;
uint16_t PacketRouter::CurrentTransportSequenceNumber() const {
rtc::CritScope lock(&modules_crit_);
return transport_seq_;
}
void PacketRouter::OnReceiveBitrateChanged(const std::vector<uint32_t>& ssrcs,

7
modules/pacing/packet_router.h
View File

@ -41,6 +41,7 @@ class PacketRouter : public RemoteBitrateObserver,
public TransportFeedbackSenderInterface {
public:
PacketRouter();
explicit PacketRouter(uint16_t start_transport_seq);
~PacketRouter() override;
void AddSendRtpModule(RtpRtcp* rtp_module, bool remb_candidate);
@ -56,9 +57,13 @@ class PacketRouter : public RemoteBitrateObserver,
virtual std::vector<std::unique_ptr<RtpPacketToSend>> GeneratePadding(
size_t target_size_bytes);
// TODO(bugs.webrtc.org/11036): Remove when downstream usage is gone.
void SetTransportWideSequenceNumber(uint16_t sequence_number);
// TODO(bugs.webrtc.org/11036): Make private when downstream usage is gone.
uint16_t AllocateSequenceNumber();
uint16_t CurrentTransportSequenceNumber() const;
// Called every time there is a new bitrate estimate for a receive channel
// group. This call will trigger a new RTCP REMB packet if the bitrate
// estimate has decreased or if no RTCP REMB packet has been sent for
@ -126,7 +131,7 @@ class PacketRouter : public RemoteBitrateObserver,
RtcpFeedbackSenderInterface* active_remb_module_
RTC_GUARDED_BY(modules_crit_);
volatile int transport_seq_;
int transport_seq_ RTC_GUARDED_BY(modules_crit_);
RTC_DISALLOW_COPY_AND_ASSIGN(PacketRouter);
};

18
modules/pacing/packet_router_unittest.cc
View File

@ -226,17 +226,27 @@ TEST_F(PacketRouterTest, PadsOnLastActiveMediaStream) {
packet_router_.RemoveSendRtpModule(&rtp_3);
}
TEST_F(PacketRouterTest, AllocateSequenceNumbers) {
TEST_F(PacketRouterTest, AllocatesTransportSequenceNumbers) {
const uint16_t kStartSeq = 0xFFF0;
const size_t kNumPackets = 32;
const uint16_t kSsrc1 = 1234;
packet_router_.SetTransportWideSequenceNumber(kStartSeq - 1);
PacketRouter packet_router(kStartSeq - 1);
NiceMock<MockRtpRtcp> rtp_1;
EXPECT_CALL(rtp_1, SSRC()).WillRepeatedly(Return(kSsrc1));
EXPECT_CALL(rtp_1, TrySendPacket).WillRepeatedly(Return(true));
packet_router.AddSendRtpModule(&rtp_1, false);
for (size_t i = 0; i < kNumPackets; ++i) {
uint16_t seq = packet_router_.AllocateSequenceNumber();
auto packet = BuildRtpPacket(kSsrc1);
EXPECT_TRUE(packet->ReserveExtension<TransportSequenceNumber>());
packet_router.SendPacket(std::move(packet), PacedPacketInfo());
uint32_t expected_unwrapped_seq = static_cast<uint32_t>(kStartSeq) + i;
EXPECT_EQ(static_cast<uint16_t>(expected_unwrapped_seq & 0xFFFF), seq);
EXPECT_EQ(static_cast<uint16_t>(expected_unwrapped_seq & 0xFFFF),
packet_router.CurrentTransportSequenceNumber());
}
packet_router.RemoveSendRtpModule(&rtp_1);
}
TEST_F(PacketRouterTest, SendTransportFeedback) {