Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
229573003:
Adds more logging for audio input issues. (Closed)
|
Created:
6 years, 8 months ago by jiayl Modified:
6 years, 8 months ago CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdds more logging for audio input issues.
-Adds the audio input device id and name to the stream generation message.
-Re-enables the no_data_timer in AudioInputController for logging. The NO_DATA_ERROR is ignored and will not stop capturing.
-Adds a AudioPowerMonitor to WebRtcAudioCapturer to log the audio power level.
R=vrk@chromium.org,xians@chromium.org,tommi@chromium.org
BUG=360756
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263127
Patch Set 1 #Patch Set 2 : #
Total comments: 15
Patch Set 3 : #
Total comments: 1
Patch Set 4 : #
Messages
Total messages: 14 (0 generated)
The audio level indicator is calculated as the max absolute value of the buffer, which I borrow the idea from MediaStreamAudioLevelCalculator::Calculate, but have no idea if it makes sense here. xians/tommi/vrk, please advise.
Updated based on the email discussion. PTAL.
On 2014/04/09 21:30:50, jiayl wrote: > Updated based on the email discussion. PTAL. The current patch logs the audio level every 10 seconds. If we decide to only log the state transitions, we still need to decide a time interval of checking the power level. Calculating the power for every sample is probably too heavy.
https://codereview.chromium.org/229573003/diff/60001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (left): https://codereview.chromium.org/229573003/diff/60001/media/audio/audio_input_... media/audio/audio_input_controller.cc:205: // This is a fix for crbug.com/357501. The timer can trigger when closing Instead of this, all streams should be paused when suspend starts and resumed some time after resume comes back. It shouldn't be too hard to do this, but different than AudioOutputStreams since AudioInputController owns the raw stream vs a proxy object. Each AudioInputController could listen for power events and then pause upon suspend and restart the stream upon resume (keeping the renderer process oblivious). Streams should not be started within x seconds of resume either, so the AudioManager should probably still maintain a ShouldDeferStreamStart() method which can be called by the controllers.
https://codereview.chromium.org/229573003/diff/60001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (left): https://codereview.chromium.org/229573003/diff/60001/media/audio/audio_input_... media/audio/audio_input_controller.cc:205: // This is a fix for crbug.com/357501. The timer can trigger when closing On 2014/04/09 22:16:11, DaleCurtis wrote: > Instead of this, all streams should be paused when suspend starts and resumed > some time after resume comes back. It shouldn't be too hard to do this, but > different than AudioOutputStreams since AudioInputController owns the raw stream > vs a proxy object. > > Each AudioInputController could listen for power events and then pause upon > suspend and restart the stream upon resume (keeping the renderer process > oblivious). > > Streams should not be started within x seconds of resume either, so the > AudioManager should probably still maintain a ShouldDeferStreamStart() method > which can be called by the controllers. That's reasonable but beyond the scope of this change. This is not trying to fix any code bug but adding log messages to help us understand where the problem is when audio input is dropped.
https://codereview.chromium.org/229573003/diff/60001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (left): https://codereview.chromium.org/229573003/diff/60001/media/audio/audio_input_... media/audio/audio_input_controller.cc:205: // This is a fix for crbug.com/357501. The timer can trigger when closing On 2014/04/09 22:24:57, jiayl wrote: > On 2014/04/09 22:16:11, DaleCurtis wrote: > > Instead of this, all streams should be paused when suspend starts and resumed > > some time after resume comes back. It shouldn't be too hard to do this, but > > different than AudioOutputStreams since AudioInputController owns the raw > stream > > vs a proxy object. > > > > Each AudioInputController could listen for power events and then pause upon > > suspend and restart the stream upon resume (keeping the renderer process > > oblivious). > > > > Streams should not be started within x seconds of resume either, so the > > AudioManager should probably still maintain a ShouldDeferStreamStart() method > > which can be called by the controllers. > > That's reasonable but beyond the scope of this change. > This is not trying to fix any code bug but adding log messages to help us > understand where the problem is when audio input is dropped. Agreed, but I don't think you should enable the no data timer here either. Ignoring the error later is also kind of sketchy. I'd just stick to adding the logging. I'm really the peanut gallery here though, so I defer tommi@, xians@, henrika@ :)
https://codereview.chromium.org/229573003/diff/60001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (left): https://codereview.chromium.org/229573003/diff/60001/media/audio/audio_input_... media/audio/audio_input_controller.cc:205: // This is a fix for crbug.com/357501. The timer can trigger when closing On 2014/04/09 22:34:18, DaleCurtis wrote: > On 2014/04/09 22:24:57, jiayl wrote: > > On 2014/04/09 22:16:11, DaleCurtis wrote: > > > Instead of this, all streams should be paused when suspend starts and > resumed > > > some time after resume comes back. It shouldn't be too hard to do this, but > > > different than AudioOutputStreams since AudioInputController owns the raw > > stream > > > vs a proxy object. > > > > > > Each AudioInputController could listen for power events and then pause upon > > > suspend and restart the stream upon resume (keeping the renderer process > > > oblivious). > > > > > > Streams should not be started within x seconds of resume either, so the > > > AudioManager should probably still maintain a ShouldDeferStreamStart() > method > > > which can be called by the controllers. > > > > That's reasonable but beyond the scope of this change. > > This is not trying to fix any code bug but adding log messages to help us > > understand where the problem is when audio input is dropped. > > Agreed, but I don't think you should enable the no data timer here either. > Ignoring the error later is also kind of sketchy. I'd just stick to adding the > logging. I'm really the peanut gallery here though, so I defer tommi@, xians@, > henrika@ :) Yeah, I should have explained it. We have to enable the timer to know if the input data stops coming in. It would be better if we simply not fire the NO_DATA_ERROR when the timer callback happens. But unfortunately the webrtc logging methods are only available inside content/, not from AudioInputController. That means we need some way to pass the log message to the content layer, while the EventHandler is the only existing connection. So we either 1) fire the error for logging and make EventHandler ignore the error, or 2) add a new method like OnLogMessage to the EventHandler interface. I chose 1) because it's less code so better for merging.
https://codereview.chromium.org/229573003/diff/60001/content/browser/renderer... File content/browser/renderer_host/media/audio_input_renderer_host.cc (right): https://codereview.chromium.org/229573003/diff/60001/content/browser/renderer... content/browser/renderer_host/media/audio_input_renderer_host.cc:251: std::string device_name; nit: Use the AudioDeviceName type instead? https://codereview.chromium.org/229573003/diff/60001/content/browser/renderer... content/browser/renderer_host/media/audio_input_renderer_host.cc:338: ", device id: " + device_id); Afaik we have actually stayed away from writing the physical device id to the log for privacy reasons. Are we doing this elsewhere? https://codereview.chromium.org/229573003/diff/60001/content/renderer/media/w... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/229573003/diff/60001/content/renderer/media/w... content/renderer/media/webrtc_audio_capturer.cc:502: audio_power_monitor_.ReadCurrentPowerAndClip(); nit: I don't suppose there's a single threaded AudioPowerMonitor that we could use only on the capture thread and avoid grabbing locks? https://codereview.chromium.org/229573003/diff/60001/content/renderer/media/w... content/renderer/media/webrtc_audio_capturer.cc:504: "WAC::Capture: current_audio_power=%.2fdBFS.", result.first)); indent
https://codereview.chromium.org/229573003/diff/60001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (left): https://codereview.chromium.org/229573003/diff/60001/media/audio/audio_input_... media/audio/audio_input_controller.cc:205: // This is a fix for crbug.com/357501. The timer can trigger when closing On 2014/04/09 22:16:11, DaleCurtis wrote: > Instead of this, all streams should be paused when suspend starts and resumed > some time after resume comes back. It shouldn't be too hard to do this, but > different than AudioOutputStreams since AudioInputController owns the raw stream > vs a proxy object. > > Each AudioInputController could listen for power events and then pause upon > suspend and restart the stream upon resume (keeping the renderer process > oblivious). > > Streams should not be started within x seconds of resume either, so the > AudioManager should probably still maintain a ShouldDeferStreamStart() method > which can be called by the controllers. I also believe hooking up ShouldDeferStreamStart to the input code is necessary to be done here. I am going to create a bug for it. As Jiayang pointed out, lets do it in a separate Cl. https://codereview.chromium.org/229573003/diff/60001/media/audio/audio_input_... media/audio/audio_input_controller.cc:205: // This is a fix for crbug.com/357501. The timer can trigger when closing On 2014/04/09 22:34:18, DaleCurtis wrote: > On 2014/04/09 22:24:57, jiayl wrote: > > On 2014/04/09 22:16:11, DaleCurtis wrote: > > > Instead of this, all streams should be paused when suspend starts and > resumed > > > some time after resume comes back. It shouldn't be too hard to do this, but > > > different than AudioOutputStreams since AudioInputController owns the raw > > stream > > > vs a proxy object. > > > > > > Each AudioInputController could listen for power events and then pause upon > > > suspend and restart the stream upon resume (keeping the renderer process > > > oblivious). > > > > > > Streams should not be started within x seconds of resume either, so the > > > AudioManager should probably still maintain a ShouldDeferStreamStart() > method > > > which can be called by the controllers. > > > > That's reasonable but beyond the scope of this change. > > This is not trying to fix any code bug but adding log messages to help us > > understand where the problem is when audio input is dropped. > > Agreed, but I don't think you should enable the no data timer here either. > Ignoring the error later is also kind of sketchy. I'd just stick to adding the > logging. I'm really the peanut gallery here though, so I defer tommi@, xians@, > henrika@ :) I hope the current code will just be temporary to allow us collect some information on if we are still hitting the NO_DATA_ERROR in unexpected cases. Hopefully we will remove it completely (if NO_DATA_ERROR does not happen any more) or somehow handle the error in another way (if NO_DATA_ERROR happens quite often). I talked to Henrik offline, and he said we'd better get some logs so that we would understand more before taking another steps. So I would just defer to Tommi on if this is a good way to add log or do it another way. https://codereview.chromium.org/229573003/diff/60001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/229573003/diff/60001/media/audio/audio_input_... media/audio/audio_input_controller.cc:323: handler_->OnError(this, NO_DATA_ERROR); Tommi, on the output side, Dale added a Media.AudioOutputControllerPlaybackStartupSuccess to keep track if getting a OnMoreData callback within 5 seconds after the stream is started. From the histogram, it reports an average of 0.45% failure on Mac, while it is about 0..1% on other platforms. should we also put some UMA stats here for the input? It probably should be done on another CL though.
Addressed tommi's comments. PTAL https://codereview.chromium.org/229573003/diff/60001/content/browser/renderer... File content/browser/renderer_host/media/audio_input_renderer_host.cc (right): https://codereview.chromium.org/229573003/diff/60001/content/browser/renderer... content/browser/renderer_host/media/audio_input_renderer_host.cc:251: std::string device_name; On 2014/04/10 09:07:22, tommi wrote: > nit: Use the AudioDeviceName type instead? Done. https://codereview.chromium.org/229573003/diff/60001/content/browser/renderer... content/browser/renderer_host/media/audio_input_renderer_host.cc:338: ", device id: " + device_id); On 2014/04/10 09:07:22, tommi wrote: > Afaik we have actually stayed away from writing the physical device id to the > log for privacy reasons. Are we doing this elsewhere? Good point. Removed device_id. https://codereview.chromium.org/229573003/diff/60001/content/renderer/media/w... File content/renderer/media/webrtc_audio_capturer.cc (right): https://codereview.chromium.org/229573003/diff/60001/content/renderer/media/w... content/renderer/media/webrtc_audio_capturer.cc:502: audio_power_monitor_.ReadCurrentPowerAndClip(); On 2014/04/10 09:07:22, tommi wrote: > nit: I don't suppose there's a single threaded AudioPowerMonitor that we could > use only on the capture thread and avoid grabbing locks? No, there isn't. https://codereview.chromium.org/229573003/diff/60001/content/renderer/media/w... content/renderer/media/webrtc_audio_capturer.cc:504: "WAC::Capture: current_audio_power=%.2fdBFS.", result.first)); On 2014/04/10 09:07:22, tommi wrote: > indent Done.
lgtm https://codereview.chromium.org/229573003/diff/80001/content/browser/renderer... File content/browser/renderer_host/media/audio_input_renderer_host.cc (right): https://codereview.chromium.org/229573003/diff/80001/content/browser/renderer... content/browser/renderer_host/media/audio_input_renderer_host.cc:264: device_name.unique_id = info->device.id; ah, I thought that |device| was of type AudioDeviceName.. I guess there was little to no benefit in my suggestion then :-/
The CQ bit was checked by jiayl@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/229573003/100001
Message was sent while issue was closed.
Change committed as 263127 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
