Eliminate allow_unsafe_buffers in //media/audio/[alsa,android,pulse]. [chromium/src : main]
Weidong Liu (Gerrit)
Weidong Liu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Weidong Liu
| Commit-Queue | +1 |
1 comment
Related details
- Tomas Gunnarsson
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Weidong Liu (Gerrit)
Weidong Liu added 1 comment![]( "Open in Gerrit")
soft ping.
Related details
- Tomas Gunnarsson
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Weidong Liu (Gerrit)
Weidong Liu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Weidong Liu
| Commit-Queue | +1 |
1 comment
Tommi has been unresponsive for too long, add tguilbert@.
Hi, tguilbert@. PTAL. :)
Related details
- Thomas Guilbert
- Tomas Gunnarsson
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Guilbert (Gerrit)
Thomas Guilbert added 7 comments![]( "Open in Gerrit")
guessed_device = base::as_string_view(name);This is going to be converted into a string anyways. Use `base::ToString()` directly, here and throughout this CL.
virtual base::HeapArray<char, base::FreeDeleter> DeviceNameGetHint(NIT: It could be clearer to create a `using ScopedAlsaString = HeapArray...` alias (or any other name that makes sense), and use that instead of the `HeapArray<char, base::FreeDeleter`.
const char* ptr = snd_mixer_selem_get_name(elem);Is this leaking memory? The other `char*` are free'd. I couldn't find documentation on this. If you do, could you add a comment?
name.unique_id = base::as_string_view(unique_device_name);`base::ToString()` here and below.
base::byte_span_from_ref(*symbol.sl_iid)Can this be `base::as_byte_span()` directly? Or some other variation. Same question below.
auto it = std::ranges::find_if(ports, [](auto* port) {You might be able to simplify this with `std::ranges::none_of` or `std::ranges::any_of`
.first(base::checked_cast<size_t>(stream->channels_)));Is `values` ever bigger than `stream->channels_`? If not, omit the `.first()`.
Related details
- Tomas Gunnarsson
- Weidong Liu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Weidong Liu (Gerrit)
Weidong Liu voted and added 7 comments![]( "Open in Gerrit")
Votes added by Weidong Liu
| Commit-Queue | +1 |
7 comments
This is going to be converted into a string anyways. Use `base::ToString()` directly, here and throughout this CL.
Done
virtual base::HeapArray<char, base::FreeDeleter> DeviceNameGetHint(NIT: It could be clearer to create a `using ScopedAlsaString = HeapArray...` alias (or any other name that makes sense), and use that instead of the `HeapArray<char, base::FreeDeleter`.
Done
Is this leaking memory? The other `char*` are free'd. I couldn't find documentation on this. If you do, could you add a comment?
Done
name.unique_id = base::as_string_view(unique_device_name);`base::ToString()` here and below.
Done
base::byte_span_from_ref(*symbol.sl_iid)Can this be `base::as_byte_span()` directly? Or some other variation. Same question below.
`SLInterfaceID` is a complex structure. Here is its definition
```c
/** Interface ID defined as a UUID */
typedef const struct SLInterfaceID_ {
SLuint32 time_low;
SLuint16 time_mid;
SLuint16 time_hi_and_version;
SLuint16 clock_seq;
SLuint8 node[6];
} * SLInterfaceID;
```
I didn't find a better API than `base::byte_span_from_ref`.
auto it = std::ranges::find_if(ports, [](auto* port) {You might be able to simplify this with `std::ranges::none_of` or `std::ranges::any_of`
Done
.first(base::checked_cast<size_t>(stream->channels_)));Is `values` ever bigger than `stream->channels_`? If not, omit the `.first()`.
Yes. That's his definition.
```c
/** A structure encapsulating a per-channel volume */
typedef struct pa_cvolume {
uint8_t channels; /**< Number of channels */
pa_volume_t values[PA_CHANNELS_MAX]; /**< Per-channel volume */
} pa_cvolume;
```
Related details
- Thomas Guilbert
- Tomas Gunnarsson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Weidong Liu (Gerrit)
Weidong Liu added 2 comments![]( "Open in Gerrit")
guessed_device = base::as_string_view(name);Weidong LiuThis is going to be converted into a string anyways. Use `base::ToString()` directly, here and throughout this CL.
Done
Changing base::as_string_view to base::ToString caused the unit test to fail. I didn’t dig into the reason for this.
name.unique_id = base::as_string_view(unique_device_name);Weidong Liu`base::ToString()` here and below.
Done
Ditto.
Related details
- Thomas Guilbert
- Tomas Gunnarsson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Guilbert (Gerrit)
Thomas Guilbert added 8 comments![]( "Open in Gerrit")
Mostly nits, but there is still one comment that is substantive in `media/audio/pulse/audio_manager_pulse.cc`
guessed_device = base::as_string_view(name);Weidong LiuThis is going to be converted into a string anyways. Use `base::ToString()` directly, here and throughout this CL.
Weidong LiuDone
Changing base::as_string_view to base::ToString caused the unit test to fail. I didn’t dig into the reason for this.
My apologies. I think that `base::ToString()` produces `"[surround40]"` and such, for testing purposes. This LGTM.
using ScopedAlsaString = base::HeapArray<char, base::FreeDeleter>;NIT: Move `using` directive to the top of the class, per [style-guide](https://google.github.io/styleguide/cppguide.html#Declaration_Order).
name.unique_id = base::as_string_view(unique_device_name);Weidong Liu`base::ToString()` here and below.
Weidong LiuDone
Ditto.
Ack. See other comment.
const std::string_view unwanted_type =
UnwantedDeviceTypeWhenEnumerating(stream);NIT: move this out of the for-loop and while-loop since this never changes.
base::byte_span_from_ref(*symbol.sl_iid)Weidong LiuCan this be `base::as_byte_span()` directly? Or some other variation. Same question below.
`SLInterfaceID` is a complex structure. Here is its definition
```c
/** Interface ID defined as a UUID */
typedef const struct SLInterfaceID_ {
SLuint32 time_low;
SLuint16 time_mid;
SLuint16 time_hi_and_version;
SLuint16 clock_seq;
SLuint8 node[6];
} * SLInterfaceID;
```
I didn't find a better API than `base::byte_span_from_ref`.
Ah, thanks! I got confused by `byte_span_from_ref` with the pointer derefs. Your usage is correct.
bool all_ports_available = std::ranges::none_of(ports, [](auto* port) {I think this behavior is different. If any port is unavailable, we will skip the entire device now.
From reading the comment above the `if`, I think that this is the intended behavior:
```
bool no_ports_available = std::ranges::all_of(ports, [](auto* port) {
return port->available == PA_PORT_AVAILABLE_NO;
});
if (no_ports_available) {
return;
}
```
.first(base::checked_cast<size_t>(stream->channels_)));Weidong LiuIs `values` ever bigger than `stream->channels_`? If not, omit the `.first()`.
Yes. That's his definition.
```c
/** A structure encapsulating a per-channel volume */
typedef struct pa_cvolume {
uint8_t channels; /**< Number of channels */
pa_volume_t values[PA_CHANNELS_MAX]; /**< Per-channel volume */
} pa_cvolume;
```
This is very nitty, but could you use `.first(base::checked_cast<size_t>(info->volume.channels)))` here instead, so we're getting the right value from the source, in case the `stream->channels_ = info->channel_map.channels` line above is ever modified?
Related details
- Tomas Gunnarsson
- Weidong Liu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Weidong Liu (Gerrit)
Weidong Liu voted and added 4 comments![]( "Open in Gerrit")
Votes added by Weidong Liu
| Commit-Queue | +1 |
4 comments
using ScopedAlsaString = base::HeapArray<char, base::FreeDeleter>;NIT: Move `using` directive to the top of the class, per [style-guide](https://google.github.io/styleguide/cppguide.html#Declaration_Order).
Done
const std::string_view unwanted_type =
UnwantedDeviceTypeWhenEnumerating(stream);NIT: move this out of the for-loop and while-loop since this never changes.
Done
bool all_ports_available = std::ranges::none_of(ports, [](auto* port) {I think this behavior is different. If any port is unavailable, we will skip the entire device now.
From reading the comment above the `if`, I think that this is the intended behavior:
```
bool no_ports_available = std::ranges::all_of(ports, [](auto* port) {
return port->available == PA_PORT_AVAILABLE_NO;
});
if (no_ports_available) {
return;
}
```
Done. Oh. You are right. I misunderstood this.
.first(base::checked_cast<size_t>(stream->channels_)));Weidong LiuIs `values` ever bigger than `stream->channels_`? If not, omit the `.first()`.
Thomas GuilbertYes. That's his definition.
```c
/** A structure encapsulating a per-channel volume */
typedef struct pa_cvolume {
uint8_t channels; /**< Number of channels */
pa_volume_t values[PA_CHANNELS_MAX]; /**< Per-channel volume */
} pa_cvolume;
```
This is very nitty, but could you use `.first(base::checked_cast<size_t>(info->volume.channels)))` here instead, so we're getting the right value from the source, in case the `stream->channels_ = info->channel_map.channels` line above is ever modified?
- Thomas Guilbert
- Tomas Gunnarsson
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Guilbert (Gerrit)
Thomas Guilbert voted Code-Review+1![]( "Open in Gerrit")
- Tomas Gunnarsson
- Weidong Liu
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Guilbert (Gerrit)
Thomas Guilbert added 1 comment![]( "Open in Gerrit")
Weidong Liu (Gerrit)
Weidong Liu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Weidong Liu
| Commit-Queue | +2 |
1 comment
Thanks for your review.
Related details
- Tomas Gunnarsson
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Eliminate allow_unsafe_buffers in //media/audio/[alsa,android,pulse].
- M media/audio/alsa/alsa_output.cc
- M media/audio/alsa/alsa_output_unittest.cc
- M media/audio/alsa/alsa_util.cc
- M media/audio/alsa/alsa_wrapper.cc
- M media/audio/alsa/alsa_wrapper.h
- M media/audio/alsa/audio_manager_alsa.cc
- M media/audio/alsa/audio_manager_alsa.h
- M media/audio/alsa/mock_alsa_wrapper.h
- M media/audio/android/aaudio_stream_wrapper.cc
- M media/audio/android/opensles_input.cc
- M media/audio/android/opensles_input.h
- M media/audio/android/opensles_output.cc
- M media/audio/android/opensles_output.h
- M media/audio/android/opensles_wrapper.cc
- M media/audio/pulse/audio_manager_pulse.cc
- M media/audio/pulse/pulse_input.cc
- M media/audio/pulse/pulse_output.cc
- M media/audio/pulse/pulse_util.cc
Code-Review: +1 by Thomas Guilbert
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |