[rtp_rtcp] ReportBlockInformation cleaned and moved out of the rtcp_receiver_help.

[rtp_rtcp] ReportBlockInformation cleaned and moved out of the rtcp_receiver_help.
  filling ReportBlockInformation moved from RtcpReceiver::HandleReportBlock to the class itself.
  maxjitter calculation dropped because is not used.

BUG=webrtc:5565

[danilchap, 2016-02-01 12:53:44]

Description was changed from

==========
[rtp_rtcp] ReportBlockInformation cleaned and moved out of the
rtcp_receiver_help.
  filling ReportBlockInformation moved from RtcpReceiver::HandleReportBlock to
the class itself.
  maxjitter calculation dropped because is not used.
  RTT calculation changed to use compact ntp representation instead of keeping
mapping to less precise milliseconds. (that duplicated half of the
CL#1491843004).
  To make change above more visible, mapping for sender report send times
removed from rtcp_sender.

R=åsapersson
BUG=webrtc:5260
==========

to

==========
[rtp_rtcp] ReportBlockInformation cleaned and moved out of the
rtcp_receiver_help.
  filling ReportBlockInformation moved from RtcpReceiver::HandleReportBlock to
the class itself.
  maxjitter calculation dropped because is not used.
  RTT calculation changed to use compact ntp representation instead of keeping
mapping to less precise milliseconds. (that duplicated half of the
CL#1491843004).
  To make change above more visible, mapping for sender report send times
removed from rtcp_sender.

R=åsapersson
BUG=webrtc:5260
==========

[danilchap, 2016-02-01 12:53:44]

danilchap@webrtc.org changed reviewers:
- asapersson@webrtc.org

[danilchap, 2016-02-22 19:03:37]

Description was changed from

==========
[rtp_rtcp] ReportBlockInformation cleaned and moved out of the
rtcp_receiver_help.
  filling ReportBlockInformation moved from RtcpReceiver::HandleReportBlock to
the class itself.
  maxjitter calculation dropped because is not used.
  RTT calculation changed to use compact ntp representation instead of keeping
mapping to less precise milliseconds. (that duplicated half of the
CL#1491843004).
  To make change above more visible, mapping for sender report send times
removed from rtcp_sender.

R=åsapersson
BUG=webrtc:5260
==========

to

==========
[rtp_rtcp] ReportBlockInformation cleaned and moved out of the
rtcp_receiver_help.
  filling ReportBlockInformation moved from RtcpReceiver::HandleReportBlock to
the class itself.
  maxjitter calculation dropped because is not used.

BUG=webrtc:5565
==========

[danilchap, 2016-02-25 10:42:25]

danilchap@webrtc.org changed reviewers:
+ philipel@webrtc.org

[philipel, 2016-02-29 13:21:48]

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/report_block_information.cc (right):

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.cc:37:
RTC_CHECK_GT(number_rtt_, 0u);
Expected value first:
RTC_CHECK_GT(0u, number_rtt_);

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/report_block_information.h (right):

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.h:43: const
RTCPReportBlock& LastBlock() const { return last_received_block_; }
Move to .cc file.

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.h:44: bool HasRtt()
const { return number_rtt_ > 0; }
Move to .cc file.

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.h:45: uint32_t
LastRttMs() const;
If you don't rely on uin32_t arithmetic then change this and the next three
uint32_t to int.

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.h:55: size_t
number_rtt_;
Change to int.

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.h:57: uint32_t
last_rtt_;
int, for the following 3 as well.

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.h:60: uint64_t sum_rtt_;
int :)

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/report_block_information_unittest.cc
(right):

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information_unittest.cc:77:
EXPECT_EQ(rbi.LastBlock().sourceSSRC, kSourceSsrc);
Switch to expected value first:
EXPECT_EQ(kSourceSsrc, rbi.LastBlock().sourceSSRC);

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information_unittest.cc:101:
EXPECT_EQ(rbi.LastBlock().sourceSSRC, kSourceSsrc);
switch

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information_unittest.cc:121:
EXPECT_EQ(rbi.LastBlock().sourceSSRC, kSourceSsrc);
switch

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information_unittest.cc:152:
SimulatedClock clock(Rand<uint32_t>());
Why do you use Random values? If you seed the rand object then you will always
get the same sequence of numbers, and if you don't seed it then you might not be
able to reproduce it since you don't know the seed.

[danilchap, 2016-02-29 14:41:28]

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/report_block_information.cc (right):

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.cc:37:
RTC_CHECK_GT(number_rtt_, 0u);
On 2016/02/29 13:21:47, philipel wrote:
> Expected value first:
> RTC_CHECK_GT(0u, number_rtt_);
Disagree,
Code search shows that expected value last is used for CHECK_GT/LT/LE/GE
There are reasons to use expected value last even for CHECK_EQ.

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/report_block_information.h (right):

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.h:43: const
RTCPReportBlock& LastBlock() const { return last_received_block_; }
On 2016/02/29 13:21:48, philipel wrote:
> Move to .cc file.

Done.

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.h:44: bool HasRtt()
const { return number_rtt_ > 0; }
On 2016/02/29 13:21:48, philipel wrote:
> Move to .cc file.

Done.

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.h:45: uint32_t
LastRttMs() const;
On 2016/02/29 13:21:48, philipel wrote:
> If you don't rely on uin32_t arithmetic then change this and the next three
> uint32_t to int.

What is motivation for using int?
uint32 shows the range of possible values.

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.h:55: size_t
number_rtt_;
On 2016/02/29 13:21:48, philipel wrote:
> Change to int.

Isn't size_t default type for counting?

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.h:57: uint32_t
last_rtt_;
On 2016/02/29 13:21:48, philipel wrote:
> int, for the following 3 as well.

Acknowledged.

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.h:60: uint64_t sum_rtt_;
On 2016/02/29 13:21:48, philipel wrote:
> int :)

Unsure if 32 bits would be enough, prefer to use 64 bits to be sure.

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/report_block_information_unittest.cc
(right):

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information_unittest.cc:77:
EXPECT_EQ(rbi.LastBlock().sourceSSRC, kSourceSsrc);
On 2016/02/29 13:21:48, philipel wrote:
> Switch to expected value first:
> EXPECT_EQ(kSourceSsrc, rbi.LastBlock().sourceSSRC);

Done.

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information_unittest.cc:101:
EXPECT_EQ(rbi.LastBlock().sourceSSRC, kSourceSsrc);
On 2016/02/29 13:21:48, philipel wrote:
> switch

Done.

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information_unittest.cc:121:
EXPECT_EQ(rbi.LastBlock().sourceSSRC, kSourceSsrc);
On 2016/02/29 13:21:48, philipel wrote:
> switch

Done.

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information_unittest.cc:152:
SimulatedClock clock(Rand<uint32_t>());
On 2016/02/29 13:21:48, philipel wrote:
> Why do you use Random values? If you seed the rand object then you will always
> get the same sequence of numbers, and if you don't seed it then you might not
be
> able to reproduce it since you don't know the seed.

Random values demonstrate the range of values this test should work with.
It is also less obvious with human-generated values what are the relations
between constants.
Rand is seed and always get the same sequence of numbers to avoid flakiness.

[philipel, 2016-03-03 14:00:59]

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/report_block_information.cc (right):

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.cc:37:
RTC_CHECK_GT(number_rtt_, 0u);
On 2016/02/29 14:41:27, danilchap wrote:
> On 2016/02/29 13:21:47, philipel wrote:
> > Expected value first:
> > RTC_CHECK_GT(0u, number_rtt_);
> Disagree,
> Code search shows that expected value last is used for CHECK_GT/LT/LE/GE
> There are reasons to use expected value last even for CHECK_EQ.

I think you are right. Shouldn't it be DCHECK instead of CHECK though?

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/report_block_information.h (right):

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.h:45: uint32_t
LastRttMs() const;
On 2016/02/29 14:41:27, danilchap wrote:
> On 2016/02/29 13:21:48, philipel wrote:
> > If you don't rely on uin32_t arithmetic then change this and the next three
> > uint32_t to int.
> 
> What is motivation for using int?
> uint32 shows the range of possible values.

This should no longer be an int, it should now be an int64_t. Apparently we
always use int64_t for intervals of time and timestamps.

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.h:55: size_t
number_rtt_;
On 2016/02/29 14:41:27, danilchap wrote:
> On 2016/02/29 13:21:48, philipel wrote:
> > Change to int.
> 
> Isn't size_t default type for counting?

The style guide is not clear, size_t is fine.

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.h:57: uint32_t
last_rtt_;
On 2016/02/29 14:41:27, danilchap wrote:
> On 2016/02/29 13:21:48, philipel wrote:
> > int, for the following 3 as well.
> 
> Acknowledged.

Should use int64_t instead of uint32_t.

https://codereview.webrtc.org/1540383002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/report_block_information.cc (right):

https://codereview.webrtc.org/1540383002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.cc:66: uint32_t
ReportBlockInformation::LastRttMs() const {
Change to int64_t for Last, Min, Max and AvgRttMs.

https://codereview.webrtc.org/1540383002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.cc:81: uint32_t
send_time = last_received_block_.lastSR;
int64_t

https://codereview.webrtc.org/1540383002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.cc:82: uint32_t rtt_ms =
0;
int64_t

https://codereview.webrtc.org/1540383002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.cc:84: uint32_t delay =
last_received_block_.delaySinceLastSR;
int64_t

https://codereview.webrtc.org/1540383002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.cc:87: uint32_t rtt_ntp
= receive_time - delay - send_time;
int64_t

https://codereview.webrtc.org/1540383002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/report_block_information.h (right):

https://codereview.webrtc.org/1540383002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.h:53: void
AddRtt(uint32_t now);
(int64_t now) for storing thing related to time.

[danilchap, 2016-03-03 14:44:49]

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/report_block_information.cc (right):

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.cc:37:
RTC_CHECK_GT(number_rtt_, 0u);
On 2016/03/03 14:00:58, philipel wrote:
> On 2016/02/29 14:41:27, danilchap wrote:
> > On 2016/02/29 13:21:47, philipel wrote:
> > > Expected value first:
> > > RTC_CHECK_GT(0u, number_rtt_);
> > Disagree,
> > Code search shows that expected value last is used for CHECK_GT/LT/LE/GE
> > There are reasons to use expected value last even for CHECK_EQ.
> 
> I think you are right. Shouldn't it be DCHECK instead of CHECK though?

Done.

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/report_block_information.h (right):

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.h:45: uint32_t
LastRttMs() const;
On 2016/03/03 14:00:59, philipel wrote:
> On 2016/02/29 14:41:27, danilchap wrote:
> > On 2016/02/29 13:21:48, philipel wrote:
> > > If you don't rely on uin32_t arithmetic then change this and the next
three
> > > uint32_t to int.
> > 
> > What is motivation for using int?
> > uint32 shows the range of possible values.
> 
> This should no longer be an int, it should now be an int64_t. Apparently we
> always use int64_t for intervals of time and timestamps.

Not always, when time units are not ms, uint32_t is often used. E.g. both rtp
timestamp and compact ntp time use unsigned integers because of legal
wrap-around.
Though ms is returned here, will update to int64_t.

https://codereview.webrtc.org/1540383002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.h:57: uint32_t
last_rtt_;
On 2016/03/03 14:00:59, philipel wrote:
> On 2016/02/29 14:41:27, danilchap wrote:
> > On 2016/02/29 13:21:48, philipel wrote:
> > > int, for the following 3 as well.
> > 
> > Acknowledged.
> 
> Should use int64_t instead of uint32_t.

This time is based on compact ntp time units, so prefer to use unsigned type.

https://codereview.webrtc.org/1540383002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/report_block_information.cc (right):

https://codereview.webrtc.org/1540383002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.cc:66: uint32_t
ReportBlockInformation::LastRttMs() const {
On 2016/03/03 14:00:59, philipel wrote:
> Change to int64_t for Last, Min, Max and AvgRttMs.

Done.

https://codereview.webrtc.org/1540383002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.cc:81: uint32_t
send_time = last_received_block_.lastSR;
On 2016/03/03 14:00:59, philipel wrote:
> int64_t

No, see comment below.

https://codereview.webrtc.org/1540383002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.cc:82: uint32_t rtt_ms =
0;
On 2016/03/03 14:00:59, philipel wrote:
> int64_t

Done.

https://codereview.webrtc.org/1540383002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.cc:87: uint32_t rtt_ntp
= receive_time - delay - send_time;
On 2016/03/03 14:00:59, philipel wrote:
> int64_t

using int64_t in this function may produce incorrect result because of
wrap-around that happen every 2^16 seconds (~18 hours).

https://codereview.webrtc.org/1540383002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/report_block_information.h (right):

https://codereview.webrtc.org/1540383002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/report_block_information.h:53: void
AddRtt(uint32_t now);
On 2016/03/03 14:00:59, philipel wrote:
> (int64_t now) for storing thing related to time.

changed to NtpTime from compact ntp time for clarity.

[philipel, 2016-03-04 10:07:00]

lgtm