Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1995683003:
Allow FakeNetworkPipe to drop packets in bursts. (Closed)
|
Created:
4 years, 7 months ago by philipel Modified:
4 years, 6 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, pbos-webrtc, perkj_webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAllow FakeNetworkPipe to drop packets in bursts.
The fake network pipe will still only drop packets at an average rate of
|loss_percent| but in bursts at an average length specified by
|avg_burst_loss_length|.
Also added the flag -avg_burst_loss_length to video loopback.
BUG=
Committed: https://crrev.com/536378bf37c7c7624832e4deabd4fcf032ebcefb
Cr-Commit-Position: refs/heads/master@{#12969}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Feedback fixes, also added sanity check for avg_burst_loss_length parameter. #
Total comments: 10
Patch Set 3 : feedback fixes. #
Total comments: 6
Patch Set 4 : Fixed comment. #Patch Set 5 : Included cmath for std::ceil. #
Messages
Total messages: 25 (8 generated)
philipel@webrtc.org changed reviewers: + mflodman@webrtc.org, terelius@webrtc.org
Comments based on offline discussion below. https://codereview.webrtc.org/1995683003/diff/1/webrtc/test/fake_network_pipe.cc File webrtc/test/fake_network_pipe.cc (right): https://codereview.webrtc.org/1995683003/diff/1/webrtc/test/fake_network_pipe... webrtc/test/fake_network_pipe.cc:40: prob_burst_loss_ = (static_cast<double>(config_.loss_percent) / This should probably be config_.loss_percent / (1-config_.loss_percent) / (config_.avg_burst_loss_length) in order to lose on average config_.loss_percent packets. https://codereview.webrtc.org/1995683003/diff/1/webrtc/test/fake_network_pipe... webrtc/test/fake_network_pipe.cc:129: if (bursting_ || random_.Rand<double>() < prob_burst_loss_) { It would easier to model the process if it was if (bursting_) { // Drop the packet with some probability } else { // Drop the packet with some other probability } bursting = did_we_just_drop_a_packet https://codereview.webrtc.org/1995683003/diff/1/webrtc/test/fake_network_pipe... File webrtc/test/fake_network_pipe_unittest.cc (right): https://codereview.webrtc.org/1995683003/diff/1/webrtc/test/fake_network_pipe... webrtc/test/fake_network_pipe_unittest.cc:407: EXPECT_NEAR((100.0 - kLossPercent) / 100.0, loss_percent, 0.1); This tests that the receive percentage is between 81% and 99%. Would be better to test that the loss percentage is between 9 and 11% or something.
https://codereview.webrtc.org/1995683003/diff/1/webrtc/test/fake_network_pipe.cc File webrtc/test/fake_network_pipe.cc (right): https://codereview.webrtc.org/1995683003/diff/1/webrtc/test/fake_network_pipe... webrtc/test/fake_network_pipe.cc:40: prob_burst_loss_ = (static_cast<double>(config_.loss_percent) / On 2016/05/30 14:00:29, terelius wrote: > This should probably be > config_.loss_percent / (1-config_.loss_percent) / > (config_.avg_burst_loss_length) > in order to lose on average config_.loss_percent packets. Done. https://codereview.webrtc.org/1995683003/diff/1/webrtc/test/fake_network_pipe... webrtc/test/fake_network_pipe.cc:129: if (bursting_ || random_.Rand<double>() < prob_burst_loss_) { On 2016/05/30 14:00:29, terelius wrote: > It would easier to model the process if it was > if (bursting_) { > // Drop the packet with some probability > } > else { > // Drop the packet with some other probability > } > bursting = did_we_just_drop_a_packet Done. https://codereview.webrtc.org/1995683003/diff/1/webrtc/test/fake_network_pipe... File webrtc/test/fake_network_pipe_unittest.cc (right): https://codereview.webrtc.org/1995683003/diff/1/webrtc/test/fake_network_pipe... webrtc/test/fake_network_pipe_unittest.cc:407: EXPECT_NEAR((100.0 - kLossPercent) / 100.0, loss_percent, 0.1); On 2016/05/30 14:00:30, terelius wrote: > This tests that the receive percentage is between 81% and 99%. Would be better > to test that the loss percentage is between 9 and 11% or something. Done.
stefan@webrtc.org changed reviewers: + stefan@webrtc.org
https://codereview.webrtc.org/1995683003/diff/1/webrtc/test/fake_network_pipe.cc File webrtc/test/fake_network_pipe.cc (right): https://codereview.webrtc.org/1995683003/diff/1/webrtc/test/fake_network_pipe... webrtc/test/fake_network_pipe.cc:129: if (bursting_ || random_.Rand<double>() < prob_burst_loss_) { On 2016/05/30 14:42:42, philipel wrote: > On 2016/05/30 14:00:29, terelius wrote: > > It would easier to model the process if it was > > if (bursting_) { > > // Drop the packet with some probability > > } > > else { > > // Drop the packet with some other probability > > } > > bursting = did_we_just_drop_a_packet > > Done. Fyi, burst loss is typically modelled using a gilbert-elliot model. I think we have an implementation already somewhere in the code
https://codereview.webrtc.org/1995683003/diff/20001/webrtc/test/fake_network_... File webrtc/test/fake_network_pipe.cc (right): https://codereview.webrtc.org/1995683003/diff/20001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe.cc:42: prob_loss_bursting_ = 0; This won't really be uniform. After each loss, the probability is zero to lose another packet. https://codereview.webrtc.org/1995683003/diff/20001/webrtc/test/fake_network_... File webrtc/test/fake_network_pipe.h (right): https://codereview.webrtc.org/1995683003/diff/20001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe.h:145: // The probability to drop F%a burst of packets. F%a -> a https://codereview.webrtc.org/1995683003/diff/20001/webrtc/test/fake_network_... File webrtc/test/fake_network_pipe_unittest.cc (right): https://codereview.webrtc.org/1995683003/diff/20001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe_unittest.cc:404: double loss_percent = nit: maybe call it loss_fraction to make it clear why you don't divide this by 100? (I.e. why this is treated differently from kLossPercent.) https://codereview.webrtc.org/1995683003/diff/20001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe_unittest.cc:408: EXPECT_NEAR(kLossPercent / 100.0, loss_percent, 0.01); nit: 0.01 is the relative error, right? Seems rather tight. Does this work with other parameters and other random seeds? https://codereview.webrtc.org/1995683003/diff/20001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe_unittest.cc:420: int lost_packets = kNumPackets - receiver->delivered_sequence_numbers_.size(); Move this up and reuse in computation of loss_percent.
Took a quick look at gilbert-elliot model and it looks like that is what has been implemented. There is a GilbertElliot class in webrtc/modules/audio_coding/neteq/tools/neteq_quality_test.cc, but it would require some refactoring to use it. https://codereview.webrtc.org/1995683003/diff/20001/webrtc/test/fake_network_... File webrtc/test/fake_network_pipe.cc (right): https://codereview.webrtc.org/1995683003/diff/20001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe.cc:42: prob_loss_bursting_ = 0; On 2016/05/30 15:40:23, terelius wrote: > This won't really be uniform. After each loss, the probability is zero to lose > another packet. Fixed. https://codereview.webrtc.org/1995683003/diff/20001/webrtc/test/fake_network_... File webrtc/test/fake_network_pipe.h (right): https://codereview.webrtc.org/1995683003/diff/20001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe.h:145: // The probability to drop F%a burst of packets. On 2016/05/30 15:40:23, terelius wrote: > F%a -> a Done. https://codereview.webrtc.org/1995683003/diff/20001/webrtc/test/fake_network_... File webrtc/test/fake_network_pipe_unittest.cc (right): https://codereview.webrtc.org/1995683003/diff/20001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe_unittest.cc:404: double loss_percent = On 2016/05/30 15:40:23, terelius wrote: > nit: maybe call it loss_fraction to make it clear why you don't divide this by > 100? (I.e. why this is treated differently from kLossPercent.) Done. https://codereview.webrtc.org/1995683003/diff/20001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe_unittest.cc:408: EXPECT_NEAR(kLossPercent / 100.0, loss_percent, 0.01); On 2016/05/30 15:40:23, terelius wrote: > nit: 0.01 is the relative error, right? Seems rather tight. Does this work with > other parameters and other random seeds? It's a bit tight, adjusted it to 0.05. Also tried a few different values and it only seems to be the the check on line 423 that fails instead. https://codereview.webrtc.org/1995683003/diff/20001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe_unittest.cc:420: int lost_packets = kNumPackets - receiver->delivered_sequence_numbers_.size(); On 2016/05/30 15:40:23, terelius wrote: > Move this up and reuse in computation of loss_percent. Done.
https://codereview.webrtc.org/1995683003/diff/40001/webrtc/test/fake_network_... File webrtc/test/fake_network_pipe.h (right): https://codereview.webrtc.org/1995683003/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe.h:84: int avg_burst_loss_length = -1; I think it would be good to make it a double. There doesn't seem to be any particular reason why the average burst loss should be an integer.
https://codereview.webrtc.org/1995683003/diff/40001/webrtc/test/fake_network_... File webrtc/test/fake_network_pipe.h (right): https://codereview.webrtc.org/1995683003/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe.h:84: int avg_burst_loss_length = -1; On 2016/05/30 16:26:32, terelius wrote: > I think it would be good to make it a double. There doesn't seem to be any > particular reason why the average burst loss should be an integer. I guess it makes more sense to use a double here instead, but I think it would be better with a follow-up CL where I change that (and maybe loss_percent as well) instead.
lgtm
LGTM with comment addressed. https://codereview.webrtc.org/1995683003/diff/40001/webrtc/test/fake_network_... File webrtc/test/fake_network_pipe.h (right): https://codereview.webrtc.org/1995683003/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe.h:84: int avg_burst_loss_length = -1; On 2016/05/30 16:29:25, philipel wrote: > On 2016/05/30 16:26:32, terelius wrote: > > I think it would be good to make it a double. There doesn't seem to be any > > particular reason why the average burst loss should be an integer. > > I guess it makes more sense to use a double here instead, but I think it would > be better with a follow-up CL where I change that (and maybe loss_percent as > well) instead. Do we really need this as a double? I think int precision is fine in this case. https://codereview.webrtc.org/1995683003/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe.h:145: // The probability to drop a burst of packets. It would be good to add something telling the model, e.g. "according to gilbert-elliot model."
lgtm
https://codereview.webrtc.org/1995683003/diff/40001/webrtc/test/fake_network_... File webrtc/test/fake_network_pipe.h (right): https://codereview.webrtc.org/1995683003/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe.h:84: int avg_burst_loss_length = -1; On 2016/05/31 07:42:02, mflodman wrote: > On 2016/05/30 16:29:25, philipel wrote: > > On 2016/05/30 16:26:32, terelius wrote: > > > I think it would be good to make it a double. There doesn't seem to be any > > > particular reason why the average burst loss should be an integer. > > > > I guess it makes more sense to use a double here instead, but I think it would > > be better with a follow-up CL where I change that (and maybe loss_percent as > > well) instead. > > Do we really need this as a double? I think int precision is fine in this case. Don't think we need this as a double, just something that might have been nice. If anyone needs it they can easily change it... https://codereview.webrtc.org/1995683003/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe.h:145: // The probability to drop a burst of packets. On 2016/05/31 07:42:02, mflodman wrote: > It would be good to add something telling the model, e.g. "according to > gilbert-elliot model." Done kind of. I added a comment to the .cc file since we configure |prob_loss_bursting_| and |prob_start_bursting_| differently depending on which type of loss we want.
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, stefan@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/1995683003/#ps60001 (title: "Fixed comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1995683003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995683003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/13767)
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, stefan@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/1995683003/#ps80001 (title: "Included cmath for std::ceil.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1995683003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1995683003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Allow FakeNetworkPipe to drop packets in bursts. The fake network pipe will still only drop packets at an average rate of |loss_percent| but in bursts at an average length specified by |avg_burst_loss_length|. Also added the flag -avg_burst_loss_length to video loopback. BUG= ========== to ========== Allow FakeNetworkPipe to drop packets in bursts. The fake network pipe will still only drop packets at an average rate of |loss_percent| but in bursts at an average length specified by |avg_burst_loss_length|. Also added the flag -avg_burst_loss_length to video loopback. BUG= Committed: https://crrev.com/536378bf37c7c7624832e4deabd4fcf032ebcefb Cr-Commit-Position: refs/heads/master@{#12969} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/536378bf37c7c7624832e4deabd4fcf032ebcefb Cr-Commit-Position: refs/heads/master@{#12969} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
