Use CallStats for RTT in Call, rather than VideoSendStream::GetRtt()

Use CallStats for RTT in Call, rather than VideoSendStream::GetRtt()

Also move some stats reporting from vie_channel to send stats proxy

BUG=

Committed: https://crrev.com/e2d83d6560272ee68cf99c4fd4f78a437adeb98c
Cr-Commit-Position: refs/heads/master@{#11688}

[sprang_webrtc, 2016-02-04 10:04:32]

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

[åsapersson, 2016-02-05 13:50:13]

https://codereview.webrtc.org/1669623004/diff/20001/webrtc/video/call_stats.cc
File webrtc/video/call_stats.cc (right):

https://codereview.webrtc.org/1669623004/diff/20001/webrtc/video/call_stats.c...
webrtc/video/call_stats.cc:121: // TODO(asapersson): Consider changing this to
report the average rtt.
this todo seems no longer valid, maybe remove

https://codereview.webrtc.org/1669623004/diff/20001/webrtc/video/send_statist...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1669623004/diff/20001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy.cc:361:
uma_container_->report_block_stats_.Store(statistics, ssrc, 0);
Should this be Store(statistics, 0, ssrc)?

https://codereview.webrtc.org/1669623004/diff/20001/webrtc/video/vie_channel.cc
File webrtc/video/vie_channel.cc (right):

https://codereview.webrtc.org/1669623004/diff/20001/webrtc/video/vie_channel....
webrtc/video/vie_channel.cc:35: #include "webrtc/video/report_block_stats.h"
remove

https://codereview.webrtc.org/1669623004/diff/20001/webrtc/video/vie_channel.h
File webrtc/video/vie_channel.h (right):

https://codereview.webrtc.org/1669623004/diff/20001/webrtc/video/vie_channel....
webrtc/video/vie_channel.h:44: class ReportBlockStats;
remove

https://codereview.webrtc.org/1669623004/diff/20001/webrtc/video/vie_channel....
webrtc/video/vie_channel.h:151: int64_t* rtt_ms);
remove?

[sprang, 2016-02-05 14:10:21]

sprang@google.com changed reviewers:
+ sprang@google.com

[sprang, 2016-02-05 14:10:22]

Addressed comments, but also moved rtt histogram reporting to CallStats plus
added a test for that. wdyt?

https://codereview.webrtc.org/1669623004/diff/20001/webrtc/video/call_stats.cc
File webrtc/video/call_stats.cc (right):

https://codereview.webrtc.org/1669623004/diff/20001/webrtc/video/call_stats.c...
webrtc/video/call_stats.cc:121: // TODO(asapersson): Consider changing this to
report the average rtt.
On 2016/02/05 13:50:12, åsapersson wrote:
> this todo seems no longer valid, maybe remove

Done.

https://codereview.webrtc.org/1669623004/diff/20001/webrtc/video/send_statist...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1669623004/diff/20001/webrtc/video/send_statist...
webrtc/video/send_statistics_proxy.cc:361:
uma_container_->report_block_stats_.Store(statistics, ssrc, 0);
On 2016/02/05 13:50:12, åsapersson wrote:
> Should this be Store(statistics, 0, ssrc)?

Done.

https://codereview.webrtc.org/1669623004/diff/20001/webrtc/video/vie_channel.cc
File webrtc/video/vie_channel.cc (right):

https://codereview.webrtc.org/1669623004/diff/20001/webrtc/video/vie_channel....
webrtc/video/vie_channel.cc:35: #include "webrtc/video/report_block_stats.h"
On 2016/02/05 13:50:12, åsapersson wrote:
> remove

Done.

https://codereview.webrtc.org/1669623004/diff/20001/webrtc/video/vie_channel.h
File webrtc/video/vie_channel.h (right):

https://codereview.webrtc.org/1669623004/diff/20001/webrtc/video/vie_channel....
webrtc/video/vie_channel.h:44: class ReportBlockStats;
On 2016/02/05 13:50:12, åsapersson wrote:
> remove

Done.

https://codereview.webrtc.org/1669623004/diff/20001/webrtc/video/vie_channel....
webrtc/video/vie_channel.h:151: int64_t* rtt_ms);
On 2016/02/05 13:50:12, åsapersson wrote:
> remove?

Done.

[åsapersson, 2016-02-08 09:26:14]

https://codereview.webrtc.org/1669623004/diff/60001/webrtc/video/call_stats.cc
File webrtc/video/call_stats.cc (right):

https://codereview.webrtc.org/1669623004/diff/60001/webrtc/video/call_stats.c...
webrtc/video/call_stats.cc:102: RTC_CHECK(observers_.empty());
maybe DCHECK?

https://codereview.webrtc.org/1669623004/diff/60001/webrtc/video/call_stats.c...
webrtc/video/call_stats.cc:171: rtc::CritScope cs(&crit_);
if time_of_first_rtt_ms_ != -1

https://codereview.webrtc.org/1669623004/diff/60001/webrtc/video/call_stats.c...
webrtc/video/call_stats.cc:174: if (avg_rtt_ms() != -1 && elapsed_sec >=
metrics::kMinRunTimeInSeconds) {
avg_rtt_ms_?

https://codereview.webrtc.org/1669623004/diff/60001/webrtc/video/call_stats.c...
webrtc/video/call_stats.cc:176:
"WebRTC.Video.AverageRoundTripTimeInMilliseconds", avg_rtt_ms());
ditto

https://codereview.webrtc.org/1669623004/diff/60001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/1669623004/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:2104: sender_call_->GetStats();
can this be removed?

https://codereview.webrtc.org/1669623004/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:2202: if (screenshare) {
use video_prefix?

[sprang, 2016-02-08 10:34:24]

https://codereview.webrtc.org/1669623004/diff/60001/webrtc/video/call_stats.cc
File webrtc/video/call_stats.cc (right):

https://codereview.webrtc.org/1669623004/diff/60001/webrtc/video/call_stats.c...
webrtc/video/call_stats.cc:102: RTC_CHECK(observers_.empty());
On 2016/02/08 09:26:14, åsapersson wrote:
> maybe DCHECK?

Done.

https://codereview.webrtc.org/1669623004/diff/60001/webrtc/video/call_stats.c...
webrtc/video/call_stats.cc:171: rtc::CritScope cs(&crit_);
On 2016/02/08 09:26:14, åsapersson wrote:
> if time_of_first_rtt_ms_ != -1

Done.

https://codereview.webrtc.org/1669623004/diff/60001/webrtc/video/call_stats.c...
webrtc/video/call_stats.cc:174: if (avg_rtt_ms() != -1 && elapsed_sec >=
metrics::kMinRunTimeInSeconds) {
On 2016/02/08 09:26:14, åsapersson wrote:
> avg_rtt_ms_?

Done.

https://codereview.webrtc.org/1669623004/diff/60001/webrtc/video/call_stats.c...
webrtc/video/call_stats.cc:176:
"WebRTC.Video.AverageRoundTripTimeInMilliseconds", avg_rtt_ms());
On 2016/02/08 09:26:14, åsapersson wrote:
> ditto

Done.

https://codereview.webrtc.org/1669623004/diff/60001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/1669623004/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:2104: sender_call_->GetStats();
On 2016/02/08 09:26:14, åsapersson wrote:
> can this be removed?

Done.

https://codereview.webrtc.org/1669623004/diff/60001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:2202: if (screenshare) {
On 2016/02/08 09:26:14, åsapersson wrote:
> use video_prefix?

Done.

[åsapersson, 2016-02-08 11:00:52]

https://codereview.webrtc.org/1669623004/diff/80001/webrtc/video/call_stats.cc
File webrtc/video/call_stats.cc (right):

https://codereview.webrtc.org/1669623004/diff/80001/webrtc/video/call_stats.c...
webrtc/video/call_stats.cc:179:
"WebRTC.Video.AverageRoundTripTimeInMilliseconds", avg_rtt_ms_);
Should this report average of avg_rtt_ms_?

[sprang, 2016-02-08 11:12:13]

https://codereview.webrtc.org/1669623004/diff/80001/webrtc/video/call_stats.cc
File webrtc/video/call_stats.cc (right):

https://codereview.webrtc.org/1669623004/diff/80001/webrtc/video/call_stats.c...
webrtc/video/call_stats.cc:179:
"WebRTC.Video.AverageRoundTripTimeInMilliseconds", avg_rtt_ms_);
On 2016/02/08 11:00:51, åsapersson wrote:
> Should this report average of avg_rtt_ms_? 

Yes, that's probably a good idea. Changed to record the average average RTT
instead.

[åsapersson, 2016-02-08 11:22:31]

https://codereview.webrtc.org/1669623004/diff/100001/webrtc/video/call_stats.cc
File webrtc/video/call_stats.cc (right):

https://codereview.webrtc.org/1669623004/diff/100001/webrtc/video/call_stats....
webrtc/video/call_stats.cc:183: if (avg_rtt_ms != -1 && elapsed_sec >=
metrics::kMinRunTimeInSeconds) {
maybe
int64_t avg_rtt_ms = (sum_avg_rtt_ms_ + num_avg_rtt_ / 2) / num_avg_rtt_;

and skip check avg_rtt_ms != -1

[sprang, 2016-02-08 12:20:45]

https://codereview.webrtc.org/1669623004/diff/100001/webrtc/video/call_stats.cc
File webrtc/video/call_stats.cc (right):

https://codereview.webrtc.org/1669623004/diff/100001/webrtc/video/call_stats....
webrtc/video/call_stats.cc:183: if (avg_rtt_ms != -1 && elapsed_sec >=
metrics::kMinRunTimeInSeconds) {
On 2016/02/08 11:22:31, åsapersson wrote:
> maybe
> int64_t avg_rtt_ms = (sum_avg_rtt_ms_ + num_avg_rtt_ / 2) / num_avg_rtt_;
> 
> and skip check avg_rtt_ms != -1

Done.

[åsapersson, 2016-02-08 12:31:10]

lgtm

[åsapersson, 2016-02-10 10:35:58]

https://codereview.webrtc.org/1669623004/diff/120001/webrtc/video/call_stats.cc
File webrtc/video/call_stats.cc (right):

https://codereview.webrtc.org/1669623004/diff/120001/webrtc/video/call_stats....
webrtc/video/call_stats.cc:98: avg_rtt_ms_(-1),
Note, now that rtt can be -1 if not set, there might be some places that needs
to be updated when checking if there is a valid rtt (e.g. vie_receiver.cc, line
429). I think rtt used to be unsigned and zero considered invalid.

[sprang_webrtc, 2016-02-15 09:21:28]

sprang@webrtc.org changed reviewers:
- sprang@google.com

[sprang_webrtc, 2016-02-15 09:21:29]

ping stefan

https://codereview.webrtc.org/1669623004/diff/120001/webrtc/video/call_stats.cc
File webrtc/video/call_stats.cc (right):

https://codereview.webrtc.org/1669623004/diff/120001/webrtc/video/call_stats....
webrtc/video/call_stats.cc:98: avg_rtt_ms_(-1),
On 2016/02/10 10:35:58, åsapersson (ooo until Feb 29) wrote:
> Note, now that rtt can be -1 if not set, there might be some places that needs
> to be updated when checking if there is a valid rtt (e.g. vie_receiver.cc,
line
> 429). I think rtt used to be unsigned and zero considered invalid.
> 
> 
>  

Right, I updated the call from the rtp module to only use the value if it is
valid. All other rtt calls use that as a proxy, so behavior should be the same
now.

[stefan-webrtc, 2016-02-19 10:44:17]

lgtm, but consider my last comments.

https://codereview.webrtc.org/1669623004/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/1669623004/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:188: if (last_rtt >= 0)
I have some memory of 0 not being a valid rtt, for instance here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

Probably doesn't matter here though, so feel free to ignore.

https://codereview.webrtc.org/1669623004/diff/140001/webrtc/video/call_stats.cc
File webrtc/video/call_stats.cc (right):

https://codereview.webrtc.org/1669623004/diff/140001/webrtc/video/call_stats....
webrtc/video/call_stats.cc:47: return -1;
Have you checked that we don't break any existing callers?

https://codereview.webrtc.org/1669623004/diff/140001/webrtc/video/call_stats....
webrtc/video/call_stats.cc:126: for (std::list<CallStatsObserver*>::iterator it
= observers_.begin();
Should we dcheck that avg_rtt_ms_ >= 0 too?

[sprang_webrtc, 2016-02-19 15:01:43]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2016-02-19 15:01:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
asapersson@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1669623004/#ps160001 (title:
"Addressed comment, rebase")

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1669623004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1669623004/160001

[sprang, 2016-02-19 15:03:15]

sprang@google.com changed reviewers:
+ sprang@google.com

[sprang, 2016-02-19 15:03:16]

https://codereview.webrtc.org/1669623004/diff/140001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/1669623004/diff/140001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:188: if (last_rtt >= 0)
On 2016/02/19 10:44:16, stefan-webrtc (holmer) wrote:
> I have some memory of 0 not being a valid rtt, for instance here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
> 
> Probably doesn't matter here though, so feel free to ignore.

Yes, but the behavior from outside won't change from before. So this should be
fine.

https://codereview.webrtc.org/1669623004/diff/140001/webrtc/video/call_stats.cc
File webrtc/video/call_stats.cc (right):

https://codereview.webrtc.org/1669623004/diff/140001/webrtc/video/call_stats....
webrtc/video/call_stats.cc:47: return -1;
On 2016/02/19 10:44:16, stefan-webrtc (holmer) wrote:
> Have you checked that we don't break any existing callers?

Yes, there was only one instance (see rtp_rtcp_impl.cc) which has been updated.

https://codereview.webrtc.org/1669623004/diff/140001/webrtc/video/call_stats....
webrtc/video/call_stats.cc:126: for (std::list<CallStatsObserver*>::iterator it
= observers_.begin();
On 2016/02/19 10:44:16, stefan-webrtc (holmer) wrote:
> Should we dcheck that avg_rtt_ms_ >= 0 too?

Done.

[commit-bot: I haz the power, 2016-02-19 17:03:30]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-02-19 17:03:37]

Description was changed from

==========
Use CallStats for RTT in Call, rather than VideoSendStream::GetRtt()

Also move some stats reporting from vie_channel to send stats proxy

BUG=
==========

to

==========
Use CallStats for RTT in Call, rather than VideoSendStream::GetRtt()

Also move some stats reporting from vie_channel to send stats proxy

BUG=

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

[commit-bot: I haz the power, 2016-02-19 17:03:38]

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