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

Issue 2525293002: Sent bitrate stats are incorrect if FlexFEC is configured. (Closed)

Created:
4 years ago by åsapersson
Modified:
4 years ago
Reviewers:
brandtr, stefan-webrtc
CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, mflodman
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

Sent bitrate stats are incorrect if FlexFEC is configured: WebRTC.Video.BitrateSentInKbps WebRTC.Video.MediaBitrateSentInKbps WebRTC.Video.PaddingBitrateSentInKbps WebRTC.Video.RetransmittedBitrateSentInKbps WebRTC.Video.FecBitrateSentInKbps RtpSender has two StreamDataCounters: for the non-RTX and the RTX stream. The same counter (for the non-RTX stream) is reported for both the media SSRC and the FlexFEC SSRC. Bitrate stats are summed for all SSRCs, thus the counter for the non-RTX stream is counted twice. Do not store the counter for the FlexFEC SSRC. Do not include info from FlexFEC substreams in VideoSendStream::Stats::ToString (periodically logged during a call). BUG=webrtc:6774 Committed: https://crrev.com/a6a699a13078bc8ccc4ed38128c90e6fedc882dc Cr-Commit-Position: refs/heads/master@{#15238}

Patch Set 1 #

Total comments: 2

Patch Set 2 : address comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+130 lines, -9 lines) Patch
M webrtc/video/send_statistics_proxy.cc View 2 chunks +7 lines, -0 lines 0 comments Download
M webrtc/video/send_statistics_proxy_unittest.cc View 1 6 chunks +121 lines, -8 lines 0 comments Download
M webrtc/video/video_send_stream.cc View 1 chunk +1 line, -1 line 0 comments Download
M webrtc/video_send_stream.h View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 27 (19 generated)
åsapersson
4 years ago (2016-11-24 12:58:56 UTC) #6
brandtr
Thanks for finding this problem, and fixing it! And sorry for introducing it in the ...
4 years ago (2016-11-24 14:11:11 UTC) #7
åsapersson
https://codereview.webrtc.org/2525293002/diff/1/webrtc/video/send_statistics_proxy_unittest.cc File webrtc/video/send_statistics_proxy_unittest.cc (right): https://codereview.webrtc.org/2525293002/diff/1/webrtc/video/send_statistics_proxy_unittest.cc#newcode936 webrtc/video/send_statistics_proxy_unittest.cc:936: static_cast<int>((rtx_counters.fec.TotalBytes() * 2 * 8) / On 2016/11/24 14:11:11, ...
4 years ago (2016-11-24 15:46:57 UTC) #8
brandtr
lgtm
4 years ago (2016-11-24 16:27:05 UTC) #18
stefan-webrtc
lgtm
4 years ago (2016-11-25 11:50:13 UTC) #20
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/2525293002/40001
4 years ago (2016-11-25 11:51:32 UTC) #22
commit-bot: I haz the power
Committed patchset #2 (id:40001)
4 years ago (2016-11-25 11:52:50 UTC) #25
commit-bot: I haz the power
4 years ago (2016-11-25 11:52:59 UTC) #27
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/a6a699a13078bc8ccc4ed38128c90e6fedc882dc
Cr-Commit-Position: refs/heads/master@{#15238}

Powered by Google App Engine
This is Rietveld 408576698