changed the input low latency input impl to use client's requested buffer size on Mac

changed the input low latency input impl to use client's native buffer size on Mac.  This allows all the low low latency clients to use their requested buffer size.



BUG=365044
TEST=content_unittests --gtest_filter="*MediaStreamAudioProcessorTest*"
webrtc and speech demo tests:
apprtc.appspot.com/?&debug=loopback
https://www.google.com/intl/en/chrome/demos/speech.html
bots

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

[no longer working on chromium, 2014-06-26 20:01:37]

Dale, could you please review this patch?

Thanks,
SX

[DaleCurtis, 2014-06-27 00:00:42]

I think this should work.  lgtm as is, or if you want to make the improvements I
suggested, I'll review again afterward.

https://codereview.chromium.org/358823002/diff/40001/media/audio/mac/audio_lo...
File media/audio/mac/audio_low_latency_input_mac.cc (right):

https://codereview.chromium.org/358823002/diff/40001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:513:
audio_wrapper_->FromInterleaved(
You should be able to set it up such that the AUHAL is capturing Float planar
data, which will allow you to avoid AudioBus construction and deinterleaving. 
You could just use a wrapper w/ channel pointers that you push into  the FIFO.

https://codereview.chromium.org/358823002/diff/40001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:519: sink_->OnData(this,
audio_wrapper_.get(), capture_delay_bytes,
clang-format? I think this style is frowned upon.

https://codereview.chromium.org/358823002/diff/40001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:529:
fifo_->Consume(audio_bus_.get(), 0, audio_bus_->frames());
We could consider making a FIFO::Peek() object which allows callers to directly
use the contents of the FIFO through an AudioBus wrapper.  Thus avoiding this
output copy.  I think that'd be useful on the output side as well.  I can take a
look at that in another CL, or feel free to do it here.

[no longer working on chromium, 2014-06-27 20:25:37]

https://codereview.chromium.org/358823002/diff/40001/media/audio/mac/audio_lo...
File media/audio/mac/audio_low_latency_input_mac.cc (right):

https://codereview.chromium.org/358823002/diff/40001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:513:
audio_wrapper_->FromInterleaved(
On 2014/06/27 00:00:42, DaleCurtis wrote:
> You should be able to set it up such that the AUHAL is capturing Float planar
> data, which will allow you to avoid AudioBus construction and deinterleaving. 
> You could just use a wrapper w/ channel pointers that you push into  the FIFO.

I guess you meant opening the device with format:
kAudioFormatFlagsNativeFloatPacked | kLinearPCMFormatFlagIsNonInterleaved;
As how it is for the output.

It is actually my first attempt, but AudioUnitRender() call fails with error
code -50 with the float format, I played around the float format a bit but could
not make it work.

I am wondering if the input ever support the float deinterleaved format.

https://codereview.chromium.org/358823002/diff/40001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:519: sink_->OnData(this,
audio_wrapper_.get(), capture_delay_bytes,
On 2014/06/27 00:00:42, DaleCurtis wrote:
> clang-format? I think this style is frowned upon.

Done.

https://codereview.chromium.org/358823002/diff/40001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:529:
fifo_->Consume(audio_bus_.get(), 0, audio_bus_->frames());
On 2014/06/27 00:00:42, DaleCurtis wrote:
> We could consider making a FIFO::Peek() object which allows callers to
directly
> use the contents of the FIFO through an AudioBus wrapper.  Thus avoiding this
> output copy.  I think that'd be useful on the output side as well.  I can take
a
> look at that in another CL, or feel free to do it here.

It is great if we can avoid the copying.

I took a quick look at the AudioFifo code, and it seems to me that the
read/write memory is wrapped internally, it is a bit unclear to me how we can
use a wrapper to get a linear memory out from the Fifo.
please guide me through if I miss something here.

[DaleCurtis, 2014-06-28 00:45:50]

https://codereview.chromium.org/358823002/diff/40001/media/audio/mac/audio_lo...
File media/audio/mac/audio_low_latency_input_mac.cc (right):

https://codereview.chromium.org/358823002/diff/40001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:513:
audio_wrapper_->FromInterleaved(
On 2014/06/27 20:25:37, xians1 wrote:
> On 2014/06/27 00:00:42, DaleCurtis wrote:
> > You should be able to set it up such that the AUHAL is capturing Float
planar
> > data, which will allow you to avoid AudioBus construction and
deinterleaving. 
> > You could just use a wrapper w/ channel pointers that you push into  the
FIFO.
> 
> I guess you meant opening the device with format:
> kAudioFormatFlagsNativeFloatPacked | kLinearPCMFormatFlagIsNonInterleaved;
> As how it is for the output.
> 
> It is actually my first attempt, but AudioUnitRender() call fails with error
> code -50 with the float format, I played around the float format a bit but
could
> not make it work.
> 
> I am wondering if the input ever support the float deinterleaved format.
> 

Ah, that's unfortunate.

https://codereview.chromium.org/358823002/diff/40001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:529:
fifo_->Consume(audio_bus_.get(), 0, audio_bus_->frames());
On 2014/06/27 20:25:37, xians1 wrote:
> On 2014/06/27 00:00:42, DaleCurtis wrote:
> > We could consider making a FIFO::Peek() object which allows callers to
> directly
> > use the contents of the FIFO through an AudioBus wrapper.  Thus avoiding
this
> > output copy.  I think that'd be useful on the output side as well.  I can
take
> a
> > look at that in another CL, or feel free to do it here.
> 
> It is great if we can avoid the copying.
> 
> I took a quick look at the AudioFifo code, and it seems to me that the
> read/write memory is wrapped internally, it is a bit unclear to me how we can
> use a wrapper to get a linear memory out from the Fifo.
> please guide me through if I miss something here.
> 

Ah I forgot about the circular buffer properties, you can probably avoid the
copy some of the time, but not all the times in that case.  If you write a Peek
method() you could choose to only use it if Peek() returns enough frames:

const AudioBus* Peek(int max_frames) {
  audio_bus_wrapper_->set_frames(std::min(max_frames, max_frames_ - read_pos_));
  for(int i = 0; i < audio_bus_->channels(); ++i)
     audio_bus_wrapper_->SetChannelData(audio_bus_->channel(i) + read_pos_);
  return audio_bus_wrapper_.get();
}

Then if returned AudioBus* has enough linear frames you can use it. Otherwise
call the normal Consume() w/ copy. So long as there are a couple multiples of
frames_per_buffer() the chances of a linear view are pretty good.

Another idea: Since it seems you'll have to deinterleave the audio, what you can
instead do is keep N AudioBus objects and rotate between them, deinterleaving
directly into each one and passing it to the OnData() callback.  Where N is the
ceil((frames_per_buffer + requested_frames) / frames_per_buffer).  You can use
FromInterleavedPartial() to accomplish this.

[no longer working on chromium, 2014-06-29 20:30:00]

Thanks Dale, as I replied in the inline comment, I will write a new FIFO in a
separate CL because the optimization can be done on all platforms.

Best,
SX

https://codereview.chromium.org/358823002/diff/40001/media/audio/mac/audio_lo...
File media/audio/mac/audio_low_latency_input_mac.cc (right):

https://codereview.chromium.org/358823002/diff/40001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:529:
fifo_->Consume(audio_bus_.get(), 0, audio_bus_->frames());
On 2014/06/28 00:45:50, DaleCurtis wrote:
> On 2014/06/27 20:25:37, xians1 wrote:
> > On 2014/06/27 00:00:42, DaleCurtis wrote:
> > > We could consider making a FIFO::Peek() object which allows callers to
> > directly
> > > use the contents of the FIFO through an AudioBus wrapper.  Thus avoiding
> this
> > > output copy.  I think that'd be useful on the output side as well.  I can
> take
> > a
> > > look at that in another CL, or feel free to do it here.
> > 
> > It is great if we can avoid the copying.
> > 
> > I took a quick look at the AudioFifo code, and it seems to me that the
> > read/write memory is wrapped internally, it is a bit unclear to me how we
can
> > use a wrapper to get a linear memory out from the Fifo.
> > please guide me through if I miss something here.
> > 
> 
> Ah I forgot about the circular buffer properties, you can probably avoid the
> copy some of the time, but not all the times in that case.  If you write a
Peek
> method() you could choose to only use it if Peek() returns enough frames:
> 
> const AudioBus* Peek(int max_frames) {
>   audio_bus_wrapper_->set_frames(std::min(max_frames, max_frames_ -
read_pos_));
>   for(int i = 0; i < audio_bus_->channels(); ++i)
>      audio_bus_wrapper_->SetChannelData(audio_bus_->channel(i) + read_pos_);
>   return audio_bus_wrapper_.get();
> }
> 
> Then if returned AudioBus* has enough linear frames you can use it. Otherwise
> call the normal Consume() w/ copy. So long as there are a couple multiples of
> frames_per_buffer() the chances of a linear view are pretty good.
> 
> Another idea: Since it seems you'll have to deinterleave the audio, what you
can
> instead do is keep N AudioBus objects and rotate between them, deinterleaving
> directly into each one and passing it to the OnData() callback.  Where N is
the
> ceil((frames_per_buffer + requested_frames) / frames_per_buffer).  You can use
> FromInterleavedPartial() to accomplish this.

Both are good ideas, and I like the second one particularly since it might be
able to save two copies.
But I prefer writing a new FIFO implementation and taking this as separate
optimization on  the input code of all  platforms.
So I am going to land this CL as it is now.

[no longer working on chromium, 2014-06-29 20:30:03]

The CQ bit was checked by xians@chromium.org

[commit-bot: I haz the power, 2014-06-29 20:30:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xians@chromium.org/358823002/60001

[commit-bot: I haz the power, 2014-06-30 06:41:26]

Change committed as 280567

[DaleCurtis, 2014-06-30 21:00:29]

https://codereview.chromium.org/358823002/diff/40001/media/audio/mac/audio_lo...
File media/audio/mac/audio_low_latency_input_mac.cc (right):

https://codereview.chromium.org/358823002/diff/40001/media/audio/mac/audio_lo...
media/audio/mac/audio_low_latency_input_mac.cc:529:
fifo_->Consume(audio_bus_.get(), 0, audio_bus_->frames());
On 2014/06/29 20:30:00, xians1 wrote:
> On 2014/06/28 00:45:50, DaleCurtis wrote:
> > On 2014/06/27 20:25:37, xians1 wrote:
> > > On 2014/06/27 00:00:42, DaleCurtis wrote:
> > > > We could consider making a FIFO::Peek() object which allows callers to
> > > directly
> > > > use the contents of the FIFO through an AudioBus wrapper.  Thus avoiding
> > this
> > > > output copy.  I think that'd be useful on the output side as well.  I
can
> > take
> > > a
> > > > look at that in another CL, or feel free to do it here.
> > > 
> > > It is great if we can avoid the copying.
> > > 
> > > I took a quick look at the AudioFifo code, and it seems to me that the
> > > read/write memory is wrapped internally, it is a bit unclear to me how we
> can
> > > use a wrapper to get a linear memory out from the Fifo.
> > > please guide me through if I miss something here.
> > > 
> > 
> > Ah I forgot about the circular buffer properties, you can probably avoid the
> > copy some of the time, but not all the times in that case.  If you write a
> Peek
> > method() you could choose to only use it if Peek() returns enough frames:
> > 
> > const AudioBus* Peek(int max_frames) {
> >   audio_bus_wrapper_->set_frames(std::min(max_frames, max_frames_ -
> read_pos_));
> >   for(int i = 0; i < audio_bus_->channels(); ++i)
> >      audio_bus_wrapper_->SetChannelData(audio_bus_->channel(i) + read_pos_);
> >   return audio_bus_wrapper_.get();
> > }
> > 
> > Then if returned AudioBus* has enough linear frames you can use it.
Otherwise
> > call the normal Consume() w/ copy. So long as there are a couple multiples
of
> > frames_per_buffer() the chances of a linear view are pretty good.
> > 
> > Another idea: Since it seems you'll have to deinterleave the audio, what you
> can
> > instead do is keep N AudioBus objects and rotate between them,
deinterleaving
> > directly into each one and passing it to the OnData() callback.  Where N is
> the
> > ceil((frames_per_buffer + requested_frames) / frames_per_buffer).  You can
use
> > FromInterleavedPartial() to accomplish this.
> 
> Both are good ideas, and I like the second one particularly since it might be
> able to save two copies.
> But I prefer writing a new FIFO implementation and taking this as separate
> optimization on  the input code of all  platforms.
> So I am going to land this CL as it is now.

I think that's a great idea.  Thanks! I don't think any of our current AudioFIFO
users actually rely on the circular buffer logic, so you could theoretically
replace that one with a multi-bus solution which would allow copy-removal for
all current users.