Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
51003005:
Add AudioOutputController trace events and UMA backed wedge detection. (Closed)
|
Created:
7 years, 1 month ago by DaleCurtis Modified:
7 years, 1 month ago CC:
chromium-reviews, feature-media-reviews_chromium.org, Ilya Sherman, jar (doing other things), asvitkine+watch_chromium.org, miu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd AudioOutputController trace events and UMA backed wedge detection.
More extraction of debug code from my local test build. This adds:
- Trace events for easy logging of AudioOutputController actions.
- A wedge detection UMA statistic for field monitoring.
BUG=160920
TEST=UMA stat is reported correctly when wedged and not.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233034
Patch Set 1 #
Total comments: 16
Patch Set 2 : Comments #Patch Set 3 : Use scoped_ptr. #
Messages
Total messages: 19 (0 generated)
I'm OOO this week... and may be slow to respond. +cc Alexei who might comment faster. https://codereview.chromium.org/51003005/diff/1/media/audio/audio_output_cont... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/51003005/diff/1/media/audio/audio_output_cont... media/audio/audio_output_controller.cc:475: "Media.AudioOutputControllerWedgeDetected", wedge_detected_); This seems to use a histogram as a counter. With nothing to normalize this count, what will it mean? Have you considered an event instead of a histogram? Have you considered gathering both == and != in the histogram? ...or is there a way to normalize this? .... or is "any error" and hence "any sample" in this histogram a hint that there is a problem (and that is what you are after)? https://codereview.chromium.org/51003005/diff/1/tools/metrics/histograms/hist... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/51003005/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:5839: + <summary>Whether a wedge was detected during audio playback or not.</summary> It is hard to tell if "success" or "failure" means had a wedge or not. You might choose a better custom enum, or choose an enum that is "true" vs "false" and then get rid of the confusing prose "...or not." that you used above.
jar -> cc https://codereview.chromium.org/51003005/diff/1/media/audio/audio_output_cont... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/51003005/diff/1/media/audio/audio_output_cont... media/audio/audio_output_controller.cc:475: "Media.AudioOutputControllerWedgeDetected", wedge_detected_); On 2013/11/04 07:03:43, jar wrote: > This seems to use a histogram as a counter. With nothing to normalize this > count, what will it mean? > > Have you considered an event instead of a histogram? > > Have you considered gathering both == and != in the histogram? > > ...or is there a way to normalize this? .... or is "any error" and hence "any > sample" in this histogram a hint that there is a problem (and that is what you > are after)? I'm confused with what you mean by == and !=. |wedge_detected_| may be true or false depending on if OnMoreIOData() ran. Ideally it'll be mostly false, but we know it will be sometimes true on OSX. Since we're logging both true and false, this should be normalized, no? https://codereview.chromium.org/51003005/diff/1/tools/metrics/histograms/hist... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/51003005/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:5839: + <summary>Whether a wedge was detected during audio playback or not.</summary> On 2013/11/04 07:03:43, jar wrote: > It is hard to tell if "success" or "failure" means had a wedge or not. > > You might choose a better custom enum, or choose an enum that is "true" vs > "false" and then get rid of the confusing prose "...or not." that you used > above. I'm always confused with BooleanSuccess when recording "bad" things. I could rename this something like "PlaybackStartupSuccess" which might make it clearer.
https://codereview.chromium.org/51003005/diff/1/media/audio/audio_output_cont... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/51003005/diff/1/media/audio/audio_output_cont... media/audio/audio_output_controller.cc:207: // it. Currently, all buffers are less than 42ms, so anything larger is fine. isn't the important # here how long it takes between starting the device and time until the first OnMoreDataIO() call, which I assume is independent of the buffer size? https://codereview.chromium.org/51003005/diff/1/media/audio/audio_output_cont... media/audio/audio_output_controller.cc:209: // Timer self-manages its lifetime and WedgeCheck() will only fire if the nit on comment accuracy: WedgeCheck() always runs, it just doesn't do anything if state != kPlaying https://codereview.chromium.org/51003005/diff/1/media/audio/audio_output_cont... media/audio/audio_output_controller.cc:215: wedge_detected_ = true; how about on_more_data_io_called_, since that's what you're actually checking for? (when I first read this code it caught me off guard since it reads as if we've immediately detected a wedge) https://codereview.chromium.org/51003005/diff/1/media/audio/audio_output_cont... media/audio/audio_output_controller.cc:344: if (wedge_detected_) you don't need this if statement https://codereview.chromium.org/51003005/diff/1/media/audio/audio_output_cont... media/audio/audio_output_controller.cc:345: wedge_detected_ = false; IIRC OnMoreIOData() runs on a different thread vs. the thread where you read this value in WedgeCheck()
https://codereview.chromium.org/51003005/diff/1/media/audio/audio_output_cont... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/51003005/diff/1/media/audio/audio_output_cont... media/audio/audio_output_controller.cc:207: // it. Currently, all buffers are less than 42ms, so anything larger is fine. On 2013/11/04 19:05:12, scherkus wrote: > isn't the important # here how long it takes between starting the device and > time until the first OnMoreDataIO() call, which I assume is independent of the > buffer size? Ah, yeah, you're right. The PlayTime UMA values show this code completes in < 1 sec for 99% of users. If I look at the OSX graph, that's not true until ~ 1.5 seconds. That's still not the same as the time until OnMoreIOData, but that gives a baseline at least. So it looks like a better value might be ~3 to 5 seconds. https://codereview.chromium.org/51003005/diff/1/media/audio/audio_output_cont... media/audio/audio_output_controller.cc:209: // Timer self-manages its lifetime and WedgeCheck() will only fire if the On 2013/11/04 19:05:12, scherkus wrote: > nit on comment accuracy: WedgeCheck() always runs, it just doesn't do anything > if state != kPlaying Done. https://codereview.chromium.org/51003005/diff/1/media/audio/audio_output_cont... media/audio/audio_output_controller.cc:215: wedge_detected_ = true; On 2013/11/04 19:05:12, scherkus wrote: > how about on_more_data_io_called_, since that's what you're actually checking > for? > > (when I first read this code it caught me off guard since it reads as if we've > immediately detected a wedge) Done. https://codereview.chromium.org/51003005/diff/1/media/audio/audio_output_cont... media/audio/audio_output_controller.cc:344: if (wedge_detected_) On 2013/11/04 19:05:12, scherkus wrote: > you don't need this if statement I was trying to avoid setting it if it's already set since another thread might be reading it. That said, the compiler might just optimize it out, so removed. https://codereview.chromium.org/51003005/diff/1/media/audio/audio_output_cont... media/audio/audio_output_controller.cc:345: wedge_detected_ = false; On 2013/11/04 19:05:12, scherkus wrote: > IIRC OnMoreIOData() runs on a different thread vs. the thread where you read > this value in WedgeCheck() Correct, hence the comment above. It seemed overkill to use an AtomicRefCount, but after some experimenting its not that bad. Fixed.
lgtm
https://codereview.chromium.org/51003005/diff/1/tools/metrics/histograms/hist... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/51003005/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:5839: + <summary>Whether a wedge was detected during audio playback or not.</summary> On 2013/11/04 18:57:32, DaleCurtis wrote: > On 2013/11/04 07:03:43, jar wrote: > > It is hard to tell if "success" or "failure" means had a wedge or not. > > > > You might choose a better custom enum, or choose an enum that is "true" vs > > "false" and then get rid of the confusing prose "...or not." that you used > > above. > > I'm always confused with BooleanSuccess when recording "bad" things. I could > rename this something like "PlaybackStartupSuccess" which might make it clearer. If BooleanSuccess is a bad description, please do switch to a different enum (adding it if necessary). In this description, could you clarify what a wedge is e.g. in parentheses after that word.
https://codereview.chromium.org/51003005/diff/1/tools/metrics/histograms/hist... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/51003005/diff/1/tools/metrics/histograms/hist... tools/metrics/histograms/histograms.xml:5839: + <summary>Whether a wedge was detected during audio playback or not.</summary> On 2013/11/04 22:20:24, Alexei Svitkine wrote: > On 2013/11/04 18:57:32, DaleCurtis wrote: > > On 2013/11/04 07:03:43, jar wrote: > > > It is hard to tell if "success" or "failure" means had a wedge or not. > > > > > > You might choose a better custom enum, or choose an enum that is "true" vs > > > "false" and then get rid of the confusing prose "...or not." that you used > > > above. > > > > I'm always confused with BooleanSuccess when recording "bad" things. I could > > rename this something like "PlaybackStartupSuccess" which might make it > clearer. > > If BooleanSuccess is a bad description, please do switch to a different enum > (adding it if necessary). > > In this description, could you clarify what a wedge is e.g. in parentheses after > that word. I've renamed it completely in the latest patchset. PTAL.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/51003005/80001
Ack, since AudioOutputController is ref-counted it may not always be destructed on the AudioThread. I've changed the code so that the OneShotTimer is constructed during play and destructed during StopStream() (pause/close). scherkus: PTAL.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/51003005/340001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/51003005/340001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/51003005/340001
Retried try job too often on win7_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/51003005/340001
Message was sent while issue was closed.
Change committed as 233034 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
