Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1952123003:
Surface the IntelligibilityEnhancer on MediaConstraints (Closed)
|
Created:
4 years, 7 months ago by aluebs-webrtc Modified:
4 years, 7 months ago CC:
webrtc-reviews_webrtc.org, kwiberg-webrtc, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, sdk-team_agora.io, minyue-webrtc, the sun, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionSurface the IntelligibilityEnhancer on MediaConstraints
R=henrika@webrtc.org, peah@webrtc.org, tommi@webrtc.org
Committed: https://crrev.com/c9b0c26e0c9c43074692a19ac18532062e5b589e
Cr-Commit-Position: refs/heads/master@{#12763}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Change to use MediaConstraints #
Total comments: 14
Patch Set 3 : Disable HWNS when IE is enabled #
Total comments: 8
Patch Set 4 : Fix NS enabling logic #
Total comments: 6
Patch Set 5 : Add component to debug proto #Patch Set 6 : Rebasing #Messages
Total messages: 42 (10 generated)
aluebs@webrtc.org changed reviewers: + henrika@webrtc.org, peah@webrtc.org, solenberg@webrtc.org
peah: webrtc/modules/audio_processing/* solenberg: webrtc/media/engine/webrtcvoiceengine.cc henrika: webrtc/modules/audio_device/* As discussed, here I am surfacing Intelligibility Enhancer to the native Android WebRtcAudioUtils java API. I know that I still need to update the tests, but I wanted to get your opinion on this architecture before I spent a significant time doing so.
https://codereview.webrtc.org/1952123003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1952123003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:413: if(capture_nonlocked_.intelligibility_enabled != Could you please explain more why it is desired to move the activation of IE from the constructor to SetExtraOptions? I was not part of the discussions mentioned so I don't know the background. In general I think SetExtraOptions is something we should deprecate ASAP and instead do all the configuration at creation-time so I'd definitely prefer to have the activation of IE at the APM creation rather than using SetExtraOptions.
https://codereview.webrtc.org/1952123003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1952123003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:413: if(capture_nonlocked_.intelligibility_enabled != On 2016/05/05 18:49:41, peah-webrtc wrote: > Could you please explain more why it is desired to move the activation of IE > from the constructor to SetExtraOptions? I was not part of the discussions > mentioned so I don't know the background. > > In general I think SetExtraOptions is something we should deprecate ASAP and > instead do all the configuration at creation-time so I'd definitely prefer to > have the activation of IE at the APM creation rather than using SetExtraOptions. It was not moved from the constructor. Just added the possibility to enable it through SetExtraOptions. And I agree we should eventually deprecate this method and do all configuration at creation-time, but unfortunately this is not how the VoiceEngine works today. Probably solenberg can comment on this. The behavior of the IE would be similar to the possibility to ask for a pointer to certain component (AEC for example) and call the Enable method on it dynamically.
https://codereview.webrtc.org/1952123003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1952123003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:413: if(capture_nonlocked_.intelligibility_enabled != On 2016/05/05 21:53:05, aluebs-webrtc wrote: > On 2016/05/05 18:49:41, peah-webrtc wrote: > > Could you please explain more why it is desired to move the activation of IE > > from the constructor to SetExtraOptions? I was not part of the discussions > > mentioned so I don't know the background. > > > > In general I think SetExtraOptions is something we should deprecate ASAP and > > instead do all the configuration at creation-time so I'd definitely prefer to > > have the activation of IE at the APM creation rather than using > SetExtraOptions. > > It was not moved from the constructor. Just added the possibility to enable it > through SetExtraOptions. And I agree we should eventually deprecate this method > and do all configuration at creation-time, but unfortunately this is not how the > VoiceEngine works today. Probably solenberg can comment on this. The behavior of > the IE would be similar to the possibility to ask for a pointer to certain > component (AEC for example) and call the Enable method on it dynamically. Yes, you are right, it is still activated in the constructor since the constructor calls SetExtraOptions. The ability to activate this outside of the constructor was my main concern though. I know that it is allowed to activate other components this way, via the enable methods and the SetExtraOptions method. But that I think is something we'll move away from as soon as we are able to and therefore I think we should try hard not to include the activation of anything else this way. Regarding that, could you please explain why it is not possible to activate the IE at construction time. Is the reason VoE?
https://codereview.webrtc.org/1952123003/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1952123003/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:846: const bool intelligibility_enhancer = adm()->IntelligibilityIsEnabled(); Is the idea to pass the option to enable IE via the ADM? Why not pass it through the regular AudioOptions? The ADM should be a wrapper for actual hardware resources. https://codereview.webrtc.org/1952123003/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java (right): https://codereview.webrtc.org/1952123003/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:197: && !WebRtcAudioUtils.useWebRtcBasedIntelligibilityEnhancer() IIUC WebRtcAudioEffects is meant to provide an interface to hardware effects, so it is not apparent to me that IE needs to be taken into account here when we don't appear to have hardware support for IE. henrika@, WDYT ? https://codereview.webrtc.org/1952123003/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioUtils.java (right): https://codereview.webrtc.org/1952123003/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioUtils.java:68: private static boolean useWebRtcBasedIntelligibilityEnhancer = false; So is there a non-webrtc based IE? https://codereview.webrtc.org/1952123003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1952123003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:413: if(capture_nonlocked_.intelligibility_enabled != On 2016/05/05 22:13:18, peah-webrtc wrote: > On 2016/05/05 21:53:05, aluebs-webrtc wrote: > > On 2016/05/05 18:49:41, peah-webrtc wrote: > > > Could you please explain more why it is desired to move the activation of IE > > > from the constructor to SetExtraOptions? I was not part of the discussions > > > mentioned so I don't know the background. > > > > > > In general I think SetExtraOptions is something we should deprecate ASAP and > > > instead do all the configuration at creation-time so I'd definitely prefer > to > > > have the activation of IE at the APM creation rather than using > > SetExtraOptions. > > > > It was not moved from the constructor. Just added the possibility to enable it > > through SetExtraOptions. And I agree we should eventually deprecate this > method > > and do all configuration at creation-time, but unfortunately this is not how > the > > VoiceEngine works today. Probably solenberg can comment on this. The behavior > of > > the IE would be similar to the possibility to ask for a pointer to certain > > component (AEC for example) and call the Enable method on it dynamically. > > Yes, you are right, it is still activated in the constructor since the > constructor calls SetExtraOptions. The ability to activate this outside of the > constructor was my main concern though. I know that it is allowed to activate > other components this way, via the enable methods and the SetExtraOptions > method. But that I think is something we'll move away from as soon as we are > able to and therefore I think we should try hard not to include the activation > of anything else this way. > > Regarding that, could you please explain why it is not possible to activate the > IE at construction time. Is the reason VoE? > > > > Different processing can be enabled/disabled depending on options selected/negotiated after the VoE has been constructed (the APM is constructed at same time by VoE). The VoE could potentially recreate the APM instead when options change and we wouldn't need this.
I don't feel confident reviewing as is since I miss the background information of the IE. Please create a simple design doc with some fundamentals first. The existing code is very sensitive and we don't want to break anything here. My main questions are: - What is the goal of using the IntelligibilityEnhancer? - Should it replace other parts or work in tandem? - How much complexity does it add on Android? - Are there any demos where we can see the effect? - Do you plan to add some sort of button to turn the IE on/off?
https://codereview.webrtc.org/1952123003/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1952123003/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:846: const bool intelligibility_enhancer = adm()->IntelligibilityIsEnabled(); On 2016/05/06 09:00:15, the sun wrote: > Is the idea to pass the option to enable IE via the ADM? Why not pass it through > the regular AudioOptions? > > The ADM should be a wrapper for actual hardware resources. Because I was pointed to the WebRtcAudioUtils API as to where this should be enabled I found the AudioDevice to be the natural path, but I agree that it is not intuitive. On the other hand I couldn't find a simple way to use the AudioOptions with that API. But that is why I am asking for your feedback early. I am happy to implement any other path you suggest. https://codereview.webrtc.org/1952123003/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java (right): https://codereview.webrtc.org/1952123003/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:197: && !WebRtcAudioUtils.useWebRtcBasedIntelligibilityEnhancer() On 2016/05/06 09:00:16, the sun wrote: > IIUC WebRtcAudioEffects is meant to provide an interface to hardware effects, so > it is not apparent to me that IE needs to be taken into account here when we > don't appear to have hardware support for IE. > > henrika@, WDYT ? What this line does is to disable hardware NS when the IE is enabled and use the software NS instead. This is necessary because it requires the software NS's background noise estimate, which would be completely suppressed if the hardware NS is enabled. https://codereview.webrtc.org/1952123003/diff/1/webrtc/modules/audio_device/a... File webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioUtils.java (right): https://codereview.webrtc.org/1952123003/diff/1/webrtc/modules/audio_device/a... webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioUtils.java:68: private static boolean useWebRtcBasedIntelligibilityEnhancer = false; On 2016/05/06 09:00:16, the sun wrote: > So is there a non-webrtc based IE? No, so I am happy to change the naming here if you think it is better to not mention WebRTC. https://codereview.webrtc.org/1952123003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1952123003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:413: if(capture_nonlocked_.intelligibility_enabled != On 2016/05/06 09:00:16, the sun wrote: > On 2016/05/05 22:13:18, peah-webrtc wrote: > > On 2016/05/05 21:53:05, aluebs-webrtc wrote: > > > On 2016/05/05 18:49:41, peah-webrtc wrote: > > > > Could you please explain more why it is desired to move the activation of > IE > > > > from the constructor to SetExtraOptions? I was not part of the discussions > > > > mentioned so I don't know the background. > > > > > > > > In general I think SetExtraOptions is something we should deprecate ASAP > and > > > > instead do all the configuration at creation-time so I'd definitely prefer > > to > > > > have the activation of IE at the APM creation rather than using > > > SetExtraOptions. > > > > > > It was not moved from the constructor. Just added the possibility to enable > it > > > through SetExtraOptions. And I agree we should eventually deprecate this > > method > > > and do all configuration at creation-time, but unfortunately this is not how > > the > > > VoiceEngine works today. Probably solenberg can comment on this. The > behavior > > of > > > the IE would be similar to the possibility to ask for a pointer to certain > > > component (AEC for example) and call the Enable method on it dynamically. > > > > Yes, you are right, it is still activated in the constructor since the > > constructor calls SetExtraOptions. The ability to activate this outside of the > > constructor was my main concern though. I know that it is allowed to activate > > other components this way, via the enable methods and the SetExtraOptions > > method. But that I think is something we'll move away from as soon as we are > > able to and therefore I think we should try hard not to include the activation > > of anything else this way. > > > > Regarding that, could you please explain why it is not possible to activate > the > > IE at construction time. Is the reason VoE? > > > > > > > > > > Different processing can be enabled/disabled depending on options > selected/negotiated after the VoE has been constructed (the APM is constructed > at same time by VoE). The VoE could potentially recreate the APM instead when > options change and we wouldn't need this. It would be awesome to move to recreating the APM when the options change, but I don't think this CL is the right place to do it. And I think that having the possibility of enabling IE in SetExtraOptions (as other components already are) doesn't make it harder to move away from it.
On 2016/05/06 09:16:46, henrika_webrtc wrote: > I don't feel confident reviewing as is since I miss the background information > of the IE. Please create a simple design doc with some fundamentals first. The > existing code is very sensitive and we don't want to break anything here. > > My main questions are: > > - What is the goal of using the IntelligibilityEnhancer? > - Should it replace other parts or work in tandem? > - How much complexity does it add on Android? > - Are there any demos where we can see the effect? > - Do you plan to add some sort of button to turn the IE on/off? I shared the design doc that was in place since the beginning of this project with you. There you can find the answers to all your questions and it links to some nice demos you can listen to. This CL doesn't intend to add any UI, just a native API that could be potentially accessed by Android apps using the library.
solenberg@google.com changed reviewers: + henrik.lundin@webrtc.org
Description was changed from ========== Surface the IntelligibilityEnhancer on the WebRtcAudioUtils API ========== to ========== Surface the IntelligibilityEnhancer on MediaConstraints ==========
aluebs@webrtc.org changed reviewers: + tommi@webrtc.org
tommi: webrtc/api/* Changed to use MediaConstraints as discussed. Unfortunately I don't find a simple way of disabling HWNS when IE is enabled, so the burden is put on the user to disable it manually.
Reviewed on my phone, so I might have missed some things https://codereview.webrtc.org/1952123003/diff/20001/webrtc/api/mediaconstrain... File webrtc/api/mediaconstraintsinterface.cc (right): https://codereview.webrtc.org/1952123003/diff/20001/webrtc/api/mediaconstrain... webrtc/api/mediaconstraintsinterface.cc:50: "IntelligibilityEnhancer"; This doesn't follow the convention for audio constraints. Btw, does this need to be available in Chrome? If so, talk to hta@ about how to do that. https://codereview.webrtc.org/1952123003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1952123003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:417: InitializeIntelligibility(); Don't you need to check first if the value is true? Also, is the above copy/pasted code possibly buggy? Is the initialization thread safe? Is it global or will it only affect the current instance of AudioProcessingImpl?
Looks good but I think you can add support to turn off the HW NS as well. See comments.
https://codereview.webrtc.org/1952123003/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1952123003/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:847: if (options.intelligibility_enhancer) { You have access to the ADM here. Why can't you use it to disable the HW NS. Like so: adm()->EnableBuiltInNS()
https://codereview.webrtc.org/1952123003/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1952123003/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:854: new webrtc::Intelligibility(*intelligibility_enhancer_)); nit: the call to adm()->EnableBuiltInNS(false) should be here. Also, note that on Android, the hardware effects settings are only applied when adm()->InitRecord() is called, which will happen as soon as we know there is a stream to be sent. I believe you should be fine though, since constraints are passed when creating and audio source and applied when a send stream is added, but you may want to keep an eye on the hardware NS and verify it isn't left enabled for some reason.
https://codereview.webrtc.org/1952123003/diff/20001/webrtc/api/mediaconstrain... File webrtc/api/mediaconstraintsinterface.cc (right): https://codereview.webrtc.org/1952123003/diff/20001/webrtc/api/mediaconstrain... webrtc/api/mediaconstraintsinterface.cc:50: "IntelligibilityEnhancer"; On 2016/05/11 06:06:48, tommi-webrtc wrote: > This doesn't follow the convention for audio constraints. > Btw, does this need to be available in Chrome? If so, talk to hta@ about how to > do that. By convention do you mean the "goog" prefix? Because I thought we were moving away from that. At this point we are only targeting native and I am familiar adding media constraints to Chrome, but I will contact hta if/when we decide to do so. Thanks! :) https://codereview.webrtc.org/1952123003/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1952123003/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:847: if (options.intelligibility_enhancer) { On 2016/05/11 08:50:36, henrika_webrtc wrote: > You have access to the ADM here. Why can't you use it to disable the HW NS. > Like so: adm()->EnableBuiltInNS() Great point! I didn't realize there was a 2 way communication of these flags with the Java layer. Done. https://codereview.webrtc.org/1952123003/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:854: new webrtc::Intelligibility(*intelligibility_enhancer_)); On 2016/05/11 11:23:41, the sun wrote: > nit: the call to > adm()->EnableBuiltInNS(false) > should be here. > > Also, note that on Android, the hardware effects settings are only applied when > adm()->InitRecord() is called, which will happen as soon as we know there is a > stream to be sent. I believe you should be fine though, since constraints are > passed when creating and audio source and applied when a send stream is added, > but you may want to keep an eye on the hardware NS and verify it isn't left > enabled for some reason. Actually I think the best thing is to do it above together with the NS, so that we don't disable the SWNS when IE is enabled. https://codereview.webrtc.org/1952123003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1952123003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:417: InitializeIntelligibility(); On 2016/05/11 06:06:48, tommi-webrtc wrote: > Don't you need to check first if the value is true? > > Also, is the above copy/pasted code possibly buggy? > > Is the initialization thread safe? > > Is it global or will it only affect the current instance of AudioProcessingImpl? > The check if the value is true is done inside InitializeIntelligibility(). I am familiar with this code and I am quite sure it is not buggy, but please let me know if you see something wrong with it. The initialization is thread safe since SetExtraOptions takes both cs_render and cs_capture. This only affects the current instance of AudioProcessingImpl, like all the settings in it.
LGTM https://codereview.webrtc.org/1952123003/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1952123003/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:750: if (options.intelligibility_enhancer) { Please add logs here as well and verify that you never accidentally turn off the HW NS even if IE is off. Will also allow others to see if the IE is on or off in logs.
https://codereview.webrtc.org/1952123003/diff/20001/webrtc/api/mediaconstrain... File webrtc/api/mediaconstraintsinterface.cc (right): https://codereview.webrtc.org/1952123003/diff/20001/webrtc/api/mediaconstrain... webrtc/api/mediaconstraintsinterface.cc:50: "IntelligibilityEnhancer"; This should be intelligibilityEnhancer https://codereview.webrtc.org/1952123003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (left): https://codereview.webrtc.org/1952123003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1218: if (constants_.intelligibility_enabled) { This should be a dcheck and the function should do as the functions below do. https://codereview.webrtc.org/1952123003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1952123003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:417: InitializeIntelligibility(); On 2016/05/11 21:22:05, aluebs-webrtc wrote: > On 2016/05/11 06:06:48, tommi-webrtc wrote: > > Don't you need to check first if the value is true? > > > > Also, is the above copy/pasted code possibly buggy? > > > > Is the initialization thread safe? > > > > Is it global or will it only affect the current instance of > AudioProcessingImpl? > > > > The check if the value is true is done inside InitializeIntelligibility(). > > I am familiar with this code and I am quite sure it is not buggy, but please let > me know if you see something wrong with it. > > The initialization is thread safe since SetExtraOptions takes both cs_render and > cs_capture. > > This only affects the current instance of AudioProcessingImpl, like all the > settings in it. OK. It is confusing that if the value is false, we call Initialize and _then_ check the value. I would rather expect that there be only a dcheck inside Initialize that confirms that the feature is enabled and force the caller to only call the method when it should be called.
On 2016/05/12 07:06:38, tommi-webrtc wrote: > https://codereview.webrtc.org/1952123003/diff/20001/webrtc/api/mediaconstrain... > File webrtc/api/mediaconstraintsinterface.cc (right): > > https://codereview.webrtc.org/1952123003/diff/20001/webrtc/api/mediaconstrain... > webrtc/api/mediaconstraintsinterface.cc:50: "IntelligibilityEnhancer"; > This should be intelligibilityEnhancer > > https://codereview.webrtc.org/1952123003/diff/20001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/audio_processing_impl.cc (left): > > https://codereview.webrtc.org/1952123003/diff/20001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_impl.cc:1218: if > (constants_.intelligibility_enabled) { > This should be a dcheck and the function should do as the functions below do. > > https://codereview.webrtc.org/1952123003/diff/20001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/audio_processing_impl.cc (right): > > https://codereview.webrtc.org/1952123003/diff/20001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_impl.cc:417: > InitializeIntelligibility(); > On 2016/05/11 21:22:05, aluebs-webrtc wrote: > > On 2016/05/11 06:06:48, tommi-webrtc wrote: > > > Don't you need to check first if the value is true? > > > > > > Also, is the above copy/pasted code possibly buggy? > > > > > > Is the initialization thread safe? > > > > > > Is it global or will it only affect the current instance of > > AudioProcessingImpl? > > > > > > > The check if the value is true is done inside InitializeIntelligibility(). > > > > I am familiar with this code and I am quite sure it is not buggy, but please > let > > me know if you see something wrong with it. > > > > The initialization is thread safe since SetExtraOptions takes both cs_render > and > > cs_capture. > > > > This only affects the current instance of AudioProcessingImpl, like all the > > settings in it. > > OK. It is confusing that if the value is false, we call Initialize and _then_ > check the value. I would rather expect that there be only a dcheck inside > Initialize that confirms that the feature is enabled and force the caller to > only call the method when it should be called. Lgtm with the above addressed.
https://codereview.webrtc.org/1952123003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1952123003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:413: if(capture_nonlocked_.intelligibility_enabled != On 2016/05/06 16:27:10, aluebs-webrtc wrote: > On 2016/05/06 09:00:16, the sun wrote: > > On 2016/05/05 22:13:18, peah-webrtc wrote: > > > On 2016/05/05 21:53:05, aluebs-webrtc wrote: > > > > On 2016/05/05 18:49:41, peah-webrtc wrote: > > > > > Could you please explain more why it is desired to move the activation > of > > IE > > > > > from the constructor to SetExtraOptions? I was not part of the > discussions > > > > > mentioned so I don't know the background. > > > > > > > > > > In general I think SetExtraOptions is something we should deprecate ASAP > > and > > > > > instead do all the configuration at creation-time so I'd definitely > prefer > > > to > > > > > have the activation of IE at the APM creation rather than using > > > > SetExtraOptions. > > > > > > > > It was not moved from the constructor. Just added the possibility to > enable > > it > > > > through SetExtraOptions. And I agree we should eventually deprecate this > > > method > > > > and do all configuration at creation-time, but unfortunately this is not > how > > > the > > > > VoiceEngine works today. Probably solenberg can comment on this. The > > behavior > > > of > > > > the IE would be similar to the possibility to ask for a pointer to certain > > > > component (AEC for example) and call the Enable method on it dynamically. > > > > > > Yes, you are right, it is still activated in the constructor since the > > > constructor calls SetExtraOptions. The ability to activate this outside of > the > > > constructor was my main concern though. I know that it is allowed to > activate > > > other components this way, via the enable methods and the SetExtraOptions > > > method. But that I think is something we'll move away from as soon as we are > > > able to and therefore I think we should try hard not to include the > activation > > > of anything else this way. > > > > > > Regarding that, could you please explain why it is not possible to activate > > the > > > IE at construction time. Is the reason VoE? > > > > > > > > > > > > > > > > Different processing can be enabled/disabled depending on options > > selected/negotiated after the VoE has been constructed (the APM is constructed > > at same time by VoE). The VoE could potentially recreate the APM instead when > > options change and we wouldn't need this. > > It would be awesome to move to recreating the APM when the options change, but I > don't think this CL is the right place to do it. And I think that having the > possibility of enabling IE in SetExtraOptions (as other components already are) > doesn't make it harder to move away from it. What I meant was that we should be able to activate the IE at creation-time, similarly to how the beamformer is activated, and similarly to how IE was created before. Does this CL really require the activation of IE to be done during runtime instead of at creation? To clarify, with this I don't mean that the APM should be recreated every time an option change (that is for sure something for another CL). What I mean is that I'm concerned that this CL changes the way that IE is activated/created from being done at creation-time to being done at runtime. This is something that we should go at lengths not do do.
https://codereview.webrtc.org/1952123003/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1952123003/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:752: if (intelligibility_enhancer_ && *intelligibility_enhancer_) { why do you need the first half of this condition? Did you mean for this to be: if (options.intelligibility_enhancer) { intelligibility_enhancer_ = options.intelligibility_enhancer; } if (intelligibility_enhancer_ && *intelligibility_enhancer_) { options.noise_suppression = intelligibility_enhancer_; } ? https://codereview.webrtc.org/1952123003/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:760: bool builtin_ns = You have one variable named "built_in_ns" and one named "builtin_ns" in the same scope. Perhaps remove the first one by putting the function call directly in the conditional? https://codereview.webrtc.org/1952123003/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:762: !(intelligibility_enhancer_ && * intelligibility_enhancer_); nit: no space after *
https://codereview.webrtc.org/1952123003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1952123003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:413: if(capture_nonlocked_.intelligibility_enabled != On 2016/05/12 08:05:12, peah-webrtc wrote: > On 2016/05/06 16:27:10, aluebs-webrtc wrote: > > On 2016/05/06 09:00:16, the sun wrote: > > > On 2016/05/05 22:13:18, peah-webrtc wrote: > > > > On 2016/05/05 21:53:05, aluebs-webrtc wrote: > > > > > On 2016/05/05 18:49:41, peah-webrtc wrote: > > > > > > Could you please explain more why it is desired to move the activation > > of > > > IE > > > > > > from the constructor to SetExtraOptions? I was not part of the > > discussions > > > > > > mentioned so I don't know the background. > > > > > > > > > > > > In general I think SetExtraOptions is something we should deprecate > ASAP > > > and > > > > > > instead do all the configuration at creation-time so I'd definitely > > prefer > > > > to > > > > > > have the activation of IE at the APM creation rather than using > > > > > SetExtraOptions. > > > > > > > > > > It was not moved from the constructor. Just added the possibility to > > enable > > > it > > > > > through SetExtraOptions. And I agree we should eventually deprecate this > > > > method > > > > > and do all configuration at creation-time, but unfortunately this is not > > how > > > > the > > > > > VoiceEngine works today. Probably solenberg can comment on this. The > > > behavior > > > > of > > > > > the IE would be similar to the possibility to ask for a pointer to > certain > > > > > component (AEC for example) and call the Enable method on it > dynamically. > > > > > > > > Yes, you are right, it is still activated in the constructor since the > > > > constructor calls SetExtraOptions. The ability to activate this outside of > > the > > > > constructor was my main concern though. I know that it is allowed to > > activate > > > > other components this way, via the enable methods and the SetExtraOptions > > > > method. But that I think is something we'll move away from as soon as we > are > > > > able to and therefore I think we should try hard not to include the > > activation > > > > of anything else this way. > > > > > > > > Regarding that, could you please explain why it is not possible to > activate > > > the > > > > IE at construction time. Is the reason VoE? > > > > > > > > > > > > > > > > > > > > > > Different processing can be enabled/disabled depending on options > > > selected/negotiated after the VoE has been constructed (the APM is > constructed > > > at same time by VoE). The VoE could potentially recreate the APM instead > when > > > options change and we wouldn't need this. > > > > It would be awesome to move to recreating the APM when the options change, but > I > > don't think this CL is the right place to do it. And I think that having the > > possibility of enabling IE in SetExtraOptions (as other components already > are) > > doesn't make it harder to move away from it. > > What I meant was that we should be able to activate the IE at creation-time, > similarly to how the beamformer is activated, and similarly to how IE was > created before. > > Does this CL really require the activation of IE to be done during runtime > instead of at creation? > > To clarify, with this I don't mean that the APM should be recreated every time > an option change (that is for sure something for another CL). What I mean is > that I'm concerned that this CL changes the way that IE is activated/created > from being done at creation-time to being done at runtime. This is something > that we should go at lengths not do do. The Beamformer can be activated only at creation time, because it is only used in Chrome, which creates their own APM (doesn't use the VoiceEngine's one) and sets all configs at creation time. The IE could only be activated at creation time, because it was not activated anywhere. But because the main use case for the IE is mobile we require it to be activated from the VoiceEngine, which today activates components (calling Enable methods of other components for example) during run time. Once the VoiceEngine migrates to re-creating the APM with new options I totally agree we should drop SetExtraOptions and the possibility of accessing the components directly for that matter. https://codereview.webrtc.org/1952123003/diff/20001/webrtc/api/mediaconstrain... File webrtc/api/mediaconstraintsinterface.cc (right): https://codereview.webrtc.org/1952123003/diff/20001/webrtc/api/mediaconstrain... webrtc/api/mediaconstraintsinterface.cc:50: "IntelligibilityEnhancer"; On 2016/05/12 07:06:37, tommi-webrtc wrote: > This should be intelligibilityEnhancer You are totally right. Changed. https://codereview.webrtc.org/1952123003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (left): https://codereview.webrtc.org/1952123003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1218: if (constants_.intelligibility_enabled) { On 2016/05/12 07:06:38, tommi-webrtc wrote: > This should be a dcheck and the function should do as the functions below do. Please see explanation above. https://codereview.webrtc.org/1952123003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1952123003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:417: InitializeIntelligibility(); On 2016/05/12 07:06:38, tommi-webrtc wrote: > On 2016/05/11 21:22:05, aluebs-webrtc wrote: > > On 2016/05/11 06:06:48, tommi-webrtc wrote: > > > Don't you need to check first if the value is true? > > > > > > Also, is the above copy/pasted code possibly buggy? > > > > > > Is the initialization thread safe? > > > > > > Is it global or will it only affect the current instance of > > AudioProcessingImpl? > > > > > > > The check if the value is true is done inside InitializeIntelligibility(). > > > > I am familiar with this code and I am quite sure it is not buggy, but please > let > > me know if you see something wrong with it. > > > > The initialization is thread safe since SetExtraOptions takes both cs_render > and > > cs_capture. > > > > This only affects the current instance of AudioProcessingImpl, like all the > > settings in it. > > OK. It is confusing that if the value is false, we call Initialize and _then_ > check the value. I would rather expect that there be only a dcheck inside > Initialize that confirms that the feature is enabled and force the caller to > only call the method when it should be called. The older components (that use to inherit from ProcessingComponent) were forced to have a Initialize method that would re-allocate all the internal memory and re-initialize the internal state. This was bug prone, since it had to maintain this method on top of the constructor. In the newer components we decided to just re-create the whole component itself since it is simpler and more scalable. Older components are always created at construction time, whether they are enabled or not. And when Initializing them they re-allocate their internal memory whether they are enabled or not. On the other hand, newer components are not necessarily created at construction time, but only if/when enabled. But all Initialize*() methods in APM (there are 11, one for each component) are called without checking if the component is enabled. I think there is definitively room for improvement, but I think we should do it in a consistent way and this is not the right CL for it. WDYT? https://codereview.webrtc.org/1952123003/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1952123003/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:750: if (options.intelligibility_enhancer) { On 2016/05/12 06:58:32, henrika_webrtc wrote: > Please add logs here as well and verify that you never accidentally turn off the > HW NS even if IE is off. > Will also allow others to see if the IE is on or off in logs. Good point! The log of "IE enabled" is below where it actually sets the config, but now I added an additional log to make the dependency clear. https://codereview.webrtc.org/1952123003/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:752: if (intelligibility_enhancer_ && *intelligibility_enhancer_) { On 2016/05/12 11:50:01, the sun (OOO until May 30th) wrote: > why do you need the first half of this condition? Did you mean for this to be: > > if (options.intelligibility_enhancer) { > intelligibility_enhancer_ = options.intelligibility_enhancer; > } > if (intelligibility_enhancer_ && *intelligibility_enhancer_) { > options.noise_suppression = intelligibility_enhancer_; > } > > ? That is exactly what I meant it to be, thank you :) https://codereview.webrtc.org/1952123003/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:760: bool builtin_ns = On 2016/05/12 11:50:01, the sun (OOO until May 30th) wrote: > You have one variable named "built_in_ns" and one named "builtin_ns" in the same > scope. Perhaps remove the first one by putting the function call directly in the > conditional? Great suggestion! Done. https://codereview.webrtc.org/1952123003/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:762: !(intelligibility_enhancer_ && * intelligibility_enhancer_); On 2016/05/12 11:50:01, the sun (OOO until May 30th) wrote: > nit: no space after * Done.
https://codereview.webrtc.org/1952123003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1952123003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:413: if(capture_nonlocked_.intelligibility_enabled != On 2016/05/13 02:04:37, aluebs-webrtc wrote: > On 2016/05/12 08:05:12, peah-webrtc wrote: > > On 2016/05/06 16:27:10, aluebs-webrtc wrote: > > > On 2016/05/06 09:00:16, the sun wrote: > > > > On 2016/05/05 22:13:18, peah-webrtc wrote: > > > > > On 2016/05/05 21:53:05, aluebs-webrtc wrote: > > > > > > On 2016/05/05 18:49:41, peah-webrtc wrote: > > > > > > > Could you please explain more why it is desired to move the > activation > > > of > > > > IE > > > > > > > from the constructor to SetExtraOptions? I was not part of the > > > discussions > > > > > > > mentioned so I don't know the background. > > > > > > > > > > > > > > In general I think SetExtraOptions is something we should deprecate > > ASAP > > > > and > > > > > > > instead do all the configuration at creation-time so I'd definitely > > > prefer > > > > > to > > > > > > > have the activation of IE at the APM creation rather than using > > > > > > SetExtraOptions. > > > > > > > > > > > > It was not moved from the constructor. Just added the possibility to > > > enable > > > > it > > > > > > through SetExtraOptions. And I agree we should eventually deprecate > this > > > > > method > > > > > > and do all configuration at creation-time, but unfortunately this is > not > > > how > > > > > the > > > > > > VoiceEngine works today. Probably solenberg can comment on this. The > > > > behavior > > > > > of > > > > > > the IE would be similar to the possibility to ask for a pointer to > > certain > > > > > > component (AEC for example) and call the Enable method on it > > dynamically. > > > > > > > > > > Yes, you are right, it is still activated in the constructor since the > > > > > constructor calls SetExtraOptions. The ability to activate this outside > of > > > the > > > > > constructor was my main concern though. I know that it is allowed to > > > activate > > > > > other components this way, via the enable methods and the > SetExtraOptions > > > > > method. But that I think is something we'll move away from as soon as we > > are > > > > > able to and therefore I think we should try hard not to include the > > > activation > > > > > of anything else this way. > > > > > > > > > > Regarding that, could you please explain why it is not possible to > > activate > > > > the > > > > > IE at construction time. Is the reason VoE? > > > > > > > > > > > > > > > > > > > > > > > > > > > > Different processing can be enabled/disabled depending on options > > > > selected/negotiated after the VoE has been constructed (the APM is > > constructed > > > > at same time by VoE). The VoE could potentially recreate the APM instead > > when > > > > options change and we wouldn't need this. > > > > > > It would be awesome to move to recreating the APM when the options change, > but > > I > > > don't think this CL is the right place to do it. And I think that having the > > > possibility of enabling IE in SetExtraOptions (as other components already > > are) > > > doesn't make it harder to move away from it. > > > > What I meant was that we should be able to activate the IE at creation-time, > > similarly to how the beamformer is activated, and similarly to how IE was > > created before. > > > > Does this CL really require the activation of IE to be done during runtime > > instead of at creation? > > > > To clarify, with this I don't mean that the APM should be recreated every time > > an option change (that is for sure something for another CL). What I mean is > > that I'm concerned that this CL changes the way that IE is activated/created > > from being done at creation-time to being done at runtime. This is something > > that we should go at lengths not do do. > > The Beamformer can be activated only at creation time, because it is only used > in Chrome, which creates their own APM (doesn't use the VoiceEngine's one) and > sets all configs at creation time. The IE could only be activated at creation > time, because it was not activated anywhere. But because the main use case for > the IE is mobile we require it to be activated from the VoiceEngine, which today > activates components (calling Enable methods of other components for example) > during run time. Once the VoiceEngine migrates to re-creating the APM with new > options I totally agree we should drop SetExtraOptions and the possibility of > accessing the components directly for that matter. Thanks, then I see the problem. https://codereview.webrtc.org/1952123003/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1952123003/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:759: if (adm()->BuiltInNSIsAvailable()) { I'm concerned about the AEC functionality as the IE has not been tested yet with the software AEC. How is this envisioned to be used at the moment? Is it possible to restrict the usage of the IE to platforms which have HW AEC support? I think that there could be a point in turning off the IE functionality if there is no HW AEC available, similarly to this CL turns off the HW NS if IE is active. But it depends fully on how this intended to be used. If it is used in a way without explicitly checking whether the HW AEC is available on the device then there must be code for turning off the IE functionality if a HW AEC is not available (at least until the IE has been properly tested with the AEC).
> https://codereview.webrtc.org/1952123003/diff/20001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_impl.cc:417: > InitializeIntelligibility(); > On 2016/05/12 07:06:38, tommi-webrtc wrote: > > On 2016/05/11 21:22:05, aluebs-webrtc wrote: > > > On 2016/05/11 06:06:48, tommi-webrtc wrote: > > > > Don't you need to check first if the value is true? > > > > > > > > Also, is the above copy/pasted code possibly buggy? > > > > > > > > Is the initialization thread safe? > > > > > > > > Is it global or will it only affect the current instance of > > > AudioProcessingImpl? > > > > > > > > > > The check if the value is true is done inside InitializeIntelligibility(). > > > > > > I am familiar with this code and I am quite sure it is not buggy, but please > > let > > > me know if you see something wrong with it. > > > > > > The initialization is thread safe since SetExtraOptions takes both cs_render > > and > > > cs_capture. > > > > > > This only affects the current instance of AudioProcessingImpl, like all the > > > settings in it. > > > > OK. It is confusing that if the value is false, we call Initialize and _then_ > > check the value. I would rather expect that there be only a dcheck inside > > Initialize that confirms that the feature is enabled and force the caller to > > only call the method when it should be called. > > The older components (that use to inherit from ProcessingComponent) were forced > to have a Initialize method that would re-allocate all the internal memory and > re-initialize the internal state. This was bug prone, since it had to maintain > this method on top of the constructor. In the newer components we decided to > just re-create the whole component itself since it is simpler and more scalable. > Older components are always created at construction time, whether they are > enabled or not. And when Initializing them they re-allocate their internal > memory whether they are enabled or not. On the other hand, newer components are > not necessarily created at construction time, but only if/when enabled. But all > Initialize*() methods in APM (there are 11, one for each component) are called > without checking if the component is enabled. I think there is definitively room > for improvement, but I think we should do it in a consistent way and this is not > the right CL for it. WDYT? > OK, I'm fine with taking care of it later. My preference is against "maybe" methods where you can't know from the outside if they actually did what you wanted them to do or not. lgtm once other reviewers are happy.
https://codereview.webrtc.org/1952123003/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1952123003/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:759: if (adm()->BuiltInNSIsAvailable()) { On 2016/05/13 12:39:49, peah-webrtc wrote: > I'm concerned about the AEC functionality as the IE has not been tested yet with > the software AEC. > > How is this envisioned to be used at the moment? Is it possible to restrict the > usage of the IE to platforms which have HW AEC support? > > I think that there could be a point in turning off the IE functionality if there > is no HW AEC available, similarly to this CL turns off the HW NS if IE is > active. > > But it depends fully on how this intended to be used. If it is used in a way > without explicitly checking whether the HW AEC is available on the device then > there must be code for turning off the IE functionality if a HW AEC is not > available (at least until the IE has been properly tested with the AEC). This CL doesn't enable IE, it just surfaces it to the MediaConstraints. Later we will use this API to test it and have an A/B experiment limited to the team before we decide to move forward with it (or not). I would argue that the only way we can gather real-life data about the interaction between the IE and SW AEC is by having that case be part of the testing and experiment. I am not saying we shouldn't also test it offline on more recordings, I just think that those recordings will never cover all possible scenarios and at the end of the day the only way of being sure how it affects users is to test it on real usage.
Regarding A/B tests of IE. It will most likely be super tricky to ensure that the AEC performance is not affected during such tests since the effect will not be experienced on the same side as the evaluations is done. The user who is focused on experiencing pros and cons with the IE will be the one who can cause worse echo performance for the other side. Not sure how that will be handled in an A/B test.
https://codereview.webrtc.org/1952123003/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1952123003/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:759: if (adm()->BuiltInNSIsAvailable()) { On 2016/05/13 16:24:28, aluebs-webrtc wrote: > On 2016/05/13 12:39:49, peah-webrtc wrote: > > I'm concerned about the AEC functionality as the IE has not been tested yet > with > > the software AEC. > > > > How is this envisioned to be used at the moment? Is it possible to restrict > the > > usage of the IE to platforms which have HW AEC support? > > > > I think that there could be a point in turning off the IE functionality if > there > > is no HW AEC available, similarly to this CL turns off the HW NS if IE is > > active. > > > > But it depends fully on how this intended to be used. If it is used in a way > > without explicitly checking whether the HW AEC is available on the device then > > there must be code for turning off the IE functionality if a HW AEC is not > > available (at least until the IE has been properly tested with the AEC). > > This CL doesn't enable IE, it just surfaces it to the MediaConstraints. Later we > will use this API to test it and have an A/B experiment limited to the team > before we decide to move forward with it (or not). I would argue that the only > way we can gather real-life data about the interaction between the IE and SW AEC > is by having that case be part of the testing and experiment. I am not saying we > shouldn't also test it offline on more recordings, I just think that those > recordings will never cover all possible scenarios and at the end of the day the > only way of being sure how it affects users is to test it on real usage. Thanks for the clarification! I definitely agree on the value of real-life data on the interaction between IE and SW AEC but I also definitely think it should be preceded by a proper analysis of that interaction. To this date none such has been done. My main concern here is though that during the testing it should be known when the IE is tested on a platform without HW AEC support. But from what you write the current planned usage of the MediaConstraints will be such that it can only be used within a limited control group which is great.
On 2016/05/16 07:38:46, henrika_dont_use wrote: > Regarding A/B tests of IE. It will most likely be super tricky to ensure that > the AEC performance is not affected during such tests since the effect will not > be experienced on the same side as the evaluations is done. > The user who is focused on experiencing pros and cons with the IE will be the > one > who can cause worse echo performance for the other side. Not sure how that will > be handled in an A/B test. That is definitely tricky. I think the correct way is to as far as possible test the IE vs AEC interaction via exploratory testing and then consider that as working in the IE A/B-test. That won't fully guarantee that there are no echo issues but it will avoid polluting the AB-test results with uncertainty about AEC issues and avoid polluting any other echo issues with (potential) issues caused by the IE. Preferably, since there are so few mobile devices which do not have HW AEC, I think those should be removed from the test so that the IE A/B-test results are not affected by that bias. One way to do that could be to turn off the IE when there is no HW AEC.
lgtm for audio_processing* conditioned on that a text is added to the Experiments field in the aecdump recordings whenever IE is active. https://codereview.webrtc.org/1952123003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1952123003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1455: // TODO(peah): Add semicolon-separated concatenations of experiment Please also add a string to the aecdump experiment description that describes whether the IE is active or not. In that way we will from aecdump recording be able to tell whether it was active or not.
Patchset #5 (id:80001) has been deleted
Personally I don't think it will be that hard to correlate the potential AEC issues with the IE, since the AEC experiencing the issues will be in the same device as the running IE. It is true that the person hearing the potential echo will be on the other end, but this other-end-hears-effect-situation is true for all audio processing components all the time except the IE. And it is not complicated to get metrics (ERLE and call length for example) and their distributions with and without the IE. That said, I completely agree with peah that we need to start with exploratory tests and then move to real-world tests. On the other hand, although the AEC issues could pollute the IE experiment, because of the small scale of this experiment it would not pollute the AEC numbers in any significant way. If the team really only wants to enable this for HWAEC without any data it could be done, but I think it makes more sense to enable it for all at least for the testing phase, so we can evaluate their interaction better and then decide if we should disable it in that case. https://codereview.webrtc.org/1952123003/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1952123003/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:759: if (adm()->BuiltInNSIsAvailable()) { On 2016/05/16 07:43:38, peah-webrtc wrote: > On 2016/05/13 16:24:28, aluebs-webrtc wrote: > > On 2016/05/13 12:39:49, peah-webrtc wrote: > > > I'm concerned about the AEC functionality as the IE has not been tested yet > > with > > > the software AEC. > > > > > > How is this envisioned to be used at the moment? Is it possible to restrict > > the > > > usage of the IE to platforms which have HW AEC support? > > > > > > I think that there could be a point in turning off the IE functionality if > > there > > > is no HW AEC available, similarly to this CL turns off the HW NS if IE is > > > active. > > > > > > But it depends fully on how this intended to be used. If it is used in a way > > > without explicitly checking whether the HW AEC is available on the device > then > > > there must be code for turning off the IE functionality if a HW AEC is not > > > available (at least until the IE has been properly tested with the AEC). > > > > This CL doesn't enable IE, it just surfaces it to the MediaConstraints. Later > we > > will use this API to test it and have an A/B experiment limited to the team > > before we decide to move forward with it (or not). I would argue that the only > > way we can gather real-life data about the interaction between the IE and SW > AEC > > is by having that case be part of the testing and experiment. I am not saying > we > > shouldn't also test it offline on more recordings, I just think that those > > recordings will never cover all possible scenarios and at the end of the day > the > > only way of being sure how it affects users is to test it on real usage. > > Thanks for the clarification! I definitely agree on the value of real-life data > on the interaction between IE and SW AEC but I also definitely think it should > be preceded by a proper analysis of that interaction. To this date none such has > been done. > > My main concern here is though that during the testing it should be known when > the IE is tested on a platform without HW AEC support. But from what you write > the current planned usage of the MediaConstraints will be such that it can only > be used within a limited control group which is great. I completely agree we should do a proper analysis of the IE-AEC interaction and I will do it myself. And yes, the plan is to start testing only within the team. https://codereview.webrtc.org/1952123003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1952123003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1455: // TODO(peah): Add semicolon-separated concatenations of experiment On 2016/05/16 08:09:35, peah-webrtc wrote: > Please also add a string to the aecdump experiment description that describes > whether the IE is active or not. In that way we will from aecdump recording be > able to tell whether it was active or not. Good point. Done.
Will land now, but if anyone has any additional comment (solenberg in particular) please let me know.
The CQ bit was checked by aluebs@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrika@webrtc.org, tommi@webrtc.org, peah@webrtc.org Link to the patchset: https://codereview.webrtc.org/1952123003/#ps100001 (title: "Add component to debug proto")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1952123003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1952123003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Description was changed from ========== Surface the IntelligibilityEnhancer on MediaConstraints ========== to ========== Surface the IntelligibilityEnhancer on MediaConstraints R=henrika@webrtc.org, peah@webrtc.org, tommi@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/c9b0c26e0c9c43074692a19ac... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) manually as c9b0c26e0c9c43074692a19ac18532062e5b589e (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Surface the IntelligibilityEnhancer on MediaConstraints R=henrika@webrtc.org, peah@webrtc.org, tommi@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/c9b0c26e0c9c43074692a19ac... ========== to ========== Surface the IntelligibilityEnhancer on MediaConstraints R=henrika@webrtc.org, peah@webrtc.org, tommi@webrtc.org Committed: https://crrev.com/c9b0c26e0c9c43074692a19ac18532062e5b589e Cr-Commit-Position: refs/heads/master@{#12763} ========== |
