AudioDeviceBuffer now uses 16-bit buffers

AudioDeviceBuffer now uses 16-bit buffers

BUG=webrtc:6560
Committed: https://crrev.com/51e9608f0194af090c83d9a3dd9efb43a39f8e6f
Cr-Commit-Position: refs/heads/master@{#15008}

[henrika_webrtc, 2016-11-08 11:20:38]

Description was changed from

==========
AudioDeviceBuffer now uses 16-bit buffers

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

to

==========
AudioDeviceBuffer now uses 16-bit buffers

BUG=webrtc:6560
==========

[henrika_webrtc, 2016-11-08 11:20:38]

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

[henrika_webrtc, 2016-11-08 11:25:41]

As requested in a previous CL, the rtc::Buffer members are now of 16-bit size
instead (rtc::BufferT<uint16_t>). It results in a more clean implement ion where
the contained size can be used in several places without conversions.

Note that, the WebRTC audio stack is currently designed for 16-bit PCM audio
samples only.

[henrika_webrtc, 2016-11-08 11:25:53]

PTAL

[henrika_webrtc, 2016-11-08 11:32:56]

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

[commit-bot: I haz the power, 2016-11-08 11:32:59]

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, 2016-11-08 11:39:09]

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

[commit-bot: I haz the power, 2016-11-08 11:39:10]

Dry run: Try jobs failed on following builders:
  win_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/15852)

[kwiberg-webrtc, 2016-11-08 12:19:59]

https://codereview.webrtc.org/2482053003/diff/1/webrtc/modules/audio_device/a...
File webrtc/modules/audio_device/audio_device_buffer.cc (right):

https://codereview.webrtc.org/2482053003/diff/1/webrtc/modules/audio_device/a...
webrtc/modules/audio_device/audio_device_buffer.cc:306: const size_t
packet_size_in_frames = sample_frame_size * num_samples;
No... Since a frame consists of 1 or more samples (the same number as the number
of channels), the packet size in frames should be the packet size in samples
DIVIDED BY the number of samples per frame. It looks like what you're doing here
is converting the number of samples per packet per channel ("num_samples") into
the total number of samples per packet ("packet_size_in_frames"). If this is the
case, the variables are confusingly named, and I would suggest something like

  num_samples           -> samples_per_channel
  sample_frame_size     -> remove, and just use rec_channels_
  packet_size_in_frames -> total_samples = samples_per_channel * rec_channels_

Or is this a case of the reviewer being confused?

https://codereview.webrtc.org/2482053003/diff/1/webrtc/modules/audio_device/a...
webrtc/modules/audio_device/audio_device_buffer.cc:321: reinterpret_cast<const
int16_t*>(rec_buffer_.data()),
You shouldn't need this cast anymore, since rec_buffer_.data() returns int16_t*.

https://codereview.webrtc.org/2482053003/diff/1/webrtc/modules/audio_device/a...
webrtc/modules/audio_device/audio_device_buffer.cc:369: const size_t
packet_size_in_frames = sample_frame_size * num_samples;
Same comment as above about the variable names.

https://codereview.webrtc.org/2482053003/diff/1/webrtc/modules/audio_device/a...
webrtc/modules/audio_device/audio_device_buffer.cc:401: reinterpret_cast<const
int16_t*>(play_buffer_.data()),
Remove cast?

[henrika_webrtc, 2016-11-08 12:33:59]

PTAL

https://codereview.webrtc.org/2482053003/diff/1/webrtc/modules/audio_device/a...
File webrtc/modules/audio_device/audio_device_buffer.cc (right):

https://codereview.webrtc.org/2482053003/diff/1/webrtc/modules/audio_device/a...
webrtc/modules/audio_device/audio_device_buffer.cc:306: const size_t
packet_size_in_frames = sample_frame_size * num_samples;
I am using a standard notation from how audio samples are denoted.
It is used in e.g Core Audio on MacOS X.

In Core Audio, the following definitions apply:

An audio stream is a continuous series of data that represents a sound, such as
a song.

A channel is a discrete track of monophonic audio. A monophonic stream has one
channel; a stereo stream has two channels.

A sample is single numerical value for a single audio channel in an audio
stream.

A frame is a collection of time-coincident samples. For instance, a linear PCM
stereo sound file has two samples per frame, one for the left channel and one
for the right channel.

A packet is a collection of one or more contiguous frames. A packet defines the
smallest meaningful set of frames for a given audio data format, and is the
smallest data unit for which time can be measured. In linear PCM audio, a packet
holds a single frame. In compressed formats, it typically holds more; in some
formats, the number of frames per packet varies.

The sample rate for a stream is the number of frames per second of uncompressed
(or, for compressed formats, the equivalent in decompressed) audio.

Still confused?

In any case, introducing the term 'samples_per_channel' is IMHO plain wrong.

https://codereview.webrtc.org/2482053003/diff/1/webrtc/modules/audio_device/a...
webrtc/modules/audio_device/audio_device_buffer.cc:321: reinterpret_cast<const
int16_t*>(rec_buffer_.data()),
On 2016/11/08 12:19:58, kwiberg-webrtc wrote:
> You shouldn't need this cast anymore, since rec_buffer_.data() returns
int16_t*.

Acknowledged.

https://codereview.webrtc.org/2482053003/diff/1/webrtc/modules/audio_device/a...
webrtc/modules/audio_device/audio_device_buffer.cc:369: const size_t
packet_size_in_frames = sample_frame_size * num_samples;
See above

https://codereview.webrtc.org/2482053003/diff/1/webrtc/modules/audio_device/a...
webrtc/modules/audio_device/audio_device_buffer.cc:401: reinterpret_cast<const
int16_t*>(play_buffer_.data()),
On 2016/11/08 12:19:58, kwiberg-webrtc wrote:
> Remove cast?

Done.

[kwiberg-webrtc, 2016-11-08 13:01:55]

https://codereview.webrtc.org/2482053003/diff/1/webrtc/modules/audio_device/a...
File webrtc/modules/audio_device/audio_device_buffer.cc (right):

https://codereview.webrtc.org/2482053003/diff/1/webrtc/modules/audio_device/a...
webrtc/modules/audio_device/audio_device_buffer.cc:306: const size_t
packet_size_in_frames = sample_frame_size * num_samples;
On 2016/11/08 12:33:59, henrika_webrtc wrote:
> I am using a standard notation from how audio samples are denoted.
> It is used in e.g Core Audio on MacOS X.
> 
> In Core Audio, the following definitions apply:
> 
> An audio stream is a continuous series of data that represents a
> sound, such as a song.

OK.

> A channel is a discrete track of monophonic audio. A monophonic
> stream has one channel; a stereo stream has two channels.

OK. That's the same definition I was using.

> A sample is single numerical value for a single audio channel in an
> audio stream.

OK, same definition I was using. So in this case, a sample is a single
int16_t.

> A frame is a collection of time-coincident samples. For instance, a
> linear PCM stereo sound file has two samples per frame, one for the
> left channel and one for the right channel.

OK. That's the same definition I was using.

> A packet is a collection of one or more contiguous frames.

OK. That's the same definition I was using.

> A packet defines the smallest meaningful set of frames for a given
> audio data format, and is the smallest data unit for which time can
> be measured.

Sounds reasonable.

> In linear PCM audio, a packet holds a single frame. In compressed
> formats, it typically holds more; in some formats, the number of
> frames per packet varies.

Sounds reasonable.

> The sample rate for a stream is the number of frames per second of
> uncompressed (or, for compressed formats, the equivalent in
> decompressed) audio.

OK. That's the same definition I was using.

> Still confused?

Well, yes... on line 305,

  const size_t sample_frame_size = rec_channels_;

you're computing the number of samples per frame (or, phrased another
way, the size of a frame in samples). "sample_frame_size" is not a
great name for this---I would have picked "samples_per_frame", or not
made a separate variable for this---but I don't find it actively
misleading.

On line 306,

  const size_t packet_size_in_frames = sample_frame_size * num_samples;

you seem to be computing the number of samples per packet by
multiplying the number of samples per frame (sample_frame_size) by the
number of frames per packet (num_samples). If that's the case, there
are two problems here: "packet_size_in_frames" is a bad name for the
number of samples in a packet. Also, this name seems to imply that
what was passed to SetRecordedBuffer was a single packet, but since
this is uncompressed PCM, a packet should by the definition you gave
contain just 1 frame. To me, it looks like you're just computing the
total number of samples the caller passed to us, no matter how many
frames is in a packet, so "total_samples" or "num_samples" or
something would be a better name.

> In any case, introducing the term 'samples_per_channel' is IMHO
> plain wrong.

The number of samples per channel is per definition the same as the
number of frames. If you assume the reader knows what a "frame" is,
then calling it "num_frames" would be a good idea. If you don't assume
such knowledge, calling it "samples_per_channel" would seem like a
reasonable alternative.

Do I still seem confused? I'm guessing one of us must be, since we
agree on all the definitions... :-)

[henrika_webrtc, 2016-11-08 14:05:00]

PTAL

[kwiberg-webrtc, 2016-11-08 14:54:40]

Excellent! lgtm

Thanks for putting up with all my complaints...

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

https://codereview.webrtc.org/2482053003/diff/60001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/audio_device_buffer.cc:343: const size_t
frame_size_in_bytes = rec_channels_ * sizeof(int16_t);
Good. Or "bytes_per_frame", which means the same thing but is slightly shorter.

[henrika_webrtc, 2016-11-08 15:31:02]

Thanks!

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

https://codereview.webrtc.org/2482053003/diff/60001/webrtc/modules/audio_devi...
webrtc/modules/audio_device/audio_device_buffer.cc:343: const size_t
frame_size_in_bytes = rec_channels_ * sizeof(int16_t);
On 2016/11/08 14:54:40, kwiberg-webrtc wrote:
> Good. Or "bytes_per_frame", which means the same thing but is slightly
shorter.

Done.

[henrika_webrtc, 2016-11-08 15:31:07]

The CQ bit was checked by henrika@webrtc.org

[henrika_webrtc, 2016-11-08 15:31:08]

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/2482053003/#ps80001 (title:
"nit")

[commit-bot: I haz the power, 2016-11-08 15:31:11]

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, 2016-11-08 15:40:42]

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

[commit-bot: I haz the power, 2016-11-08 15:40:43]

Try jobs failed on following builders:
  mac_asan on master.tryserver.webrtc (JOB_FAILED, no build URL)

[henrika_webrtc, 2016-11-09 17:33:12]

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

[commit-bot: I haz the power, 2016-11-09 17:33:14]

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, 2016-11-09 17:46:05]

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

[commit-bot: I haz the power, 2016-11-09 17:46:06]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)

[henrika_webrtc, 2016-11-09 18:17:07]

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

[commit-bot: I haz the power, 2016-11-09 18:17:11]

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, 2016-11-09 18:40:09]

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

[commit-bot: I haz the power, 2016-11-09 18:40:10]

Dry run: This issue passed the CQ dry run.

[henrika_webrtc, 2016-11-10 08:38:48]

I did spend some time fixing the last nits yesterday. I had missed some details
for non-mono cases but they are fixed now.
My plan was to land using your existing OK. Ping me if you have concerns and I
can make the changes after.

[henrika_webrtc, 2016-11-10 08:38:54]

The CQ bit was checked by henrika@webrtc.org

[henrika_webrtc, 2016-11-10 08:38:55]

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/2482053003/#ps120001 (title:
"Final fix for Android")

[commit-bot: I haz the power, 2016-11-10 08:39:03]

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, 2016-11-10 08:40:39]

Description was changed from

==========
AudioDeviceBuffer now uses 16-bit buffers

BUG=webrtc:6560
==========

to

==========
AudioDeviceBuffer now uses 16-bit buffers

BUG=webrtc:6560
==========

[commit-bot: I haz the power, 2016-11-10 08:40:40]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-10 08:40:51]

Description was changed from

==========
AudioDeviceBuffer now uses 16-bit buffers

BUG=webrtc:6560
==========

to

==========
AudioDeviceBuffer now uses 16-bit buffers

BUG=webrtc:6560

Committed: https://crrev.com/51e9608f0194af090c83d9a3dd9efb43a39f8e6f
Cr-Commit-Position: refs/heads/master@{#15008}
==========

[commit-bot: I haz the power, 2016-11-10 08:40:52]

Patchset 7 (id:??) landed as
https://crrev.com/51e9608f0194af090c83d9a3dd9efb43a39f8e6f
Cr-Commit-Position: refs/heads/master@{#15008}