Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(338)

Issue 2863123002: Wire up BWE stats through WebrtcSession so that they are filled in both for audio and video calls. (Closed)

Created:
3 years, 7 months ago by stefan-webrtc
Modified:
3 years, 6 months ago
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 16

Patch Set 2 : Comments addressed, generalized InvokeOnWorker(). #

Total comments: 18

Patch Set 3 : Comments addresed. #

Total comments: 4

Patch Set 4 : Crash fix. #

Patch Set 5 : Changed according to offline discussion with hbos. #

Patch Set 6 : Guard for call_->OnSentPacket. #

Total comments: 2

Patch Set 7 : Moved call_ = nullptr. #

Patch Set 8 : Comments addressed." #

Unified diffs Side-by-side diffs Delta from patch set Stats (+165 lines, -107 lines) Patch
M webrtc/media/base/fakemediaengine.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/media/base/mediachannel.h View 1 2 2 chunks +9 lines, -2 lines 0 comments Download
M webrtc/media/engine/webrtcvideoengine2.h View 1 2 2 chunks +2 lines, -1 line 0 comments Download
M webrtc/media/engine/webrtcvideoengine2.cc View 1 2 3 chunks +5 lines, -13 lines 0 comments Download
M webrtc/media/engine/webrtcvideoengine2_unittest.cc View 1 2 1 chunk +6 lines, -4 lines 0 comments Download
M webrtc/pc/channel.h View 1 2 2 chunks +5 lines, -5 lines 0 comments Download
M webrtc/pc/channel.cc View 1 12 chunks +43 lines, -33 lines 0 comments Download
M webrtc/pc/rtcstatscollector.h View 1 2 3 4 5 6 7 3 chunks +5 lines, -1 line 0 comments Download
M webrtc/pc/rtcstatscollector.cc View 1 2 3 4 5 6 7 4 chunks +24 lines, -21 lines 0 comments Download
M webrtc/pc/rtcstatscollector_unittest.cc View 1 5 6 7 3 chunks +9 lines, -9 lines 0 comments Download
M webrtc/pc/statscollector.h View 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/pc/statscollector.cc View 1 2 3 4 4 chunks +23 lines, -9 lines 0 comments Download
M webrtc/pc/statscollector_unittest.cc View 1 2 chunks +17 lines, -8 lines 0 comments Download
M webrtc/pc/test/mock_webrtcsession.h View 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/pc/webrtcsession.h View 3 4 2 chunks +3 lines, -1 line 0 comments Download
M webrtc/pc/webrtcsession.cc View 1 2 3 4 5 6 3 chunks +11 lines, -0 lines 0 comments Download

Messages

Total messages: 39 (21 generated)
stefan-webrtc
3 years, 7 months ago (2017-05-05 15:38:11 UTC) #2
stefan-webrtc
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.cc#newcode890 webrtc/pc/rtcstatscollector.cc:890: if (call_stats.send_bandwidth_bps > 0) { Not sure this check ...
3 years, 7 months ago (2017-05-05 15:41:54 UTC) #3
Taylor Brandstetter
I don't really understand why FillBitrateInfo is needed, as opposed to adding fields to existing ...
3 years, 7 months ago (2017-05-07 21:30:44 UTC) #4
stefan-webrtc
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/mediachannel.h#newcode1084 webrtc/media/base/mediachannel.h:1084: virtual bool FillBitrateInfo(BandwidthEstimationInfo* bwe_info) = 0; On 2017/05/07 21:30:44, ...
3 years, 7 months ago (2017-05-08 07:12:57 UTC) #5
stefan-webrtc
On 2017/05/07 21:30:44, Taylor Brandstetter wrote: > I don't really understand why FillBitrateInfo is needed, ...
3 years, 7 months ago (2017-05-08 07:21:34 UTC) #6
Taylor Brandstetter
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/mediachannel.h#newcode1084 webrtc/media/base/mediachannel.h:1084: virtual bool FillBitrateInfo(BandwidthEstimationInfo* bwe_info) = 0; On 2017/05/08 07:12:56, ...
3 years, 7 months ago (2017-05-08 17:47:41 UTC) #7
hbos
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.cc#newcode699 webrtc/pc/rtcstatscollector.cc:699: Call::Stats call_stats = pc_->session()->GetCallStats(); On 2017/05/08 17:47:41, Taylor Brandstetter ...
3 years, 7 months ago (2017-05-09 12:48:14 UTC) #8
stefan-webrtc
Comments addresed.
3 years, 6 months ago (2017-05-30 14:43:52 UTC) #10
holmer
https://codereview.webrtc.org/2863123002/diff/20001/webrtc/media/engine/webrtcvideoengine2.cc File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2863123002/diff/20001/webrtc/media/engine/webrtcvideoengine2.cc#newcode1347 webrtc/media/engine/webrtcvideoengine2.cc:1347: webrtc::Call::Stats stats = call_->GetStats(); On 2017/05/09 12:48:14, hbos wrote: ...
3 years, 6 months ago (2017-05-30 14:44:29 UTC) #12
Taylor Brandstetter
lgtm. CL description should be cleaned up though; I'd clarify that it's the BWE *stats* ...
3 years, 6 months ago (2017-05-31 00:03:52 UTC) #17
hbos
https://codereview.webrtc.org/2863123002/diff/20001/webrtc/media/engine/webrtcvideoengine2.cc File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2863123002/diff/20001/webrtc/media/engine/webrtcvideoengine2.cc#newcode1347 webrtc/media/engine/webrtcvideoengine2.cc:1347: webrtc::Call::Stats stats = call_->GetStats(); On 2017/05/30 14:44:28, holmer wrote: ...
3 years, 6 months ago (2017-05-31 14:31:59 UTC) #18
stefan-webrtc
PTAL, did a bunch of changes after discussing further with hbos. https://codereview.webrtc.org/2863123002/diff/20001/webrtc/media/engine/webrtcvideoengine2.cc File webrtc/media/engine/webrtcvideoengine2.cc (right): ...
3 years, 6 months ago (2017-05-31 16:48:03 UTC) #27
Taylor Brandstetter
lgtm https://codereview.webrtc.org/2863123002/diff/100001/webrtc/pc/rtcstatscollector.cc File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2863123002/diff/100001/webrtc/pc/rtcstatscollector.cc#newcode655 webrtc/pc/rtcstatscollector.cc:655: Call::Stats call_stats = pc_->session()->GetCallStats(); This is still resulting ...
3 years, 6 months ago (2017-05-31 17:50:28 UTC) #30
hbos
lgtm % comment https://codereview.webrtc.org/2863123002/diff/100001/webrtc/pc/rtcstatscollector.cc File webrtc/pc/rtcstatscollector.cc (right): https://codereview.webrtc.org/2863123002/diff/100001/webrtc/pc/rtcstatscollector.cc#newcode655 webrtc/pc/rtcstatscollector.cc:655: Call::Stats call_stats = pc_->session()->GetCallStats(); On 2017/05/31 ...
3 years, 6 months ago (2017-06-01 11:40:39 UTC) #31
stefan-webrtc
Moved call_ = nullptr.
3 years, 6 months ago (2017-06-01 12:48:08 UTC) #32
hbos
lgtm
3 years, 6 months ago (2017-06-01 13:00:18 UTC) #35
stefan-webrtc
Committed patchset #8 (id:140001) manually as e80f4c91d0a2854a339e419162fdcd1d916f7de0 (presubmit successful).
3 years, 6 months ago (2017-06-01 14:29:47 UTC) #38
charujain
3 years, 6 months ago (2017-06-01 15:54:33 UTC) #39
Message was sent while issue was closed.
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.

Powered by Google App Engine
This is Rietveld 408576698