Audio: Separate E_ACCESSDENIED from WASAPI input initialization metrics [chromium/src : main]
Henrik Andreasson (Gerrit)
Henrik Andreasson removed Tomas Gunnarsson from this change![]( "Open in Gerrit")
- Tomas Gunnarsson
Related details
- Johannes Kron
- Olga Sharonova
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Henrik Andreasson (Gerrit)
Henrik Andreasson added 1 comment![]( "Open in Gerrit")
<histogram name="Media.Audio.Capture.Win.SystemPermissionDenied" enum="Boolean"NIT: Since `E_ACCESSDENIED` is no longer logged in `Media.Audio.Capture.Win.InitError` and `Media.Audio.Capture.Win.ProcessLoopbackInitError`, consider updating the summaries of those two histograms to mention that `E_ACCESSDENIED` is intentionally excluded and is tracked by this histogram instead.
Related details
- Johannes Kron
- Olga Sharonova
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Henrik Andreasson (Gerrit)
Henrik Andreasson added 1 comment![]( "Open in Gerrit")
<histogram name="Media.Audio.Capture.Win.SystemPermissionDenied" enum="Boolean"NIT: Since `E_ACCESSDENIED` is no longer logged in `Media.Audio.Capture.Win.InitError` and `Media.Audio.Capture.Win.ProcessLoopbackInitError`, consider updating the summaries of those two histograms to mention that `E_ACCESSDENIED` is intentionally excluded and is tracked by this histogram instead.
Done
Related details
- Johannes Kron
- Olga Sharonova
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tomas Gunnarsson (Gerrit)
Tomas Gunnarsson voted and added 2 comments![]( "Open in Gerrit")
Votes added by Tomas Gunnarsson
| Code-Review | +1 |
2 comments
hr == E_ACCESSDENIED);is there a way to know if IAudioClient::Initialize failed because of permission errors (e.g. privacy settings) or if it could be is because exclusive access is held by another application? Does chromium use `AUDCLNT_SHAREMODE_EXCLUSIVE` in some configurations? (just wondering if there might e.g. be a conflict if someone is using both canary and stable versions of chrome).
audio capture was blocked by system permissions. Logged once perit would be good to be 100% sure that it's related to system permissions or privacy settings and not a conflict due to exclusive mode.
Related details
- Henrik Andreasson
- Johannes Kron
- Olga Sharonova
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Henrik Andreasson (Gerrit)
Henrik Andreasson added 1 comment![]( "Open in Gerrit")
audio capture was blocked by system permissions. Logged once perit would be good to be 100% sure that it's related to system permissions or privacy settings and not a conflict due to exclusive mode.
We know that for sure. I have written a native command-line utility [1] which allows me to create a conflict where an external application has opened the capture side in exclusive mode to see the effect.
The error we then get is AUDCLNT_E_DEVICE_IN_USE (0x88890010) and not E_ACCESSDENIED (0x80070005).
I have only been able to provoke E_ACCESSDENIED when I have explicitly disabled microphone access in Settings.
[1] https://github.com/henrik-and/my-windows-samples/blob/main/exclusive-audio/README.md
Related details
- Johannes Kron
- Olga Sharonova
Henrik Andreasson (Gerrit)
Henrik Andreasson added 1 comment![]( "Open in Gerrit")
hr == E_ACCESSDENIED);is there a way to know if IAudioClient::Initialize failed because of permission errors (e.g. privacy settings) or if it could be is because exclusive access is held by another application? Does chromium use `AUDCLNT_SHAREMODE_EXCLUSIVE` in some configurations? (just wondering if there might e.g. be a conflict if someone is using both canary and stable versions of chrome).
We only support exclusive mode on the output side in Chrome, and it is behind a command-line flag (`switches::kEnableExclusiveAudio`). Hence, internal conflicts (Stable/Canary) should be very rare.
For potential conflicts with other applications, see my other comment in: https://chromium-review.git.corp.google.com/c/chromium/src/+/7852378/comment/2000bfd1_34ce2ada/ for more details.
Related details
- Johannes Kron
- Olga Sharonova
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Kron (Gerrit)
Johannes Kron added 1 comment![]( "Open in Gerrit")
base::UmaHistogramSparse("Media.Audio.Capture.Win.InitError", hr);
if (is_process_loopback_capture_) {
base::UmaHistogramSparse(
"Media.Audio.Capture.Win.ProcessLoopbackInitError", hr);
}Since the scope of these histograms is changing, their names must also be updated. The suggested naming pattern is to append "2" to the existing names [\[1\]](https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md#revising).
Related details
- Henrik Andreasson
- Olga Sharonova
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Henrik Andreasson (Gerrit)
Henrik Andreasson added 1 comment![]( "Open in Gerrit")
base::UmaHistogramSparse("Media.Audio.Capture.Win.InitError", hr);
if (is_process_loopback_capture_) {
base::UmaHistogramSparse(
"Media.Audio.Capture.Win.ProcessLoopbackInitError", hr);
}Since the scope of these histograms is changing, their names must also be updated. The suggested naming pattern is to append "2" to the existing names [\[1\]](https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md#revising).
It is still a representation of initializing errors (as before). The only change is that I break out one particular enum which is not a "true error" since it depends on a user setting and the outcome is known in advance.
IMO, this change is mainly a fix to ensure that the original UMA represents what it was intended for.
I do also break out this single enum into a new histogram with a unique name.
Just feels a bit odd to rename under these conditions.
WDYT?
Related details
- Johannes Kron
- Olga Sharonova
Henrik Andreasson (Gerrit)
Henrik Andreasson added 1 comment![]( "Open in Gerrit")
base::UmaHistogramSparse("Media.Audio.Capture.Win.InitError", hr);
if (is_process_loopback_capture_) {
base::UmaHistogramSparse(
"Media.Audio.Capture.Win.ProcessLoopbackInitError", hr);
}Henrik AndreassonSince the scope of these histograms is changing, their names must also be updated. The suggested naming pattern is to append "2" to the existing names [\[1\]](https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md#revising).
It is still a representation of initializing errors (as before). The only change is that I break out one particular enum which is not a "true error" since it depends on a user setting and the outcome is known in advance.
IMO, this change is mainly a fix to ensure that the original UMA represents what it was intended for.
I do also break out this single enum into a new histogram with a unique name.
Just feels a bit odd to rename under these conditions.
WDYT?
But yes, the new histogram will look very differently, I agree.
Just wanted to check if you feel strongly that I should add a new histogram.
Johannes Kron (Gerrit)
Johannes Kron added 1 comment![]( "Open in Gerrit")
base::UmaHistogramSparse("Media.Audio.Capture.Win.InitError", hr);
if (is_process_loopback_capture_) {
base::UmaHistogramSparse(
"Media.Audio.Capture.Win.ProcessLoopbackInitError", hr);
}Henrik AndreassonSince the scope of these histograms is changing, their names must also be updated. The suggested naming pattern is to append "2" to the existing names [\[1\]](https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md#revising).
Henrik AndreassonIt is still a representation of initializing errors (as before). The only change is that I break out one particular enum which is not a "true error" since it depends on a user setting and the outcome is known in advance.
IMO, this change is mainly a fix to ensure that the original UMA represents what it was intended for.
I do also break out this single enum into a new histogram with a unique name.
Just feels a bit odd to rename under these conditions.
WDYT?
But yes, the new histogram will look very differently, I agree.
Just wanted to check if you feel strongly that I should add a new histogram.
The reason to change the name to `Media.Audio.Capture.Win.InitError2` and `Media.Audio.Capture.Win.ProcessLoopbackInitError2` is simply to avoid comparing data between the two histograms and avoid misleading conclusions.
See https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md#revising for more info.
Related details
- Henrik Andreasson
- Olga Sharonova
Olga Sharonova (Gerrit)
Olga Sharonova added 1 comment![]( "Open in Gerrit")
base::UmaHistogramSparse("Media.Audio.Capture.Win.InitError", hr);
if (is_process_loopback_capture_) {
base::UmaHistogramSparse(
"Media.Audio.Capture.Win.ProcessLoopbackInitError", hr);
}Henrik AndreassonSince the scope of these histograms is changing, their names must also be updated. The suggested naming pattern is to append "2" to the existing names [\[1\]](https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md#revising).
Henrik AndreassonIt is still a representation of initializing errors (as before). The only change is that I break out one particular enum which is not a "true error" since it depends on a user setting and the outcome is known in advance.
IMO, this change is mainly a fix to ensure that the original UMA represents what it was intended for.
I do also break out this single enum into a new histogram with a unique name.
Just feels a bit odd to rename under these conditions.
WDYT?
Johannes KronBut yes, the new histogram will look very differently, I agree.
Just wanted to check if you feel strongly that I should add a new histogram.
The reason to change the name to `Media.Audio.Capture.Win.InitError2` and `Media.Audio.Capture.Win.ProcessLoopbackInitError2` is simply to avoid comparing data between the two histograms and avoid misleading conclusions.
See https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md#revising for more info.
+1 we need a new version of the histogram. And Henrik has another CL which calls for a version change, so we should probably do it once.
Related details
- Henrik Andreasson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Henrik Andreasson (Gerrit)
Henrik Andreasson added 2 comments![]( "Open in Gerrit")
base::UmaHistogramSparse("Media.Audio.Capture.Win.InitError", hr);
if (is_process_loopback_capture_) {
base::UmaHistogramSparse(
"Media.Audio.Capture.Win.ProcessLoopbackInitError", hr);
}Henrik AndreassonSince the scope of these histograms is changing, their names must also be updated. The suggested naming pattern is to append "2" to the existing names [\[1\]](https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md#revising).
Henrik AndreassonIt is still a representation of initializing errors (as before). The only change is that I break out one particular enum which is not a "true error" since it depends on a user setting and the outcome is known in advance.
IMO, this change is mainly a fix to ensure that the original UMA represents what it was intended for.
I do also break out this single enum into a new histogram with a unique name.
Just feels a bit odd to rename under these conditions.
WDYT?
Johannes KronBut yes, the new histogram will look very differently, I agree.
Just wanted to check if you feel strongly that I should add a new histogram.
Olga SharonovaThe reason to change the name to `Media.Audio.Capture.Win.InitError2` and `Media.Audio.Capture.Win.ProcessLoopbackInitError2` is simply to avoid comparing data between the two histograms and avoid misleading conclusions.
See https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md#revising for more info.
+1 we need a new version of the histogram. And Henrik has another CL which calls for a version change, so we should probably do it once.
- Johannes Kron
- Olga Sharonova
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Kron (Gerrit)
Johannes Kron voted and added 1 comment![]( "Open in Gerrit")
Votes added by Johannes Kron
| Code-Review | +1 |
1 comment
- Henrik Andreasson
- Olga Sharonova
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Henrik Andreasson (Gerrit)
Henrik Andreasson added 1 comment![]( "Open in Gerrit")
Waiting for final OK from olka@ as well just in case.
PTAL
Related details
- Olga Sharonova
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olga Sharonova (Gerrit)
Olga Sharonova voted and added 2 comments![]( "Open in Gerrit")
Votes added by Olga Sharonova
| Code-Review | +1 |
2 comments
<owner>ol...@chromium.org</owner>Please put yourself as the first contact 😊
<histogram name="Media.Audio.Capture.Win.SystemPermissionDenied" enum="Boolean"Should it be Media.Audio.Capture.Win.InitError.SystemPermissionDenied? (similarly, we already have Media.Audio.Capture.Win.InitError.FormatRelated)
Related details
- Henrik Andreasson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Henrik Andreasson (Gerrit)
Henrik Andreasson added 3 comments![]( "Open in Gerrit")
Please put yourself as the first contact 😊
Of course :-)
<histogram name="Media.Audio.Capture.Win.SystemPermissionDenied" enum="Boolean"Should it be Media.Audio.Capture.Win.InitError.SystemPermissionDenied? (similarly, we already have Media.Audio.Capture.Win.InitError.FormatRelated)
Yes, I agree. Good catch. Will update.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Henrik Andreasson (Gerrit)
Henrik Andreasson voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
7 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: media/audio/win/audio_low_latency_input_win.cc
Insertions: 3, Deletions: 2.
The diff is too large to show. Please review the diff.
```
```
The name of the file: tools/metrics/histograms/metadata/media/histograms.xml
Insertions: 14, Deletions: 13.
The diff is too large to show. Please review the diff.
```
Change information
Audio: Separate E_ACCESSDENIED from WASAPI input initialization metrics
This CL prevents expected OS-level microphone permission blocks
(E_ACCESSDENIED) from polluting the Windows audio input hardware failure
metrics.
Currently, E_ACCESSDENIED accounts for over 80% of the errors in
Media.Audio.Capture.Win.InitError. This noise masks genuine technical
regressions, such as crashed audio services or unsupported formats.
Specific changes in audio_low_latency_input_win.cc:
* Adds a new boolean histogram, Media.Audio.Capture.Win.SystemPermissionDenied,
to cleanly track the distribution of OS permission blocks across
all initialization attempts.
* Wraps the InitError and ProcessLoopbackInitError sparse histograms
in an `if (hr != E_ACCESSDENIED)` condition to filter out the noise.
* Leaves the actual return value and error propagation untouched,
ensuring no functional change for web clients.
OBSOLETE_HISTOGRAMS=Replaced by Media.Audio.Capture.Win.InitError2 and
Media.Audio.Capture.Win.ProcessLoopbackInitError2 because E_ACCESSDENIED
was filtered out.
- M media/audio/win/audio_low_latency_input_win.cc
- M tools/metrics/histograms/metadata/media/histograms.xml
Code-Review: +1 by Johannes Kron, +1 by Tomas Gunnarsson, +1 by Olga Sharonova
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |