Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2433393002:
Avoids invalid copy of audio buffer to task queue (Closed)
|
Created:
4 years, 2 months ago by henrika_webrtc Modified:
4 years, 2 months ago CC:
webrtc-reviews_webrtc.org Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAvoids invalid copy of audio buffer to task queue.
Now does level estimate on the audio threads to avoid complex
copying of audio data to task queue. The old implementation could
also crash due to unclear ownership of the audio buffer.
BUG=webrtc:6569
R=kwiberg@webrtc.org
Committed: https://crrev.com/3355f6d6f5bdcd529705228c6f48612ecc0f0a62
Cr-Commit-Position: refs/heads/master@{#14720}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Feedback from kwiberg@ #
Total comments: 4
Patch Set 3 : nit #
Messages
Total messages: 15 (8 generated)
Description was changed from ========== Avoids invalid copy of audio buffer to task queue BUG= ========== to ========== Avoids invalid copy of audio buffer to task queue. Now does level estimate on the audio threads to avoid complex copying of audio data to task queue. The old implementation could also crash due to unclear ownership of the audio buffer. BUG=webrtc:6569 ==========
henrika@webrtc.org changed reviewers: + kwiberg@webrtc.org
As discussed. PTAL
https://codereview.webrtc.org/2433393002/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2433393002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:254: if (++rec_stat_count_ == 50) { Either DCHECK that the value isn't more than 50, or test >= 50 here. (Or both.) https://codereview.webrtc.org/2433393002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:257: static_cast<int16_t*>(const_cast<void*>(audio_buffer)), size); You shouldn't need to cast away the constness. WebRtcSpl_MaxAbsValueW16 takes a const array argument. https://codereview.webrtc.org/2433393002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:262: // non-zero two times per second approximately. Don't you mean rec_stat_count_? Or that max_abs will only change ~twice a second? https://codereview.webrtc.org/2433393002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:342: if (++play_stat_count_ == 50) { DCHECK and/or >= https://codereview.webrtc.org/2433393002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:345: reinterpret_cast<int16_t*>(play_buffer_.data()), size); You use static_cast in the other loop. Please be consistent. https://codereview.webrtc.org/2433393002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:350: // non-zero two times per second approximately. Comment wrong here too?
Thanks! PTAL https://codereview.chromium.org/2433393002/diff/1/webrtc/modules/audio_device... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.chromium.org/2433393002/diff/1/webrtc/modules/audio_device... webrtc/modules/audio_device/audio_device_buffer.cc:254: if (++rec_stat_count_ == 50) { Sure thing but should it really be needed? https://codereview.chromium.org/2433393002/diff/1/webrtc/modules/audio_device... webrtc/modules/audio_device/audio_device_buffer.cc:262: // non-zero two times per second approximately. I mean max_abs. Let me elaborate. https://codereview.chromium.org/2433393002/diff/1/webrtc/modules/audio_device... webrtc/modules/audio_device/audio_device_buffer.cc:342: if (++play_stat_count_ == 50) { On 2016/10/20 17:02:59, kwiberg-webrtc wrote: > DCHECK and/or >= Done. https://codereview.chromium.org/2433393002/diff/1/webrtc/modules/audio_device... webrtc/modules/audio_device/audio_device_buffer.cc:345: reinterpret_cast<int16_t*>(play_buffer_.data()), size); Will fix. https://codereview.chromium.org/2433393002/diff/1/webrtc/modules/audio_device... webrtc/modules/audio_device/audio_device_buffer.cc:350: // non-zero two times per second approximately. Actually no but let me explain better.
lgtm https://codereview.chromium.org/2433393002/diff/1/webrtc/modules/audio_device... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.chromium.org/2433393002/diff/1/webrtc/modules/audio_device... webrtc/modules/audio_device/audio_device_buffer.cc:254: if (++rec_stat_count_ == 50) { On 2016/10/21 08:40:22, henrika_webrtc wrote: > Sure thing but should it really be needed? Well, no. DCHECKs are, by design, never needed, since they only verify things that you already know to be true. :-) Think of it as machine-checkable documentation. https://codereview.chromium.org/2433393002/diff/20001/webrtc/modules/audio_de... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.chromium.org/2433393002/diff/20001/webrtc/modules/audio_de... webrtc/modules/audio_device/audio_device_buffer.cc:255: RTC_DCHECK_LE(rec_stat_count_, 50); It doesn't make a lot of difference, but you could place the DCHECK one line up, before the if. (And make it _LT in that case.) https://codereview.chromium.org/2433393002/diff/20001/webrtc/modules/audio_de... webrtc/modules/audio_device/audio_device_buffer.cc:265: // approximately two times per second and can then change the stats. Ah, OK, I see now. Thanks.
henrika@chromium.org changed reviewers: + henrika@chromium.org
https://codereview.chromium.org/2433393002/diff/20001/webrtc/modules/audio_de... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.chromium.org/2433393002/diff/20001/webrtc/modules/audio_de... webrtc/modules/audio_device/audio_device_buffer.cc:255: RTC_DCHECK_LE(rec_stat_count_, 50); On 2016/10/21 09:04:43, kwiberg-webrtc wrote: > It doesn't make a lot of difference, but you could place the DCHECK one line up, > before the if. (And make it _LT in that case.) Done. https://codereview.chromium.org/2433393002/diff/20001/webrtc/modules/audio_de... webrtc/modules/audio_device/audio_device_buffer.cc:265: // approximately two times per second and can then change the stats. On 2016/10/21 09:04:43, kwiberg-webrtc wrote: > Ah, OK, I see now. Thanks. Acknowledged.
The CQ bit was checked by henrika@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org Link to the patchset: https://codereview.chromium.org/2433393002/#ps40001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by henrika@webrtc.org
Description was changed from ========== Avoids invalid copy of audio buffer to task queue. Now does level estimate on the audio threads to avoid complex copying of audio data to task queue. The old implementation could also crash due to unclear ownership of the audio buffer. BUG=webrtc:6569 ========== to ========== Avoids invalid copy of audio buffer to task queue. Now does level estimate on the audio threads to avoid complex copying of audio data to task queue. The old implementation could also crash due to unclear ownership of the audio buffer. BUG=webrtc:6569 R=kwiberg@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/3355f6d6f5bdcd529705228c6... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as 3355f6d6f5bdcd529705228c6f48612ecc0f0a62 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Avoids invalid copy of audio buffer to task queue. Now does level estimate on the audio threads to avoid complex copying of audio data to task queue. The old implementation could also crash due to unclear ownership of the audio buffer. BUG=webrtc:6569 R=kwiberg@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/3355f6d6f5bdcd529705228c6... ========== to ========== Avoids invalid copy of audio buffer to task queue. Now does level estimate on the audio threads to avoid complex copying of audio data to task queue. The old implementation could also crash due to unclear ownership of the audio buffer. BUG=webrtc:6569 R=kwiberg@webrtc.org Committed: https://crrev.com/3355f6d6f5bdcd529705228c6f48612ecc0f0a62 Cr-Commit-Position: refs/heads/master@{#14720} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
