Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1470373004:
Rewrote pacer and bandwidth UMA stats. (Closed)
|
Created:
5 years ago by stefan-webrtc Modified:
5 years ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRewrote pacer and bandwidth UMA stats.
The new version measures receive bitrates from time of first packet to
time of last packet, and send/pacer BWE as the average BWE reported
while we have send streams.
R=asapersson@webrtc.org, pbos@webrtc.org
Committed: https://crrev.com/226befecfb5e56287482a2d6250f01d019761884
Cr-Commit-Position: refs/heads/master@{#10810}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #
Total comments: 6
Patch Set 4 : Comments addressed. #
Total comments: 4
Patch Set 5 : Comments addressed - again. #Messages
Total messages: 22 (7 generated)
stefan@webrtc.org changed reviewers: + asapersson@webrtc.org, pbos@webrtc.org
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1470373004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1470373004/1
I'd happily receive comments on this CL. :)
https://codereview.webrtc.org/1470373004/diff/40001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1470373004/diff/40001/webrtc/call/call.cc#newco... webrtc/call/call.cc:157: int64_t received_rtcp_bytes_; size_t? https://codereview.webrtc.org/1470373004/diff/40001/webrtc/call/call.cc#newco... webrtc/call/call.cc:166: int64_t pacer_bitrate_sum_kbits_ GUARDED_BY(&bitrate_crit_); size_t https://codereview.webrtc.org/1470373004/diff/40001/webrtc/call/call.cc#newco... webrtc/call/call.cc:167: int num_bitrate_updates_ GUARDED_BY(&bitrate_crit_); size_t?
Comments addressed.
https://codereview.webrtc.org/1470373004/diff/40001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1470373004/diff/40001/webrtc/call/call.cc#newco... webrtc/call/call.cc:157: int64_t received_rtcp_bytes_; On 2015/11/26 09:14:32, åsapersson wrote: > size_t? Done. https://codereview.webrtc.org/1470373004/diff/40001/webrtc/call/call.cc#newco... webrtc/call/call.cc:166: int64_t pacer_bitrate_sum_kbits_ GUARDED_BY(&bitrate_crit_); On 2015/11/26 09:14:32, åsapersson wrote: > size_t Done. https://codereview.webrtc.org/1470373004/diff/40001/webrtc/call/call.cc#newco... webrtc/call/call.cc:167: int num_bitrate_updates_ GUARDED_BY(&bitrate_crit_); On 2015/11/26 09:14:32, åsapersson wrote: > size_t? Done.
lgtm
https://codereview.webrtc.org/1470373004/diff/60001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1470373004/diff/60001/webrtc/call/call.cc#newco... webrtc/call/call.cc:155: size_t received_video_bytes_; total bytes can quite easily overflow a size_t (usually 32bit I think), if my crappy calculations are right >2mbit will cause this to happen within a day, int64_t please https://codereview.webrtc.org/1470373004/diff/60001/webrtc/call/call.cc#newco... webrtc/call/call.cc:165: size_t estimated_send_bitrate_sum_kbits_ GUARDED_BY(&bitrate_crit_); Don't we wanna use ints for this (not int64)?
https://codereview.webrtc.org/1470373004/diff/60001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1470373004/diff/60001/webrtc/call/call.cc#newco... webrtc/call/call.cc:155: size_t received_video_bytes_; On 2015/11/26 10:19:38, pbos-webrtc wrote: > total bytes can quite easily overflow a size_t (usually 32bit I think), if my > crappy calculations are right >2mbit will cause this to happen within a day, > int64_t please Right, that was why I had int64s... I just forgot why. :) https://codereview.webrtc.org/1470373004/diff/60001/webrtc/call/call.cc#newco... webrtc/call/call.cc:165: size_t estimated_send_bitrate_sum_kbits_ GUARDED_BY(&bitrate_crit_); On 2015/11/26 10:19:38, pbos-webrtc wrote: > Don't we wanna use ints for this (not int64)? Might as well use int64 here too. That way we can easily have a 1 year long call, which wouldn't be possible with 32 bits if my calculations are right. Doing the same for the num_bitrate_updates_, makes divison more clear (no casting) and avoids being on the border if the bitrate is updated around 10 times per second and we want to be able to run for a year... :) Better be on the safe side!
Comments addressed - again.
lgtm
The CQ bit was checked by stefan@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from asapersson@webrtc.org Link to the patchset: https://codereview.webrtc.org/1470373004/#ps80001 (title: "Comments addressed - again.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1470373004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1470373004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Description was changed from ========== Rewrote pacer and bandwidth UMA stats. The new version measures receive bitrates from time of first packet to time of last packet, and send/pacer BWE as the average BWE reported while we have send streams. ========== to ========== Rewrote pacer and bandwidth UMA stats. The new version measures receive bitrates from time of first packet to time of last packet, and send/pacer BWE as the average BWE reported while we have send streams. R=asapersson@webrtc.org, pbos@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/226befecfb5e56287482a2d62... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 226befecfb5e56287482a2d6250f01d019761884 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Rewrote pacer and bandwidth UMA stats. The new version measures receive bitrates from time of first packet to time of last packet, and send/pacer BWE as the average BWE reported while we have send streams. R=asapersson@webrtc.org, pbos@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/226befecfb5e56287482a2d62... ========== to ========== Rewrote pacer and bandwidth UMA stats. The new version measures receive bitrates from time of first packet to time of last packet, and send/pacer BWE as the average BWE reported while we have send streams. R=asapersson@webrtc.org, pbos@webrtc.org Committed: https://crrev.com/226befecfb5e56287482a2d6250f01d019761884 Cr-Commit-Position: refs/heads/master@{#10810} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/226befecfb5e56287482a2d6250f01d019761884 Cr-Commit-Position: refs/heads/master@{#10810} |
