Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(473)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 833643005: Add a media constraint to enable beamforming. (Closed)

Created:
5 years, 11 months ago by aluebs-chromium
Modified:
5 years, 11 months ago
Reviewers:
ajm, DaleCurtis
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, wjia+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add a media constraint to enable beamforming. Check the ChromeOS model to pass down the correct microphone array geometry. BUG=405270 Committed: https://crrev.com/0201ae167ae81ba069f4f9a5498d5f95694edc8d Cr-Commit-Position: refs/heads/master@{#310777}

Patch Set 1 #

Patch Set 2 : Configure beamforming depending on device #

Total comments: 4

Patch Set 3 : Nits #

Total comments: 2

Patch Set 4 : Declare and initialize geometry at once #

Unified diffs Side-by-side diffs Delta from patch set Stats (+31 lines, -1 line) Patch
M content/renderer/media/media_stream_audio_processor.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M content/renderer/media/media_stream_audio_processor.cc View 1 2 3 4 chunks +27 lines, -1 line 0 comments Download
M content/renderer/media/media_stream_audio_processor_options.h View 1 chunk +1 line, -0 lines 0 comments Download
M content/renderer/media/media_stream_audio_processor_options.cc View 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 15 (4 generated)
aluebs-chromium
This still needs WebRTC to be rolled to Chromium, before being able to land.
5 years, 11 months ago (2015-01-06 04:38:04 UTC) #2
ajm
lgtm +tommi for owner's approval.
5 years, 11 months ago (2015-01-06 07:07:56 UTC) #4
aluebs-chromium
-tommi +dalecurtis for owner's approval ajm, please take another look
5 years, 11 months ago (2015-01-07 23:51:49 UTC) #6
ajm
lgtm with nits, and update the CL description to mention the addition of geometry. https://codereview.chromium.org/833643005/diff/20001/content/renderer/media/media_stream_audio_processor.cc ...
5 years, 11 months ago (2015-01-08 00:03:17 UTC) #7
aluebs-chromium
https://codereview.chromium.org/833643005/diff/20001/content/renderer/media/media_stream_audio_processor.cc File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/833643005/diff/20001/content/renderer/media/media_stream_audio_processor.cc#newcode521 content/renderer/media/media_stream_audio_processor.cc:521: geometry.push_back(webrtc::Point(0.05f, 0.f, 0.f)); On 2015/01/08 00:03:17, ajm wrote: > ...
5 years, 11 months ago (2015-01-08 00:29:17 UTC) #8
DaleCurtis
lgtm https://codereview.chromium.org/833643005/diff/40001/content/renderer/media/media_stream_audio_processor.cc File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/833643005/diff/40001/content/renderer/media/media_stream_audio_processor.cc#newcode515 content/renderer/media/media_stream_audio_processor.cc:515: std::vector<webrtc::Point> geometry; std::vector<webrtc::Point> geometry(1, webrtc::Point(0.f, 0.f, 0.f)) ?
5 years, 11 months ago (2015-01-08 02:03:53 UTC) #9
aluebs-chromium
https://codereview.chromium.org/833643005/diff/40001/content/renderer/media/media_stream_audio_processor.cc File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/833643005/diff/40001/content/renderer/media/media_stream_audio_processor.cc#newcode515 content/renderer/media/media_stream_audio_processor.cc:515: std::vector<webrtc::Point> geometry; On 2015/01/08 02:03:53, DaleCurtis wrote: > std::vector<webrtc::Point> ...
5 years, 11 months ago (2015-01-08 17:32:43 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/833643005/60001
5 years, 11 months ago (2015-01-09 15:38:55 UTC) #12
commit-bot: I haz the power
Committed patchset #4 (id:60001)
5 years, 11 months ago (2015-01-09 16:29:11 UTC) #13
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/0201ae167ae81ba069f4f9a5498d5f95694edc8d Cr-Commit-Position: refs/heads/master@{#310777}
5 years, 11 months ago (2015-01-09 16:31:05 UTC) #14
scottmg
5 years, 11 months ago (2015-01-09 18:40:17 UTC) #15
Message was sent while issue was closed. 
A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/844983002/ by scottmg@chromium.org.

The reason for reverting is: I had to revert the WebRTC roll 
https://codereview.chromium.org/839173004/

And so now this doesn't compile so I'm going to revert it also. Sorry for the
hassle, hopefully it's an easy reland.
.

Powered by Google App Engine
This is Rietveld 408576698