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

Issue 227743004: Added a kEchoCancellation constraint to turn off the audio processing. (Closed)

Created:
6 years, 8 months ago by no longer working on chromium
Modified:
6 years, 7 months ago
CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

Added a kEchoCancellation constraint to turn off the audio processing. Also, change the the existing code not to use the libjingle interfaces. BUG=264611 TEST=content_unittests --getst_filter="*MediaStreamAudioProcessor*" Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270728

Patch Set 1 : #

Patch Set 2 : #

Total comments: 59

Patch Set 3 : addressed the comments. #

Total comments: 25

Patch Set 4 : moved some of the constraints code to MediaAudioConstraints class, and hope it makes the code more … #

Total comments: 19

Patch Set 5 : addressed Per's comments. #

Total comments: 45

Patch Set 6 : addressed the comments and added more unittests #

Patch Set 7 : added missing new unittest #

Total comments: 5

Patch Set 8 : added a GetEchoCancellationProperty method. #

Total comments: 1

Patch Set 9 : GetEchoCancellationProperty handles both kEchoCancellation and kGoogEchoCancellation #

Total comments: 26

Patch Set 10 : addressed Tommi's comments. #

Total comments: 6

Patch Set 11 : minor changes: adding CONTENT_EXPORT, KSourceId to fix all the bots #

Total comments: 3

Patch Set 12 : addressed the comments. #

Patch Set 13 : when kEchoCancellation is false, default values of all goog* constraints are false, added unittests #

Total comments: 15

Patch Set 14 : addressed Tommi's comments. #

Total comments: 2

Patch Set 15 : #

Total comments: 2

Patch Set 16 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+766 lines, -185 lines) Patch
M content/content_renderer.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +2 lines, -0 lines 0 comments Download
M content/content_tests.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M content/renderer/media/media_stream_audio_processor.h View 1 2 3 4 5 6 7 3 chunks +1 line, -4 lines 0 comments Download
M content/renderer/media/media_stream_audio_processor.cc View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +37 lines, -53 lines 0 comments Download
M content/renderer/media/media_stream_audio_processor_options.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +62 lines, -25 lines 0 comments Download
M content/renderer/media/media_stream_audio_processor_options.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +147 lines, -39 lines 0 comments Download
M content/renderer/media/media_stream_audio_processor_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 7 chunks +121 lines, -13 lines 0 comments Download
A content/renderer/media/media_stream_constraints_util.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +73 lines, -0 lines 0 comments Download
A content/renderer/media/media_stream_constraints_util.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +107 lines, -0 lines 0 comments Download
A content/renderer/media/media_stream_constraints_util_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +103 lines, -0 lines 0 comments Download
M content/renderer/media/media_stream_dependency_factory.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -1 line 0 comments Download
M content/renderer/media/media_stream_source.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +4 lines, -0 lines 0 comments Download
M content/renderer/media/media_stream_source.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -0 lines 0 comments Download
M content/renderer/media/media_stream_video_source.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 6 chunks +18 lines, -27 lines 0 comments Download
M content/renderer/media/mock_media_constraint_factory.h View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M content/renderer/media/mock_media_constraint_factory.cc View 1 2 3 4 5 4 chunks +32 lines, -1 line 0 comments Download
M content/renderer/media/webrtc/webrtc_local_audio_track_adapter_unittest.cc View 1 2 3 4 5 2 chunks +8 lines, -5 lines 0 comments Download
M content/renderer/media/webrtc_audio_capturer.cc View 1 2 3 4 5 6 7 8 3 chunks +9 lines, -4 lines 0 comments Download
M content/renderer/media/webrtc_audio_capturer_unittest.cc View 1 2 3 4 5 2 chunks +20 lines, -2 lines 0 comments Download
M content/renderer/media/webrtc_local_audio_source_provider_unittest.cc View 1 2 3 4 5 3 chunks +6 lines, -5 lines 0 comments Download
M content/renderer/media/webrtc_local_audio_track_unittest.cc View 1 2 3 4 5 3 chunks +8 lines, -6 lines 0 comments Download

Messages

Total messages: 41 (0 generated)
no longer working on chromium
Hi Per, could you please review this CL. Tommi, feel free to take a look ...
6 years, 8 months ago (2014-04-08 09:55:25 UTC) #1
no longer working on chromium
https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/media_stream_audio_processor.cc File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/media_stream_audio_processor.cc#newcode327 content/renderer/media/media_stream_audio_processor.cc:327: if (!echo_calcellation || The main change is here, echo_calcellation ...
6 years, 8 months ago (2014-04-08 10:02:52 UTC) #2
perkj_chrome
https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/media_stream_audio_processor.cc File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/media_stream_audio_processor.cc#newcode293 content/renderer/media/media_stream_audio_processor.cc:293: const bool echo_calcellation = GetPropertyFromConstraints( nit: move to where ...
6 years, 8 months ago (2014-04-08 10:28:12 UTC) #3
tommi (sloooow) - chröme
https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/media_stream_audio_processor.cc File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/media_stream_audio_processor.cc#newcode293 content/renderer/media/media_stream_audio_processor.cc:293: const bool echo_calcellation = GetPropertyFromConstraints( echo_cancellation (not calcellation) Actually, ...
6 years, 8 months ago (2014-04-08 10:49:30 UTC) #4
no longer working on chromium
https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/media_stream_audio_processor.cc File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/media_stream_audio_processor.cc#newcode293 content/renderer/media/media_stream_audio_processor.cc:293: const bool echo_calcellation = GetPropertyFromConstraints( On 2014/04/08 10:28:12, perkj ...
6 years, 8 months ago (2014-04-11 08:56:29 UTC) #5
perkj_chrome
https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/media_stream_audio_processor.cc File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/media_stream_audio_processor.cc#newcode163 content/renderer/media/media_stream_audio_processor.cc:163: goog_audio_mirroring_(false), why prefix with goog? https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/media_stream_audio_processor.cc#newcode305 content/renderer/media/media_stream_audio_processor.cc:305: // TODO(xians): ...
6 years, 8 months ago (2014-04-11 11:45:03 UTC) #6
tommi (sloooow) - chröme
https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/media_stream_audio_processor_options.cc File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/40001/content/renderer/media/media_stream_audio_processor_options.cc#newcode84 content/renderer/media/media_stream_audio_processor_options.cc:84: base::IntToString(kDefaultAudioConstraints[i].value), false); On 2014/04/11 08:56:30, xians1 wrote: > On ...
6 years, 8 months ago (2014-04-11 12:51:15 UTC) #7
no longer working on chromium
Hi Per and Tommi, PTAL. Andrew, could you please take a look at media_stream_audio_processor_options.cc::GetProperty()? And ...
6 years, 8 months ago (2014-04-11 16:47:05 UTC) #8
perkj_chrome
https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/media_stream_audio_processor.cc File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/100001/content/renderer/media/media_stream_audio_processor.cc#newcode305 content/renderer/media/media_stream_audio_processor.cc:305: // TODO(xians): goog_aec should be just echo_cancellation. On 2014/04/11 ...
6 years, 8 months ago (2014-04-14 12:15:19 UTC) #9
no longer working on chromium
Per, PTAL. https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/media_stream_audio_processor.cc File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/120001/content/renderer/media/media_stream_audio_processor.cc#newcode281 content/renderer/media/media_stream_audio_processor.cc:281: DCHECK(audio_constraints.IsValid()); On 2014/04/14 12:15:19, perkj wrote: > ...
6 years, 8 months ago (2014-04-14 14:40:50 UTC) #10
ajm
https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/media_stream_audio_processor.cc File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/media_stream_audio_processor.cc#newcode294 content/renderer/media/media_stream_audio_processor.cc:294: const bool echo_cancellation = audio_constraints.GetProperty( Is this defaulted to ...
6 years, 8 months ago (2014-04-15 01:29:57 UTC) #11
no longer working on chromium
https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/media_stream_audio_processor.cc File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/media_stream_audio_processor.cc#newcode294 content/renderer/media/media_stream_audio_processor.cc:294: const bool echo_cancellation = audio_constraints.GetProperty( On 2014/04/15 01:29:58, ajm ...
6 years, 8 months ago (2014-04-15 12:43:45 UTC) #12
perkj_chrome
LGTM with nits below. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/media_stream_audio_processor_unittest.cc File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/media_stream_audio_processor_unittest.cc#newcode292 content/renderer/media/media_stream_audio_processor_unittest.cc:292: for (size_t i = 0; ...
6 years, 8 months ago (2014-04-15 13:01:06 UTC) #13
ajm
lgtm on media_stream_audio_processor.cc. Have you verified with some real-time testing that this works properly? https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/media_stream_audio_processor.cc ...
6 years, 8 months ago (2014-04-16 05:17:42 UTC) #14
tommi (sloooow) - chröme
sorry for the delay in reviewing. I reviewed through 'media_stream_dependency_factory.cc' this time around. Will continue ...
6 years, 8 months ago (2014-04-17 10:56:41 UTC) #15
ajm
I agree with Tommi's comments around checking if kEchoCancellation was explicitly set by the app. ...
6 years, 8 months ago (2014-04-17 16:16:27 UTC) #16
no longer working on chromium
Tommi and Andrew, PTAL. https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/media_stream_audio_processor.cc File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/media_stream_audio_processor.cc#newcode295 content/renderer/media/media_stream_audio_processor.cc:295: MediaAudioConstraints::kEchoCancellation); On 2014/04/17 10:56:41, tommi ...
6 years, 8 months ago (2014-04-23 14:59:06 UTC) #17
no longer working on chromium
https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/media_stream_constraints_util.cc File content/renderer/media/media_stream_constraints_util.cc (right): https://codereview.chromium.org/227743004/diff/140001/content/renderer/media/media_stream_constraints_util.cc#newcode56 content/renderer/media/media_stream_constraints_util.cc:56: return base::StringToInt(value_str.utf8(), value); On 2014/04/17 10:56:41, tommi wrote: > ...
6 years, 8 months ago (2014-04-23 15:11:55 UTC) #18
no longer working on chromium
Andrew, FYI, the code will do a) if kEchoCancellation is not set explicitly. > If ...
6 years, 8 months ago (2014-04-23 18:16:47 UTC) #19
no longer working on chromium
A gentle ping.
6 years, 8 months ago (2014-04-24 14:54:19 UTC) #20
ajm
lgtm again on media_stream_audio_processor.cc, but with some potential concerns. https://codereview.chromium.org/227743004/diff/170001/content/renderer/media/media_stream_audio_processor_options.cc File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/170001/content/renderer/media/media_stream_audio_processor_options.cc#newcode127 content/renderer/media/media_stream_audio_processor_options.cc:127: ...
6 years, 8 months ago (2014-04-24 21:39:11 UTC) #21
tommi (sloooow) - chröme
will do a more thorough review later but I think I've mentioned the below point ...
6 years, 8 months ago (2014-04-25 15:09:31 UTC) #22
no longer working on chromium
Tommi, PTAL. Thanks, SX https://codereview.chromium.org/227743004/diff/170001/content/renderer/media/media_stream_audio_processor_options.cc File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/170001/content/renderer/media/media_stream_audio_processor_options.cc#newcode127 content/renderer/media/media_stream_audio_processor_options.cc:127: if (effects_ & media::AudioParameters::ECHO_CANCELLER) On ...
6 years, 7 months ago (2014-04-28 12:42:53 UTC) #23
no longer working on chromium
On 2014/04/28 12:42:53, xians1 wrote: > Tommi, PTAL. > > Thanks, > > SX > ...
6 years, 7 months ago (2014-04-28 14:44:29 UTC) #24
tommi (sloooow) - chröme
one comment... more coming today https://codereview.chromium.org/227743004/diff/190001/content/renderer/media/media_stream_audio_processor.cc File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/190001/content/renderer/media/media_stream_audio_processor.cc#newcode307 content/renderer/media/media_stream_audio_processor.cc:307: const bool echo_cancellation = ...
6 years, 7 months ago (2014-04-29 09:27:03 UTC) #25
tommi (sloooow) - chröme
got half way through it. We're still not communicating wrt GetEchoCancellationProperty. Also, I'm still seeing ...
6 years, 7 months ago (2014-04-30 10:58:43 UTC) #26
no longer working on chromium
Tommi, I hope I have already explained clearly that the stringtoint is needed and used ...
6 years, 7 months ago (2014-05-02 12:24:02 UTC) #27
tommi (sloooow) - chröme
Almost there. I still believe we can do better with the GetEchoCancellation function. https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/media_stream_audio_processor.cc File ...
6 years, 7 months ago (2014-05-03 10:26:27 UTC) #28
no longer working on chromium
Tommi, PTAL. Thanks. SX https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/media_stream_audio_processor.cc File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/media_stream_audio_processor.cc#newcode308 content/renderer/media/media_stream_audio_processor.cc:308: MediaAudioConstraints::kEchoCancellation); On 2014/05/03 10:26:28, tommi ...
6 years, 7 months ago (2014-05-05 12:55:55 UTC) #29
tommi (sloooow) - chröme
https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/media_stream_audio_processor.cc File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/227743004/diff/200001/content/renderer/media/media_stream_audio_processor.cc#newcode308 content/renderer/media/media_stream_audio_processor.cc:308: MediaAudioConstraints::kEchoCancellation); On 2014/05/05 12:55:56, xians1 wrote: > On 2014/05/03 ...
6 years, 7 months ago (2014-05-05 13:58:12 UTC) #30
no longer working on chromium
Tommi, another look?
6 years, 7 months ago (2014-05-07 13:08:28 UTC) #31
tommi (sloooow) - chröme
this is great. I've got a couple of minor requests and then we're done https://codereview.chromium.org/227743004/diff/260001/content/renderer/media/media_stream_audio_processor_options.cc ...
6 years, 7 months ago (2014-05-07 14:19:44 UTC) #32
no longer working on chromium
https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/media_stream_audio_processor_options.cc File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/media_stream_audio_processor_options.cc#newcode105 content/renderer/media/media_stream_audio_processor_options.cc:105: turn_off_constraints_as_default_ = true; On 2014/05/07 14:19:45, tommi wrote: > ...
6 years, 7 months ago (2014-05-07 15:44:59 UTC) #33
tommi (sloooow) - chröme
https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/media_stream_audio_processor_options.cc File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/media_stream_audio_processor_options.cc#newcode105 content/renderer/media/media_stream_audio_processor_options.cc:105: turn_off_constraints_as_default_ = true; On 2014/05/07 15:45:00, xians1 wrote: > ...
6 years, 7 months ago (2014-05-07 17:01:05 UTC) #34
perkj_chrome
https://codereview.chromium.org/227743004/diff/340001/content/renderer/media/media_stream_constraints_util.cc File content/renderer/media/media_stream_constraints_util.cc (right): https://codereview.chromium.org/227743004/diff/340001/content/renderer/media/media_stream_constraints_util.cc#newcode78 content/renderer/media/media_stream_constraints_util.cc:78: return base::StringToInt(value_str.utf8(), value); Humm - you are aware that ...
6 years, 7 months ago (2014-05-08 13:42:23 UTC) #35
no longer working on chromium
https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/media_stream_audio_processor_options.cc File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/320001/content/renderer/media/media_stream_audio_processor_options.cc#newcode105 content/renderer/media/media_stream_audio_processor_options.cc:105: turn_off_constraints_as_default_ = true; On 2014/05/07 17:01:06, tommi wrote: > ...
6 years, 7 months ago (2014-05-12 14:39:08 UTC) #36
tommi (sloooow) - chröme
lgtm https://codereview.chromium.org/227743004/diff/360001/content/renderer/media/media_stream_audio_processor_options.cc File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/360001/content/renderer/media/media_stream_audio_processor_options.cc#newcode173 content/renderer/media/media_stream_audio_processor_options.cc:173: if (key == kDefaultAudioConstraints[j].key && should this perhaps ...
6 years, 7 months ago (2014-05-13 14:19:40 UTC) #37
no longer working on chromium
landing now. https://codereview.chromium.org/227743004/diff/360001/content/renderer/media/media_stream_audio_processor_options.cc File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/227743004/diff/360001/content/renderer/media/media_stream_audio_processor_options.cc#newcode173 content/renderer/media/media_stream_audio_processor_options.cc:173: if (key == kDefaultAudioConstraints[j].key && On 2014/05/13 ...
6 years, 7 months ago (2014-05-15 10:04:25 UTC) #38
no longer working on chromium
The CQ bit was checked by xians@chromium.org
6 years, 7 months ago (2014-05-15 10:04:52 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/227743004/380001
6 years, 7 months ago (2014-05-15 10:05:28 UTC) #40
commit-bot: I haz the power
6 years, 7 months ago (2014-05-15 17:42:51 UTC) #41
Message was sent while issue was closed.
Change committed as 270728

Powered by Google App Engine
This is Rietveld 408576698