Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1823763004:
Keep reads within buffer in AnalysisUpdateNeon(). (Closed)
|
Created:
4 years, 9 months ago by Simon Hosie Modified:
4 years, 8 months ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionKeep reads within buffer in AnalysisUpdateNeon().
BUG=webrtc:5631
Committed: https://crrev.com/de81ea8524de328cf0e76280291485f55f74f5db
Cr-Commit-Position: refs/heads/master@{#12228}
Patch Set 1 #
Total comments: 7
Messages
Total messages: 22 (5 generated)
henrik.lundin@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, peah@webrtc.org
Thanks for your patch. Please, see one comment inline. I also added peah@ as reviewer. https://codereview.webrtc.org/1823763004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/ns/nsx_core_neon.c (right): https://codereview.webrtc.org/1823763004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/ns/nsx_core_neon.c:598: int32x4_t tmp32_low = vmull_s16(vget_low_s16(window), vget_low_s16(buffer)); Can you explain to me what this new code after the loop does?
https://codereview.webrtc.org/1823763004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/ns/nsx_core_neon.c (right): https://codereview.webrtc.org/1823763004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/ns/nsx_core_neon.c:598: int32x4_t tmp32_low = vmull_s16(vget_low_s16(window), vget_low_s16(buffer)); On 2016/03/31 07:55:09, hlundin-webrtc wrote: > Can you explain to me what this new code after the loop does? The change to the loop condition is to make it run one iteration fewer. We still want to do all of that last iteration of work _except_ the `vld1q_s16()` operations (which are preparation for the subsequent iteration); so this extra code repeats the loop body with those removed.
https://codereview.webrtc.org/1823763004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/ns/nsx_core_neon.c (right): https://codereview.webrtc.org/1823763004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/ns/nsx_core_neon.c:573: int16_t* p_end_buffer = inst->analysisBuffer + inst->anaLen; Could you please explain why this code changed. As far as I can see, the change is bitexact as both p_start_out and p_start_buffer are updated in the same manner and differ from p_end_out and p_end_buffer in the same manner. However, since data is written into p_start_out and only read from p_start_buffer, to me the former loop condition more clearly shows/promises that there is no data written outside of the out buffer. https://codereview.webrtc.org/1823763004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/ns/nsx_core_neon.c:604: vst1q_s16(p_start_out, vcombine_s16(result_low, result_high)); As far as I can see, p_start_out will be > out + inst->anaLen here, right? Is that intended and safe? As far as I can see, this differs from how it is done in the C counterpart function AnalysisUpdateC() in nsx_core.c. Could you please explain this codechange further.
https://codereview.webrtc.org/1823763004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/ns/nsx_core_neon.c (right): https://codereview.webrtc.org/1823763004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/ns/nsx_core_neon.c:604: vst1q_s16(p_start_out, vcombine_s16(result_low, result_high)); Changing the loop condition to work with the input buffer reduces the number of iterations by one (because `p_start_buffer` has already been incremented once before the loop), so the writes should stay within range in both cases. Before the change the final iteration would perform reads for the subsequent iteration, which is never used so it's likely that those reads are unsafe (I haven't actually confirmed the the safety, but it's bad practice and should be corrected anyway).
On 2016/04/01 06:12:59, Simon Hosie wrote: > https://codereview.webrtc.org/1823763004/diff/1/webrtc/modules/audio_processi... > File webrtc/modules/audio_processing/ns/nsx_core_neon.c (right): > > https://codereview.webrtc.org/1823763004/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/ns/nsx_core_neon.c:604: vst1q_s16(p_start_out, > vcombine_s16(result_low, result_high)); > Changing the loop condition to work with the input buffer reduces the number of > iterations by one (because `p_start_buffer` has already been incremented once > before the loop), so the writes should stay within range in both cases. Before > the change the final iteration would perform reads for the subsequent iteration, > which is never used so it's likely that those reads are unsafe (I haven't > actually confirmed the the safety, but it's bad practice and should be corrected > anyway). Simon: one more question from peah to answer inline. Thanks!
On 2016/04/01 08:34:41, hlundin-webrtc wrote: > On 2016/04/01 06:12:59, Simon Hosie wrote: > > > https://codereview.webrtc.org/1823763004/diff/1/webrtc/modules/audio_processi... > > File webrtc/modules/audio_processing/ns/nsx_core_neon.c (right): > > > > > https://codereview.webrtc.org/1823763004/diff/1/webrtc/modules/audio_processi... > > webrtc/modules/audio_processing/ns/nsx_core_neon.c:604: vst1q_s16(p_start_out, > > vcombine_s16(result_low, result_high)); > > Changing the loop condition to work with the input buffer reduces the number > of > > iterations by one (because `p_start_buffer` has already been incremented once > > before the loop), so the writes should stay within range in both cases. > Before > > the change the final iteration would perform reads for the subsequent > iteration, > > which is never used so it's likely that those reads are unsafe (I haven't > > actually confirmed the the safety, but it's bad practice and should be > corrected > > anyway). > > Simon: one more question from peah to answer inline. Thanks! I think I got it now (including the unanswered question). Thanks for the explanation. I have one final question: How have you verified the bitexactness of this change?
On 2016/04/01 10:59:10, peah-webrtc wrote: > On 2016/04/01 08:34:41, hlundin-webrtc wrote: > > On 2016/04/01 06:12:59, Simon Hosie wrote: > > > > > > https://codereview.webrtc.org/1823763004/diff/1/webrtc/modules/audio_processi... > > > File webrtc/modules/audio_processing/ns/nsx_core_neon.c (right): > > > > > > > > > https://codereview.webrtc.org/1823763004/diff/1/webrtc/modules/audio_processi... > > > webrtc/modules/audio_processing/ns/nsx_core_neon.c:604: > vst1q_s16(p_start_out, > > > vcombine_s16(result_low, result_high)); > > > Changing the loop condition to work with the input buffer reduces the number > > of > > > iterations by one (because `p_start_buffer` has already been incremented > once > > > before the loop), so the writes should stay within range in both cases. > > Before > > > the change the final iteration would perform reads for the subsequent > > iteration, > > > which is never used so it's likely that those reads are unsafe (I haven't > > > actually confirmed the the safety, but it's bad practice and should be > > corrected > > > anyway). > > > > Simon: one more question from peah to answer inline. Thanks! > > I think I got it now (including the unanswered question). Thanks for the > explanation. > > I have one final question: How have you verified the bitexactness of this > change? Yep. I used the audioproc tool with a chunk of /dev/urandom: $ dd if=/dev/urandom of=raw.pcm bs=65536 count=100 $ ./audioproc -i raw.pcm -ns -o before.pcm <apply this patch> $ ./audioproc -i raw.pcm -ns -o after.pcm <edit code to change last shift to 13 instead of 14> $ ./audioproc -i raw.pcm -ns -o error.pcm $ md5sum {before,after,error}.pcm.wav 525fd44c6b501f957c82c118438924d7 before.pcm.wav 525fd44c6b501f957c82c118438924d7 after.pcm.wav d4a94a8231d3a3aa0ef3fe5825f13e1f error.pcm.wav Introducing a deliberate error into the code ensures that I'm not accidentally testing the C path or anything like that. I edited the last shift on the outside of the loop to make sure it wasn't working beyond the end of the buffer on data that would be ignored. Are there other tests I should run as well?
https://codereview.webrtc.org/1823763004/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/ns/nsx_core_neon.c (right): https://codereview.webrtc.org/1823763004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/ns/nsx_core_neon.c:573: int16_t* p_end_buffer = inst->analysisBuffer + inst->anaLen; FTR; the change from testing the progress of the destination pointer to testing the progress of the source pointer is because the source pointer leads the destination pointer by one iteration, and so it will reach its limit first. https://codereview.webrtc.org/1823763004/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/ns/nsx_core_neon.c:580: p_start_buffer += 8; Here is where the source pointer starts to lead the destination.
On 2016/04/01 21:57:46, Simon Hosie wrote: > https://codereview.webrtc.org/1823763004/diff/1/webrtc/modules/audio_processi... > File webrtc/modules/audio_processing/ns/nsx_core_neon.c (right): > > https://codereview.webrtc.org/1823763004/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/ns/nsx_core_neon.c:573: int16_t* p_end_buffer = > inst->analysisBuffer + inst->anaLen; > FTR; the change from testing the progress of the destination pointer to testing > the progress of the source pointer is because the source pointer leads the > destination pointer by one iteration, and so it will reach its limit first. > > https://codereview.webrtc.org/1823763004/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/ns/nsx_core_neon.c:580: p_start_buffer += 8; > Here is where the source pointer starts to lead the destination. Great! Supernice! Thanks for the explanations! lgtm
Thanks for helping out, Simon! LGTM. There are no more tests to run, except the regular trybots, which I'm kicking off now. Do you want us to land this for you now?
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/1823763004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1823763004/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_drmemory_light on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/buil...)
On 2016/04/04 07:14:14, hlundin-webrtc wrote: > Thanks for helping out, Simon! LGTM. > > There are no more tests to run, except the regular trybots, which I'm kicking > off now. > > Do you want us to land this for you now? Yep, suits me. Although commit-bot says otherwise. Maybe that's nothing?
On 2016/04/05 01:28:54, Simon Hosie wrote: > On 2016/04/04 07:14:14, hlundin-webrtc wrote: > > Thanks for helping out, Simon! LGTM. > > > > There are no more tests to run, except the regular trybots, which I'm kicking > > off now. > > > > Do you want us to land this for you now? > > Yep, suits me. Although commit-bot says otherwise. Maybe that's nothing? Yeah, the win_drmemory_light bot seems to be struggling a bit, but that is most likely an unrelated flake.
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/1823763004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1823763004/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Keep reads within buffer in AnalysisUpdateNeon(). BUG=webrtc:5631 ========== to ========== Keep reads within buffer in AnalysisUpdateNeon(). BUG=webrtc:5631 Committed: https://crrev.com/de81ea8524de328cf0e76280291485f55f74f5db Cr-Commit-Position: refs/heads/master@{#12228} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/de81ea8524de328cf0e76280291485f55f74f5db Cr-Commit-Position: refs/heads/master@{#12228} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
