Drop 48kHz sample rate support in the APM for ARM architecture

Drop 48kHz sample rate support in the APM for ARM architecture

The 3-band splitting filter is highly complex on this architecture. Today this is not a problem, because on those platforms we mostly use AECM which forces us to downsample to 16kHz anyway, but this is a way of guarding against it. In the long term we want to optimize the 3-band splitting filter for ARM architectures, but for now we can just disable it.

Committed: https://crrev.com/4c279b852c7f3c92654fa09f813259ff427d1830
Cr-Commit-Position: refs/heads/master@{#11900}

[aluebs-webrtc, 2016-03-04 23:08:36]

Description was changed from

==========
Drop 48kHz sample rate support in the APM for ARM architecture

The 3-band splitting filter is highly complex in this architecture. Today this
is not a problem, because on those platforms we mostly use AECM which forces us
to downsample to 16kHz anyway, but this is a way of guarding against it. IN the
long term we want to optimize the 3-band splitting filter for ARM architectures,
but for now we can just disable it.
==========

to

==========
Drop 48kHz sample rate support in the APM for ARM architecture

The 3-band splitting filter is highly complex on this architecture. Today this
is not a problem, because on those platforms we mostly use AECM which forces us
to downsample to 16kHz anyway, but this is a way of guarding against it. In the
long term we want to optimize the 3-band splitting filter for ARM architectures,
but for now we can just disable it.
==========

[aluebs-webrtc, 2016-03-04 23:08:36]

aluebs@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org, peah@webrtc.org, turaj@webrtc.org

[peah-webrtc, 2016-03-05 10:56:41]

https://codereview.webrtc.org/1766103002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/1766103002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:116:
AudioProcessing::kSampleRate32kHz};
What is the impact of this on current platforms? What happens if APM is actually
being opened in 48 kHz on ARM? I guess that there is an error message?

We need to make sure that 
1) Make sure that this change does not break WebRTC for any currently running
ARM platforms.
2) The error message that is caused by this is properly addressed when received
by a caller of APM.

The next question is what to do instead. On Android, I'm quite sure that we are
called with 48 kHz when AECM is not active. Should we then downsample to 32 kHz
internally?

[aluebs-webrtc, 2016-03-07 11:32:15]

https://codereview.webrtc.org/1766103002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/1766103002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:116:
AudioProcessing::kSampleRate32kHz};
On 2016/03/05 10:56:41, peah-webrtc wrote:
> What is the impact of this on current platforms? What happens if APM is
actually
> being opened in 48 kHz on ARM? I guess that there is an error message?
> 
> We need to make sure that 
> 1) Make sure that this change does not break WebRTC for any currently running
> ARM platforms.
> 2) The error message that is caused by this is properly addressed when
received
> by a caller of APM.
> 
> The next question is what to do instead. On Android, I'm quite sure that we
are
> called with 48 kHz when AECM is not active. Should we then downsample to 32
kHz
> internally?

What do you mean by "being opened"? If the APM receives a signal of 48kHz it
would just downsample to 32kHz at the input and upsample it again at the end. So
there is no error message.

[peah-webrtc, 2016-03-07 12:50:48]

https://codereview.webrtc.org/1766103002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/1766103002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:116:
AudioProcessing::kSampleRate32kHz};
On 2016/03/07 11:32:15, aluebs-webrtc wrote:
> On 2016/03/05 10:56:41, peah-webrtc wrote:
> > What is the impact of this on current platforms? What happens if APM is
> actually
> > being opened in 48 kHz on ARM? I guess that there is an error message?
> > 
> > We need to make sure that 
> > 1) Make sure that this change does not break WebRTC for any currently
running
> > ARM platforms.
> > 2) The error message that is caused by this is properly addressed when
> received
> > by a caller of APM.
> > 
> > The next question is what to do instead. On Android, I'm quite sure that we
> are
> > called with 48 kHz when AECM is not active. Should we then downsample to 32
> kHz
> > internally?
> 
> What do you mean by "being opened"? If the APM receives a signal of 48kHz it
> would just downsample to 32kHz at the input and upsample it again at the end.
So
> there is no error message.


I have another question about this code. Currently it is only used as to set the
processing rate as:

const int AudioProcessing::kNativeSampleRatesHz[] = {
    AudioProcessing::kSampleRate8kHz,
    AudioProcessing::kSampleRate16kHz,
    AudioProcessing::kSampleRate32kHz,
    AudioProcessing::kSampleRate48kHz};
const size_t AudioProcessing::kNumNativeSampleRates =
    arraysize(AudioProcessing::kNativeSampleRatesHz);
const int AudioProcessing::kMaxNativeSampleRateHz = AudioProcessing::
    kNativeSampleRatesHz[AudioProcessing::kNumNativeSampleRates - 1];

...

int fwd_proc_rate;
  for (size_t i = 0; i < kNumNativeSampleRates; ++i) {
    fwd_proc_rate = kNativeSampleRatesHz[i];
    if (fwd_proc_rate >= min_proc_rate) {
      break;
    }
  }


Why not change that to (the example excluding your changes in this CL)
const int AudioProcessing::kMaxNativeSampleRateHz =
AudioProcessing::kSampleRate48kHz;
...
const int fwd_proc_rate = AudioProcessing::kMaxNativeSampleRateHz;

https://codereview.webrtc.org/1766103002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:116:
AudioProcessing::kSampleRate32kHz};
On 2016/03/07 11:32:15, aluebs-webrtc wrote:
> On 2016/03/05 10:56:41, peah-webrtc wrote:
> > What is the impact of this on current platforms? What happens if APM is
> actually
> > being opened in 48 kHz on ARM? I guess that there is an error message?
> > 
> > We need to make sure that 
> > 1) Make sure that this change does not break WebRTC for any currently
running
> > ARM platforms.
> > 2) The error message that is caused by this is properly addressed when
> received
> > by a caller of APM.
> > 
> > The next question is what to do instead. On Android, I'm quite sure that we
> are
> > called with 48 kHz when AECM is not active. Should we then downsample to 32
> kHz
> > internally?
> 
> What do you mean by "being opened"? If the APM receives a signal of 48kHz it
> would just downsample to 32kHz at the input and upsample it again at the end.
So
> there is no error message.

I think I see it now. So this change has nothing to do with supported sample
rates, right? The kNativeSampleRatesHz array is only used to determine the
minimum processing rate, right? And has nothing to do with the supported sample
rates, right?

Then this change should be fine as you say.

So what then instead happens is that the 3-bandsplitting is replaced by an
down+upsampling? Which should be cheaper than the 3-bandsplitting, right?

[aluebs-webrtc, 2016-03-07 13:02:02]

https://codereview.webrtc.org/1766103002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/1766103002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:116:
AudioProcessing::kSampleRate32kHz};
On 2016/03/07 12:50:48, peah-webrtc wrote:
> On 2016/03/07 11:32:15, aluebs-webrtc wrote:
> > On 2016/03/05 10:56:41, peah-webrtc wrote:
> > > What is the impact of this on current platforms? What happens if APM is
> > actually
> > > being opened in 48 kHz on ARM? I guess that there is an error message?
> > > 
> > > We need to make sure that 
> > > 1) Make sure that this change does not break WebRTC for any currently
> running
> > > ARM platforms.
> > > 2) The error message that is caused by this is properly addressed when
> > received
> > > by a caller of APM.
> > > 
> > > The next question is what to do instead. On Android, I'm quite sure that
we
> > are
> > > called with 48 kHz when AECM is not active. Should we then downsample to
32
> > kHz
> > > internally?
> > 
> > What do you mean by "being opened"? If the APM receives a signal of 48kHz it
> > would just downsample to 32kHz at the input and upsample it again at the
end.
> So
> > there is no error message.
> 
> 
> I have another question about this code. Currently it is only used as to set
the
> processing rate as:
> 
> const int AudioProcessing::kNativeSampleRatesHz[] = {
>     AudioProcessing::kSampleRate8kHz,
>     AudioProcessing::kSampleRate16kHz,
>     AudioProcessing::kSampleRate32kHz,
>     AudioProcessing::kSampleRate48kHz};
> const size_t AudioProcessing::kNumNativeSampleRates =
>     arraysize(AudioProcessing::kNativeSampleRatesHz);
> const int AudioProcessing::kMaxNativeSampleRateHz = AudioProcessing::
>     kNativeSampleRatesHz[AudioProcessing::kNumNativeSampleRates - 1];
> 
> ...
> 
> int fwd_proc_rate;
>   for (size_t i = 0; i < kNumNativeSampleRates; ++i) {
>     fwd_proc_rate = kNativeSampleRatesHz[i];
>     if (fwd_proc_rate >= min_proc_rate) {
>       break;
>     }
>   }
> 
> 
> Why not change that to (the example excluding your changes in this CL)
> const int AudioProcessing::kMaxNativeSampleRateHz =
> AudioProcessing::kSampleRate48kHz;
> ...
> const int fwd_proc_rate = AudioProcessing::kMaxNativeSampleRateHz;
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 

As you say, this would replace the 3-band splitting by a down- and up-sampling.
Regarding your suggestion, this would force to use 48kHz always, which is not
what we want to do if we get and input (or output) sample rate of, let's say,
16kHz.

[peah-webrtc, 2016-03-07 14:22:59]

On 2016/03/07 13:02:02, aluebs-webrtc wrote:
>
https://codereview.webrtc.org/1766103002/diff/1/webrtc/modules/audio_processi...
> File webrtc/modules/audio_processing/audio_processing_impl.cc (right):
> 
>
https://codereview.webrtc.org/1766103002/diff/1/webrtc/modules/audio_processi...
> webrtc/modules/audio_processing/audio_processing_impl.cc:116:
> AudioProcessing::kSampleRate32kHz};
> On 2016/03/07 12:50:48, peah-webrtc wrote:
> > On 2016/03/07 11:32:15, aluebs-webrtc wrote:
> > > On 2016/03/05 10:56:41, peah-webrtc wrote:
> > > > What is the impact of this on current platforms? What happens if APM is
> > > actually
> > > > being opened in 48 kHz on ARM? I guess that there is an error message?
> > > > 
> > > > We need to make sure that 
> > > > 1) Make sure that this change does not break WebRTC for any currently
> > running
> > > > ARM platforms.
> > > > 2) The error message that is caused by this is properly addressed when
> > > received
> > > > by a caller of APM.
> > > > 
> > > > The next question is what to do instead. On Android, I'm quite sure that
> we
> > > are
> > > > called with 48 kHz when AECM is not active. Should we then downsample to
> 32
> > > kHz
> > > > internally?
> > > 
> > > What do you mean by "being opened"? If the APM receives a signal of 48kHz
it
> > > would just downsample to 32kHz at the input and upsample it again at the
> end.
> > So
> > > there is no error message.
> > 
> > 
> > I have another question about this code. Currently it is only used as to set
> the
> > processing rate as:
> > 
> > const int AudioProcessing::kNativeSampleRatesHz[] = {
> >     AudioProcessing::kSampleRate8kHz,
> >     AudioProcessing::kSampleRate16kHz,
> >     AudioProcessing::kSampleRate32kHz,
> >     AudioProcessing::kSampleRate48kHz};
> > const size_t AudioProcessing::kNumNativeSampleRates =
> >     arraysize(AudioProcessing::kNativeSampleRatesHz);
> > const int AudioProcessing::kMaxNativeSampleRateHz = AudioProcessing::
> >     kNativeSampleRatesHz[AudioProcessing::kNumNativeSampleRates - 1];
> > 
> > ...
> > 
> > int fwd_proc_rate;
> >   for (size_t i = 0; i < kNumNativeSampleRates; ++i) {
> >     fwd_proc_rate = kNativeSampleRatesHz[i];
> >     if (fwd_proc_rate >= min_proc_rate) {
> >       break;
> >     }
> >   }
> > 
> > 
> > Why not change that to (the example excluding your changes in this CL)
> > const int AudioProcessing::kMaxNativeSampleRateHz =
> > AudioProcessing::kSampleRate48kHz;
> > ...
> > const int fwd_proc_rate = AudioProcessing::kMaxNativeSampleRateHz;
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> 
> As you say, this would replace the 3-band splitting by a down- and
up-sampling.
> Regarding your suggestion, this would force to use 48kHz always, which is not
> what we want to do if we get and input (or output) sample rate of, let's say,
> 16kHz.

lgtm

[aluebs-webrtc, 2016-03-07 14:29:44]

turaj, hlundin, any additional comments?

[turaj, 2016-03-07 16:15:17]

lgtm

[aluebs-webrtc, 2016-03-08 08:48:35]

Landing this now, but please let me know if you have any additional comments.

[aluebs-webrtc, 2016-03-08 08:48:39]

The CQ bit was checked by aluebs@webrtc.org

[commit-bot: I haz the power, 2016-03-08 08:48:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1766103002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1766103002/1

[commit-bot: I haz the power, 2016-03-08 09:48:21]

Description was changed from

==========
Drop 48kHz sample rate support in the APM for ARM architecture

The 3-band splitting filter is highly complex on this architecture. Today this
is not a problem, because on those platforms we mostly use AECM which forces us
to downsample to 16kHz anyway, but this is a way of guarding against it. In the
long term we want to optimize the 3-band splitting filter for ARM architectures,
but for now we can just disable it.
==========

to

==========
Drop 48kHz sample rate support in the APM for ARM architecture

The 3-band splitting filter is highly complex on this architecture. Today this
is not a problem, because on those platforms we mostly use AECM which forces us
to downsample to 16kHz anyway, but this is a way of guarding against it. In the
long term we want to optimize the 3-band splitting filter for ARM architectures,
but for now we can just disable it.
==========

[commit-bot: I haz the power, 2016-03-08 09:48:22]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-03-08 09:48:29]

Description was changed from

==========
Drop 48kHz sample rate support in the APM for ARM architecture

The 3-band splitting filter is highly complex on this architecture. Today this
is not a problem, because on those platforms we mostly use AECM which forces us
to downsample to 16kHz anyway, but this is a way of guarding against it. In the
long term we want to optimize the 3-band splitting filter for ARM architectures,
but for now we can just disable it.
==========

to

==========
Drop 48kHz sample rate support in the APM for ARM architecture

The 3-band splitting filter is highly complex on this architecture. Today this
is not a problem, because on those platforms we mostly use AECM which forces us
to downsample to 16kHz anyway, but this is a way of guarding against it. In the
long term we want to optimize the 3-band splitting filter for ARM architectures,
but for now we can just disable it.

Committed: https://crrev.com/4c279b852c7f3c92654fa09f813259ff427d1830
Cr-Commit-Position: refs/heads/master@{#11900}
==========

[commit-bot: I haz the power, 2016-03-08 09:48:30]

Patchset 1 (id:??) landed as
https://crrev.com/4c279b852c7f3c92654fa09f813259ff427d1830
Cr-Commit-Position: refs/heads/master@{#11900}

[hlundin-webrtc, 2016-03-08 09:58:47]

lgtm

[peah-webrtc, 2016-03-10 07:01:00]

https://codereview.webrtc.org/1766103002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/1766103002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:116:
AudioProcessing::kSampleRate32kHz};
Ah, then I see. Then the change is fine.