Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2906663002:
FecControler disables FEC when *below* threshold (Closed)
|
Created:
3 years, 7 months ago by eladalon Modified:
3 years, 6 months ago CC:
webrtc-reviews_webrtc.org, AleBzk, peah-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFecControler disables FEC when *below* threshold
(This CL concerns both the PLR-based as well as the RPLR-based versions of FecController.)
1. Make FecController disable only when below the disabling-threshold, so as to prevent toggling when the enabling-curve and the disabling-curve are identical.
2. Extend unit-test coverage accordingly.
BUG=None
Review-Url: https://codereview.webrtc.org/2906663002
Cr-Commit-Position: refs/heads/master@{#18337}
Committed: https://chromium.googlesource.com/external/webrtc/+/cff9dfdc4ae6cd41d30e4d5bf5a2fe54807a6288
Patch Set 1 #
Total comments: 16
Patch Set 2 : s/0.00001/1e-5 #Patch Set 3 : What happened here? Ignore this patchset. #Patch Set 4 : 1. Replicate the change into PlrBased. 2. s/kPacketLoss/kRecoverablePacketLoss. 3. Add comments. #
Total comments: 4
Patch Set 5 : . #
Total comments: 17
Patch Set 6 : CR response #Patch Set 7 : nit #
Total comments: 3
Patch Set 8 : . #
Messages
Total messages: 28 (9 generated)
Description was changed from ========== FecControler disables FEC when *below* threshold (This CL concerns both the PLR-based as well as the RPLR-based versions of FecController.) 1. Make FecController disable only when below the disabling-threshold, so as to prevent toggling when the enabling-curve and the disabling-curve are identical. 2. Extend unit-test coverage accordingly. BUG=None ========== to ========== FecControler disables FEC when *below* threshold (This CL concerns both the PLR-based as well as the RPLR-based versions of FecController.) 1. Make FecController disable only when below the disabling-threshold, so as to prevent toggling when the enabling-curve and the disabling-curve are identical. 2. Extend unit-test coverage accordingly. BUG=None ==========
eladalon@webrtc.org changed reviewers: + minyue@webrtc.org, stefan@webrtc.org
I've only uploaded the RPLR-version. Once we've finished code-review, I'll upload the equivalent changes for PLR, you can take a final look, and then we can land.
https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc (left): https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc:132: TEST(FecControllerRplrBasedTest, OutputInitValueWhenUplinkBandwidthUnknown) { Small heads up here - text comparison is mistaken here. OutputInitValueWhenUplinkBandwidthUnknown is not replaced by OutputInitValueBeforeAnyInputsAreReceived. You might want to use an external text comparison tool that allows manual alignment. (I like BeyondCompare, btw.)
Thanks for the work! Please think about changing the Enabling logic to strictly "Above" also? Then some small comments on the UT (but I have not finished it yet.) https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.cc (right): https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.cc:72: return config_.fec_disabling_threshold.IsBelowCurve( yes, we solved -1.0f for always-on but we need 1.01f for always off now. how about changing FecEnablingDecision from !IsBeblowCurve to IsAboveCurve? https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc (left): https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc:135: // Let uplink recoverable packet loss fraction be so low that it I still hope to use "let xxx be so low such that it would cause FEC to turn off if xxx was known" type of test instead of testing all combinations without clear intention. The reason why the comment did not match here was that we added "for (bool initial_fec_enabled : {false, true})" which was not in other test. To test the case that "initial_fec_enabled = false" you could create a "let xxx be so high such that it would cause FEC to turn on if xxx was known" test case. https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc (right): https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc:45: constexpr float kEpsilon = 0.00001; 1e-5 https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc:168: for (int bandwidth : {kDisablingBandwidthLow - 1, kDisablingBandwidthLow, same here.
Answered in-line. I'll replicate the change for PLR-based now, as it seems that no major changes to this CL are upcoming. https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.cc (right): https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.cc:72: return config_.fec_disabling_threshold.IsBelowCurve( On 2017/05/26 06:30:05, minyue-webrtc wrote: > yes, we solved -1.0f for always-on but we need 1.01f for always off now. > > how about changing FecEnablingDecision from !IsBeblowCurve to IsAboveCurve? I think it's good to make sure that all possible values are explicitly treated - on, above and below. Changing to !IsBelow to IsAbove would mean that on-curve would be "no-change", which is not as clear, IMHO. https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc (left): https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc:135: // Let uplink recoverable packet loss fraction be so low that it On 2017/05/26 06:30:05, minyue-webrtc wrote: > I still hope to use "let xxx be so low such that it would cause FEC to turn off > if xxx was known" type of test instead of testing all combinations without clear > intention. > > The reason why the comment did not match here was that we added "for (bool > initial_fec_enabled : {false, true})" which was not in other test. > > To test the case that "initial_fec_enabled = false" you could create a "let xxx > be so high such that it would cause FEC to turn on if xxx was known" test case. I'm sorry if the intention was unclear. Let me explain, and perhaps you could help me make it clearer. The test, according to its name, is here to test that the init-value is outputed unmodified when the BW is unknown. I think the test should cover all (init-value, loss rate) combinations, or risk passing the test "by accident" (it might only work for the combination tested). Could you please suggest a comment that might make it even clearer? https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc (right): https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc:45: constexpr float kEpsilon = 0.00001; On 2017/05/26 06:30:05, minyue-webrtc wrote: > 1e-5 Done. https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc:168: for (int bandwidth : {kDisablingBandwidthLow - 1, kDisablingBandwidthLow, On 2017/05/26 06:30:05, minyue-webrtc wrote: > same here. Same answer, and same request for assistance in coming up with an explanatory comment. :-)
https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.cc (right): https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.cc:72: return config_.fec_disabling_threshold.IsBelowCurve( On 2017/05/30 08:07:36, eladalon wrote: > On 2017/05/26 06:30:05, minyue-webrtc wrote: > > yes, we solved -1.0f for always-on but we need 1.01f for always off now. > > > > how about changing FecEnablingDecision from !IsBeblowCurve to IsAboveCurve? > > I think it's good to make sure that all possible values are explicitly treated - > on, above and below. Changing to !IsBelow to IsAbove would mean that on-curve > would be "no-change", which is not as clear, IMHO. But isn't "no-change for the region between two curves (inclusive)" a clear action? https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc (left): https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc:135: // Let uplink recoverable packet loss fraction be so low that it On 2017/05/30 08:07:36, eladalon wrote: > On 2017/05/26 06:30:05, minyue-webrtc wrote: > > I still hope to use "let xxx be so low such that it would cause FEC to turn > off > > if xxx was known" type of test instead of testing all combinations without > clear > > intention. > > > > The reason why the comment did not match here was that we added "for (bool > > initial_fec_enabled : {false, true})" which was not in other test. > > > > To test the case that "initial_fec_enabled = false" you could create a "let > xxx > > be so high such that it would cause FEC to turn on if xxx was known" test > case. > > I'm sorry if the intention was unclear. Let me explain, and perhaps you could > help me make it clearer. The test, according to its name, is here to test that > the init-value is outputed unmodified when the BW is unknown. I think the test > should cover all (init-value, loss rate) combinations, or risk passing the test > "by accident" (it might only work for the combination tested). Could you please > suggest a comment that might make it even clearer? how about just do two tests: auto controller = CreateFecControllerRplrBased(true); // Let uplink recoverable packet loss fraction be so low that it // would cause FEC to turn off if uplink bandwidth was known. UpdateNetworkMetrics( controller.get(), rtc::Optional<int>(), rtc::Optional<float>(kDisablingRecoverablePacketLossAtHighBw - kEpsilon)); CheckDecision(controller.get(), true, kDisablingRecoverablePacketLossAtHighBw); controller = CreateFecControllerRplrBased(false); // Let uplink recoverable packet loss fraction be so high that it // would cause FEC to turn on if uplink bandwidth was known. UpdateNetworkMetrics( controller.get(), rtc::Optional<int>(), rtc::Optional<float>(kEnablingRecoverablePacketLossAtHighBw + kEpsilon)); // +kEpsilon if you agree to change the enabling logic. CheckDecision(controller.get(), initial_fec_enabled, kDisablingRecoverablePacketLossAtHighBw);
https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.cc (right): https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.cc:72: return config_.fec_disabling_threshold.IsBelowCurve( On 2017/05/30 08:30:32, minyue-webrtc wrote: > On 2017/05/30 08:07:36, eladalon wrote: > > On 2017/05/26 06:30:05, minyue-webrtc wrote: > > > yes, we solved -1.0f for always-on but we need 1.01f for always off now. > > > > > > how about changing FecEnablingDecision from !IsBeblowCurve to IsAboveCurve? > > > > I think it's good to make sure that all possible values are explicitly treated > - > > on, above and below. Changing to !IsBelow to IsAbove would mean that on-curve > > would be "no-change", which is not as clear, IMHO. > > But isn't "no-change for the region between two curves (inclusive)" a clear > action? Yes, but I think it's clearer to have a stateless mapping from value to action, whereas any no-change region makes everything stateful. Wdyt? https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc (left): https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc:135: // Let uplink recoverable packet loss fraction be so low that it On 2017/05/30 08:30:32, minyue-webrtc wrote: > On 2017/05/30 08:07:36, eladalon wrote: > > On 2017/05/26 06:30:05, minyue-webrtc wrote: > > > I still hope to use "let xxx be so low such that it would cause FEC to turn > > off > > > if xxx was known" type of test instead of testing all combinations without > > clear > > > intention. > > > > > > The reason why the comment did not match here was that we added "for (bool > > > initial_fec_enabled : {false, true})" which was not in other test. > > > > > > To test the case that "initial_fec_enabled = false" you could create a "let > > xxx > > > be so high such that it would cause FEC to turn on if xxx was known" test > > case. > > > > I'm sorry if the intention was unclear. Let me explain, and perhaps you could > > help me make it clearer. The test, according to its name, is here to test that > > the init-value is outputed unmodified when the BW is unknown. I think the test > > should cover all (init-value, loss rate) combinations, or risk passing the > test > > "by accident" (it might only work for the combination tested). Could you > please > > suggest a comment that might make it even clearer? > > how about just do two tests: > > auto controller = CreateFecControllerRplrBased(true); > // Let uplink recoverable packet loss fraction be so low that it > // would cause FEC to turn off if uplink bandwidth was known. > UpdateNetworkMetrics( > controller.get(), rtc::Optional<int>(), > rtc::Optional<float>(kDisablingRecoverablePacketLossAtHighBw - > kEpsilon)); > CheckDecision(controller.get(), true, > kDisablingRecoverablePacketLossAtHighBw); > > controller = CreateFecControllerRplrBased(false); > // Let uplink recoverable packet loss fraction be so high that it > // would cause FEC to turn on if uplink bandwidth was known. > UpdateNetworkMetrics( > controller.get(), rtc::Optional<int>(), > rtc::Optional<float>(kEnablingRecoverablePacketLossAtHighBw + > kEpsilon)); // +kEpsilon if you agree to change the enabling logic. > CheckDecision(controller.get(), initial_fec_enabled, > kDisablingRecoverablePacketLossAtHighBw); nit: You probably mean CheckDecision(controller.get(), true, kDisablingRecoverablePacketLossAtHighBw - kEpsilon); I'll respond under the assumption that that's what you'd meant. 1. Wouldn't we then have the suspicion that behavior might be wrong for on-curve behavior? 2. IMHO, clarity is lost by unrolling the loop; one needs to read carefully to see if there are any other changes there.
https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.cc (right): https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.cc:72: return config_.fec_disabling_threshold.IsBelowCurve( On 2017/05/30 09:23:54, eladalon wrote: > On 2017/05/30 08:30:32, minyue-webrtc wrote: > > On 2017/05/30 08:07:36, eladalon wrote: > > > On 2017/05/26 06:30:05, minyue-webrtc wrote: > > > > yes, we solved -1.0f for always-on but we need 1.01f for always off now. > > > > > > > > how about changing FecEnablingDecision from !IsBeblowCurve to > IsAboveCurve? > > > > > > I think it's good to make sure that all possible values are explicitly > treated > > - > > > on, above and below. Changing to !IsBelow to IsAbove would mean that > on-curve > > > would be "no-change", which is not as clear, IMHO. > > > > But isn't "no-change for the region between two curves (inclusive)" a clear > > action? > > Yes, but I think it's clearer to have a stateless mapping from value to action, > whereas any no-change region makes everything stateful. Wdyt? Isn't the margin between two-curves designed to make it stateful, thereas we can avoid toggling. https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc (left): https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc:135: // Let uplink recoverable packet loss fraction be so low that it On 2017/05/30 09:23:54, eladalon wrote: > On 2017/05/30 08:30:32, minyue-webrtc wrote: > > On 2017/05/30 08:07:36, eladalon wrote: > > > On 2017/05/26 06:30:05, minyue-webrtc wrote: > > > > I still hope to use "let xxx be so low such that it would cause FEC to > turn > > > off > > > > if xxx was known" type of test instead of testing all combinations without > > > clear > > > > intention. > > > > > > > > The reason why the comment did not match here was that we added "for (bool > > > > initial_fec_enabled : {false, true})" which was not in other test. > > > > > > > > To test the case that "initial_fec_enabled = false" you could create a > "let > > > xxx > > > > be so high such that it would cause FEC to turn on if xxx was known" test > > > case. > > > > > > I'm sorry if the intention was unclear. Let me explain, and perhaps you > could > > > help me make it clearer. The test, according to its name, is here to test > that > > > the init-value is outputed unmodified when the BW is unknown. I think the > test > > > should cover all (init-value, loss rate) combinations, or risk passing the > > test > > > "by accident" (it might only work for the combination tested). Could you > > please > > > suggest a comment that might make it even clearer? > > > > how about just do two tests: > > > > auto controller = CreateFecControllerRplrBased(true); > > // Let uplink recoverable packet loss fraction be so low that it > > // would cause FEC to turn off if uplink bandwidth was known. > > UpdateNetworkMetrics( > > controller.get(), rtc::Optional<int>(), > > rtc::Optional<float>(kDisablingRecoverablePacketLossAtHighBw - > > kEpsilon)); > > CheckDecision(controller.get(), true, > > kDisablingRecoverablePacketLossAtHighBw); > > > > controller = CreateFecControllerRplrBased(false); > > // Let uplink recoverable packet loss fraction be so high that it > > // would cause FEC to turn on if uplink bandwidth was known. > > UpdateNetworkMetrics( > > controller.get(), rtc::Optional<int>(), > > rtc::Optional<float>(kEnablingRecoverablePacketLossAtHighBw + > > kEpsilon)); // +kEpsilon if you agree to change the enabling logic. > > CheckDecision(controller.get(), initial_fec_enabled, > > kDisablingRecoverablePacketLossAtHighBw); > > nit: You probably mean > CheckDecision(controller.get(), true, > kDisablingRecoverablePacketLossAtHighBw - kEpsilon); > I'll respond under the assumption that that's what you'd meant. > > 1. Wouldn't we then have the suspicion that behavior might be wrong for on-curve > behavior? > 2. IMHO, clarity is lost by unrolling the loop; one needs to read carefully to > see if there are any other changes there. Ah, I think I meant CheckDecision(controller.get(), false, kEnablingRecoverablePacketLossAtHighBw); on the last line :) https://codereview.webrtc.org/2906663002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc:135: // Let uplink recoverable packet loss fraction be so low that it On 2017/05/30 09:23:54, eladalon wrote: > On 2017/05/30 08:30:32, minyue-webrtc wrote: > > On 2017/05/30 08:07:36, eladalon wrote: > > > On 2017/05/26 06:30:05, minyue-webrtc wrote: > > > > I still hope to use "let xxx be so low such that it would cause FEC to > turn > > > off > > > > if xxx was known" type of test instead of testing all combinations without > > > clear > > > > intention. > > > > > > > > The reason why the comment did not match here was that we added "for (bool > > > > initial_fec_enabled : {false, true})" which was not in other test. > > > > > > > > To test the case that "initial_fec_enabled = false" you could create a > "let > > > xxx > > > > be so high such that it would cause FEC to turn on if xxx was known" test > > > case. > > > > > > I'm sorry if the intention was unclear. Let me explain, and perhaps you > could > > > help me make it clearer. The test, according to its name, is here to test > that > > > the init-value is outputed unmodified when the BW is unknown. I think the > test > > > should cover all (init-value, loss rate) combinations, or risk passing the > > test > > > "by accident" (it might only work for the combination tested). Could you > > please > > > suggest a comment that might make it even clearer? > > > > how about just do two tests: > > > > auto controller = CreateFecControllerRplrBased(true); > > // Let uplink recoverable packet loss fraction be so low that it > > // would cause FEC to turn off if uplink bandwidth was known. > > UpdateNetworkMetrics( > > controller.get(), rtc::Optional<int>(), > > rtc::Optional<float>(kDisablingRecoverablePacketLossAtHighBw - > > kEpsilon)); > > CheckDecision(controller.get(), true, > > kDisablingRecoverablePacketLossAtHighBw); > > > > controller = CreateFecControllerRplrBased(false); > > // Let uplink recoverable packet loss fraction be so high that it > > // would cause FEC to turn on if uplink bandwidth was known. > > UpdateNetworkMetrics( > > controller.get(), rtc::Optional<int>(), > > rtc::Optional<float>(kEnablingRecoverablePacketLossAtHighBw + > > kEpsilon)); // +kEpsilon if you agree to change the enabling logic. > > CheckDecision(controller.get(), initial_fec_enabled, > > kDisablingRecoverablePacketLossAtHighBw); > > nit: You probably mean > CheckDecision(controller.get(), true, > kDisablingRecoverablePacketLossAtHighBw - kEpsilon); > I'll respond under the assumption that that's what you'd meant. > > 1. Wouldn't we then have the suspicion that behavior might be wrong for on-curve > behavior? On-curve behavior is not what this test focuses on, therefore, I want to exclude cases that are not sensitive to init value. > 2. IMHO, clarity is lost by unrolling the loop; one needs to read carefully to > see if there are any other changes there. The code is simple enough to follow, so it should be fine. If it had been more complicated, we could write two tests.
For posterity - the discussion was continued offline, and we've decided to keep the enabling behavior as is. Two of the points for this were: 1. Initial FEC state is not configurable by the ANA protobuf string. In the always-on case, we'd get the initial state so long as the R/PLR is 0%. 2. FEC would otherwise not be enable-able for the minimal BWE, if the enable-curve is defined using a point on the minimum BWE, which is likely.
https://codereview.webrtc.org/2906663002/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based_unittest.cc (right): https://codereview.webrtc.org/2906663002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based_unittest.cc:256: // Note: Disabling happens when the value is *below* the disable-threshold. *below* -> strictly below https://codereview.webrtc.org/2906663002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based_unittest.cc:453: // Test that FEC is turned on whenever we're on the curve or about it "on the curve or about" -> "on or above"
https://codereview.webrtc.org/2906663002/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based_unittest.cc (right): https://codereview.webrtc.org/2906663002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based_unittest.cc:256: // Note: Disabling happens when the value is *below* the disable-threshold. On 2017/05/30 12:30:11, minyue-webrtc wrote: > *below* -> strictly below Done. https://codereview.webrtc.org/2906663002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based_unittest.cc:453: // Test that FEC is turned on whenever we're on the curve or about it On 2017/05/30 12:30:10, minyue-webrtc wrote: > "on the curve or about" -> "on or above" Thanks. :-)
Good work. Only some cosmetic suggestions. https://codereview.webrtc.org/2906663002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based_unittest.cc (right): https://codereview.webrtc.org/2906663002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based_unittest.cc:134: // No matter the initial FEC state, no matter the packet-loss rate, Regardless of the initial FEC state and the packet-loss rate, ... https://codereview.webrtc.org/2906663002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based_unittest.cc:152: // No matter the initial FEC state, no matter the BWE, the initial FEC state same here https://codereview.webrtc.org/2906663002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based_unittest.cc:388: // kPacketLossAthighBw as (negative) integer powers of 2. I do not quite understand this comment. What would it be if it is not 2^-N? https://codereview.webrtc.org/2906663002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based_unittest.cc:454: // curve, independent of the starting FEC state. delete "curve " https://codereview.webrtc.org/2906663002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc (right): https://codereview.webrtc.org/2906663002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc:151: // No matter the initial FEC state, no matter the recoverable-packet-loss No matter -> Regardless of https://codereview.webrtc.org/2906663002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc:172: // No matter the initial FEC state, no matter the BWE, the initial FEC state same https://codereview.webrtc.org/2906663002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc:301: kDisablingRecoverablePacketLossAtHighBw) / try to reformat https://codereview.webrtc.org/2906663002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc:438: // kRecoverablePacketLossAthighBw as (negative) integer powers of 2. I do not follow this comment https://codereview.webrtc.org/2906663002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc:507: // Test that FEC is turned on whenever we're on the curve or above it curve, remove last "curve" https://codereview.webrtc.org/2906663002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc:565: } // namespace webrtc I think an empty line above "// namespace webrtc" is preferred
https://codereview.webrtc.org/2906663002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based_unittest.cc (right): https://codereview.webrtc.org/2906663002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based_unittest.cc:134: // No matter the initial FEC state, no matter the packet-loss rate, On 2017/05/30 14:07:04, minyue-webrtc wrote: > Regardless of the initial FEC state and the packet-loss rate, ... Done. https://codereview.webrtc.org/2906663002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based_unittest.cc:388: // kPacketLossAthighBw as (negative) integer powers of 2. On 2017/05/30 14:07:04, minyue-webrtc wrote: > I do not quite understand this comment. What would it be if it is not 2^-N? Numerical errors could make it slightly below the curve instead of exactly on it. https://codereview.webrtc.org/2906663002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based_unittest.cc:454: // curve, independent of the starting FEC state. On 2017/05/30 14:07:04, minyue-webrtc wrote: > delete "curve " Thanks for catching. :-) https://codereview.webrtc.org/2906663002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc (right): https://codereview.webrtc.org/2906663002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc:301: kDisablingRecoverablePacketLossAtHighBw) / On 2017/05/30 14:07:05, minyue-webrtc wrote: > try to reformat git-cl-format is a harsh mistress. Suggestions? https://codereview.webrtc.org/2906663002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc:438: // kRecoverablePacketLossAthighBw as (negative) integer powers of 2. On 2017/05/30 14:07:05, minyue-webrtc wrote: > I do not follow this comment Explained on other file. https://codereview.webrtc.org/2906663002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc:565: } // namespace webrtc On 2017/05/30 14:07:05, minyue-webrtc wrote: > I think an empty line above "// namespace webrtc" is preferred I prefer that too. I see I have accidentally removed it(*), then mistakenly thought it was done by git-cl-format. I'll restore that line. (*) - I had "#if 0" around some of the tests, and terminated it here, then removed the #endif part here.
lgtm + nit https://codereview.webrtc.org/2906663002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc (right): https://codereview.webrtc.org/2906663002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc:301: kDisablingRecoverablePacketLossAtHighBw) / On 2017/05/30 14:37:39, eladalon wrote: > On 2017/05/30 14:07:05, minyue-webrtc wrote: > > try to reformat > > git-cl-format is a harsh mistress. Suggestions? do it manually.
The CQ bit was checked by eladalon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2906663002/#ps120001 (title: "nit")
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: win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/21704)
https://codereview.webrtc.org/2906663002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based_unittest.cc (right): https://codereview.webrtc.org/2906663002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based_unittest.cc:48: constexpr float kEpsilon = 1e-5; add suffix f https://codereview.webrtc.org/2906663002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc (right): https://codereview.webrtc.org/2906663002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based_unittest.cc:45: constexpr float kEpsilon = 1e-5; please add suffix f
https://codereview.webrtc.org/2906663002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based_unittest.cc (right): https://codereview.webrtc.org/2906663002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based_unittest.cc:48: constexpr float kEpsilon = 1e-5; On 2017/05/30 19:39:25, minyue-webrtc wrote: > add suffix f I know. :-)
The CQ bit was checked by eladalon@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2906663002/#ps140001 (title: ".")
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": 140001, "attempt_start_ts": 1496177588249710,
"parent_rev": "7ec573aeff00e46ae2498d9f1ebfa5096515ae54", "commit_rev":
"cff9dfdc4ae6cd41d30e4d5bf5a2fe54807a6288"}
Message was sent while issue was closed.
Description was changed from ========== FecControler disables FEC when *below* threshold (This CL concerns both the PLR-based as well as the RPLR-based versions of FecController.) 1. Make FecController disable only when below the disabling-threshold, so as to prevent toggling when the enabling-curve and the disabling-curve are identical. 2. Extend unit-test coverage accordingly. BUG=None ========== to ========== FecControler disables FEC when *below* threshold (This CL concerns both the PLR-based as well as the RPLR-based versions of FecController.) 1. Make FecController disable only when below the disabling-threshold, so as to prevent toggling when the enabling-curve and the disabling-curve are identical. 2. Extend unit-test coverage accordingly. BUG=None Review-Url: https://codereview.webrtc.org/2906663002 Cr-Commit-Position: refs/heads/master@{#18337} Committed: https://chromium.googlesource.com/external/webrtc/+/cff9dfdc4ae6cd41d30e4d5bf... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/external/webrtc/+/cff9dfdc4ae6cd41d30e4d5bf... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
