Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1434403004:
Add UMA for send bwe and pacer bitrate. (Closed)
|
Created:
5 years, 1 month ago by stefan-webrtc Modified:
5 years, 1 month ago CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), tterriberry_mozilla.com, andresp, the sun, perkj_webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd UMA for send bwe and pacer bitrate.
Committed: https://crrev.com/18adf0a79d4a0ea124c7f27fd553382d0b0cb7ac
Cr-Commit-Position: refs/heads/master@{#10675}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Comment addressed. #Patch Set 3 : . #Patch Set 4 : Add locks to handle OnNetworkChanged from multiple threads. #
Messages
Total messages: 30 (10 generated)
stefan@webrtc.org changed reviewers: + asapersson@webrtc.org, pbos@webrtc.org
lgtm https://codereview.webrtc.org/1434403004/diff/1/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1434403004/diff/1/webrtc/call/call.cc#newcode152 webrtc/call/call.cc:152: // The RateTrackers are only accessed (exclusively) from one method and from s/method/thread should be true
https://codereview.webrtc.org/1434403004/diff/1/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1434403004/diff/1/webrtc/call/call.cc#newcode244 webrtc/call/call.cc:244: int pacer_bitrate_kbps = pacer_bitrate_bps_.ComputeTotalRate() / 1000; Should these be average of added samples.
https://codereview.webrtc.org/1434403004/diff/1/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1434403004/diff/1/webrtc/call/call.cc#newcode152 webrtc/call/call.cc:152: // The RateTrackers are only accessed (exclusively) from one method and from On 2015/11/13 11:27:23, pbos-webrtc wrote: > s/method/thread should be true Done. https://codereview.webrtc.org/1434403004/diff/1/webrtc/call/call.cc#newcode244 webrtc/call/call.cc:244: int pacer_bitrate_kbps = pacer_bitrate_bps_.ComputeTotalRate() / 1000; On 2015/11/13 12:38:53, åsapersson wrote: > Should these be average of added samples. Hah, yes, nice catch... I didn't fully think this through :)
https://codereview.webrtc.org/1434403004/diff/1/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/1434403004/diff/1/webrtc/call/call.cc#newcode244 webrtc/call/call.cc:244: int pacer_bitrate_kbps = pacer_bitrate_bps_.ComputeTotalRate() / 1000; On 2015/11/13 13:33:18, stefan-webrtc (holmer) wrote: > On 2015/11/13 12:38:53, åsapersson wrote: > > Should these be average of added samples. > > Hah, yes, nice catch... I didn't fully think this through :) What do you think about this new approach?
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/1434403004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1434403004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by stefan@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/1434403004/#ps40001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1434403004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1434403004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_tsan2 on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/8005)
Not sure that the tsan error is due to your change. Are you on top of it though? :)
Add locks to handle OnNetworkChanged from multiple threads.
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/1434403004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1434403004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
It's a race in OnNetworkChanged, since that method can be called from two different threads. Adding critsects around it for now, but we should fix OnNetworkChanged at some point.
ptal
lgtm, woo
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/1434403004/#ps60001 (title: "Add locks to handle OnNetworkChanged from multiple threads.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1434403004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1434403004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/18adf0a79d4a0ea124c7f27fd553382d0b0cb7ac Cr-Commit-Position: refs/heads/master@{#10675} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
