Adding method IsInBeam to beamformer class.

Adding method IsInBeam to beamformer class.

[bloch, 2015-06-23 21:47:13]

bloch@google.com changed reviewers:
+ aluebs@webrtc.org, andrew@webrtc.org

[bloch, 2015-06-23 21:47:14]

First CL. Added IsInBeam method.

[aluebs, 2015-06-24 00:53:57]

https://webrtc-codereview.appspot.com/53729004/diff/20001/webrtc/modules/audi...
File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc (right):

https://webrtc-codereview.appspot.com/53729004/diff/20001/webrtc/modules/audi...
webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc:339: if
(fabs(azimuth * (180 / M_PI) - 90) > 20) {
No need for this if statement:

return fabs(azimuth - kTargetAngleRadians) < kBeamWidthAngle

And in the annonymous namespace above you can define:

const float kBeamWidthAngle = static_cast<float>(M_PI) 20.f / 180.f;

[bloch, 2015-06-24 01:35:13]

Added namespace const variables to simplify/generalize IsInBeam in
nonlinear_beamformer.cc

[aluebs, 2015-06-24 01:58:13]

lgtm

But please wait for ajm's review as well.

[ajm, 2015-06-24 04:12:01]

What is the plan for this API? It seems like just exposing kHalfBeamWidthRadians
would be more useful to the client.

https://webrtc-codereview.appspot.com/53729004/diff/30001/webrtc/modules/audi...
File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc (right):

https://webrtc-codereview.appspot.com/53729004/diff/30001/webrtc/modules/audi...
webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc:54: const
float kBeamWidthAngle = static_cast<float>(M_PI)20.f / 180.f;
Shouldn't this be based on an existing parameter used to generate the masks? If
not, we should explain why here.

Call it kHalfBeamWidthRadians, which obviates the comment.

You're missing a "*", and you shouldn't need the cast:
M_PI * 20.f / 180.f;

It's not clear to me what the result of your existing expression is.

https://webrtc-codereview.appspot.com/53729004/diff/30001/webrtc/modules/audi...
webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc:343: return
fabs(azimuth - kTargetAngleRadians) < kBeamWidthAngle;
std::fabs

[aluebs, 2015-06-24 17:51:54]

On 2015/06/24 04:12:01, ajm wrote:
> What is the plan for this API? It seems like just exposing
kHalfBeamWidthRadians
> would be more useful to the client.
> 
>
https://webrtc-codereview.appspot.com/53729004/diff/30001/webrtc/modules/audi...
> File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc
(right):
> 
>
https://webrtc-codereview.appspot.com/53729004/diff/30001/webrtc/modules/audi...
> webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc:54: const
> float kBeamWidthAngle = static_cast<float>(M_PI)20.f / 180.f;
> Shouldn't this be based on an existing parameter used to generate the masks?
If
> not, we should explain why here.
> 
> Call it kHalfBeamWidthRadians, which obviates the comment.
> 
> You're missing a "*", and you shouldn't need the cast:
> M_PI * 20.f / 180.f;
> 
> It's not clear to me what the result of your existing expression is.
> 
>
https://webrtc-codereview.appspot.com/53729004/diff/30001/webrtc/modules/audi...
> webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc:343: return
> fabs(azimuth - kTargetAngleRadians) < kBeamWidthAngle;
> std::fabs

I actually think having an API that looks like this is more flexible for
different beamformers than exposing the beamwidth. But if we want to be
completely general we need to get a Point as input and not just an azimuth.