Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
141513006:
Wire up AGC to the MediaStreamAudioProcessor (Closed)
|
Created:
6 years, 11 months ago by no longer working on chromium Modified:
6 years, 11 months ago CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionWire up AGC to the MediaStreamAudioProcessor.
This CL enables the AGC in the processor based on the constraints, and also trigger SetVolume() in the capturer if the AGC tries to adjust the volume.
There are also some small changes in the MediaStreamAudioProcessor and its unittest.
BUG=264611
TEST= content_unittest --gtest_filter="*MediaStreamAudioProcessor*"
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246894
Patch Set 1 #Patch Set 2 : uploaded again #
Total comments: 38
Patch Set 3 : addressed the comments. #
Total comments: 10
Patch Set 4 : addressed Andrew's comments #Patch Set 5 : fixed the unittest on android. #
Total comments: 2
Patch Set 6 : Addressed Tommi's comment. #
Messages
Total messages: 14 (0 generated)
Guys, could you please review this CL? Thanks. SX https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:257: // from the constraints. Andrew, do you know if SetAgcConfig is needed or not? I don't how it is hooked up with the constraints. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:394: int err = agc->set_stream_analog_level(volume); Andrew, could you please take a look at this comment. The question is about a old issue that the OS volume's precision is too small to let AGC change the volume, we made a workaround in voe_base_impl but I was told by bjorn that you guys would fix the problem inside AGC.
https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor.h (right): https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.h:67: // will be 0 if the microphone volume is not adjusted. nit: // |new_volume| receives the new microphone volume from the AGC. // The value will be 0 if the microphone volume should not be // adjusted. do you want to add to that a note about the range? https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.h:155: bool stereo_channels_swapping_; nit: audio_mirroring_ https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/w... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/w... content/renderer/media/webrtc_audio_capturer.cc:491: // TODO(xians): Check with ajm@ if we should set curret_volume to current_volume
Ping. Andrew, could you please take a look? Thanks, SX
https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:26: #if defined(ANDROID) What about iOS? I think it is actually appropriate to use 32 kHz for iOS though, since AEC is disabled. Just wanted to make sure you'd thought about it. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:189: while (render_converter_->Convert(&render_frame_)) FYI, we should be able to handle this conversion in a smarter way inside AnalyzeReverseStream, so hopefully this can go away eventually. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:229: if (effects & media::AudioParameters::ECHO_CANCELLER) { How does this interact with the code in media_stream_dependency_factory.cc? Do we need this logic in more than one place? And what if we add support for additional effects? MSDF handles this as soon as the effect/constraint map is updated. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:230: // If platform echo cancellator is enabled, disable the software AEC. cancellator -> canceller https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:242: #if defined(IOS) || defined(ANDROID) Set enable_agc = false here as well. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:257: // from the constraints. On 2014/01/21 09:05:47, xians1 wrote: > Andrew, do you know if SetAgcConfig is needed or not? I don't how it is hooked > up with the constraints. No, we don't support these currently. You can remove this TODO. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:291: // has to be done after all the needed components are enabled. Why is that? Did you hit an error? Typically it would be better to do it first. And just FYI: Although until recently it was necessary to explicitly reinitialize, ProcessStream is now able to handle changes on the fly (it initializes internally). set_sample_rate_hz and friends are basically deprecated; instead I'd like to add an overloaded Initialize() to AudioProcessing which will take these parameters, which can optionally be called if the user knows the parameters ahead of time. In your case here it doesn't really matter since you're hardcoding the sample rates. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:380: if (total_delay_ms > 1000) { In voice engine, this was a lower value (300 ms I believe) and the AEC is actually capped at 500 ms. Any reason for this change? https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.cc:91: #if defined(OS_IOS) For echo cancellation you have the disable switch here, but for e.g. typing detection you have it in: MediaStreamAudioProcessor::InitializeAudioProcessingModule Probably better to standardize on one place. Either move this out to Initialize, or move the typing detection platform-specific decision to EnableTypingDetection. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.cc:97: err |= audio_processing->echo_control_mobile()->set_routing_mode( nit: it's slightly (though probably unmeasurably) better to configure the components before enabling. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/w... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/w... content/renderer/media/webrtc_audio_capturer.cc:442: // Map internal volume range of [0.0, 1.0] into [0, 255] used by the FYI, I'd like to change the AudioProcessing interface to take a float. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/w... content/renderer/media/webrtc_audio_capturer.cc:492: // new_volume to avoid setting the volume twice. Yes, you should update current_volume.
All the comments are addressed. PTAL. Andrew, could you please take a look at comment in media_stream_audio_processor.cc line 391 to see if I can skip that workaround? Thanks, SX https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:26: #if defined(ANDROID) On 2014/01/22 21:50:55, ajm wrote: > What about iOS? I think it is actually appropriate to use 32 kHz for iOS though, > since AEC is disabled. Just wanted to make sure you'd thought about it. WebRtc in Chrome is supported on IOS at all, that is why we don't have any special configuration for IOS. Actually the whole media stack in Chrome is not used at all on IOS. But for the future if webrtc in Chrome works on IOS, I think 32KHz is appropriate. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:189: while (render_converter_->Convert(&render_frame_)) On 2014/01/22 21:50:55, ajm wrote: > FYI, we should be able to handle this conversion in a smarter way inside > AnalyzeReverseStream, so hopefully this can go away eventually. That will be awesome. I hope AnalyzeReverseStream can accept float* as data type and any number as buffer size. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:229: if (effects & media::AudioParameters::ECHO_CANCELLER) { On 2014/01/22 21:50:55, ajm wrote: > How does this interact with the code in media_stream_dependency_factory.cc? Do > we need this logic in more than one place? > > And what if we add support for additional effects? MSDF handles this as soon as > the effect/constraint map is updated. Those code in media_stream_dependency_factory will be removed to a static method in media_stream_audio_procesor_options.h. And when WebRtcAudioCapturer gets the constraings and effect, it will call the static method to update the effect, then WebRtcAudioCapturer opens the hardware. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:230: // If platform echo cancellator is enabled, disable the software AEC. done https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:242: #if defined(IOS) || defined(ANDROID) On 2014/01/22 21:50:55, ajm wrote: > Set enable_agc = false here as well. Done, thanks. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:257: // from the constraints. On 2014/01/22 21:50:55, ajm wrote: > On 2014/01/21 09:05:47, xians1 wrote: > > Andrew, do you know if SetAgcConfig is needed or not? I don't how it is hooked > > up with the constraints. > > No, we don't support these currently. You can remove this TODO. Done. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:291: // has to be done after all the needed components are enabled. On 2014/01/22 21:50:55, ajm wrote: > Why is that? Did you hit an error? Typically it would be better to do it first. > I never hit any problem here. I think the reason why I call set_sample_rate_hz() after the configuration is because we call set_sample_rate_hz() and set_num_channels() for each packet. So I assume it is better to do it after the configuration. I moved it up after the creation. > And just FYI: > Although until recently it was necessary to explicitly reinitialize, > ProcessStream is now able to handle changes on the fly (it initializes > internally). > > set_sample_rate_hz and friends are basically deprecated; instead I'd like to add > an overloaded Initialize() to AudioProcessing which will take these parameters, > which can optionally be called if the user knows the parameters ahead of time. > Do you mean that I can remove these two setters instead? It will be great if I can. How about passing those values via the constructor instead of Initialize() ? > In your case here it doesn't really matter since you're hardcoding the sample > rates. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:380: if (total_delay_ms > 1000) { On 2014/01/22 21:50:55, ajm wrote: > In voice engine, this was a lower value (300 ms I believe) and the AEC is > actually capped at 500 ms. Any reason for this change? This is from old code in WebRtcAudioDeviceImpl, but I agree that we should report a warning bigger than 300. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor.h (right): https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.h:67: // will be 0 if the microphone volume is not adjusted. On 2014/01/21 15:06:20, tommi wrote: > nit: > // |new_volume| receives the new microphone volume from the AGC. > // The value will be 0 if the microphone volume should not be > // adjusted. > > do you want to add to that a note about the range? Done. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.h:155: bool stereo_channels_swapping_; On 2014/01/21 15:06:20, tommi wrote: > nit: audio_mirroring_ Done. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.cc:91: #if defined(OS_IOS) On 2014/01/22 21:50:55, ajm wrote: > For echo cancellation you have the disable switch here, but for e.g. typing > detection you have it in: > MediaStreamAudioProcessor::InitializeAudioProcessingModule > > Probably better to standardize on one place. Either move this out to Initialize, > or move the typing detection platform-specific decision to > EnableTypingDetection. Done. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor_options.cc:97: err |= audio_processing->echo_control_mobile()->set_routing_mode( On 2014/01/22 21:50:55, ajm wrote: > nit: it's slightly (though probably unmeasurably) better to configure the > components before enabling. Done. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/w... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/w... content/renderer/media/webrtc_audio_capturer.cc:442: // Map internal volume range of [0.0, 1.0] into [0, 255] used by the On 2014/01/22 21:50:55, ajm wrote: > FYI, I'd like to change the AudioProcessing interface to take a float. I will be happy to do the chromium change once APM accepts float. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/w... content/renderer/media/webrtc_audio_capturer.cc:491: // TODO(xians): Check with ajm@ if we should set curret_volume to On 2014/01/21 15:06:20, tommi wrote: > current_volume comment was removed. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/w... content/renderer/media/webrtc_audio_capturer.cc:492: // new_volume to avoid setting the volume twice. On 2014/01/22 21:50:55, ajm wrote: > Yes, you should update current_volume. Done.
lgtm with minor changes https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:26: #if defined(ANDROID) On 2014/01/23 12:46:08, xians1 wrote: > On 2014/01/22 21:50:55, ajm wrote: > > What about iOS? I think it is actually appropriate to use 32 kHz for iOS > though, > > since AEC is disabled. Just wanted to make sure you'd thought about it. > > WebRtc in Chrome is supported on IOS at all, that is why we don't have any > special configuration for IOS. Actually the whole media stack in Chrome is not > used at all on IOS. > > But for the future if webrtc in Chrome works on IOS, I think 32KHz is > appropriate. Good. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:189: while (render_converter_->Convert(&render_frame_)) On 2014/01/23 12:46:08, xians1 wrote: > On 2014/01/22 21:50:55, ajm wrote: > > FYI, we should be able to handle this conversion in a smarter way inside > > AnalyzeReverseStream, so hopefully this can go away eventually. > > That will be awesome. I hope AnalyzeReverseStream can accept float* as data type > and any number as buffer size. I unfortunately didn't mean handle everything ;) It should be able to handle downmixing (which is knowledge that should belong to the AEC) and resampling itself. It can do resampling more efficiently, because there are some cases in which it can be avoided. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:229: if (effects & media::AudioParameters::ECHO_CANCELLER) { On 2014/01/23 12:46:08, xians1 wrote: > Those code in media_stream_dependency_factory will be removed to a static method > in media_stream_audio_procesor_options.h. > And when WebRtcAudioCapturer gets the constraings and effect, it will call the > static method to update the effect, then WebRtcAudioCapturer opens the hardware. Sounds good. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:394: int err = agc->set_stream_analog_level(volume); On 2014/01/21 09:05:47, xians1 wrote: > Andrew, could you please take a look at this comment. The question is about a > old issue that the OS volume's precision is too small to let AGC change the > volume, we made a workaround in voe_base_impl but I was told by bjorn that you > guys would fix the problem inside AGC. As discussed off review, you can remove this TODO. https://codereview.chromium.org/141513006/diff/170001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/141513006/diff/170001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:237: // On IOS, VPIO provides built-in AEC. nit: iOS :) https://codereview.chromium.org/141513006/diff/170001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:249: #if defined(OS_IOS) || defined(OS_ANDROID) nit: perhaps move this block up, so all platform specific stuff is together? https://codereview.chromium.org/141513006/diff/170001/content/renderer/media/... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/141513006/diff/170001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:494: // Update the |current_volume| to avoid setting the volume more than once. maybe: "Update the |current_volume| to avoid passing the old volume."?
Still lgtm with these changes. https://codereview.chromium.org/141513006/diff/170001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/141513006/diff/170001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:252: const bool enable_agc = false; As in off review comments, move this just up to the OS_IOS block. https://codereview.chromium.org/141513006/diff/170001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/141513006/diff/170001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:162: const webrtc::GainControl::Mode mode = webrtc::GainControl::kAdaptiveAnalog; As in off review comments, change this to kFixedDigital.
Thanks Andrew. Tommi, would you like take another look? Or you have deferred it to Andrew? https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:189: while (render_converter_->Convert(&render_frame_)) On 2014/01/23 17:30:06, ajm wrote: > On 2014/01/23 12:46:08, xians1 wrote: > > On 2014/01/22 21:50:55, ajm wrote: > > > FYI, we should be able to handle this conversion in a smarter way inside > > > AnalyzeReverseStream, so hopefully this can go away eventually. > > > > That will be awesome. I hope AnalyzeReverseStream can accept float* as data > type > > and any number as buffer size. > > I unfortunately didn't mean handle everything ;) It should be able to handle > downmixing (which is knowledge that should belong to the AEC) and resampling > itself. It can do resampling more efficiently, because there are some cases in > which it can be avoided. That is good thing anyway, please let me know when I can remove the resampling in chrome. https://codereview.chromium.org/141513006/diff/80001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:394: int err = agc->set_stream_analog_level(volume); On 2014/01/23 17:30:06, ajm wrote: > On 2014/01/21 09:05:47, xians1 wrote: > > Andrew, could you please take a look at this comment. The question is about a > > old issue that the OS volume's precision is too small to let AGC change the > > volume, we made a workaround in voe_base_impl but I was told by bjorn that you > > guys would fix the problem inside AGC. > > As discussed off review, you can remove this TODO. Done. Thanks. https://codereview.chromium.org/141513006/diff/170001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/141513006/diff/170001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:237: // On IOS, VPIO provides built-in AEC. On 2014/01/23 17:30:07, ajm wrote: > nit: iOS :) Done. https://codereview.chromium.org/141513006/diff/170001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:249: #if defined(OS_IOS) || defined(OS_ANDROID) On 2014/01/23 17:30:07, ajm wrote: > nit: perhaps move this block up, so all platform specific stuff is together? Done. https://codereview.chromium.org/141513006/diff/170001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:252: const bool enable_agc = false; On 2014/01/24 06:48:59, ajm wrote: > As in off review comments, move this just up to the OS_IOS block. Done. https://codereview.chromium.org/141513006/diff/170001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/141513006/diff/170001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:162: const webrtc::GainControl::Mode mode = webrtc::GainControl::kAdaptiveAnalog; On 2014/01/24 06:48:59, ajm wrote: > As in off review comments, change this to kFixedDigital. Ah, thanks. I noticed this mistake while I was making the unittest, I thought I corrected it but obviously I didn't. https://codereview.chromium.org/141513006/diff/170001/content/renderer/media/... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/141513006/diff/170001/content/renderer/media/... content/renderer/media/webrtc_audio_capturer.cc:494: // Update the |current_volume| to avoid setting the volume more than once. On 2014/01/23 17:30:07, ajm wrote: > maybe: > "Update the |current_volume| to avoid passing the old volume."? Done.
Hi Tommi, I know you are very busy so I am going to land this CL now. And feel free to take a look when you have time, I will create a new CL if you have more comments.
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
lgtm. It would be good to fix the below thing (it's a tiny change), but I'm ok with it as is since it's not a problem in this case. (consistency is though a good thing) https://codereview.chromium.org/141513006/diff/570001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/141513006/diff/570001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:199: *new_volume = 0; Is it correct to do this here and not below the Convert() check? A rule of thumb I like to follow is that if a method returns failure, it should not have modified any of the output parameters.
https://codereview.chromium.org/141513006/diff/570001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/141513006/diff/570001/content/renderer/media/... content/renderer/media/media_stream_audio_processor.cc:199: *new_volume = 0; On 2014/01/24 12:41:22, tommi wrote: > Is it correct to do this here and not below the Convert() check? > > A rule of thumb I like to follow is that if a method returns failure, it should > not have modified any of the output parameters. Right, actually the new_volume won't be used if the method returns false. So I removed the setter here.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/141513006/630001
Message was sent while issue was closed.
Change committed as 246894 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
