Audio: Standardize WASAPI output errors to HRESULT for UMA [chromium/src : main]
Henrik Andreasson (Gerrit)
Henrik Andreasson added 2 comments![]( "Open in Gerrit")
return CreateClientInternal(device.Get(), hr_out);If `CreateDevice` fails, it populates `hr_out` with the exact failure reason (e.g. `E_INVALIDARG`). However, `device.Get()` evaluates to `nullptr`.
The subsequent call to `CreateClientInternal(nullptr, hr_out)` will then overwrite the `hr_out` with `E_POINTER`, discarding the actual `CreateDevice` error. This results in the UMA logs being polluted by `E_POINTER` errors instead of the specific actionable COM code.
To fix this, add an early return:
```cpp
ComPtr<IMMDevice> device(CreateDevice(device_id, data_flow, role, hr_out));
if (!device) {
return ComPtr<IAudioClient>();
}
return CreateClientInternal(device.Get(), hr_out);
```
return CreateClientInternal3(device.Get(), hr_out);Similar to `CreateClient`, calling `CreateClientInternal3(nullptr, hr_out)` when `CreateDevice` fails will overwrite the actual `hr_out` with `E_POINTER`. You should add an early return here as well.
Related details
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 added 3 comments![]( "Open in Gerrit")
We may need to reversion UMA histograms, otherwise it will remain polluted for a long time.
HRESULT hr = S_FALSE;Declare locally where used?
HRESULT* hr_out = nullptr);Do we have to have it as a pointer defaulted to nullptr, rather than a reference?
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")
HRESULT hr = S_FALSE;Henrik AndreassonDeclare locally where used?
Done. Removed the top-level function-scoped declaration of hr and declared
it locally in the blocks where it is used.
Do we have to have it as a pointer defaulted to nullptr, rather than a reference?
I felt it was better to use a pointer defaulted to nullptr primarily to keep the out-parameter optional. That way we avoid forcing all users follow my changes.
Additionally (my personal preference) using a pointer requires callers to explicitly pass &hr, which makes it visually clear at the call site that the variable is going to be mutated.
So, No it is not *required* but nice to have imo.
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. |
Henrik Andreasson (Gerrit)
Henrik Andreasson added 1 comment![]( "Open in Gerrit")
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 added 2 comments![]( "Open in Gerrit")
We may need to reversion UMA histograms, otherwise it will remain polluted for a long time.
Still holds.
HRESULT* hr_out = nullptr);Henrik AndreassonDo we have to have it as a pointer defaulted to nullptr, rather than a reference?
I felt it was better to use a pointer defaulted to nullptr primarily to keep the out-parameter optional. That way we avoid forcing all users follow my changes.
Additionally (my personal preference) using a pointer requires callers to explicitly pass &hr, which makes it visually clear at the call site that the variable is going to be mutated.
So, No it is not *required* but nice to have imo.
https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs
Are there many users we are concerned with? (Also: we can overload a function for those)
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")
Olga SharonovaWe may need to reversion UMA histograms, otherwise it will remain polluted for a long time.
Still holds.
Missed this. Now fixed.
HRESULT* hr_out = nullptr);Henrik AndreassonDo we have to have it as a pointer defaulted to nullptr, rather than a reference?
Olga SharonovaI felt it was better to use a pointer defaulted to nullptr primarily to keep the out-parameter optional. That way we avoid forcing all users follow my changes.
Additionally (my personal preference) using a pointer requires callers to explicitly pass &hr, which makes it visually clear at the call site that the variable is going to be mutated.
So, No it is not *required* but nice to have imo.
https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs
Are there many users we are concerned with? (Also: we can overload a function for those)
A summary of what broke is given in https://paste.googleplex.com/5385095356743680. I did not list all details but instead implemented the overloads as you suggested.
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. |
Johannes Kron (Gerrit)
Johannes Kron voted Code-Review+1![]( "Open in Gerrit")
- 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. |