Simplifies FineAudioBuffer by using rtc::Buffer

Simplifies FineAudioBuffer by using rtc::Buffer


BUG=NONE

Review-Url: https://codereview.webrtc.org/2715963002
Cr-Commit-Position: refs/heads/master@{#16864}
Committed: https://chromium.googlesource.com/external/webrtc/+/b3ebc1aa598ed19af238c5b53cb1b19c13a2c72d

[henrika_webrtc, 2017-02-24 14:43:43]

Description was changed from

==========
Simplifies FineAudioBuffer by using rtc::Buffer

Simplifies FineAudioBuffer by using rtc::Buffer

BUG=
==========

to

==========
Simplifies FineAudioBuffer by using rtc::Buffer


BUG=NONE
==========

[henrika_webrtc, 2017-02-24 14:43:43]

henrika@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[henrika_webrtc, 2017-02-24 14:44:31]

Trying to improve the playout side as well using rtc::Buffer.
What do you think?

[henrika_webrtc, 2017-02-24 14:44:35]

Trying to improve the playout side as well using rtc::Buffer.
What do you think?

[henrika_webrtc, 2017-02-24 15:49:17]

The CQ bit was checked by henrika@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 15:49:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[henrika_webrtc, 2017-02-24 15:52:15]

The CQ bit was checked by henrika@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 15:52:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-24 17:09:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 17:09:27]

Dry run: This issue passed the CQ dry run.

[henrika_webrtc, 2017-02-26 19:34:12]

The CQ bit was checked by henrika@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-26 19:34:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-26 19:36:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-26 19:36:47]

Dry run: Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/17993)

[kwiberg-webrtc, 2017-02-27 09:45:35]

Looks correct, but I have a suggestion.

https://codereview.webrtc.org/2715963002/diff/40001/webrtc/modules/audio_devi...
File webrtc/modules/audio_device/fine_audio_buffer.cc (right):

https://codereview.webrtc.org/2715963002/diff/40001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/fine_audio_buffer.cc:53: int samples_per_channel =
const?

https://codereview.webrtc.org/2715963002/diff/40001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/fine_audio_buffer.cc:57:
playout_buffer_.AppendData(buffer_10_ms_.get(), bytes_per_10_ms_);
Hmm. The only use of buffer_10_ms_ is as temp storage in this loop, right? Why
not append the data to playout_buffer_ directly? rtc::Buffer has an AppendData
overload that seems to fit:

  const int bytes_written = playout_buffer_.AppendData(samples_per_10_ms_,
      [&](rtc::ArrayView<int8_t> buf){
    return device_buffer_->GetPlayoutData(buf.data());
  });
  RTC_DCHECK_EQ(bytes_per_10_ms_, bytes_written);

(I've probably mixed up total number of bytes with bytes per channel in this
example, so nick with care.)

From the docs:

  // Append at most |max_elements| to the end of the buffer, using the function
  // |setter|, which should have the following signature:
  //   size_t setter(ArrayView<U> view)
  // |setter| is given an appropriately typed ArrayView of the area in which to
  // write the data (i.e. starting at the former end of the buffer) and should
  // return the number of elements actually written. This number must be <=
  // |max_elements|.
  template <typename U = T,
            typename F,
            typename std::enable_if<
                internal::BufferCompat<T, U>::value>::type* = nullptr>
  size_t AppendData(size_t max_elements, F&& setter) {

[henrika_webrtc, 2017-02-27 12:32:28]

Thanks for the proposals.

PTAL

https://codereview.webrtc.org/2715963002/diff/40001/webrtc/modules/audio_devi...
File webrtc/modules/audio_device/fine_audio_buffer.cc (right):

https://codereview.webrtc.org/2715963002/diff/40001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/fine_audio_buffer.cc:53: int samples_per_channel =
On 2017/02/27 09:45:35, kwiberg-webrtc wrote:
> const?

Done.

https://codereview.webrtc.org/2715963002/diff/40001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/fine_audio_buffer.cc:57:
playout_buffer_.AppendData(buffer_10_ms_.get(), bytes_per_10_ms_);
Correct. Only used as temp storage. Will try to implement your proposal. Thanks.

[henrika_webrtc, 2017-02-27 12:32:40]

The CQ bit was checked by henrika@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-27 12:32:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-27 12:42:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-27 12:42:18]

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/6791)

[kwiberg-webrtc, 2017-02-27 12:45:57]

lgtm

https://codereview.webrtc.org/2715963002/diff/60001/webrtc/modules/audio_devi...
File webrtc/modules/audio_device/fine_audio_buffer.cc (right):

https://codereview.webrtc.org/2715963002/diff/60001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/fine_audio_buffer.cc:54: // end of the buffer. This
approach avoids need for any temporary buffer.
You can probably delete all but the first sentence of this comment. And because
of the DCHECK on line 63, you can remove the "at most" too.

https://codereview.webrtc.org/2715963002/diff/60001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/fine_audio_buffer.cc:61: return (sizeof(int16_t) *
samples_per_channel);
Remove the outer parentheses.

[henrika_webrtc, 2017-02-27 12:52:43]

https://codereview.webrtc.org/2715963002/diff/60001/webrtc/modules/audio_devi...
File webrtc/modules/audio_device/fine_audio_buffer.cc (right):

https://codereview.webrtc.org/2715963002/diff/60001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/fine_audio_buffer.cc:54: // end of the buffer. This
approach avoids need for any temporary buffer.
On 2017/02/27 12:45:57, kwiberg-webrtc wrote:
> You can probably delete all but the first sentence of this comment. And
because
> of the DCHECK on line 63, you can remove the "at most" too.

Done.

https://codereview.webrtc.org/2715963002/diff/60001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/fine_audio_buffer.cc:61: return (sizeof(int16_t) *
samples_per_channel);
On 2017/02/27 12:45:57, kwiberg-webrtc wrote:
> Remove the outer parentheses.

Done.

[henrika_webrtc, 2017-02-27 12:52:51]

The CQ bit was checked by henrika@webrtc.org

[henrika_webrtc, 2017-02-27 12:52:52]

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/2715963002/#ps80001 (title:
"final nits")

[commit-bot: I haz the power, 2017-02-27 12:53:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-27 13:14:18]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488199971683920,
"parent_rev": "686aa37382428bc667adff8cd6895674cae4e5fd", "commit_rev":
"b3ebc1aa598ed19af238c5b53cb1b19c13a2c72d"}

[commit-bot: I haz the power, 2017-02-27 13:14:20]

Description was changed from

==========
Simplifies FineAudioBuffer by using rtc::Buffer


BUG=NONE
==========

to

==========
Simplifies FineAudioBuffer by using rtc::Buffer


BUG=NONE

Review-Url: https://codereview.webrtc.org/2715963002
Cr-Commit-Position: refs/heads/master@{#16864}
Committed:
https://chromium.googlesource.com/external/webrtc/+/b3ebc1aa598ed19af238c5b53...
==========

[commit-bot: I haz the power, 2017-02-27 13:14:21]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/b3ebc1aa598ed19af238c5b53...