Fix issue where the number of packets reported in tests/simulations sometimes are negative.

Fix issue where the number of packets reported in tests/simulations sometimes are negative.

BUG=webrtc:6159
Committed: https://crrev.com/d48717b45515283e432f4a49abcc050ef5a47085
Cr-Commit-Position: refs/heads/master@{#13850}

[stefan-webrtc, 2016-08-08 14:04:12]

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

[terelius, 2016-08-09 08:31:07]

https://codereview.webrtc.org/2223033002/diff/1/webrtc/modules/bitrate_contro...
File webrtc/modules/bitrate_controller/bitrate_controller_impl.cc (right):

https://codereview.webrtc.org/2223033002/diff/1/webrtc/modules/bitrate_contro...
webrtc/modules/bitrate_controller/bitrate_controller_impl.cc:66: if
(total_number_of_packets < 0) {
Can/should this happen, or are we hiding bugs by ignoring bad receiver reports?

[stefan-webrtc, 2016-08-09 08:56:04]

https://codereview.webrtc.org/2223033002/diff/1/webrtc/modules/bitrate_contro...
File webrtc/modules/bitrate_controller/bitrate_controller_impl.cc (right):

https://codereview.webrtc.org/2223033002/diff/1/webrtc/modules/bitrate_contro...
webrtc/modules/bitrate_controller/bitrate_controller_impl.cc:66: if
(total_number_of_packets < 0) {
On 2016/08/09 08:31:07, terelius wrote:
> Can/should this happen, or are we hiding bugs by ignoring bad receiver
reports?

It can happen since a malicious RTCP sender could decide to send us a weird
report.

[terelius, 2016-08-09 11:28:37]

https://codereview.webrtc.org/2223033002/diff/1/webrtc/modules/bitrate_contro...
File webrtc/modules/bitrate_controller/bitrate_controller_impl.cc (right):

https://codereview.webrtc.org/2223033002/diff/1/webrtc/modules/bitrate_contro...
webrtc/modules/bitrate_controller/bitrate_controller_impl.cc:66: if
(total_number_of_packets < 0) {
On 2016/08/09 08:56:03, stefan-webrtc (holmer) wrote:
> On 2016/08/09 08:31:07, terelius wrote:
> > Can/should this happen, or are we hiding bugs by ignoring bad receiver
> reports?
> 
> It can happen since a malicious RTCP sender could decide to send us a weird
> report.

Good point. Is/should there be a unit test for this? (If I understood the rest
of the CL correctly, you changed the tests so that the receiver reports are
sensible, but that also means that this isn't tested.)

[stefan-webrtc, 2016-08-09 11:32:13]

https://codereview.webrtc.org/2223033002/diff/1/webrtc/modules/bitrate_contro...
File webrtc/modules/bitrate_controller/bitrate_controller_impl.cc (right):

https://codereview.webrtc.org/2223033002/diff/1/webrtc/modules/bitrate_contro...
webrtc/modules/bitrate_controller/bitrate_controller_impl.cc:66: if
(total_number_of_packets < 0) {
On 2016/08/09 11:28:37, terelius wrote:
> On 2016/08/09 08:56:03, stefan-webrtc (holmer) wrote:
> > On 2016/08/09 08:31:07, terelius wrote:
> > > Can/should this happen, or are we hiding bugs by ignoring bad receiver
> > reports?
> > 
> > It can happen since a malicious RTCP sender could decide to send us a weird
> > report.
> 
> Good point. Is/should there be a unit test for this? (If I understood the rest
> of the CL correctly, you changed the tests so that the receiver reports are
> sensible, but that also means that this isn't tested.)

Right. We could add one, but I'm not sure exactly how we would verify that it's
working as intended... Maybe by making sure the BWE isn't affected if we give it
a report with a bogus extendedHighSeqNum and a high fraction lost?

[terelius, 2016-08-10 09:45:12]

https://codereview.webrtc.org/2223033002/diff/1/webrtc/modules/bitrate_contro...
File webrtc/modules/bitrate_controller/bitrate_controller_impl.cc (right):

https://codereview.webrtc.org/2223033002/diff/1/webrtc/modules/bitrate_contro...
webrtc/modules/bitrate_controller/bitrate_controller_impl.cc:66: if
(total_number_of_packets < 0) {
On 2016/08/09 11:32:12, stefan-webrtc (holmer) wrote:
> On 2016/08/09 11:28:37, terelius wrote:
> > On 2016/08/09 08:56:03, stefan-webrtc (holmer) wrote:
> > > On 2016/08/09 08:31:07, terelius wrote:
> > > > Can/should this happen, or are we hiding bugs by ignoring bad receiver
> > > reports?
> > > 
> > > It can happen since a malicious RTCP sender could decide to send us a
weird
> > > report.
> > 
> > Good point. Is/should there be a unit test for this? (If I understood the
rest
> > of the CL correctly, you changed the tests so that the receiver reports are
> > sensible, but that also means that this isn't tested.)
> 
> Right. We could add one, but I'm not sure exactly how we would verify that
it's
> working as intended... Maybe by making sure the BWE isn't affected if we give
it
> a report with a bogus extendedHighSeqNum and a high fraction lost?

Yes, or possibly check that owner_->OnReceivedRtcpReceiverReport() isn't called.
But maybe that is hard to mock?

[terelius, 2016-08-18 14:14:26]

I'd like to have a test to check that bogus receiver reports don't affect the
estimate, but if it is too difficult to create such a test maybe we should just
commit this as-is?

lgtm

[stefan-webrtc, 2016-08-22 13:29:53]

The CQ bit was checked by stefan@webrtc.org

[commit-bot: I haz the power, 2016-08-22 13:30:01]

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-22 15:50:36]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-08-22 15:50:37]

Description was changed from

==========
Fix issue where the number of packets reported in tests/simulations sometimes
are negative.

BUG=webrtc:6159
==========

to

==========
Fix issue where the number of packets reported in tests/simulations sometimes
are negative.

BUG=webrtc:6159

Committed: https://crrev.com/d48717b45515283e432f4a49abcc050ef5a47085
Cr-Commit-Position: refs/heads/master@{#13850}
==========

[commit-bot: I haz the power, 2016-08-22 15:50:39]

Patchset 1 (id:??) landed as
https://crrev.com/d48717b45515283e432f4a49abcc050ef5a47085
Cr-Commit-Position: refs/heads/master@{#13850}