Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2894873002:
Improved audio buffer handling for iOS (Closed)
|
Created:
3 years, 7 months ago by henrika_webrtc Modified:
3 years, 6 months ago Reviewers:
kwiberg-webrtc CC:
webrtc-reviews_webrtc.org Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionImproved audio buffer handling for iOS.
This change:
Reduces complexity for audio playout by removing a redundant memcopy in the output audio path.
Adds support for iOS simulator for playout since we now allow the audio layer to ask for different sizes
of audio buffers at each callback. Real iOS devices always asks for the same size, simulators does not.
This change comes without any new cost for real devices.
BUG=b/37580746
Review-Url: https://codereview.webrtc.org/2894873002
Cr-Commit-Position: refs/heads/master@{#18321}
Committed: https://chromium.googlesource.com/external/webrtc/+/bb6f7524bab7c839f8281e1db1d0e7f6cfaeeba6
Patch Set 1 #Patch Set 2 : First version with working playout audio #Patch Set 3 : Now uses ArrayView #Patch Set 4 : Playout parts is done #Patch Set 5 : nit #Patch Set 6 : Removed unit test changes #Patch Set 7 : Added capacity parameter #
Total comments: 19
Patch Set 8 : Feedback from kwiberg@ #Patch Set 9 : nit #Patch Set 10 : Added support for Android and did some minor changes for the recoridng side as well #Patch Set 11 : iOS fix #Patch Set 12 : nit #
Total comments: 3
Patch Set 13 : Final changes #Patch Set 14 : rebased #
Messages
Total messages: 52 (34 generated)
Description was changed from ========== Improved audio buffer handling for iOS nit nit Test version that works (debug) initial tests BUG= ========== to ========== Improved audio buffer handling for iOS ==========
Description was changed from ========== Improved audio buffer handling for iOS ========== to ========== Improved audio buffer handling for iOS b/37580746 ==========
Description was changed from ========== Improved audio buffer handling for iOS b/37580746 ========== to ========== Improved audio buffer handling for iOS. This change: Reduces complexity for audio playout by removing a redundant memcopy in the output audio path. Adds support for iOS simulator for playout since we now allow the audio layer to ask for different sizes of audio buffers at each callback. Real iOS devices always asks for the same size, simulators does not. This change comes without any new cost for real devices. b/37580746 ==========
henrika@webrtc.org changed reviewers: + kwiberg@webrtc.org
PTAL
lgtm, but comments https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/fine_audio_buffer.cc (right): https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/fine_audio_buffer.cc:30: playout_buffer_(0, capacity) { You don't specify an initial capacity for record_buffer_. Should you? https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/fine_audio_buffer.h (right): https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/fine_audio_buffer.h:50: // elements is specified by |audio.buffer.size()|. One . should be _ https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/fine_audio_buffer.h:51: void GetPlayoutData(rtc::ArrayView<int8_t> audio_buffer); Can it ever be the case that you're not able to fill audio_buffer? https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/fine_audio_buffer_unittest.cc (right): https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/fine_audio_buffer_unittest.cc:122: out_buffer.reset(new int8_t[kFrameSizeBytes]); Replace these two lines with e.g. int8_t out_buffer[kFrameSizeBytes]; ... https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/fine_audio_buffer_unittest.cc:127: rtc::ArrayView<int8_t>(out_buffer.get(), kFrameSizeBytes)); ...and you'll be able to say just fine_buffer.GetPlayoutData(out_buffer); here. Oh, and because stack allocations are essentially free, move the allocation into the loop body, just before first use.
https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/fine_audio_buffer.cc (right): https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/fine_audio_buffer.cc:30: playout_buffer_(0, capacity) { This CL is focusing on playout side since that is where we have experienced issues lately. I will take it one more round and check if we can do similar improvements for recording side. But I would like to do that in a separate CL. https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/fine_audio_buffer.h (right): https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/fine_audio_buffer.h:50: // elements is specified by |audio.buffer.size()|. On 2017/05/29 04:09:02, kwiberg-webrtc wrote: > One . should be _ Done. https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/fine_audio_buffer.h:51: void GetPlayoutData(rtc::ArrayView<int8_t> audio_buffer); No. We ask native WebRTC until we get enough and if no audio exists, WebRTC will return silence instead. https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/fine_audio_buffer_unittest.cc (right): https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/fine_audio_buffer_unittest.cc:122: out_buffer.reset(new int8_t[kFrameSizeBytes]); On 2017/05/29 04:09:02, kwiberg-webrtc wrote: > Replace these two lines with e.g. > > int8_t out_buffer[kFrameSizeBytes]; > > ... Done. https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/fine_audio_buffer_unittest.cc:127: rtc::ArrayView<int8_t>(out_buffer.get(), kFrameSizeBytes)); On 2017/05/29 04:09:02, kwiberg-webrtc wrote: > ...and you'll be able to say just > > fine_buffer.GetPlayoutData(out_buffer); > > here. > > Oh, and because stack allocations are essentially free, move the allocation into > the loop body, just before first use. Done.
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/2894873002/#ps140001 (title: "Feedback from kwiberg@")
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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/17484)
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/2894873002/#ps160001 (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 commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/17485)
Description was changed from ========== Improved audio buffer handling for iOS. This change: Reduces complexity for audio playout by removing a redundant memcopy in the output audio path. Adds support for iOS simulator for playout since we now allow the audio layer to ask for different sizes of audio buffers at each callback. Real iOS devices always asks for the same size, simulators does not. This change comes without any new cost for real devices. b/37580746 ========== to ========== Improved audio buffer handling for iOS. This change: Reduces complexity for audio playout by removing a redundant memcopy in the output audio path. Adds support for iOS simulator for playout since we now allow the audio layer to ask for different sizes of audio buffers at each callback. Real iOS devices always asks for the same size, simulators does not. This change comes without any new cost for real devices. BUG=b/37580746 ==========
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: android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...) linux32_arm_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux32_arm_dbg/builds/34)
https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/fine_audio_buffer.cc (right): https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/fine_audio_buffer.cc:30: playout_buffer_(0, capacity) { On 2017/05/29 10:33:51, henrika_webrtc wrote: > This CL is focusing on playout side since that is where we have experienced > issues lately. > I will take it one more round and check if we can do similar improvements for > recording side. > But I would like to do that in a separate CL. Acknowledged. It might make sense to call the argument playout_buffer_capacity or something---"capacity" is probably too broad. https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/fine_audio_buffer.h (right): https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/fine_audio_buffer.h:51: void GetPlayoutData(rtc::ArrayView<int8_t> audio_buffer); On 2017/05/29 10:33:51, henrika_webrtc wrote: > No. We ask native WebRTC until we get enough and if no audio exists, WebRTC will > return silence instead. OK. Document this? https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/fine_audio_buffer_unittest.cc (right): https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/fine_audio_buffer_unittest.cc:127: rtc::ArrayView<int8_t>(out_buffer.get(), kFrameSizeBytes)); On 2017/05/29 10:33:51, henrika_webrtc wrote: > On 2017/05/29 04:09:02, kwiberg-webrtc wrote: > > ...and you'll be able to say just > > > > fine_buffer.GetPlayoutData(out_buffer); > > > > here. > > > > Oh, and because stack allocations are essentially free, move the allocation > into > > the loop body, just before first use. > > Done. You don't appear to have actually done this. :-)
The CQ bit was checked by henrika@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/...
Done ;-) https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/fine_audio_buffer.cc (right): https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/fine_audio_buffer.cc:30: playout_buffer_(0, capacity) { Actually. I kept "capacity" and ensured that the recording side is affected in the same way. If not, construction of an object which only does recording would be confusing. I also modified the signature for the recording side to rtc::ArrayView. https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/fine_audio_buffer.h (right): https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/fine_audio_buffer.h:51: void GetPlayoutData(rtc::ArrayView<int8_t> audio_buffer); On 2017/05/29 11:07:01, kwiberg-webrtc wrote: > On 2017/05/29 10:33:51, henrika_webrtc wrote: > > No. We ask native WebRTC until we get enough and if no audio exists, WebRTC > will > > return silence instead. > > OK. Document this? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/20434) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/24893)
The CQ bit was checked by henrika@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: Try jobs failed on following builders: win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/25857)
The CQ bit was checked by henrika@webrtc.org to run a CQ dry run
https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/fine_audio_buffer_unittest.cc (right): https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/fine_audio_buffer_unittest.cc:127: rtc::ArrayView<int8_t>(out_buffer.get(), kFrameSizeBytes)); Actually. I don't really understand your comment. The is no conversion between a plain array and ArrayView afaik. I tried it and it does not build.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
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: Try jobs failed on following builders: win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/9381)
https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/fine_audio_buffer_unittest.cc (right): https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/fine_audio_buffer_unittest.cc:127: rtc::ArrayView<int8_t>(out_buffer.get(), kFrameSizeBytes)); On 2017/05/29 15:01:34, henrika_webrtc wrote: > Actually. I don't really understand your comment. The is no conversion between a > plain array and ArrayView afaik. I tried it and it does not build. There's supposed to be one. See e.g. https://cs.chromium.org/chromium/src/third_party/webrtc/base/array_view_unitt... What's the error message? https://codereview.webrtc.org/2894873002/diff/220001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/android/opensles_player.cc (right): https://codereview.webrtc.org/2894873002/diff/220001/webrtc/modules/audio_dev... webrtc/modules/audio_device/android/opensles_player.cc:215: 2 * buffer_size_in_bytes)); Not necessary right now, but you can avoid the "new" and use MakeUnique instead: fine_audio_buffer_ = MakeUnique<FineAudioBuffer>(audio_device_buffer_, ...); In the future, we'll probably make an effort to avoid "new" just like we're already avoiding "delete". https://codereview.webrtc.org/2894873002/diff/220001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/fine_audio_buffer.cc (right): https://codereview.webrtc.org/2894873002/diff/220001/webrtc/modules/audio_dev... webrtc/modules/audio_device/fine_audio_buffer.cc:77: record_buffer_.AppendData(audio_buffer.data(), audio_buffer.size()); You should be able to do just record_buffer_.AppendData(audio_buffer); thanks to this overload: https://cs.chromium.org/chromium/src/third_party/webrtc/base/buffer.h?l=258 https://codereview.webrtc.org/2894873002/diff/220001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/fine_audio_buffer.h (right): https://codereview.webrtc.org/2894873002/diff/220001/webrtc/modules/audio_dev... webrtc/modules/audio_device/fine_audio_buffer.h:63: void DeliverRecordedData(rtc::ArrayView<const int8_t> audio_buffer, Excellent!
https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/fine_audio_buffer_unittest.cc (right): https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/fine_audio_buffer_unittest.cc:127: rtc::ArrayView<int8_t>(out_buffer.get(), kFrameSizeBytes)); I should have explained better. The issue is that the size or xx_buffer is given by an input parameter. One test tests when the size is less than 10ms and another when it is larger that 10ms. I did not come up with any other solution than using unique_ptr. Hope that is OK. Thanks for all valuable feedback.
The CQ bit was checked by henrika@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 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/2894873002/#ps260001 (title: "rebased")
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": 260001, "attempt_start_ts": 1496134775824430,
"parent_rev": "d7620582342bb896b766bb306135b12aefbeb12c", "commit_rev":
"bb6f7524bab7c839f8281e1db1d0e7f6cfaeeba6"}
Message was sent while issue was closed.
Description was changed from ========== Improved audio buffer handling for iOS. This change: Reduces complexity for audio playout by removing a redundant memcopy in the output audio path. Adds support for iOS simulator for playout since we now allow the audio layer to ask for different sizes of audio buffers at each callback. Real iOS devices always asks for the same size, simulators does not. This change comes without any new cost for real devices. BUG=b/37580746 ========== to ========== Improved audio buffer handling for iOS. This change: Reduces complexity for audio playout by removing a redundant memcopy in the output audio path. Adds support for iOS simulator for playout since we now allow the audio layer to ask for different sizes of audio buffers at each callback. Real iOS devices always asks for the same size, simulators does not. This change comes without any new cost for real devices. BUG=b/37580746 Review-Url: https://codereview.webrtc.org/2894873002 Cr-Commit-Position: refs/heads/master@{#18321} Committed: https://chromium.googlesource.com/external/webrtc/+/bb6f7524bab7c839f8281e1db... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/external/webrtc/+/bb6f7524bab7c839f8281e1db...
Message was sent while issue was closed.
https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/fine_audio_buffer_unittest.cc (right): https://codereview.webrtc.org/2894873002/diff/120001/webrtc/modules/audio_dev... webrtc/modules/audio_device/fine_audio_buffer_unittest.cc:127: rtc::ArrayView<int8_t>(out_buffer.get(), kFrameSizeBytes)); On 2017/05/30 08:37:15, henrika_webrtc wrote: > I should have explained better. The issue is that the size or xx_buffer is given > by an input parameter. One test tests when the size is less than 10ms and > another when it is larger that 10ms. I did not come up with any other solution > than using unique_ptr. Hope that is OK. Thanks for all valuable feedback. OK. The name "kFrameSizeBytes" confused me into thinking it was a constant... You can get away with less manual twiddling in these situations if you use e.g. std::vector or rtc::Buffer instead: rtc::Buffer out_buffer(kFrameSizeBytes); ... fine_buffer.GetPlayoutData(out_buffer); But no need to change it now.
Message was sent while issue was closed.
Will do next time. Did not write this test originally and wanted to do an as small change as possible. See your point. Thanks! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
