|
|
Created:
3 years, 9 months ago by åsapersson Modified:
3 years, 8 months ago 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. |
DescriptionChange 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
Messages
Total messages: 21 (14 generated)
Description was changed from ========== Change VideoReceiveStream::Stats total_bitrate_bps to include all received packets. - The total_bitrate_bps is currently 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 ========== to ========== 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 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
asapersson@webrtc.org changed reviewers: + philipel@webrtc.org, stefan@webrtc.org
https://codereview.webrtc.org/2775813002/diff/40001/webrtc/video/receive_stat... File webrtc/video/receive_statistics_proxy_unittest.cc (left): https://codereview.webrtc.org/2775813002/diff/40001/webrtc/video/receive_stat... webrtc/video/receive_statistics_proxy_unittest.cc:122: TEST_F(ReceiveStatisticsProxyTest, GetStatsReportsOnCompleteFrame) { Did not add a test, cannot use fake clock with RateTracker.
Description was changed from ========== 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 ========== to ========== 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 ==========
Description was changed from ========== 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 ========== to ========== 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 ==========
Description was changed from ========== 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 ========== to ========== 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 ==========
lgtm
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: This issue passed the CQ dry run.
lg, just one question https://codereview.webrtc.org/2775813002/diff/40001/webrtc/video/receive_stat... File webrtc/video/receive_statistics_proxy.cc (right): https://codereview.webrtc.org/2775813002/diff/40001/webrtc/video/receive_stat... webrtc/video/receive_statistics_proxy.cc:430: if (total_bytes > last_total_bytes) What should we do if this is not true? Seems like we should reset something?
https://codereview.webrtc.org/2775813002/diff/40001/webrtc/video/receive_stat... File webrtc/video/receive_statistics_proxy.cc (right): https://codereview.webrtc.org/2775813002/diff/40001/webrtc/video/receive_stat... webrtc/video/receive_statistics_proxy.cc:430: if (total_bytes > last_total_bytes) On 2017/03/28 07:13:02, stefan-webrtc wrote: > What should we do if this is not true? Seems like we should reset something? Not sure if it can happen but if the counters are reset I think it is ok to keep the already counted bytes during the last second (interval used by ComputeRate()).
lgtm https://codereview.webrtc.org/2775813002/diff/40001/webrtc/video/receive_stat... File webrtc/video/receive_statistics_proxy.cc (right): https://codereview.webrtc.org/2775813002/diff/40001/webrtc/video/receive_stat... webrtc/video/receive_statistics_proxy.cc:430: if (total_bytes > last_total_bytes) On 2017/03/28 07:59:23, åsapersson wrote: > On 2017/03/28 07:13:02, stefan-webrtc wrote: > > What should we do if this is not true? Seems like we should reset something? > > Not sure if it can happen but if the counters are reset I think it is ok to keep > the already counted bytes during the last second (interval used by > ComputeRate()). Ok.
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": 1490693816602190, "parent_rev": "b1a897680d5bb2b0a6fa8f350083bc22b38162df", "commit_rev": "0255acb0855b0b1c5f14238b809052f85cc8c220"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/0255acb0855b0b1c5f14238b8... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/0255acb0855b0b1c5f14238b8... |