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

Issue 2775813002: Change VideoReceiveStream::Stats total_bitrate_bps to include all received packets. (Closed)

Created:
3 years, 9 months ago by åsapersson
Modified:
3 years, 8 months ago
Reviewers:
philipel, stefan-webrtc
CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, mflodman
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

Change VideoReceiveStream::Stats total_bitrate_bps to include all received packets. The total_bitrate_bps is based on complete frames (from jitter buffer callback: ReceiveStatisticsProxy::OnCompleteFrame(bool is_keyframe, size_t size_bytes)) (since M58). Receive total_bitrate_bps can be incorrectly reported. Example log of stats_.total_bitrate_bps in video_loopback call (target bitrate 600kbps): SEND: stats_.total_bitrate_bps 682832 RECV: stats_.total_bitrate_bps 1078104 (new: 677384) SEND: stats_.total_bitrate_bps 694280 RECV: stats_.total_bitrate_bps 1091768 (new: 663152) SEND: stats_.total_bitrate_bps 626248 RECV: stats_.total_bitrate_bps 7683776 (new: 636080) Changed total_bitrate_bps to be based on incoming packets (reported via callback from ReceiveStatisticsImpl: ReceiveStatisticsProxy::DataCountersUpdated(const webrtc::StreamDataCounters& counters, uint32_t ssrc)). BUG=webrtc:7400 Review-Url: https://codereview.webrtc.org/2775813002 Cr-Commit-Position: refs/heads/master@{#17411} Committed: https://chromium.googlesource.com/external/webrtc/+/0255acb0855b0b1c5f14238b809052f85cc8c220

Patch Set 1 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+18 lines, -15 lines) Patch
M webrtc/video/receive_statistics_proxy.h View 3 chunks +3 lines, -4 lines 0 comments Download
M webrtc/video/receive_statistics_proxy.cc View 4 chunks +15 lines, -10 lines 3 comments Download
M webrtc/video/receive_statistics_proxy_unittest.cc View 1 chunk +0 lines, -1 line 1 comment Download

Messages

Total messages: 21 (14 generated)
åsapersson
https://codereview.webrtc.org/2775813002/diff/40001/webrtc/video/receive_statistics_proxy_unittest.cc File webrtc/video/receive_statistics_proxy_unittest.cc (left): https://codereview.webrtc.org/2775813002/diff/40001/webrtc/video/receive_statistics_proxy_unittest.cc#oldcode122 webrtc/video/receive_statistics_proxy_unittest.cc:122: TEST_F(ReceiveStatisticsProxyTest, GetStatsReportsOnCompleteFrame) { Did not add a test, cannot ...
3 years, 9 months ago (2017-03-24 16:01:34 UTC) #5
philipel
lgtm
3 years, 8 months ago (2017-03-27 12:20:42 UTC) #9
stefan-webrtc
lg, just one question https://codereview.webrtc.org/2775813002/diff/40001/webrtc/video/receive_statistics_proxy.cc File webrtc/video/receive_statistics_proxy.cc (right): https://codereview.webrtc.org/2775813002/diff/40001/webrtc/video/receive_statistics_proxy.cc#newcode430 webrtc/video/receive_statistics_proxy.cc:430: if (total_bytes > last_total_bytes) What ...
3 years, 8 months ago (2017-03-28 07:13:02 UTC) #14
åsapersson
https://codereview.webrtc.org/2775813002/diff/40001/webrtc/video/receive_statistics_proxy.cc File webrtc/video/receive_statistics_proxy.cc (right): https://codereview.webrtc.org/2775813002/diff/40001/webrtc/video/receive_statistics_proxy.cc#newcode430 webrtc/video/receive_statistics_proxy.cc:430: if (total_bytes > last_total_bytes) On 2017/03/28 07:13:02, stefan-webrtc wrote: ...
3 years, 8 months ago (2017-03-28 07:59:23 UTC) #15
stefan-webrtc
lgtm https://codereview.webrtc.org/2775813002/diff/40001/webrtc/video/receive_statistics_proxy.cc File webrtc/video/receive_statistics_proxy.cc (right): https://codereview.webrtc.org/2775813002/diff/40001/webrtc/video/receive_statistics_proxy.cc#newcode430 webrtc/video/receive_statistics_proxy.cc:430: if (total_bytes > last_total_bytes) On 2017/03/28 07:59:23, åsapersson ...
3 years, 8 months ago (2017-03-28 09:18:53 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2775813002/40001
3 years, 8 months ago (2017-03-28 09:37:06 UTC) #18
commit-bot: I haz the power
3 years, 8 months ago (2017-03-28 09:45:01 UTC) #21
Message was sent while issue was closed.
Committed patchset #1 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/0255acb0855b0b1c5f14238b8...

Powered by Google App Engine
This is Rietveld 408576698