Skip to content

Commit 2169d8b

Browse files
pbosCommit bot
pbos
authored and
Commit bot
committed
Reland of move audio/video distinction for probe packets. (patchset #1 id:1 of https://codereview.webrtc.org/2086633002/ )
Reason for revert: Fix already landed in google3, this revert actually breaks the import. Original issue's description: > Revert of Remove audio/video distinction for probe packets. (patchset #2 id:20001 of https://codereview.webrtc.org/2061193002/ ) > > Reason for revert: > Revert this because it broke the google3 import build. > http://webrtc-buildbot-master.mtv.corp.google.com:21000/builders/WebRTC%20google3%20Importer%20%28Shem%20TOT%29/builds/67/steps/blaze_regular_tests/logs/stdio > > Original issue's description: > > Remove audio/video distinction for probe packets. > > > > Allows detecting large-enough audio packets as part of a probe, > > speculative fix for a rampup-time regression in M50. These packets are > > accounted on the send side when probing. > > > > BUG=webrtc:5985 > > R=mflodman@webrtc.org, philipel@webrtc.org > > > > Committed: https://crrev.com/a7d88d38448f6a5677a017562765ab505b89d468 > > Cr-Commit-Position: refs/heads/master@{#13210} > > TBR=mflodman@webrtc.org,philipel@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:5985 > > Committed: https://crrev.com/17bde8c96ee8b5a7e496a7dc98828b84f9756925 > Cr-Commit-Position: refs/heads/master@{#13221} TBR=mflodman@webrtc.org,philipel@webrtc.org,honghaiz@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=webrtc:5985 Review-Url: https://codereview.webrtc.org/2085653002 Cr-Commit-Position: refs/heads/master@{#13223}
1 parent 6d3e0c2 commit 2169d8b

34 files changed

Lines changed: 139 additions & 213 deletions

‎webrtc/audio/audio_receive_stream.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -254,7 +254,7 @@ bool AudioReceiveStream::DeliverRtp(const uint8_t* packet,
254254
arrival_time_ms = (packet_time.timestamp + 500) / 1000;
255255
size_t payload_size = length - header.headerLength;
256256
remote_bitrate_estimator_->IncomingPacket(arrival_time_ms, payload_size,
257-
header, false);
257+
header);
258258
}
259259

260260
return channel_proxy_->ReceivedRTPPacket(packet, length, packet_time);

‎webrtc/audio/audio_receive_stream_unittest.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -279,7 +279,7 @@ TEST(AudioReceiveStreamTest, ReceiveRtpPacket) {
279279
EXPECT_CALL(*helper.remote_bitrate_estimator(),
280280
IncomingPacket(packet_time.timestamp / 1000,
281281
rtp_packet.size() - kExpectedHeaderLength,
282-
VerifyHeaderExtension(expected_extension), false))
282+
VerifyHeaderExtension(expected_extension)))
283283
.Times(1);
284284
EXPECT_CALL(*helper.channel_proxy(),
285285
ReceivedRTPPacket(&rtp_packet[0],

‎webrtc/modules/congestion_controller/congestion_controller.cc‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,11 +48,10 @@ class WrappingBitrateEstimator : public RemoteBitrateEstimator {
4848

4949
void IncomingPacket(int64_t arrival_time_ms,
5050
size_t payload_size,
51-
const RTPHeader& header,
52-
bool was_paced) override {
51+
const RTPHeader& header) override {
5352
CriticalSectionScoped cs(crit_sect_.get());
5453
PickEstimatorFromHeader(header);
55-
rbe_->IncomingPacket(arrival_time_ms, payload_size, header, was_paced);
54+
rbe_->IncomingPacket(arrival_time_ms, payload_size, header);
5655
}
5756

5857
void Process() override {

‎webrtc/modules/congestion_controller/delay_based_bwe.cc‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -198,15 +198,14 @@ void DelayBasedBwe::IncomingPacketFeedbackVector(
198198
for (const auto& packet_info : packet_feedback_vector) {
199199
IncomingPacketInfo(packet_info.arrival_time_ms,
200200
ConvertMsTo24Bits(packet_info.send_time_ms),
201-
packet_info.payload_size, 0, packet_info.was_paced,
201+
packet_info.payload_size, 0,
202202
packet_info.probe_cluster_id);
203203
}
204204
}
205205

206206
void DelayBasedBwe::IncomingPacket(int64_t arrival_time_ms,
207207
size_t payload_size,
208-
const RTPHeader& header,
209-
bool was_paced) {
208+
const RTPHeader& header) {
210209
RTC_DCHECK(network_thread_.CalledOnValidThread());
211210
if (!header.extension.hasAbsoluteSendTime) {
212211
// NOTE! The BitrateEstimatorTest relies on this EXACT log line.
@@ -215,14 +214,12 @@ void DelayBasedBwe::IncomingPacket(int64_t arrival_time_ms,
215214
return;
216215
}
217216
IncomingPacketInfo(arrival_time_ms, header.extension.absoluteSendTime,
218-
payload_size, header.ssrc, was_paced,
219-
PacketInfo::kNotAProbe);
217+
payload_size, header.ssrc, PacketInfo::kNotAProbe);
220218
}
221219

222220
void DelayBasedBwe::IncomingPacket(int64_t arrival_time_ms,
223221
size_t payload_size,
224222
const RTPHeader& header,
225-
bool was_paced,
226223
int probe_cluster_id) {
227224
RTC_DCHECK(network_thread_.CalledOnValidThread());
228225
if (!header.extension.hasAbsoluteSendTime) {
@@ -232,14 +229,13 @@ void DelayBasedBwe::IncomingPacket(int64_t arrival_time_ms,
232229
return;
233230
}
234231
IncomingPacketInfo(arrival_time_ms, header.extension.absoluteSendTime,
235-
payload_size, header.ssrc, was_paced, probe_cluster_id);
232+
payload_size, header.ssrc, probe_cluster_id);
236233
}
237234

238235
void DelayBasedBwe::IncomingPacketInfo(int64_t arrival_time_ms,
239236
uint32_t send_time_24bits,
240237
size_t payload_size,
241238
uint32_t ssrc,
242-
bool was_paced,
243239
int probe_cluster_id) {
244240
assert(send_time_24bits < (1ul << 24));
245241
// Shift up send time to use the full 32 bits that inter_arrival works with,

‎webrtc/modules/congestion_controller/delay_based_bwe.h‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,13 +40,11 @@ class DelayBasedBwe : public RemoteBitrateEstimator {
4040

4141
void IncomingPacket(int64_t arrival_time_ms,
4242
size_t payload_size,
43-
const RTPHeader& header,
44-
bool was_paced) override;
43+
const RTPHeader& header) override;
4544

4645
void IncomingPacket(int64_t arrival_time_ms,
4746
size_t payload_size,
4847
const RTPHeader& header,
49-
bool was_paced,
5048
int probe_cluster_id);
5149

5250
// This class relies on Process() being called periodically (at least once
@@ -112,7 +110,6 @@ class DelayBasedBwe : public RemoteBitrateEstimator {
112110
uint32_t send_time_24bits,
113111
size_t payload_size,
114112
uint32_t ssrc,
115-
bool was_paced,
116113
int probe_cluster_id);
117114

118115
void ComputeClusters(std::list<Cluster>* clusters) const;

‎webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc‎

Lines changed: 13 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@ class TestDelayBasedBwe : public ::testing::Test, public RemoteBitrateObserver {
3333
int64_t arrival_time,
3434
uint32_t rtp_timestamp,
3535
uint32_t absolute_send_time,
36-
bool was_paced,
3736
int probe_cluster_id) {
3837
RTPHeader header;
3938
memset(&header, 0, sizeof(header));
@@ -42,7 +41,7 @@ class TestDelayBasedBwe : public ::testing::Test, public RemoteBitrateObserver {
4241
header.extension.hasAbsoluteSendTime = true;
4342
header.extension.absoluteSendTime = absolute_send_time;
4443
bwe_.IncomingPacket(arrival_time + kArrivalTimeClockOffsetMs, payload_size,
45-
header, was_paced, probe_cluster_id);
44+
header, probe_cluster_id);
4645
}
4746

4847
void OnReceiveBitrateChanged(const std::vector<uint32_t>& ssrcs,
@@ -74,17 +73,15 @@ TEST_F(TestDelayBasedBwe, ProbeDetection) {
7473
for (int i = 0; i < kNumProbes; ++i) {
7574
clock_.AdvanceTimeMilliseconds(10);
7675
now_ms = clock_.TimeInMilliseconds();
77-
IncomingPacket(0, 1000, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000),
78-
true, 0);
76+
IncomingPacket(0, 1000, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000), 0);
7977
}
8078
EXPECT_TRUE(bitrate_updated());
8179

8280
// Second burst sent at 8 * 1000 / 5 = 1600 kbps.
8381
for (int i = 0; i < kNumProbes; ++i) {
8482
clock_.AdvanceTimeMilliseconds(5);
8583
now_ms = clock_.TimeInMilliseconds();
86-
IncomingPacket(0, 1000, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000),
87-
true, 1);
84+
IncomingPacket(0, 1000, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000), 1);
8885
}
8986

9087
EXPECT_TRUE(bitrate_updated());
@@ -98,12 +95,11 @@ TEST_F(TestDelayBasedBwe, ProbeDetectionNonPacedPackets) {
9895
for (int i = 0; i < kNumProbes; ++i) {
9996
clock_.AdvanceTimeMilliseconds(5);
10097
now_ms = clock_.TimeInMilliseconds();
101-
IncomingPacket(0, 1000, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000),
102-
true, 0);
98+
IncomingPacket(0, 1000, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000), 0);
10399
// Non-paced packet, arriving 5 ms after.
104100
clock_.AdvanceTimeMilliseconds(5);
105101
IncomingPacket(0, PacedSender::kMinProbePacketSize + 1, now_ms, 90 * now_ms,
106-
AbsSendTime(now_ms, 1000), false, PacketInfo::kNotAProbe);
102+
AbsSendTime(now_ms, 1000), PacketInfo::kNotAProbe);
107103
}
108104

109105
EXPECT_TRUE(bitrate_updated());
@@ -121,7 +117,7 @@ TEST_F(TestDelayBasedBwe, ProbeDetectionTooHighBitrate) {
121117
now_ms = clock_.TimeInMilliseconds();
122118
send_time_ms += 10;
123119
IncomingPacket(0, 1000, now_ms, 90 * send_time_ms,
124-
AbsSendTime(send_time_ms, 1000), true, 0);
120+
AbsSendTime(send_time_ms, 1000), 0);
125121
}
126122

127123
// Second burst sent at 8 * 1000 / 5 = 1600 kbps, arriving at 8 * 1000 / 8 =
@@ -131,7 +127,7 @@ TEST_F(TestDelayBasedBwe, ProbeDetectionTooHighBitrate) {
131127
now_ms = clock_.TimeInMilliseconds();
132128
send_time_ms += 5;
133129
IncomingPacket(0, 1000, now_ms, send_time_ms,
134-
AbsSendTime(send_time_ms, 1000), true, 1);
130+
AbsSendTime(send_time_ms, 1000), 1);
135131
}
136132

137133
EXPECT_TRUE(bitrate_updated());
@@ -148,7 +144,7 @@ TEST_F(TestDelayBasedBwe, ProbeDetectionSlightlyFasterArrival) {
148144
send_time_ms += 10;
149145
now_ms = clock_.TimeInMilliseconds();
150146
IncomingPacket(0, 1000, now_ms, 90 * send_time_ms,
151-
AbsSendTime(send_time_ms, 1000), true, 23);
147+
AbsSendTime(send_time_ms, 1000), 23);
152148
}
153149

154150
EXPECT_TRUE(bitrate_updated());
@@ -165,7 +161,7 @@ TEST_F(TestDelayBasedBwe, ProbeDetectionFasterArrival) {
165161
send_time_ms += 10;
166162
now_ms = clock_.TimeInMilliseconds();
167163
IncomingPacket(0, 1000, now_ms, 90 * send_time_ms,
168-
AbsSendTime(send_time_ms, 1000), true, 0);
164+
AbsSendTime(send_time_ms, 1000), 0);
169165
}
170166

171167
EXPECT_FALSE(bitrate_updated());
@@ -181,7 +177,7 @@ TEST_F(TestDelayBasedBwe, ProbeDetectionSlowerArrival) {
181177
send_time_ms += 5;
182178
now_ms = clock_.TimeInMilliseconds();
183179
IncomingPacket(0, 1000, now_ms, 90 * send_time_ms,
184-
AbsSendTime(send_time_ms, 1000), true, 1);
180+
AbsSendTime(send_time_ms, 1000), 1);
185181
}
186182

187183
EXPECT_TRUE(bitrate_updated());
@@ -198,7 +194,7 @@ TEST_F(TestDelayBasedBwe, ProbeDetectionSlowerArrivalHighBitrate) {
198194
send_time_ms += 1;
199195
now_ms = clock_.TimeInMilliseconds();
200196
IncomingPacket(0, 1000, now_ms, 90 * send_time_ms,
201-
AbsSendTime(send_time_ms, 1000), true, 1);
197+
AbsSendTime(send_time_ms, 1000), 1);
202198
}
203199

204200
EXPECT_TRUE(bitrate_updated());
@@ -213,7 +209,7 @@ TEST_F(TestDelayBasedBwe, ProbingIgnoresSmallPackets) {
213209
clock_.AdvanceTimeMilliseconds(10);
214210
now_ms = clock_.TimeInMilliseconds();
215211
IncomingPacket(0, PacedSender::kMinProbePacketSize, now_ms, 90 * now_ms,
216-
AbsSendTime(now_ms, 1000), true, 1);
212+
AbsSendTime(now_ms, 1000), 1);
217213
}
218214

219215
EXPECT_FALSE(bitrate_updated());
@@ -223,8 +219,7 @@ TEST_F(TestDelayBasedBwe, ProbingIgnoresSmallPackets) {
223219
for (int i = 0; i < kNumProbes; ++i) {
224220
clock_.AdvanceTimeMilliseconds(10);
225221
now_ms = clock_.TimeInMilliseconds();
226-
IncomingPacket(0, 1000, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000),
227-
true, 1);
222+
IncomingPacket(0, 1000, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000), 1);
228223
}
229224

230225
// Wait long enough so that we can call Process again.

‎webrtc/modules/remote_bitrate_estimator/include/mock/mock_remote_bitrate_estimator.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ class MockRemoteBitrateEstimator : public RemoteBitrateEstimator {
2828
public:
2929
MOCK_METHOD1(IncomingPacketFeedbackVector,
3030
void(const std::vector<PacketInfo>&));
31-
MOCK_METHOD4(IncomingPacket, void(int64_t, size_t, const RTPHeader&, bool));
31+
MOCK_METHOD3(IncomingPacket, void(int64_t, size_t, const RTPHeader&));
3232
MOCK_METHOD1(RemoveStream, void(uint32_t));
3333
MOCK_CONST_METHOD2(LatestEstimate, bool(std::vector<uint32_t>*, uint32_t*));
3434

‎webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,7 @@ class RemoteBitrateEstimator : public CallStatsObserver, public Module {
5858
// Note that |arrival_time_ms| can be of an arbitrary time base.
5959
virtual void IncomingPacket(int64_t arrival_time_ms,
6060
size_t payload_size,
61-
const RTPHeader& header,
62-
bool was_paced) = 0;
61+
const RTPHeader& header) = 0;
6362

6463
// Removes all data for |ssrc|.
6564
virtual void RemoveStream(uint32_t ssrc) = 0;

‎webrtc/modules/remote_bitrate_estimator/include/send_time_history.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ class SendTimeHistory {
2626

2727
void AddAndRemoveOld(uint16_t sequence_number,
2828
size_t length,
29-
bool was_paced,
3029
int probe_cluster_id);
3130
bool OnSentPacket(uint16_t sequence_number, int64_t timestamp);
3231
// Look up PacketInfo for a sent packet, based on the sequence number, and

‎webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc‎

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -211,30 +211,29 @@ void RemoteBitrateEstimatorAbsSendTime::IncomingPacketFeedbackVector(
211211
for (const auto& packet_info : packet_feedback_vector) {
212212
IncomingPacketInfo(packet_info.arrival_time_ms,
213213
ConvertMsTo24Bits(packet_info.send_time_ms),
214-
packet_info.payload_size, 0, packet_info.was_paced);
214+
packet_info.payload_size, 0);
215215
}
216216
}
217217

218-
void RemoteBitrateEstimatorAbsSendTime::IncomingPacket(int64_t arrival_time_ms,
219-
size_t payload_size,
220-
const RTPHeader& header,
221-
bool was_paced) {
218+
void RemoteBitrateEstimatorAbsSendTime::IncomingPacket(
219+
int64_t arrival_time_ms,
220+
size_t payload_size,
221+
const RTPHeader& header) {
222222
RTC_DCHECK(network_thread_.CalledOnValidThread());
223223
if (!header.extension.hasAbsoluteSendTime) {
224224
LOG(LS_WARNING) << "RemoteBitrateEstimatorAbsSendTimeImpl: Incoming packet "
225225
"is missing absolute send time extension!";
226226
return;
227227
}
228228
IncomingPacketInfo(arrival_time_ms, header.extension.absoluteSendTime,
229-
payload_size, header.ssrc, was_paced);
229+
payload_size, header.ssrc);
230230
}
231231

232232
void RemoteBitrateEstimatorAbsSendTime::IncomingPacketInfo(
233233
int64_t arrival_time_ms,
234234
uint32_t send_time_24bits,
235235
size_t payload_size,
236-
uint32_t ssrc,
237-
bool was_paced) {
236+
uint32_t ssrc) {
238237
assert(send_time_24bits < (1ul << 24));
239238
// Shift up send time to use the full 32 bits that inter_arrival works with,
240239
// so wrapping works properly.
@@ -264,10 +263,6 @@ void RemoteBitrateEstimatorAbsSendTime::IncomingPacketInfo(
264263
uint32_t ts_delta = 0;
265264
int64_t t_delta = 0;
266265
int size_delta = 0;
267-
// For now only try to detect probes while we don't have a valid estimate, and
268-
// make sure the packet was paced. We currently assume that only packets
269-
// larger than 200 bytes are paced by the sender.
270-
was_paced = was_paced && payload_size > PacedSender::kMinProbePacketSize;
271266
bool update_estimate = false;
272267
uint32_t target_bitrate_bps = 0;
273268
std::vector<uint32_t> ssrcs;
@@ -279,7 +274,10 @@ void RemoteBitrateEstimatorAbsSendTime::IncomingPacketInfo(
279274
RTC_DCHECK(estimator_.get());
280275
ssrcs_[ssrc] = now_ms;
281276

282-
if (was_paced &&
277+
// For now only try to detect probes while we don't have a valid estimate.
278+
// We currently assume that only packets larger than 200 bytes are paced by
279+
// the sender.
280+
if (payload_size > PacedSender::kMinProbePacketSize &&
283281
(!remote_rate_.ValidEstimate() ||
284282
now_ms - first_packet_time_ms_ < kInitialProbingIntervalMs)) {
285283
// TODO(holmer): Use a map instead to get correct order?

0 commit comments

Comments
 (0)