Modifies AudioInputCallback::OnData and use media::AudioBus instead of plain byte vector

Modifies AudioInputCallback::OnData and use media::AudioBus instead of plain byte vector

BUG=375155
TEST=WebRTC and WebSpeech demos

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277794

[henrika (OOO until Aug 14), 2014-06-03 14:43:51]

https://codereview.chromium.org/314713002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/314713002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/audio_input_sync_writer.cc:71:
data->ToInterleaved(data->frames(), params_.bits_per_sample() / 8,
Can we do something here to prevent what is done currently:

Native (interleaved) -> FromInterleaved to AudioBus -> ToInterleaved -> Shared
Memory -> FromInterleaved to AudioBus

[DaleCurtis, 2014-06-03 21:03:05]

https://codereview.chromium.org/314713002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/314713002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/audio_input_sync_writer.cc:71:
data->ToInterleaved(data->frames(), params_.bits_per_sample() / 8,
On 2014/06/03 14:43:51, henrika wrote:
> Can we do something here to prevent what is done currently:
> 
> Native (interleaved) -> FromInterleaved to AudioBus -> ToInterleaved -> Shared
> Memory -> FromInterleaved to AudioBus

Just use AudioBus::WrapMemory() and size the shared memory appropriately: N *
(AudioBus::CalculateMemorySize() + sizeof(AudioInputBuffer)) where N == number
of segments.  You'll still need at least a CopyTo() with this model, which I
think is fine.

You could go further, but it'd be a bit difficult and weird.  Essentially you'd
need to have the AudioSyncWriter construct the AudioBus using WrapMemory() and
then hand it all the way down to the input stream.  The input stream would then
be writing directly into the shared memory itself.

https://codereview.chromium.org/314713002/diff/1/media/audio/alsa/alsa_input.cc
File media/audio/alsa/alsa_input.cc (right):

https://codereview.chromium.org/314713002/diff/1/media/audio/alsa/alsa_input....
media/audio/alsa/alsa_input.cc:215: callback_->OnData(this, audio_bus_.get(),
hardware_delay_bytes,
Formatting is wrong, run this section through clang-format.  Or just run git cl
format.

[henrika (OOO until Aug 14), 2014-06-04 07:50:23]

https://codereview.chromium.org/314713002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/314713002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/audio_input_sync_writer.cc:71:
data->ToInterleaved(data->frames(), params_.bits_per_sample() / 8,
Thanks Dale! I will use your first idea and upload new version. Really
appreciate your fast reply.

https://codereview.chromium.org/314713002/diff/1/media/audio/alsa/alsa_input.cc
File media/audio/alsa/alsa_input.cc (right):

https://codereview.chromium.org/314713002/diff/1/media/audio/alsa/alsa_input....
media/audio/alsa/alsa_input.cc:215: callback_->OnData(this, audio_bus_.get(),
hardware_delay_bytes,
Thx. Done. CL was not cleaned up. Mainly wanted to ask about better wrapping.
But thanks anyhow ;-)

[henrika (OOO until Aug 14), 2014-06-04 11:57:07]

Dale, could you please check the essential parts (AudioBus::WrapMemory) in #3. I
hope I got it correct. Not 100% on how I create the local versions of
AudioBus:es. I can't come up with any other solution.

[henrika (OOO until Aug 14), 2014-06-04 11:58:50]

https://codereview.chromium.org/314713002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/314713002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_renderer_host.cc:298: const
uint32 segment_size = (sizeof(media::AudioInputBufferParameters) +
..and here

https://codereview.chromium.org/314713002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/314713002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.cc:75: // Create
audio bus wrapping existing block of shared memory and fill it up
Dale, please check here.

https://codereview.chromium.org/314713002/diff/40001/media/audio/audio_input_...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/314713002/diff/40001/media/audio/audio_input_...
media/audio/audio_input_device.cc:300: // Create audio bus wrapping existing
block of shared memory.
...and here.

[no longer working on chromium, 2014-06-04 12:38:13]

Thanks Henrik, it looks really good, but it seems some more work is needed on
Mac and windows.

https://codereview.chromium.org/314713002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/314713002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/audio_input_sync_writer.cc:71:
data->ToInterleaved(data->frames(), params_.bits_per_sample() / 8,
On 2014/06/03 21:03:06, DaleCurtis wrote:
> On 2014/06/03 14:43:51, henrika wrote:
> > Can we do something here to prevent what is done currently:
> > 
> > Native (interleaved) -> FromInterleaved to AudioBus -> ToInterleaved ->
Shared
> > Memory -> FromInterleaved to AudioBus
> 
> Just use AudioBus::WrapMemory() and size the shared memory appropriately: N *
> (AudioBus::CalculateMemorySize() + sizeof(AudioInputBuffer)) where N == number
> of segments.  You'll still need at least a CopyTo() with this model, which I
> think is fine.
> 
> You could go further, but it'd be a bit difficult and weird.  Essentially
you'd
> need to have the AudioSyncWriter construct the AudioBus using WrapMemory() and
> then hand it all the way down to the input stream.  The input stream would
then
> be writing directly into the shared memory itself.

This is a bit hard, on the input side, we create a vector of share memories
(size is [shared_memory_segment_count_]) to pass the data to the renderer, it is
because the capture callbacks can come in a row with irregular intervals and we
simply cache the data to the share memory if the renderer has not consumed it
yet.

https://codereview.chromium.org/314713002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/314713002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.cc:72:
buffer->params.size = media::AudioBus::CalculateMemorySize(params_);
probably it is worthy caching the value into a static variable or a member.

https://codereview.chromium.org/314713002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/media/audio_input_sync_writer.h (right):

https://codereview.chromium.org/314713002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.h:73:
media::AudioParameters params_;
nit, const

https://codereview.chromium.org/314713002/diff/40001/media/audio/audio_input_...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/314713002/diff/40001/media/audio/audio_input_...
media/audio/audio_input_controller.cc:448: // TODO(henrika): fix WebSpeech as
well....
looks like you need to fix it to land this CL.

[henrika (OOO until Aug 14), 2014-06-04 13:52:32]

Minor changes based on quick review by xians.

https://codereview.chromium.org/314713002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/314713002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.cc:72:
buffer->params.size = media::AudioBus::CalculateMemorySize(params_);
On 2014/06/04 12:38:12, xians1 wrote:
> probably it is worthy caching the value into a static variable or a member.

Done.

https://codereview.chromium.org/314713002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/media/audio_input_sync_writer.h (right):

https://codereview.chromium.org/314713002/diff/40001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.h:73:
media::AudioParameters params_;
On 2014/06/04 12:38:13, xians1 wrote:
> nit, const

Done.

https://codereview.chromium.org/314713002/diff/40001/media/audio/audio_input_...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/314713002/diff/40001/media/audio/audio_input_...
media/audio/audio_input_controller.cc:448: // TODO(henrika): fix WebSpeech as
well....
Yes. I just want the core stuff to be OK first.

[DaleCurtis, 2014-06-04 17:39:17]

WrapMemory changes look good modulo pre-construction.

https://codereview.chromium.org/314713002/diff/60001/content/browser/renderer...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/314713002/diff/60001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.cc:26: DVLOG(1) <<
__FUNCTION__;
Please drop these DVLOGs before submission.

https://codereview.chromium.org/314713002/diff/60001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_sync_writer.cc:81:
scoped_ptr<media::AudioBus> audio_bus =
I'd just pre-create all these busses based on the segment size during
construction, that way you're not repeatedly reallocating these objects. You can
transfer the AudioBus objects via release() into a ScopedVector.

https://codereview.chromium.org/314713002/diff/60001/media/audio/audio_input_...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/314713002/diff/60001/media/audio/audio_input_...
media/audio/audio_input_device.cc:301: scoped_ptr<AudioBus> audio_bus =
Again create these during construction.

[henrika (OOO until Aug 14), 2014-06-10 15:08:34]

https://codereview.chromium.org/314713002/diff/140001/media/audio/audio_low_l...
File media/audio/audio_low_latency_input_output_unittest.cc (right):

https://codereview.chromium.org/314713002/diff/140001/media/audio/audio_low_l...
media/audio/audio_low_latency_input_output_unittest.cc:213: // Store the
captured audio packet in a seekable media buffer.
Will fix.

[henrika (OOO until Aug 14), 2014-06-10 15:12:25]

Dale and Shijing.

I have now:
- fixed the memory handling and ensured that we pre-allocate audio buses during
construction for all shmem segments;
- added AudioBus support for WebSpeech (saves one FromInterleaved call,
jiiippi);
- fixed fake audio input to use AudioBus as well.

If you are OK with the existing style I can finalize for the remaining platforms
as well. Should not be much left.

So, PTAL ;-)

[henrika (OOO until Aug 14), 2014-06-10 15:14:06]

https://codereview.chromium.org/314713002/diff/160001/media/audio/audio_low_l...
File media/audio/audio_low_latency_input_output_unittest.cc (right):

https://codereview.chromium.org/314713002/diff/160001/media/audio/audio_low_l...
media/audio/audio_low_latency_input_output_unittest.cc:87: /*
This complete test is actually disabled. I only use it for testing here but my
goal is to fix it in this CL as well.

[DaleCurtis, 2014-06-10 17:03:15]

Style looks fine to me, let me know when it's ready for full review.

https://codereview.chromium.org/314713002/diff/160001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/314713002/diff/160001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host.cc:22: using
media::AudioBus;
I'd drop this, you don't see much content code doing this.

[henrika (OOO until Aug 14), 2014-06-12 12:00:33]

https://codereview.chromium.org/314713002/diff/160001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/314713002/diff/160001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host.cc:22: using
media::AudioBus;
On 2014/06/10 17:03:14, DaleCurtis wrote:
> I'd drop this, you don't see much content code doing this.

Done.

[henrika (OOO until Aug 14), 2014-06-13 13:27:07]

Dale and Shijing, I am almost done now. All that remains is to fix it for Chrome
OS and some minor changes for (DISABLED) unit tests for Android. Note that much
code is duplicated for each platform, hence the CL "looks worse than it is".

I have tested performance on Linux, Android, Windows and Mac.

Many thanks for reviewing ;-)

[henrika (OOO until Aug 14), 2014-06-13 13:38:03]

...and it looks like I will have to fix content_unittests as well. Should be
simple and same for all platforms.

[henrika (OOO until Aug 14), 2014-06-16 15:49:23]

Done more changes; should be more or less complete now.

Dale and Shijing; I would really appreciate a review since I will be OOO for two
week starting next week and I don't want this patch to turn old. Thanks ;-)

[DaleCurtis, 2014-06-17 01:03:50]

lgtm % Qs.

https://codereview.chromium.org/314713002/diff/320001/media/audio/audio_low_l...
File media/audio/audio_low_latency_input_output_unittest.cc (right):

https://codereview.chromium.org/314713002/diff/320001/media/audio/audio_low_l...
media/audio/audio_low_latency_input_output_unittest.cc:210: #if (false)
What is this actually testing? It seems to just add the data to the
SeekableBuffer.  Seems you could just replace this with an AudioFifo.

https://codereview.chromium.org/314713002/diff/320001/media/audio/pulse/audio...
File media/audio/pulse/audio_manager_pulse.cc (right):

https://codereview.chromium.org/314713002/diff/320001/media/audio/pulse/audio...
media/audio/pulse/audio_manager_pulse.cc:43: static const int
kDefaultInputBufferSize = 512;
Should this be in this CL?

[no longer working on chromium, 2014-06-17 06:59:58]

lgtm with nits.

https://codereview.chromium.org/314713002/diff/320001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/314713002/diff/320001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_sync_writer.cc:8: #include
<algorithm>
?? remove

https://codereview.chromium.org/314713002/diff/320001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_sync_writer.cc:52: void
AudioInputSyncWriter::Write(const media::AudioBus* data,
you don't need to change it if you want to keep the current code, perferably it
should be const media::AudioBus& for the input code

https://codereview.chromium.org/314713002/diff/320001/media/audio/android/aud...
File media/audio/android/audio_record_input.cc (right):

https://codereview.chromium.org/314713002/diff/320001/media/audio/android/aud...
media/audio/android/audio_record_input.cc:21:
bytes_per_sample_(params.bits_per_sample() / 8) {
nit, this does not need to be a member, move it to a static variable in OnData()

[henrika (OOO until Aug 14), 2014-06-17 11:39:44]

Thanks guys!

https://codereview.chromium.org/314713002/diff/320001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/314713002/diff/320001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_sync_writer.cc:8: #include
<algorithm>
On 2014/06/17 06:59:58, xians1 wrote:
> ?? remove

Done.

https://codereview.chromium.org/314713002/diff/320001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_sync_writer.cc:52: void
AudioInputSyncWriter::Write(const media::AudioBus* data,
Discussed it with Dale and we decided to go for existing. Lots of changes to
make changes now.... ;-)

https://codereview.chromium.org/314713002/diff/320001/media/audio/android/aud...
File media/audio/android/audio_record_input.cc (right):

https://codereview.chromium.org/314713002/diff/320001/media/audio/android/aud...
media/audio/android/audio_record_input.cc:21:
bytes_per_sample_(params.bits_per_sample() / 8) {
I actually need it here since we don't store the audio parameters.

https://codereview.chromium.org/314713002/diff/320001/media/audio/audio_low_l...
File media/audio/audio_low_latency_input_output_unittest.cc (right):

https://codereview.chromium.org/314713002/diff/320001/media/audio/audio_low_l...
media/audio/audio_low_latency_input_output_unittest.cc:210: #if (false)
The buffer is read out in the OnMoreData, hence you will hear the audio in
loopback. I will add a TODO to replace with AudioFifo. Would like to land this
one first.

PS, this file is not even included in media.gyp.

https://codereview.chromium.org/314713002/diff/320001/media/audio/pulse/audio...
File media/audio/pulse/audio_manager_pulse.cc (right):

https://codereview.chromium.org/314713002/diff/320001/media/audio/pulse/audio...
media/audio/pulse/audio_manager_pulse.cc:43: static const int
kDefaultInputBufferSize = 512;
It is not used by any client today but you are correct. I will remove it just in
case.

[henrika (OOO until Aug 14), 2014-06-17 11:41:48]

The CQ bit was checked by henrika@chromium.org

[commit-bot: I haz the power, 2014-06-17 11:43:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/henrika@chromium.org/314713002/340001

[henrika (OOO until Aug 14), 2014-06-17 14:57:56]

The CQ bit was checked by henrika@chromium.org

[commit-bot: I haz the power, 2014-06-17 14:58:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/henrika@chromium.org/314713002/380001

[commit-bot: I haz the power, 2014-06-17 17:27:15]

Change committed as 277794

[henrika (OOO until Aug 14), 2014-06-18 08:24:23]

This CL has been reverted.

https://codereview.chromium.org/335343004/

[henrika (OOO until Aug 14), 2014-06-18 08:52:47]

See
http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Chrome...
for details.