Spanify WebAudioSourceProvider::ProvideInput audio_data parameter [chromium/src : main]
kelsen liu (Gerrit)
kelsen liu added 1 comment![]( "Open in Gerrit")
Hi Kent, please take a look, thank you!
Related details
- Kent Tamura
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kent Tamura (Gerrit)
Kent Tamura voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kent Tamura
| Code-Review | +1 |
1 comment
LGTM. Please ask a webaudio/OWNERS for review too.
Related details
- Hongchan Choi
- kelsen liu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
kelsen liu (Gerrit)
kelsen liu added 1 comment![]( "Open in Gerrit")
LGTM. Please ask a webaudio/OWNERS for review too.
Thanks for the review!
Related details
- Hongchan Choi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongchan Choi (Gerrit)
Hongchan Choi added 5 comments![]( "Open in Gerrit")
Thanks for cleaning these up!
Generally looks good, but allocating something new in the audio hot path is not recommended so I made some suggestions.
Also adding mjwilson@ since he was looking into spanification of audio code.
std::unique_ptr<WebAudioSourceProvider> web_audio_source_provider_;```suggestion
std::unique_ptr<WebAudioSourceProvider> web_audio_source_provider_;
// Re-allocate only when the channel count changes to avoid heap
// allocations on the real-time audio thread.
std::vector<base::span<float>> web_audio_data_;
```
std::vector<base::span<float>> web_audio_data(static_cast<size_t>(n));```suggestion
if (web_audio_data_.size() != n) {
web_audio_data_.resize(static_cast<size_t>(n));
}
```
web_audio_data[i] = bus->Channel(i)->MutableSpan().first(```suggestion
web_audio_data_[i] = bus->Channel(i)->MutableSpan().first(
```
web_audio_source_provider_->ProvideInput(web_audio_data, frames_to_process);```suggestion
web_audio_source_provider_->ProvideInput(web_audio_data_, frames_to_process);
```
Related details
- Michael Wilson
- kelsen liu
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. |
Michael Wilson (Gerrit)
Michael Wilson added 1 comment![]( "Open in Gerrit")
Thank you for this work! I agree with the performance comments Hongchan added; no further requests from me. The span methods in AudioChannel will help my current work, so looking forward to this patch landing.
Related details
- kelsen liu
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. |
kelsen liu (Gerrit)
kelsen liu added 5 comments![]( "Open in Gerrit")
Thank you for this work! I agree with the performance comments Hongchan added; no further requests from me. The span methods in AudioChannel will help my current work, so looking forward to this patch landing.
Thanks for the review!
Thanks for cleaning these up!
Generally looks good, but allocating something new in the audio hot path is not recommended so I made some suggestions.
Also adding mjwilson@ since he was looking into spanification of audio code.
Thanks for the review.
The reasons I initially removed the web_audio_data_ member variable were:
If we keep it as a class member, the linter requires it to be defined as `std::vector<base::raw_span<float>>`. However, `std::vector<base::raw_span<float>>` and `std::vector<base::span<float>>` (which is expected by the function signature) are different types and cannot be implicitly converted.
I referred to the existing implementation in `HTMLMediaElement::AudioSourceProviderImpl::ProvideInput`, which uses `web_audio_data` as a local variable. I assumed that since the audio channel count is quite limited, the overhead of allocating a small vector might be acceptable here.
To strictly avoid frequent memory allocations on the audio hot path, I have restored the member variable and applied `RAW_PTR_EXCLUSION std::vector<base::span<float>>` to fix this in the updated code.
Could you please take another look? Thanks!
std::vector<base::span<float>> web_audio_data(static_cast<size_t>(n));```suggestion
if (web_audio_data_.size() != n) {
web_audio_data_.resize(static_cast<size_t>(n));
}```
Done
web_audio_data[i] = bus->Channel(i)->MutableSpan().first(```suggestion
web_audio_data_[i] = bus->Channel(i)->MutableSpan().first(
```
Done
web_audio_source_provider_->ProvideInput(web_audio_data, frames_to_process);kelsen liu```suggestion
web_audio_source_provider_->ProvideInput(web_audio_data_, frames_to_process);
```
Done
Related details
- Hongchan Choi
- Michael Wilson
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. |
kelsen liu (Gerrit)
kelsen liu added 1 comment![]( "Open in Gerrit")
std::unique_ptr<WebAudioSourceProvider> web_audio_source_provider_;```suggestion
std::unique_ptr<WebAudioSourceProvider> web_audio_source_provider_;
// Re-allocate only when the channel count changes to avoid heap
// allocations on the real-time audio thread.
kelsen liustd::vector<base::span<float>> web_audio_data_;
```
Done
Related details
- Hongchan Choi
- Michael Wilson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |