Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2254733005:
Only use payload size within the receive/send interval for bitrate probing. (Closed)
DescriptionOnly use payload size within the know send/receive interval for probing calculations.
BUG=webrtc:5859
Committed: https://crrev.com/0561bdf8338aa58f164b7d82d75368d9777c2684
Cr-Commit-Position: refs/heads/master@{#13889}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Keep track of unique send times. #
Total comments: 3
Patch Set 3 : Keep track of the size of the last/first sent/receive packet. #
Total comments: 10
Patch Set 4 : Fix comments. #Patch Set 5 : Rebase #Patch Set 6 : Rebase fix #
Messages
Total messages: 36 (17 generated)
philipel@webrtc.org changed reviewers: + danilchap@webrtc.org, stefan@webrtc.org
How real is the problem you are solving here? (bug is too generic and doesn't give a hint why this is needed for mid-call probing). How often this happen, what is negative implication when it does? I wonder if this additional complexity worse the problem you are solving. If you just want to measure how often this happen, might be enough to log when num_probes exceed size. https://codereview.webrtc.org/2254733005/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/probe_bitrate_estimator.cc (right): https://codereview.webrtc.org/2254733005/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/probe_bitrate_estimator.cc:51: if (cluster->sequence_numbers.count(packet_info.sequence_number) == 1) { may be better compare with 0: .count(seq_number) > 0 https://codereview.webrtc.org/2254733005/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc (right): https://codereview.webrtc.org/2254733005/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc:45: uint16_t Rand() { return rand_.Rand(std::numeric_limits<uint16_t>::max()); } return rand_.Rand<uint16_t>(); if you want to use rand https://codereview.webrtc.org/2254733005/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc:50: Random rand_; May be cleaner use some constant kSeqNo = 7716; for initial seq_num: seq_num valid range is both trivial and irrelevant for this tests. I can't imagine code under this test that looks ok but able to misbehave with some sequence numbers and not with others.
isheriff@chromium.org changed reviewers: + isheriff@chromium.org
What triggers multiple sequence numbers to be inserted ?
danilchap@: Pretty much reimplemented the fix, so I didn't respond to your comments. irfan@: We did some more digging and discovered that requesting to send padding can result in multiple packets being sent. When this happens the all have the same send time, so now we use that information instead. ptal
Description was changed from ========== Keep track of insert probe packets. In order to avoid having the same probing packet being inserted twice we keep track of the sequence number of the inserted packets for every cluster. BUG=webrtc:5859 ========== to ========== Keep track of unique send times. Since a request to send padding can result in multiple packets being sent (all with the same send time), we keep track of how many unique send times we have seen for a certain cluster in order to not count every padding packet as a separate packet in the probing cluster. BUG=webrtc:5859 ==========
Description was changed from ========== Keep track of unique send times. Since a request to send padding can result in multiple packets being sent (all with the same send time), we keep track of how many unique send times we have seen for a certain cluster in order to not count every padding packet as a separate packet in the probing cluster. BUG=webrtc:5859 ========== to ========== Keep track of unique send times. Since a request to send padding can result in multiple packets being sent (all with the same send time), we keep track of how many unique send times we have seen for a certain cluster in order to not count every padding packet as a separate packet in the probing cluster. BUG=webrtc:5859 ==========
https://codereview.webrtc.org/2254733005/diff/20001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probe_bitrate_estimator.cc (right): https://codereview.webrtc.org/2254733005/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_bitrate_estimator.cc:61: cluster->send_times_ms.insert(packet_info.send_time_ms); why is it needed? https://codereview.webrtc.org/2254733005/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_bitrate_estimator.cc:67: if (cluster->send_times_ms.size() < kMinNumProbesValidCluster) If you worry two small padding packet is not enough to do the probing calculation, may be better expand this if instead: if (cluster->num_probes < kMinProbes || cluster->size < kMinSize) https://codereview.webrtc.org/2254733005/diff/20001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probe_bitrate_estimator.h (right): https://codereview.webrtc.org/2254733005/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_bitrate_estimator.h:44: // Since sending padding can result in multiple packets being sent (all same send time is common, but not guaranteed (it is calculated per packet and just happen to be close enough to look same).
Description was changed from ========== Keep track of unique send times. Since a request to send padding can result in multiple packets being sent (all with the same send time), we keep track of how many unique send times we have seen for a certain cluster in order to not count every padding packet as a separate packet in the probing cluster. BUG=webrtc:5859 ========== to ========== Only use payload size within the know send/receive interval for probing calculations. BUG=webrtc:5859 ==========
lgtm https://codereview.webrtc.org/2254733005/diff/40001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probe_bitrate_estimator.cc (right): https://codereview.webrtc.org/2254733005/diff/40001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_bitrate_estimator.cc:50: int payload_size = packet_info.payload_size * 8; may be call it payload_size_bits https://codereview.webrtc.org/2254733005/diff/40001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_bitrate_estimator.cc:91: float send_size = cluster->size_total - cluster->size_last_send; probably RTC_DCHECK_GT(cluster->size_total - cluster->size_last_send, 0); [e.g. to catch when payload size would be reported as 0 for padding packets] https://codereview.webrtc.org/2254733005/diff/40001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc (right): https://codereview.webrtc.org/2254733005/diff/40001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc:133: AddPacketFeedback(0, 1500, 40, 50); May be add note that increased packet size increase received speed, but not send speed, and since minimum of those two is returned, CheckResult validates send bitrate is unaffected by last packet. https://codereview.webrtc.org/2254733005/diff/40001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc:135: CheckResult(0, 800000, 10, 40); May be add a note that 1st CheckResult check bitrate after 4 packets, last - after 5 packets. https://codereview.webrtc.org/2254733005/diff/40001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc:146: CheckResult(0, 800000, 10, 40); does it make sense to CheckResult twice in this test?
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2254733005/#ps60001 (title: "Fix comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2254733005/diff/40001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probe_bitrate_estimator.cc (right): https://codereview.webrtc.org/2254733005/diff/40001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_bitrate_estimator.cc:50: int payload_size = packet_info.payload_size * 8; On 2016/08/22 13:53:35, danilchap wrote: > may be call it payload_size_bits Done. https://codereview.webrtc.org/2254733005/diff/40001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_bitrate_estimator.cc:91: float send_size = cluster->size_total - cluster->size_last_send; On 2016/08/22 13:53:35, danilchap wrote: > probably RTC_DCHECK_GT(cluster->size_total - cluster->size_last_send, 0); > [e.g. to catch when payload size would be reported as 0 for padding packets] Done. https://codereview.webrtc.org/2254733005/diff/40001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc (right): https://codereview.webrtc.org/2254733005/diff/40001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc:133: AddPacketFeedback(0, 1500, 40, 50); On 2016/08/22 13:53:35, danilchap wrote: > May be add note that increased packet size increase received speed, but not send > speed, and since minimum of those two is returned, CheckResult validates send > bitrate is unaffected by last packet. Acknowledged. https://codereview.webrtc.org/2254733005/diff/40001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc:135: CheckResult(0, 800000, 10, 40); On 2016/08/22 13:53:35, danilchap wrote: > May be add a note that 1st CheckResult check bitrate after 4 packets, last - > after 5 packets. Acknowledged. https://codereview.webrtc.org/2254733005/diff/40001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc:146: CheckResult(0, 800000, 10, 40); On 2016/08/22 13:53:35, danilchap wrote: > does it make sense to CheckResult twice in this test? Copy-paste ftw :) But no, it doesn't.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/7589)
Suggest updating issue subject since this CL is no more about "Keep track of insert probe packets"
lgtm
The CQ bit was checked by philipel@webrtc.org
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
Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
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, danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2254733005/#ps80001 (title: "Rebase")
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
Try jobs failed on following builders: mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/13605)
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, danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2254733005/#ps100001 (title: "Rebase fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Only use payload size within the know send/receive interval for probing calculations. BUG=webrtc:5859 ========== to ========== Only use payload size within the know send/receive interval for probing calculations. BUG=webrtc:5859 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Only use payload size within the know send/receive interval for probing calculations. BUG=webrtc:5859 ========== to ========== Only use payload size within the know send/receive interval for probing calculations. BUG=webrtc:5859 Committed: https://crrev.com/0561bdf8338aa58f164b7d82d75368d9777c2684 Cr-Commit-Position: refs/heads/master@{#13889} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/0561bdf8338aa58f164b7d82d75368d9777c2684 Cr-Commit-Position: refs/heads/master@{#13889} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
