Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
24742007:
When an audio track is disabled, still pass the data to webrtc for audio processing. (Closed)
|
Created:
7 years, 2 months ago by jiayl Modified:
7 years, 2 months ago Reviewers:
vrk (LEFT CHROMIUM), ajm, no longer working on chromium, tommi (sloooow) - chröme, juberti CC:
chromium-reviews, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, jam, wjia+watch_chromium.org, darin-cc_chromium.org, jansson, tnakamura, juberti2 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionWhen an audio track is disabled, still pass the data to webrtc for audio processing.
The remote side will not get the real audio because webrtc will replace the frames with silence.
BUG=299007
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226339
Patch Set 1 #
Total comments: 12
Patch Set 2 : #
Total comments: 3
Patch Set 3 : passing real volume #Patch Set 4 : sync #Patch Set 5 : sync #Messages
Total messages: 23 (0 generated)
This is desired for M31. PTAL. Thanks!
Your CL is not the right solution, lets continue the discussion in the email thread and figure out a correct solution instead of pushing out quick hack. https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... File content/renderer/media/webrtc_local_audio_track.cc (right): https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... content/renderer/media/webrtc_local_audio_track.cc:163: volume = 0; This is wrong, it will confuse the APM that the current analog volume is 0, then APM will set the microphone volume to a value closed to 0.
https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... File content/renderer/media/webrtc_local_audio_renderer.cc (right): https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... content/renderer/media/webrtc_local_audio_renderer.cc:72: if (!current_volume) remove https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... File content/renderer/media/webrtc_local_audio_track.cc (right): https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... content/renderer/media/webrtc_local_audio_track.cc:60: ConfiguredBuffer() : sink_buffer_size_(0) {} , zero_output_(false) https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... content/renderer/media/webrtc_local_audio_track.cc:82: DCHECK(fifo_->frames() + audio_source->frames() <= fifo_->max_frames()); zero_output_ = false; https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... content/renderer/media/webrtc_local_audio_track.cc:86: bool Consume() { add if (zero_output_) return true; https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... content/renderer/media/webrtc_local_audio_track.cc:96: add function: void ZeroOutput() { if (zero_output_) return; fifo_->Clear(); memset(buffer_.get(), 0, sink_buffer_size_ * params_.channels() * sizeof(int16)); zero_output_ = true; } https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... content/renderer/media/webrtc_local_audio_track.cc:109: int sink_buffer_size_; add bool zero_output_; https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... content/renderer/media/webrtc_local_audio_track.cc:156: scoped_refptr<ConfiguredBuffer> current_buffer; add bool enabled = false; https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... content/renderer/media/webrtc_local_audio_track.cc:162: if (!enabled()) { remove this if check https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... content/renderer/media/webrtc_local_audio_track.cc:172: sinks = sinks_; enabled = enabled(); https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... content/renderer/media/webrtc_local_audio_track.cc:176: current_buffer->Push(audio_source); if (enabled) { // Push the data to the fifo. current_buffer->Push(audio_source); } else { // add comment current_buffer->ZeroOutput(); } https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... content/renderer/media/webrtc_local_audio_track.cc:193: key_pressed); if (!enabled) break;
On 2013/09/27 12:31:52, xians1 wrote: > https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... > File content/renderer/media/webrtc_local_audio_renderer.cc (right): > > https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... > content/renderer/media/webrtc_local_audio_renderer.cc:72: if (!current_volume) > remove > > https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... > File content/renderer/media/webrtc_local_audio_track.cc (right): > > https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... > content/renderer/media/webrtc_local_audio_track.cc:60: ConfiguredBuffer() : > sink_buffer_size_(0) {} > , zero_output_(false) > > https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... > content/renderer/media/webrtc_local_audio_track.cc:82: DCHECK(fifo_->frames() + > audio_source->frames() <= fifo_->max_frames()); > zero_output_ = false; > > https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... > content/renderer/media/webrtc_local_audio_track.cc:86: bool Consume() { > add > if (zero_output_) > return true; > > https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... > content/renderer/media/webrtc_local_audio_track.cc:96: > add function: > > void ZeroOutput() { > if (zero_output_) > return; > > fifo_->Clear(); > memset(buffer_.get(), 0, sink_buffer_size_ * params_.channels() * > sizeof(int16)); > zero_output_ = true; > } > > https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... > content/renderer/media/webrtc_local_audio_track.cc:109: int sink_buffer_size_; > add bool zero_output_; > > https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... > content/renderer/media/webrtc_local_audio_track.cc:156: > scoped_refptr<ConfiguredBuffer> current_buffer; > add bool enabled = false; > > https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... > content/renderer/media/webrtc_local_audio_track.cc:162: if (!enabled()) { > remove this if check > > https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... > content/renderer/media/webrtc_local_audio_track.cc:172: sinks = sinks_; > enabled = enabled(); > > https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... > content/renderer/media/webrtc_local_audio_track.cc:176: > current_buffer->Push(audio_source); > if (enabled) { > // Push the data to the fifo. > current_buffer->Push(audio_source); > } else { > // add comment > current_buffer->ZeroOutput(); > } > > https://codereview.chromium.org/24742007/diff/1/content/renderer/media/webrtc... > content/renderer/media/webrtc_local_audio_track.cc:193: key_pressed); > if (!enabled) > break; If I understand it correctly, the approach you suggested will pass zeros to webrtc, which will lead to incorrect typing detection result when the user is typing, because the typing detection looks for energy spikes to match the keyboard event.
+ Andrew, Andrew, do you know if APM will set the vad_activity_ to other values if we pass 0 as data to APM?
On 2013/09/27 15:24:47, xians1 wrote: > + Andrew, > > Andrew, do you know if APM will set the vad_activity_ to other values if we pass > 0 as data to APM? Discussed with Shijing offline. Confirmed that passing 0 does break typing detection. We agree to pass real data down and set volume = 0 but do not set capturer's volume when disabled. Patch updated. PTAL.
lgtm if it passes your test. sx https://codereview.chromium.org/24742007/diff/10001/content/renderer/media/we... File content/renderer/media/webrtc_local_audio_track.cc (right): https://codereview.chromium.org/24742007/diff/10001/content/renderer/media/we... content/renderer/media/webrtc_local_audio_track.cc:176: int current_volume = disabled ? 0 : volume; Note that you are fooling the AGC, please test this CL in your local machine and make sure AGC won't try to set the volume to a value closed to 0 when changing the track from disable to enable state.
On 2013/09/27 20:22:52, xians1 wrote: > lgtm if it passes your test. > > sx > > https://codereview.chromium.org/24742007/diff/10001/content/renderer/media/we... > File content/renderer/media/webrtc_local_audio_track.cc (right): > > https://codereview.chromium.org/24742007/diff/10001/content/renderer/media/we... > content/renderer/media/webrtc_local_audio_track.cc:176: int current_volume = > disabled ? 0 : volume; > Note that you are fooling the AGC, please test this CL in your local machine and > make sure AGC won't try to set the volume to a value closed to 0 when changing > the track from disable to enable state. Verified that the input volume is not changed when switching the enabled state.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/24742007/10001
I think this needs some work. "The remote side will not get the audio either because webrtc will replace the frames with silence." What in webrtc is doing this? https://codereview.chromium.org/24742007/diff/10001/content/renderer/media/we... File content/renderer/media/webrtc_local_audio_renderer.cc (right): https://codereview.chromium.org/24742007/diff/10001/content/renderer/media/we... content/renderer/media/webrtc_local_audio_renderer.cc:72: if (!current_volume) This seems really hacky. What if the current volume really is zero? https://codereview.chromium.org/24742007/diff/10001/content/renderer/media/we... File content/renderer/media/webrtc_local_audio_track.cc (right): https://codereview.chromium.org/24742007/diff/10001/content/renderer/media/we... content/renderer/media/webrtc_local_audio_track.cc:176: int current_volume = disabled ? 0 : volume; On 2013/09/27 20:22:52, xians1 wrote: > Note that you are fooling the AGC, please test this CL in your local machine and > make sure AGC won't try to set the volume to a value closed to 0 when changing > the track from disable to enable state. This doesn't look good. You shouldn't be messing around with the volume you provide, even if disabled.
On 2013/09/27 22:54:40, ajm wrote: > I think this needs some work. > > "The remote side will not get the audio either because webrtc will replace the > frames with silence." > > What in webrtc is doing this? > Triggered from WebRtcSession::SetAudioSend, done in https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > https://codereview.chromium.org/24742007/diff/10001/content/renderer/media/we... > File content/renderer/media/webrtc_local_audio_renderer.cc (right): > > https://codereview.chromium.org/24742007/diff/10001/content/renderer/media/we... > content/renderer/media/webrtc_local_audio_renderer.cc:72: if (!current_volume) > This seems really hacky. What if the current volume really is zero? > Would we want to render anything if volume is zero? > https://codereview.chromium.org/24742007/diff/10001/content/renderer/media/we... > File content/renderer/media/webrtc_local_audio_track.cc (right): > > https://codereview.chromium.org/24742007/diff/10001/content/renderer/media/we... > content/renderer/media/webrtc_local_audio_track.cc:176: int current_volume = > disabled ? 0 : volume; > On 2013/09/27 20:22:52, xians1 wrote: > > Note that you are fooling the AGC, please test this CL in your local machine > and > > make sure AGC won't try to set the volume to a value closed to 0 when changing > > the track from disable to enable state. > > This doesn't look good. You shouldn't be messing around with the volume you > provide, even if disabled. This is not intended to be the long term solution and will go away when audio processing is moved to Chrome. There is no better alternative to unblock typing detection at the moment. I tested that this does not seem to trigger AGC when switching the enabled state.
ajm, if you have another suggestion on how this could be done, please advise. Want to make sure we find a reasonable solution, even in the short term. On Fri, Sep 27, 2013 at 4:02 PM, <jiayl@chromium.org> wrote: > On 2013/09/27 22:54:40, ajm wrote: > >> I think this needs some work. >> > > "The remote side will not get the audio either because webrtc will >> replace the >> frames with silence." >> > > What in webrtc is doing this? >> > > Triggered from WebRtcSession::SetAudioSend, done in > https://code.google.com/p/**chromium/codesearch#chromium/** > src/third_party/webrtc/voice_**engine/channel.cc&rcl=**1380196616&l=4454<https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc/voice_engine/channel.cc&rcl=1380196616&l=4454> > > > > https://codereview.chromium.**org/24742007/diff/10001/** > content/renderer/media/webrtc_**local_audio_renderer.cc<https://codereview.chromium.org/24742007/diff/10001/content/renderer/media/webrtc_local_audio_renderer.cc> > >> File content/renderer/media/webrtc_**local_audio_renderer.cc (right): >> > > > https://codereview.chromium.**org/24742007/diff/10001/** > content/renderer/media/webrtc_**local_audio_renderer.cc#**newcode72<https://codereview.chromium.org/24742007/diff/10001/content/renderer/media/webrtc_local_audio_renderer.cc#newcode72> > >> content/renderer/media/webrtc_**local_audio_renderer.cc:72: if >> (!current_volume) >> This seems really hacky. What if the current volume really is zero? >> > > Would we want to render anything if volume is zero? > > > > https://codereview.chromium.**org/24742007/diff/10001/** > content/renderer/media/webrtc_**local_audio_track.cc<https://codereview.chromium.org/24742007/diff/10001/content/renderer/media/webrtc_local_audio_track.cc> > >> File content/renderer/media/webrtc_**local_audio_track.cc (right): >> > > > https://codereview.chromium.**org/24742007/diff/10001/** > content/renderer/media/webrtc_**local_audio_track.cc#**newcode176<https://codereview.chromium.org/24742007/diff/10001/content/renderer/media/webrtc_local_audio_track.cc#newcode176> > >> content/renderer/media/webrtc_**local_audio_track.cc:176: int >> current_volume = >> disabled ? 0 : volume; >> On 2013/09/27 20:22:52, xians1 wrote: >> > Note that you are fooling the AGC, please test this CL in your local >> machine >> and >> > make sure AGC won't try to set the volume to a value closed to 0 when >> > changing > >> > the track from disable to enable state. >> > > This doesn't look good. You shouldn't be messing around with the volume >> you >> provide, even if disabled. >> > This is not intended to be the long term solution and will go away when > audio > processing is moved to Chrome. > There is no better alternative to unblock typing detection at the moment. > I tested that this does not seem to trigger AGC when switching the enabled > state. > > > > > https://codereview.chromium.**org/24742007/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/09/27 23:12:38, juberti wrote: > ajm, if you have another suggestion on how this could be done, please > advise. Want to make sure we find a reasonable solution, even in the short > term. Would something like this work? https://codereview.chromium.org/25062005/ Sorry if I'm missing something in the architecture that would prevent this.
On 2013/09/28 01:32:25, ajm wrote: > On 2013/09/27 23:12:38, juberti wrote: > > ajm, if you have another suggestion on how this could be done, please > > advise. Want to make sure we find a reasonable solution, even in the short > > term. > > Would something like this work? > https://codereview.chromium.org/25062005/ > > Sorry if I'm missing something in the architecture that would prevent this. Hi Andrew, I took a look at your proposal and it injects more hacky code to workaround the problem. As I explained to Jiayang offline, the local renderer is the least thing we concern about since the usage is very rare. If injecting 0 as volume to APM is not appropriate (curiously, why is it a problem?), then just feed the correct volume down the sinks and fail the "disable" attribute. The AGC problem Jiayang found out was a separate issue, I am looking into it now.
I did some debugging on the AGC in Linux, the anglog AGC is on, but APM is always returning the value which is the same as the current volume, and it does not adjust the volume in any case. FYI, I tested it without Jiay's patch.
Patch updated to pass the real volume to the sinks. PTAL.
lgtm, thanks Jiayang.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/24742007/33001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/24742007/43001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/24742007/43001
Message was sent while issue was closed.
Change committed as 226339 |
