Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2525293002:
Sent bitrate stats are incorrect if FlexFEC is configured. (Closed)
|
Created:
4 years ago by åsapersson Modified:
4 years ago 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. |
DescriptionSent 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 #
Messages
Total messages: 27 (19 generated)
Description was changed from ========== 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. BUG= ========== to ========== 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. Remove info from flexfec substreams in VideoSendStream::Stats::ToString (periodically logged during a call). BUG= ==========
asapersson@webrtc.org changed reviewers: + brandtr@webrtc.org, stefan@webrtc.org
Description was changed from ========== 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. Remove info from flexfec substreams in VideoSendStream::Stats::ToString (periodically logged during a call). BUG= ========== to ========== 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. Remove info from flexfec substreams in VideoSendStream::Stats::ToString (periodically logged during a call). BUG= ==========
Description was changed from ========== 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. Remove info from flexfec substreams in VideoSendStream::Stats::ToString (periodically logged during a call). BUG= ========== to ========== 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. Remove info from flexfec substreams in VideoSendStream::Stats::ToString (periodically logged during a call). BUG= ==========
Description was changed from ========== 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. Remove info from flexfec substreams in VideoSendStream::Stats::ToString (periodically logged during a call). BUG= ========== to ========== 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= ==========
Thanks for finding this problem, and fixing it! And sorry for introducing it in the first place :) For now I think it makes sense to report the FlexFEC send stats along with the media SSRC stats, just like how the old FEC stats are reported. Long-term however, especially if we start supporting multi stream protection, maybe there is a point in reporting the FlexFEC SSRC stats separately from the media stats. I’ll discuss with you Åsa, when that time comes! https://codereview.webrtc.org/2525293002/diff/1/webrtc/video/send_statistics_... File webrtc/video/send_statistics_proxy_unittest.cc (right): https://codereview.webrtc.org/2525293002/diff/1/webrtc/video/send_statistics_... webrtc/video/send_statistics_proxy_unittest.cc:936: static_cast<int>((rtx_counters.fec.TotalBytes() * 2 * 8) / Should this not be |counters|? It seems to me that |rtx_counters.fec| will be empty?
https://codereview.webrtc.org/2525293002/diff/1/webrtc/video/send_statistics_... File webrtc/video/send_statistics_proxy_unittest.cc (right): https://codereview.webrtc.org/2525293002/diff/1/webrtc/video/send_statistics_... webrtc/video/send_statistics_proxy_unittest.cc:936: static_cast<int>((rtx_counters.fec.TotalBytes() * 2 * 8) / On 2016/11/24 14:11:11, brandtr wrote: > Should this not be |counters|? It seems to me that |rtx_counters.fec| will be > empty? Right, the bitrate for counters.fec was zero. Increased the counter values.
The CQ bit was checked by asapersson@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10609)
The CQ bit was checked by asapersson@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10613)
Patchset #2 (id:20001) has been deleted
lgtm
Description was changed from ========== 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= ========== to ========== 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 ==========
lgtm
The CQ bit was checked by asapersson@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480074685973060,
"parent_rev": "a1f371ecfb66e67144823b09e66dbc7689278a0b", "commit_rev":
"b5c4946c368eb8b6cf2f7204e4dd4e9c64633b69"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/a6a699a13078bc8ccc4ed38128c90e6fedc882dc Cr-Commit-Position: refs/heads/master@{#15238} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
