Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1211613005:
Adding method IsInBeam to beamformer class. (Closed)
|
Created:
5 years, 6 months ago by bloch Modified:
5 years, 5 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, hlundin-webrtc, kwiberg-webrtc, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdding method IsInBeam to beamformer class.
This was previously reviewed at:
https://webrtc-codereview.appspot.com/53729004/
Committed: https://crrev.com/1adbacb19dbe6f4c537c9cc9d04242b61735a2b8
Cr-Commit-Position: refs/heads/master@{#9517}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Applied Spherical point; fixed casting bug #Patch Set 3 : un-uploading excess files #Patch Set 4 : cast M_PI as float to solve truncation bug #
Total comments: 5
Patch Set 5 : Changed SphericalPointf to reference #
Total comments: 3
Patch Set 6 : changed azimuth to point in comment #
Messages
Total messages: 23 (6 generated)
bloch@google.com changed reviewers: + aluebs@webrtc.org, andrew@webrtc.org
https://codereview.webrtc.org/1211613005/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/beamformer/beamformer.h (right): https://codereview.webrtc.org/1211613005/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/beamformer/beamformer.h:26: Remove these blank lines. https://codereview.webrtc.org/1211613005/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/beamformer/beamformer.h:39: virtual bool IsInBeam(float azimuth) { return true; } We decided to change this to Point, right? https://codereview.webrtc.org/1211613005/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc (right): https://codereview.webrtc.org/1211613005/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc:54: const float kBeamWidthAngle = static_cast<float>(M_PI)20.f / 180.f; const float kBeamWidthAngle = static_cast<float>(M_PI) * 20.f / 180.f;
On 2015/06/25 01:04:38, aluebs-webrtc wrote: > https://codereview.webrtc.org/1211613005/diff/1/webrtc/modules/audio_processi... > File webrtc/modules/audio_processing/beamformer/beamformer.h (right): > > https://codereview.webrtc.org/1211613005/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/beamformer/beamformer.h:26: > Remove these blank lines. > Done. > https://codereview.webrtc.org/1211613005/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/beamformer/beamformer.h:39: virtual bool > IsInBeam(float azimuth) { return true; } > We decided to change this to Point, right? > Done. > https://codereview.webrtc.org/1211613005/diff/1/webrtc/modules/audio_processi... > File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc (right): > > https://codereview.webrtc.org/1211613005/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc:54: const > float kBeamWidthAngle = static_cast<float>(M_PI)20.f / 180.f; > const float kBeamWidthAngle = static_cast<float>(M_PI) * 20.f / 180.f; Fixed.
The CQ bit was checked by bloch@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1211613005/20001
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was unchecked by bloch@google.com
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Accidentally included other people's files in my cl, but I resolved that.
https://codereview.webrtc.org/1211613005/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc (right): https://codereview.webrtc.org/1211613005/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc:54: const float kBeamWidthAngle = static_cast<float>(M_PI)20.f / 180.f; On 2015/06/25 01:04:36, aluebs-webrtc wrote: > const float kBeamWidthAngle = static_cast<float>(M_PI) * 20.f / 180.f; I think you can omit the cast. https://codereview.webrtc.org/1211613005/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/beamformer/beamformer.h (right): https://codereview.webrtc.org/1211613005/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer/beamformer.h:35: virtual bool IsInBeam(SphericalPointf spherical_point) { return true; } Do we want to make this pure virtual after we update derived classes? If so, add a TODO. Doesn't really matter with such a small type, but make it a const reference: const SphericalPointf& spherical_point https://codereview.webrtc.org/1211613005/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.h (right): https://codereview.webrtc.org/1211613005/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.h:51: // Indicates whether a given azimuth is inside the beam. I know we've copied the comments from the base class everywhere else, but I think that just invites rot. Really they should be removed. Can you remove this one at least?
...and I took the liberty of fixing up the CL description :) The title isn't added to the commit message, just the "description" field.
https://codereview.webrtc.org/1211613005/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc (right): https://codereview.webrtc.org/1211613005/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc:54: const float kBeamWidthAngle = static_cast<float>(M_PI)20.f / 180.f; On 2015/06/26 02:04:24, andrew wrote: > On 2015/06/25 01:04:36, aluebs-webrtc wrote: > > const float kBeamWidthAngle = static_cast<float>(M_PI) * 20.f / 180.f; > > I think you can omit the cast. I tried it without the cast and there were some compiler errors when I ran "git cl try" https://codereview.webrtc.org/1211613005/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/beamformer/beamformer.h (right): https://codereview.webrtc.org/1211613005/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer/beamformer.h:35: virtual bool IsInBeam(SphericalPointf spherical_point) { return true; } On 2015/06/26 02:04:24, andrew wrote: > Do we want to make this pure virtual after we update derived classes? If so, add > a TODO. I'm not sure. I like the logic of assuming "true" unless otherwise specified, so every class that inherits from beamformer can accommodate a call to "IsInBeam". Someone who implements a weak beamformer with no "outside of beam" area shouldn't have to write their own version that always returns true. WDYT? > Doesn't really matter with such a small type, but make it a const reference: > const SphericalPointf& spherical_point Done. https://codereview.webrtc.org/1211613005/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.h (right): https://codereview.webrtc.org/1211613005/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.h:51: // Indicates whether a given azimuth is inside the beam. On 2015/06/26 02:04:24, andrew wrote: > I know we've copied the comments from the base class everywhere else, but I > think that just invites rot. Really they should be removed. Can you remove this > one at least? Done.
lgtm with a minor change. https://codereview.webrtc.org/1211613005/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc (right): https://codereview.webrtc.org/1211613005/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc:54: const float kBeamWidthAngle = static_cast<float>(M_PI)20.f / 180.f; On 2015/06/26 03:37:28, bloch wrote: > On 2015/06/26 02:04:24, andrew wrote: > > On 2015/06/25 01:04:36, aluebs-webrtc wrote: > > > const float kBeamWidthAngle = static_cast<float>(M_PI) * 20.f / 180.f; > > > > I think you can omit the cast. > > I tried it without the cast and there were some compiler errors when I ran "git > cl try" OK, thanks for trying. https://codereview.webrtc.org/1211613005/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/beamformer/beamformer.h (right): https://codereview.webrtc.org/1211613005/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer/beamformer.h:35: virtual bool IsInBeam(SphericalPointf spherical_point) { return true; } On 2015/06/26 03:37:28, bloch wrote: > On 2015/06/26 02:04:24, andrew wrote: > > Do we want to make this pure virtual after we update derived classes? If so, > add > > a TODO. > > I'm not sure. I like the logic of assuming "true" unless otherwise specified, so > every class that inherits from beamformer can accommodate a call to "IsInBeam". > Someone who implements a weak beamformer with no "outside of beam" area > shouldn't have to write their own version that always returns true. WDYT? I'd argue the creator of such a beamformer _should_ be forced to implement this. No big deal though, you can leave it as-is. https://codereview.webrtc.org/1211613005/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/beamformer/beamformer.h (right): https://codereview.webrtc.org/1211613005/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer/beamformer.h:34: // Indicates whether a given azimuth is inside of the beam. s/azimuth/point
https://codereview.webrtc.org/1211613005/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/beamformer/beamformer.h (right): https://codereview.webrtc.org/1211613005/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer/beamformer.h:34: // Indicates whether a given azimuth is inside of the beam. On 2015/06/26 03:46:09, andrew wrote: > s/azimuth/point I changed "azimuth" to "point". Is this what you were indicating?
lgtm https://codereview.webrtc.org/1211613005/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/beamformer/beamformer.h (right): https://codereview.webrtc.org/1211613005/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/beamformer/beamformer.h:34: // Indicates whether a given azimuth is inside of the beam. On 2015/06/26 04:01:56, bloch wrote: > On 2015/06/26 03:46:09, andrew wrote: > > s/azimuth/point > > I changed "azimuth" to "point". Is this what you were indicating? Yep.
lgtm
The CQ bit was checked by bloch@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1211613005/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/1adbacb19dbe6f4c537c9cc9d04242b61735a2b8 Cr-Commit-Position: refs/heads/master@{#9517}
Message was sent while issue was closed.
Hey, I realized I wasn't including array_util in beamformer.h, so I moved the inclusion to there.
Message was sent while issue was closed.
Patchset #7 (id:120001) has been deleted
Message was sent while issue was closed.
On 2015/07/20 17:17:38, bloch wrote: > Hey, I realized I wasn't including array_util in beamformer.h, so I moved the > inclusion to there. I deleted your last patch, since this is a CL that already was landed. Please do a different CL with your changes. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
