Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
550933002:
Adding media.MicrophoneVolume UMA stat (Closed)
|
Created:
6 years, 3 months ago by henrika (OOO until Aug 14) Modified:
6 years, 3 months ago Reviewers:
Henrik Grunell, no longer working on chromium, Alexei Svitkine (slow), tommi (sloooow) - chröme CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdding media.MicrophoneVolume UMA stat.
BUG=405449, 411380
Committed: https://crrev.com/758955325212b2f561f09ca481967fb8c497e1ec
Cr-Commit-Position: refs/heads/master@{#293919}
Patch Set 1 #Patch Set 2 : Added native WebRTC log #Patch Set 3 : Adding XML part #
Total comments: 5
Patch Set 4 : Adds warning if mic level is below 10% of max #
Total comments: 12
Patch Set 5 : Feedback from all reviewers #Patch Set 6 : Fixed unit in XML file #Patch Set 7 : Fixed build warning #
Messages
Total messages: 24 (6 generated)
henrika@chromium.org changed reviewers: + grunell@chromium.org, xians@chromium.org
henrika@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine@: for XML parts. Thanks!
https://codereview.chromium.org/550933002/diff/40001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/550933002/diff/40001/media/audio/audio_input_... media/audio/audio_input_controller.cc:514: int mic_volume_percent = static_cast<int>(100.0 * volume); why can't we log it as a float? https://codereview.chromium.org/550933002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/550933002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:11486: +<histogram name="Media.MicrophoneVolume" units="%"> I don't see how this is used.
https://codereview.chromium.org/550933002/diff/40001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/550933002/diff/40001/media/audio/audio_input_... media/audio/audio_input_controller.cc:514: int mic_volume_percent = static_cast<int>(100.0 * volume); It seems to be the preferred style to use int here in Chrome and it does not add much information. We just want a rough figure of where we are here. https://codereview.chromium.org/550933002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/550933002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:11486: +<histogram name="Media.MicrophoneVolume" units="%"> Not sure if I understand. The histogram is called: Media.MicrophoneVolume
Also added warning if mic level is below 10% of max.
lgtm https://codereview.chromium.org/550933002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/550933002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:11490: + than 100% on Linux. Measured approximately four times per second given that Nit: "given that" -> "when"
tommi@chromium.org changed reviewers: + tommi@chromium.org
https://codereview.chromium.org/550933002/diff/60001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/550933002/diff/60001/media/audio/audio_input_... media/audio/audio_input_controller.cc:578: log_string += " <=> low microphone level!"; nit: ":" instead of <=> Can we log the actual value? https://codereview.chromium.org/550933002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/550933002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:11490: + than 100% on Linux. Measured approximately four times per second given that On 2014/09/08 14:51:38, Alexei Svitkine wrote: > Nit: "given that" -> "when" nit: I think the "given that a low-latency input audio stream is active" part can just be removed.
On 2014/09/08 18:38:40, tommi wrote: > https://codereview.chromium.org/550933002/diff/60001/media/audio/audio_input_... > File media/audio/audio_input_controller.cc (right): > > https://codereview.chromium.org/550933002/diff/60001/media/audio/audio_input_... > media/audio/audio_input_controller.cc:578: log_string += " <=> low microphone > level!"; > nit: ":" instead of <=> > Can we log the actual value? > > https://codereview.chromium.org/550933002/diff/60001/tools/metrics/histograms... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/550933002/diff/60001/tools/metrics/histograms... > tools/metrics/histograms/histograms.xml:11490: + than 100% on Linux. Measured > approximately four times per second given that > On 2014/09/08 14:51:38, Alexei Svitkine wrote: > > Nit: "given that" -> "when" > > nit: I think the "given that a low-latency input audio stream is active" part > can just be removed. lgtm % those nits
LGTM with nits fixed. (Mine and others.) https://codereview.chromium.org/550933002/diff/60001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/550933002/diff/60001/media/audio/audio_input_... media/audio/audio_input_controller.cc:37: const int kLowLevelMicrophoneLevelInPercent = 10; Nit: Remove "In". https://codereview.chromium.org/550933002/diff/60001/media/audio/audio_input_... media/audio/audio_input_controller.cc:576: "AIC::OnData: microphone volume=%d percent", microphone_volume_percent); Nit: Replace "percent" with "%" in the string. Mainly to reduce amount of log. https://codereview.chromium.org/550933002/diff/60001/media/audio/audio_input_... media/audio/audio_input_controller.cc:578: log_string += " <=> low microphone level!"; On 2014/09/08 18:38:40, tommi wrote: > nit: ":" instead of <=> > Can we log the actual value? But the actual value is just 1/100 of the % value. Seems redundant to me to add that one too.
A general reflection: I'm afraid of spamming the logs. (Still :)) Maybe we should find out a smart way to trigger logging when we think something is (or could be) wrong instead of having to parse the log manually? Wdyt? Fine to do in a follow-up I think.
"A general reflection: I'm afraid of spamming the logs. (Still :)) Maybe we should find out a smart way to trigger logging when we think something is (or could be) wrong instead of having to parse the log manually? Wdyt? Fine to do in a follow-up I think." I agree (in general) but here it is with a clear purpose to track down an known problem. I am more concerned about the existing logs we see in libjingle where very few knows why the logs were added and if they reflect a real problem or not. The logs in this CL will only be done 4 times per minute. But yes, we should strive to remove if possible.
lgtm https://codereview.chromium.org/550933002/diff/40001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/550933002/diff/40001/media/audio/audio_input_... media/audio/audio_input_controller.cc:514: int mic_volume_percent = static_cast<int>(100.0 * volume); On 2014/09/08 13:50:57, henrika wrote: > It seems to be the preferred style to use int here in Chrome and it does not add > much information. We just want a rough figure of where we are here. Acknowledged.
On 2014/09/09 08:20:11, henrika wrote: > "A general reflection: I'm afraid of spamming the logs. (Still :)) Maybe we > should find out a smart way to trigger logging when we think something is (or > could be) wrong instead of having to parse the log manually? Wdyt? Fine to do in > a follow-up I think." > > I agree (in general) but here it is with a clear purpose to track down an known > problem. > I am more concerned about the existing logs we see in libjingle where very few > knows why the logs were added and if they reflect a real problem or not. > The logs in this CL will only be done 4 times per minute. > > But yes, we should strive to remove if possible. Haha, I was thinking 4 times a second. (Or did I read that somewhere?) But that would have been plain horrible. Nevermind.
Thanks all. Landing. https://codereview.chromium.org/550933002/diff/60001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/550933002/diff/60001/media/audio/audio_input_... media/audio/audio_input_controller.cc:37: const int kLowLevelMicrophoneLevelInPercent = 10; On 2014/09/09 08:11:31, Henrik Grunell wrote: > Nit: Remove "In". Done. https://codereview.chromium.org/550933002/diff/60001/media/audio/audio_input_... media/audio/audio_input_controller.cc:576: "AIC::OnData: microphone volume=%d percent", microphone_volume_percent); On 2014/09/09 08:11:31, Henrik Grunell wrote: > Nit: Replace "percent" with "%" in the string. Mainly to reduce amount of log. Done. https://codereview.chromium.org/550933002/diff/60001/media/audio/audio_input_... media/audio/audio_input_controller.cc:578: log_string += " <=> low microphone level!"; On 2014/09/09 08:11:31, Henrik Grunell wrote: > On 2014/09/08 18:38:40, tommi wrote: > > nit: ":" instead of <=> > > Can we log the actual value? > > But the actual value is just 1/100 of the % value. Seems redundant to me to add > that one too. Done. https://codereview.chromium.org/550933002/diff/60001/media/audio/audio_input_... media/audio/audio_input_controller.cc:578: log_string += " <=> low microphone level!"; It is logged. Note that I add an extra warning to the existing value. To be clear. One example: [stream_id=1] AIC::OnData: microphone volume=2% <=> low microphone level! https://codereview.chromium.org/550933002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/550933002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:11490: + than 100% on Linux. Measured approximately four times per second given that Removed. See comment from tommi@ below. https://codereview.chromium.org/550933002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:11490: + than 100% on Linux. Measured approximately four times per second given that On 2014/09/08 18:38:40, tommi wrote: > On 2014/09/08 14:51:38, Alexei Svitkine wrote: > > Nit: "given that" -> "when" > > nit: I think the "given that a low-latency input audio stream is active" part > can just be removed. Acknowledged.
The CQ bit was checked by henrika@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/henrika@chromium.org/550933002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by henrika@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/henrika@chromium.org/550933002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as 7447e578139cc6243b0ceef8a42a7644e3a15f4e
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/758955325212b2f561f09ca481967fb8c497e1ec Cr-Commit-Position: refs/heads/master@{#293919} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
