Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2121183002:
New ProbingCalculator class. (Closed)
|
Created:
4 years, 5 months ago by philipel Modified:
4 years, 4 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, stefan-webrtc, mflodman, terelius Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionNew ProbingCalculator class.
The ProbingCalculator class calculates and validates the results from
probing attempts.
BUG=webrtc:5859
Committed: https://crrev.com/233c4ba4fd294822fd878847feee596c2922d4c9
Cr-Commit-Position: refs/heads/master@{#13589}
Patch Set 1 #Patch Set 2 : Clean up #
Total comments: 1
Patch Set 3 : Interface for result callback. #Patch Set 4 : Compile fixes. #
Total comments: 11
Patch Set 5 : Feedback fixes. #
Total comments: 2
Patch Set 6 : Calculate send/receive rate using floats. #
Total comments: 5
Patch Set 7 : Added kMaxNumSavedClusters. #
Total comments: 10
Patch Set 8 : Removed callback, return result instead. #
Total comments: 2
Patch Set 9 : Renamed ProbingCalculator to ProbeBitrateEstimator. #Patch Set 10 : Rebase #Patch Set 11 : BUILD estiamtor to estimator. #
Messages
Total messages: 46 (18 generated)
philipel@webrtc.org changed reviewers: + stefan@webrtc.org, terelius@webrtc.org
The CQ bit was checked by philipel@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: Try jobs failed on following builders: android_compile_x86_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...)
The CQ bit was checked by philipel@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.
philipel@webrtc.org changed reviewers: + brandtr@webrtc.org - terelius@webrtc.org
https://codereview.webrtc.org/2121183002/diff/60001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probing_calculator.cc (right): https://codereview.webrtc.org/2121183002/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probing_calculator.cc:22: constexpr float kValidRatio = 1.2f; Is there a motivation for this number? [For the future: would it be relevant to adapt this number depending on the network the user is on?] https://codereview.webrtc.org/2121183002/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probing_calculator.cc:31: // If this is not probing packet or if this probing packet Minor: "If this is not _a_ probing..." https://codereview.webrtc.org/2121183002/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probing_calculator.cc:66: int receive_bps = cluster->size / receive_interval_ms * 1000; Integer division may not be desirable when the cluster size and |receive_interval_ms| have different orders of magnitude. https://codereview.webrtc.org/2121183002/diff/60001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probing_calculator.h (right): https://codereview.webrtc.org/2121183002/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probing_calculator.h:19: namespace webrtc { Add newline in between. https://codereview.webrtc.org/2121183002/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probing_calculator.h:29: void PacketFeedback(const PacketInfo& packet_info); Maybe add a comment what this member function does? It was not immediately obvious to me, without checking out the definition.
https://codereview.webrtc.org/2121183002/diff/60001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probing_calculator.cc (right): https://codereview.webrtc.org/2121183002/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probing_calculator.cc:22: constexpr float kValidRatio = 1.2f; On 2016/07/07 14:58:30, brandtr wrote: > Is there a motivation for this number? Kind of, we don't want the receive rate to be that much higher since that would indicate that packets have been sent in a burst, which does not give us information about the available bandwidth. If someone were to ask me if 1.15 or 1.25 is a better ratio, I would say "I don't know". > [For the future: would it be relevant to adapt this number depending on the > network the user is on?] I don't think so, what would be interesting is to increase the number of packets in the probing attempt in order to avoid having them all delivered in a single burst. https://codereview.webrtc.org/2121183002/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probing_calculator.cc:31: // If this is not probing packet or if this probing packet On 2016/07/07 14:58:30, brandtr wrote: > Minor: "If this is not _a_ probing..." Done. https://codereview.webrtc.org/2121183002/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probing_calculator.cc:66: int receive_bps = cluster->size / receive_interval_ms * 1000; On 2016/07/07 14:58:30, brandtr wrote: > Integer division may not be desirable when the cluster size and > |receive_interval_ms| have different orders of magnitude. Switched to float. https://codereview.webrtc.org/2121183002/diff/60001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probing_calculator.h (right): https://codereview.webrtc.org/2121183002/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probing_calculator.h:19: namespace webrtc { On 2016/07/07 14:58:30, brandtr wrote: > Add newline in between. Done. https://codereview.webrtc.org/2121183002/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probing_calculator.h:29: void PacketFeedback(const PacketInfo& packet_info); On 2016/07/07 14:58:30, brandtr wrote: > Maybe add a comment what this member function does? It was not immediately > obvious to me, without checking out the definition. Done.
https://codereview.webrtc.org/2121183002/diff/60001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probing_calculator.cc (right): https://codereview.webrtc.org/2121183002/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probing_calculator.cc:22: constexpr float kValidRatio = 1.2f; On 2016/07/08 09:27:48, philipel wrote: > If someone were to ask me if 1.15 or 1.25 is a better ratio, I would say "I > don't know". Right, this is what I wondered :) https://codereview.webrtc.org/2121183002/diff/80001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probing_calculator.h (right): https://codereview.webrtc.org/2121183002/diff/80001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probing_calculator.h:43: float size = 0; Why not cast to a float where needed for the computation instead?
https://codereview.webrtc.org/2121183002/diff/80001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probing_calculator.h (right): https://codereview.webrtc.org/2121183002/diff/80001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probing_calculator.h:43: float size = 0; On 2016/07/08 09:39:21, brandtr wrote: > Why not cast to a float where needed for the computation instead? I guess that's cleaner, done.
lgtm
Nice! https://codereview.webrtc.org/2121183002/diff/100001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/probing_calculator.cc (right): https://codereview.webrtc.org/2121183002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_calculator.cc:50: if (cluster->num_probes < kMinNumProbesValidCluster) Question about future follow up: Do we have any plans to take packet loss into account in the probe? If the probe cluster is supposed to contain 8 packets and we lose 4 but receive the other 4 in rapid succession, we still consider the probe a success and set a high bitrate. https://codereview.webrtc.org/2121183002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_calculator.cc:97: clusters_.erase(clusters_.begin(), erase_to); We only clear the map after a successful probe? What happens if a bug causes (almost) all probes to fail and the map grows without bounds?
Description was changed from ========== New ProbingCalculator class. The ProbingCalculator class calculates and validates the results from probing attempts. BUG=webrtc:5859 ========== to ========== New ProbingCalculator class. The ProbingCalculator class calculates and validates the results from probing attempts. BUG=webrtc:5859 ==========
https://codereview.webrtc.org/2121183002/diff/100001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/probing_calculator.cc (right): https://codereview.webrtc.org/2121183002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_calculator.cc:50: if (cluster->num_probes < kMinNumProbesValidCluster) On 2016/07/11 10:07:48, terelius wrote: > Question about future follow up: Do we have any plans to take packet loss into > account in the probe? If the probe cluster is supposed to contain 8 packets and > we lose 4 but receive the other 4 in rapid succession, we still consider the > probe a success and set a high bitrate. Might be something that's interesting in the future, but for now we know that a probing cluster contains 5-6 packets. This threshold/logic might have to be revised as we tune/update the probing. https://codereview.webrtc.org/2121183002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_calculator.cc:97: clusters_.erase(clusters_.begin(), erase_to); On 2016/07/11 10:07:48, terelius wrote: > We only clear the map after a successful probe? What happens if a bug causes > (almost) all probes to fail and the map grows without bounds? fixed.
terelius@webrtc.org changed reviewers: + terelius@webrtc.org
lgtm https://codereview.webrtc.org/2121183002/diff/100001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/probing_calculator.cc (right): https://codereview.webrtc.org/2121183002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_calculator.cc:50: if (cluster->num_probes < kMinNumProbesValidCluster) On 2016/07/11 11:18:55, philipel wrote: > On 2016/07/11 10:07:48, terelius wrote: > > Question about future follow up: Do we have any plans to take packet loss into > > account in the probe? If the probe cluster is supposed to contain 8 packets > and > > we lose 4 but receive the other 4 in rapid succession, we still consider the > > probe a success and set a high bitrate. > > Might be something that's interesting in the future, but for now we know that a > probing cluster contains 5-6 packets. This threshold/logic might have to be > revised as we tune/update the probing. Acknowledged.
ping
https://codereview.webrtc.org/2121183002/diff/20001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/probing_calculator.cc (right): https://codereview.webrtc.org/2121183002/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/probing_calculator.cc:91: << " kB/s]" kbps or possibly kb/s (but the former is more common in the code). kB/s will likely be interpreted as kilo bytes/s. https://codereview.webrtc.org/2121183002/diff/120001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/probing_calculator.cc (right): https://codereview.webrtc.org/2121183002/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_calculator.cc:65: LOG(LS_INFO) << "Probing unsuccessful, invalid send/receive interval!" Can we change from "interval!" to "interval."? Just to not have people react to this as something negative when reading logs from bug reports. :) https://codereview.webrtc.org/2121183002/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_calculator.cc:78: LOG(LS_INFO) << "Probing unsuccessful, receive/send ratio to high!" Same here. https://codereview.webrtc.org/2121183002/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_calculator.cc:81: << send_interval_ms << " ms = " << send_bps / 1000 << " kB/s]" kbps instead of kB/s. B == bytes. https://codereview.webrtc.org/2121183002/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_calculator.cc:92: result_callback_->ProbingResult(result_bps, packet_info.arrival_time_ms); Maybe we can change this class to simply return the result instead of having a callback? I think that would be simpler.
https://codereview.webrtc.org/2121183002/diff/120001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/probing_calculator.cc (right): https://codereview.webrtc.org/2121183002/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_calculator.cc:78: LOG(LS_INFO) << "Probing unsuccessful, receive/send ratio to high!" On 2016/07/15 11:05:31, stefan-webrtc (holmer) wrote: > Same here. Also s/to/too/
https://codereview.webrtc.org/2121183002/diff/120001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/probing_calculator.cc (right): https://codereview.webrtc.org/2121183002/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_calculator.cc:65: LOG(LS_INFO) << "Probing unsuccessful, invalid send/receive interval!" On 2016/07/15 11:05:31, stefan-webrtc (holmer) wrote: > Can we change from "interval!" to "interval."? Just to not have people react to > this as something negative when reading logs from bug reports. :) Done. https://codereview.webrtc.org/2121183002/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_calculator.cc:78: LOG(LS_INFO) << "Probing unsuccessful, receive/send ratio to high!" On 2016/07/15 11:05:31, stefan-webrtc (holmer) wrote: > Same here. Done. https://codereview.webrtc.org/2121183002/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_calculator.cc:78: LOG(LS_INFO) << "Probing unsuccessful, receive/send ratio to high!" On 2016/07/15 11:13:01, terelius wrote: > On 2016/07/15 11:05:31, stefan-webrtc (holmer) wrote: > > Same here. > > Also s/to/too/ Done. https://codereview.webrtc.org/2121183002/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_calculator.cc:81: << send_interval_ms << " ms = " << send_bps / 1000 << " kB/s]" On 2016/07/15 11:05:31, stefan-webrtc (holmer) wrote: > kbps instead of kB/s. B == bytes. Done. https://codereview.webrtc.org/2121183002/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_calculator.cc:92: result_callback_->ProbingResult(result_bps, packet_info.arrival_time_ms); On 2016/07/15 11:05:31, stefan-webrtc (holmer) wrote: > Maybe we can change this class to simply return the result instead of having a > callback? I think that would be simpler. Done.
ping
still lgtm
lgtm, but consider my nits. https://codereview.webrtc.org/2121183002/diff/140001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/probing_calculator.cc (right): https://codereview.webrtc.org/2121183002/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_calculator.cc:82: float ratio = receive_bps / send_bps; DCHECK_GT(send_bps, 0); https://codereview.webrtc.org/2121183002/diff/140001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/probing_calculator.h (right): https://codereview.webrtc.org/2121183002/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probing_calculator.h:31: class ProbingCalculator { Perhaps ProbeBitrateEstimator would be a better name that aligns with the other names of the congestion controller?
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org, terelius@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2121183002/#ps160001 (title: "Renamed ProbingCalculator to ProbeBitrateEstimator.")
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_compile_x86_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...) linux_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...) linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...) linux_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/13852) linux_ubsan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/3728) linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...) mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/13012)
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from terelius@webrtc.org, brandtr@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2121183002/#ps180001 (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: linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...) mac_gn_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/11126)
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from terelius@webrtc.org, brandtr@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2121183002/#ps200001 (title: "BUILD estiamtor to estimator.")
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 ========== New ProbingCalculator class. The ProbingCalculator class calculates and validates the results from probing attempts. BUG=webrtc:5859 ========== to ========== New ProbingCalculator class. The ProbingCalculator class calculates and validates the results from probing attempts. BUG=webrtc:5859 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== New ProbingCalculator class. The ProbingCalculator class calculates and validates the results from probing attempts. BUG=webrtc:5859 ========== to ========== New ProbingCalculator class. The ProbingCalculator class calculates and validates the results from probing attempts. BUG=webrtc:5859 Committed: https://crrev.com/233c4ba4fd294822fd878847feee596c2922d4c9 Cr-Commit-Position: refs/heads/master@{#13589} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/233c4ba4fd294822fd878847feee596c2922d4c9 Cr-Commit-Position: refs/heads/master@{#13589} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
