Fix the maximum native sample rate in AudioProcessing

Fix the maximum native sample rate in AudioProcessing

BUG=webrtc:4983
R=andrew@webrtc.org, henrik.lundin@webrtc.org

Committed: https://crrev.com/cdfe20bfc1146030aa59eb37635fd2fbcecd6cdb
Cr-Commit-Position: refs/heads/master@{#10037}

[aluebs-webrtc, 2015-09-11 18:00:10]

aluebs@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org

[aluebs-webrtc, 2015-09-11 18:01:21]

Henrik, could you please have a quick look at this tiny fix for the 48kHz
support?

[Andrew MacDonald, 2015-09-11 18:06:23]

andrew@webrtc.org changed reviewers:
+ andrew@webrtc.org

[Andrew MacDonald, 2015-09-11 18:06:23]

https://codereview.webrtc.org/1338833002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/include/audio_processing.h (right):

https://codereview.webrtc.org/1338833002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/include/audio_processing.h:131: static const int
kAudioProcMaxNativeSampleRateHz = 48000;
Looks like this should only affect:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

Have you tested that it works fine? It _looks_ like it should but my TODO there
worries me a bit :) I assume that was written before voice engine used the
SincResampler.

Assuming everything's fine, remove the TODO there.

[hlundin-webrtc, 2015-09-14 07:44:24]

LGTM modulo Andrew's comment.

[aluebs-webrtc, 2015-09-15 22:56:09]

PTAL, I had to add some logic to handle the 44100 sample rate case.

https://codereview.webrtc.org/1338833002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/include/audio_processing.h (right):

https://codereview.webrtc.org/1338833002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/include/audio_processing.h:131: static const int
kAudioProcMaxNativeSampleRateHz = 48000;
On 2015/09/11 18:06:23, Andrew MacDonald wrote:
> Looks like this should only affect:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
> 
> Have you tested that it works fine? It _looks_ like it should but my TODO
there
> worries me a bit :) I assume that was written before voice engine used the
> SincResampler.
> 
> Assuming everything's fine, remove the TODO there.

You were right, it was not working for 44100, thank you for catching that! :)
Now I added some more logic to chose the destination sample rate.

[Andrew MacDonald, 2015-09-16 02:35:09]

https://codereview.webrtc.org/1338833002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/include/audio_processing.h (right):

https://codereview.webrtc.org/1338833002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/include/audio_processing.h:476: static const int
kAudioProcNativeSampleRatesHz[] = {
nit: Make these class members:

class AudioProcessing {
  static const int kNativeSampleRatesHz[];
  static const int kMaxNativeSampleRateHz;
};

in .cc
const int AudioProcessing::kNativeSampleRateHz[] = { ... }
const int AudioProcsesing::kMaxNativeSampleRateHz = ...;

https://codereview.webrtc.org/1338833002/diff/20001/webrtc/voice_engine/utili...
File webrtc/voice_engine/utility.cc (right):

https://codereview.webrtc.org/1338833002/diff/20001/webrtc/voice_engine/utili...
webrtc/voice_engine/utility.cc:90: int destination_rate;
I think you should move this to transmit_mixer.cc and do the whole thing in one
place.

It's unfortunate we have to do this at all. Ideally voice engine would be using
an APM interface that handled arbitrary rates.

I suppose it's better to get 48 kHz support in voice engine immediately, but
what do you think about delaying until we have arbitrary rate support in the
int16 interface? We could rip out a bunch of this stuff from voice engine and
just tell APM what we want.

[aluebs-webrtc, 2015-09-16 03:02:53]

https://codereview.webrtc.org/1338833002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/include/audio_processing.h (right):

https://codereview.webrtc.org/1338833002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/include/audio_processing.h:476: static const int
kAudioProcNativeSampleRatesHz[] = {
On 2015/09/16 02:35:08, Andrew MacDonald wrote:
> nit: Make these class members:
> 
> class AudioProcessing {
>   static const int kNativeSampleRatesHz[];
>   static const int kMaxNativeSampleRateHz;
> };
> 
> in .cc
> const int AudioProcessing::kNativeSampleRateHz[] = { ... }
> const int AudioProcsesing::kMaxNativeSampleRateHz = ...;

I though about that, but there is no cc file. Is there another way around it?

https://codereview.webrtc.org/1338833002/diff/20001/webrtc/voice_engine/utili...
File webrtc/voice_engine/utility.cc (right):

https://codereview.webrtc.org/1338833002/diff/20001/webrtc/voice_engine/utili...
webrtc/voice_engine/utility.cc:90: int destination_rate;
On 2015/09/16 02:35:08, Andrew MacDonald wrote:
> I think you should move this to transmit_mixer.cc and do the whole thing in
one
> place.
> 
> It's unfortunate we have to do this at all. Ideally voice engine would be
using
> an APM interface that handled arbitrary rates.
> 
> I suppose it's better to get 48 kHz support in voice engine immediately, but
> what do you think about delaying until we have arbitrary rate support in the
> int16 interface? We could rip out a bunch of this stuff from voice engine and
> just tell APM what we want.

What do you mean with "do the whole thing" in one place. I feel this is the
right place, since it is the function that takes the codec and input signal
constrains and converts to the right format. I think that passing in directly
destination_rate is not consistent with passing in codec_num_channels for
example.
I agree that it is unfortunate that we have to do this. I didn't know there was
an effort to support arbitrary sample rates in the int16 interface. I actually
thought that interface was going away completely and we didn't want to change
its behavior at this point. But we could wait until the new voiceengine uses the
float interface, which supports arbitrary sample rates. But I feel that could
take some time. WDYT?

[Andrew MacDonald, 2015-09-16 04:04:08]

https://codereview.webrtc.org/1338833002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/include/audio_processing.h (right):

https://codereview.webrtc.org/1338833002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/include/audio_processing.h:476: static const int
kAudioProcNativeSampleRatesHz[] = {
On 2015/09/16 03:02:53, aluebs-webrtc wrote:
> On 2015/09/16 02:35:08, Andrew MacDonald wrote:
> > nit: Make these class members:
> > 
> > class AudioProcessing {
> >   static const int kNativeSampleRatesHz[];
> >   static const int kMaxNativeSampleRateHz;
> > };
> > 
> > in .cc
> > const int AudioProcessing::kNativeSampleRateHz[] = { ... }
> > const int AudioProcsesing::kMaxNativeSampleRateHz = ...;
> 
> I though about that, but there is no cc file. Is there another way around it?

You can use audio_processing_impl.cc.

https://codereview.webrtc.org/1338833002/diff/20001/webrtc/voice_engine/utili...
File webrtc/voice_engine/utility.cc (right):

https://codereview.webrtc.org/1338833002/diff/20001/webrtc/voice_engine/utili...
webrtc/voice_engine/utility.cc:90: int destination_rate;
On 2015/09/16 03:02:53, aluebs-webrtc wrote:
> What do you mean with "do the whole thing" in one place. I feel this is the
> right place, since it is the function that takes the codec and input signal
> constrains and converts to the right format. I think that passing in directly
> destination_rate is not consistent with passing in codec_num_channels for
> example.

Mainly, I don't like that both DownConvertToCodecFormat and GenerateAudioFrame
have to know about AudioProcessing details.

Also (just noticed) transmit_mixer.cc isn't the only caller. It's called from
channel.cc as well, and I'm not sure that this behavior change won't impact it.

I'd revert to the original behavior here and move this additional check to
GenerateAudioFrame instead. Is that possible?
 
> I agree that it is unfortunate that we have to do this. I didn't know there
was
> an effort to support arbitrary sample rates in the int16 interface. I actually
> thought that interface was going away completely and we didn't want to change
> its behavior at this point. But we could wait until the new voiceengine uses
the
> float interface, which supports arbitrary sample rates. But I feel that could
> take some time. WDYT?

Ha, no effort, but I thought we talked about it at some point. I want the
AudioFrame interface to go away, but it remains to be seen if we replace that
with another int16 interface. If voice engine moves to the float interface, all
the better.

[hlundin-webrtc, 2015-09-16 13:45:49]

One nit. Otherwise I delegate my reviewing power to Andrew in this case.

https://codereview.webrtc.org/1338833002/diff/20001/webrtc/voice_engine/utili...
File webrtc/voice_engine/utility.cc (right):

https://codereview.webrtc.org/1338833002/diff/20001/webrtc/voice_engine/utili...
webrtc/voice_engine/utility.cc:91: for (size_t i = 0; i <
kNumAudioProcNativeSampleRates; ++i) {
Nit: You can use a C++11 range-based for loop here.

[aluebs-webrtc, 2015-09-22 01:35:29]

https://codereview.webrtc.org/1338833002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/include/audio_processing.h (right):

https://codereview.webrtc.org/1338833002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/include/audio_processing.h:476: static const int
kAudioProcNativeSampleRatesHz[] = {
On 2015/09/16 04:04:08, Andrew MacDonald wrote:
> On 2015/09/16 03:02:53, aluebs-webrtc wrote:
> > On 2015/09/16 02:35:08, Andrew MacDonald wrote:
> > > nit: Make these class members:
> > > 
> > > class AudioProcessing {
> > >   static const int kNativeSampleRatesHz[];
> > >   static const int kMaxNativeSampleRateHz;
> > > };
> > > 
> > > in .cc
> > > const int AudioProcessing::kNativeSampleRateHz[] = { ... }
> > > const int AudioProcsesing::kMaxNativeSampleRateHz = ...;
> > 
> > I though about that, but there is no cc file. Is there another way around
it?
> 
> You can use audio_processing_impl.cc.

Good point. Done.

https://codereview.webrtc.org/1338833002/diff/20001/webrtc/voice_engine/utili...
File webrtc/voice_engine/utility.cc (right):

https://codereview.webrtc.org/1338833002/diff/20001/webrtc/voice_engine/utili...
webrtc/voice_engine/utility.cc:90: int destination_rate;
On 2015/09/16 04:04:08, Andrew MacDonald wrote:
> On 2015/09/16 03:02:53, aluebs-webrtc wrote:
> > What do you mean with "do the whole thing" in one place. I feel this is the
> > right place, since it is the function that takes the codec and input signal
> > constrains and converts to the right format. I think that passing in
directly
> > destination_rate is not consistent with passing in codec_num_channels for
> > example.
> 
> Mainly, I don't like that both DownConvertToCodecFormat and GenerateAudioFrame
> have to know about AudioProcessing details.
> 
> Also (just noticed) transmit_mixer.cc isn't the only caller. It's called from
> channel.cc as well, and I'm not sure that this behavior change won't impact
it.
> 
> I'd revert to the original behavior here and move this additional check to
> GenerateAudioFrame instead. Is that possible?
>  
> > I agree that it is unfortunate that we have to do this. I didn't know there
> was
> > an effort to support arbitrary sample rates in the int16 interface. I
actually
> > thought that interface was going away completely and we didn't want to
change
> > its behavior at this point. But we could wait until the new voiceengine uses
> the
> > float interface, which supports arbitrary sample rates. But I feel that
could
> > take some time. WDYT?
> 
> Ha, no effort, but I thought we talked about it at some point. I want the
> AudioFrame interface to go away, but it remains to be seen if we replace that
> with another int16 interface. If voice engine moves to the float interface,
all
> the better.
> 

Agreed. I didn't move it to the float interface, since that is better done in
another CL.
We discussed using the AudioConverter, but that one has a float interface as
well, so I just unified DownConvertToCodecFormat tp RemixAndResample and let
GenerateAudioFrame (and the caller in channel.cc) take care of which sampling
rate to use. WDYT?

https://codereview.webrtc.org/1338833002/diff/20001/webrtc/voice_engine/utili...
webrtc/voice_engine/utility.cc:91: for (size_t i = 0; i <
kNumAudioProcNativeSampleRates; ++i) {
On 2015/09/16 13:45:49, hlundin-webrtc wrote:
> Nit: You can use a C++11 range-based for loop here.

Since now it is defined in the header and declared in the cc file, this is no
longer possible.

[Andrew MacDonald, 2015-09-23 02:35:53]

https://codereview.webrtc.org/1338833002/diff/20001/webrtc/voice_engine/utili...
File webrtc/voice_engine/utility.cc (right):

https://codereview.webrtc.org/1338833002/diff/20001/webrtc/voice_engine/utili...
webrtc/voice_engine/utility.cc:90: int destination_rate;
On 2015/09/22 01:35:29, aluebs-webrtc wrote:
> Agreed. I didn't move it to the float interface, since that is better done in
> another CL.
> We discussed using the AudioConverter, but that one has a float interface as
> well, so I just unified DownConvertToCodecFormat tp RemixAndResample and let
> GenerateAudioFrame (and the caller in channel.cc) take care of which sampling
> rate to use. WDYT?

What you have looks good. Thanks for the cleanup.

https://codereview.webrtc.org/1338833002/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1338833002/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:3347: _audioFrame.num_channels_ = codec.channels;
I think this is a change in behavior. In DownConvertToCodecFormat we would never
upmix. But here, if number_of_channels is 1 and codec.channels is 2, we will.

https://codereview.webrtc.org/1338833002/diff/40001/webrtc/voice_engine/trans...
File webrtc/voice_engine/transmit_mixer.cc (right):

https://codereview.webrtc.org/1338833002/diff/40001/webrtc/voice_engine/trans...
webrtc/voice_engine/transmit_mixer.cc:1147: // Chose the lowest native sample
rate which is higher or equal than at least
A bit neater is to use the minimum of them. I'd reword the documentation a bit
too.

// We want to process at the lowest rate possible without losing information.
Choose the lowest native rate at least equal to the input and codec rates.
const int min_processing_rate = std::min(sample_rate_hz, codec_rate);

https://codereview.webrtc.org/1338833002/diff/40001/webrtc/voice_engine/utili...
File webrtc/voice_engine/utility.h (right):

https://codereview.webrtc.org/1338833002/diff/40001/webrtc/voice_engine/utili...
webrtc/voice_engine/utility.h:30: // members will be changed.
This comment has been false for awhile. It applies to the overload you added but
not the existing one. Could you clarify the difference between the two versions?

[aluebs-webrtc, 2015-09-23 17:13:43]

https://codereview.webrtc.org/1338833002/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1338833002/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:3347: _audioFrame.num_channels_ = codec.channels;
On 2015/09/23 02:35:52, Andrew MacDonald wrote:
> I think this is a change in behavior. In DownConvertToCodecFormat we would
never
> upmix. But here, if number_of_channels is 1 and codec.channels is 2, we will.

Good catch. Fixed.

https://codereview.webrtc.org/1338833002/diff/40001/webrtc/voice_engine/trans...
File webrtc/voice_engine/transmit_mixer.cc (right):

https://codereview.webrtc.org/1338833002/diff/40001/webrtc/voice_engine/trans...
webrtc/voice_engine/transmit_mixer.cc:1147: // Chose the lowest native sample
rate which is higher or equal than at least
On 2015/09/23 02:35:52, Andrew MacDonald wrote:
> A bit neater is to use the minimum of them. I'd reword the documentation a bit
> too.
> 
> // We want to process at the lowest rate possible without losing information.
> Choose the lowest native rate at least equal to the input and codec rates.
> const int min_processing_rate = std::min(sample_rate_hz, codec_rate);

Done.

https://codereview.webrtc.org/1338833002/diff/40001/webrtc/voice_engine/utili...
File webrtc/voice_engine/utility.h (right):

https://codereview.webrtc.org/1338833002/diff/40001/webrtc/voice_engine/utili...
webrtc/voice_engine/utility.h:30: // members will be changed.
On 2015/09/23 02:35:52, Andrew MacDonald wrote:
> This comment has been false for awhile. It applies to the overload you added
but
> not the existing one. Could you clarify the difference between the two
versions?

Done.

[Andrew MacDonald, 2015-09-23 17:31:47]

lgtm

https://codereview.webrtc.org/1338833002/diff/60001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1338833002/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:3344: // Never upsample the capture signal here.
This should be done at the
add "or upmix"

https://codereview.webrtc.org/1338833002/diff/60001/webrtc/voice_engine/trans...
File webrtc/voice_engine/transmit_mixer.cc (right):

https://codereview.webrtc.org/1338833002/diff/60001/webrtc/voice_engine/trans...
webrtc/voice_engine/transmit_mixer.cc:1144: codec_rate = std::min(codec_rate,
max_sample_rate_hz);
Sorry, just noticed this. Can you remove this check and integrate below? You can
remove max_sample_rate_hz then as it will already be handled by the loop:

const int min_processing_rate = std::min(sample_rate_hz, codec_rate);
for (...) {
  ...
}
if (audioproc_->echo_control_mobile()->is_enabled()) {
  // AECM only supports 8 and 16 kHz.
  _audioFrame.sample_rate_hz = std::min(_audioFrame.sample_rate_hz, 16000);
}

where we might want to advertise the 16000 as a const in the APM interface, up
to you.

https://codereview.webrtc.org/1338833002/diff/60001/webrtc/voice_engine/utili...
File webrtc/voice_engine/utility.h (right):

https://codereview.webrtc.org/1338833002/diff/60001/webrtc/voice_engine/utili...
webrtc/voice_engine/utility.h:36: // dst_frame| to have its sample rate and
channels members set to the desired
Rather than repeating the comments please write the shared block once above both
and then say "This version..." above each to describe the unique bit.

[aluebs-webrtc, 2015-09-23 19:48:54]

https://codereview.webrtc.org/1338833002/diff/60001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/1338833002/diff/60001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:3344: // Never upsample the capture signal here.
This should be done at the
On 2015/09/23 17:31:47, Andrew MacDonald wrote:
> add "or upmix"

Good point. Done.

https://codereview.webrtc.org/1338833002/diff/60001/webrtc/voice_engine/trans...
File webrtc/voice_engine/transmit_mixer.cc (right):

https://codereview.webrtc.org/1338833002/diff/60001/webrtc/voice_engine/trans...
webrtc/voice_engine/transmit_mixer.cc:1144: codec_rate = std::min(codec_rate,
max_sample_rate_hz);
On 2015/09/23 17:31:47, Andrew MacDonald wrote:
> Sorry, just noticed this. Can you remove this check and integrate below? You
can
> remove max_sample_rate_hz then as it will already be handled by the loop:
> 
> const int min_processing_rate = std::min(sample_rate_hz, codec_rate);
> for (...) {
>   ...
> }
> if (audioproc_->echo_control_mobile()->is_enabled()) {
>   // AECM only supports 8 and 16 kHz.
>   _audioFrame.sample_rate_hz = std::min(_audioFrame.sample_rate_hz, 16000);
> }
> 
> where we might want to advertise the 16000 as a const in the APM interface, up
> to you.

Great point. Done.

https://codereview.webrtc.org/1338833002/diff/60001/webrtc/voice_engine/utili...
File webrtc/voice_engine/utility.h (right):

https://codereview.webrtc.org/1338833002/diff/60001/webrtc/voice_engine/utili...
webrtc/voice_engine/utility.h:36: // dst_frame| to have its sample rate and
channels members set to the desired
On 2015/09/23 17:31:47, Andrew MacDonald wrote:
> Rather than repeating the comments please write the shared block once above
both
> and then say "This version..." above each to describe the unique bit.

Done.

[aluebs-webrtc, 2015-09-23 19:49:28]

Committed patchset #7 (id:120001) manually as
cdfe20bfc1146030aa59eb37635fd2fbcecd6cdb (presubmit successful).

[commit-bot: I haz the power, 2015-09-23 19:49:32]

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