Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2876133002:
Field trial support to whenever possible turn off the AGC and HPF (Closed)
|
Created:
3 years, 7 months ago by peah-webrtc Modified:
3 years, 7 months ago CC:
webrtc-reviews_webrtc.org Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionField trial support to whenever possible turn off the AGC and HPF
When operating on mobile devices, where hardware support is available
for the AEC and NS functionality, it is desirable to be able to
operate without hardcoded behaviors for the WebRTC AGC and HPF.
This CL adds support to allow a field trial to turn these off
whenever that is possible.
BUG=webrtc:6220, webrtc:6183, webrtc:6181
Review-Url: https://codereview.webrtc.org/2876133002
Cr-Commit-Position: refs/heads/master@{#18226}
Committed: https://chromium.googlesource.com/external/webrtc/+/8a8ebd94b01def6a6182645976e063465c04446b
Patch Set 1 #Patch Set 2 : Added Obj-C field trial support #
Total comments: 10
Patch Set 3 : Changes in response to reviewer comments #
Messages
Total messages: 32 (14 generated)
Description was changed from ========== Created experiment for avoiding resampling BUG= ========== to ========== Adding field trial support to whenever possible turn off the AGC and HPF When operating on mobile devices, where hardware support is available for the AEC and NS functionality, it is desirable to be able to operate without hardcoded behaviors for the WebRTC AGC and HPF. This CL adds support to allow a field trial to turn these off whenever that is possible. BUG=webrtc:6220, webrtc:6183 ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== Adding field trial support to whenever possible turn off the AGC and HPF When operating on mobile devices, where hardware support is available for the AEC and NS functionality, it is desirable to be able to operate without hardcoded behaviors for the WebRTC AGC and HPF. This CL adds support to allow a field trial to turn these off whenever that is possible. BUG=webrtc:6220, webrtc:6183 ========== to ========== Field trial support to whenever possible turn off the AGC and HPF When operating on mobile devices, where hardware support is available for the AEC and NS functionality, it is desirable to be able to operate without hardcoded behaviors for the WebRTC AGC and HPF. This CL adds support to allow a field trial to turn these off whenever that is possible. BUG=webrtc:6220, webrtc:6183 ==========
peah@webrtc.org changed reviewers: + henrika@webrtc.org, solenberg@webrtc.org
Hi, Could you please take a look at this CL?
peah@webrtc.org changed reviewers: + magjed@webrtc.org
Added Obj-C support for the field_trial, and magjed@ as a reviewer.
https://codereview.webrtc.org/2876133002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2876133002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:372: // Set and adjust noise suppressor options. It is not clear to me what you change here. Is it more than only things based on the field-trial flag?
https://codereview.webrtc.org/2876133002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2876133002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:372: // Set and adjust noise suppressor options. On 2017/05/12 09:27:31, henrika_webrtc wrote: > It is not clear to me what you change here. Is it more than only things based on > the field-trial flag? The actual changes related to the field trial are present on lines 394-407. The rest of the changes are re-arranging the hardcoded effects for the Android and iOS platforms by the effect applied. That part is not necessary for this code, but as the changes in this CL adds further platform specific behavior I was concerned that just adding that to the former code would cause it to be more messy. One example is the AEC settings: Before the change: -Line 349 turns off the AEC for IOS -Line 355 hardcodes AECM to be used for Android. -Line 361 turns off the extended filter AEC for IOS and Android -Line 373 turns on the extended filter if delay agnostic AEC is used and the platform is not IOS. That is a lot of logic which is bundled with a lot of platform specific logic for setting the NS and AGC effects. As the new code on 394-406 affects all of the set AEC, NS, AGC settings, I thought it made sense to make the preceeding logic more clear. Please let me know if you think those changes make sense, otherwise I'd be happy to revert them.
lgtm https://codereview.webrtc.org/2876133002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2876133002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:394: #if defined(WEBRTC_IOS) || defined(WEBRTC_ANDROID) Please add some comments here about what you intend to test using this field trial. And add link to bug as well.
peah@webrtc.org changed reviewers: + perkj@webrtc.org, tkchin@webrtc.org - magjed@webrtc.org, solenberg@webrtc.org
-magjed@, solenberg@ +tkchin@, perkj@ tkchin@: Could you please take a look at the changes in webrtc/sdk/objc/Framework/*? perkj@: Could you please take a look at the changes in webrtc/media/engine/webrtcvoiceengine.cc?
perkj@google.com changed reviewers: + perkj@google.com
I will have to trust the other reviewers. I know very little about this code. https://codereview.webrtc.org/2876133002/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCFieldTrials.mm (right): https://codereview.webrtc.org/2876133002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCFieldTrials.mm:30: NSString *const kRTCFieldTrialMinimizeResamplingOnMobileKey = nit: And you fix so that this file is consequent when it comes to where * is used. https://codereview.webrtc.org/2876133002/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCFieldTrials.h (right): https://codereview.webrtc.org/2876133002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCFieldTrials.h:22: RTC_EXTERN NSString* const kRTCFieldTrialMinimizeResamplingOnMobileKey; nit: Can you fix so that this file is consequent ? Compare this line with line 29 and 21.
On 2017/05/17 07:05:40, Per K wrote: > I will have to trust the other reviewers. I know very little about this code. > > https://codereview.webrtc.org/2876133002/diff/40001/webrtc/sdk/objc/Framework... > File webrtc/sdk/objc/Framework/Classes/RTCFieldTrials.mm (right): > > https://codereview.webrtc.org/2876133002/diff/40001/webrtc/sdk/objc/Framework... > webrtc/sdk/objc/Framework/Classes/RTCFieldTrials.mm:30: NSString *const > kRTCFieldTrialMinimizeResamplingOnMobileKey = > nit: And you fix so that this file is consequent when it comes to where * is > used. > > https://codereview.webrtc.org/2876133002/diff/40001/webrtc/sdk/objc/Framework... > File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCFieldTrials.h (right): > > https://codereview.webrtc.org/2876133002/diff/40001/webrtc/sdk/objc/Framework... > webrtc/sdk/objc/Framework/Headers/WebRTC/RTCFieldTrials.h:22: RTC_EXTERN > NSString* const kRTCFieldTrialMinimizeResamplingOnMobileKey; > nit: Can you fix so that this file is consequent ? Compare this line with line > 29 and 21. I think henrika is the best reviewer for webrtcvoiceengine.cc. rs lgtm
On 2017/05/17 07:07:00, Per K wrote: > On 2017/05/17 07:05:40, Per K wrote: > > I will have to trust the other reviewers. I know very little about this code. > > > > > https://codereview.webrtc.org/2876133002/diff/40001/webrtc/sdk/objc/Framework... > > File webrtc/sdk/objc/Framework/Classes/RTCFieldTrials.mm (right): > > > > > https://codereview.webrtc.org/2876133002/diff/40001/webrtc/sdk/objc/Framework... > > webrtc/sdk/objc/Framework/Classes/RTCFieldTrials.mm:30: NSString *const > > kRTCFieldTrialMinimizeResamplingOnMobileKey = > > nit: And you fix so that this file is consequent when it comes to where * is > > used. > > > > > https://codereview.webrtc.org/2876133002/diff/40001/webrtc/sdk/objc/Framework... > > File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCFieldTrials.h (right): > > > > > https://codereview.webrtc.org/2876133002/diff/40001/webrtc/sdk/objc/Framework... > > webrtc/sdk/objc/Framework/Headers/WebRTC/RTCFieldTrials.h:22: RTC_EXTERN > > NSString* const kRTCFieldTrialMinimizeResamplingOnMobileKey; > > nit: Can you fix so that this file is consequent ? Compare this line with > line > > 29 and 21. > > I think henrika is the best reviewer for webrtcvoiceengine.cc. rs lgtm ... rs lgtm
My lgtm still stands
It might be wise to run some local tests in AppRTCMobile where you hard-code field-trial settings an verify that the effect is as expected. https://codereview.webrtc.org/2876133002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2876133002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:372: // Set and adjust noise suppressor options. The new code looks less bad ;-)
Description was changed from ========== Field trial support to whenever possible turn off the AGC and HPF When operating on mobile devices, where hardware support is available for the AEC and NS functionality, it is desirable to be able to operate without hardcoded behaviors for the WebRTC AGC and HPF. This CL adds support to allow a field trial to turn these off whenever that is possible. BUG=webrtc:6220, webrtc:6183 ========== to ========== Field trial support to whenever possible turn off the AGC and HPF When operating on mobile devices, where hardware support is available for the AEC and NS functionality, it is desirable to be able to operate without hardcoded behaviors for the WebRTC AGC and HPF. This CL adds support to allow a field trial to turn these off whenever that is possible. BUG=webrtc:6220, webrtc:6183, webrtc:6181 ==========
lgtm
On 2017/05/17 07:33:00, henrika_webrtc wrote: > It might be wise to run some local tests in AppRTCMobile where you hard-code > field-trial settings an verify that the effect is as expected. > > https://codereview.webrtc.org/2876133002/diff/40001/webrtc/media/engine/webrt... > File webrtc/media/engine/webrtcvoiceengine.cc (right): > > https://codereview.webrtc.org/2876133002/diff/40001/webrtc/media/engine/webrt... > webrtc/media/engine/webrtcvoiceengine.cc:372: // Set and adjust noise suppressor > options. > The new code looks less bad ;-) Yes, that is a good suggestion. I have landed a CL that adds that as an option in AppRTCMobile so that should be done.
Thanks for the reviews!!! https://codereview.webrtc.org/2876133002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2876133002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:372: // Set and adjust noise suppressor options. On 2017/05/17 07:32:59, henrika_webrtc wrote: > The new code looks less bad ;-) :-) That is a step in the right direction. https://codereview.webrtc.org/2876133002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:394: #if defined(WEBRTC_IOS) || defined(WEBRTC_ANDROID) On 2017/05/12 12:20:47, henrika_webrtc wrote: > Please add some comments here about what you intend to test using this field > trial. And add link to bug as well. Done. https://codereview.webrtc.org/2876133002/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/RTCFieldTrials.mm (right): https://codereview.webrtc.org/2876133002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/RTCFieldTrials.mm:30: NSString *const kRTCFieldTrialMinimizeResamplingOnMobileKey = On 2017/05/17 07:05:39, Per K wrote: > nit: And you fix so that this file is consequent when it comes to where * is > used. Done. https://codereview.webrtc.org/2876133002/diff/40001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCFieldTrials.h (right): https://codereview.webrtc.org/2876133002/diff/40001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Headers/WebRTC/RTCFieldTrials.h:22: RTC_EXTERN NSString* const kRTCFieldTrialMinimizeResamplingOnMobileKey; On 2017/05/17 07:05:39, Per K wrote: > nit: Can you fix so that this file is consequent ? Compare this line with line > 29 and 21. Absolutely! Thanks! Done.
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkchin@webrtc.org, perkj@webrtc.org, perkj@google.com, henrika@webrtc.org Link to the patchset: https://codereview.webrtc.org/2876133002/#ps60001 (title: "Changes in response to reviewer comments")
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: win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...)
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/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495492101318370,
"parent_rev": "ef37ca5fb3431864130de3c2fd0ff865f9eb47dd", "commit_rev":
"8a8ebd94b01def6a6182645976e063465c04446b"}
Message was sent while issue was closed.
Description was changed from ========== Field trial support to whenever possible turn off the AGC and HPF When operating on mobile devices, where hardware support is available for the AEC and NS functionality, it is desirable to be able to operate without hardcoded behaviors for the WebRTC AGC and HPF. This CL adds support to allow a field trial to turn these off whenever that is possible. BUG=webrtc:6220, webrtc:6183, webrtc:6181 ========== to ========== Field trial support to whenever possible turn off the AGC and HPF When operating on mobile devices, where hardware support is available for the AEC and NS functionality, it is desirable to be able to operate without hardcoded behaviors for the WebRTC AGC and HPF. This CL adds support to allow a field trial to turn these off whenever that is possible. BUG=webrtc:6220, webrtc:6183, webrtc:6181 Review-Url: https://codereview.webrtc.org/2876133002 Cr-Commit-Position: refs/heads/master@{#18226} Committed: https://chromium.googlesource.com/external/webrtc/+/8a8ebd94b01def6a618264597... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/8a8ebd94b01def6a618264597... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
