Bitrate prober and paced sender improvements

Bitrate prober and paced sender improvements

- Removes unnecessary casts to compute timedelta.
- Renames ProbingState for clarity. This should help when we probe mid-call.
- Enables probing by default to avoid checking on each incoming packet.
- Removes duplicate probing state tracking in paced sender. These duplicate
  states were conflicting at times.
- Removes passing through packets for bug 5307 which seems long fixed.
- Cleanup handling of time_last_send_ms and avoid side effects of changing
  probing state at TimeUntilNextProbe().
- Clear cluster data when probing is restarted to avoid having old data after a reset.

BUG=5859
R=stefan@webrtc.org

Committed: https://crrev.com/6e11efa6dced914e6f0927db3fcc92d875ec2cf5
Cr-Commit-Position: refs/heads/master@{#13612}

[Irfan, 2016-07-25 17:13:41]

Description was changed from

==========
Bitrate prober and paced sender improvements

- Removes unnecessary casts to compute timedelta.
- Renames ProbingState for clarity. This should help when we probe mid-call.
- Enables probing by default to avoid checking on each incoming packet.
- Removes duplicate probing state tracking in paced sender. These duplicate
  states were conflicting at times.
- Removes passing through packets for bug 5307 which seems long fixed.
- Cleanup handling of time_last_send_ms and avoid side effects of changing
  probing state at TimeUntilNextProbe().

BUG=5859
==========

to

==========
Bitrate prober and paced sender improvements

- Removes unnecessary casts to compute timedelta.
- Renames ProbingState for clarity. This should help when we probe mid-call.
- Enables probing by default to avoid checking on each incoming packet.
- Removes duplicate probing state tracking in paced sender. These duplicate
  states were conflicting at times.
- Removes passing through packets for bug 5307 which seems long fixed.
- Cleanup handling of time_last_send_ms and avoid side effects of changing
  probing state at TimeUntilNextProbe().

BUG=5859
==========

[Irfan, 2016-07-25 17:13:41]

isheriff@chromium.org changed reviewers:
+ philipel@webrtc.org, stefan@webrtc.org

[Irfan, 2016-07-25 17:14:54]

Description was changed from

==========
Bitrate prober and paced sender improvements

- Removes unnecessary casts to compute timedelta.
- Renames ProbingState for clarity. This should help when we probe mid-call.
- Enables probing by default to avoid checking on each incoming packet.
- Removes duplicate probing state tracking in paced sender. These duplicate
  states were conflicting at times.
- Removes passing through packets for bug 5307 which seems long fixed.
- Cleanup handling of time_last_send_ms and avoid side effects of changing
  probing state at TimeUntilNextProbe().

BUG=5859
==========

to

==========
Bitrate prober and paced sender improvements

- Removes unnecessary casts to compute timedelta.
- Renames ProbingState for clarity. This should help when we probe mid-call.
- Enables probing by default to avoid checking on each incoming packet.
- Removes duplicate probing state tracking in paced sender. These duplicate
  states were conflicting at times.
- Removes passing through packets for bug 5307 which seems long fixed.
- Cleanup handling of time_last_send_ms and avoid side effects of changing
  probing state at TimeUntilNextProbe().
- Clear cluster data when probing is restarted to avoid having old data after a
reset.

BUG=5859
==========

[Irfan, 2016-07-25 17:17:37]

Patchset #1 (id:1) has been deleted

[Irfan, 2016-07-25 17:18:28]

Some improvements based on my reading of the code. Currently tested on tests,
but will be running more real tests.

[Irfan, 2016-07-28 23:23:33]

Stefan, have you had a chance to look at this ?

[stefan-webrtc, 2016-07-29 07:12:07]

Thanks for the improvements!

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

https://codereview.webrtc.org/2182603002/diff/20001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:94: std::queue<ProbeCluster>
empty_cluster_;
No _ at the end of this variable name.

https://codereview.webrtc.org/2182603002/diff/20001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:97: std::swap(clusters_,
empty_cluster_);
Is this more efficient than doing:
clusters_ = std::queue<ProbeCluster>();?

Seems a bit simpler.

https://codereview.webrtc.org/2182603002/diff/20001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober_unittest.cc (right):

https://codereview.webrtc.org/2182603002/diff/20001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober_unittest.cc:68: prober.PacketSent(now_ms,
1000);
Did you add this to test the inactivity threshold?

https://codereview.webrtc.org/2182603002/diff/20001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/paced_sender.cc (left):

https://codereview.webrtc.org/2182603002/diff/20001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/paced_sender.cc:441: if (paused_ && packet.priority !=
kHighPriority)
If there're still bugs in the network layer code which causes the pacer to be
paused when it's not supposed to be, removing this can break audio. Therefore
I'd prefer if we at least did it in a separate CL from all the other
improvements you are making here.

Same for the other TODOs in this file that you're fixing.

[philipel, 2016-08-01 11:35:05]

Just some minor comments, but you should change the related bug from 5859 to
webrtc:5859

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

https://codereview.webrtc.org/2182603002/diff/20001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:28: static constexpr int
kInactivityThresholdMs = 5000;
No need for static since you are using an anonymous namespace.

https://codereview.webrtc.org/2182603002/diff/20001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:39: :
probing_state_(ProbingState::kDisabled),
ProbingState::kDisabled -> kDisabled
Or is there a reason you explicitly use the enum name?

https://codereview.webrtc.org/2182603002/diff/20001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.cc:145: RTC_DCHECK(ProbingState::kActive ==
probing_state_);
Why change from RTC_DCHECK_EQ?

[Irfan, 2016-08-01 18:29:47]

https://codereview.chromium.org/2182603002/diff/20001/webrtc/modules/pacing/b...
File webrtc/modules/pacing/bitrate_prober.cc (right):

https://codereview.chromium.org/2182603002/diff/20001/webrtc/modules/pacing/b...
webrtc/modules/pacing/bitrate_prober.cc:28: static constexpr int
kInactivityThresholdMs = 5000;
On 2016/08/01 11:35:05, philipel wrote:
> No need for static since you are using an anonymous namespace.

Done.

https://codereview.chromium.org/2182603002/diff/20001/webrtc/modules/pacing/b...
webrtc/modules/pacing/bitrate_prober.cc:39: :
probing_state_(ProbingState::kDisabled),
On 2016/08/01 11:35:05, philipel wrote:
> ProbingState::kDisabled -> kDisabled
> Or is there a reason you explicitly use the enum name?

Yes, this is now a scoped enum.

https://codereview.chromium.org/2182603002/diff/20001/webrtc/modules/pacing/b...
webrtc/modules/pacing/bitrate_prober.cc:94: std::queue<ProbeCluster>
empty_cluster_;
On 2016/07/29 07:12:07, stefan-webrtc (holmer) wrote:
> No _ at the end of this variable name.

Done.

https://codereview.chromium.org/2182603002/diff/20001/webrtc/modules/pacing/b...
webrtc/modules/pacing/bitrate_prober.cc:97: std::swap(clusters_,
empty_cluster_);
On 2016/07/29 07:12:07, stefan-webrtc (holmer) wrote:
> Is this more efficient than doing:
> clusters_ = std::queue<ProbeCluster>();?
> 
> Seems a bit simpler.

Either works mostly the same. I switched it.

https://codereview.chromium.org/2182603002/diff/20001/webrtc/modules/pacing/b...
webrtc/modules/pacing/bitrate_prober.cc:145: RTC_DCHECK(ProbingState::kActive ==
probing_state_);
On 2016/08/01 11:35:05, philipel wrote:
> Why change from RTC_DCHECK_EQ?

RTC_DCHECK_EQ does not work well with scoped enums. Probably a fix for another
CL.

https://codereview.chromium.org/2182603002/diff/20001/webrtc/modules/pacing/b...
File webrtc/modules/pacing/bitrate_prober_unittest.cc (right):

https://codereview.chromium.org/2182603002/diff/20001/webrtc/modules/pacing/b...
webrtc/modules/pacing/bitrate_prober_unittest.cc:68: prober.PacketSent(now_ms,
1000);
On 2016/07/29 07:12:07, stefan-webrtc (holmer) wrote:
> Did you add this to test the inactivity threshold?

Yes, this is necessary (and makes sense in the flow) for the test with the
changes in TimeUntilNextProbe().

https://codereview.chromium.org/2182603002/diff/20001/webrtc/modules/pacing/p...
File webrtc/modules/pacing/paced_sender.cc (left):

https://codereview.chromium.org/2182603002/diff/20001/webrtc/modules/pacing/p...
webrtc/modules/pacing/paced_sender.cc:441: if (paused_ && packet.priority !=
kHighPriority)
On 2016/07/29 07:12:07, stefan-webrtc (holmer) wrote:
> If there're still bugs in the network layer code which causes the pacer to be
> paused when it's not supposed to be, removing this can break audio. Therefore
> I'd prefer if we at least did it in a separate CL from all the other
> improvements you are making here.
> 
> Same for the other TODOs in this file that you're fixing.

ok I will pull the paused changes into a seperate CL

[Irfan, 2016-08-01 18:31:52]

Patchset #2 (id:40001) has been deleted

[Irfan, 2016-08-01 18:32:33]

PS#2 addresses comments and reverts the paused changes that are supposed to be
fixed according to bug

[stefan-webrtc, 2016-08-02 11:59:27]

lgtm

https://codereview.webrtc.org/2182603002/diff/80001/webrtc/modules/pacing/bit...
File webrtc/modules/pacing/bitrate_prober.h (right):

https://codereview.webrtc.org/2182603002/diff/80001/webrtc/modules/pacing/bit...
webrtc/modules/pacing/bitrate_prober.h:59: void ResetState();
Put methods after enums/classes

[Irfan, 2016-08-02 16:44:30]

https://codereview.chromium.org/2182603002/diff/80001/webrtc/modules/pacing/b...
File webrtc/modules/pacing/bitrate_prober.h (right):

https://codereview.chromium.org/2182603002/diff/80001/webrtc/modules/pacing/b...
webrtc/modules/pacing/bitrate_prober.h:59: void ResetState();
On 2016/08/02 11:59:27, stefan-webrtc (holmer) wrote:
> Put methods after enums/classes

Done.

[Irfan, 2016-08-02 19:45:15]

The CQ bit was checked by isheriff@chromium.org

[Irfan, 2016-08-02 19:45:15]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2182603002/#ps100001 (title:
"Fix nits")

[commit-bot: I haz the power, 2016-08-02 19:45:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-08-02 19:56:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-02 19:56:32]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)

[Irfan, 2016-08-02 19:57:50]

Description was changed from

==========
Bitrate prober and paced sender improvements

- Removes unnecessary casts to compute timedelta.
- Renames ProbingState for clarity. This should help when we probe mid-call.
- Enables probing by default to avoid checking on each incoming packet.
- Removes duplicate probing state tracking in paced sender. These duplicate
  states were conflicting at times.
- Removes passing through packets for bug 5307 which seems long fixed.
- Cleanup handling of time_last_send_ms and avoid side effects of changing
  probing state at TimeUntilNextProbe().
- Clear cluster data when probing is restarted to avoid having old data after a
reset.

BUG=5859
==========

to

==========
Bitrate prober and paced sender improvements

- Removes unnecessary casts to compute timedelta.
- Renames ProbingState for clarity. This should help when we probe mid-call.
- Enables probing by default to avoid checking on each incoming packet.
- Removes duplicate probing state tracking in paced sender. These duplicate
  states were conflicting at times.
- Removes passing through packets for bug 5307 which seems long fixed.
- Cleanup handling of time_last_send_ms and avoid side effects of changing
  probing state at TimeUntilNextProbe().
- Clear cluster data when probing is restarted to avoid having old data after a
reset.

BUG=5859
R=stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/6e11efa6dced914e6f0927db3...
==========

[Irfan, 2016-08-02 19:57:52]

Committed patchset #4 (id:100001) manually as
6e11efa6dced914e6f0927db3fcc92d875ec2cf5 (presubmit successful).

[commit-bot: I haz the power, 2016-08-02 19:57:53]

Description was changed from

==========
Bitrate prober and paced sender improvements

- Removes unnecessary casts to compute timedelta.
- Renames ProbingState for clarity. This should help when we probe mid-call.
- Enables probing by default to avoid checking on each incoming packet.
- Removes duplicate probing state tracking in paced sender. These duplicate
  states were conflicting at times.
- Removes passing through packets for bug 5307 which seems long fixed.
- Cleanup handling of time_last_send_ms and avoid side effects of changing
  probing state at TimeUntilNextProbe().
- Clear cluster data when probing is restarted to avoid having old data after a
reset.

BUG=5859
R=stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/6e11efa6dced914e6f0927db3...
==========

to

==========
Bitrate prober and paced sender improvements

- Removes unnecessary casts to compute timedelta.
- Renames ProbingState for clarity. This should help when we probe mid-call.
- Enables probing by default to avoid checking on each incoming packet.
- Removes duplicate probing state tracking in paced sender. These duplicate
  states were conflicting at times.
- Removes passing through packets for bug 5307 which seems long fixed.
- Cleanup handling of time_last_send_ms and avoid side effects of changing
  probing state at TimeUntilNextProbe().
- Clear cluster data when probing is restarted to avoid having old data after a
reset.

BUG=5859
R=stefan@webrtc.org

Committed: https://crrev.com/6e11efa6dced914e6f0927db3fcc92d875ec2cf5
Cr-Commit-Position: refs/heads/master@{#13612}
==========