Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1685743003:
Avoid overflow in WebRtcSpl_Sqrt (Closed)
|
Created:
4 years, 10 months ago by hlundin-webrtc Modified:
4 years, 10 months ago Reviewers:
kwiberg-webrtc CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, peah-webrtc, minyue-webrtc, aluebs-webrtc, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAvoid overflow in WebRtcSpl_Sqrt
When the input to WebRtcSpl_Sqrt was the maximum negative value
(-2147483648), the calculations would overflow. This is now solved by
nudging this particular input value one step.
BUG=webrtc:5512
Committed: https://crrev.com/71e92dc5e8f56da447865a66df2076e4095a7348
Cr-Commit-Position: refs/heads/master@{#11631}
Patch Set 1 #
Total comments: 2
Patch Set 2 : After review #Patch Set 3 : Patch set 2 didn't work - fixing it #Messages
Total messages: 26 (10 generated)
The CQ bit was checked by henrik.lundin@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1685743003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1685743003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
henrik.lundin@webrtc.org changed reviewers: + kwiberg@webrtc.org
Karl, Please, take a look. Thanks!
lgtm https://codereview.webrtc.org/1685743003/diff/1/webrtc/common_audio/signal_pr... File webrtc/common_audio/signal_processing/spl_sqrt.c (right): https://codereview.webrtc.org/1685743003/diff/1/webrtc/common_audio/signal_pr... webrtc/common_audio/signal_processing/spl_sqrt.c:150: } The optimizer may already be doing this for you, but it might not be a bad idea to hide the most uncommon condition (WORD32_MIN) behind the less uncommon one (negative). Also, you can avoid loss of precision by the decrement if you simply return the correct result: if (A < 0) { if (A == WEBRTC_SPL_WORD32_MIN) { COMPILE_ASSERT(WEBRTC_SPL_WORD32_MIN == -2147483648); return 46341; // sqrt(2147483648) = 46340.950012... } A = -A; } else if (A == 0) { return 0; // sqrt(0) = 0 }
After review
https://codereview.webrtc.org/1685743003/diff/1/webrtc/common_audio/signal_pr... File webrtc/common_audio/signal_processing/spl_sqrt.c (right): https://codereview.webrtc.org/1685743003/diff/1/webrtc/common_audio/signal_pr... webrtc/common_audio/signal_processing/spl_sqrt.c:150: } On 2016/02/11 09:28:10, kwiberg-webrtc wrote: > The optimizer may already be doing this for you, but it might not be a bad idea > to hide the most uncommon condition (WORD32_MIN) behind the less uncommon one > (negative). Also, you can avoid loss of precision by the decrement if you simply > return the correct result: > > if (A < 0) { > if (A == WEBRTC_SPL_WORD32_MIN) { > COMPILE_ASSERT(WEBRTC_SPL_WORD32_MIN == -2147483648); > return 46341; // sqrt(2147483648) = 46340.950012... > } > A = -A; > } else if (A == 0) { > return 0; // sqrt(0) = 0 > } Much better. Done.
The CQ bit was checked by henrik.lundin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1685743003/#ps20001 (title: "After review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1685743003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1685743003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_compile_dbg/builds/...)
Try again
Fixing it again
Patchset #3 (id:40001) has been deleted
Patch set 2 didn't work - fixing it
Patchset #3 (id:60001) has been deleted
Karl, your suggestion with COMPILE_ASSERT didn't compile on some bots (see patch set 2). So, I had to back-track a bit in patch set 3.
On 2016/02/15 15:24:07, hlundin-webrtc wrote: > Karl, your suggestion with COMPILE_ASSERT didn't compile on some bots (see patch > set 2). So, I had to back-track a bit in patch set 3. You can probably make the compilers eat it if you spell it as e.g. -2147483647 - 1, or -(int32_t)2147483647 - 1, or something. Or omit the assert, since it's rather obvious what the min value must be. Or keep what you do in patch set #3. lgtm either way.
The CQ bit was checked by henrik.lundin@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1685743003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1685743003/80001
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Avoid overflow in WebRtcSpl_Sqrt When the input to WebRtcSpl_Sqrt was the maximum negative value (-2147483648), the calculations would overflow. This is now solved by nudging this particular input value one step. BUG=webrtc:5512 ========== to ========== Avoid overflow in WebRtcSpl_Sqrt When the input to WebRtcSpl_Sqrt was the maximum negative value (-2147483648), the calculations would overflow. This is now solved by nudging this particular input value one step. BUG=webrtc:5512 Committed: https://crrev.com/71e92dc5e8f56da447865a66df2076e4095a7348 Cr-Commit-Position: refs/heads/master@{#11631} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/71e92dc5e8f56da447865a66df2076e4095a7348 Cr-Commit-Position: refs/heads/master@{#11631} |
