Wire up BWE stats through WebrtcSession so that they are filled in both for audio and video calls.

Wire up BWE stats through WebrtcSession so that they are filled in both for audio and video calls.

Prior to this CL Call::Stats were collected via WebRtcVideoEngine2, but not via WebRtcVoiceEngine, causing these stats to be missing for audio-only calls. Call lives on the peerconnection/session level and should only be collected once independent on how many streams we have.

BUG=webrtc:5079
R=deadbeef@webrtc.org, hbos@webrtc.org

Review-Url: https://codereview.webrtc.org/2863123002 .
Cr-Commit-Position: refs/heads/master@{#18384}
Committed: https://chromium.googlesource.com/external/webrtc/+/e80f4c91d0a2854a339e419162fdcd1d916f7de0

[stefan-webrtc, 2017-05-05 15:38:11]

stefan@webrtc.org changed reviewers:
+ deadbeef@webrtc.org, hbos@webrtc.org

[stefan-webrtc, 2017-05-05 15:41:54]

https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/rtcstatscollector.cc
File webrtc/pc/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/rtcstatscollector.c...
webrtc/pc/rtcstatscollector.cc:890: if (call_stats.send_bandwidth_bps > 0) {
Not sure this check makes sense, but kept it to not change the behavior. Wdyt
hbos?

[Taylor Brandstetter, 2017-05-07 21:30:44]

I don't really understand why FillBitrateInfo is needed, as opposed to adding
fields to existing stats structures. Can you give more background?

https://codereview.webrtc.org/2863123002/diff/1/webrtc/media/base/mediachannel.h
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/2863123002/diff/1/webrtc/media/base/mediachanne...
webrtc/media/base/mediachannel.h:1084: virtual bool
FillBitrateInfo(BandwidthEstimationInfo* bwe_info) = 0;
nit: Why not "FillBandwidthEstimationInfo(BandwidthEstimationInfo*)"?

Also, what's the advantage of adding a new method over just adding
"bw_estimations" to VoiceMediaInfo? Is there a time FillBitrateInfo will be
called outside of the stats context?

https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/channel.cc#newcode1991
webrtc/pc/channel.cc:1991: RTC_DCHECK(InvokeOnWorker(
Why is this in an RTC_DCHECK?

https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/rtcstatscollector.cc
File webrtc/pc/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/rtcstatscollector.c...
webrtc/pc/rtcstatscollector.cc:699: Call::Stats call_stats =
pc_->session()->GetCallStats();
This will cause a new invoke to the worker thread, which the new stats collector
implementation is meant to avoid. Maybe hbos has some thoughts on what to do.

https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/rtcstatscollector.c...
webrtc/pc/rtcstatscollector.cc:885: if (info.best_connection &&
video_media_info) {
Does this still need "&& video_media_info"?

https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/rtcstatscollector_u...
File webrtc/pc/rtcstatscollector_unittest.cc (right):

https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/rtcstatscollector_u...
webrtc/pc/rtcstatscollector_unittest.cc:1346: //
video_media_info.bw_estimations[0].available_recv_bandwidth = 0;
Remember to remove this before landing

https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/statscollector_unit...
File webrtc/pc/statscollector_unittest.cc (right):

https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/statscollector_unit...
webrtc/pc/statscollector_unittest.cc:947: const int kPacerDelay = 123;
nit: It would be more readable to me if the constants were declared together,
here and in the other test.

[stefan-webrtc, 2017-05-08 07:12:57]

https://codereview.webrtc.org/2863123002/diff/1/webrtc/media/base/mediachannel.h
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/2863123002/diff/1/webrtc/media/base/mediachanne...
webrtc/media/base/mediachannel.h:1084: virtual bool
FillBitrateInfo(BandwidthEstimationInfo* bwe_info) = 0;
On 2017/05/07 21:30:44, Taylor Brandstetter wrote:
> nit: Why not "FillBandwidthEstimationInfo(BandwidthEstimationInfo*)"?

It only fills the "bitrate parts" (rtx, video bitrate) of the
BandwidthEstimationInfo, since that's the part that isn't possible to get
through Call::GetStats, since they are statistics of the send streams. I think
it would make sense to change this in the future, but I added this method now
since I don't know how we should change it, and it probably deserves a separate
CL.

> 
> Also, what's the advantage of adding a new method over just adding
> "bw_estimations" to VoiceMediaInfo? Is there a time FillBitrateInfo will be
> called outside of the stats context?

I find it strange that the Call::Stats are collected via WebRtcVideoEngine2 (and
with your suggestion, WebRtcVoiceEngine). They are on a peerconnection/session
level and should only be collected once. That's why I decided to move it. I
don't think there's bw_estimations for both audio and video, there's only
bw_estimations for the currently active transport. Our current design is to have
one BWE per session.

https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/channel.cc#newcode1991
webrtc/pc/channel.cc:1991: RTC_DCHECK(InvokeOnWorker(
On 2017/05/07 21:30:44, Taylor Brandstetter wrote:
> Why is this in an RTC_DCHECK?

Copy paste from below, but my method didn't have to return bool... Changed it
now to call worker_thread_->Invoke<void>() instead.

https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/rtcstatscollector.cc
File webrtc/pc/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/rtcstatscollector.c...
webrtc/pc/rtcstatscollector.cc:699: Call::Stats call_stats =
pc_->session()->GetCallStats();
On 2017/05/07 21:30:44, Taylor Brandstetter wrote:
> This will cause a new invoke to the worker thread, which the new stats
collector
> implementation is meant to avoid. Maybe hbos has some thoughts on what to do.

I guess we could merge it with WebRtcSession::GetStats()?

https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/rtcstatscollector.c...
webrtc/pc/rtcstatscollector.cc:885: if (info.best_connection &&
video_media_info) {
On 2017/05/07 21:30:44, Taylor Brandstetter wrote:
> Does this still need "&& video_media_info"?

No probably not.

https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/rtcstatscollector_u...
File webrtc/pc/rtcstatscollector_unittest.cc (right):

https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/rtcstatscollector_u...
webrtc/pc/rtcstatscollector_unittest.cc:1346: //
video_media_info.bw_estimations[0].available_recv_bandwidth = 0;
On 2017/05/07 21:30:44, Taylor Brandstetter wrote:
> Remember to remove this before landing

Done.

https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/statscollector_unit...
File webrtc/pc/statscollector_unittest.cc (right):

https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/statscollector_unit...
webrtc/pc/statscollector_unittest.cc:947: const int kPacerDelay = 123;
On 2017/05/07 21:30:44, Taylor Brandstetter wrote:
> nit: It would be more readable to me if the constants were declared together,
> here and in the other test.

Done.

[stefan-webrtc, 2017-05-08 07:21:34]

On 2017/05/07 21:30:44, Taylor Brandstetter wrote:
> I don't really understand why FillBitrateInfo is needed, as opposed to adding
> fields to existing stats structures. Can you give more background?

Understandable, as it is a bit strange. To be able to get video encoder target
bitrate, actual encoder bitrate, rtx bitrate, padding bitrate, etc, into the
same graph as the BWE to make it easier to read and understand the graphs in
chrome://webrtc-internals, this was needed since BWE is owned by Call but enc
bitrate, rtx, etc, is owned by send streams. I'm not particularly fond of the
solution, and I think it deserves to be redone in a different way, but probably
not in this CL. This CL makes a small change to how the stats from
VideoSendStream is added to Call::Stats. A possibility could be to make a call
to "pc_->session()->video_channel()->GetStats(&video_info);" in
StatsCollector::ExtractBweInfo() and then do the merging there instead. Maybe
that's preferable?

> 
>
https://codereview.webrtc.org/2863123002/diff/1/webrtc/media/base/mediachannel.h
> File webrtc/media/base/mediachannel.h (right):
> 
>
https://codereview.webrtc.org/2863123002/diff/1/webrtc/media/base/mediachanne...
> webrtc/media/base/mediachannel.h:1084: virtual bool
> FillBitrateInfo(BandwidthEstimationInfo* bwe_info) = 0;
> nit: Why not "FillBandwidthEstimationInfo(BandwidthEstimationInfo*)"?
> 
> Also, what's the advantage of adding a new method over just adding
> "bw_estimations" to VoiceMediaInfo? Is there a time FillBitrateInfo will be
> called outside of the stats context?
> 
> https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/channel.cc
> File webrtc/pc/channel.cc (right):
> 
>
https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/channel.cc#newcode1991
> webrtc/pc/channel.cc:1991: RTC_DCHECK(InvokeOnWorker(
> Why is this in an RTC_DCHECK?
> 
> https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/rtcstatscollector.cc
> File webrtc/pc/rtcstatscollector.cc (right):
> 
>
https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/rtcstatscollector.c...
> webrtc/pc/rtcstatscollector.cc:699: Call::Stats call_stats =
> pc_->session()->GetCallStats();
> This will cause a new invoke to the worker thread, which the new stats
collector
> implementation is meant to avoid. Maybe hbos has some thoughts on what to do.
> 
>
https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/rtcstatscollector.c...
> webrtc/pc/rtcstatscollector.cc:885: if (info.best_connection &&
> video_media_info) {
> Does this still need "&& video_media_info"?
> 
>
https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/rtcstatscollector_u...
> File webrtc/pc/rtcstatscollector_unittest.cc (right):
> 
>
https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/rtcstatscollector_u...
> webrtc/pc/rtcstatscollector_unittest.cc:1346: //
> video_media_info.bw_estimations[0].available_recv_bandwidth = 0;
> Remember to remove this before landing
> 
>
https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/statscollector_unit...
> File webrtc/pc/statscollector_unittest.cc (right):
> 
>
https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/statscollector_unit...
> webrtc/pc/statscollector_unittest.cc:947: const int kPacerDelay = 123;
> nit: It would be more readable to me if the constants were declared together,
> here and in the other test.

[Taylor Brandstetter, 2017-05-08 17:47:41]

https://codereview.webrtc.org/2863123002/diff/1/webrtc/media/base/mediachannel.h
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/2863123002/diff/1/webrtc/media/base/mediachanne...
webrtc/media/base/mediachannel.h:1084: virtual bool
FillBitrateInfo(BandwidthEstimationInfo* bwe_info) = 0;
On 2017/05/08 07:12:56, stefan-webrtc wrote:
> On 2017/05/07 21:30:44, Taylor Brandstetter wrote:
> > nit: Why not "FillBandwidthEstimationInfo(BandwidthEstimationInfo*)"?
> 
> It only fills the "bitrate parts" (rtx, video bitrate) of the
> BandwidthEstimationInfo, since that's the part that isn't possible to get
> through Call::GetStats, since they are statistics of the send streams. I think
> it would make sense to change this in the future, but I added this method now
> since I don't know how we should change it, and it probably deserves a
separate
> CL.

Can you describe this in a comment above FillBitrateInfo, maybe with a TODO?

> 
> > 
> > Also, what's the advantage of adding a new method over just adding
> > "bw_estimations" to VoiceMediaInfo? Is there a time FillBitrateInfo will be
> > called outside of the stats context?
> 
> I find it strange that the Call::Stats are collected via WebRtcVideoEngine2
(and
> with your suggestion, WebRtcVoiceEngine). They are on a peerconnection/session
> level and should only be collected once. That's why I decided to move it. I
> don't think there's bw_estimations for both audio and video, there's only
> bw_estimations for the currently active transport. Our current design is to
have
> one BWE per session.

I understand when you explain it that way. Can you describe this more in the CL
description?

https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/rtcstatscollector.cc
File webrtc/pc/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/rtcstatscollector.c...
webrtc/pc/rtcstatscollector.cc:699: Call::Stats call_stats =
pc_->session()->GetCallStats();
On 2017/05/08 07:12:56, stefan-webrtc wrote:
> On 2017/05/07 21:30:44, Taylor Brandstetter wrote:
> > This will cause a new invoke to the worker thread, which the new stats
> collector
> > implementation is meant to avoid. Maybe hbos has some thoughts on what to
do.
> 
> I guess we could merge it with WebRtcSession::GetStats()?

WebRtcSession::GetStats invokes on the network thread, which is why it's called
in ProducePartialResultsOnNeworkThread. But GetCallStats invokes on the worker
thread, so ideally it should be called in a
"ProducePartialResultsOnWorkerThread", along with
[Voice|Video]Channel::GetStats.

Since we have a few situations where a single "RTCStats" object needs
information from multiple threads, it would make sense (to me) to gather and
store the "raw" stats on different threads, then only create the RTCStats
objects in a final pass once every thread has finished. hbos@, what do you
think?

[hbos, 2017-05-09 12:48:14]

https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/rtcstatscollector.cc
File webrtc/pc/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2863123002/diff/1/webrtc/pc/rtcstatscollector.c...
webrtc/pc/rtcstatscollector.cc:699: Call::Stats call_stats =
pc_->session()->GetCallStats();
On 2017/05/08 17:47:41, Taylor Brandstetter wrote:
> On 2017/05/08 07:12:56, stefan-webrtc wrote:
> > On 2017/05/07 21:30:44, Taylor Brandstetter wrote:
> > > This will cause a new invoke to the worker thread, which the new stats
> > collector
> > > implementation is meant to avoid. Maybe hbos has some thoughts on what to
> do.
> > 
> > I guess we could merge it with WebRtcSession::GetStats()?
> 
> WebRtcSession::GetStats invokes on the network thread, which is why it's
called
> in ProducePartialResultsOnNeworkThread. But GetCallStats invokes on the worker
> thread, so ideally it should be called in a
> "ProducePartialResultsOnWorkerThread", along with
> [Voice|Video]Channel::GetStats.
> 
> Since we have a few situations where a single "RTCStats" object needs
> information from multiple threads, it would make sense (to me) to gather and
> store the "raw" stats on different threads, then only create the RTCStats
> objects in a final pass once every thread has finished. hbos@, what do you
> think?

Doing synchronous invokes from the network thread could also be a possible risk
of deadlock.

Getting "raw" stats on different threads and creating RTCStats objects in a
final pass sounds good to me. Though it could get complicated or require
refactoring, that might be good to do in a separate CL if we want to generalize
this technique?

You could simply fetch them as a "Prepare" step,
https://cs.chromium.org/chromium/src/third_party/webrtc/pc/rtcstatscollector....,
and pass them to ProducePartialResultsOnNetworkThread, but that doesn't get rid
of the extra thread jump.

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1347: webrtc::Call::Stats stats =
call_->GetStats();
What are these stats, which contain send/recv_bandwidth_bps that we don't use,
compared to WebRtcVideoSendStream::FillBitrateInfo bandwidth estimates?
FillBandwidthEstimationStats used to be the sum of both..

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1382: void
WebRtcVideoChannel2::FillBitrateInfo(BandwidthEstimationInfo* bwe_info) {
nit: Do we still have a need for a bwe pointer or can we return
BandwidthEstimationInfo by value?

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/pc/channel.h#newco...
webrtc/pc/channel.h:353: // Helper function for invoking bool-returning methods
on the worker thread.
nit: Update comment.

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/pc/channel.h#newco...
webrtc/pc/channel.h:556: void FillBitrateInfo(BandwidthEstimationInfo*
bwe_info);
Why is this only in VideoChannel and not also in VoiceChannel (virtual in
BaseChannel)?

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/pc/rtcstatscollect...
File webrtc/pc/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/pc/rtcstatscollect...
webrtc/pc/rtcstatscollector.cc:699: Call::Stats call_stats =
pc_->session()->GetCallStats();
Is this OK? Here we are on the worker thread, but GetCallStats might want call_
to be touched on the worker thread. See other comment.

Also (how) does this end up invoking FillBandwidthInfo? The
webrtc::Call::GetStats and FillBandwidthInfo seem to have separated in this CL.
Does this only use one of them now? Why?

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/pc/rtcstatscollect...
webrtc/pc/rtcstatscollector.cc:896:
static_cast<double>(call_stats.recv_bandwidth_bps);
It would be nice to turn these into rtc::Optional<uint32_t> instead of checking
if an int > 0 since 0 could theoretically be an estimate and negative values are
not allowed. available_outgoing/incoming_bitrate defaults to undefined, not 0,
so the distinction is meaningful. webrtc::Call::GetStats invokes
AvailableBandwidth which returns a boolean if it is able to set or not, so looks
simple to change. Can you change to optinal unless that is a lot of hassle?
Otherwise you can skip that in this CL.

As it happens AvailableBandwidth is implemented so that 0 means undefined so it
wouldn't be noticeable but it would be a step in the right direction if
AvailableBandwidth is ever updated.

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/pc/webrtcsession.cc
File webrtc/pc/webrtcsession.cc (right):

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/pc/webrtcsession.c...
webrtc/pc/webrtcsession.cc:1899: return call_->GetStats();
I suspect this is not safe? call_ is a raw pointer, typically accessed by
call_w() which DCHECKs to be on the worker thread and might modify it. Can you
add a DCHECK or invoke on worker?

[holmer, 2017-05-30 12:07:36]

Description was changed from

==========
Wire up BWE through WebrtcSession so that they are filled in both for audio and
video calls.

BUG=webrtc:5079
==========

to

==========
Wire up BWE through WebrtcSession so that they are filled in both for audio and
video calls.

Prior to this CL Call::Stats were collected via WebRtcVideoEngine2, but not via
WebRtcVoiceEngine, causing these stats to be missing for audio-only calls. Call
lives on the peerconnection/session level and should only be collected once
independent on how many streams we have.

BUG=webrtc:5079
==========

[stefan-webrtc, 2017-05-30 14:43:52]

Comments addresed.

[holmer, 2017-05-30 14:44:27]

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

[holmer, 2017-05-30 14:44:29]

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1347: webrtc::Call::Stats stats =
call_->GetStats();
On 2017/05/09 12:48:14, hbos wrote:
> What are these stats, which contain send/recv_bandwidth_bps that we don't use,
> compared to WebRtcVideoSendStream::FillBitrateInfo bandwidth estimates?
> FillBandwidthEstimationStats used to be the sum of both..

The only reason I have to call Call::GetStats() is because I need the RTT, since
we have to give it to info->senders. After this CL Call::GetStats() should be
called through WebRtcSession instead of going through WebRtcVideoChannel2.

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1382: void
WebRtcVideoChannel2::FillBitrateInfo(BandwidthEstimationInfo* bwe_info) {
On 2017/05/09 12:48:13, hbos wrote:
> nit: Do we still have a need for a bwe pointer or can we return
> BandwidthEstimationInfo by value?

We could, but it'd require that we copy the BandwidthEstimationInfo just to fill
in the bitrate info. I could make that change, but I'm not sure it's better?

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/pc/channel.h#newco...
webrtc/pc/channel.h:353: // Helper function for invoking bool-returning methods
on the worker thread.
On 2017/05/09 12:48:14, hbos wrote:
> nit: Update comment.

Done.

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/pc/channel.h#newco...
webrtc/pc/channel.h:556: void FillBitrateInfo(BandwidthEstimationInfo*
bwe_info);
On 2017/05/09 12:48:14, hbos wrote:
> Why is this only in VideoChannel and not also in VoiceChannel (virtual in
> BaseChannel)?

We should probably add it to VoiceChannel later as well. Currently voice bitrate
isn't filled in. That'll have to be in a separate CL though.

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/pc/rtcstatscollect...
File webrtc/pc/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/pc/rtcstatscollect...
webrtc/pc/rtcstatscollector.cc:699: Call::Stats call_stats =
pc_->session()->GetCallStats();
On 2017/05/09 12:48:14, hbos wrote:
> Is this OK? Here we are on the worker thread, but GetCallStats might want
call_
> to be touched on the worker thread. See other comment.

Made sure the necessary thread hop is done.

> 
> Also (how) does this end up invoking FillBandwidthInfo? The
> webrtc::Call::GetStats and FillBandwidthInfo seem to have separated in this
CL.
> Does this only use one of them now? Why?

It doesn't, and AFAICT it doesn't use the members (transmit_bitrate,
retransmit_bitrate, target_enc_bitrate and actual_enc_bitrate) filled in by
FillBitrateInfo().

I guess that is because those are only used to generate the webrtc-internals
page?

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/pc/rtcstatscollect...
webrtc/pc/rtcstatscollector.cc:896:
static_cast<double>(call_stats.recv_bandwidth_bps);
On 2017/05/09 12:48:14, hbos wrote:
> It would be nice to turn these into rtc::Optional<uint32_t> instead of
checking
> if an int > 0 since 0 could theoretically be an estimate and negative values
are
> not allowed. available_outgoing/incoming_bitrate defaults to undefined, not 0,
> so the distinction is meaningful. webrtc::Call::GetStats invokes
> AvailableBandwidth which returns a boolean if it is able to set or not, so
looks
> simple to change. Can you change to optinal unless that is a lot of hassle?
> Otherwise you can skip that in this CL.
> 
> As it happens AvailableBandwidth is implemented so that 0 means undefined so
it
> wouldn't be noticeable but it would be a step in the right direction if
> AvailableBandwidth is ever updated.

I don't think it will be trivial since it will affect APIs used in other places.
Let's do it separately and try to make this CL as small as possible.

TBH, I don't know why we even do this check though. Why can't we report 0? I
can't see that there'd be much of a difference?

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/pc/webrtcsession.cc
File webrtc/pc/webrtcsession.cc (right):

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/pc/webrtcsession.c...
webrtc/pc/webrtcsession.cc:1899: return call_->GetStats();
On 2017/05/09 12:48:14, hbos wrote:
> I suspect this is not safe? call_ is a raw pointer, typically accessed by
> call_w() which DCHECKs to be on the worker thread and might modify it. Can you
> add a DCHECK or invoke on worker?

As discussed offline, I have no added thread checkers and introduced a
GetCallStats_s() method to be used in statscollector.cc.

[holmer, 2017-05-30 14:44:52]

The CQ bit was checked by holmer@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 14:45:05]

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, 2017-05-30 15:13:48]

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

[commit-bot: I haz the power, 2017-05-30 15:13:49]

Dry run: 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...)

[Taylor Brandstetter, 2017-05-31 00:03:52]

lgtm. CL description should be cleaned up though; I'd clarify that it's the BWE
*stats* that are being wired up through WebRtcSession in this CL, not the
bandwidth estimator as a whole.

https://codereview.webrtc.org/2863123002/diff/40001/webrtc/media/base/mediach...
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/2863123002/diff/40001/webrtc/media/base/mediach...
webrtc/media/base/mediachannel.h:1091: virtual void
FillBitrateInfo(BandwidthEstimationInfo* bwe_info) = 0;
This method requires that the relevant fields in |bwe_info| are initialized to
0; may want to mention that in the comment.

https://codereview.webrtc.org/2863123002/diff/40001/webrtc/pc/webrtcsession.cc
File webrtc/pc/webrtcsession.cc (right):

https://codereview.webrtc.org/2863123002/diff/40001/webrtc/pc/webrtcsession.c...
webrtc/pc/webrtcsession.cc:1906: RTC_FROM_HERE,
rtc::Bind(&WebRtcSession::GetCallStats, this));
Would "rtc::Bind(&Call::GetStats, call_)" work?

https://codereview.webrtc.org/2863123002/diff/40001/webrtc/pc/webrtcsession.h
File webrtc/pc/webrtcsession.h (right):

https://codereview.webrtc.org/2863123002/diff/40001/webrtc/pc/webrtcsession.h...
webrtc/pc/webrtcsession.h:298: Call::Stats GetCallStats_s();
Do both of these need to be public?

[hbos, 2017-05-31 14:31:59]

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1347: webrtc::Call::Stats stats =
call_->GetStats();
On 2017/05/30 14:44:28, holmer wrote:
> On 2017/05/09 12:48:14, hbos wrote:
> > What are these stats, which contain send/recv_bandwidth_bps that we don't
use,
> > compared to WebRtcVideoSendStream::FillBitrateInfo bandwidth estimates?
> > FillBandwidthEstimationStats used to be the sum of both..
> 
> The only reason I have to call Call::GetStats() is because I need the RTT,
since
> we have to give it to info->senders. After this CL Call::GetStats() should be
> called through WebRtcSession instead of going through WebRtcVideoChannel2.

Here call_->GetStats() is invoked from WebRtcVideoChannel2::GetStats which is
invoked from VideoChannel::GetStats doing InvokeOnWorker. That is,
call_->GetStats() is invoked on the worker thread.

RTCStatsCollector does GetCallStats() aka call_->GetStats() on the network
thread.

It seems the VideoChannel ensures call_ is used on the worker thread and
WebRtcSession ensures call_ is used on the network thread. Is both valid? Is
either wrong? Does it not matter?

If it doesn't matter then maybe we don't need GetCallStats_s and can call
GetCallStats directly on the StatsCollector case.

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1382: void
WebRtcVideoChannel2::FillBitrateInfo(BandwidthEstimationInfo* bwe_info) {
On 2017/05/30 14:44:28, holmer wrote:
> On 2017/05/09 12:48:13, hbos wrote:
> > nit: Do we still have a need for a bwe pointer or can we return
> > BandwidthEstimationInfo by value?
> 
> We could, but it'd require that we copy the BandwidthEstimationInfo just to
fill
> in the bitrate info. I could make that change, but I'm not sure it's better?

Acknowledged.

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/pc/channel.h#newco...
webrtc/pc/channel.h:556: void FillBitrateInfo(BandwidthEstimationInfo*
bwe_info);
On 2017/05/30 14:44:28, holmer wrote:
> On 2017/05/09 12:48:14, hbos wrote:
> > Why is this only in VideoChannel and not also in VoiceChannel (virtual in
> > BaseChannel)?
> 
> We should probably add it to VoiceChannel later as well. Currently voice
bitrate
> isn't filled in. That'll have to be in a separate CL though.

Acknowledged.

https://codereview.webrtc.org/2863123002/diff/40001/webrtc/pc/webrtcsession.cc
File webrtc/pc/webrtcsession.cc (right):

https://codereview.webrtc.org/2863123002/diff/40001/webrtc/pc/webrtcsession.c...
webrtc/pc/webrtcsession.cc:1906: RTC_FROM_HERE,
rtc::Bind(&WebRtcSession::GetCallStats, this));
On 2017/05/31 00:03:51, Taylor Brandstetter wrote:
> Would "rtc::Bind(&Call::GetStats, call_)" work?

Yes but if GetCallStats is ever updated you'd have to remember to update two
methods instead of one.

[stefan-webrtc, 2017-05-31 16:04:44]

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

[commit-bot: I haz the power, 2017-05-31 16:04:48]

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, 2017-05-31 16:17:45]

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

[commit-bot: I haz the power, 2017-05-31 16:17:46]

Dry run: Try jobs failed on following builders:
  android_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/24535)

[stefan-webrtc, 2017-05-31 16:38:18]

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

[commit-bot: I haz the power, 2017-05-31 16:39:03]

Dry run: CQ is trying da patch.

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

[stefan-webrtc, 2017-05-31 16:45:47]

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

[commit-bot: I haz the power, 2017-05-31 16:45:52]

Dry run: CQ is trying da patch.

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

[stefan-webrtc, 2017-05-31 16:48:03]

PTAL, did a bunch of changes after discussing further with hbos.

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2863123002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoengine2.cc:1347: webrtc::Call::Stats stats =
call_->GetStats();
On 2017/05/31 14:31:58, hbos wrote:
> On 2017/05/30 14:44:28, holmer wrote:
> > On 2017/05/09 12:48:14, hbos wrote:
> > > What are these stats, which contain send/recv_bandwidth_bps that we don't
> use,
> > > compared to WebRtcVideoSendStream::FillBitrateInfo bandwidth estimates?
> > > FillBandwidthEstimationStats used to be the sum of both..
> > 
> > The only reason I have to call Call::GetStats() is because I need the RTT,
> since
> > we have to give it to info->senders. After this CL Call::GetStats() should
be
> > called through WebRtcSession instead of going through WebRtcVideoChannel2.
> 
> Here call_->GetStats() is invoked from WebRtcVideoChannel2::GetStats which is
> invoked from VideoChannel::GetStats doing InvokeOnWorker. That is,
> call_->GetStats() is invoked on the worker thread.
> 
> RTCStatsCollector does GetCallStats() aka call_->GetStats() on the network
> thread.
> 
> It seems the VideoChannel ensures call_ is used on the worker thread and
> WebRtcSession ensures call_ is used on the network thread. Is both valid? Is
> either wrong? Does it not matter?
> 
> If it doesn't matter then maybe we don't need GetCallStats_s and can call
> GetCallStats directly on the StatsCollector case.

Ideally we should probably call Call::GetStats on the worker thread. Changed in
an attempt to achieve this.

[commit-bot: I haz the power, 2017-05-31 17:12:59]

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

[commit-bot: I haz the power, 2017-05-31 17:13:00]

Dry run: This issue passed the CQ dry run.

[Taylor Brandstetter, 2017-05-31 17:50:28]

lgtm

https://codereview.webrtc.org/2863123002/diff/100001/webrtc/pc/rtcstatscollec...
File webrtc/pc/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2863123002/diff/100001/webrtc/pc/rtcstatscollec...
webrtc/pc/rtcstatscollector.cc:655: Call::Stats call_stats =
pc_->session()->GetCallStats();
This is still resulting in a new, blocking worker thread invoke; should call
that out in a TODO.

[hbos, 2017-06-01 11:40:39]

lgtm % comment

https://codereview.webrtc.org/2863123002/diff/100001/webrtc/pc/rtcstatscollec...
File webrtc/pc/rtcstatscollector.cc (right):

https://codereview.webrtc.org/2863123002/diff/100001/webrtc/pc/rtcstatscollec...
webrtc/pc/rtcstatscollector.cc:655: Call::Stats call_stats =
pc_->session()->GetCallStats();
On 2017/05/31 17:50:28, Taylor Brandstetter wrote:
> This is still resulting in a new, blocking worker thread invoke; should call
> that out in a TODO.

Yes, add a TODO.

Other things that are prepared here and used in one or more threads later have a
member variable. Look at track_to_id_ and consider a call_stats_ member instead
of an additional argument to the function. This avoids a copy of Call::Stats and
is consistent with the other code.

[stefan-webrtc, 2017-06-01 12:48:08]

Moved call_ = nullptr.

[stefan-webrtc, 2017-06-01 12:56:56]

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

[commit-bot: I haz the power, 2017-06-01 12:57:00]

Dry run: CQ is trying da patch.

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

[hbos, 2017-06-01 13:00:18]

lgtm

[stefan-webrtc, 2017-06-01 14:29:13]

Description was changed from

==========
Wire up BWE through WebrtcSession so that they are filled in both for audio and
video calls.

Prior to this CL Call::Stats were collected via WebRtcVideoEngine2, but not via
WebRtcVoiceEngine, causing these stats to be missing for audio-only calls. Call
lives on the peerconnection/session level and should only be collected once
independent on how many streams we have.

BUG=webrtc:5079
==========

to

==========
Wire up BWE stats through WebrtcSession so that they are filled in both for
audio and video calls.

Prior to this CL Call::Stats were collected via WebRtcVideoEngine2, but not via
WebRtcVoiceEngine, causing these stats to be missing for audio-only calls. Call
lives on the peerconnection/session level and should only be collected once
independent on how many streams we have.

BUG=webrtc:5079
==========

[stefan-webrtc, 2017-06-01 14:29:46]

Description was changed from

==========
Wire up BWE stats through WebrtcSession so that they are filled in both for
audio and video calls.

Prior to this CL Call::Stats were collected via WebRtcVideoEngine2, but not via
WebRtcVoiceEngine, causing these stats to be missing for audio-only calls. Call
lives on the peerconnection/session level and should only be collected once
independent on how many streams we have.

BUG=webrtc:5079
==========

to

==========
Wire up BWE stats through WebrtcSession so that they are filled in both for
audio and video calls.

Prior to this CL Call::Stats were collected via WebRtcVideoEngine2, but not via
WebRtcVoiceEngine, causing these stats to be missing for audio-only calls. Call
lives on the peerconnection/session level and should only be collected once
independent on how many streams we have.

BUG=webrtc:5079
R=deadbeef@webrtc.org, hbos@webrtc.org

Review-Url: https://codereview.webrtc.org/2863123002 .
Cr-Commit-Position: refs/heads/master@{#18384}
Committed:
https://chromium.googlesource.com/external/webrtc/+/e80f4c91d0a2854a339e41916...
==========

[stefan-webrtc, 2017-06-01 14:29:47]

Committed patchset #8 (id:140001) manually as
e80f4c91d0a2854a339e419162fdcd1d916f7de0 (presubmit successful).

[charujain, 2017-06-01 15:54:33]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.webrtc.org/2916793003/ by charujain@webrtc.org.

The reason for reverting is: Broken downstream projects.