Skip to content

Commit 17bde8c

Browse files
honghaizCommit bot
authored andcommitted
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 Review-Url: https://codereview.webrtc.org/2086633002 Cr-Commit-Position: refs/heads/master@{#13221}
1 parent 4b6c8b7 commit 17bde8c

34 files changed

Lines changed: 213 additions & 139 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);
257+
header, false);
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)))
282+
VerifyHeaderExtension(expected_extension), false))
283283
.Times(1);
284284
EXPECT_CALL(*helper.channel_proxy(),
285285
ReceivedRTPPacket(&rtp_packet[0],

webrtc/modules/congestion_controller/congestion_controller.cc

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

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

5758
void Process() override {

webrtc/modules/congestion_controller/delay_based_bwe.cc

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -198,14 +198,15 @@ 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,
201+
packet_info.payload_size, 0, packet_info.was_paced,
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) {
208+
const RTPHeader& header,
209+
bool was_paced) {
209210
RTC_DCHECK(network_thread_.CalledOnValidThread());
210211
if (!header.extension.hasAbsoluteSendTime) {
211212
// NOTE! The BitrateEstimatorTest relies on this EXACT log line.
@@ -214,12 +215,14 @@ void DelayBasedBwe::IncomingPacket(int64_t arrival_time_ms,
214215
return;
215216
}
216217
IncomingPacketInfo(arrival_time_ms, header.extension.absoluteSendTime,
217-
payload_size, header.ssrc, PacketInfo::kNotAProbe);
218+
payload_size, header.ssrc, was_paced,
219+
PacketInfo::kNotAProbe);
218220
}
219221

220222
void DelayBasedBwe::IncomingPacket(int64_t arrival_time_ms,
221223
size_t payload_size,
222224
const RTPHeader& header,
225+
bool was_paced,
223226
int probe_cluster_id) {
224227
RTC_DCHECK(network_thread_.CalledOnValidThread());
225228
if (!header.extension.hasAbsoluteSendTime) {
@@ -229,13 +232,14 @@ void DelayBasedBwe::IncomingPacket(int64_t arrival_time_ms,
229232
return;
230233
}
231234
IncomingPacketInfo(arrival_time_ms, header.extension.absoluteSendTime,
232-
payload_size, header.ssrc, probe_cluster_id);
235+
payload_size, header.ssrc, was_paced, probe_cluster_id);
233236
}
234237

235238
void DelayBasedBwe::IncomingPacketInfo(int64_t arrival_time_ms,
236239
uint32_t send_time_24bits,
237240
size_t payload_size,
238241
uint32_t ssrc,
242+
bool was_paced,
239243
int probe_cluster_id) {
240244
assert(send_time_24bits < (1ul << 24));
241245
// 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: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,11 +40,13 @@ class DelayBasedBwe : public RemoteBitrateEstimator {
4040

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

4546
void IncomingPacket(int64_t arrival_time_ms,
4647
size_t payload_size,
4748
const RTPHeader& header,
49+
bool was_paced,
4850
int probe_cluster_id);
4951

5052
// This class relies on Process() being called periodically (at least once
@@ -110,6 +112,7 @@ class DelayBasedBwe : public RemoteBitrateEstimator {
110112
uint32_t send_time_24bits,
111113
size_t payload_size,
112114
uint32_t ssrc,
115+
bool was_paced,
113116
int probe_cluster_id);
114117

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

webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ 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,
3637
int probe_cluster_id) {
3738
RTPHeader header;
3839
memset(&header, 0, sizeof(header));
@@ -41,7 +42,7 @@ class TestDelayBasedBwe : public ::testing::Test, public RemoteBitrateObserver {
4142
header.extension.hasAbsoluteSendTime = true;
4243
header.extension.absoluteSendTime = absolute_send_time;
4344
bwe_.IncomingPacket(arrival_time + kArrivalTimeClockOffsetMs, payload_size,
44-
header, probe_cluster_id);
45+
header, was_paced, probe_cluster_id);
4546
}
4647

4748
void OnReceiveBitrateChanged(const std::vector<uint32_t>& ssrcs,
@@ -73,15 +74,17 @@ TEST_F(TestDelayBasedBwe, ProbeDetection) {
7374
for (int i = 0; i < kNumProbes; ++i) {
7475
clock_.AdvanceTimeMilliseconds(10);
7576
now_ms = clock_.TimeInMilliseconds();
76-
IncomingPacket(0, 1000, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000), 0);
77+
IncomingPacket(0, 1000, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000),
78+
true, 0);
7779
}
7880
EXPECT_TRUE(bitrate_updated());
7981

8082
// Second burst sent at 8 * 1000 / 5 = 1600 kbps.
8183
for (int i = 0; i < kNumProbes; ++i) {
8284
clock_.AdvanceTimeMilliseconds(5);
8385
now_ms = clock_.TimeInMilliseconds();
84-
IncomingPacket(0, 1000, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000), 1);
86+
IncomingPacket(0, 1000, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000),
87+
true, 1);
8588
}
8689

8790
EXPECT_TRUE(bitrate_updated());
@@ -95,11 +98,12 @@ TEST_F(TestDelayBasedBwe, ProbeDetectionNonPacedPackets) {
9598
for (int i = 0; i < kNumProbes; ++i) {
9699
clock_.AdvanceTimeMilliseconds(5);
97100
now_ms = clock_.TimeInMilliseconds();
98-
IncomingPacket(0, 1000, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000), 0);
101+
IncomingPacket(0, 1000, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000),
102+
true, 0);
99103
// Non-paced packet, arriving 5 ms after.
100104
clock_.AdvanceTimeMilliseconds(5);
101105
IncomingPacket(0, PacedSender::kMinProbePacketSize + 1, now_ms, 90 * now_ms,
102-
AbsSendTime(now_ms, 1000), PacketInfo::kNotAProbe);
106+
AbsSendTime(now_ms, 1000), false, PacketInfo::kNotAProbe);
103107
}
104108

105109
EXPECT_TRUE(bitrate_updated());
@@ -117,7 +121,7 @@ TEST_F(TestDelayBasedBwe, ProbeDetectionTooHighBitrate) {
117121
now_ms = clock_.TimeInMilliseconds();
118122
send_time_ms += 10;
119123
IncomingPacket(0, 1000, now_ms, 90 * send_time_ms,
120-
AbsSendTime(send_time_ms, 1000), 0);
124+
AbsSendTime(send_time_ms, 1000), true, 0);
121125
}
122126

123127
// Second burst sent at 8 * 1000 / 5 = 1600 kbps, arriving at 8 * 1000 / 8 =
@@ -127,7 +131,7 @@ TEST_F(TestDelayBasedBwe, ProbeDetectionTooHighBitrate) {
127131
now_ms = clock_.TimeInMilliseconds();
128132
send_time_ms += 5;
129133
IncomingPacket(0, 1000, now_ms, send_time_ms,
130-
AbsSendTime(send_time_ms, 1000), 1);
134+
AbsSendTime(send_time_ms, 1000), true, 1);
131135
}
132136

133137
EXPECT_TRUE(bitrate_updated());
@@ -144,7 +148,7 @@ TEST_F(TestDelayBasedBwe, ProbeDetectionSlightlyFasterArrival) {
144148
send_time_ms += 10;
145149
now_ms = clock_.TimeInMilliseconds();
146150
IncomingPacket(0, 1000, now_ms, 90 * send_time_ms,
147-
AbsSendTime(send_time_ms, 1000), 23);
151+
AbsSendTime(send_time_ms, 1000), true, 23);
148152
}
149153

150154
EXPECT_TRUE(bitrate_updated());
@@ -161,7 +165,7 @@ TEST_F(TestDelayBasedBwe, ProbeDetectionFasterArrival) {
161165
send_time_ms += 10;
162166
now_ms = clock_.TimeInMilliseconds();
163167
IncomingPacket(0, 1000, now_ms, 90 * send_time_ms,
164-
AbsSendTime(send_time_ms, 1000), 0);
168+
AbsSendTime(send_time_ms, 1000), true, 0);
165169
}
166170

167171
EXPECT_FALSE(bitrate_updated());
@@ -177,7 +181,7 @@ TEST_F(TestDelayBasedBwe, ProbeDetectionSlowerArrival) {
177181
send_time_ms += 5;
178182
now_ms = clock_.TimeInMilliseconds();
179183
IncomingPacket(0, 1000, now_ms, 90 * send_time_ms,
180-
AbsSendTime(send_time_ms, 1000), 1);
184+
AbsSendTime(send_time_ms, 1000), true, 1);
181185
}
182186

183187
EXPECT_TRUE(bitrate_updated());
@@ -194,7 +198,7 @@ TEST_F(TestDelayBasedBwe, ProbeDetectionSlowerArrivalHighBitrate) {
194198
send_time_ms += 1;
195199
now_ms = clock_.TimeInMilliseconds();
196200
IncomingPacket(0, 1000, now_ms, 90 * send_time_ms,
197-
AbsSendTime(send_time_ms, 1000), 1);
201+
AbsSendTime(send_time_ms, 1000), true, 1);
198202
}
199203

200204
EXPECT_TRUE(bitrate_updated());
@@ -209,7 +213,7 @@ TEST_F(TestDelayBasedBwe, ProbingIgnoresSmallPackets) {
209213
clock_.AdvanceTimeMilliseconds(10);
210214
now_ms = clock_.TimeInMilliseconds();
211215
IncomingPacket(0, PacedSender::kMinProbePacketSize, now_ms, 90 * now_ms,
212-
AbsSendTime(now_ms, 1000), 1);
216+
AbsSendTime(now_ms, 1000), true, 1);
213217
}
214218

215219
EXPECT_FALSE(bitrate_updated());
@@ -219,7 +223,8 @@ TEST_F(TestDelayBasedBwe, ProbingIgnoresSmallPackets) {
219223
for (int i = 0; i < kNumProbes; ++i) {
220224
clock_.AdvanceTimeMilliseconds(10);
221225
now_ms = clock_.TimeInMilliseconds();
222-
IncomingPacket(0, 1000, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000), 1);
226+
IncomingPacket(0, 1000, now_ms, 90 * now_ms, AbsSendTime(now_ms, 1000),
227+
true, 1);
223228
}
224229

225230
// 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_METHOD3(IncomingPacket, void(int64_t, size_t, const RTPHeader&));
31+
MOCK_METHOD4(IncomingPacket, void(int64_t, size_t, const RTPHeader&, bool));
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: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,8 @@ 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) = 0;
61+
const RTPHeader& header,
62+
bool was_paced) = 0;
6263

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

webrtc/modules/remote_bitrate_estimator/include/send_time_history.h

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

2727
void AddAndRemoveOld(uint16_t sequence_number,
2828
size_t length,
29+
bool was_paced,
2930
int probe_cluster_id);
3031
bool OnSentPacket(uint16_t sequence_number, int64_t timestamp);
3132
// 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: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -211,29 +211,30 @@ 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);
214+
packet_info.payload_size, 0, packet_info.was_paced);
215215
}
216216
}
217217

218-
void RemoteBitrateEstimatorAbsSendTime::IncomingPacket(
219-
int64_t arrival_time_ms,
220-
size_t payload_size,
221-
const RTPHeader& header) {
218+
void RemoteBitrateEstimatorAbsSendTime::IncomingPacket(int64_t arrival_time_ms,
219+
size_t payload_size,
220+
const RTPHeader& header,
221+
bool was_paced) {
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);
229+
payload_size, header.ssrc, was_paced);
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) {
236+
uint32_t ssrc,
237+
bool was_paced) {
237238
assert(send_time_24bits < (1ul << 24));
238239
// Shift up send time to use the full 32 bits that inter_arrival works with,
239240
// so wrapping works properly.
@@ -263,6 +264,10 @@ void RemoteBitrateEstimatorAbsSendTime::IncomingPacketInfo(
263264
uint32_t ts_delta = 0;
264265
int64_t t_delta = 0;
265266
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;
266271
bool update_estimate = false;
267272
uint32_t target_bitrate_bps = 0;
268273
std::vector<uint32_t> ssrcs;
@@ -274,10 +279,7 @@ void RemoteBitrateEstimatorAbsSendTime::IncomingPacketInfo(
274279
RTC_DCHECK(estimator_.get());
275280
ssrcs_[ssrc] = now_ms;
276281

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 &&
282+
if (was_paced &&
281283
(!remote_rate_.ValidEstimate() ||
282284
now_ms - first_packet_time_ms_ < kInitialProbingIntervalMs)) {
283285
// TODO(holmer): Use a map instead to get correct order?

0 commit comments

Comments
 (0)