Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2095563002:
Adding activation logic of the new APM level control functionality using MediaConstraints (Closed)
|
Created:
4 years, 6 months ago by peah-webrtc Modified:
4 years, 5 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun Base URL:
https://chromium.googlesource.com/external/webrtc.git@ALC_RC2_CL Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionThis CL adds activation logic of the new APM level control
functionality and exposes the functionality using the
MediaConstraints.
The exposing of the feature through the MediaConstraints
was done similarly to what was done for the intelligibility
enhancer in the CL
https://codereview.webrtc.org/1952123003
This CL is dependent on the CL https://codereview.webrtc.org/2090583002/ which contains
the level control functionality.
NOTRY=true
BUG=webrtc:5920
Committed: https://crrev.com/a3333bfafb9eca157b5d1bb59365b720ca9d7c38
Cr-Commit-Position: refs/heads/master@{#13336}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Changed to a more proper log message #
Total comments: 2
Patch Set 3 : Changed logging behavior according to reviewer comments #
Total comments: 6
Patch Set 4 : Changes in response to reviewer comments #Patch Set 5 : Rebase #
Depends on Patchset: Messages
Total messages: 35 (17 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Adding activation logic of the new APM level control functionality BUG= ========== to ========== This CL adds activation logic of the new APM level control functionality and exposes the functionality using the MediaConstraints. The exposing of the feature through the MediaConstraints was done similarly to what was done for the intelligibility enhancer in the CL https://codereview.webrtc.org/1952123003 BUG=webrtc:5920 ==========
peah@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, tommi@webrtc.org
Description was changed from ========== This CL adds activation logic of the new APM level control functionality and exposes the functionality using the MediaConstraints. The exposing of the feature through the MediaConstraints was done similarly to what was done for the intelligibility enhancer in the CL https://codereview.webrtc.org/1952123003 BUG=webrtc:5920 ========== to ========== This CL adds activation logic of the new APM level control functionality and exposes the functionality using the MediaConstraints. The exposing of the feature through the MediaConstraints was done similarly to what was done for the intelligibility enhancer in the CL https://codereview.webrtc.org/1952123003 This CL is dependent on the CL https://codereview.webrtc.org/2090583002/ which contains the level control functionality. BUG=webrtc:5920 ==========
Hi, Here is the CL for the activation of the level control functionality. I did the changes similarly to what was done for the intelligibility enhancement submodule and similar to that, the level control submodule is to be controlled using the MediaConstraints as well. Best, Per
lgtm. Will there be a followup cl to enable this in Chrome? https://codereview.webrtc.org/2095563002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2095563002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:869: LOG(LS_INFO) << "Level Control is enabled? " << *level_control_; Should this be: "Level control enabled, value=" << *level_control_; (since level_control_ itself will always be set here, but *foo will get the value)
On 2016/06/27 07:19:41, tommi-webrtc wrote: > lgtm. Will there be a followup cl to enable this in Chrome? > > https://codereview.webrtc.org/2095563002/diff/60001/webrtc/media/engine/webrt... > File webrtc/media/engine/webrtcvoiceengine.cc (right): > > https://codereview.webrtc.org/2095563002/diff/60001/webrtc/media/engine/webrt... > webrtc/media/engine/webrtcvoiceengine.cc:869: LOG(LS_INFO) << "Level Control is > enabled? " << *level_control_; > Should this be: > "Level control enabled, value=" << *level_control_; > > (since level_control_ itself will always be set here, but *foo will get the > value) Hi, No, not at this point. Best, Per
https://codereview.webrtc.org/2095563002/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2095563002/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:869: LOG(LS_INFO) << "Level Control is enabled? " << *level_control_; On 2016/06/27 07:19:41, tommi-webrtc wrote: > Should this be: > "Level control enabled, value=" << *level_control_; > > (since level_control_ itself will always be set here, but *foo will get the > value) That makes sense! I wrote it in this way to be consistent with how it is in the log messages for the other APM features. In particular, I think it would be good to have a log message regardless of whether level_control has been set or not. I rewrote the log message in a different way that what was proposed. Could you please check whether that way makes sense and is preferrable to your proposal! Otherwise I'll change to what you propose.
https://codereview.webrtc.org/2095563002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2095563002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:870: << "enabled"; My preference is to keep the amount of code for informational logging, down. The code behind the two log statements dwarves the actual functionality. I'd go with one LOG line, and keep the code size down: LOG((LS_INFO) << "Level control: " << level_control_ ? *level_control_ : -1; if (level_control_) { config.Set<webrtc::LevelControl>(new webrtc::LevelControl(*level_control_)); }
https://codereview.webrtc.org/2095563002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2095563002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:870: << "enabled"; On 2016/06/27 08:34:33, tommi-webrtc wrote: > My preference is to keep the amount of code for informational logging, down. > The code behind the two log statements dwarves the actual functionality. I'd go > with one LOG line, and keep the code size down: > > LOG((LS_INFO) << "Level control: " << level_control_ ? *level_control_ : -1; > if (level_control_) { > config.Set<webrtc::LevelControl>(new webrtc::LevelControl(*level_control_)); > } That looks great! Done.
lgtm
LGTM with comments. https://codereview.webrtc.org/2095563002/diff/100001/webrtc/api/localaudiosou... File webrtc/api/localaudiosource.cc (right): https://codereview.webrtc.org/2095563002/diff/100001/webrtc/api/localaudiosou... webrtc/api/localaudiosource.cc:56: {MediaConstraintsInterface::kAudioMirroring, options->stereo_swapping}}; I presume clang-format gave you this, but in this case it makes sense to ignore that. https://codereview.webrtc.org/2095563002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2095563002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:695: if (*built_in_agc_avaliable) { I wouldn't mind adding a DCHECK(built_in_agc_avaliable) just to document that it cannot be empty at this point. https://codereview.webrtc.org/2095563002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:707: if (*built_in_agc_avaliable) { DCHECK here too.
https://codereview.webrtc.org/2095563002/diff/100001/webrtc/api/localaudiosou... File webrtc/api/localaudiosource.cc (right): https://codereview.webrtc.org/2095563002/diff/100001/webrtc/api/localaudiosou... webrtc/api/localaudiosource.cc:56: {MediaConstraintsInterface::kAudioMirroring, options->stereo_swapping}}; On 2016/06/27 11:44:27, hlundin-webrtc wrote: > I presume clang-format gave you this, but in this case it makes sense to ignore > that. Yes, that was clang-format, and I definitely agree that it was better before. I reverted that format change. Done. https://codereview.webrtc.org/2095563002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2095563002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:695: if (*built_in_agc_avaliable) { On 2016/06/27 11:44:27, hlundin-webrtc wrote: > I wouldn't mind adding a DCHECK(built_in_agc_avaliable) just to document that it > cannot be empty at this point. Makes sense. Added that! Done. https://codereview.webrtc.org/2095563002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:707: if (*built_in_agc_avaliable) { On 2016/06/27 11:44:27, hlundin-webrtc wrote: > DCHECK here too. Makes sense. Added that! Done.
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org, henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/2095563002/#ps140001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14656)
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14657)
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14660)
Description was changed from ========== This CL adds activation logic of the new APM level control functionality and exposes the functionality using the MediaConstraints. The exposing of the feature through the MediaConstraints was done similarly to what was done for the intelligibility enhancer in the CL https://codereview.webrtc.org/1952123003 This CL is dependent on the CL https://codereview.webrtc.org/2090583002/ which contains the level control functionality. BUG=webrtc:5920 ========== to ========== This CL adds activation logic of the new APM level control functionality and exposes the functionality using the MediaConstraints. The exposing of the feature through the MediaConstraints was done similarly to what was done for the intelligibility enhancer in the CL https://codereview.webrtc.org/1952123003 This CL is dependent on the CL https://codereview.webrtc.org/2090583002/ which contains the level control functionality. NOTRY=true BUG=webrtc:5920 ==========
The CQ bit was checked by peah@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== This CL adds activation logic of the new APM level control functionality and exposes the functionality using the MediaConstraints. The exposing of the feature through the MediaConstraints was done similarly to what was done for the intelligibility enhancer in the CL https://codereview.webrtc.org/1952123003 This CL is dependent on the CL https://codereview.webrtc.org/2090583002/ which contains the level control functionality. NOTRY=true BUG=webrtc:5920 ========== to ========== This CL adds activation logic of the new APM level control functionality and exposes the functionality using the MediaConstraints. The exposing of the feature through the MediaConstraints was done similarly to what was done for the intelligibility enhancer in the CL https://codereview.webrtc.org/1952123003 This CL is dependent on the CL https://codereview.webrtc.org/2090583002/ which contains the level control functionality. NOTRY=true BUG=webrtc:5920 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== This CL adds activation logic of the new APM level control functionality and exposes the functionality using the MediaConstraints. The exposing of the feature through the MediaConstraints was done similarly to what was done for the intelligibility enhancer in the CL https://codereview.webrtc.org/1952123003 This CL is dependent on the CL https://codereview.webrtc.org/2090583002/ which contains the level control functionality. NOTRY=true BUG=webrtc:5920 ========== to ========== This CL adds activation logic of the new APM level control functionality and exposes the functionality using the MediaConstraints. The exposing of the feature through the MediaConstraints was done similarly to what was done for the intelligibility enhancer in the CL https://codereview.webrtc.org/1952123003 This CL is dependent on the CL https://codereview.webrtc.org/2090583002/ which contains the level control functionality. NOTRY=true BUG=webrtc:5920 Committed: https://crrev.com/a3333bfafb9eca157b5d1bb59365b720ca9d7c38 Cr-Commit-Position: refs/heads/master@{#13336} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a3333bfafb9eca157b5d1bb59365b720ca9d7c38 Cr-Commit-Position: refs/heads/master@{#13336} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
