Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 10 months ago by elad.alon Modified:
3 years, 8 months ago Reviewers:
minyue-webrtc, sprang_webrtc, stefan-webrtc, kwiberg-webrtc, michaelt CC:
webrtc-reviews_webrtc.org, kwiberg-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAvoid code duplication between PLR/RPLR-based FecController
BUG=webrtc:7058
Review-Url: https://codereview.webrtc.org/2688613003
Cr-Commit-Position: refs/heads/master@{#17437}
Committed: https://chromium.googlesource.com/external/webrtc/+/326263a678cc84ce08d141d7e507c0282fa61200
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : Code rebased. #Patch Set 4 : nit #
Total comments: 4
Patch Set 5 : Ignore (bad rebase) #Patch Set 6 : Rebase #Patch Set 7 : . #
Total comments: 2
Patch Set 8 : s/point/curve in one comment. #Patch Set 9 : s/place/space #Patch Set 10 : . #
Total comments: 1
Patch Set 11 : . #
Total comments: 42
Patch Set 12 : Add horizontal threshold check. #
Total comments: 1
Patch Set 13 : . #Patch Set 14 : . #
Total comments: 1
Patch Set 15 : P.S: Additional constraints on UT values. #Patch Set 16 : . #Patch Set 17 : Use static function in place of ternary expression. #
Total comments: 53
Patch Set 18 : . #Patch Set 19 : Rebased #Patch Set 20 : Narrowing conversion #
Total comments: 6
Patch Set 21 : . #
Created: 3 years, 8 months ago
Messages
Total messages: 44 (15 generated)
Description was changed from ========== Avoid code duplication between PLR/RPLR-based FecController BUG= ========== to ========== Avoid code duplication between PLR/RPLR-based FecController BUG=webrtc:7058 ==========
elad.alon@webrtc.org changed reviewers: + michaelt@webrtc.org, minyue@webrtc.org, sprang@webrtc.org, stefan@webrtc.org
Ready for review.
https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h (right): https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:20: struct ThresholdCurve { I think this can be made a class https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:38: float ValueAt(int x) const { I think more useful API can be enum Position { ABOVE, ON, BELOW; } Position PositionOfPoint(int x, float y);
https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h (right): https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:20: struct ThresholdCurve { On 2017/03/22 11:35:07, minyue-webrtc wrote: > I think this can be made a class Will do. I'll group this with other changes. https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:38: float ValueAt(int x) const { On 2017/03/22 11:35:07, minyue-webrtc wrote: > I think more useful API can be > > enum Position { > ABOVE, ON, BELOW; > } > > Position PositionOfPoint(int x, float y); > > Okay. Thoughts: 1. Parameterizing seems to be overkill, but I'd like to turn the X axis into float, too. Agreed? 2. Do we want to give any special attention to the case that Y assume infinity, but x < a_x? In that case, we'd normally be giving BELOW, but that might be confusing to a caller of the function who might have passed infinity in as a way of securing an ABOVE answer. In my mind, this is only theoretical; we should just return BELOW. Looking for confirmation.
On 2017/03/22 12:11:20, elad.alon wrote: > https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... > File webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h > (right): > > https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:20: > struct ThresholdCurve { > On 2017/03/22 11:35:07, minyue-webrtc wrote: > > I think this can be made a class > > Will do. I'll group this with other changes. > > https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:38: > float ValueAt(int x) const { > On 2017/03/22 11:35:07, minyue-webrtc wrote: > > I think more useful API can be > > > > enum Position { > > ABOVE, ON, BELOW; > > } > > > > Position PositionOfPoint(int x, float y); > > > > > > Okay. > Thoughts: > 1. Parameterizing seems to be overkill, but I'd like to turn the X axis into > float, too. Agreed? yes, sgtm > 2. Do we want to give any special attention to the case that Y assume infinity, > but x < a_x? In that case, we'd normally be giving BELOW, but that might be > confusing to a caller of the function who might have passed infinity in as a way > of securing an ABOVE answer. In my mind, this is only theoretical; we should > just return BELOW. Looking for confirmation. let's follow the math, then it is BELOW. That is least ambiguous, I think.
On 2017/03/22 12:20:38, minyue-webrtc wrote: > On 2017/03/22 12:11:20, elad.alon wrote: > > > https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... > > File webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h > > (right): > > > > > https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... > > webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:20: > > struct ThresholdCurve { > > On 2017/03/22 11:35:07, minyue-webrtc wrote: > > > I think this can be made a class > > > > Will do. I'll group this with other changes. > > > > > https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... > > webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:38: > > float ValueAt(int x) const { > > On 2017/03/22 11:35:07, minyue-webrtc wrote: > > > I think more useful API can be > > > > > > enum Position { > > > ABOVE, ON, BELOW; > > > } > > > > > > Position PositionOfPoint(int x, float y); > > > > > > > > > > Okay. > > Thoughts: > > 1. Parameterizing seems to be overkill, but I'd like to turn the X axis into > > float, too. Agreed? > yes, sgtm > > > 2. Do we want to give any special attention to the case that Y assume > infinity, > > but x < a_x? In that case, we'd normally be giving BELOW, but that might be > > confusing to a caller of the function who might have passed infinity in as a > way > > of securing an ABOVE answer. In my mind, this is only theoretical; we should > > just return BELOW. Looking for confirmation. > let's follow the math, then it is BELOW. That is least ambiguous, I think. Another thought, since it is a threshold curve, think of below/above/on the threshold. Then "position" may not be a good word. How about bool IsAboveThreshold(float x, float y)
On 2017/03/22 12:27:43, minyue-webrtc wrote: > On 2017/03/22 12:20:38, minyue-webrtc wrote: > > On 2017/03/22 12:11:20, elad.alon wrote: > > > > > > https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... > > > File > webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h > > > (right): > > > > > > > > > https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... > > > webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:20: > > > struct ThresholdCurve { > > > On 2017/03/22 11:35:07, minyue-webrtc wrote: > > > > I think this can be made a class > > > > > > Will do. I'll group this with other changes. > > > > > > > > > https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... > > > webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:38: > > > float ValueAt(int x) const { > > > On 2017/03/22 11:35:07, minyue-webrtc wrote: > > > > I think more useful API can be > > > > > > > > enum Position { > > > > ABOVE, ON, BELOW; > > > > } > > > > > > > > Position PositionOfPoint(int x, float y); > > > > > > > > > > > > > > Okay. > > > Thoughts: > > > 1. Parameterizing seems to be overkill, but I'd like to turn the X axis into > > > float, too. Agreed? > > yes, sgtm > > > > > 2. Do we want to give any special attention to the case that Y assume > > infinity, > > > but x < a_x? In that case, we'd normally be giving BELOW, but that might be > > > confusing to a caller of the function who might have passed infinity in as a > > way > > > of securing an ABOVE answer. In my mind, this is only theoretical; we should > > > just return BELOW. Looking for confirmation. > > let's follow the math, then it is BELOW. That is least ambiguous, I think. > > Another thought, since it is a threshold curve, think of below/above/on the > threshold. Then "position" may not be a good word. How about > > bool IsAboveThreshold(float x, float y) RelativePosition(float x, float y) ?
On 2017/03/22 14:36:44, elad.alon wrote: > On 2017/03/22 12:27:43, minyue-webrtc wrote: > > On 2017/03/22 12:20:38, minyue-webrtc wrote: > > > On 2017/03/22 12:11:20, elad.alon wrote: > > > > > > > > > > https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... > > > > File > > webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h > > > > (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... > > > > > webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:20: > > > > struct ThresholdCurve { > > > > On 2017/03/22 11:35:07, minyue-webrtc wrote: > > > > > I think this can be made a class > > > > > > > > Will do. I'll group this with other changes. > > > > > > > > > > > > > > https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... > > > > > webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:38: > > > > float ValueAt(int x) const { > > > > On 2017/03/22 11:35:07, minyue-webrtc wrote: > > > > > I think more useful API can be > > > > > > > > > > enum Position { > > > > > ABOVE, ON, BELOW; > > > > > } > > > > > > > > > > Position PositionOfPoint(int x, float y); > > > > > > > > > > > > > > > > > > Okay. > > > > Thoughts: > > > > 1. Parameterizing seems to be overkill, but I'd like to turn the X axis > into > > > > float, too. Agreed? > > > yes, sgtm > > > > > > > 2. Do we want to give any special attention to the case that Y assume > > > infinity, > > > > but x < a_x? In that case, we'd normally be giving BELOW, but that might > be > > > > confusing to a caller of the function who might have passed infinity in as > a > > > way > > > > of securing an ABOVE answer. In my mind, this is only theoretical; we > should > > > > just return BELOW. Looking for confirmation. > > > let's follow the math, then it is BELOW. That is least ambiguous, I think. > > > > Another thought, since it is a threshold curve, think of below/above/on the > > threshold. Then "position" may not be a good word. How about > > > > bool IsAboveThreshold(float x, float y) > > RelativePosition(float x, float y) ? Isn't IsAboveThreshold clearer?
On 2017/03/22 15:03:10, minyue-webrtc wrote: > On 2017/03/22 14:36:44, elad.alon wrote: > > On 2017/03/22 12:27:43, minyue-webrtc wrote: > > > On 2017/03/22 12:20:38, minyue-webrtc wrote: > > > > On 2017/03/22 12:11:20, elad.alon wrote: > > > > > > > > > > > > > > > https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... > > > > > File > > > webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... > > > > > > > webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:20: > > > > > struct ThresholdCurve { > > > > > On 2017/03/22 11:35:07, minyue-webrtc wrote: > > > > > > I think this can be made a class > > > > > > > > > > Will do. I'll group this with other changes. > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... > > > > > > > webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:38: > > > > > float ValueAt(int x) const { > > > > > On 2017/03/22 11:35:07, minyue-webrtc wrote: > > > > > > I think more useful API can be > > > > > > > > > > > > enum Position { > > > > > > ABOVE, ON, BELOW; > > > > > > } > > > > > > > > > > > > Position PositionOfPoint(int x, float y); > > > > > > > > > > > > > > > > > > > > > > Okay. > > > > > Thoughts: > > > > > 1. Parameterizing seems to be overkill, but I'd like to turn the X axis > > into > > > > > float, too. Agreed? > > > > yes, sgtm > > > > > > > > > 2. Do we want to give any special attention to the case that Y assume > > > > infinity, > > > > > but x < a_x? In that case, we'd normally be giving BELOW, but that might > > be > > > > > confusing to a caller of the function who might have passed infinity in > as > > a > > > > way > > > > > of securing an ABOVE answer. In my mind, this is only theoretical; we > > should > > > > > just return BELOW. Looking for confirmation. > > > > let's follow the math, then it is BELOW. That is least ambiguous, I think. > > > > > > Another thought, since it is a threshold curve, think of below/above/on the > > > threshold. Then "position" may not be a good word. How about > > > > > > bool IsAboveThreshold(float x, float y) > > > > RelativePosition(float x, float y) ? > > Isn't IsAboveThreshold clearer? My reservation is that as a utility class, I am not sure if we'd want to decide if the user wants to check <= or >=, and using IsAbove or IsBelow forces either. Maybe we could supply both? Another plus of giving both is that the intersection could also be used to check for on-curve, meaning we lose nothing when comparing to your suggested enum-based solution, while maintaining the more convenient boolean-based return-type?
On 2017/03/22 15:20:28, elad.alon wrote: > On 2017/03/22 15:03:10, minyue-webrtc wrote: > > On 2017/03/22 14:36:44, elad.alon wrote: > > > On 2017/03/22 12:27:43, minyue-webrtc wrote: > > > > On 2017/03/22 12:20:38, minyue-webrtc wrote: > > > > > On 2017/03/22 12:11:20, elad.alon wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... > > > > > > File > > > > webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... > > > > > > > > > webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:20: > > > > > > struct ThresholdCurve { > > > > > > On 2017/03/22 11:35:07, minyue-webrtc wrote: > > > > > > > I think this can be made a class > > > > > > > > > > > > Will do. I'll group this with other changes. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... > > > > > > > > > webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:38: > > > > > > float ValueAt(int x) const { > > > > > > On 2017/03/22 11:35:07, minyue-webrtc wrote: > > > > > > > I think more useful API can be > > > > > > > > > > > > > > enum Position { > > > > > > > ABOVE, ON, BELOW; > > > > > > > } > > > > > > > > > > > > > > Position PositionOfPoint(int x, float y); > > > > > > > > > > > > > > > > > > > > > > > > > > Okay. > > > > > > Thoughts: > > > > > > 1. Parameterizing seems to be overkill, but I'd like to turn the X > axis > > > into > > > > > > float, too. Agreed? > > > > > yes, sgtm > > > > > > > > > > > 2. Do we want to give any special attention to the case that Y assume > > > > > infinity, > > > > > > but x < a_x? In that case, we'd normally be giving BELOW, but that > might > > > be > > > > > > confusing to a caller of the function who might have passed infinity > in > > as > > > a > > > > > way > > > > > > of securing an ABOVE answer. In my mind, this is only theoretical; we > > > should > > > > > > just return BELOW. Looking for confirmation. > > > > > let's follow the math, then it is BELOW. That is least ambiguous, I > think. > > > > > > > > Another thought, since it is a threshold curve, think of below/above/on > the > > > > threshold. Then "position" may not be a good word. How about > > > > > > > > bool IsAboveThreshold(float x, float y) > > > > > > RelativePosition(float x, float y) ? > > > > Isn't IsAboveThreshold clearer? > > My reservation is that as a utility class, I am not sure if we'd want to decide > if the user wants to check <= or >=, and using IsAbove or IsBelow forces either. > Maybe we could supply both? Another plus of giving both is that the intersection > could also be used to check for on-curve, meaning we lose nothing when comparing > to your suggested enum-based solution, while maintaining the more convenient > boolean-based return-type? I might not have been clear - I meant that IsAbove checks !(value <= threshold), and IsBelow checks !(value >= threshold), and we don't want to decide for the user if he wants to receive the on-curve option as a true or as a false. If we give both IsBelow and IsAbove, the user can choose.
On 2017/03/22 15:23:41, elad.alon wrote: > On 2017/03/22 15:20:28, elad.alon wrote: > > On 2017/03/22 15:03:10, minyue-webrtc wrote: > > > On 2017/03/22 14:36:44, elad.alon wrote: > > > > On 2017/03/22 12:27:43, minyue-webrtc wrote: > > > > > On 2017/03/22 12:20:38, minyue-webrtc wrote: > > > > > > On 2017/03/22 12:11:20, elad.alon wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... > > > > > > > File > > > > > webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h > > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... > > > > > > > > > > > > webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:20: > > > > > > > struct ThresholdCurve { > > > > > > > On 2017/03/22 11:35:07, minyue-webrtc wrote: > > > > > > > > I think this can be made a class > > > > > > > > > > > > > > Will do. I'll group this with other changes. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2688613003/diff/60001/webrtc/modules/audio_codi... > > > > > > > > > > > > webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:38: > > > > > > > float ValueAt(int x) const { > > > > > > > On 2017/03/22 11:35:07, minyue-webrtc wrote: > > > > > > > > I think more useful API can be > > > > > > > > > > > > > > > > enum Position { > > > > > > > > ABOVE, ON, BELOW; > > > > > > > > } > > > > > > > > > > > > > > > > Position PositionOfPoint(int x, float y); > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Okay. > > > > > > > Thoughts: > > > > > > > 1. Parameterizing seems to be overkill, but I'd like to turn the X > > axis > > > > into > > > > > > > float, too. Agreed? > > > > > > yes, sgtm > > > > > > > > > > > > > 2. Do we want to give any special attention to the case that Y > assume > > > > > > infinity, > > > > > > > but x < a_x? In that case, we'd normally be giving BELOW, but that > > might > > > > be > > > > > > > confusing to a caller of the function who might have passed infinity > > in > > > as > > > > a > > > > > > way > > > > > > > of securing an ABOVE answer. In my mind, this is only theoretical; > we > > > > should > > > > > > > just return BELOW. Looking for confirmation. > > > > > > let's follow the math, then it is BELOW. That is least ambiguous, I > > think. > > > > > > > > > > Another thought, since it is a threshold curve, think of below/above/on > > the > > > > > threshold. Then "position" may not be a good word. How about > > > > > > > > > > bool IsAboveThreshold(float x, float y) > > > > > > > > RelativePosition(float x, float y) ? > > > > > > Isn't IsAboveThreshold clearer? > > > > My reservation is that as a utility class, I am not sure if we'd want to > decide > > if the user wants to check <= or >=, and using IsAbove or IsBelow forces > either. > > Maybe we could supply both? Another plus of giving both is that the > intersection > > could also be used to check for on-curve, meaning we lose nothing when > comparing > > to your suggested enum-based solution, while maintaining the more convenient > > boolean-based return-type? > > I might not have been clear - I meant that IsAbove checks !(value <= threshold), > and IsBelow checks !(value >= threshold), and we don't want to decide for the > user if he wants to receive the on-curve option as a true or as a false. If we > give both IsBelow and IsAbove, the user can choose. I think we can just say, for ThreadholdCurve, isAbove always include/or exclude the line. I believe it is enough for the current FEC controllers.
I hope I have struck the right balance between readability and keeping the existing behavior (enabling threshold still above-or-on, disabling threshold still below-or-on). PTAL. https://codereview.webrtc.org/2688613003/diff/110001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based.cc (right): https://codereview.webrtc.org/2688613003/diff/110001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based.cc:58: RTC_DCHECK(config_.fec_disabling_threshold < config_.fec_enabling_threshold); Please note that this cannot easily be changed to RTC_DCHECK_LT. IMHO, best to keep it as is.
kwiberg@webrtc.org changed reviewers: + kwiberg@webrtc.org
https://codereview.webrtc.org/2688613003/diff/110001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based.cc (right): https://codereview.webrtc.org/2688613003/diff/110001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based.cc:58: RTC_DCHECK(config_.fec_disabling_threshold < config_.fec_enabling_threshold); On 2017/03/24 17:42:13, elad.alon wrote: > Please note that this cannot easily be changed to RTC_DCHECK_LT. IMHO, best to > keep it as is. Why not? Are the types not printable?
excellent! only one comment https://codereview.webrtc.org/2688613003/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based.cc (right): https://codereview.webrtc.org/2688613003/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based.cc:58: RTC_DCHECK(config_.fec_disabling_threshold < config_.fec_enabling_threshold); I think we need to allow equality, i.e., a way to avoid hysteresis
https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based.cc (right): https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based.cc:104: return !config_.fec_enabling_threshold.IsBelowCurve( will this be ambiguous with FecDisablingDecision if fec_enabling_threshold == fec_disabling_threshold. say if bandwidth-packetloss is on curve, fec will be switched on and off constantly? https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.cc (right): https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.cc:65: // Enable when above the curve or exactly on it. same here https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h (right): https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:47: : ThresholdCurve({a_x, a_y}, {b_x, b_y}) {} Point() {a_x, a_y}, Point() {b_x, b_y}, even better if you add ctor to struct Point and make x,y const https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:74: // This curve is <= the rhs curve if no point from this curve is directly directly -> strictly, or even better, just remove "directly" https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:75: // above a point from the rhs curve. a point -> the corresponding point https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:76: // For the part between the endpoints, this can be directly checked (and I don't understand the comment 76 - 82 well. "the projection on the x-axis of one might be contained within the other" and some words are not clear. The code is clearer to me. I don't think we need any comment on the algorithm https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc (right): https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:19: enum RelativePosition { kBelow, kOn, kAbove }; no space after { and before } https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:31: TEST(ThresholdCurveTest, PointPositiopnRelativeToCurve) { position remove "RelativeToCurve" since it is clear in the context https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:41: // | \ Try android bot, you may not pass format check, because you cannot end a line with "\" https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:62: CheckRelativePosition(curve, {x, p1.y + 1}, kBelow); // A I think it is a bit too many points in the test. But it is not hard to read. So fine to keep it as it is. https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:112: TEST(ThresholdCurveTest, TwoCurves_SameProjection) { no underscore in test names https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:137: TEST(ThresholdCurveTest, TwoCurves_SameProjectionAndTouching) { SameProjectionAndTouching -> Overlapping (I think the focus of the test is in overlapping, the same projection part is tested in other test) https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:162: TEST(ThresholdCurveTest, TwoCurves_PotentiallyHigherCurveContinuesLeft) { PotentiallyHigherCurveContinuesLeft -> Crossing https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:189: TEST(ThresholdCurveTest, TwoCurves_PotentiallyHigherCurveLeftInLower) { PotentiallyHigherCurveLeftInLower -> DifferentProjection? https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:240: TEST(ThresholdCurveTest, TwoCurves_PotentiallyHigherRightBeyondAndOnLower) { This is also a test of touching, but how does it differ from line 137 https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:264: TEST(ThresholdCurveTest, TwoCurves_PotentiallyHigherRightBeyondAndBelowLower) { PotentiallyHigherRightBeyondAndBelowLower -> Crossing There are a few crossing tests, you may place them close to each other, can call Crossing1, Crossing2, etc. https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:311: TEST(ThresholdCurveTest, TwoCurves_OneContinuesOnOtherLeftSide) { This is also test of overlapping. you may move overlapping tests close to each other and call them Overlapping1, 2, ... https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:336: TEST(ThresholdCurveTest, TwoCurves_OneContinuesOnOtherRightSide) { Another test of overlapping. https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:363: TEST(ThresholdCurveTest, WrongOrderEdges) { You cannot call it Edge, it is vertex, or say point WrongPointsOrder https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:370: TEST(ThresholdCurveTest, EdgesHaveSameXValue) { We should allow this. it gives a half open rectangular region. FEC always on needs left = right = {0, 0} https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:377: TEST(ThresholdCurveTest, PositiveSlope) { SlopMustBePositive
https://codereview.webrtc.org/2688613003/diff/200001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc (right): https://codereview.webrtc.org/2688613003/diff/200001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:115: // The points (P1-P2) define the curve. This is a good case, but can we allow P1 == P2, which should define the same shape but a lot more natural descriptor.
On 2017/03/25 15:41:39, kwiberg-webrtc wrote: > https://codereview.webrtc.org/2688613003/diff/110001/webrtc/modules/audio_cod... > File > webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based.cc > (right): > > https://codereview.webrtc.org/2688613003/diff/110001/webrtc/modules/audio_cod... > webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based.cc:58: > RTC_DCHECK(config_.fec_disabling_threshold < config_.fec_enabling_threshold); > On 2017/03/24 17:42:13, elad.alon wrote: > > Please note that this cannot easily be changed to RTC_DCHECK_LT. IMHO, best to > > keep it as is. > > Why not? Are the types not printable? kwiberg@, I was holding off on answering you until I've had time to look more into it, but it's the end of the day and I've not gotten around to it. General, un-researched answer is "yes, something about stringification raised a compilation issue." For a better-researched answer that doesn't rely on my memory from last week, please confirm you want one, and I'll look more into it tomorrow.
https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based.cc (right): https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_plr_based.cc:104: return !config_.fec_enabling_threshold.IsBelowCurve( On 2017/03/27 16:37:06, minyue-webrtc wrote: > will this be ambiguous with FecDisablingDecision if fec_enabling_threshold == > fec_disabling_threshold. > > say if bandwidth-packetloss is on curve, fec will be switched on and off > constantly? The behavior here is exactly like it was before. If you want changes, I'd be happy to discuss them. Or do you see a way in which this does not reproduce the pre-CL behavior? https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.cc (right): https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_rplr_based.cc:65: // Enable when above the curve or exactly on it. On 2017/03/27 16:37:06, minyue-webrtc wrote: > same here Same answer. https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h (right): https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:47: : ThresholdCurve({a_x, a_y}, {b_x, b_y}) {} On 2017/03/27 16:37:06, minyue-webrtc wrote: > Point() {a_x, a_y}, Point() {b_x, b_y}, > > even better if you add ctor to struct Point and make x,y const The points are passed by value, not reference, but okay, I'll mark them const anyway. https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:74: // This curve is <= the rhs curve if no point from this curve is directly On 2017/03/27 16:37:06, minyue-webrtc wrote: > directly -> strictly, or even better, just remove "directly" Done. https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:75: // above a point from the rhs curve. On 2017/03/27 16:37:06, minyue-webrtc wrote: > a point -> the corresponding point Done. https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:76: // For the part between the endpoints, this can be directly checked (and On 2017/03/27 16:37:06, minyue-webrtc wrote: > I don't understand the comment 76 - 82 well. "the projection on the x-axis of > one might be contained within the other" and some words are not clear. > > The code is clearer to me. I don't think we need any comment on the algorithm Removed. https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc (right): https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:19: enum RelativePosition { kBelow, kOn, kAbove }; On 2017/03/27 16:37:07, minyue-webrtc wrote: > no space after { and before } "git cl format" added that, not me. So I'm going to keep it. https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:31: TEST(ThresholdCurveTest, PointPositiopnRelativeToCurve) { On 2017/03/27 16:37:07, minyue-webrtc wrote: > position > > remove "RelativeToCurve" since it is clear in the context Done. https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:41: // | \ On 2017/03/27 16:37:07, minyue-webrtc wrote: > Try android bot, you may not pass format check, because you cannot end a line > with "\" Added "//" at the end for all of those illustrations to avoid this problem. https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:62: CheckRelativePosition(curve, {x, p1.y + 1}, kBelow); // A On 2017/03/27 16:37:07, minyue-webrtc wrote: > I think it is a bit too many points in the test. But it is not hard to read. So > fine to keep it as it is. I don't think any of the points is demonstrably redundant. (That is to say, given the implementation two points might be equivalent, but one must not posit a certain implementation in the UT.) https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:112: TEST(ThresholdCurveTest, TwoCurves_SameProjection) { On 2017/03/27 16:37:07, minyue-webrtc wrote: > no underscore in test names Done. https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:137: TEST(ThresholdCurveTest, TwoCurves_SameProjectionAndTouching) { On 2017/03/27 16:37:07, minyue-webrtc wrote: > SameProjectionAndTouching -> Overlapping (I think the focus of the test is in > overlapping, the same projection part is tested in other test) "Overlapping" makes it sound like the projection overlaps, which is already implied by "same projection". The fact that one point inside the projection has the same Y value, but only one such point (not a complete overlap of the curves), is what I am trying to put the spotlight on. Do you have a suggestion for a better name under these circumstances? https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:162: TEST(ThresholdCurveTest, TwoCurves_PotentiallyHigherCurveContinuesLeft) { Used "crosses". https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:189: TEST(ThresholdCurveTest, TwoCurves_PotentiallyHigherCurveLeftInLower) { On 2017/03/27 16:37:07, minyue-webrtc wrote: > PotentiallyHigherCurveLeftInLower -> DifferentProjection? "Different projection" isn't enough, because you could get that with zero overlap in the projections. What I'm trying to show is that the vertical extensions on the curves' left side are handled correctly. Let's try... "TwoCurvesHigherLeftInsideLowerProjection"? https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:240: TEST(ThresholdCurveTest, TwoCurves_PotentiallyHigherRightBeyondAndOnLower) { On 2017/03/27 16:37:07, minyue-webrtc wrote: > This is also a test of touching, but how does it differ from line 137 Different projection. (Yes, some tests are similar, but knowing they'd all work without adding coverage is, imho, relying on knowledge of the implementation.) https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:264: TEST(ThresholdCurveTest, TwoCurves_PotentiallyHigherRightBeyondAndBelowLower) { On 2017/03/27 16:37:07, minyue-webrtc wrote: > PotentiallyHigherRightBeyondAndBelowLower -> Crossing > > There are a few crossing tests, you may place them close to each other, can call > Crossing1, Crossing2, etc. The tests are already grouped according to a certain order (I've renamed to make it clearer): 1. Position tests 2. Relative curves test 3. Configuration legality constraints And #2 is order as follows: 1. Similar projection (2 tests) 2. Variations on left side (2 tests) 3. Variations on right side (3 tests) 4. Completely/partially merged curves (3 tests) Naturally, there is some overlap between some of the tests (but no two are identical, so only through knowledge of the implementation could you say one is covered by the other). And this is not the only possible way to order things, of course. But I think it's a good order. https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:311: TEST(ThresholdCurveTest, TwoCurves_OneContinuesOnOtherLeftSide) { On 2017/03/27 16:37:07, minyue-webrtc wrote: > This is also test of overlapping. you may move overlapping tests close to each > other and call them Overlapping1, 2, ... These are three tests of (almost-) identical curves. 1. Completely identical. 2. Identical except for one extending further on left. 3. Identical except for one extending further on right. I've renamed to make it clearer. https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:336: TEST(ThresholdCurveTest, TwoCurves_OneContinuesOnOtherRightSide) { On 2017/03/27 16:37:07, minyue-webrtc wrote: > Another test of overlapping. See comment above. https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:363: TEST(ThresholdCurveTest, WrongOrderEdges) { On 2017/03/27 16:37:07, minyue-webrtc wrote: > You cannot call it Edge, it is vertex, or say point > > WrongPointsOrder Done. https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:370: TEST(ThresholdCurveTest, EdgesHaveSameXValue) { On 2017/03/27 16:37:07, minyue-webrtc wrote: > We should allow this. > > it gives a half open rectangular region. > > FEC always on needs left = right = {0, 0} As discussed offline, I think it's better to have left = (0, 0), right = (1, 0) in such a case. You'd get the exact same behavior, but without the need for special treatment. https://codereview.webrtc.org/2688613003/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:377: TEST(ThresholdCurveTest, PositiveSlope) { On 2017/03/27 16:37:07, minyue-webrtc wrote: > SlopMustBePositive Done. https://codereview.webrtc.org/2688613003/diff/230001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h (right): https://codereview.webrtc.org/2688613003/diff/230001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:23: constexpr Point(float x, float y) : x(x), y(y) {} Minyue, FYI, once this is defined, it's no longer possible to pass only some of the parameters for struct-style instantiation. So when you see the func({a, b}) in the code, don't worry, it doesn't compile with func({a}).
Did you discuss code duplication in fec_controller_plr_based_unittest.cc and fec_controller_rplr_based_unittest.cc ?
On 2017/03/28 06:56:11, michaelt wrote: > Did you discuss code duplication in fec_controller_plr_based_unittest.cc and > fec_controller_rplr_based_unittest.cc ? Yes. We've decided to keep things as they are, since the tests might change if the implementation changes. We're not likely to keep both FecControllers together for long; one needs to win and the other removed.
On 2017/03/28 08:00:54, elad.alon wrote:
> On 2017/03/28 06:56:11, michaelt wrote:
> > Did you discuss code duplication in fec_controller_plr_based_unittest.cc and
> > fec_controller_rplr_based_unittest.cc ?
>
> Yes. We've decided to keep things as they are, since the tests might change if
> the implementation changes. We're not likely to keep both FecControllers
> together for long; one needs to win and the other removed.
Even when we remove one FecControllers we would have "code duplication" ("test
logic duplication") in the unit-test because of
"threshold_curve". So we may could inject and mock threshold_curve. Which would
lead to way less code to test for the FecControllers.
On 2017/03/28 08:05:11, michaelt wrote:
> On 2017/03/28 08:00:54, elad.alon wrote:
> > On 2017/03/28 06:56:11, michaelt wrote:
> > > Did you discuss code duplication in fec_controller_plr_based_unittest.cc
and
> > > fec_controller_rplr_based_unittest.cc ?
> >
> > Yes. We've decided to keep things as they are, since the tests might change
if
> > the implementation changes. We're not likely to keep both FecControllers
> > together for long; one needs to win and the other removed.
>
> Even when we remove one FecControllers we would have "code duplication" ("test
> logic duplication") in the unit-test because of
> "threshold_curve". So we may could inject and mock threshold_curve. Which
would
> lead to way less code to test for the FecControllers.
Are there specific tests that you think is no longer necessary in the
FecController suite? Or do you mean that the FecController unit-tests should not
rely on the proper operation of ThresholdCurve, and should use a mock instead,
so that if the ThresholdCurve is ever defective, it wouldn't affect the
FecController UT?
https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc (right): https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:92: // To make sure we're really covering all of the cases, make sure that P1 P.S: I'll move this to the beginning of the scope with whatever other comments you might have.
Thanks! see my comments https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h (right): https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:40: // If either a.x == b.x or a.y == b.y, then the curve becomes defined then the curve becomes defined -> then the curve can be defined https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:41: // by a single point, either the left or the bottom one. Add "In such case, we force a = b, for easy treatment." https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:49: ThresholdCurve(const Point left, const Point right) const Point to const Point& https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:52: slope(b.x - a.x == 0.0f ? 0.0f : (b.y - a.y) / (b.x - a.x)), b.x == a.x ? https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:55: RTC_DCHECK_LE(left.x, right.x); I see why I thought you allowed wrong user input and corrected it. Because you did auto-correction in GetPoint and check the failure here. As reader, I would not let it pass before use of it. I think we can remove this two and check slope here https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:62: // Checks if a point is below the curve. (Points on the curve are neither "Points on the curve are neither below it nor above it. " gets repeated. I think fine to say Checks if a point is strictly below the curve. https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:64: bool IsBelowCurve(Point p) const { Point -> const Point& https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:65: // (p.x == a.x || p.x == b.x) special treatment avoids numerical errors. this comment needed? https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:68: } else if (p.x > b.x) { >= https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:72: } else if (p.x == b.x) { remove this else https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:79: // Checks if a point is above the curve. (Points on the curve are neither strictly above, and remove "(Points on ...)" https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:82: // (p.x == a.x || p.x == b.x) special treatment avoids numerical errors. ? https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:83: if (p.x < a.x) { <= https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:85: } else if (p.x > b.x) { >= https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:88: return false; // All points either on-curve or below it. remove this else https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:90: return p.y > b.y; remove this else https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:104: static Point GetPoint(Point a, Point b, bool use_a_if_unmerged) { I would change it slightly, see below. So that 1) it always return either arg1, or arg2, no reconstruction of a point. 2) a check on wrong user input 3) simpler var name on arg3 const Point& GetPoint(const Point& left, const Point& right, bool is_for_a) { if (left.x == right.x) { DCHECK_GE(left.y, right.y); return left; } if (left.y == right.y) { DCHECK_LE(left.x, right.x); return right; } return is_for_a ? left : right; } https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc (right): https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:11: #include <memory> I don't find it but where is this used? https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:121: // The points (P1-P2) define the curve. can we test that a == b == P1 instead https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:184: // The points (P1-P2) define the curve. can we test that a == b == P2 instead https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:252: TEST(ThresholdCurveTest, SinglePointCurveCurve) { SinglePointCurve https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:253: // The points (P1-P2) define the curve. this must be tested https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:293: TEST(ThresholdCurveTest, TwoCurvesSameProjection) { OverlappingCurves5 (let us count from the most overlapping, so identical curves is #1) https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:319: TEST(ThresholdCurveTest, HigherCurveProjectionWithinLowerProjection) { UntouchingCurves https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:346: TEST(ThresholdCurveTest, SecondCurvePointsOnFirstCurveExtensions) { OverlappingCurves4 https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:372: TEST(ThresholdCurveTest, SecondCurveCrossesLeftExtension) { CrossingCurves1 https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:399: TEST(ThresholdCurveTest, SecondCurveCrossesRightExtension) { CrossingCurves2 https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:425: TEST(ThresholdCurveTest, SecondCurveCrossesFirstCurveBetweenPoints) { CrossingCurves3 https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:461: TEST(ThresholdCurveTest, SecondCurveRightPointAboveFirstRightExtension) { similar to TwoCurvesSameProjection? keep 1 of them? https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:489: TEST(ThresholdCurveTest, IdenticalCurves) { OverlappingCurves1 (let us count from the most overlapping) https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:510: TEST(ThresholdCurveTest, AlmostIdenticalCurvesSecondContinuesOnOtherLeftSide) { OverlappingCurves2 https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:535: TEST(ThresholdCurveTest, AlmostIdenticalCurvesSecondContinuesOnOtherRightSide) { OverlappingCurves3 https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:563: constexpr ThresholdCurve::Point left{5, 10}; why is this example fail?
PTAL https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h (right): https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:40: // If either a.x == b.x or a.y == b.y, then the curve becomes defined On 2017/03/28 19:12:41, minyue-webrtc wrote: > then the curve becomes defined -> then the curve can be defined Done. https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:41: // by a single point, either the left or the bottom one. On 2017/03/28 19:12:42, minyue-webrtc wrote: > Add "In such case, we force a = b, for easy treatment." Added a different comment to that effect. https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:49: ThresholdCurve(const Point left, const Point right) On 2017/03/28 19:12:42, minyue-webrtc wrote: > const Point to const Point& Done. https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:52: slope(b.x - a.x == 0.0f ? 0.0f : (b.y - a.y) / (b.x - a.x)), On 2017/03/28 19:12:41, minyue-webrtc wrote: > b.x == a.x ? When floating point arithmetic is concerned, I'd rather err on the side of caution. https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:55: RTC_DCHECK_LE(left.x, right.x); On 2017/03/28 19:12:42, minyue-webrtc wrote: > I see why I thought you allowed wrong user input and corrected it. Because you > did auto-correction in GetPoint and check the failure here. As reader, I would > not let it pass before use of it. > > I think we can remove this two and check slope here Moved validations into GetPoint(). https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:62: // Checks if a point is below the curve. (Points on the curve are neither On 2017/03/28 19:12:41, minyue-webrtc wrote: > "Points on the curve are neither below it nor above it. " gets repeated. > > I think fine to say > Checks if a point is strictly below the curve. Done. https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:64: bool IsBelowCurve(Point p) const { On 2017/03/28 19:12:42, minyue-webrtc wrote: > Point -> const Point& Done. https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:65: // (p.x == a.x || p.x == b.x) special treatment avoids numerical errors. On 2017/03/28 19:12:42, minyue-webrtc wrote: > this comment needed? It explains why these cases must not be checked as part of the |p.y < offset + slope * p.x| case, even if we merge them with other cases. Moved the comment, though. https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:68: } else if (p.x > b.x) { On 2017/03/28 19:12:41, minyue-webrtc wrote: > >= Refactored function to do a left-to-right pass. https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:79: // Checks if a point is above the curve. (Points on the curve are neither On 2017/03/28 19:12:42, minyue-webrtc wrote: > strictly above, and remove "(Points on ...)" Done. https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:104: static Point GetPoint(Point a, Point b, bool use_a_if_unmerged) { On 2017/03/28 19:12:42, minyue-webrtc wrote: > I would change it slightly, see below. So that > > 1) it always return either arg1, or arg2, no reconstruction of a point. > 2) a check on wrong user input > 3) simpler var name on arg3 > > const Point& GetPoint(const Point& left, const Point& right, bool is_for_a) { > if (left.x == right.x) { > DCHECK_GE(left.y, right.y); > return left; > } > if (left.y == right.y) { > DCHECK_LE(left.x, right.x); > return right; > } > return is_for_a ? left : right; > } * I've used a/b instead of left/right as per our offline discussion. (Please note that when the points have the same x-value, left-right might be misleading, btw.) * I've moved the validations from the ctor to here, but not inside the if-statements; they're common to all uses of GetPoint(). * Took your suggestions otherwise. https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc (right): https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:11: #include <memory> On 2017/03/28 19:12:42, minyue-webrtc wrote: > I don't find it but where is this used? std::unique_ptr for the death-test. https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:121: // The points (P1-P2) define the curve. On 2017/03/28 19:12:42, minyue-webrtc wrote: > can we test that a == b == P1 instead Answered offline (preference for keeping this block-box, rather than rely on the implementation merging the points). https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:184: // The points (P1-P2) define the curve. On 2017/03/28 19:12:43, minyue-webrtc wrote: > can we test that a == b == P2 instead Same answer. https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:252: TEST(ThresholdCurveTest, SinglePointCurveCurve) { On 2017/03/28 19:12:42, minyue-webrtc wrote: > SinglePointCurve Done. https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:253: // The points (P1-P2) define the curve. On 2017/03/28 19:12:42, minyue-webrtc wrote: > this must be tested Answered above. https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:319: TEST(ThresholdCurveTest, HigherCurveProjectionWithinLowerProjection) { On 2017/03/28 19:12:42, minyue-webrtc wrote: > UntouchingCurves As discussed offline, I'll leave some addition of comments, renaming and possibly reordering of tests for a subsequent CL, to avoid delaying this one.
The CQ bit was checked by elad.alon@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: win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/7804)
The CQ bit was checked by elad.alon@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: linux_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/21298)
lgtm % nits https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc (right): https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:11: #include <memory> On 2017/03/28 21:16:08, elad.alon wrote: > On 2017/03/28 19:12:42, minyue-webrtc wrote: > > I don't find it but where is this used? > > std::unique_ptr for the death-test. Acknowledged. https://codereview.webrtc.org/2688613003/diff/290001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve_unittest.cc:293: TEST(ThresholdCurveTest, TwoCurvesSameProjection) { On 2017/03/28 19:12:42, minyue-webrtc wrote: > OverlappingCurves5 (let us count from the most overlapping, so identical curves > is #1) Per offline discussion, we may keep the test names and seek for future clarification. (if you come up with better ideas, they are welcome as well.) https://codereview.webrtc.org/2688613003/diff/350001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h (right): https://codereview.webrtc.org/2688613003/diff/350001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:38: // by a single point, either the left or the bottom one. (We merge the two "either the left or the bottom one" is redundant https://codereview.webrtc.org/2688613003/diff/350001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:63: } else if (p.x == a.x) { // Special treatment avoids numerical errors. I think we can make it more explicit "In principle, we can merge this into the next else, but to avoid numerical errors, we treat it separately." https://codereview.webrtc.org/2688613003/diff/350001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:91: static const Point& GetPoint(const Point& a, const Point& b, bool is_for_a) { I suggested left/right because the ctor "ThresholdCurve(const Point& left, const Point& right)" uses left/right anyway
On 2017/03/28 15:03:20, elad.alon wrote:
> On 2017/03/28 08:05:11, michaelt wrote:
> > On 2017/03/28 08:00:54, elad.alon wrote:
> > > On 2017/03/28 06:56:11, michaelt wrote:
> > > > Did you discuss code duplication in fec_controller_plr_based_unittest.cc
> and
> > > > fec_controller_rplr_based_unittest.cc ?
> > >
> > > Yes. We've decided to keep things as they are, since the tests might
change
> if
> > > the implementation changes. We're not likely to keep both FecControllers
> > > together for long; one needs to win and the other removed.
> >
> > Even when we remove one FecControllers we would have "code duplication"
("test
> > logic duplication") in the unit-test because of
> > "threshold_curve". So we may could inject and mock threshold_curve. Which
> would
> > lead to way less code to test for the FecControllers.
>
> Are there specific tests that you think is no longer necessary in the
> FecController suite? Or do you mean that the FecController unit-tests should
not
> rely on the proper operation of ThresholdCurve, and should use a mock instead,
> so that if the ThresholdCurve is ever defective, it wouldn't affect the
> FecController UT?
"FecController unit-tests should not rely on the proper operation of
ThresholdCurve" that is one point.
And leads to a removal of a lot of test in the FEC controller, since we can
remove all the test of the FEC controller which test the logic of
"threshold_curve" (which are quite a lot).
https://codereview.webrtc.org/2688613003/diff/350001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h (right): https://codereview.webrtc.org/2688613003/diff/350001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:38: // by a single point, either the left or the bottom one. (We merge the two On 2017/03/29 06:17:31, minyue-webrtc wrote: > "either the left or the bottom one" is redundant Done. https://codereview.webrtc.org/2688613003/diff/350001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:63: } else if (p.x == a.x) { // Special treatment avoids numerical errors. On 2017/03/29 06:17:31, minyue-webrtc wrote: > I think we can make it more explicit > > "In principle, we can merge this into the next else, but to avoid numerical > errors, we treat it separately." Done. https://codereview.webrtc.org/2688613003/diff/350001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/util/threshold_curve.h:91: static const Point& GetPoint(const Point& a, const Point& b, bool is_for_a) { On 2017/03/29 06:17:31, minyue-webrtc wrote: > I suggested left/right because the ctor "ThresholdCurve(const Point& left, const > Point& right)" uses left/right anyway Using left/right, then.
The CQ bit was checked by elad.alon@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/2688613003/#ps370001 (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": 370001, "attempt_start_ts": 1490780683616880,
"parent_rev": "61abe1582915fb25fe3f231d7d2c9a174c0e3e6b", "commit_rev":
"326263a678cc84ce08d141d7e507c0282fa61200"}
Message was sent while issue was closed.
Description was changed from ========== Avoid code duplication between PLR/RPLR-based FecController BUG=webrtc:7058 ========== to ========== Avoid code duplication between PLR/RPLR-based FecController BUG=webrtc:7058 Review-Url: https://codereview.webrtc.org/2688613003 Cr-Commit-Position: refs/heads/master@{#17437} Committed: https://chromium.googlesource.com/external/webrtc/+/326263a678cc84ce08d141d7e... ==========
Message was sent while issue was closed.
Committed patchset #21 (id:370001) as https://chromium.googlesource.com/external/webrtc/+/326263a678cc84ce08d141d7e... |
