Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2895083002:
Add FlexfecReceiver unit test for infinite recovery loop. (Closed)
DescriptionAdd FlexfecReceiver unit test for infinite recovery loop.
This CL adds unit tests to the FlexfecReceiver, verifying that the
infinite recovery loop described in
https://codereview.webrtc.org/2867943003/ is tested for.
BUG=webrtc:5654
Review-Url: https://codereview.webrtc.org/2895083002
Cr-Commit-Position: refs/heads/master@{#18240}
Committed: https://chromium.googlesource.com/external/webrtc/+/f27c5b8d6eed929eae066c417856eb379779f0c0
Patch Set 1 #
Total comments: 7
Patch Set 2 : nisse comments 1. #
Total comments: 6
Patch Set 3 : danilchap comments 1. #Patch Set 4 : Revert "danilchap comments 1." #Messages
Total messages: 24 (9 generated)
brandtr@webrtc.org changed reviewers: + danilchap@webrtc.org
Please take a look :)
nisse@webrtc.org changed reviewers: + nisse@webrtc.org
https://codereview.webrtc.org/2895083002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc (right): https://codereview.webrtc.org/2895083002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:523: // No extra callback, and thus no infinite loop. If this test fails (by undoing the fix in the other cl), it will fail by running out of stack space and crash, right? Would it make sense to add some recursion counter, and fail with a more user-friendly message?
https://codereview.webrtc.org/2895083002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc (right): https://codereview.webrtc.org/2895083002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:523: // No extra callback, and thus no infinite loop. On 2017/05/22 08:39:20, nisse-webrtc wrote: > If this test fails (by undoing the fix in the other cl), it will fail by running > out of stack space and crash, right? Would it make sense to add some recursion > counter, and fail with a more user-friendly message? Yes, I this test crashes by undoing your change. Is there a way to throw a nice error message here without adding a recursion counter inside the FlexfecReceiver? If not, I'm not sure if it is worth it? The logic to check that duplicate packets are not accepted and that recovered packets are only sent to the callback once already exists in ForwardErrorCorrection and FlexfecReceiver, respectively. (The latter was fixed by you.)
https://codereview.webrtc.org/2895083002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc (right): https://codereview.webrtc.org/2895083002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:523: // No extra callback, and thus no infinite loop. On 2017/05/22 09:10:09, brandtr wrote: > On 2017/05/22 08:39:20, nisse-webrtc wrote: > > If this test fails (by undoing the fix in the other cl), it will fail by > running > > out of stack space and crash, right? Would it make sense to add some recursion > > counter, and fail with a more user-friendly message? > > Yes, I this test crashes by undoing your change. > > Is there a way to throw a nice error message here without adding a recursion > counter inside the FlexfecReceiver? If not, I'm not sure if it is worth it? > > The logic to check that duplicate packets are not accepted and that recovered > packets are only sent to the callback once already exists in > ForwardErrorCorrection and FlexfecReceiver, respectively. (The latter was fixed > by you.) did you try EXPECT_DEATH ? (not sure it would in this case)
https://codereview.webrtc.org/2895083002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc (right): https://codereview.webrtc.org/2895083002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:523: // No extra callback, and thus no infinite loop. On 2017/05/22 09:17:28, danilchap wrote: > On 2017/05/22 09:10:09, brandtr wrote: > > On 2017/05/22 08:39:20, nisse-webrtc wrote: > > > If this test fails (by undoing the fix in the other cl), it will fail by > > running > > > out of stack space and crash, right? Would it make sense to add some > recursion > > > counter, and fail with a more user-friendly message? > > > > Yes, I this test crashes by undoing your change. > > > > Is there a way to throw a nice error message here without adding a recursion > > counter inside the FlexfecReceiver? If not, I'm not sure if it is worth it? > > > > The logic to check that duplicate packets are not accepted and that recovered > > packets are only sent to the callback once already exists in > > ForwardErrorCorrection and FlexfecReceiver, respectively. (The latter was > fixed > > by you.) > > did you try EXPECT_DEATH ? (not sure it would in this case) nvm... it is for opposite case.
https://codereview.webrtc.org/2895083002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc (right): https://codereview.webrtc.org/2895083002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:487: void OnRecoveredPacket(const uint8_t* packet, size_t length) { Can you add the recursion counter here? Something like if (count_ > 10) { deep_recursion_ = true; return; } ++count_; receiver_->OnRtpPacket(parsed_packet); --count; together with an EXPECT_FALSE(loopback_recovered_packet_receiver.deep_recursion_); at the end of the test. https://codereview.webrtc.org/2895083002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:523: // No extra callback, and thus no infinite loop. On 2017/05/22 09:10:09, brandtr wrote: > Is there a way to throw a nice error message here without adding a recursion > counter inside the FlexfecReceiver? If not, I'm not sure if it is worth it? I agree it's worth doing only if it can be done by some hacking in the test code only.
https://codereview.webrtc.org/2895083002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc (right): https://codereview.webrtc.org/2895083002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:487: void OnRecoveredPacket(const uint8_t* packet, size_t length) { On 2017/05/22 10:39:05, nisse-webrtc wrote: > Can you add the recursion counter here? Something like > > if (count_ > 10) { > deep_recursion_ = true; > return; > } > ++count_; > receiver_->OnRtpPacket(parsed_packet); > --count; > > together with an > > EXPECT_FALSE(loopback_recovered_packet_receiver.deep_recursion_); > > at the end of the test. Yes, that makes a lot of sense! I forgot that I had access to the recursion chain here... :)
On 2017/05/22 11:52:39, brandtr wrote: > https://codereview.webrtc.org/2895083002/diff/1/webrtc/modules/rtp_rtcp/sourc... > File webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc (right): > > https://codereview.webrtc.org/2895083002/diff/1/webrtc/modules/rtp_rtcp/sourc... > webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:487: void > OnRecoveredPacket(const uint8_t* packet, size_t length) { > On 2017/05/22 10:39:05, nisse-webrtc wrote: > > Can you add the recursion counter here? Something like > > > > if (count_ > 10) { > > deep_recursion_ = true; > > return; > > } > > ++count_; > > receiver_->OnRtpPacket(parsed_packet); > > --count; > > > > together with an > > > > EXPECT_FALSE(loopback_recovered_packet_receiver.deep_recursion_); > > > > at the end of the test. > > Yes, that makes a lot of sense! I forgot that I had access to the recursion > chain here... :) Any more thoughts? :)
lgtm if lg to nisse https://codereview.webrtc.org/2895083002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc (right): https://codereview.webrtc.org/2895083002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:313: media_it++; nit: ++media_it; https://codereview.webrtc.org/2895083002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:498: if (recursion_depth_ > kMaxRecursionDepth) { alternative approach: ASSERT_LT(recursion_depth_, kMaxRecursionDepth); that will return too prevening stack overflow, but will point to this line and save from extra member and accessor
https://codereview.webrtc.org/2895083002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc (right): https://codereview.webrtc.org/2895083002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:313: media_it++; On 2017/05/23 13:07:15, danilchap wrote: > nit: ++media_it; The style of this file is the other way :/ https://codereview.webrtc.org/2895083002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:498: if (recursion_depth_ > kMaxRecursionDepth) { On 2017/05/23 13:07:14, danilchap wrote: > alternative approach: > ASSERT_LT(recursion_depth_, kMaxRecursionDepth); > that will return too prevening stack overflow, but will point to this line and > save from extra member and accessor Good idea! Done.
lgtm, if you either undo the use of ASSERT in OnRecoveredPacket, or add a comment on why it works in this particular case. https://codereview.webrtc.org/2895083002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc (right): https://codereview.webrtc.org/2895083002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:498: if (recursion_depth_ > kMaxRecursionDepth) { On 2017/05/23 13:19:17, brandtr wrote: > On 2017/05/23 13:07:14, danilchap wrote: > > alternative approach: > > ASSERT_LT(recursion_depth_, kMaxRecursionDepth); > > that will return too prevening stack overflow, but will point to this line and > > save from extra member and accessor > > Good idea! Done. ASSERT_* does some magic which works as intended only when used in the top-level test function. It might work, but in a unexpected way, because (1) ASSERT_* is more or less is the same as EXPECT_* + a return statement on failure, and (2) the return type of this function happens to be void. I'm not sure it's a good idea to rely on that. If you really want to, add a comment to not mislead readers to think that ASSERT_* outside of top-level works in general; there's already too much confusion around this.
https://codereview.webrtc.org/2895083002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc (right): https://codereview.webrtc.org/2895083002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:498: if (recursion_depth_ > kMaxRecursionDepth) { On 2017/05/23 14:05:25, nisse-webrtc wrote: > On 2017/05/23 13:19:17, brandtr wrote: > > On 2017/05/23 13:07:14, danilchap wrote: > > > alternative approach: > > > ASSERT_LT(recursion_depth_, kMaxRecursionDepth); > > > that will return too prevening stack overflow, but will point to this line > and > > > save from extra member and accessor > > > > Good idea! Done. > > ASSERT_* does some magic which works as intended only when used in the top-level > test function. > > It might work, but in a unexpected way, because (1) ASSERT_* is more or less is > the same as EXPECT_* + a return statement on failure, and (2) the return type of > this function happens to be void. I'm not sure it's a good idea to rely on that. > If you really want to, add a comment to not mislead readers to think that > ASSERT_* outside of top-level works in general; there's already too much > confusion around this. Better avoid confusion and use the old approach then. Do ASSERTs only work at the top-level because the tests have return type void?
The CQ bit was checked by brandtr@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/...
On 2017/05/23 15:02:47, brandtr wrote: > https://codereview.webrtc.org/2895083002/diff/20001/webrtc/modules/rtp_rtcp/s... > File webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc (right): > > https://codereview.webrtc.org/2895083002/diff/20001/webrtc/modules/rtp_rtcp/s... > webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc:498: if > (recursion_depth_ > kMaxRecursionDepth) { > On 2017/05/23 14:05:25, nisse-webrtc wrote: > > On 2017/05/23 13:19:17, brandtr wrote: > > > On 2017/05/23 13:07:14, danilchap wrote: > > > > alternative approach: > > > > ASSERT_LT(recursion_depth_, kMaxRecursionDepth); > > > > that will return too prevening stack overflow, but will point to this line > > and > > > > save from extra member and accessor > > > > > > Good idea! Done. > > > > ASSERT_* does some magic which works as intended only when used in the > top-level > > test function. > > > > It might work, but in a unexpected way, because (1) ASSERT_* is more or less > is > > the same as EXPECT_* + a return statement on failure, and (2) the return type > of > > this function happens to be void. I'm not sure it's a good idea to rely on > that. > > If you really want to, add a comment to not mislead readers to think that > > ASSERT_* outside of top-level works in general; there's already too much > > confusion around this. > > Better avoid confusion and use the old approach then. > > Do ASSERTs only work at the top-level because the tests have return type void? yes, top-level return void. Assertion in other functions should be used with care. Better not to if unsure. https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...
The CQ bit was unchecked by brandtr@webrtc.org
The CQ bit was checked by brandtr@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org, nisse@webrtc.org Link to the patchset: https://codereview.webrtc.org/2895083002/#ps60001 (title: "Revert "danilchap comments 1."")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495552634119530,
"parent_rev": "cc8b14b1ec418979393f101df648a876cc8ecaf8", "commit_rev":
"f27c5b8d6eed929eae066c417856eb379779f0c0"}
Message was sent while issue was closed.
Description was changed from ========== Add FlexfecReceiver unit test for infinite recovery loop. This CL adds unit tests to the FlexfecReceiver, verifying that the infinite recovery loop described in https://codereview.webrtc.org/2867943003/ is tested for. BUG=webrtc:5654 ========== to ========== Add FlexfecReceiver unit test for infinite recovery loop. This CL adds unit tests to the FlexfecReceiver, verifying that the infinite recovery loop described in https://codereview.webrtc.org/2867943003/ is tested for. BUG=webrtc:5654 Review-Url: https://codereview.webrtc.org/2895083002 Cr-Commit-Position: refs/heads/master@{#18240} Committed: https://chromium.googlesource.com/external/webrtc/+/f27c5b8d6eed929eae066c417... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/f27c5b8d6eed929eae066c417... |
