Prevent busy-looping PacedSender on small packets.

Prevent busy-looping PacedSender on small packets.

Skip accounting for small packets and suspend the prober if no
large-enough packets have been sent for some time. This especially seems
to have triggered in audio-only calls where all packets are too small,
making TimeUntilNextProbe return 0 forever, causing the module process
thread to wake up forever.

BUG=webrtc:5506
R=stefan@webrtc.org

Committed: https://crrev.com/0453ef857f900dd0f230d3e654b54256512af64f
Cr-Commit-Position: refs/heads/master@{#11634}

[pbos-webrtc, 2016-02-10 10:40:21]

PTAL, woo!

[pbos-webrtc, 2016-02-10 10:40:27]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-10 10:40:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1688703002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1688703002/1

[stefan-webrtc, 2016-02-10 10:56:20]

https://codereview.webrtc.org/1688703002/diff/1/webrtc/modules/pacing/bitrate...
File webrtc/modules/pacing/bitrate_prober.cc (right):

https://codereview.webrtc.org/1688703002/diff/1/webrtc/modules/pacing/bitrate...
webrtc/modules/pacing/bitrate_prober.cc:87: // No probe started, or waiting for
next probe.
Given line 78, is this comment still correct?

https://codereview.webrtc.org/1688703002/diff/1/webrtc/modules/pacing/bitrate...
webrtc/modules/pacing/bitrate_prober.cc:91: // If no packets have been sent for
n milliseconds, inactivate to not keep
deactivate

https://codereview.webrtc.org/1688703002/diff/1/webrtc/modules/pacing/bitrate...
webrtc/modules/pacing/bitrate_prober.cc:98: }
Shouldn't we change the state to say we're no longer probing?

https://codereview.webrtc.org/1688703002/diff/1/webrtc/modules/pacing/paced_s...
File webrtc/modules/pacing/paced_sender.cc (right):

https://codereview.webrtc.org/1688703002/diff/1/webrtc/modules/pacing/paced_s...
webrtc/modules/pacing/paced_sender.cc:325: prober_->SetEnabled(true);
Wait until we have a packet large enough to probe with?

https://codereview.webrtc.org/1688703002/diff/1/webrtc/modules/pacing/paced_s...
webrtc/modules/pacing/paced_sender.cc:327: int64_t now_ms =
clock_->TimeInMilliseconds();
Move down again

[pbos-webrtc, 2016-02-10 11:03:38]

only initialize the prober if we have something large enough to probe with

[pbos-webrtc, 2016-02-10 13:19:08]

PTAL

https://codereview.webrtc.org/1688703002/diff/1/webrtc/modules/pacing/bitrate...
File webrtc/modules/pacing/bitrate_prober.cc (right):

https://codereview.webrtc.org/1688703002/diff/1/webrtc/modules/pacing/bitrate...
webrtc/modules/pacing/bitrate_prober.cc:87: // No probe started, or waiting for
next probe.
On 2016/02/10 10:56:19, stefan-webrtc (holmer) wrote:
> Given line 78, is this comment still correct?

Done.

https://codereview.webrtc.org/1688703002/diff/1/webrtc/modules/pacing/bitrate...
webrtc/modules/pacing/bitrate_prober.cc:91: // If no packets have been sent for
n milliseconds, inactivate to not keep
On 2016/02/10 10:56:19, stefan-webrtc (holmer) wrote:
> deactivate

Done.

https://codereview.webrtc.org/1688703002/diff/1/webrtc/modules/pacing/bitrate...
webrtc/modules/pacing/bitrate_prober.cc:98: }
On 2016/02/10 10:56:19, stefan-webrtc (holmer) wrote:
> Shouldn't we change the state to say we're no longer probing?

Changed state to allow a fresh probe.

https://codereview.webrtc.org/1688703002/diff/1/webrtc/modules/pacing/paced_s...
File webrtc/modules/pacing/paced_sender.cc (right):

https://codereview.webrtc.org/1688703002/diff/1/webrtc/modules/pacing/paced_s...
webrtc/modules/pacing/paced_sender.cc:327: int64_t now_ms =
clock_->TimeInMilliseconds();
On 2016/02/10 10:56:19, stefan-webrtc (holmer) wrote:
> Move down again

Needed in OnIncomingPacket, so added back up.

[pbos-webrtc, 2016-02-10 13:19:26]

review + fixes

[pbos-webrtc, 2016-02-15 17:02:01]

rebase

[stefan-webrtc, 2016-02-16 13:50:26]

https://codereview.webrtc.org/1688703002/diff/60001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober.cc (right):

https://codereview.webrtc.org/1688703002/diff/60001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:87: packet_size_last_send_ = 0;
Is this needed? In case we decide to start a probe mid-call, we may be able to
utilize the last sent packet before the probe.

https://codereview.webrtc.org/1688703002/diff/60001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:102: static const int
kSendDeltaUntilConsideredInactiveMs = 5000;
kInactiveSendDeltaMs?

https://codereview.webrtc.org/1688703002/diff/60001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:105: probing_state_ = kAllowedToProbe;
This will start a new probe once the next packet is added to the pacer. I don't
think we want that?

[pbos-webrtc, 2016-02-16 13:57:20]

PTAL

https://codereview.webrtc.org/1688703002/diff/60001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober.cc (right):

https://codereview.webrtc.org/1688703002/diff/60001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:87: packet_size_last_send_ = 0;
On 2016/02/16 13:50:25, stefan-webrtc (holmer) wrote:
> Is this needed? In case we decide to start a probe mid-call, we may be able to
> utilize the last sent packet before the probe.

Done.

https://codereview.webrtc.org/1688703002/diff/60001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:102: static const int
kSendDeltaUntilConsideredInactiveMs = 5000;
On 2016/02/16 13:50:25, stefan-webrtc (holmer) wrote:
> kInactiveSendDeltaMs?

Done.

https://codereview.webrtc.org/1688703002/diff/60001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:105: probing_state_ = kAllowedToProbe;
On 2016/02/16 13:50:26, stefan-webrtc (holmer) wrote:
> This will start a new probe once the next packet is added to the pacer. I
don't
> think we want that?

That's the idea. If we had a long period of small-size packets then we wouldn't
consider probing to be done and have a chance to do probing again from the
start.

[pbos-webrtc, 2016-02-16 13:57:39]

remove packet_size_last_send_ = 0 and rename constant

[stefan-webrtc, 2016-02-16 14:56:26]

lgtm

[pbos-webrtc, 2016-02-16 15:23:21]

Description was changed from

==========
Prevent busy-looping PacedSender on small packets.

Skip accounting for small packets and suspend the prober if no
large-enough packets have been sent for some time. This especially seems
to have triggered in audio-only calls where all packets are too small,
making TimeUntilNextProbe return 0 forever, causing the module process
thread to wake up forever.

BUG=webrtc:5506
R=stefan@webrtc.org
==========

to

==========
Prevent busy-looping PacedSender on small packets.

Skip accounting for small packets and suspend the prober if no
large-enough packets have been sent for some time. This especially seems
to have triggered in audio-only calls where all packets are too small,
making TimeUntilNextProbe return 0 forever, causing the module process
thread to wake up forever.

BUG=webrtc:5506
R=stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/0453ef857f900dd0f230d3e65...
==========

[pbos-webrtc, 2016-02-16 15:23:22]

Committed patchset #5 (id:80001) manually as
0453ef857f900dd0f230d3e654b54256512af64f (presubmit successful).

[commit-bot: I haz the power, 2016-02-16 15:23:24]

Description was changed from

==========
Prevent busy-looping PacedSender on small packets.

Skip accounting for small packets and suspend the prober if no
large-enough packets have been sent for some time. This especially seems
to have triggered in audio-only calls where all packets are too small,
making TimeUntilNextProbe return 0 forever, causing the module process
thread to wake up forever.

BUG=webrtc:5506
R=stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/0453ef857f900dd0f230d3e65...
==========

to

==========
Prevent busy-looping PacedSender on small packets.

Skip accounting for small packets and suspend the prober if no
large-enough packets have been sent for some time. This especially seems
to have triggered in audio-only calls where all packets are too small,
making TimeUntilNextProbe return 0 forever, causing the module process
thread to wake up forever.

BUG=webrtc:5506
R=stefan@webrtc.org

Committed: https://crrev.com/0453ef857f900dd0f230d3e654b54256512af64f
Cr-Commit-Position: refs/heads/master@{#11634}
==========

[commit-bot: I haz the power, 2016-02-16 15:23:24]

Patchset 5 (id:??) landed as
https://crrev.com/0453ef857f900dd0f230d3e654b54256512af64f
Cr-Commit-Position: refs/heads/master@{#11634}