Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1695303002:
Adds Media.Audio.InputDevicePropertyChangedMac UMA stat on Mac (Closed)
|
Created:
4 years, 10 months ago by henrika (OOO until Aug 14) Modified:
4 years, 10 months ago CC:
chromium-reviews, feature-media-reviews_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.InputDevicePropertyChangedMac UMA stat on Mac
BUG=549021
Committed: https://crrev.com/da354f2d1eccb591387b9e13622fb89581b5f0c7
Cr-Commit-Position: refs/heads/master@{#377264}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Now uses device notifier until first callback or for max 5 seconds #Patch Set 3 : Modified when device notification is started and stopped #
Total comments: 16
Patch Set 4 : Feedback from olka@ #Patch Set 5 : nit #Patch Set 6 : nit #Patch Set 7 : Device notifier now alive for complete session #
Total comments: 15
Patch Set 8 : Feedback from Tommi #Patch Set 9 : My own XML parts #
Total comments: 2
Patch Set 10 : Removed lock #
Total comments: 12
Patch Set 11 : isherman@ #Patch Set 12 : nit #
Total comments: 8
Patch Set 13 : Final round #
Messages
Total messages: 44 (12 generated)
Description was changed from ========== Adds Media.Audio.InputDeviceIsHoggedMac UMA stat when input audio fails BUG= ========== to ========== Adds Media.Audio.InputDeviceIsHoggedMac UMA stat when input audio fails BUG=549021 ==========
henrika@chromium.org changed reviewers: + tommi@chromium.org
Tommi, hope you like this one ;-)
https://codereview.chromium.org/1695303002/diff/1/media/audio/mac/audio_low_l... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/1695303002/diff/1/media/audio/mac/audio_low_l... media/audio/mac/audio_low_latency_input_mac.cc:960: if (/*IsRunning() &&*/ GetAutomaticGainControl()) { unintentional?
FYI, Tommi, working on adding a notifier as well to keep track of additional states. Some things can't be checked on demand but are only "given to" us when they happen. Still, can be useful to know if certain things have happened before input audio fails. Stay tuned.
Description was changed from ========== Adds Media.Audio.InputDeviceIsHoggedMac UMA stat when input audio fails BUG=549021 ========== to ========== Adds Media.Audio.InputDevicePropertyChangedMac if input callback fails on Mac BUG=549021 ==========
Tommi, I am convinced that we can make some solutions more efficient here. Please check that you are OK with the main idea first. Thanks! Summary: I start listening for device changes for the selected input device at construction and kill the observer at first callback or after 5 seconds or when Stop is called. While active, I store all incoming property changes in a map (and counts them). When we upload UMA stat for "no input audio", I check the map and add extra stats for each stored change.
Tommi: PTAL ;-)
henrika@chromium.org changed reviewers: + olka@chromium.org
Adding olka@ for general feedback (as discussed).
General feedback, as discussed. The idea looks good to me, though keep in mind I have a limited knowledge in this area so far :), and please excuse my ignorance of threading context here. https://codereview.chromium.org/1695303002/diff/40001/media/audio/mac/audio_l... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/1695303002/diff/40001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:72: PROPERTY_IO_STOPPED_ABNORMALLY = 1, I would not assign values to any but the first one - just to avoid somebody following this style and assigning the same number multiple times. https://codereview.chromium.org/1695303002/diff/40001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:792: OSStatus AUAudioInputStream::OnDevicePropertyChanged( Can there be a race between this call and deregistering |this| as a listener in the destructor? https://codereview.chromium.org/1695303002/diff/40001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:808: // Listeners will be called when possibly many properties have changed. Can the same listener be called concurrently? https://codereview.chromium.org/1695303002/diff/40001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:816: case kAudioDevicePropertyDeviceHasChanged: I would say that having both 'switch' and map is a bit of overkill, and having switch in two places makes it a bit difficult to maintain without introducing bugs. I would either store counters in an array by |PROPERTY_..| index and update them here in a 'switch', or store everything in a map without using 'switch' here, and select what I'm interested in during AddDevicePropertyChangesToUMA(). https://codereview.chromium.org/1695303002/diff/40001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:861: OSStatus result = AudioObjectRemovePropertyListener( Is it safe to call it second time if the first attempt failed? https://codereview.chromium.org/1695303002/diff/40001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:1086: while (!device_property_changes_map_.empty()) { You could just iterate through the map from the beginning to the end and then empty it after, it would be more efficient.
Thanks for many valuable comments Olga ;-) PTAL https://codereview.chromium.org/1695303002/diff/40001/media/audio/mac/audio_l... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/1695303002/diff/40001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:72: PROPERTY_IO_STOPPED_ABNORMALLY = 1, This style is used all over Chrome in combination with UMA histograms. I have been advices to use it before. See e.g. https://code.google.com/p/chromium/codesearch#chromium/src/ui/gl/gl_surface_e... or search for UMA_HISTOGRAM_ENUMERATION and you will find many examples. https://codereview.chromium.org/1695303002/diff/40001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:792: OSStatus AUAudioInputStream::OnDevicePropertyChanged( The call in the destructor is invalid (not needed) since we dereg in Close and Close is responsible for the destruction. Will add a DCHECK just in case. https://codereview.chromium.org/1695303002/diff/40001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:808: // Listeners will be called when possibly many properties have changed. The documentation does not say anything about that actually. Did some tests and it seems like different types of callbacks can come from different threads (as if some sort of thread pool was used). However, these callbacks simply must be serialized somehow by the system and I have not found any example that uses locks to make this method reentrant. https://codereview.chromium.org/1695303002/diff/40001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:816: case kAudioDevicePropertyDeviceHasChanged: Great comment, thanks. Will add all to the map and filter at a later stage. https://codereview.chromium.org/1695303002/diff/40001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:861: OSStatus result = AudioObjectRemovePropertyListener( I don't know since I've never been able to make the first call fail. Feels unsafe to do so. Agree? https://codereview.chromium.org/1695303002/diff/40001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:1086: while (!device_property_changes_map_.empty()) { Thanks. Will change.
https://codereview.chromium.org/1695303002/diff/40001/media/audio/mac/audio_l... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/1695303002/diff/40001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:72: PROPERTY_IO_STOPPED_ABNORMALLY = 1, On 2016/02/18 15:58:03, henrika wrote: > This style is used all over Chrome in combination with UMA histograms. I have > been advices to use it before. See e.g. > https://code.google.com/p/chromium/codesearch#chromium/src/ui/gl/gl_surface_e... > or search for UMA_HISTOGRAM_ENUMERATION and you will find many examples. Thank you for the info, will save me some time in the future reviews :) https://codereview.chromium.org/1695303002/diff/40001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:861: OSStatus result = AudioObjectRemovePropertyListener( On 2016/02/18 15:58:03, henrika wrote: > I don't know since I've never been able to make the first call fail. Feels > unsafe to do so. Agree? Yes, I have the same feeling. In this case, it should be SetDeviceListenerIsActive(false) before the call, I believe
https://codereview.chromium.org/1695303002/diff/40001/media/audio/mac/audio_l... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/1695303002/diff/40001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:72: PROPERTY_IO_STOPPED_ABNORMALLY = 1, On 2016/02/18 16:54:30, o1ka wrote: > On 2016/02/18 15:58:03, henrika wrote: > > This style is used all over Chrome in combination with UMA histograms. I have > > been advices to use it before. See e.g. > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/gl/gl_surface_e... > > or search for UMA_HISTOGRAM_ENUMERATION and you will find many examples. > > Thank you for the info, will save me some time in the future reviews :) Acknowledged. https://codereview.chromium.org/1695303002/diff/40001/media/audio/mac/audio_l... media/audio/mac/audio_low_latency_input_mac.cc:861: OSStatus result = AudioObjectRemovePropertyListener( On 2016/02/18 16:54:30, o1ka wrote: > On 2016/02/18 15:58:03, henrika wrote: > > I don't know since I've never been able to make the first call fail. Feels > > unsafe to do so. Agree? > > Yes, I have the same feeling. In this case, it should be > SetDeviceListenerIsActive(false) before the call, I believe Done.
Discussed with Tommi, will make some additional changes to keep the notifier alive during the complete call. We must learn more about what can actually happen while a stream is active.
Tommi, we now create two different UMA stats: (1) Media.Audio.InputDevicePropertyChangedStartupFailedMac and (2) Media.Audio.InputDevicePropertyChangedMac Both filter out stats from a map of device-property changes. (1) is only created when input audio start fails and (2) is always created at Close(). PTAL (will add XML when have OK from you)
https://codereview.chromium.org/1695303002/diff/120001/media/audio/mac/audio_... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/1695303002/diff/120001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:99: DCHECK_LE(result, PROPERTY_MAX); is the dcheck necessary if all the possible values are legal? https://codereview.chromium.org/1695303002/diff/120001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:830: device_property_changes_map_[property]++; ++foo https://codereview.chromium.org/1695303002/diff/120001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:830: device_property_changes_map_[property]++; It looks like we may not be on the same thread as thread_checker_ represents. How is it guaranteed that we don't have a race? https://codereview.chromium.org/1695303002/diff/120001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:836: DCHECK(thread_checker_.CalledOnValidThread()); also DCHECK(!device_listener_is_active_); https://codereview.chromium.org/1695303002/diff/120001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:1140: device_property_changes_map_.clear(); chance of a race? https://codereview.chromium.org/1695303002/diff/120001/media/audio/mac/audio_... File media/audio/mac/audio_low_latency_input_mac.h (right): https://codereview.chromium.org/1695303002/diff/120001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.h:244: // number of times we have been notified about a change in such a type. can you add a note about what thread is allowed to touch it? https://codereview.chromium.org/1695303002/diff/120001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.h:248: bool device_listener_is_active_; modified/checked on what threads?
Thanks Tommi, PTAL ;-) https://codereview.chromium.org/1695303002/diff/120001/media/audio/mac/audio_... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/1695303002/diff/120001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:99: DCHECK_LE(result, PROPERTY_MAX); Actually no. Removed. https://codereview.chromium.org/1695303002/diff/120001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:830: device_property_changes_map_[property]++; See earlier comments. A lock has now been added. https://codereview.chromium.org/1695303002/diff/120001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:830: device_property_changes_map_[property]++; On 2016/02/19 15:17:33, tommi-chromium wrote: > ++foo Acknowledged. https://codereview.chromium.org/1695303002/diff/120001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:836: DCHECK(thread_checker_.CalledOnValidThread()); On 2016/02/19 15:17:33, tommi-chromium wrote: > also DCHECK(!device_listener_is_active_); Done. https://codereview.chromium.org/1695303002/diff/120001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:1140: device_property_changes_map_.clear(); Not any longer ;-) https://codereview.chromium.org/1695303002/diff/120001/media/audio/mac/audio_... File media/audio/mac/audio_low_latency_input_mac.h (right): https://codereview.chromium.org/1695303002/diff/120001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.h:244: // number of times we have been notified about a change in such a type. Tommi, FYI, each callback can come from "any" thread it seems like. My guess is that the inner implementation uses a serial dispatch queue ("Serial queues (also known as private dispatch queues) execute one task at a time in the order in which they are added to the queue. The currently executing task runs on a distinct thread (which can vary from task to task) that is managed by the dispatch queue. Serial queues are often used to synchronize access to a specific resource.". There is no documentation that states it but I have never found any examples from Apple that uses locks in these callbacks. But, hhm., I do read it from the creating thread while active. Hence, will add a lock. https://codereview.chromium.org/1695303002/diff/120001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.h:248: bool device_listener_is_active_; Done, also uses DCHECK to ensure proper calling.
henrika@chromium.org changed reviewers: + isherman@chromium.org
Adding ischerman@ for XML. FYI; I got a warning when I tried to upload XML part but pretty_print.py did *not* make changes to my parts. If I said OK to the changes, I still see these errors when I upload (again, not related to my changes). Running presubmit upload checks ... INFO:root:Loading histograms.xml... INFO:root:histograms.xml is correctly pretty-printed. ERROR:root:Duplicate histogram definition SiteEngagementService.ScoreDecayedFrom ERROR:root:Duplicate histogram definition SiteEngagementService.ScoreDecayedTo ERROR:root:Error parsing /Users/henrika/src/chromium/src/tools/metrics/histograms/histograms.xml I therefore, excluded the changes suggested by pretty_print and uploaded with --bypass-hooks. Seems like either the XML file or the script is broken and I don't know what to do next. Hope my changes can land anyhow.
https://codereview.chromium.org/1695303002/diff/120001/media/audio/mac/audio_... File media/audio/mac/audio_low_latency_input_mac.h (right): https://codereview.chromium.org/1695303002/diff/120001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.h:244: // number of times we have been notified about a change in such a type. On 2016/02/22 12:32:52, henrika wrote: > Tommi, FYI, each callback can come from "any" thread it seems like. My guess is > that the inner implementation uses a serial dispatch queue ("Serial queues (also > known as private dispatch queues) execute one task at a time in the order in > which they are added to the queue. The currently executing task runs on a > distinct thread (which can vary from task to task) that is managed by the > dispatch queue. Serial queues are often used to synchronize access to a specific > resource.". There is no documentation that states it but I have never found any > examples from Apple that uses locks in these callbacks. > > But, hhm., I do read it from the creating thread while active. Hence, will add a > lock. If we don't need a lock, I'd prefer to not have a lock. My question is more about us understanding and documenting why it's safe to not use a lock. https://codereview.chromium.org/1695303002/diff/160001/media/audio/mac/audio_... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/1695303002/diff/160001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:1140: device_property_changes_map_.clear(); if it turns out we really need the lock, what about doing something like this: std::map<UInt32, int> device_property_changes; { base::AutoLock auto_lock(lock_); device_property_changes.swap(device_property_changes_map_); } Use device_property_changes without a lock and then also don't need to call clear().
Was able to remove the lock by always ensuring that the listener is disabled before reading the map. https://codereview.chromium.org/1695303002/diff/160001/media/audio/mac/audio_... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/1695303002/diff/160001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:1140: device_property_changes_map_.clear(); Thanks for the idea. Let me see if I can remove the lock by ensuring that the notifier is always disabled when we read it.
ischerman@: if you are OK with the XML parts..many thanks for pushing the button to save some time ;-)
On 2016/02/22 13:02:29, henrika wrote: > Adding ischerman@ for XML. > > FYI; I got a warning when I tried to upload XML part but pretty_print.py did > *not* make changes to my parts. > If I said OK to the changes, I still see these errors when I upload (again, not > related to my changes). > > Running presubmit upload checks ... > INFO:root:Loading histograms.xml... > INFO:root:histograms.xml is correctly pretty-printed. > ERROR:root:Duplicate histogram definition SiteEngagementService.ScoreDecayedFrom > ERROR:root:Duplicate histogram definition SiteEngagementService.ScoreDecayedTo > ERROR:root:Error parsing > /Users/henrika/src/chromium/src/tools/metrics/histograms/histograms.xml > > I therefore, excluded the changes suggested by pretty_print and uploaded with > --bypass-hooks. > > Seems like either the XML file or the script is broken and I don't know what to > do next. Hope my changes can land anyhow. The XML file indeed was corrupted by a bad CQ commit. I think if you sync your branch, you should get back to a good state. https://codereview.chromium.org/1695303002/diff/180001/media/audio/mac/audio_... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/1695303002/diff/180001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:97: UMA_HISTOGRAM_ENUMERATION(name, result, PROPERTY_MAX + 1); The name parameter of the UMA_HISTOGRAM_* macros must be a runtime constant, because the macros internally use static variables (for efficiency). https://codereview.chromium.org/1695303002/diff/180001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:1132: uma_result = PROPERTY_NOT_SPECIFIED; nit: Maybe name this "PROPERTY_OTHER" or something like that? I'm not sure what "not specified" means, at least. https://codereview.chromium.org/1695303002/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1695303002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:18932: + session. Sampled when an AUAudioInputStream object is closed. Please make extra clear that multiple enum values can be emitted each time an AUAudioInputStream object is closed. "Sampled" might suggest that a random property, or perhaps the most relevant property, is chosen among all that apply. https://codereview.chromium.org/1695303002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:58250: + <int value="12" label="Not specified"/> I'd recommend listing this case first, so that it's not buried in the middle if you add more cases later. (It's the default case, right?)
Thanks isherman@. Adding comments only since I don't have access to the source right now. Will rebase and take your feedback into account tomorrow. https://codereview.chromium.org/1695303002/diff/180001/media/audio/mac/audio_... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/1695303002/diff/180001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:97: UMA_HISTOGRAM_ENUMERATION(name, result, PROPERTY_MAX + 1); OK, will create one method for each histogram then. https://codereview.chromium.org/1695303002/diff/180001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:1132: uma_result = PROPERTY_NOT_SPECIFIED; Will rename to OTHER. https://codereview.chromium.org/1695303002/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1695303002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:18932: + session. Sampled when an AUAudioInputStream object is closed. Will change and make that more clear. https://codereview.chromium.org/1695303002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:58250: + <int value="12" label="Not specified"/> It covers a set of property changes that can happen but that we don't log since they are not related to the problem we are tracking. I see this as the "default" in a switch/case statement hence it felt natural to use as last value.
https://codereview.chromium.org/1695303002/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1695303002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:58250: + <int value="12" label="Not specified"/> On 2016/02/22 18:51:58, henrika wrote: > It covers a set of property changes that can happen but that we don't log since > they are not related to the problem we are tracking. I see this as the "default" > in a switch/case statement hence it felt natural to use as last value. Sorry, let me be clearer: Currently, you've assigned the label "other" to the backing value of "12". That makes sense while you have 12 known device properties. What happens if you add a 13th known device property? You can't renumber "other" to now have value 13, because the semantics of enumerated values used to back histograms are expected to remain consistent over time. So, you'd have to add this property after the "other" property... which would leave "other" strangely buried in the middle. That's why I'm suggesting moving it up to the front to begin with: It's a form of future-proofing =)
https://codereview.chromium.org/1695303002/diff/180001/media/audio/mac/audio_... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/1695303002/diff/180001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:1132: uma_result = PROPERTY_NOT_SPECIFIED; On 2016/02/22 18:51:58, henrika wrote: > Will rename to OTHER. Done. https://codereview.chromium.org/1695303002/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1695303002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:18932: + session. Sampled when an AUAudioInputStream object is closed. On 2016/02/22 18:51:58, henrika wrote: > Will change and make that more clear. Done. https://codereview.chromium.org/1695303002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:58250: + <int value="12" label="Not specified"/> Thanks for the detailed description. See your point now ;-)
PTAL. pretty_print.py now returns OK as well.
Description was changed from ========== Adds Media.Audio.InputDevicePropertyChangedMac if input callback fails on Mac BUG=549021 ========== to ========== Adds Media.Audio.InputDevicePropertyChangedMac UMA stat on Mac BUG=549021 ==========
https://codereview.chromium.org/1695303002/diff/220001/media/audio/mac/audio_... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/1695303002/diff/220001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:820: OSStatus AUAudioInputStream::DevicePropertyChanged( can we add a thread checker that we detach before starting to receive notifications and in this function DCHECK that we're always getting these on the same thread? (assuming that's true?) https://codereview.chromium.org/1695303002/diff/220001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:852: device_listener_is_active_ = true; should this be: device_listener_is_active_ = (result == noErr); https://codereview.chromium.org/1695303002/diff/220001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:1077: void AUAudioInputStream::AddDevicePropertyChangesToUMA(bool startup_failed) { dcheck that monitoring is not active?
Metrics LGTM % a final nit: https://codereview.chromium.org/1695303002/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1695303002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:18941: + all these values are uploaded when an AUAudioInputStream object is closed, nit: s/uploaded/recorded
Thanks for all comments. Landing. https://codereview.chromium.org/1695303002/diff/220001/media/audio/mac/audio_... File media/audio/mac/audio_low_latency_input_mac.cc (right): https://codereview.chromium.org/1695303002/diff/220001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:820: OSStatus AUAudioInputStream::DevicePropertyChanged( Thanks but I can't since it will not happen on the same thread every time. See https://codereview.chromium.org/1695303002/diff/120001/media/audio/mac/audio_... for more details. https://codereview.chromium.org/1695303002/diff/220001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:852: device_listener_is_active_ = true; On 2016/02/23 16:24:59, tommi-chromium wrote: > should this be: > > device_listener_is_active_ = (result == noErr); Done. https://codereview.chromium.org/1695303002/diff/220001/media/audio/mac/audio_... media/audio/mac/audio_low_latency_input_mac.cc:1077: void AUAudioInputStream::AddDevicePropertyChangesToUMA(bool startup_failed) { Good point. Thanks. https://codereview.chromium.org/1695303002/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1695303002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:18941: + all these values are uploaded when an AUAudioInputStream object is closed, On 2016/02/23 18:45:52, Ilya Sherman wrote: > nit: s/uploaded/recorded Done.
The CQ bit was checked by henrika@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1695303002/#ps240001 (title: "Final round")
The CQ bit was unchecked by henrika@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1695303002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1695303002/240001
Sorry, pushed the buttons to early. Still need OK from Tommi ;-)
lgtm
The CQ bit was checked by henrika@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1695303002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1695303002/240001
Message was sent while issue was closed.
Description was changed from ========== Adds Media.Audio.InputDevicePropertyChangedMac UMA stat on Mac BUG=549021 ========== to ========== Adds Media.Audio.InputDevicePropertyChangedMac UMA stat on Mac BUG=549021 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Adds Media.Audio.InputDevicePropertyChangedMac UMA stat on Mac BUG=549021 ========== to ========== Adds Media.Audio.InputDevicePropertyChangedMac UMA stat on Mac BUG=549021 Committed: https://crrev.com/da354f2d1eccb591387b9e13622fb89581b5f0c7 Cr-Commit-Position: refs/heads/master@{#377264} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/da354f2d1eccb591387b9e13622fb89581b5f0c7 Cr-Commit-Position: refs/heads/master@{#377264} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
