Spanify ReverbConvolverStage::Process [chromium/src : main]
kelsen liu (Gerrit)
kelsen liu added 1 comment![]( "Open in Gerrit")
Related details
- Michael Wilson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Wilson (Gerrit)
Michael Wilson voted and added 6 comments![]( "Open in Gerrit")
Votes added by Michael Wilson
| Code-Review | +1 |
6 comments
LGTM overall with minor comments / requests. I have been working on a change that overlaps with this here: https://crrev.com/c/7640131 -- let's land your change first since I think it is more complete. If you are continuing to work in this area we should coordinate so as not to duplicate effort. Thank you!
ReverbInputBuffer* input_buffer = convolver->InputBuffer();We can inline this (see other comment)
Process(input_buffer->DirectReadFrom(&input_read_index_, frames_to_process));I think we don't need a temporary here:
```suggestion
Process(convolver->InputBuffer()->DirectReadFrom(&input_read_index_, frames_to_process));
```
bool is_temporary_buffer_safe =
frames_to_process <= temporary_buffer_.size();
DCHECK(is_temporary_buffer_safe);
if (!is_temporary_buffer_safe) {
return;
}I think this is now checked by `first()` so we can remove this block.
bool is_temporary_buffer_safe =
frames_to_process <= pre_delay_buffer_.size();
DCHECK(is_temporary_buffer_safe);
if (!is_temporary_buffer_safe) {
return;
}I think this is now checked by `first()` so we can remove this block.
if (!direct_mode_) {
fft_convolver_->Process(fft_kernel_.get(), pre_delayed_source.data(),
temporary_buffer.data(), frames_to_process);
} else {
direct_convolver_->Process(pre_delayed_source.data(),
temporary_buffer.data(), frames_to_process);
}Nit: we can swap these conditions to marginally improve readability.
```suggestion
if (direct_mode_) {
direct_convolver_->Process(pre_delayed_source.data(),
temporary_buffer.data(), frames_to_process);
} else {
fft_convolver_->Process(fft_kernel_.get(), pre_delayed_source.data(),
temporary_buffer.data(), frames_to_process);
}
```
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 1 comment![]( "Open in Gerrit")
LGTM overall with minor comments / requests. I have been working on a change that overlaps with this here: https://crrev.com/c/7640131 -- let's land your change first since I think it is more complete. If you are continuing to work in this area we should coordinate so as not to duplicate effort. Thank you!
Thanks! Checked your CL and yours is definitely the better fix. I'll abandon this so yours can land.
I'm still around to help out in this area. Is there a specific chunk of logic or a follow-up you'd like to hand off to me?
Related details
- 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 abandoned this change.
kelsen liu abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
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")
kelsen liuLGTM overall with minor comments / requests. I have been working on a change that overlaps with this here: https://crrev.com/c/7640131 -- let's land your change first since I think it is more complete. If you are continuing to work in this area we should coordinate so as not to duplicate effort. Thank you!
Thanks! Checked your CL and yours is definitely the better fix. I'll abandon this so yours can land.
I'm still around to help out in this area. Is there a specific chunk of logic or a follow-up you'd like to hand off to me?
I think there are some things you did better in this CL, and it is also more targeted, so I would prefer to land this one and rebase my CL on top of it once it lands. Sorry if I implied that we should abandon this change! I think your changes so far have been very good.
I am focusing on UNSAFE_TODO, and the bug you are working on is about general spanfication, so there are some areas that don't overlap but a large amount does.
If you have some bandwidth, it would be helpful if you could look at:
- third_party/blink/renderer/platform/audio/fft_convolver.cc
- third_party/blink/renderer/platform/audio/biquad.cc
But, if there was something you were already interested in looking at please go ahead. It's not a huge problem if we overlap a bit. I will also CC you on my other changes in the area.
Thank you again for your work here! I still think we should land this CL. If you strongly disagree please let me know and I will roll your improvements into the other CL.
Related details
- kelsen liu
Code-Coverage
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")
kelsen liuLGTM overall with minor comments / requests. I have been working on a change that overlaps with this here: https://crrev.com/c/7640131 -- let's land your change first since I think it is more complete. If you are continuing to work in this area we should coordinate so as not to duplicate effort. Thank you!
Michael WilsonThanks! Checked your CL and yours is definitely the better fix. I'll abandon this so yours can land.
I'm still around to help out in this area. Is there a specific chunk of logic or a follow-up you'd like to hand off to me?
I think there are some things you did better in this CL, and it is also more targeted, so I would prefer to land this one and rebase my CL on top of it once it lands. Sorry if I implied that we should abandon this change! I think your changes so far have been very good.
I am focusing on UNSAFE_TODO, and the bug you are working on is about general spanfication, so there are some areas that don't overlap but a large amount does.
If you have some bandwidth, it would be helpful if you could look at:
- third_party/blink/renderer/platform/audio/fft_convolver.cc
- third_party/blink/renderer/platform/audio/biquad.cc
But, if there was something you were already interested in looking at please go ahead. It's not a huge problem if we overlap a bit. I will also CC you on my other changes in the area.
Thank you again for your work here! I still think we should land this CL. If you strongly disagree please let me know and I will roll your improvements into the other CL.
Thanks! It's really no problem—please go ahead and roll this into your CL.
I'll grab fft_convolver.cc and biquad.cc when I have some free time later.
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |