Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1736973002:
Add UMA stats for OS input glitches on Mac. (Closed)
|
Created:
4 years, 10 months ago by Henrik Grunell Modified:
4 years, 8 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, vanellope-cl_google.com Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA stats for OS input glitches on Mac.
BUG=585351
Committed: https://crrev.com/d321a41cfb7c9a0e5948120567fbd15cb18880c3
Cr-Commit-Position: refs/heads/master@{#378731}
Patch Set 1 #Patch Set 2 : Working code. #Patch Set 3 : Now really the working code. #Patch Set 4 : Fix. #
Total comments: 24
Patch Set 5 : Code review. #
Total comments: 12
Patch Set 6 : Code review. #
Total comments: 2
Patch Set 7 : Final code review fix. #Patch Set 8 : Fix. #Patch Set 9 : Rebase. #
Messages
Total messages: 36 (17 generated)
Description was changed from ========== Add UMA stats for OS input glitches on Mac. BUG=585351 ========== to ========== Add UMA stats for OS input glitches on Mac. BUG=585351 ==========
grunell@chromium.org changed reviewers: + dalecurtis@chromium.org, tommi@chromium.org
grunell@chromium.org changed reviewers: + mpearson@chromium.org
grunell@chromium.org changed reviewers: - dalecurtis@chromium.org
Tommi: All Mark: histograms.xml https://codereview.chromium.org/1736973002/diff/60001/media/audio/mac/audio_l... File media/audio/mac/audio_low_latency_input_mac.h (right): https://codereview.chromium.org/1736973002/diff/60001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.h:274: // These variables are only touched on the callback thread and then read They're also read in Stop(), same as on output side. This doesn't seem safe. Tommi: can you confirm since you wrote the original code for output side?
lgtm! It would be great if we could get these stats for 50. https://codereview.chromium.org/1736973002/diff/60001/media/audio/mac/audio_l... File media/audio/mac/audio_low_latency_input_mac.h (right): https://codereview.chromium.org/1736973002/diff/60001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.h:274: // These variables are only touched on the callback thread and then read On 2016/02/25 14:40:23, Henrik Grunell wrote: > They're also read in Stop(), same as on output side. This doesn't seem safe. > > Tommi: can you confirm since you wrote the original code for output side? It's safe as long as the callbacks aren't active when we read them in Stop(). Similarly when the callbacks are active, they must only be touched from the callback thread.
grunell@chromium.org changed reviewers: + isherman@chromium.org - mpearson@chromium.org
Switching histograms.xml reviewer since no reply. Ilya: please review.
The CQ bit was checked by grunell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1736973002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1736973002/60001
mpearson@chromium.org changed reviewers: + mpearson@chromium.org
https://codereview.chromium.org/1736973002/diff/60001/media/audio/mac/audio_l... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/1736973002/diff/60001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:1198: number_of_frames_requested_); Is the standard COUNTS histogram the right range? Up to a million frames requested. That sounds implausible to me. Maybe one of the other COUNTS histograms has a better size. https://codereview.chromium.org/1736973002/diff/60001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:1202: glitches_detected_, 0, 999999, 100); With these parameters, the histogram looks basically identical to the standard UMA_HISTOGRAM_COUNTS histogram. Sure, the maximum bucket is 999,999 instead of 1,000,000 but I know you can't care about that. Please use the standard counts histogram unless you have a reason not to. https://codereview.chromium.org/1736973002/diff/60001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:1206: UMA_HISTOGRAM_COUNTS("Media.Audio.Capture.LostFramesInMs", lost_frames_ms); Either use one of the standard UMA times histograms or explain in a comment here why you're instead using a COUNTS histogram. https://codereview.chromium.org/1736973002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1736973002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18950: + The number of frames audio is actually captured at if the number is Please proofread. I can't parse this sentence. I think you're missing a "that" or an "as" somewhere or maybe you have an unnecessary "at". I can't figure out which version of the sentence you intended. Also, "frames audio" sounds awkward. Try "audio frames" or "frames of audio" or something. https://codereview.chromium.org/1736973002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18951: + different from the desired number of frames. If the desired frame size is Please briefly explain what "desired frame size" is / when it is used. That probably will also help me interpret when the statement "this value will be 0" is a natural consequence of using setting a desired frame size or a foible of your logging. It may also help me understand whether it makes sense for you to emit to this histogram at all in this case. https://codereview.chromium.org/1736973002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18956: + logged when an audio input stream is stopped. Are pauses included or does this refer to (basically) closing the stream entirely? ditto below. https://codereview.chromium.org/1736973002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18963: + The number of glitches that were detected at the OS level while an audio Consider whether "at the OS level" in this sentence and "if the OS has" (replace with "we had") adds anything. It's not clear to me it does. (Does does it matter which level of the hierarchy detects a glitch? And if the OS is the only one that does, than why bother saying it happens at that level?) ditto below https://codereview.chromium.org/1736973002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18972: + The length in milliseconds of the largest glitch that was detected at the OS optional nit: largest glitch -> largest audio glitch
Oh, and have you tested this with about:histograms to see if the numbers and histogram ranges look reasonable? I ask because as you saw, I questioned your choice of histogram ranges. --mark
hopefully adding some clarity to two questions https://codereview.chromium.org/1736973002/diff/60001/media/audio/mac/audio_l... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/1736973002/diff/60001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:1198: number_of_frames_requested_); On 2016/02/26 17:10:55, Mark P wrote: > Is the standard COUNTS histogram the right range? Up to a million frames > requested. That sounds implausible to me. Maybe one of the other COUNTS > histograms has a better size. These are audio "frames". There will be tens of thousands of these per second, so million is actually reasonable (it's about 20 seconds if we get 48000 frames per second). https://codereview.chromium.org/1736973002/diff/60001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:1202: glitches_detected_, 0, 999999, 100); On 2016/02/26 17:10:55, Mark P wrote: > With these parameters, the histogram looks basically identical to the standard > UMA_HISTOGRAM_COUNTS histogram. Sure, the maximum bucket is 999,999 instead of > 1,000,000 but I know you can't care about that. Please use the standard counts > histogram unless you have a reason not to. > This doubles the bucket count though - or am I missing something?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1736973002/diff/60001/media/audio/mac/audio_l... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/1736973002/diff/60001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:1198: number_of_frames_requested_); On 2016/02/26 17:27:43, tommi-chromium wrote: > On 2016/02/26 17:10:55, Mark P wrote: > > Is the standard COUNTS histogram the right range? Up to a million frames > > requested. That sounds implausible to me. Maybe one of the other COUNTS > > histograms has a better size. > > These are audio "frames". There will be tens of thousands of these per second, > so million is actually reasonable (it's about 20 seconds if we get 48000 frames > per second). Ah, thanks for the explanation. This seems alright then. https://codereview.chromium.org/1736973002/diff/60001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:1202: glitches_detected_, 0, 999999, 100); On 2016/02/26 17:27:43, tommi-chromium wrote: > On 2016/02/26 17:10:55, Mark P wrote: > > With these parameters, the histogram looks basically identical to the standard > > UMA_HISTOGRAM_COUNTS histogram. Sure, the maximum bucket is 999,999 instead > of > > 1,000,000 but I know you can't care about that. Please use the standard > counts > > histogram unless you have a reason not to. > > > > This doubles the bucket count though - or am I missing something? Ah, I see, you're using twice as many buckets as the default macro. Do you have a reason you need more than the typical amount of precision?
grunell@chromium.org changed reviewers: - isherman@chromium.org
https://codereview.chromium.org/1736973002/diff/60001/media/audio/mac/audio_l... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/1736973002/diff/60001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:1198: number_of_frames_requested_); On 2016/02/26 18:37:19, Mark P wrote: > On 2016/02/26 17:27:43, tommi-chromium wrote: > > On 2016/02/26 17:10:55, Mark P wrote: > > > Is the standard COUNTS histogram the right range? Up to a million frames > > > requested. That sounds implausible to me. Maybe one of the other COUNTS > > > histograms has a better size. > > > > These are audio "frames". There will be tens of thousands of these per > second, > > so million is actually reasonable (it's about 20 seconds if we get 48000 > frames > > per second). > > Ah, thanks for the explanation. This seems alright then. Actually, this is number of frames requested per callback, i.e. typically below 1000. Changed to COUNTS_10000. https://codereview.chromium.org/1736973002/diff/60001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:1202: glitches_detected_, 0, 999999, 100); On 2016/02/26 18:37:19, Mark P wrote: > On 2016/02/26 17:27:43, tommi-chromium wrote: > > On 2016/02/26 17:10:55, Mark P wrote: > > > With these parameters, the histogram looks basically identical to the > standard > > > UMA_HISTOGRAM_COUNTS histogram. Sure, the maximum bucket is 999,999 instead > > of > > > 1,000,000 but I know you can't care about that. Please use the standard > > counts > > > histogram unless you have a reason not to. > > > > > > > This doubles the bucket count though - or am I missing something? > > Ah, I see, you're using twice as many buckets as the default macro. Do you have > a reason you need more than the typical amount of precision? Tommi, do you have the rationale for this? (Since this is based on the output side stats you wrote.) I'd say 50 should be enough. Changed it to 50 meanwhile. https://codereview.chromium.org/1736973002/diff/60001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:1206: UMA_HISTOGRAM_COUNTS("Media.Audio.Capture.LostFramesInMs", lost_frames_ms); On 2016/02/26 17:10:55, Mark P wrote: > Either use one of the standard UMA times histograms or explain in a comment here > why you're instead using a COUNTS histogram. Changed to LONG_TIMES and below to CUSTOM_TIMES with 1 minute as max. https://codereview.chromium.org/1736973002/diff/60001/media/audio/mac/audio_l... File media/audio/mac/audio_low_latency_input_mac.h (right): https://codereview.chromium.org/1736973002/diff/60001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.h:274: // These variables are only touched on the callback thread and then read On 2016/02/26 10:07:20, tommi-chromium wrote: > On 2016/02/25 14:40:23, Henrik Grunell wrote: > > They're also read in Stop(), same as on output side. This doesn't seem safe. > > > > Tommi: can you confirm since you wrote the original code for output side? > > It's safe as long as the callbacks aren't active when we read them in Stop(). > Similarly when the callbacks are active, they must only be touched from the > callback thread. Acknowledged. https://codereview.chromium.org/1736973002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1736973002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18947: +<histogram name="Media.Audio.Capture.FramesRequested" units="frames"> I changed "FramesRequested" to "FramesProvided", since that's what it actually is. https://codereview.chromium.org/1736973002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18950: + The number of frames audio is actually captured at if the number is On 2016/02/26 17:10:55, Mark P wrote: > Please proofread. I can't parse this sentence. I think you're missing a "that" > or an "as" somewhere or maybe you have an unnecessary "at". I can't figure out > which version of the sentence you intended. Also, "frames audio" sounds > awkward. Try "audio frames" or "frames of audio" or something. Done. https://codereview.chromium.org/1736973002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18951: + different from the desired number of frames. If the desired frame size is On 2016/02/26 17:10:56, Mark P wrote: > Please briefly explain what "desired frame size" is / when it is used. > That probably will also help me interpret when the statement "this value will be > 0" is a natural consequence of using setting a desired frame size or a foible of > your logging. It may also help me understand whether it makes sense for you to > emit to this histogram at all in this case. Done. https://codereview.chromium.org/1736973002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18956: + logged when an audio input stream is stopped. On 2016/02/26 17:10:56, Mark P wrote: > Are pauses included or does this refer to (basically) closing the stream > entirely? > ditto below. Closed. Changed. https://codereview.chromium.org/1736973002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18963: + The number of glitches that were detected at the OS level while an audio On 2016/02/26 17:10:55, Mark P wrote: > Consider whether "at the OS level" in this sentence and "if the OS has" (replace > with "we had") adds anything. It's not clear to me it does. > (Does does it matter which level of the hierarchy detects a glitch? And if the > OS is the only one that does, than why bother saying it happens at that level?) > > ditto below It matters - this is OS glitches only. https://codereview.chromium.org/1736973002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18972: + The length in milliseconds of the largest glitch that was detected at the OS On 2016/02/26 17:10:56, Mark P wrote: > optional nit: largest glitch -> largest audio glitch Done.
And again I ask: have you tested this with about:histograms to see if the numbers and histogram ranges look reasonable? --mark https://codereview.chromium.org/1736973002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1736973002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18952: + frames that we told the operating system about when opening the capture nit: omit "about" https://codereview.chromium.org/1736973002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18956: + will be 0. This histogram then shows how frequent it is and what number of I don't understand why this value will be 0 if the desired frame size is used. Earlier you say that we only record this histogram if the number if different than the desired number of frames. Which is it? https://codereview.chromium.org/1736973002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18956: + will be 0. This histogram then shows how frequent it is and what number of "frequent it is" I can't figure out what "it" refers to. Use of desired frame size? https://codereview.chromium.org/1736973002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18958: + is closed. This final sentence contradicts the implication in the first. Do you mean that every time you get frames, you record a local variable saying what you requested and what you got, and you accumulate that into a list, and then when the audio input stream is closed, you write out all those values to the histogram? I doubt that. Please clarify the description. https://codereview.chromium.org/1736973002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18976: + suffered glitches is stopped. Given that I asked about the stopped versus closed difference before, I assume this "stopped" is different than the earlier "closed". Can you please explain or correct? ditto below https://codereview.chromium.org/1736973002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18983: + The combined length in milliseconds of audio glitches. This is useful to Is this only for OS glitches like the others, or is this all types of glitches? If it's also OS glitches, you should say so. If it's not, you should make this histogram name different enough so it's no likely to be compared / confused with the others.
I hope things have been clarified now. As for checking chrome://histograms, I do have tested that, however glitches are very rare. But we have the same for output side, were this has been verified before. https://codereview.chromium.org/1736973002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1736973002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18952: + frames that we told the operating system about when opening the capture On 2016/02/29 23:41:37, Mark P wrote: > nit: omit "about" Done. https://codereview.chromium.org/1736973002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18956: + will be 0. This histogram then shows how frequent it is and what number of On 2016/02/29 23:41:37, Mark P wrote: > "frequent it is" > I can't figure out what "it" refers to. Use of desired frame size? Yes, updated. https://codereview.chromium.org/1736973002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18956: + will be 0. This histogram then shows how frequent it is and what number of On 2016/02/29 23:41:37, Mark P wrote: > I don't understand why this value will be 0 if the desired frame size is used. > Earlier you say that we only record this histogram if the number if different > than the desired number of frames. > Which is it? Right, it's always recorded. Rewrote a bit. https://codereview.chromium.org/1736973002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18958: + is closed. On 2016/02/29 23:41:37, Mark P wrote: > This final sentence contradicts the implication in the first. Do you mean that > every time you get frames, you record a local variable saying what you requested > and what you got, and you accumulate that into a list, and then when the audio > input stream is closed, you write out all those values to the histogram? I > doubt that. Please clarify the description. Clarified. https://codereview.chromium.org/1736973002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18976: + suffered glitches is stopped. On 2016/02/29 23:41:37, Mark P wrote: > Given that I asked about the stopped versus closed difference before, I assume > this "stopped" is different than the earlier "closed". Can you please explain > or correct? > ditto below Done. https://codereview.chromium.org/1736973002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18983: + The combined length in milliseconds of audio glitches. This is useful to On 2016/02/29 23:41:37, Mark P wrote: > Is this only for OS glitches like the others, or is this all types of glitches? > If it's also OS glitches, you should say so. > If it's not, you should make this histogram name different enough so it's no > likely to be compared / confused with the others. Only OS glitches. Done.
histograms.xml lgtm baring one minor comment below Thanks for iterating on this with me. I at least think the final result is cleaner than the original draft. (I hope you do too.) --makr https://codereview.chromium.org/1736973002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1736973002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:18954: + value will be 0. This histogram then shows how frequent it is that the nit for clarity: this value will -> the emitted value is special-cased to
Thanks for the thorough review Mark. I agree the final result is clearer than the original. https://codereview.chromium.org/1736973002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1736973002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:18954: + value will be 0. This histogram then shows how frequent it is that the On 2016/03/01 19:27:53, Mark P wrote: > nit for clarity: this value will -> the emitted value is special-cased to Done.
The CQ bit was checked by grunell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/1736973002/#ps120001 (title: "Final code review fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1736973002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1736973002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by grunell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/1736973002/#ps160001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1736973002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1736973002/160001
Message was sent while issue was closed.
Description was changed from ========== Add UMA stats for OS input glitches on Mac. BUG=585351 ========== to ========== Add UMA stats for OS input glitches on Mac. BUG=585351 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Add UMA stats for OS input glitches on Mac. BUG=585351 ========== to ========== Add UMA stats for OS input glitches on Mac. BUG=585351 Committed: https://crrev.com/d321a41cfb7c9a0e5948120567fbd15cb18880c3 Cr-Commit-Position: refs/heads/master@{#378731} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/d321a41cfb7c9a0e5948120567fbd15cb18880c3 Cr-Commit-Position: refs/heads/master@{#378731}
Message was sent while issue was closed.
Description was changed from ========== Add UMA stats for OS input glitches on Mac. BUG=585351 Committed: https://crrev.com/d321a41cfb7c9a0e5948120567fbd15cb18880c3 Cr-Commit-Position: refs/heads/master@{#378731} ========== to ========== Add UMA stats for OS input glitches on Mac. BUG=585351 Committed: https://crrev.com/d321a41cfb7c9a0e5948120567fbd15cb18880c3 Cr-Commit-Position: refs/heads/master@{#378731} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
