Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2577463002:
Avoid precision loss in TrendlineEstimator from int64_t -> double conversion (Closed)
DescriptionAvoid precision loss in TrendlineEstimator by passing the arrival time as an int64_t instead of a double.
BUG=webrtc:6884
Committed: https://crrev.com/c12cbaf9dd0729dd45f3fc45a1938d1b3455e40a
Committed: https://crrev.com/b3564adc91b0ff047ac29a9855ec96b247bf0634
Cr-Original-Commit-Position: refs/heads/master@{#15631}
Cr-Commit-Position: refs/heads/master@{#15641}
Patch Set 1 #Patch Set 2 : Avoid precision loss in TrendlineFilter by passing the arrival time as an int64_t rather than a dou… #
Total comments: 5
Patch Set 3 : Simplify/refactor unit test. #Patch Set 4 : Spelling #Patch Set 5 : Increase tolerance in unit test. #
Total comments: 6
Patch Set 6 : Nits #Patch Set 7 : Rebase #Patch Set 8 : Move test function to anonymous namespace #
Messages
Total messages: 37 (21 generated)
terelius@webrtc.org changed reviewers: + brandtr@webrtc.org, stefan@webrtc.org
Looks good, just one question. https://codereview.webrtc.org/2577463002/diff/20001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/trendline_estimator.cc (right): https://codereview.webrtc.org/2577463002/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/trendline_estimator.cc:32: double x_avg = sum_x / points.size(); ... and then do the double conversion in this function instead? ... (2/3) https://codereview.webrtc.org/2577463002/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/trendline_estimator.cc:85: static_cast<double>(arrival_time_ms - first_arrival_time_ms), ... and then remove this cast. (3/3) https://codereview.webrtc.org/2577463002/diff/20001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/trendline_estimator.h (right): https://codereview.webrtc.org/2577463002/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/trendline_estimator.h:56: // Keep the arival times small by using the change from the first packet. "arrival" https://codereview.webrtc.org/2577463002/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/trendline_estimator.h:62: std::list<std::pair<double, double>> delay_hist_; Would there be a point in storing the x value as an int64_t here ... (1/3)
On 2016/12/14 09:12:39, brandtr wrote: > Looks good, just one question. > > https://codereview.webrtc.org/2577463002/diff/20001/webrtc/modules/congestion... > File webrtc/modules/congestion_controller/trendline_estimator.cc (right): > > https://codereview.webrtc.org/2577463002/diff/20001/webrtc/modules/congestion... > webrtc/modules/congestion_controller/trendline_estimator.cc:32: double x_avg = > sum_x / points.size(); > ... and then do the double conversion in this function instead? ... (2/3) > > https://codereview.webrtc.org/2577463002/diff/20001/webrtc/modules/congestion... > webrtc/modules/congestion_controller/trendline_estimator.cc:85: > static_cast<double>(arrival_time_ms - first_arrival_time_ms), > ... and then remove this cast. (3/3) > > https://codereview.webrtc.org/2577463002/diff/20001/webrtc/modules/congestion... > File webrtc/modules/congestion_controller/trendline_estimator.h (right): > > https://codereview.webrtc.org/2577463002/diff/20001/webrtc/modules/congestion... > webrtc/modules/congestion_controller/trendline_estimator.h:56: // Keep the > arival times small by using the change from the first packet. > "arrival" > > https://codereview.webrtc.org/2577463002/diff/20001/webrtc/modules/congestion... > webrtc/modules/congestion_controller/trendline_estimator.h:62: > std::list<std::pair<double, double>> delay_hist_; > Would there be a point in storing the x value as an int64_t here ... (1/3) I don't think there is any benefit in doing the cast later, and it might lead to more casts.
On 2016/12/14 09:28:22, terelius wrote: > On 2016/12/14 09:12:39, brandtr wrote: > > Looks good, just one question. > > > > > https://codereview.webrtc.org/2577463002/diff/20001/webrtc/modules/congestion... > > File webrtc/modules/congestion_controller/trendline_estimator.cc (right): > > > > > https://codereview.webrtc.org/2577463002/diff/20001/webrtc/modules/congestion... > > webrtc/modules/congestion_controller/trendline_estimator.cc:32: double x_avg = > > sum_x / points.size(); > > ... and then do the double conversion in this function instead? ... (2/3) > > > > > https://codereview.webrtc.org/2577463002/diff/20001/webrtc/modules/congestion... > > webrtc/modules/congestion_controller/trendline_estimator.cc:85: > > static_cast<double>(arrival_time_ms - first_arrival_time_ms), > > ... and then remove this cast. (3/3) > > > > > https://codereview.webrtc.org/2577463002/diff/20001/webrtc/modules/congestion... > > File webrtc/modules/congestion_controller/trendline_estimator.h (right): > > > > > https://codereview.webrtc.org/2577463002/diff/20001/webrtc/modules/congestion... > > webrtc/modules/congestion_controller/trendline_estimator.h:56: // Keep the > > arival times small by using the change from the first packet. > > "arrival" > > > > > https://codereview.webrtc.org/2577463002/diff/20001/webrtc/modules/congestion... > > webrtc/modules/congestion_controller/trendline_estimator.h:62: > > std::list<std::pair<double, double>> delay_hist_; > > Would there be a point in storing the x value as an int64_t here ... (1/3) > > I don't think there is any benefit in doing the cast later, and it might lead to > more casts. Yes, you would have to do more casts then. lgtm when the spelling is fixed.
The CQ bit was checked by terelius@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/...
Refactored the unit test. Please take another look. https://codereview.webrtc.org/2577463002/diff/20001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/trendline_estimator.h (right): https://codereview.webrtc.org/2577463002/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/trendline_estimator.h:56: // Keep the arival times small by using the change from the first packet. On 2016/12/14 09:12:38, brandtr wrote: > "arrival" Done.
Description was changed from ========== Avoid precision loss in TrendlineFilter by passing the arrival time as an int64_t rather than a double. BUG=webrtc:6884 ========== to ========== Avoid precision loss in TrendlineEstimator by passing the arrival time as an int64_t instead of a double. BUG=webrtc:6884 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with some new nits :) https://codereview.webrtc.org/2577463002/diff/80001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/trendline_estimator.cc (right): https://codereview.webrtc.org/2577463002/diff/80001/webrtc/modules/congestion... webrtc/modules/congestion_controller/trendline_estimator.cc:41: if (denominator != 0) I think it would be nicer to reverse this so the special case happens within the if clause, and the general case at the end of the function. https://codereview.webrtc.org/2577463002/diff/80001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/trendline_estimator_unittest.cc (right): https://codereview.webrtc.org/2577463002/diff/80001/webrtc/modules/congestion... webrtc/modules/congestion_controller/trendline_estimator_unittest.cc:32: for (size_t i = 0; i < kPacketCount; i++) { I think we often write ++i in for loops. https://codereview.webrtc.org/2577463002/diff/80001/webrtc/modules/congestion... webrtc/modules/congestion_controller/trendline_estimator_unittest.cc:38: for (size_t i = 1; i < kPacketCount; i++) { Here too.
https://codereview.webrtc.org/2577463002/diff/80001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/trendline_estimator.cc (right): https://codereview.webrtc.org/2577463002/diff/80001/webrtc/modules/congestion... webrtc/modules/congestion_controller/trendline_estimator.cc:41: if (denominator != 0) On 2016/12/15 12:06:08, brandtr wrote: > I think it would be nicer to reverse this so the special case happens within the > if clause, and the general case at the end of the function. Done. https://codereview.webrtc.org/2577463002/diff/80001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/trendline_estimator_unittest.cc (right): https://codereview.webrtc.org/2577463002/diff/80001/webrtc/modules/congestion... webrtc/modules/congestion_controller/trendline_estimator_unittest.cc:32: for (size_t i = 0; i < kPacketCount; i++) { On 2016/12/15 12:06:08, brandtr wrote: > I think we often write ++i in for loops. Done. However, the style guide explicitly allows either and they are used almost equally often in the code base. Interestingly, the choice depends on the name of the variable; k++ is preferred to ++k with over 80% of the cases, but ++i is preferred to i++ with 70% of all cases. https://codereview.webrtc.org/2577463002/diff/80001/webrtc/modules/congestion... webrtc/modules/congestion_controller/trendline_estimator_unittest.cc:38: for (size_t i = 1; i < kPacketCount; i++) { On 2016/12/15 12:06:08, brandtr wrote: > Here too. Done.
On 2016/12/15 13:11:08, terelius wrote: > https://codereview.webrtc.org/2577463002/diff/80001/webrtc/modules/congestion... > File webrtc/modules/congestion_controller/trendline_estimator.cc (right): > > https://codereview.webrtc.org/2577463002/diff/80001/webrtc/modules/congestion... > webrtc/modules/congestion_controller/trendline_estimator.cc:41: if (denominator > != 0) > On 2016/12/15 12:06:08, brandtr wrote: > > I think it would be nicer to reverse this so the special case happens within > the > > if clause, and the general case at the end of the function. > > Done. > > https://codereview.webrtc.org/2577463002/diff/80001/webrtc/modules/congestion... > File webrtc/modules/congestion_controller/trendline_estimator_unittest.cc > (right): > > https://codereview.webrtc.org/2577463002/diff/80001/webrtc/modules/congestion... > webrtc/modules/congestion_controller/trendline_estimator_unittest.cc:32: for > (size_t i = 0; i < kPacketCount; i++) { > On 2016/12/15 12:06:08, brandtr wrote: > > I think we often write ++i in for loops. > > Done. However, the style guide explicitly allows either and they are used almost > equally often in the code base. Interestingly, the choice depends on the name of > the variable; k++ is preferred to ++k with over 80% of the cases, but ++i is > preferred to i++ with 70% of all cases. > > https://codereview.webrtc.org/2577463002/diff/80001/webrtc/modules/congestion... > webrtc/modules/congestion_controller/trendline_estimator_unittest.cc:38: for > (size_t i = 1; i < kPacketCount; i++) { > On 2016/12/15 12:06:08, brandtr wrote: > > Here too. > > Done. Nice statistics :)
lgtm
The CQ bit was checked by terelius@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org Link to the patchset: https://codereview.webrtc.org/2577463002/#ps120001 (title: "Rebase")
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": 120001, "attempt_start_ts": 1481810576766170,
"parent_rev": "d00a44a0edc95feae8f8e0e892363bc29e57e526", "commit_rev":
"867926ac9ce54fb38447e15cd794eed955aa4855"}
Message was sent while issue was closed.
Description was changed from ========== Avoid precision loss in TrendlineEstimator by passing the arrival time as an int64_t instead of a double. BUG=webrtc:6884 ========== to ========== Avoid precision loss in TrendlineEstimator by passing the arrival time as an int64_t instead of a double. BUG=webrtc:6884 Review-Url: https://codereview.webrtc.org/2577463002 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Avoid precision loss in TrendlineEstimator by passing the arrival time as an int64_t instead of a double. BUG=webrtc:6884 Review-Url: https://codereview.webrtc.org/2577463002 ========== to ========== Avoid precision loss in TrendlineEstimator by passing the arrival time as an int64_t instead of a double. BUG=webrtc:6884 Committed: https://crrev.com/c12cbaf9dd0729dd45f3fc45a1938d1b3455e40a Cr-Commit-Position: refs/heads/master@{#15631} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/c12cbaf9dd0729dd45f3fc45a1938d1b3455e40a Cr-Commit-Position: refs/heads/master@{#15631}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.webrtc.org/2582513002/ by terelius@webrtc.org. The reason for reverting is: Multiple definitions of TestEstimator.
Message was sent while issue was closed.
Description was changed from ========== Avoid precision loss in TrendlineEstimator by passing the arrival time as an int64_t instead of a double. BUG=webrtc:6884 Committed: https://crrev.com/c12cbaf9dd0729dd45f3fc45a1938d1b3455e40a Cr-Commit-Position: refs/heads/master@{#15631} ========== to ========== Avoid precision loss in TrendlineEstimator by passing the arrival time as an int64_t instead of a double. BUG=webrtc:6884 Committed: https://crrev.com/c12cbaf9dd0729dd45f3fc45a1938d1b3455e40a Cr-Commit-Position: refs/heads/master@{#15631} ==========
The CQ bit was checked by terelius@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: This issue passed the CQ dry run.
The CQ bit was checked by terelius@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2577463002/#ps140001 (title: "Move test function to anonymous namespace")
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": 1481818724012280,
"parent_rev": "e434f69372cce9b9a9c9d068302f9c9cf55b202e", "commit_rev":
"987e25ca12cbf4ea836d9cc2b55a3ccf0821f2c3"}
Message was sent while issue was closed.
Description was changed from ========== Avoid precision loss in TrendlineEstimator by passing the arrival time as an int64_t instead of a double. BUG=webrtc:6884 Committed: https://crrev.com/c12cbaf9dd0729dd45f3fc45a1938d1b3455e40a Cr-Commit-Position: refs/heads/master@{#15631} ========== to ========== Avoid precision loss in TrendlineEstimator by passing the arrival time as an int64_t instead of a double. BUG=webrtc:6884 Committed: https://crrev.com/c12cbaf9dd0729dd45f3fc45a1938d1b3455e40a Cr-Commit-Position: refs/heads/master@{#15631} Review-Url: https://codereview.webrtc.org/2577463002 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Avoid precision loss in TrendlineEstimator by passing the arrival time as an int64_t instead of a double. BUG=webrtc:6884 Committed: https://crrev.com/c12cbaf9dd0729dd45f3fc45a1938d1b3455e40a Cr-Commit-Position: refs/heads/master@{#15631} Review-Url: https://codereview.webrtc.org/2577463002 ========== to ========== Avoid precision loss in TrendlineEstimator by passing the arrival time as an int64_t instead of a double. BUG=webrtc:6884 Committed: https://crrev.com/c12cbaf9dd0729dd45f3fc45a1938d1b3455e40a Committed: https://crrev.com/b3564adc91b0ff047ac29a9855ec96b247bf0634 Cr-Original-Commit-Position: refs/heads/master@{#15631} Cr-Commit-Position: refs/heads/master@{#15641} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/b3564adc91b0ff047ac29a9855ec96b247bf0634 Cr-Commit-Position: refs/heads/master@{#15641} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
