WebRTC Code Reviews
Help | Chromium Project | Sign in
(82)

Issues Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 53729004: Adding method IsInBeam to beamformer class.

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 7 months ago by bloch
Modified:
5 years, 6 months ago
Reviewers:
aluebs, ajm
CC:
webrtc-reviews_webrtc.org, ajm, tterriberry, HL, kwiberg, Bjorn
Base URL:
https://chromium.googlesource.com/external/webrtc.git@master
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

Adding method IsInBeam to beamformer class.

Patch Set 1 #

Patch Set 2 : Added comments, figured out upload workflow. #

Total comments: 1

Patch Set 3 : Added const variables to namespace for IsInBeam #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+15 lines, -0 lines) Patch
M webrtc/modules/audio_processing/beamformer/beamformer.h View 1 1 chunk +3 lines, -0 lines 0 comments Download
M webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.h View 1 1 chunk +3 lines, -0 lines 0 comments Download
M webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc View 1 2 2 chunks +9 lines, -0 lines 2 comments Download
Trybot results: Sign in to try more bots
Project "webrtc" does not have a commit queue.

Messages

Total messages: 7 (1 generated)
bloch
First CL. Added IsInBeam method.
5 years, 6 months ago (2015-06-23 21:47:14 UTC) #2
aluebs
https://webrtc-codereview.appspot.com/53729004/diff/20001/webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc (right): https://webrtc-codereview.appspot.com/53729004/diff/20001/webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc#newcode339 webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc:339: if (fabs(azimuth * (180 / M_PI) - 90) > ...
5 years, 6 months ago (2015-06-24 00:53:57 UTC) #3
bloch
Added namespace const variables to simplify/generalize IsInBeam in nonlinear_beamformer.cc
5 years, 6 months ago (2015-06-24 01:35:13 UTC) #4
aluebs
lgtm But please wait for ajm's review as well.
5 years, 6 months ago (2015-06-24 01:58:13 UTC) #5
ajm
What is the plan for this API? It seems like just exposing kHalfBeamWidthRadians would be ...
5 years, 6 months ago (2015-06-24 04:12:01 UTC) #6
aluebs
5 years, 6 months ago (2015-06-24 17:51:54 UTC) #7 
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 245c2c2-tainted