Introduce VoiceIsolation internal interface [chromium/src : main]
Pablo Barrera González (Gerrit)
Pablo Barrera González added 1 comment![]( "Open in Gerrit")
Internally VoiceIsolation works only in PCM format.Pablo Barrera GonzálezThe whole audio processing pipeline works only on PCM, so the description and thus the goal of the CL is unclear. Could you update?
Pablo Barrera GonzálezFrom the API it was not clear to me that audio_params.format() was only accepted in AUDIO_PCM_LINEAR and AUDIO_PCM_LINEAR_LOW_LATENCY.
Do we need the IsFormatSupported method?
I proposed a new description, PTAL
Related details
- 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. |
Olga Sharonova (Gerrit)
Olga Sharonova added 1 comment![]( "Open in Gerrit")
void ProcessAudio(const AudioBus& input_bus, AudioBus& output_bus);How will this be used by VoiceIsolationHandler when we need to accumulate 20 ms before producing audio?
Related details
- Pablo Barrera González
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. |
Olga Sharonova (Gerrit)
Olga Sharonova added 1 comment![]( "Open in Gerrit")
void ProcessAudio(const AudioBus& input_bus, AudioBus& output_bus);How will this be used by VoiceIsolationHandler when we need to accumulate 20 ms before producing audio?
Per offline discussion - please disregard this comment
Olga Sharonova (Gerrit)
Olga Sharonova added 6 comments![]( "Open in Gerrit")
// |model| and correct audio params (PCM linear format). The |model| needs toNothing about model here? (also back ticks `` are preferred to ||)
bool IsFormatSupported(const media::AudioParameters& audio_params) {not needed
if (!IsFormatSupported(audio_params)) {CHECK is enough
CHECK_EQ(input_bus.channel(0).size(), output_bus.channel(0).size());
media::AudioBus::ConstChannel input_channel = input_bus.channel(0);
media::AudioBus::Channel output_channel = output_bus.channel(0);
internal_voice_isolation_->ProcessAudio(input_channel, output_channel);
for (int i = 1; i < output_bus.channels(); ++i) {
output_bus.channel(i).copy_from_nonoverlapping(output_channel);
}Should it got through PassThrough instead?
virtual size_t frame_size() const = 0;
virtual size_t frames_per_second() const = 0;document
media::VoiceIsolation::MaybeCreate(output_params);The CL split does not look right. If in one of the previous CLs you change the signature of MaybeCreate to take all parameters it needs, we don't need to touch the service any longer.
Pablo Barrera González (Gerrit)
Pablo Barrera González added 7 comments![]( "Open in Gerrit")
// |model| and correct audio params (PCM linear format). The |model| needs toNothing about model here? (also back ticks `` are preferred to ||)
Adding model as a parameter to this function.
void ProcessAudio(const AudioBus& input_bus, AudioBus& output_bus);Olga SharonovaHow will this be used by VoiceIsolationHandler when we need to accumulate 20 ms before producing audio?
Per offline discussion - please disregard this comment
Done
bool IsFormatSupported(const media::AudioParameters& audio_params) {Pablo Barrera Gonzáleznot needed
I am leaving this as a CHECK (comment from line 38)
if (!IsFormatSupported(audio_params)) {Pablo Barrera GonzálezCHECK is enough
Done
CHECK_EQ(input_bus.channel(0).size(), output_bus.channel(0).size());
media::AudioBus::ConstChannel input_channel = input_bus.channel(0);
media::AudioBus::Channel output_channel = output_bus.channel(0);
internal_voice_isolation_->ProcessAudio(input_channel, output_channel);
for (int i = 1; i < output_bus.channels(); ++i) {
output_bus.channel(i).copy_from_nonoverlapping(output_channel);
}Should it got through PassThrough instead?
All VoiceIsolationComponent (as internal_voice_isolation_) are mono is mono.
This for loop copies the first channel to all channels. We can develop stereo support inside VoiceIsolationComponent if needed in the future. Meanwhile I think it's simpler to keep all in the processing pipeline (VoiceIsolationComponent) mono and handle fake multi-channel support here.
virtual size_t frame_size() const = 0;
virtual size_t frames_per_second() const = 0;Pablo Barrera Gonzálezdocument
Done
The CL split does not look right. If in one of the previous CLs you change the signature of MaybeCreate to take all parameters it needs, we don't need to touch the service any longer.
- 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. |
Pablo Barrera González (Gerrit)
Pablo Barrera González added 1 comment![]( "Open in Gerrit")
Internally VoiceIsolation works only in PCM format.The whole audio processing pipeline works only on PCM, so the description and thus the goal of the CL is unclear. Could you update?
Pablo Barrera GonzálezFrom the API it was not clear to me that audio_params.format() was only accepted in AUDIO_PCM_LINEAR and AUDIO_PCM_LINEAR_LOW_LATENCY.
Do we need the IsFormatSupported method?
I proposed a new description, PTAL
Done
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 7 comments![]( "Open in Gerrit")
"//media/webrtc/voice_isolation:unit_tests",Should this be conditional?
// TODO(barrerap): Add back once we have a resampler in place.Unclear. Will this fail now?
// `model` and correct audio params (PCM linear format). The `model` needs to
// stay present during all the lifetime of the VoiceIsolation instance.The relationship is unclear, as we pass a reference. What are we going to do with it? Copy? Store?
https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++.md#object-ownership-and-calling-conventions
Pass a pointer?
// TODO(b/512016773): Pass the model to VoiceIsolation once it is supported.I don't think we should link to internal bugs
// TODO(crbug.com//40176497): False positive.What does this comment mean?
for (int i = 1; i < output_bus.channels(); ++i) {Document what we are doing here
class COMPONENT_EXPORT(MEDIA_WEBRTC) VoiceIsolationComponent {Delete copy contructor and assignment?
Related details
- Pablo Barrera González
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. |
Pablo Barrera González (Gerrit)
Pablo Barrera González added 7 comments![]( "Open in Gerrit")
"//media/webrtc/voice_isolation:unit_tests",Pablo Barrera GonzálezShould this be conditional?
Done
// TODO(barrerap): Add back once we have a resampler in place.Unclear. Will this fail now?
yes, because in the tests we use 44.1kHz sampling rate and CreateVoiceIsolation enforces 48.
But if we don't check it, PassthroughVoiceIsolation::ProcessAudio will not even look at the frame_size and just copy enything that is passed to it.
Fixing this requires propagating the output_format information down and I will remove all that code in https://chromium-review.git.corp.google.com/c/chromium/src/+/8052654.
// `model` and correct audio params (PCM linear format). The `model` needs to
// stay present during all the lifetime of the VoiceIsolation instance.The relationship is unclear, as we pass a reference. What are we going to do with it? Copy? Store?
https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++.md#object-ownership-and-calling-conventionsPass a pointer?
Done
// TODO(b/512016773): Pass the model to VoiceIsolation once it is supported.I don't think we should link to internal bugs
Dropping the link to the bug.
// TODO(crbug.com//40176497): False positive.What does this comment mean?
Done
for (int i = 1; i < output_bus.channels(); ++i) {Document what we are doing here
Done
class COMPONENT_EXPORT(MEDIA_WEBRTC) VoiceIsolationComponent {Delete copy contructor and assignment?
- 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 1 comment![]( "Open in Gerrit")
CHECK(voice_isolation_);We can't have CHECK here, because MaybeCreate may return nullptr. Rather add //TODO: Asynchronously notify the client if `voice_isolation_` creation failed. ?
Related details
- Pablo Barrera González
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. |