Reset InterArrival if arrival time clock makes a jump.

Reset InterArrival if arrival time clock makes a jump.

Also adds a copy of the BWE test suite to the new DelayBasedBwe class.

BUG=webrtc:6079
Committed: https://crrev.com/5e12d36ba7e7d5f83a79e3a427dfcaa70716ebe7
Cr-Commit-Position: refs/heads/master@{#13428}

[stefan-webrtc, 2016-07-06 11:53:46]

Cleanup

[stefan-webrtc, 2016-07-06 11:54:25]

Description was changed from

==========
Reset InterArrival if arrival time clock makes a jump.

BUG=webrtc:6079
==========

to

==========
Reset InterArrival if arrival time clock makes a jump.

Also adds a copy of the BWE test suite to the new DelayBasedBwe class.

BUG=webrtc:6079
==========

[stefan-webrtc, 2016-07-06 11:56:58]

stefan@webrtc.org changed reviewers:
+ philipel@webrtc.org, sprang@webrtc.org

[stefan-webrtc, 2016-07-06 11:56:58]

philipel, ptal at the tests I copied over (unittest helper, etc).

sprang, please review the functionality changes in detail.

[sprang_webrtc, 2016-07-06 12:46:37]

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/inter_arrival.cc (right):

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/inter_arrival.cc:63: if
(*arrival_time_delta_ms > system_time_delta_ms) {
I'm not sure I follow. arrival_time_delta_ms is the arrival time delta between
the last two packets. Since this is a new timestamp group, system_time_delta_ms
is the wall clock delta between the last two packets arriving in the
ComputeDeltas() call? Shouldn't these be in the same ballpark, possibly falsely
triggering this?

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/inter_arrival.cc:74: if
(num_consecutive_reordered_packets_ >= 2) {
Is 2 enough?
Make this a public named constant so it can be used in a unit test.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/remote_bit...
File
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc
(right):

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc:239:
assert(send_time_24bits < (1ul << 24));
RTC_CHECK

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc:245:
clock_->TimeInMilliseconds();
Remove this

[philipel, 2016-07-06 13:36:36]

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc (right):

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:14: #include
"webrtc/modules/congestion_controller/delay_based_bwe.h"
Move to top

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:23: static
constexpr int kNumProbes = 5;
Move both constexpr to namespace {...} and drop static.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:31: uint32_t
AbsSendTime(int64_t t, int64_t denom) {
Remove, since it's not used anymore.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:35: void
IncomingPacket(int64_t arrival_time,
Rename to AddPacketInfo or IncomingFeedback or something like that.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:36: int64_t
send_time_ms,
|arrival_time| -> |arrival_time_ms|

Nit: don't you think it would make more sense to have send_time_ms before
arrival_time_ms?

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:42:
send_time_ms, sequence_number, payload_size,
Sequence number is not used by delay_based_bwe, set it to 0 and remove the
|sequence_number| argument.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:78:
IncomingPacket(now_ms, now_ms, seq_num++, 1000, 1);
Note: 0 is a valid probe cluster id.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc
(right):

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:16:
#include "webrtc/modules/congestion_controller/delay_based_bwe.h"
Maybe move before system includes? Not sure...

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:40:
assert(fps_ > 0);
ASSERT_GT

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:51:
assert(packets != NULL);
ASSERT

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:90:
StreamGenerator::~StreamGenerator() {}
Remove empty destructor

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:131:
assert(capacity_ > 0);
ASSERT, ASSERT, ASSERT_GT

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:171:
IncomingPacket(payload_size, arrival_time_ms, send_time_ms, sequence_number,
Remove |sequence_number| argument and set to 0.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:181:
sequence_number, payload_size, probe_cluster_id);
Remove |sequence_number| argument and set to 0.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.h
(right):

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.h:112:
class DelayBasedBweTest : public ::testing::Test {
Can we rename this to something else? The problem right now is that the existing
tests are name "TestDelayBasedBwe" and this is named "DelayBasedBweTest", a bit
confusing :)

[stefan-webrtc, 2016-07-06 14:53:09]

Comments addressed.

[stefan-webrtc, 2016-07-06 14:53:24]

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc (right):

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:14: #include
"webrtc/modules/congestion_controller/delay_based_bwe.h"
On 2016/07/06 13:36:36, philipel wrote:
> Move to top

I don't think it should be at the top since this isn't the .cc file of the .h
file.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:23: static
constexpr int kNumProbes = 5;
On 2016/07/06 13:36:36, philipel wrote:
> Move both constexpr to namespace {...} and drop static.

Done.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:31: uint32_t
AbsSendTime(int64_t t, int64_t denom) {
On 2016/07/06 13:36:36, philipel wrote:
> Remove, since it's not used anymore.

Done.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:35: void
IncomingPacket(int64_t arrival_time,
On 2016/07/06 13:36:36, philipel wrote:
> Rename to AddPacketInfo or IncomingFeedback or something like that.

Done.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:36: int64_t
send_time_ms,
On 2016/07/06 13:36:36, philipel wrote:
> |arrival_time| -> |arrival_time_ms|

Done.

> 
> Nit: don't you think it would make more sense to have send_time_ms before
> arrival_time_ms?

Maybe, but this matches the class we're testing, so I'll keep as is for now.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:42:
send_time_ms, sequence_number, payload_size,
On 2016/07/06 13:36:36, philipel wrote:
> Sequence number is not used by delay_based_bwe, set it to 0 and remove the
> |sequence_number| argument.

I don't know, it seems like a good idea to pass in a valid sequence number if we
in the future actually decide to use it? Not too much code related to it.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:78:
IncomingPacket(now_ms, now_ms, seq_num++, 1000, 1);
On 2016/07/06 13:36:36, philipel wrote:
> Note: 0 is a valid probe cluster id.

Yes, I happened to change it because I thought it wasn't, but then realized what
was really wrong... Changing back.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc
(right):

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:16:
#include "webrtc/modules/congestion_controller/delay_based_bwe.h"
On 2016/07/06 13:36:36, philipel wrote:
> Maybe move before system includes? Not sure...

No, should be after according to style guides.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:51:
assert(packets != NULL);
On 2016/07/06 13:36:36, philipel wrote:
> ASSERT

Can only be done from functions returning void. I'll change to a check.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:90:
StreamGenerator::~StreamGenerator() {}
On 2016/07/06 13:36:36, philipel wrote:
> Remove empty destructor

Done.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:131:
assert(capacity_ > 0);
On 2016/07/06 13:36:36, philipel wrote:
> ASSERT, ASSERT, ASSERT_GT

Same here.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.h
(right):

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.h:112:
class DelayBasedBweTest : public ::testing::Test {
On 2016/07/06 13:36:36, philipel wrote:
> Can we rename this to something else? The problem right now is that the
existing
> tests are name "TestDelayBasedBwe" and this is named "DelayBasedBweTest", a
bit
> confusing :)

I'm removing the existing test class the existing tests can be built on this
instead.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/inter_arrival.cc (right):

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/inter_arrival.cc:63: if
(*arrival_time_delta_ms > system_time_delta_ms) {
On 2016/07/06 12:46:36, språng wrote:
> I'm not sure I follow. arrival_time_delta_ms is the arrival time delta between
> the last two packets. Since this is a new timestamp group,
system_time_delta_ms
> is the wall clock delta between the last two packets arriving in the
> ComputeDeltas() call? Shouldn't these be in the same ballpark, possibly
falsely
> triggering this? 

You are right, changed after offline discussion.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/inter_arrival.cc:74: if
(num_consecutive_reordered_packets_ >= 2) {
On 2016/07/06 12:46:36, språng wrote:
> Is 2 enough?
> Make this a public named constant so it can be used in a unit test.

3 should be enough...

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/remote_bit...
File
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc
(right):

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc:239:
assert(send_time_24bits < (1ul << 24));
On 2016/07/06 12:46:37, språng wrote:
> RTC_CHECK

Done.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc:245:
clock_->TimeInMilliseconds();
On 2016/07/06 12:46:36, språng wrote:
> Remove this

Done.

[stefan-webrtc, 2016-07-06 14:56:36]

Clean

[stefan-webrtc, 2016-07-06 15:02:44]

Remove no longer needed test.

[sprang_webrtc, 2016-07-06 15:08:14]

lgtm

[philipel, 2016-07-07 08:11:38]

lgtm

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc (right):

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:14: #include
"webrtc/modules/congestion_controller/delay_based_bwe.h"
On 2016/07/06 14:53:24, stefan-webrtc (holmer) wrote:
> On 2016/07/06 13:36:36, philipel wrote:
> > Move to top
> 
> I don't think it should be at the top since this isn't the .cc file of the .h
> file. 

Right, my bad.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:42:
send_time_ms, sequence_number, payload_size,
On 2016/07/06 14:53:24, stefan-webrtc (holmer) wrote:
> On 2016/07/06 13:36:36, philipel wrote:
> > Sequence number is not used by delay_based_bwe, set it to 0 and remove the
> > |sequence_number| argument.
> 
> I don't know, it seems like a good idea to pass in a valid sequence number if
we
> in the future actually decide to use it? Not too much code related to it.

That makes sense.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc
(right):

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:16:
#include "webrtc/modules/congestion_controller/delay_based_bwe.h"
On 2016/07/06 14:53:24, stefan-webrtc (holmer) wrote:
> On 2016/07/06 13:36:36, philipel wrote:
> > Maybe move before system includes? Not sure...
> 
> No, should be after according to style guides.

Acknowledged.

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.h
(right):

https://codereview.webrtc.org/2126793002/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.h:112:
class DelayBasedBweTest : public ::testing::Test {
On 2016/07/06 14:53:24, stefan-webrtc (holmer) wrote:
> On 2016/07/06 13:36:36, philipel wrote:
> > Can we rename this to something else? The problem right now is that the
> existing
> > tests are name "TestDelayBasedBwe" and this is named "DelayBasedBweTest", a
> bit
> > confusing :)
> 
> I'm removing the existing test class the existing tests can be built on this
> instead.

Acknowledged.

[stefan-webrtc, 2016-07-07 08:24:48]

The CQ bit was checked by stefan@webrtc.org

[commit-bot: I haz the power, 2016-07-07 08:24:56]

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-07-07 08:34:53]

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

[commit-bot: I haz the power, 2016-07-07 08:34:55]

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

[stefan-webrtc, 2016-07-07 08:41:23]

fix build

[stefan-webrtc, 2016-07-07 08:42:35]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2016-07-07 08:42:35]

The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org,
sprang@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2126793002/#ps100001 (title:
"fix build")

[commit-bot: I haz the power, 2016-07-07 08:42:42]

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-07-07 08:53:43]

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

[commit-bot: I haz the power, 2016-07-07 08:53:44]

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

[stefan-webrtc, 2016-07-11 07:38:38]

Fix a few test issues.

[stefan-webrtc, 2016-07-11 07:39:10]

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

[commit-bot: I haz the power, 2016-07-11 07:39:17]

Dry run: 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-07-11 08:13:55]

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

[commit-bot: I haz the power, 2016-07-11 08:13:55]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2016-07-11 08:24:17]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2016-07-11 08:24:17]

The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org,
sprang@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2126793002/#ps120001 (title:
"Fix a few test issues.")

[commit-bot: I haz the power, 2016-07-11 08:24:25]

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-07-11 08:27:35]

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

[commit-bot: I haz the power, 2016-07-11 08:27:36]

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

[stefan-webrtc, 2016-07-11 08:32:56]

stefan@webrtc.org changed reviewers:
+ henrika@webrtc.org

[stefan-webrtc, 2016-07-11 08:32:56]

henrik, can you please approve changes to modules/BUILD/.gyp files?

[henrika_webrtc, 2016-07-11 08:36:16]

LGTM

[stefan-webrtc, 2016-07-11 08:41:02]

The CQ bit was checked by stefan@webrtc.org

[commit-bot: I haz the power, 2016-07-11 08:41:06]

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-07-11 08:44:07]

Description was changed from

==========
Reset InterArrival if arrival time clock makes a jump.

Also adds a copy of the BWE test suite to the new DelayBasedBwe class.

BUG=webrtc:6079
==========

to

==========
Reset InterArrival if arrival time clock makes a jump.

Also adds a copy of the BWE test suite to the new DelayBasedBwe class.

BUG=webrtc:6079
==========

[commit-bot: I haz the power, 2016-07-11 08:44:10]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-11 08:44:13]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-11 08:44:19]

Description was changed from

==========
Reset InterArrival if arrival time clock makes a jump.

Also adds a copy of the BWE test suite to the new DelayBasedBwe class.

BUG=webrtc:6079
==========

to

==========
Reset InterArrival if arrival time clock makes a jump.

Also adds a copy of the BWE test suite to the new DelayBasedBwe class.

BUG=webrtc:6079

Committed: https://crrev.com/5e12d36ba7e7d5f83a79e3a427dfcaa70716ebe7
Cr-Commit-Position: refs/heads/master@{#13428}
==========

[commit-bot: I haz the power, 2016-07-11 08:44:20]

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