Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1946173002:
Bitrate prober now keep track of probing cluster id. (Closed)
|
Created:
4 years, 7 months ago by philipel Modified:
4 years, 7 months ago Reviewers:
stefan-webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionBitrate prober now keep track of probing cluster id.
BUG=webrtc:5859
R=stefan@webrtc.org
Committed: https://crrev.com/dd3248665da89916aa2d6345815eaadcfa8ab42a
Cr-Commit-Position: refs/heads/master@{#12644}
Patch Set 1 #Patch Set 2 : Removed commented code. #
Total comments: 13
Patch Set 3 : Feedback fixes and format. #Patch Set 4 : Use DCHECKs for CurrentClusterId. #Patch Set 5 : Rebase #
Messages
Total messages: 19 (9 generated)
philipel@webrtc.org changed reviewers: + stefan@webrtc.org
https://codereview.webrtc.org/1946173002/diff/20001/webrtc/modules/pacing/bit... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.webrtc.org/1946173002/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:74: cluster.max_probe_packets = kPacketsPerProbe + (i == 0); I don't like adding a bool. Either (i==0 ? 1 : 0) or maybe even: if (i == 0) ++cluster.max_probe_packets; https://codereview.webrtc.org/1946173002/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:113: clusters_.front().probe_bitrate); git cl format https://codereview.webrtc.org/1946173002/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:134: int BitrateProber::CurrentClusterId() const { It's not clear to me how this method is going to be used since it returns -1 when all probes have been sent. Is it only for testing? In that case I prefer writing the tests differently. https://codereview.webrtc.org/1946173002/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:153: ProbeCluster& cluster = clusters_.front(); Only const references allowed https://codereview.webrtc.org/1946173002/diff/20001/webrtc/modules/pacing/bit... File webrtc/modules/pacing/bitrate_prober.h (right): https://codereview.webrtc.org/1946173002/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.h:60: uint8_t sent_probe_packets = 0; int for these too? https://codereview.webrtc.org/1946173002/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.h:61: int probe_bitrate = 0; probe_bitrate_bps
https://codereview.webrtc.org/1946173002/diff/20001/webrtc/modules/pacing/bit... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.webrtc.org/1946173002/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:74: cluster.max_probe_packets = kPacketsPerProbe + (i == 0); On 2016/05/04 09:42:35, stefan-webrtc (holmer) wrote: > I don't like adding a bool. Either (i==0 ? 1 : 0) or maybe even: > if (i == 0) > ++cluster.max_probe_packets; Done. https://codereview.webrtc.org/1946173002/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:113: clusters_.front().probe_bitrate); On 2016/05/04 09:42:35, stefan-webrtc (holmer) wrote: > git cl format Done. https://codereview.webrtc.org/1946173002/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:134: int BitrateProber::CurrentClusterId() const { On 2016/05/04 09:42:35, stefan-webrtc (holmer) wrote: > It's not clear to me how this method is going to be used since it returns -1 > when all probes have been sent. Is it only for testing? In that case I prefer > writing the tests differently. This method is suppose to be used to get the id of the cluster that the packet that is about to be sent belongs to. Returning -1 means this packet does not belong to a cluster, but maybe it should be DCHECKED instead since you should never try to get the cluster id of a packet that has not been used for probing. https://codereview.webrtc.org/1946173002/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:153: ProbeCluster& cluster = clusters_.front(); On 2016/05/04 09:42:35, stefan-webrtc (holmer) wrote: > Only const references allowed Changed to |ProbeCluster*|. https://codereview.webrtc.org/1946173002/diff/20001/webrtc/modules/pacing/bit... File webrtc/modules/pacing/bitrate_prober.h (right): https://codereview.webrtc.org/1946173002/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.h:60: uint8_t sent_probe_packets = 0; On 2016/05/04 09:42:35, stefan-webrtc (holmer) wrote: > int for these too? Done. https://codereview.webrtc.org/1946173002/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.h:61: int probe_bitrate = 0; On 2016/05/04 09:42:35, stefan-webrtc (holmer) wrote: > probe_bitrate_bps Done.
lgtm https://codereview.webrtc.org/1946173002/diff/20001/webrtc/modules/pacing/bit... File webrtc/modules/pacing/bitrate_prober.cc (right): https://codereview.webrtc.org/1946173002/diff/20001/webrtc/modules/pacing/bit... webrtc/modules/pacing/bitrate_prober.cc:134: int BitrateProber::CurrentClusterId() const { On 2016/05/04 10:36:32, philipel wrote: > On 2016/05/04 09:42:35, stefan-webrtc (holmer) wrote: > > It's not clear to me how this method is going to be used since it returns -1 > > when all probes have been sent. Is it only for testing? In that case I prefer > > writing the tests differently. > > This method is suppose to be used to get the id of the cluster that the packet > that is about to be sent belongs to. Returning -1 means this packet does not > belong to a cluster, but maybe it should be DCHECKED instead since you should > never try to get the cluster id of a packet that has not been used for probing. Yes, add a DCHECK instead.
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1946173002/#ps60001 (title: "Use DCHECKs for CurrentClusterId.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1946173002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1946173002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_mips_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_mips_db...) android_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_dbg/builds/1...) android_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_rel/builds/1...) ios32_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/7259) ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/7245) ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/9583) ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/9507) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/14788) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/13441) linux_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/14493) linux_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...) linux_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_rel/builds/10670) linux_libfuzzer_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...) mac_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/14410) mac_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/11138) mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/2932) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/9153) mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/14891) win_x64_clang_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_rel/build...) win_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/14486)
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1946173002/#ps80001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1946173002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1946173002/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)
Message was sent while issue was closed.
Description was changed from ========== Bitrate prober now keep track of probing cluster id. BUG=webrtc:5859 ========== to ========== Bitrate prober now keep track of probing cluster id. BUG=webrtc:5859 R=stefan@webrtc.org Committed: https://crrev.com/dd3248665da89916aa2d6345815eaadcfa8ab42a Cr-Commit-Position: refs/heads/master@{#12644} ==========
Message was sent while issue was closed.
Description was changed from ========== Bitrate prober now keep track of probing cluster id. BUG=webrtc:5859 R=stefan@webrtc.org Committed: https://crrev.com/dd3248665da89916aa2d6345815eaadcfa8ab42a Cr-Commit-Position: refs/heads/master@{#12644} ========== to ========== Bitrate prober now keep track of probing cluster id. BUG=webrtc:5859 R=stefan@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/dd3248665da89916aa2d63458... ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/dd3248665da89916aa2d6345815eaadcfa8ab42a Cr-Commit-Position: refs/heads/master@{#12644}
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as dd3248665da89916aa2d6345815eaadcfa8ab42a (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
