Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1750403004:
Adds Media.Audio.InputStartWasDeferredAudioWorkedMac UMA stat (Closed)
|
Created:
4 years, 9 months ago by henrika (OOO until Aug 14) Modified:
4 years, 9 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds Media.Audio.InputStartWasDeferredAudioWorkedMac UMA stat.
Goal is to measure the total number of times we delay calling start and not only when audio fails as is the case today.
Also adds Media.Audio.InputBufferSizeWasChangedAudioWorkedMac
BUG=549021
Committed: https://crrev.com/1873de96e562feb05156d92a749e382af361bcc0
Cr-Commit-Position: refs/heads/master@{#379281}
Patch Set 1 #Patch Set 2 : nit #Patch Set 3 : Also adds Media.Audio.InputBufferSizeWasChangedAudioWorkedMac #
Total comments: 8
Patch Set 4 : Final nits #Patch Set 5 : rebased #Messages
Total messages: 20 (10 generated)
Description was changed from ========== Adds Media.Audio.InputStartWasDeferredAudioWorkedMac UMA stat BUG= ========== to ========== Adds Media.Audio.InputStartWasDeferredAudioWorkedMac UMA stat. Goal is to measure the total number of times we delay calling start and not only when audio fails as is the case today. BUG=549021 ==========
henrika@chromium.org changed reviewers: + isherman@chromium.org, tommi@chromium.org
tommi@ and isherman@: one more please!
Description was changed from ========== Adds Media.Audio.InputStartWasDeferredAudioWorkedMac UMA stat. Goal is to measure the total number of times we delay calling start and not only when audio fails as is the case today. BUG=549021 ========== to ========== Adds Media.Audio.InputStartWasDeferredAudioWorkedMac UMA stat. Goal is to measure the total number of times we delay calling start and not only when audio fails as is the case today. Also adds Media.Audio.InputBufferSizeWasChangedAudioWorkedMac BUG=549021 ==========
Metrics LGTM % nits: https://codereview.chromium.org/1750403004/diff/40001/media/audio/mac/audio_l... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/1750403004/diff/40001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:547: // Log if call to Start() was deferred or not. To be compared with nit: s/Log if ... or not/Log whether ... https://codereview.chromium.org/1750403004/diff/40001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:549: // when input audio fail to start. nit: s/fail/fails https://codereview.chromium.org/1750403004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1750403004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19076: <histogram name="Media.Audio.InputStartWasDeferredMac" enum="BooleanSuccess"> Please use BooleanDeferred here too. https://codereview.chromium.org/1750403004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1750403004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19045: + Media.Audio.InputBufferSizeWasChangedMac which is only added when input These metric names are now rather confusing, since the general-sounding metric is only logged on failure. It might be nice to rename the existing metrics for clarity. (OTOH, renaming the metrics results in a discontinuity in terms of the data, which might be inconvenient enough to not be worth the trouble.)
lgtm
https://codereview.chromium.org/1750403004/diff/40001/media/audio/mac/audio_l... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/1750403004/diff/40001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:547: // Log if call to Start() was deferred or not. To be compared with On 2016/03/02 22:57:24, Ilya Sherman wrote: > nit: s/Log if ... or not/Log whether ... Done. https://codereview.chromium.org/1750403004/diff/40001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:549: // when input audio fail to start. On 2016/03/02 22:57:24, Ilya Sherman wrote: > nit: s/fail/fails Done. https://codereview.chromium.org/1750403004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1750403004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19076: <histogram name="Media.Audio.InputStartWasDeferredMac" enum="BooleanSuccess"> Will do but I was not sure how that would affect existing results. I guess that is safe, right? https://codereview.chromium.org/1750403004/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1750403004/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19045: + Media.Audio.InputBufferSizeWasChangedMac which is only added when input I realize that and I would have selected other names if I could start from scratch. I tried to provide very detailed comments to make it less confusing. Would like to stick with the existing names to avoid hiccups in the data. Hope that is OK.
The CQ bit was checked by henrika@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1750403004/#ps60001 (title: "Final nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1750403004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750403004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Failing tests on try bots does not seem to be related to this CL. They must be flaky. Will rebase and try one more time and land manually if needed.
The CQ bit was checked by henrika@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1750403004/#ps80001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1750403004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750403004/80001
Message was sent while issue was closed.
Description was changed from ========== Adds Media.Audio.InputStartWasDeferredAudioWorkedMac UMA stat. Goal is to measure the total number of times we delay calling start and not only when audio fails as is the case today. Also adds Media.Audio.InputBufferSizeWasChangedAudioWorkedMac BUG=549021 ========== to ========== Adds Media.Audio.InputStartWasDeferredAudioWorkedMac UMA stat. Goal is to measure the total number of times we delay calling start and not only when audio fails as is the case today. Also adds Media.Audio.InputBufferSizeWasChangedAudioWorkedMac BUG=549021 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Adds Media.Audio.InputStartWasDeferredAudioWorkedMac UMA stat. Goal is to measure the total number of times we delay calling start and not only when audio fails as is the case today. Also adds Media.Audio.InputBufferSizeWasChangedAudioWorkedMac BUG=549021 ========== to ========== Adds Media.Audio.InputStartWasDeferredAudioWorkedMac UMA stat. Goal is to measure the total number of times we delay calling start and not only when audio fails as is the case today. Also adds Media.Audio.InputBufferSizeWasChangedAudioWorkedMac BUG=549021 Committed: https://crrev.com/1873de96e562feb05156d92a749e382af361bcc0 Cr-Commit-Position: refs/heads/master@{#379281} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1873de96e562feb05156d92a749e382af361bcc0 Cr-Commit-Position: refs/heads/master@{#379281} |
