Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2333273002:
Now uses rtc::Buffer in AudioDeviceBuffer (Closed)
|
Created:
4 years, 3 months ago by henrika_webrtc Modified:
4 years, 2 months ago Reviewers:
kwiberg-webrtc CC:
webrtc-reviews_webrtc.org Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionNow uses rtc::Buffer in AudioDeviceBuffer.
The main goal of this CL is to remove old buffer handling using static arrays
and switch to the improved rtc::Buffer class instead.
By doing so, we can remove some members (since Buffer maintains them instead) and
do some additional cleanup.
This CL also fixes some minor style issues and improves the locking mechanism.
Finally, AudioDeviceBuffer::SetRecordingChannel() is deprecated since it has never been
used and is not included in any test.
BUG=NONE
Committed: https://crrev.com/5588a13fe7c2768bf6a6b9dc6492e8076db02369
Cr-Commit-Position: refs/heads/master@{#14661}
Patch Set 1 #
Total comments: 24
Patch Set 2 : rebased #Patch Set 3 : Now uses lamdas #Patch Set 4 : Minor changes before asking for new review #Patch Set 5 : nit #Patch Set 6 : nit #Patch Set 7 : Fixes broken unittests #
Messages
Total messages: 21 (9 generated)
Description was changed from ========== Now uses rtc::Buffer in AudioDeviceBuffer BUG= ========== to ========== Now uses rtc::Buffer in AudioDeviceBuffer. The main goal of this CL is to remove old buffer handling using static arrays and switch to the improved rtc::Buffer class instead. By doing so, we can remove some members (since Buffer maintains them instead) and do some additional cleanup. This CL also fixes some minor style issues and improves the locking mechanism. BUG=NONE ==========
henrika@webrtc.org changed reviewers: + kwiberg@webrtc.org
PTAL
Description was changed from ========== Now uses rtc::Buffer in AudioDeviceBuffer. The main goal of this CL is to remove old buffer handling using static arrays and switch to the improved rtc::Buffer class instead. By doing so, we can remove some members (since Buffer maintains them instead) and do some additional cleanup. This CL also fixes some minor style issues and improves the locking mechanism. BUG=NONE ========== to ========== Now uses rtc::Buffer in AudioDeviceBuffer. The main goal of this CL is to remove old buffer handling using static arrays and switch to the improved rtc::Buffer class instead. By doing so, we can remove some members (since Buffer maintains them instead) and do some additional cleanup. This CL also fixes some minor style issues and improves the locking mechanism. Finally, AudioDeviceBuffer::SetRecordingChannel() is deprecated since it has never been used and is not included in any test. BUG=NONE ==========
https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:241: } An elegant way to do this, which avoids the use of uninitialized variables, is const size_t rec_bytes_per_sample = [&]{ rtc::CritScope lock(&lock_); return rec_bytes_per_sample_; }(); (This creates a lambda that you immediately call without storing it anywhere.) https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:243: size_t size_in_bytes = num_samples * rec_bytes_per_sample; const https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:262: } You can use the lambda trick here too. https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:303: } lambda https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:308: size_t size_in_bytes = num_samples * play_bytes_per_sample; const https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:338: } lambda https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:339: memcpy(audio_buffer, &play_buffer_[0], play_buffer_.size()); play_buffer_.data() instead of &play_buffer_[0]; both work, but the former is simpler and is also well-defined when the buffer is empty. https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/audio_device_buffer.h (right): https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.h:103: rtc::CriticalSection lock_cb_; This class uses none of our very cool and very handy compiler-checked lock annotations (available for both mutexes and thread checkers). It's a good idea to always use them in new code, and start using them in old code when you have to touch locking code, because it's too easy to make mistakes when verifying correctness manually. Example CL here, if you need it: https://codereview.webrtc.org/2286343002/ Since this CL reduces the scope that's protected by locks in several member functions, I'd strongly recommend that you add annotations (for all member variables; annotating just a subset is seldom a good compromise). Because that's going to touch a fair number of lines, it may make sense to do it in a separate CL. https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.h:131: rtc::Buffer play_buffer_; The dynamic resizing is obvious from the type, so just e.g. rtc::Buffer rec_buffer_; // Recorded audio samples. rtc::Buffer play_buffer_; // Audio samples to be played out. ought to be sufficient. Also, from the way these are used in the .cc file, it seems like they should contain a bunch of int16_t rather than bytes---i.e., they should be rtc::BufferT<int16_t>.
Thanks. Uploading answers and comments. Will try to make time and implement the changes today as well. https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:241: } Thanks, will change. Must admit that I have not worked with lambdas. What does the [&] stand for? https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:243: size_t size_in_bytes = num_samples * rec_bytes_per_sample; On 2016/09/14 08:39:04, kwiberg-webrtc wrote: > const Acknowledged. https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:262: } On 2016/09/14 08:39:04, kwiberg-webrtc wrote: > You can use the lambda trick here too. Acknowledged. https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:303: } On 2016/09/14 08:39:04, kwiberg-webrtc wrote: > lambda Acknowledged. https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:308: size_t size_in_bytes = num_samples * play_bytes_per_sample; On 2016/09/14 08:39:04, kwiberg-webrtc wrote: > const Acknowledged. https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:338: } On 2016/09/14 08:39:04, kwiberg-webrtc wrote: > lambda Acknowledged. https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:339: memcpy(audio_buffer, &play_buffer_[0], play_buffer_.size()); On 2016/09/14 08:39:04, kwiberg-webrtc wrote: > play_buffer_.data() instead of &play_buffer_[0]; both work, but the former is > simpler and is also well-defined when the buffer is empty. Acknowledged. https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/audio_device_buffer.h (right): https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.h:103: rtc::CriticalSection lock_cb_; Would love to improve this area. Hope you are OK with if I do that in a separate CLs. Feels better to use a title where it is clear what the change does. Thanks for the reference ;-) PS I might also be able to remove some of these locks based on knowledge how we call the APIs. https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.h:131: rtc::Buffer play_buffer_; This class has always worked with bytes but I must agree that int16 feels better. The declarations for input buffers uses void* and same for output (not perfect I know but it would touch many files to change that). Let me try to change to int16 and see what that looks like. I think it can be beneficial.
https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:241: } On 2016/09/14 10:26:53, henrika_webrtc wrote: > Thanks, will change. Must admit that I have not worked with lambdas. What does > the [&] stand for? It's the capture list---the list of variables of the surrounding scope that's made available to the code in the lambda. This particular list means "capture everything, by reference"; this is typically what you want for lambdas that don't outlive the scope in which they were created. https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/audio_device_buffer.h (right): https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.h:103: rtc::CriticalSection lock_cb_; On 2016/09/14 10:26:54, henrika_webrtc wrote: > Would love to improve this area. Hope you are OK with if I do that in a separate > CLs. Feels better to use a title where it is clear what the change does. Thanks > for the reference ;-) > > PS I might also be able to remove some of these locks based on knowledge how we > call the APIs. Acknowledged. https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.h:131: rtc::Buffer play_buffer_; On 2016/09/14 10:26:54, henrika_webrtc wrote: > This class has always worked with bytes but I must agree that int16 feels > better. The declarations for input buffers uses void* and same for output (not > perfect I know but it would touch many files to change that). Let me try to > change to int16 and see what that looks like. I think it can be beneficial. Acknowledged.
Sorry for long delay here. Rebased and added lambdas as proposed. Will see if I can use 16-bit buffer next. No need for review yet. https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:241: } On 2016/09/14 11:17:17, kwiberg-webrtc wrote: > On 2016/09/14 10:26:53, henrika_webrtc wrote: > > Thanks, will change. Must admit that I have not worked with lambdas. What does > > the [&] stand for? > > It's the capture list---the list of variables of the surrounding scope that's > made available to the code in the lambda. This particular list means "capture > everything, by reference"; this is typically what you want for lambdas that > don't outlive the scope in which they were created. Acknowledged. https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.cc:241: } On 2016/09/14 08:39:04, kwiberg-webrtc wrote: > An elegant way to do this, which avoids the use of uninitialized variables, is > > const size_t rec_bytes_per_sample = [&]{ > rtc::CritScope lock(&lock_); > return rec_bytes_per_sample_; > }(); > > (This creates a lambda that you immediately call without storing it anywhere.) Done.
Sorry for the delay here. Now asking for one more round. Keeping it simple and will break out two new CLs once the existing has landed. One for "lock utilities" and one for 16-bit audio buffers. PTAL https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/audio_device_buffer.h (right): https://codereview.webrtc.org/2333273002/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/audio_device_buffer.h:131: rtc::Buffer play_buffer_; Tried it but it is rather tricky actually since several interfaces (that also are used in Chrome) assumes byte vectors. I would like to skip this part now and come back with a change in a separate CL with a more clear heading. Thanks.
lgtm. Doing those two follow-up CLs sounds good to me.
The CQ bit was checked by henrika@webrtc.org
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
Try jobs failed on following builders: mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/15218)
The CQ bit was checked by henrika@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/2333273002/#ps120001 (title: "Fixes broken unittests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Now uses rtc::Buffer in AudioDeviceBuffer. The main goal of this CL is to remove old buffer handling using static arrays and switch to the improved rtc::Buffer class instead. By doing so, we can remove some members (since Buffer maintains them instead) and do some additional cleanup. This CL also fixes some minor style issues and improves the locking mechanism. Finally, AudioDeviceBuffer::SetRecordingChannel() is deprecated since it has never been used and is not included in any test. BUG=NONE ========== to ========== Now uses rtc::Buffer in AudioDeviceBuffer. The main goal of this CL is to remove old buffer handling using static arrays and switch to the improved rtc::Buffer class instead. By doing so, we can remove some members (since Buffer maintains them instead) and do some additional cleanup. This CL also fixes some minor style issues and improves the locking mechanism. Finally, AudioDeviceBuffer::SetRecordingChannel() is deprecated since it has never been used and is not included in any test. BUG=NONE ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Now uses rtc::Buffer in AudioDeviceBuffer. The main goal of this CL is to remove old buffer handling using static arrays and switch to the improved rtc::Buffer class instead. By doing so, we can remove some members (since Buffer maintains them instead) and do some additional cleanup. This CL also fixes some minor style issues and improves the locking mechanism. Finally, AudioDeviceBuffer::SetRecordingChannel() is deprecated since it has never been used and is not included in any test. BUG=NONE ========== to ========== Now uses rtc::Buffer in AudioDeviceBuffer. The main goal of this CL is to remove old buffer handling using static arrays and switch to the improved rtc::Buffer class instead. By doing so, we can remove some members (since Buffer maintains them instead) and do some additional cleanup. This CL also fixes some minor style issues and improves the locking mechanism. Finally, AudioDeviceBuffer::SetRecordingChannel() is deprecated since it has never been used and is not included in any test. BUG=NONE Committed: https://crrev.com/5588a13fe7c2768bf6a6b9dc6492e8076db02369 Cr-Commit-Position: refs/heads/master@{#14661} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/5588a13fe7c2768bf6a6b9dc6492e8076db02369 Cr-Commit-Position: refs/heads/master@{#14661} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
