Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2677893002:
Adding functionality for activating for the WebRTC EchoCanceller3 functionality in Chrome. (Closed)
|
Created:
3 years, 10 months ago by peah Modified:
3 years, 10 months ago Reviewers:
Alexei Svitkine (slow), jochen (gone - plz use gerrit), tommi (sloooow) - chröme, hlundin-chromium CC:
avayvod+watch_chromium.org, chfremer+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, imcheng+watch_chromium.org, isheriff+watch_chromium.org, jam, jasonroberts+watch_google.com, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, xjz+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding functionality for activating for the WebRTC EchoCanceller3
functionality in Chrome.
This CL adds the ability to activate the upcoming WebRTC AEC3
functionality in Chrome. The AEC3 should be possible to activate
both via an experiment as a feature, and as a Chrome flag
(via chrome://flags and the command line).
BUG=688388
Review-Url: https://codereview.chromium.org/2677893002
Cr-Commit-Position: refs/heads/master@{#451251}
Committed: https://chromium.googlesource.com/chromium/src/+/1767e59153ee506fe6394efe4018b4cd3f2a2671
Patch Set 1 #
Total comments: 12
Patch Set 2 : Changes in response to reviewer comments #
Total comments: 1
Patch Set 3 : Changed the histogram name #
Total comments: 2
Patch Set 4 : Added WebRtcUseEchoCanceller3:disabled to histograms.xml and corrected the values #Patch Set 5 : Rebase #Patch Set 6 : Reverted formatting done by mistake #
Messages
Total messages: 36 (20 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Adding feature and feature support for activating the WebRTC EchoCanceller3 functionality in Chrome. BUG=crbug:688388 ========== to ========== Adding functionality for activating for the WebRTC EchoCanceller3 functionality in Chrome. BUG=crbug:688388 ==========
peah@chromium.org changed reviewers: + hlundin@chromium.org
Patchset #1 (id:40001) has been deleted
Mostly LG, but I think you've declared the feature too many times. See inline. https://codereview.chromium.org/2677893002/diff/60001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (left): https://codereview.chromium.org/2677893002/diff/60001/chrome/browser/about_fl... chrome/browser/about_flags.cc:1852: FEATURE_WITH_VARIATIONS_VALUE_TYPE( I think you should avoid changing the formatting for parts you don't touch. https://codereview.chromium.org/2677893002/diff/60001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2209: IDS_FLAGS_SHOW_ARC_FILES_APP_DESCRIPTION, kOsCrOS, Same here. Don't change format. https://codereview.chromium.org/2677893002/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/2677893002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:583: // Features for http://crbug.com/688388. This value is sent to WebRTC's echo Move this information to content_features.{h|cc}. https://codereview.chromium.org/2677893002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:585: const base::Feature kUseEchoCanceller3{"UseEchoCanceller3", You are defining this feature twice: here and in content_features.{h,cc}. I think the pattern is to use one of them. I can only guess that you must define it in content_features in order to connect it to chrome://flags, which means you should delete this one. Instead, you will have to include #include "content/public/common/content_features.h" in this file.
mcasas@chromium.org changed reviewers: + mcasas@chromium.org
Drive-by, don't mind me. Also, BUG=XYZ, don't need crbug: prepended. https://codereview.chromium.org/2677893002/diff/60001/content/public/common/c... File content/public/common/content_features.cc (right): https://codereview.chromium.org/2677893002/diff/60001/content/public/common/c... content/public/common/content_features.cc:264: const base::Feature kUseEchoCanceller3{"UseEchoCanceller3", s/UseEchoCanceller3/WebRtcUseEchoCanceller3/ https://codereview.chromium.org/2677893002/diff/60001/content/public/common/c... File content/public/common/content_features.h (right): https://codereview.chromium.org/2677893002/diff/60001/content/public/common/c... content/public/common/content_features.h:70: CONTENT_EXPORT extern const base::Feature kUseEchoCanceller3; This should be ordered alphabetically. Or just s/kUseEcho/kWebRtcUseEcho/ which will probably be better long term.
mcasas@chromium.org changed reviewers: - mcasas@chromium.org
Patchset #3 (id:90001) has been deleted
Description was changed from ========== Adding functionality for activating for the WebRTC EchoCanceller3 functionality in Chrome. BUG=crbug:688388 ========== to ========== Adding functionality for activating for the WebRTC EchoCanceller3 functionality in Chrome. This CL adds the ability to activate the upcoming WebRTC AEC3 functionality in Chrome. The AEC3 should be possible to activate both via an experiment as a feature, and as a Chrome flag (via chrome://flags and the command line). BUG=crbug:688388 ==========
peah@chromium.org changed reviewers: + asvitkine@chromium.org, jochen@chromium.org, tommi@chromium.org
Hi, I've created this CL for allowing activation in Chrome of the upcoming WebRTC new echo canceller (AEC3) functionality. I may have added some of you as redundant reviewers, but please let me know in that case. I would appreciate if you could have a close look at how the AEC3 feature is hooked up as a chrome feature as well as a Chrome flag. In particular, I'm not sure that I got the naming of the Chrome flag correct. I left a comment about that in patch #2 that it would be great if you can have a look at! https://codereview.chromium.org/2677893002/diff/60001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (left): https://codereview.chromium.org/2677893002/diff/60001/chrome/browser/about_fl... chrome/browser/about_flags.cc:1852: FEATURE_WITH_VARIATIONS_VALUE_TYPE( On 2017/02/09 10:20:45, hlundin-chromium wrote: > I think you should avoid changing the formatting for parts you don't touch. Thanks! Done. https://codereview.chromium.org/2677893002/diff/60001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2209: IDS_FLAGS_SHOW_ARC_FILES_APP_DESCRIPTION, kOsCrOS, On 2017/02/09 10:20:45, hlundin-chromium wrote: > Same here. Don't change format. Thanks! Done. https://codereview.chromium.org/2677893002/diff/60001/content/public/common/c... File content/public/common/content_features.cc (right): https://codereview.chromium.org/2677893002/diff/60001/content/public/common/c... content/public/common/content_features.cc:264: const base::Feature kUseEchoCanceller3{"UseEchoCanceller3", On 2017/02/10 18:13:01, mcasas wrote: > s/UseEchoCanceller3/WebRtcUseEchoCanceller3/ Thanks! Done. https://codereview.chromium.org/2677893002/diff/60001/content/public/common/c... File content/public/common/content_features.h (right): https://codereview.chromium.org/2677893002/diff/60001/content/public/common/c... content/public/common/content_features.h:70: CONTENT_EXPORT extern const base::Feature kUseEchoCanceller3; On 2017/02/10 18:13:01, mcasas wrote: > This should be ordered alphabetically. > Or just s/kUseEcho/kWebRtcUseEcho/ which > will probably be better long term. Thanks! Great name suggestion as well! Done. https://codereview.chromium.org/2677893002/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/2677893002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:583: // Features for http://crbug.com/688388. This value is sent to WebRTC's echo On 2017/02/09 10:20:45, hlundin-chromium wrote: > Move this information to content_features.{h|cc}. Done. https://codereview.chromium.org/2677893002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:585: const base::Feature kUseEchoCanceller3{"UseEchoCanceller3", On 2017/02/09 10:20:45, hlundin-chromium wrote: > You are defining this feature twice: here and in content_features.{h,cc}. I > think the pattern is to use one of them. I can only guess that you must define > it in content_features in order to connect it to chrome://flags, which means you > should delete this one. Instead, you will have to include > #include "content/public/common/content_features.h" > in this file. Thanks! Great suggestion! Done. https://codereview.chromium.org/2677893002/diff/70001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2677893002/diff/70001/chrome/browser/about_fl... chrome/browser/about_flags.cc:752: {"WebRtcUseEchoCanceller3", IDS_FLAGS_WEBRTC_ECHO_CANCELLER_3_NAME, I'm not sure about whether the "name" of the flag should be WebRtcUseEchoCanceller3? This is the name that ends up in chrome://flags. All the other flags have names of the format "enable-<feature>" but if I use something that is different from the name of the feature that the flag controls, I cannot set this via commandline. Setting the feature via chrome://flags works though. By naming the feature, and the flag the same, I'm able to set the flag both via command line and chrome://flags Please let me know if this is how it is expected to be done, or whether I should use some other naming!
Description was changed from ========== Adding functionality for activating for the WebRTC EchoCanceller3 functionality in Chrome. This CL adds the ability to activate the upcoming WebRTC AEC3 functionality in Chrome. The AEC3 should be possible to activate both via an experiment as a feature, and as a Chrome flag (via chrome://flags and the command line). BUG=crbug:688388 ========== to ========== Adding functionality for activating for the WebRTC EchoCanceller3 functionality in Chrome. This CL adds the ability to activate the upcoming WebRTC AEC3 functionality in Chrome. The AEC3 should be possible to activate both via an experiment as a feature, and as a Chrome flag (via chrome://flags and the command line). BUG=688388 ==========
lgtm
lgtm https://codereview.chromium.org/2677893002/diff/110001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2677893002/diff/110001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:96475: + <int value="241711238" label="WebRtcUseEchoCanceller3:disabled"/> I think you need to add both :enabled & :disabled here.
lgtm with Alexei's comment.
https://codereview.chromium.org/2677893002/diff/110001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2677893002/diff/110001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:96475: + <int value="241711238" label="WebRtcUseEchoCanceller3:disabled"/> On 2017/02/15 17:16:18, Alexei Svitkine (slow) wrote: > I think you need to add both :enabled & :disabled here. Thanks! Done.
lgtm
Thanks everyone for the quick review!
The CQ bit was checked by peah@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, tommi@chromium.org, hlundin@chromium.org Link to the patchset: https://codereview.chromium.org/2677893002/#ps130001 (title: "Added WebRtcUseEchoCanceller3:disabled to histograms.xml and corrected the values")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for tools/metrics/histograms/histograms.xml: While running git apply --index -p1; error: patch failed: tools/metrics/histograms/histograms.xml:96700 error: tools/metrics/histograms/histograms.xml: patch does not apply Patch: tools/metrics/histograms/histograms.xml Index: tools/metrics/histograms/histograms.xml diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index 25e02d82dd501d7b4a2d8fc34a020a7934ed7aa9..50d08e009ae69a40bd1f42c0c2964b45b0ef302d 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -96315,6 +96315,7 @@ value. <int value="-535208779" label="enable-native-cups"/> <int value="-531810064" label="saveas-menu-label"/> <int value="-528927088" label="AutofillCreditCardPopupLayout:disabled"/> + <int value="-528149352" label="WebRtcUseEchoCanceller3:enabled"/> <int value="-519960638" label="enable-site-engagement-service"/> <int value="-518104091" label="NewAudioRenderingMixingStrategy:enabled"/> <int value="-516845951" label="enable-embedded-extension-options"/> @@ -96700,6 +96701,7 @@ value. <int value="1220171692" label="SpeculativeLaunchServiceWorker:enabled"/> <int value="1220464509" label="enable-first-run-ui-transitions"/> <int value="1221559505" label="enable-spelling-feedback-field-trial"/> + <int value="1222017136" label="WebRtcUseEchoCanceller3:disabled"/> <int value="1235800887" label="V8Ignition:enabled"/> <int value="1237297772" label="no-pings"/> <int value="1245889469" label="enable-surface-worker"/>
The CQ bit was checked by peah@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, jochen@chromium.org, tommi@chromium.org, hlundin@chromium.org Link to the patchset: https://codereview.chromium.org/2677893002/#ps170001 (title: "Reverted formatting done by mistake")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by peah@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 170001, "attempt_start_ts": 1487310007405640,
"parent_rev": "a16900cd0e022d0ef255e55f121bfb8639194477", "commit_rev":
"1767e59153ee506fe6394efe4018b4cd3f2a2671"}
Message was sent while issue was closed.
Description was changed from ========== Adding functionality for activating for the WebRTC EchoCanceller3 functionality in Chrome. This CL adds the ability to activate the upcoming WebRTC AEC3 functionality in Chrome. The AEC3 should be possible to activate both via an experiment as a feature, and as a Chrome flag (via chrome://flags and the command line). BUG=688388 ========== to ========== Adding functionality for activating for the WebRTC EchoCanceller3 functionality in Chrome. This CL adds the ability to activate the upcoming WebRTC AEC3 functionality in Chrome. The AEC3 should be possible to activate both via an experiment as a feature, and as a Chrome flag (via chrome://flags and the command line). BUG=688388 Review-Url: https://codereview.chromium.org/2677893002 Cr-Commit-Position: refs/heads/master@{#451251} Committed: https://chromium.googlesource.com/chromium/src/+/1767e59153ee506fe6394efe4018... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:170001) as https://chromium.googlesource.com/chromium/src/+/1767e59153ee506fe6394efe4018... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
