Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1606183002:
Allow packets to be reordered in the fake network pipe. (Closed)
DescriptionAllow packets to be reordered in the fake network pipe.
BUG=
Committed: https://crrev.com/a2c55235cae5f16715911bff6109fe915a8085bf
Cr-Commit-Position: refs/heads/master@{#11384}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Unittest and flag for video/screenshare loopback. #Patch Set 3 : Comments #
Total comments: 19
Patch Set 4 : Comments. #
Total comments: 5
Patch Set 5 : Nit and format. #Patch Set 6 : uint8_t[] instead of uint8_t #
Messages
Total messages: 25 (10 generated)
Description was changed from ========== Allow packets to be reordered in the fake network pipe. BUG= ========== to ========== Allow packets to be reordered in the fake network pipe. BUG= ==========
philipel@webrtc.org changed reviewers: + pbos@webrtc.org
pbos@webrtc.org changed reviewers: + stefan@webrtc.org
+R stefan@
https://codereview.webrtc.org/1606183002/diff/1/webrtc/test/fake_network_pipe.cc File webrtc/test/fake_network_pipe.cc (right): https://codereview.webrtc.org/1606183002/diff/1/webrtc/test/fake_network_pipe... webrtc/test/fake_network_pipe.cc:137: // earlier than the last packet in the queue. Half of this comment should be moved down to line 144 or something. https://codereview.webrtc.org/1606183002/diff/1/webrtc/test/fake_network_pipe... webrtc/test/fake_network_pipe.cc:138: int extra_delay = GaussianRandom(config_.queue_delay_ms, I think it would be good to rename this random_jitter_ms or maybe extra_jitter_ms. What do you think? https://codereview.webrtc.org/1606183002/diff/1/webrtc/test/fake_network_pipe... webrtc/test/fake_network_pipe.cc:141: delay_link_.size() > 0 && !delay_link_.empty() https://codereview.webrtc.org/1606183002/diff/1/webrtc/test/fake_network_pipe.h File webrtc/test/fake_network_pipe.h (right): https://codereview.webrtc.org/1606183002/diff/1/webrtc/test/fake_network_pipe... webrtc/test/fake_network_pipe.h:54: uint8_t* data_; Please change this to a scoped_ptr<uint8_t[]>, or maybe even a vector<uint8_t>? https://codereview.webrtc.org/1606183002/diff/1/webrtc/test/fake_network_pipe... webrtc/test/fake_network_pipe.h:115: struct PacketArrivalTimeComperator { Comparator https://codereview.webrtc.org/1606183002/diff/1/webrtc/test/fake_network_pipe... webrtc/test/fake_network_pipe.h:120: std::multiset<NetworkPacket*, PacketArrivalTimeComperator> delay_link_; Maybe add a comment on why this needs to be sorted.
Could you add a unittest for this new feature too? And maybe hook it up to video_loopback as a command line option?
https://codereview.webrtc.org/1606183002/diff/1/webrtc/test/fake_network_pipe.cc File webrtc/test/fake_network_pipe.cc (right): https://codereview.webrtc.org/1606183002/diff/1/webrtc/test/fake_network_pipe... webrtc/test/fake_network_pipe.cc:137: // earlier than the last packet in the queue. On 2016/01/20 10:57:33, stefan-webrtc (holmer) wrote: > Half of this comment should be moved down to line 144 or something. Done. https://codereview.webrtc.org/1606183002/diff/1/webrtc/test/fake_network_pipe... webrtc/test/fake_network_pipe.cc:138: int extra_delay = GaussianRandom(config_.queue_delay_ms, On 2016/01/20 10:57:33, stefan-webrtc (holmer) wrote: > I think it would be good to rename this random_jitter_ms or maybe > extra_jitter_ms. What do you think? Renamed to arrival_time_jitter. https://codereview.webrtc.org/1606183002/diff/1/webrtc/test/fake_network_pipe... webrtc/test/fake_network_pipe.cc:141: delay_link_.size() > 0 && On 2016/01/20 10:57:33, stefan-webrtc (holmer) wrote: > !delay_link_.empty() Done. https://codereview.webrtc.org/1606183002/diff/1/webrtc/test/fake_network_pipe.h File webrtc/test/fake_network_pipe.h (right): https://codereview.webrtc.org/1606183002/diff/1/webrtc/test/fake_network_pipe... webrtc/test/fake_network_pipe.h:54: uint8_t* data_; On 2016/01/20 10:57:33, stefan-webrtc (holmer) wrote: > Please change this to a scoped_ptr<uint8_t[]>, or maybe even a vector<uint8_t>? Done. https://codereview.webrtc.org/1606183002/diff/1/webrtc/test/fake_network_pipe... webrtc/test/fake_network_pipe.h:115: struct PacketArrivalTimeComperator { On 2016/01/20 10:57:33, stefan-webrtc (holmer) wrote: > Comparator Done. https://codereview.webrtc.org/1606183002/diff/1/webrtc/test/fake_network_pipe... webrtc/test/fake_network_pipe.h:120: std::multiset<NetworkPacket*, PacketArrivalTimeComperator> delay_link_; On 2016/01/20 10:57:33, stefan-webrtc (holmer) wrote: > Maybe add a comment on why this needs to be sorted. Done.
https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... File webrtc/test/fake_network_pipe_unittest.cc (right): https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe_unittest.cc:335: // At first don't disallow reordering and then allow reordering. Woops, commend fixed.
https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... File webrtc/test/fake_network_pipe.h (right): https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe.h:55: // The time the packet should arrive at the reciver. s/reciver/receiver https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe.h:113: // hence, we cannot use a priority queue. I guess it's also a way to make sure that the packets are delivered in arrival time order, which may not be the case now that we introduced reordering? https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... File webrtc/test/fake_network_pipe_unittest.cc (right): https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe_unittest.cc:26: class UnitTestReceiver : public PacketReceiver { TestReceiver would be enough https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe_unittest.cc:41: class UnitTestImplReceiver : public UnitTestReceiver { Impl suggests that this is implementing from an interface, which it's not. I'd suggest naming this ReorderTestReceiver and have it inherit directly from PacketReceiver. Then you can simply reset receiver_ with a ReorderTestReceiver in the test cases where you need it. https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe_unittest.cc:346: // Add 1000 packets of 1000 bytes, = 10 kB. Remove "," https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe_unittest.cc:348: const int kPacketSize = 10; How does this add 1000 packets of 1000 bytes? Looks like 100 packets of 10 bytes. :) https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe_unittest.cc:370: EXPECT_EQ(receiver_impl_->delivered_sequence_numbers_.size(), kNumPackets); Expectation first, i.e., EXPECT_EQ(kNumPackets, ...); https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe_unittest.cc:380: EXPECT_TRUE(reordering_has_occured); Can this be flaky, or can you make sure it always succeeds by seeding the random generator? Make sure the FakeNetworkPipe uses the Random class and not rand() since rand() generates different sequences on different platforms.
https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... File webrtc/test/fake_network_pipe.h (right): https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe.h:55: // The time the packet should arrive at the reciver. On 2016/01/25 13:16:57, stefan-webrtc (holmer) wrote: > s/reciver/receiver Done. https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe.h:113: // hence, we cannot use a priority queue. On 2016/01/25 13:16:57, stefan-webrtc (holmer) wrote: > I guess it's also a way to make sure that the packets are delivered in arrival > time order, which may not be the case now that we introduced reordering? The packets are always delivered in arrival time order, only now we allow the arrival time to be modified in such a way that reordering occurs. https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... File webrtc/test/fake_network_pipe_unittest.cc (right): https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe_unittest.cc:26: class UnitTestReceiver : public PacketReceiver { On 2016/01/25 13:16:57, stefan-webrtc (holmer) wrote: > TestReceiver would be enough Done. https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe_unittest.cc:41: class UnitTestImplReceiver : public UnitTestReceiver { On 2016/01/25 13:16:57, stefan-webrtc (holmer) wrote: > Impl suggests that this is implementing from an interface, which it's not. > > I'd suggest naming this ReorderTestReceiver and have it inherit directly from > PacketReceiver. Then you can simply reset receiver_ with a ReorderTestReceiver > in the test cases where you need it. I have tried to do this but then the receiver_ has to be casted to either a TestReceiver or a ReorderedTestReceiver for every test case to either set up the mock behavior or to access the delivered_sequence_numbers_. https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe_unittest.cc:346: // Add 1000 packets of 1000 bytes, = 10 kB. On 2016/01/25 13:16:57, stefan-webrtc (holmer) wrote: > Remove "," Removed comment all together. https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe_unittest.cc:348: const int kPacketSize = 10; On 2016/01/25 13:16:57, stefan-webrtc (holmer) wrote: > How does this add 1000 packets of 1000 bytes? Looks like 100 packets of 10 > bytes. :) No idea :) https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe_unittest.cc:380: EXPECT_TRUE(reordering_has_occured); On 2016/01/25 13:16:57, stefan-webrtc (holmer) wrote: > Can this be flaky, or can you make sure it always succeeds by seeding the random > generator? Make sure the FakeNetworkPipe uses the Random class and not rand() > since rand() generates different sequences on different platforms. Can't say for sure if it's flaky. The random function is never seeded so it always start with the same seed, but I'm not sure if these test cases run in isolation or if they share state (rand internal state) between test cases. Anyhow, I have now changed in the FakeNetworkPipe to use the Random class and it is now possible its internal random generator on construction.
Fix my nits and remove the default argument, then lgtm. https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... File webrtc/test/fake_network_pipe.h (right): https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe.h:113: // hence, we cannot use a priority queue. On 2016/01/26 10:50:47, philipel wrote: > On 2016/01/25 13:16:57, stefan-webrtc (holmer) wrote: > > I guess it's also a way to make sure that the packets are delivered in arrival > > time order, which may not be the case now that we introduced reordering? > > The packets are always delivered in arrival time order, only now we allow the > arrival time to be modified in such a way that reordering occurs. Yes, and that's one of the reasons why you need a sorted data structure, since otherwise the modified arrival time would be out of order. You may want to add that to the comment, but I can live without it. https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... File webrtc/test/fake_network_pipe_unittest.cc (right): https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe_unittest.cc:41: class UnitTestImplReceiver : public UnitTestReceiver { On 2016/01/26 10:50:47, philipel wrote: > On 2016/01/25 13:16:57, stefan-webrtc (holmer) wrote: > > Impl suggests that this is implementing from an interface, which it's not. > > > > I'd suggest naming this ReorderTestReceiver and have it inherit directly from > > PacketReceiver. Then you can simply reset receiver_ with a ReorderTestReceiver > > in the test cases where you need it. > > I have tried to do this but then the receiver_ has to be casted to either a > TestReceiver or a ReorderedTestReceiver for every test case to either set up the > mock behavior or to access the delivered_sequence_numbers_. OK, I can live with how it is now. https://codereview.webrtc.org/1606183002/diff/40001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe_unittest.cc:380: EXPECT_TRUE(reordering_has_occured); On 2016/01/26 10:50:47, philipel wrote: > On 2016/01/25 13:16:57, stefan-webrtc (holmer) wrote: > > Can this be flaky, or can you make sure it always succeeds by seeding the > random > > generator? Make sure the FakeNetworkPipe uses the Random class and not rand() > > since rand() generates different sequences on different platforms. > > Can't say for sure if it's flaky. The random function is never seeded so it > always start with the same seed, but I'm not sure if these test cases run in > isolation or if they share state (rand internal state) between test cases. > > Anyhow, I have now changed in the FakeNetworkPipe to use the Random class and it > is now possible its internal random generator on construction. Good. rand() doesn't generate the same sequences on all platforms, so it's a lot better to use Random. https://codereview.webrtc.org/1606183002/diff/60001/webrtc/test/fake_network_... File webrtc/test/fake_network_pipe.cc (right): https://codereview.webrtc.org/1606183002/diff/60001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe.cc:123: config_.delay_standard_deviation_ms); git cl format https://codereview.webrtc.org/1606183002/diff/60001/webrtc/test/fake_network_... File webrtc/test/fake_network_pipe.h (right): https://codereview.webrtc.org/1606183002/diff/60001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe.h:21: #include "webrtc/base/random.h" Alphabetic order https://codereview.webrtc.org/1606183002/diff/60001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe.h:85: uint64_t seed = 1); We generally try to avoid default values. Maybe you should add it to the pipe config and give it a default value there? Or simply have two constructors.
https://codereview.webrtc.org/1606183002/diff/60001/webrtc/test/fake_network_... File webrtc/test/fake_network_pipe.h (right): https://codereview.webrtc.org/1606183002/diff/60001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe.h:21: #include "webrtc/base/random.h" On 2016/01/26 11:50:47, stefan-webrtc (holmer) wrote: > Alphabetic order Done. https://codereview.webrtc.org/1606183002/diff/60001/webrtc/test/fake_network_... webrtc/test/fake_network_pipe.h:85: uint64_t seed = 1); On 2016/01/26 11:50:47, stefan-webrtc (holmer) wrote: > We generally try to avoid default values. Maybe you should add it to the pipe > config and give it a default value there? Or simply have two constructors. Done.
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/1606183002/#ps80001 (title: "Nit and format.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1606183002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1606183002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_drmemory_light on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/buil...)
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/1606183002/#ps100001 (title: "uint8_t[] instead of uint8_t")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1606183002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1606183002/100001
Message was sent while issue was closed.
Description was changed from ========== Allow packets to be reordered in the fake network pipe. BUG= ========== to ========== Allow packets to be reordered in the fake network pipe. BUG= ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Allow packets to be reordered in the fake network pipe. BUG= ========== to ========== Allow packets to be reordered in the fake network pipe. BUG= Committed: https://crrev.com/a2c55235cae5f16715911bff6109fe915a8085bf Cr-Commit-Position: refs/heads/master@{#11384} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a2c55235cae5f16715911bff6109fe915a8085bf Cr-Commit-Position: refs/heads/master@{#11384} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
