Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2801853005:
Create a private API for controlling WebRTC's AEC3 (Closed)
|
Created:
3 years, 8 months ago by hlundin-chromium Modified:
3 years, 8 months ago Reviewers:
Henrik Grunell, Devlin, jochen (gone - plz use gerrit), Guido Urdaneta, Alexei Svitkine (slow), mnilsson, Mike West CC:
arv+watch_chromium.org, asvitkine+watch_chromium.org, avayvod+watch_chromium.org, chfremer+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, extensions-reviews_chromium.org, feature-media-reviews_chromium.org, imcheng+watch_chromium.org, isheriff+watch_chromium.org, jam, jasonroberts+watch_google.com, miu+watch_chromium.org, mlamouri+watch-content_chromium.org, mnilsson, nasko+codewatch_chromium.org, posciak+watch_chromium.org, xjz+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate a private API for controlling WebRTC's AEC3
This change opens up a new API to the Hangouts service extension. The
purpose of the API is to let the application control which version of
echo canceler will be used in WebRTC -- the old AEC2 or the new AEC3.
The CL contains the API in Hangouts service extension, the
corresponding API in webrtc_audio_private_api, and the required wiring
to reach the MediaStreamAudioProcessor where WebRTC's AudioProcessing
module lives.
BUG=708475
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2801853005
Cr-Commit-Position: refs/heads/master@{#463971}
Committed: https://chromium.googlesource.com/chromium/src/+/95829c0d06a5bcfb677bfd1d65799a43ddae4f46
Patch Set 1 #
Total comments: 29
Patch Set 2 : Changes after grunell's first round of comments #Patch Set 3 : Remember the override in RPHI #
Total comments: 16
Patch Set 4 : After grunell's second round of comments #
Total comments: 29
Patch Set 5 : After grunell and devlin #
Total comments: 2
Patch Set 6 : Initialize has_echo_cancellation_ #
Total comments: 2
Patch Set 7 : Fix IDL comment #Patch Set 8 : Rebase and rename a constraint #Patch Set 9 : Fixing typo #Messages
Total messages: 53 (25 generated)
Description was changed from ========== Create a private API for controlling WebRTC's AEC3 This change opens up a new API to the Hangouts service extension. The purpose of the API is to let the application control which version of echo canceler will be used in WebRTC -- the old AEC2 or the new AEC3. The CL contains the API in Hangouts service extension, the corresponding API in webrtc_audio_private_api, and the required wiring to reach the MediaStreamAudioProcessor where WebRTC's AudioProcessing module lives. BUG=708475 ========== to ========== Create a private API for controlling WebRTC's AEC3 This change opens up a new API to the Hangouts service extension. The purpose of the API is to let the application control which version of echo canceler will be used in WebRTC -- the old AEC2 or the new AEC3. The CL contains the API in Hangouts service extension, the corresponding API in webrtc_audio_private_api, and the required wiring to reach the MediaStreamAudioProcessor where WebRTC's AudioProcessing module lives. BUG=708475 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== Create a private API for controlling WebRTC's AEC3 This change opens up a new API to the Hangouts service extension. The purpose of the API is to let the application control which version of echo canceler will be used in WebRTC -- the old AEC2 or the new AEC3. The CL contains the API in Hangouts service extension, the corresponding API in webrtc_audio_private_api, and the required wiring to reach the MediaStreamAudioProcessor where WebRTC's AudioProcessing module lives. BUG=708475 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Create a private API for controlling WebRTC's AEC3 This change opens up a new API to the Hangouts service extension. The purpose of the API is to let the application control which version of echo canceler will be used in WebRTC -- the old AEC2 or the new AEC3. The CL contains the API in Hangouts service extension, the corresponding API in webrtc_audio_private_api, and the required wiring to reach the MediaStreamAudioProcessor where WebRTC's AudioProcessing module lives. BUG=708475 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
hlundin@chromium.org changed reviewers: + asvitkine@chromium.org, grunell@chromium.org, guidou@chromium.org, tommi@chromium.org
Reviewers, PTAL. grunell: General sanity of all changes. chrome/browser/resources/hangout_services/thunk.js tommi: content/renderer/media/* asvitkine: content/browser/renderer_host/render_process_host_impl.* extensions/browser/extension_function_histogram_value.h tools/metrics/histograms/histograms.xml cpu: chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.h chrome/common/extensions/api/webrtc_audio_private.idl content/public/browser/render_process_host.h content/public/test/mock_render_process_host.cc content/public/test/mock_render_process_host.h meacer: content/common/media/aec_dump_messages.h Thanks a lot! https://codereview.chromium.org/2801853005/diff/20001/content/common/media/ae... File content/common/media/aec_dump_messages.h (right): https://codereview.chromium.org/2801853005/diff/20001/content/common/media/ae... content/common/media/aec_dump_messages.h:38: // TODO(hlundin) Move to new file? Should this be moved to another file? If so, which file? audio_messages.h? New file?
hlundin@chromium.org changed reviewers: + cpu@chromium.org, meacer@chromium.org
Actually adding cpu@ and meacer@.
hlundin@chromium.org changed reviewers: + jochen@chromium.org, mkwst@chromium.org - cpu@chromium.org, meacer@chromium.org
hlundin@chromium.org changed reviewers: - guidou@chromium.org
Adding jochen@ and mkwst@ Removing cpu@ and meacer@ grunell: General sanity of all changes. chrome/browser/resources/hangout_services/thunk.js tommi: content/renderer/media/* asvitkine: extensions/browser/extension_function_histogram_value.h tools/metrics/histograms/histograms.xml mkwst: content/common/media/aec_dump_messages.h jochen: chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.* chrome/common/extensions/api/webrtc_audio_private.idl content/browser/renderer_host/render_process_host_impl.* content/public/* Thanks, and sorry for the messing with the reviewer selection.
hlundin@chromium.org changed reviewers: + guidou@chromium.org - tommi@chromium.org
... aaaand changing to guidou@ for content/renderer/media/*
jochen@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
rdevlin.cronin should review this for extensions
On 2017/04/07 at 06:59:51, jochen wrote: > rdevlin.cronin should review this for extensions Likewise, the new IPC seems reasonable, but I'll hold off on stamping this until the design is solidified with the extensions team.
cc: mnilsson
First round. Also update the version number in the extension manifest file. https://codereview.chromium.org/2801853005/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc (right): https://codereview.chromium.org/2801853005/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:554: bool WebrtcAudioPrivateSetAudioExperimentsFunction::RunAsync() { DCHECK_CURRENTLY_ON(BrowserThread::UI); https://codereview.chromium.org/2801853005/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:563: return false; Also SendResponse(false); https://codereview.chromium.org/2801853005/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:565: host->SetEchoCanceller3(*params->audio_experiments.enable_aec3); The optional callback in the interface is never used. It seems like it should be removed. Or should success/failure be sent back from the MediaStreamAudioProcessor? https://codereview.chromium.org/2801853005/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2801853005/diff/20001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2350: // TODO(hlundin) Consider changing name for aec_dump_consumers_. I think this should be done. I'm OK with a follow-up CL. Can you file a bug to track? https://codereview.chromium.org/2801853005/diff/20001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2353: SetEchoCanceller3ForId(enable, *it); Just do Send(new AudioProcessingMsg_EnableAec3(id, enable)); here and remove SetEchoCanceller3ForId(). https://codereview.chromium.org/2801853005/diff/20001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:3009: } Here, you need to set experiments for a new consumer. So the experiments must be stored somewhere, either this class or if possible to access the extension API class (not sure about this) in that class. Otherwise, a getUserMedia call after being set won't honor the settings. https://codereview.chromium.org/2801853005/diff/20001/content/common/media/ae... File content/common/media/aec_dump_messages.h (right): https://codereview.chromium.org/2801853005/diff/20001/content/common/media/ae... content/common/media/aec_dump_messages.h:38: // TODO(hlundin) Move to new file? On 2017/04/06 19:09:08, hlundin-chromium wrote: > Should this be moved to another file? If so, which file? audio_messages.h? New > file? Keep it in this file, but this file should be renamed to audio_processing_messages.h and the AEC dump message filter class to AudioProcessingMessageFilter. I'm fine with doing this in a follow-up CL together with the change in another comment. Update todo and refer to bug. https://codereview.chromium.org/2801853005/diff/20001/content/public/browser/... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/2801853005/diff/20001/content/public/browser/... content/public/browser/render_process_host.h:248: // Enables or disables WebRTC's echo canceller AEC3. Disabled implies This will be removed at some later point when fully rolled out, right? Add comment about this. https://codereview.chromium.org/2801853005/diff/20001/content/renderer/media/... File content/renderer/media/aec_dump_message_filter.h (right): https://codereview.chromium.org/2801853005/diff/20001/content/renderer/media/... content/renderer/media/aec_dump_message_filter.h:24: class CONTENT_EXPORT AecDumpMessageFilter : public IPC::MessageFilter { Add TODO comment for renaming the class, refer to bug. https://codereview.chromium.org/2801853005/diff/20001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/2801853005/diff/20001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:491: if (override_aec3_ != base::Optional<bool>(enable)) { Early returns instead in this function, e.g. if (override_aec3_ == base::Optional<bool>(enable)) return; ... https://codereview.chromium.org/2801853005/diff/20001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:494: DCHECK(audio_processing_); Remove (redundant). It will crash on the next line if not set. https://codereview.chromium.org/2801853005/diff/20001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:497: apm_config.echo_canceller3.enabled = enable; I'm not sure how this is supposed to work. If not overridden, should it not go back to the features::kWebRtcUseEchoCanceller3 value? https://codereview.chromium.org/2801853005/diff/20001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.h (right): https://codereview.chromium.org/2801853005/diff/20001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.h:197: base::Optional<bool> override_aec3_; Add comment how the different values (true, false, not set) is used.
Thanks for the comments! Please, see new patch set. https://codereview.chromium.org/2801853005/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc (right): https://codereview.chromium.org/2801853005/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:554: bool WebrtcAudioPrivateSetAudioExperimentsFunction::RunAsync() { On 2017/04/10 08:13:28, Henrik Grunell wrote: > DCHECK_CURRENTLY_ON(BrowserThread::UI); Done. https://codereview.chromium.org/2801853005/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:563: return false; On 2017/04/10 08:13:28, Henrik Grunell wrote: > Also > SendResponse(false); Done. https://codereview.chromium.org/2801853005/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:565: host->SetEchoCanceller3(*params->audio_experiments.enable_aec3); On 2017/04/10 08:13:28, Henrik Grunell wrote: > The optional callback in the interface is never used. It seems like it should be > removed. Or should success/failure be sent back from the > MediaStreamAudioProcessor? It should probably be removed. Done. https://codereview.chromium.org/2801853005/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2801853005/diff/20001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2350: // TODO(hlundin) Consider changing name for aec_dump_consumers_. On 2017/04/10 08:13:28, Henrik Grunell wrote: > I think this should be done. I'm OK with a follow-up CL. Can you file a bug to > track? Done. https://codereview.chromium.org/2801853005/diff/20001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2353: SetEchoCanceller3ForId(enable, *it); On 2017/04/10 08:13:28, Henrik Grunell wrote: > Just do > > Send(new AudioProcessingMsg_EnableAec3(id, enable)); > > here and remove SetEchoCanceller3ForId(). Done. https://codereview.chromium.org/2801853005/diff/20001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:3009: } On 2017/04/10 08:13:28, Henrik Grunell wrote: > Here, you need to set experiments for a new consumer. So the experiments must be > stored somewhere, either this class or if possible to access the extension API > class (not sure about this) in that class. > > Otherwise, a getUserMedia call after being set won't honor the settings. Done. Added a class member to remember the state. https://codereview.chromium.org/2801853005/diff/20001/content/common/media/ae... File content/common/media/aec_dump_messages.h (right): https://codereview.chromium.org/2801853005/diff/20001/content/common/media/ae... content/common/media/aec_dump_messages.h:38: // TODO(hlundin) Move to new file? On 2017/04/10 08:13:28, Henrik Grunell wrote: > On 2017/04/06 19:09:08, hlundin-chromium wrote: > > Should this be moved to another file? If so, which file? audio_messages.h? New > > file? > > Keep it in this file, but this file should be renamed to > audio_processing_messages.h and the AEC dump message filter class to > AudioProcessingMessageFilter. I'm fine with doing this in a follow-up CL > together with the change in another comment. Update todo and refer to bug. Done. https://codereview.chromium.org/2801853005/diff/20001/content/public/browser/... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/2801853005/diff/20001/content/public/browser/... content/public/browser/render_process_host.h:248: // Enables or disables WebRTC's echo canceller AEC3. Disabled implies On 2017/04/10 08:13:28, Henrik Grunell wrote: > This will be removed at some later point when fully rolled out, right? Add > comment about this. Done. https://codereview.chromium.org/2801853005/diff/20001/content/renderer/media/... File content/renderer/media/aec_dump_message_filter.h (right): https://codereview.chromium.org/2801853005/diff/20001/content/renderer/media/... content/renderer/media/aec_dump_message_filter.h:24: class CONTENT_EXPORT AecDumpMessageFilter : public IPC::MessageFilter { On 2017/04/10 08:13:28, Henrik Grunell wrote: > Add TODO comment for renaming the class, refer to bug. Done. https://codereview.chromium.org/2801853005/diff/20001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/2801853005/diff/20001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:491: if (override_aec3_ != base::Optional<bool>(enable)) { On 2017/04/10 08:13:28, Henrik Grunell wrote: > Early returns instead in this function, e.g. > > if (override_aec3_ == base::Optional<bool>(enable)) > return; > > ... Done. https://codereview.chromium.org/2801853005/diff/20001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:494: DCHECK(audio_processing_); On 2017/04/10 08:13:28, Henrik Grunell wrote: > Remove (redundant). It will crash on the next line if not set. Done. https://codereview.chromium.org/2801853005/diff/20001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:497: apm_config.echo_canceller3.enabled = enable; On 2017/04/10 08:13:28, Henrik Grunell wrote: > I'm not sure how this is supposed to work. If not overridden, should it not go > back to the features::kWebRtcUseEchoCanceller3 value? If not overridden, it will already be at the value determined by features::kWebRtcUseEchoCanceller3. https://codereview.chromium.org/2801853005/diff/20001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.h (right): https://codereview.chromium.org/2801853005/diff/20001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.h:197: base::Optional<bool> override_aec3_; On 2017/04/10 08:13:28, Henrik Grunell wrote: > Add comment how the different values (true, false, not set) is used. Done.
Aaaaand the version in the manifest file. :) https://codereview.chromium.org/2801853005/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc (right): https://codereview.chromium.org/2801853005/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:565: host->SetEchoCanceller3(*params->audio_experiments.enable_aec3); On 2017/04/10 10:10:16, hlundin-chromium wrote: > On 2017/04/10 08:13:28, Henrik Grunell wrote: > > The optional callback in the interface is never used. It seems like it should > be > > removed. Or should success/failure be sent back from the > > MediaStreamAudioProcessor? > > It should probably be removed. Done. Acknowledged. https://codereview.chromium.org/2801853005/diff/20001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/2801853005/diff/20001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:497: apm_config.echo_canceller3.enabled = enable; On 2017/04/10 10:10:16, hlundin-chromium wrote: > On 2017/04/10 08:13:28, Henrik Grunell wrote: > > I'm not sure how this is supposed to work. If not overridden, should it not go > > back to the features::kWebRtcUseEchoCanceller3 value? > > If not overridden, it will already be at the value determined by > features::kWebRtcUseEchoCanceller3. I realized that it can never be un-overridden. Setting it via the API means that it overrides. Fine. https://codereview.chromium.org/2801853005/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc (right): https://codereview.chromium.org/2801853005/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:564: SendResponse(true); true -> false https://codereview.chromium.org/2801853005/diff/60001/chrome/browser/resource... File chrome/browser/resources/hangout_services/thunk.js (right): https://codereview.chromium.org/2801853005/diff/60001/chrome/browser/resource... chrome/browser/resources/hangout_services/thunk.js:209: return true; Here, you have to do doSendResponse(); return false; (instead of return true;) See for example logging.uploadOnRenderClose above. https://codereview.chromium.org/2801853005/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2801853005/diff/60001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2350: // TODO(hlundin) Change name for aec_dump_consumers_; http://crbug.com/709919. Super-nit: add : after ) https://codereview.chromium.org/2801853005/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/2801853005/diff/60001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:570: // When this variable is not set, the use of AEC3 is governed by the Finch Great comment, thanks! https://codereview.chromium.org/2801853005/diff/60001/content/common/media/ae... File content/common/media/aec_dump_messages.h (right): https://codereview.chromium.org/2801853005/diff/60001/content/common/media/ae... content/common/media/aec_dump_messages.h:38: // TODO(hlundin) Rename file to reflect expanded use; http://crbug.com/709919. Same super-nit. https://codereview.chromium.org/2801853005/diff/60001/content/renderer/media/... File content/renderer/media/aec_dump_message_filter.h (right): https://codereview.chromium.org/2801853005/diff/60001/content/renderer/media/... content/renderer/media/aec_dump_message_filter.h:24: // TODO(hlundin) Rename class to reflect expanded use; http://crbug.com/709919. Nit: Here too. https://codereview.chromium.org/2801853005/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/2801853005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:495: if (has_echo_cancellation_) I suppose it should be !has_echo_cancellation_ https://codereview.chromium.org/2801853005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:499: if (apm_config.echo_canceller3.enabled != enable) { Here too. if (apm_config.echo_canceller3.enabled == enable) return;
Thanks! See new patch set. https://codereview.chromium.org/2801853005/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc (right): https://codereview.chromium.org/2801853005/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:564: SendResponse(true); On 2017/04/10 12:33:47, Henrik Grunell wrote: > true -> false Darn... Thanks! https://codereview.chromium.org/2801853005/diff/60001/chrome/browser/resource... File chrome/browser/resources/hangout_services/thunk.js (right): https://codereview.chromium.org/2801853005/diff/60001/chrome/browser/resource... chrome/browser/resources/hangout_services/thunk.js:209: return true; On 2017/04/10 12:33:47, Henrik Grunell wrote: > Here, you have to do > > doSendResponse(); > return false; > > (instead of return true;) > > See for example logging.uploadOnRenderClose above. Done. https://codereview.chromium.org/2801853005/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2801853005/diff/60001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2350: // TODO(hlundin) Change name for aec_dump_consumers_; http://crbug.com/709919. On 2017/04/10 12:33:47, Henrik Grunell wrote: > Super-nit: add : after ) Done, but it breaks my line-length optimization... :-( https://codereview.chromium.org/2801853005/diff/60001/content/common/media/ae... File content/common/media/aec_dump_messages.h (right): https://codereview.chromium.org/2801853005/diff/60001/content/common/media/ae... content/common/media/aec_dump_messages.h:38: // TODO(hlundin) Rename file to reflect expanded use; http://crbug.com/709919. On 2017/04/10 12:33:47, Henrik Grunell wrote: > Same super-nit. Done. This one still fits in one line. https://codereview.chromium.org/2801853005/diff/60001/content/renderer/media/... File content/renderer/media/aec_dump_message_filter.h (right): https://codereview.chromium.org/2801853005/diff/60001/content/renderer/media/... content/renderer/media/aec_dump_message_filter.h:24: // TODO(hlundin) Rename class to reflect expanded use; http://crbug.com/709919. On 2017/04/10 12:33:47, Henrik Grunell wrote: > Nit: Here too. Done. https://codereview.chromium.org/2801853005/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/2801853005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:495: if (has_echo_cancellation_) On 2017/04/10 12:33:47, Henrik Grunell wrote: > I suppose it should be !has_echo_cancellation_ Holy cow! I'm sloppy today... Good thing you are on your toes. Thanks! https://codereview.chromium.org/2801853005/diff/60001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:499: if (apm_config.echo_canceller3.enabled != enable) { On 2017/04/10 12:33:47, Henrik Grunell wrote: > Here too. > > if (apm_config.echo_canceller3.enabled == enable) > return; Done.
lgtm with nits fixed. https://codereview.chromium.org/2801853005/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2801853005/diff/60001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.cc:2350: // TODO(hlundin) Change name for aec_dump_consumers_; http://crbug.com/709919. On 2017/04/10 13:03:17, hlundin-chromium wrote: > On 2017/04/10 12:33:47, Henrik Grunell wrote: > > Super-nit: add : after ) > > Done, but it breaks my line-length optimization... :-( Sorry... :o https://codereview.chromium.org/2801853005/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/2801853005/diff/80001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:491: if (override_aec3_ == base::Optional<bool>(enable)) Nit: can be written if (override_aec3_ == enable) https://codereview.chromium.org/2801853005/diff/80001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:494: override_aec3_ = base::Optional<bool>(enable); Nit: override_aec3_ = enable; https://codereview.chromium.org/2801853005/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/2801853005/diff/80001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_unittest.cc:606: TEST_F(MediaStreamAudioProcessorTest, TestAec3Switch) { Great tests. Interesting that no test has a InitializeAudioProcessingModule() call. It would be nice to have a test for regression verifying that the override has effect when it's called. However given that this feature is temporary I think these tests are enough.
content/renderer/media lgtm
One more thing I realized. https://codereview.chromium.org/2801853005/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/2801853005/diff/80001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:574: base::Optional<bool> override_aec3_; Actually, I realized that this belongs in AecDumpMessageFilter on the render side. The reason the AEC dump is handled in this class at consumer registration is that a file needs to be opened here on the browser side. That's not the case for the AEC3 override obviously, so it can be closer to where it's used. So, please move it there. (see other comment.) https://codereview.chromium.org/2801853005/diff/80001/content/renderer/media/... File content/renderer/media/aec_dump_message_filter.cc (right): https://codereview.chromium.org/2801853005/diff/80001/content/renderer/media/... content/renderer/media/aec_dump_message_filter.cc:50: Possibly set AEC3 here on the new delegate instead of in RPHI.
mnilsson@chromium.org changed reviewers: + mnilsson@chromium.org
https://codereview.chromium.org/2801853005/diff/80001/chrome/browser/resource... File chrome/browser/resources/hangout_services/thunk.js (right): https://codereview.chromium.org/2801853005/diff/80001/chrome/browser/resource... chrome/browser/resources/hangout_services/thunk.js:208: requestInfo, origin, experiments); Why not embed the doSendResponse as a callback method to setAudioExperiments (like other methods in this file)? Wouldn't it otherwise be racy to know when the experiments have actually been set?
https://codereview.chromium.org/2801853005/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc (right): https://codereview.chromium.org/2801853005/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:228: if (request.guest_process_id.get()) nit: no need for .get() (same for below) https://codereview.chromium.org/2801853005/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:242: error_ = extensions::ErrorUtils::FormatErrorMessage( GetTabById() will populate this error for you if you pass it in. https://codereview.chromium.org/2801853005/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:247: if (!contents) { This should never happen if GetTabById() succeeds. If you want some assertion here, you could [D]CHECK(web_contents), but there shouldn't need to be a branch. https://codereview.chromium.org/2801853005/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:252: if (expected_origin.spec() != security_origin) { Using strings to compare origins is scary for lots of reasons. Can we instead use the url libraries? https://codereview.chromium.org/2801853005/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.h (right): https://codereview.chromium.org/2801853005/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.h:107: content::RenderProcessHost* RphFromRequest( Nit: avoid uncommon abbreviation in names [1]. It's longer, but this should be GetRenderProcessHostFromRequest(). [1] https://google.github.io/styleguide/cppguide.html#General_Naming_Rules https://codereview.chromium.org/2801853005/diff/80001/chrome/common/extension... File chrome/common/extensions/api/webrtc_audio_private.idl (right): https://codereview.chromium.org/2801853005/diff/80001/chrome/common/extension... chrome/common/extensions/api/webrtc_audio_private.idl:85: static void setAudioExperiments(RequestInfo request, Document the parameters this accepts. Also, probably add an optional callback unless there's a concrete reason not to.
https://codereview.chromium.org/2801853005/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc (right): https://codereview.chromium.org/2801853005/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:247: if (!contents) { On 2017/04/10 21:34:25, Devlin wrote: > This should never happen if GetTabById() succeeds. If you want some assertion > here, you could [D]CHECK(web_contents), but there shouldn't need to be a branch. Good to know, thanks. Henrik L, remove this block completely. Dereference on next line will act as a check. https://codereview.chromium.org/2801853005/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:252: if (expected_origin.spec() != security_origin) { On 2017/04/10 21:34:24, Devlin wrote: > Using strings to compare origins is scary for lots of reasons. Can we instead > use the url libraries? Exactly which way do you suggest to compare? Origin::IsSameOriginWith()? Note that the security origin to compare with is given as a string by the extension. This is an existing design. Given that this code is taken from another extension API (see comment before function), I suggest leaving this as is for now, file a bug for changing this in both places (and probably consolidate the two functions). A simple change is of course fine. https://codereview.chromium.org/2801853005/diff/80001/chrome/browser/resource... File chrome/browser/resources/hangout_services/thunk.js (right): https://codereview.chromium.org/2801853005/diff/80001/chrome/browser/resource... chrome/browser/resources/hangout_services/thunk.js:208: requestInfo, origin, experiments); On 2017/04/10 19:15:45, mnilsson wrote: > Why not embed the doSendResponse as a callback method to setAudioExperiments > (like other methods in this file)? Wouldn't it otherwise be racy to know when > the experiments have actually been set? If the application needs to know exactly when it has been set then yes. (Or if success/failure information is needed.) I don't see why that would be necessary though. If it is, then by all means there should be a callback. https://codereview.chromium.org/2801853005/diff/80001/chrome/common/extension... File chrome/common/extensions/api/webrtc_audio_private.idl (right): https://codereview.chromium.org/2801853005/diff/80001/chrome/common/extension... chrome/common/extensions/api/webrtc_audio_private.idl:85: static void setAudioExperiments(RequestInfo request, On 2017/04/10 21:34:25, Devlin wrote: > Document the parameters this accepts. Also, probably add an optional callback > unless there's a concrete reason not to. I asked the author to remove the callback that was originally there since there is no need to signal success/failure (and it was actually never run in the original patch). There's also discussion in another comment whether the application would need to know when the operation has finished. If that's not necessary either, I see no reason for a callback.
Fixed comments in new patch set. PTAL. https://codereview.chromium.org/2801853005/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc (right): https://codereview.chromium.org/2801853005/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:228: if (request.guest_process_id.get()) On 2017/04/10 21:34:24, Devlin wrote: > nit: no need for .get() (same for below) Done. https://codereview.chromium.org/2801853005/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:242: error_ = extensions::ErrorUtils::FormatErrorMessage( On 2017/04/10 21:34:24, Devlin wrote: > GetTabById() will populate this error for you if you pass it in. Iiuc, this is only true for the version of GetTabById that is defined in https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/tabs/tabs_..., which is in an unnamed namespace in that file. So I cannot use it here. https://codereview.chromium.org/2801853005/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:247: if (!contents) { On 2017/04/11 06:49:39, Henrik Grunell wrote: > On 2017/04/10 21:34:25, Devlin wrote: > > This should never happen if GetTabById() succeeds. If you want some assertion > > here, you could [D]CHECK(web_contents), but there shouldn't need to be a > branch. > > Good to know, thanks. Henrik L, remove this block completely. Dereference on > next line will act as a check. Done. https://codereview.chromium.org/2801853005/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:252: if (expected_origin.spec() != security_origin) { On 2017/04/11 06:49:39, Henrik Grunell wrote: > On 2017/04/10 21:34:24, Devlin wrote: > > Using strings to compare origins is scary for lots of reasons. Can we instead > > use the url libraries? > > Exactly which way do you suggest to compare? Origin::IsSameOriginWith()? > > Note that the security origin to compare with is given as a string by the > extension. This is an existing design. > > Given that this code is taken from another extension API (see comment before > function), I suggest leaving this as is for now, file a bug for changing this in > both places (and probably consolidate the two functions). A simple change is of > course fine. I filed a bug and added to the TODO. https://codereview.chromium.org/2801853005/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.h (right): https://codereview.chromium.org/2801853005/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.h:107: content::RenderProcessHost* RphFromRequest( On 2017/04/10 21:34:25, Devlin wrote: > Nit: avoid uncommon abbreviation in names [1]. It's longer, but this should be > GetRenderProcessHostFromRequest(). > > [1] https://google.github.io/styleguide/cppguide.html#General_Naming_Rules Done. https://codereview.chromium.org/2801853005/diff/80001/chrome/common/extension... File chrome/common/extensions/api/webrtc_audio_private.idl (right): https://codereview.chromium.org/2801853005/diff/80001/chrome/common/extension... chrome/common/extensions/api/webrtc_audio_private.idl:85: static void setAudioExperiments(RequestInfo request, On 2017/04/11 06:49:39, Henrik Grunell wrote: > On 2017/04/10 21:34:25, Devlin wrote: > > Document the parameters this accepts. Also, probably add an optional callback > > unless there's a concrete reason not to. > > I asked the author to remove the callback that was originally there since there > is no need to signal success/failure (and it was actually never run in the > original patch). There's also discussion in another comment whether the > application would need to know when the operation has finished. If that's not > necessary either, I see no reason for a callback. Added documentation. https://codereview.chromium.org/2801853005/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_process_host_impl.h (right): https://codereview.chromium.org/2801853005/diff/80001/content/browser/rendere... content/browser/renderer_host/render_process_host_impl.h:574: base::Optional<bool> override_aec3_; On 2017/04/10 18:08:13, Henrik Grunell wrote: > Actually, I realized that this belongs in AecDumpMessageFilter on the render > side. The reason the AEC dump is handled in this class at consumer registration > is that a file needs to be opened here on the browser side. That's not the case > for the AEC3 override obviously, so it can be closer to where it's used. > > So, please move it there. (see other comment.) Done. https://codereview.chromium.org/2801853005/diff/80001/content/renderer/media/... File content/renderer/media/aec_dump_message_filter.cc (right): https://codereview.chromium.org/2801853005/diff/80001/content/renderer/media/... content/renderer/media/aec_dump_message_filter.cc:50: On 2017/04/10 18:08:14, Henrik Grunell wrote: > Possibly set AEC3 here on the new delegate instead of in RPHI. Done. https://codereview.chromium.org/2801853005/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/2801853005/diff/80001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:491: if (override_aec3_ == base::Optional<bool>(enable)) On 2017/04/10 17:08:40, Henrik Grunell wrote: > Nit: can be written > if (override_aec3_ == enable) Done. https://codereview.chromium.org/2801853005/diff/80001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:494: override_aec3_ = base::Optional<bool>(enable); On 2017/04/10 17:08:40, Henrik Grunell wrote: > Nit: override_aec3_ = enable; Done. https://codereview.chromium.org/2801853005/diff/80001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/2801853005/diff/80001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_unittest.cc:606: TEST_F(MediaStreamAudioProcessorTest, TestAec3Switch) { On 2017/04/10 17:08:40, Henrik Grunell wrote: > Great tests. > > Interesting that no test has a InitializeAudioProcessingModule() call. It would > be nice to have a test for regression verifying that the override has effect > when it's called. However given that this feature is temporary I think these > tests are enough. Acknowledged.
Fix the comment, then still lgtm. https://codereview.chromium.org/2801853005/diff/100001/content/renderer/media... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/2801853005/diff/100001/content/renderer/media... content/renderer/media/media_stream_audio_processor.cc:495: if (!has_echo_cancellation_) |has_echo_cancellation_| is not initialized in the ctor - fix.
https://codereview.chromium.org/2801853005/diff/100001/content/renderer/media... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/2801853005/diff/100001/content/renderer/media... content/renderer/media/media_stream_audio_processor.cc:495: if (!has_echo_cancellation_) On 2017/04/11 10:12:43, Henrik Grunell wrote: > |has_echo_cancellation_| is not initialized in the ctor - fix. Done. It was unconditionally initialized from the constructor through InitializeAudioProcessingModule, but now also in the initializer list.
histograms.xml lgtm assuming this lg to other reviewers
On 2017/04/11 14:54:38, Alexei Svitkine (very slow) wrote: > histograms.xml lgtm assuming this lg to other reviewers Thanks! Friendly ping for the following reviews. rdevlin.cronin (ongoing): chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.* chrome/common/extensions/api/webrtc_audio_private.idl jochen: (chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.*) (chrome/common/extensions/api/webrtc_audio_private.idl) content/browser/renderer_host/render_process_host_impl.* content/public/* mkwst: content/common/media/aec_dump_messages.h
lgtm % nits https://codereview.chromium.org/2801853005/diff/80001/chrome/common/extension... File chrome/common/extensions/api/webrtc_audio_private.idl (right): https://codereview.chromium.org/2801853005/diff/80001/chrome/common/extension... chrome/common/extensions/api/webrtc_audio_private.idl:85: static void setAudioExperiments(RequestInfo request, On 2017/04/11 06:49:39, Henrik Grunell wrote: > On 2017/04/10 21:34:25, Devlin wrote: > > Document the parameters this accepts. Also, probably add an optional callback > > unless there's a concrete reason not to. > > I asked the author to remove the callback that was originally there since there > is no need to signal success/failure (and it was actually never run in the > original patch). There's also discussion in another comment whether the > application would need to know when the operation has finished. If that's not > necessary either, I see no reason for a callback. The main reason is consistency with other APIs and because it's difficult to predict all use cases (it's entirely possible that there could be a reason to wait for the operation to complete). But, given it's a private API, we can hold off if you prefer. https://codereview.chromium.org/2801853005/diff/120001/chrome/common/extensio... File chrome/common/extensions/api/webrtc_audio_private.idl (right): https://codereview.chromium.org/2801853005/diff/120001/chrome/common/extensio... chrome/common/extensions/api/webrtc_audio_private.idl:87: // dictionary. For IDL files, comments should be formatted like so: // <high level description> // |<param-1>|: <param-1 description> ... // |<param-n>|: <param-n description> so for this, e.g.: // Sets the active experiments. // |request|: Information about the requesting process. // |securityOrigin|: The origin to restrict the settings to. // |audioExperiments|: The experiments to enable or disable.
Thanks! Fixed the comment in the new patch set. https://codereview.chromium.org/2801853005/diff/80001/chrome/common/extension... File chrome/common/extensions/api/webrtc_audio_private.idl (right): https://codereview.chromium.org/2801853005/diff/80001/chrome/common/extension... chrome/common/extensions/api/webrtc_audio_private.idl:85: static void setAudioExperiments(RequestInfo request, On 2017/04/11 20:49:37, Devlin wrote: > On 2017/04/11 06:49:39, Henrik Grunell wrote: > > On 2017/04/10 21:34:25, Devlin wrote: > > > Document the parameters this accepts. Also, probably add an optional > callback > > > unless there's a concrete reason not to. > > > > I asked the author to remove the callback that was originally there since > there > > is no need to signal success/failure (and it was actually never run in the > > original patch). There's also discussion in another comment whether the > > application would need to know when the operation has finished. If that's not > > necessary either, I see no reason for a callback. > > The main reason is consistency with other APIs and because it's difficult to > predict all use cases (it's entirely possible that there could be a reason to > wait for the operation to complete). But, given it's a private API, we can hold > off if you prefer. Time is not on our side now, so I'll prefer to hold off. https://codereview.chromium.org/2801853005/diff/120001/chrome/common/extensio... File chrome/common/extensions/api/webrtc_audio_private.idl (right): https://codereview.chromium.org/2801853005/diff/120001/chrome/common/extensio... chrome/common/extensions/api/webrtc_audio_private.idl:87: // dictionary. On 2017/04/11 20:49:37, Devlin wrote: > For IDL files, comments should be formatted like so: > // <high level description> > // |<param-1>|: <param-1 description> > ... > // |<param-n>|: <param-n description> > > so for this, e.g.: > > // Sets the active experiments. > // |request|: Information about the requesting process. > // |securityOrigin|: The origin to restrict the settings to. > // |audioExperiments|: The experiments to enable or disable. Done.
The CQ bit was checked by hlundin@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
IPC LGTM.
The CQ bit was checked by hlundin@chromium.org to run a CQ dry run
Dry run: 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 hlundin@chromium.org
The CQ bit was checked by hlundin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hlundin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from guidou@chromium.org, grunell@chromium.org, rdevlin.cronin@chromium.org, asvitkine@chromium.org, mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/2801853005/#ps180001 (title: "Fixing typo")
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": 180001, "attempt_start_ts": 1491986782939240,
"parent_rev": "f47783859bd6c4b4c363a1a4131c97c0abbaba98", "commit_rev":
"95829c0d06a5bcfb677bfd1d65799a43ddae4f46"}
Message was sent while issue was closed.
Description was changed from ========== Create a private API for controlling WebRTC's AEC3 This change opens up a new API to the Hangouts service extension. The purpose of the API is to let the application control which version of echo canceler will be used in WebRTC -- the old AEC2 or the new AEC3. The CL contains the API in Hangouts service extension, the corresponding API in webrtc_audio_private_api, and the required wiring to reach the MediaStreamAudioProcessor where WebRTC's AudioProcessing module lives. BUG=708475 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Create a private API for controlling WebRTC's AEC3 This change opens up a new API to the Hangouts service extension. The purpose of the API is to let the application control which version of echo canceler will be used in WebRTC -- the old AEC2 or the new AEC3. The CL contains the API in Hangouts service extension, the corresponding API in webrtc_audio_private_api, and the required wiring to reach the MediaStreamAudioProcessor where WebRTC's AudioProcessing module lives. BUG=708475 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2801853005 Cr-Commit-Position: refs/heads/master@{#463971} Committed: https://chromium.googlesource.com/chromium/src/+/95829c0d06a5bcfb677bfd1d6579... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/95829c0d06a5bcfb677bfd1d6579... |
