Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1525173002:
Bugfix that fixes the integration issue that cause WebRTC AEC mobile to fail (Closed)
|
Created:
5 years ago by peah-webrtc Modified:
5 years ago CC:
webrtc-reviews_webrtc.org, peah-webrtc, Andrew MacDonald, henrika_webrtc, tterriberry_mozilla.com, audio-team_agora.io, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, tlegrand-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionBugfix that fixes the error where the audio processing module is called
using the wrong sample rate for the render signal.
The CL is basically a partial revert of the related changes done on
output_mixer.cc in the CL https://codereview.webrtc.org/1234463003.
The CL also reverts the removal of the input_sample_rate_hz() method
that was removed as part of the CL
https://codereview.webrtc.org/1379123002 (as it was at that point
no longer used).
It should be noted that this CL turns off the effect of the
IntelligibilityEnhancer when the AudioFrame AudioProcessing APIs are
used. While it may be possible to solve that by adding upsampling after
the API call, that approach was discarded due to that:
-That would add extra processing in the echo path, leading to possible
AEC performance reduction.
-That would add extra complexity for the mobile case.
-That would only patch the intelligibility enhancer operation as the
proper way to do such an operation is within APM.
-The intelligibility enhancer is not active by default anywhere.
BUG=webrtc:5237
Committed: https://crrev.com/66085beef83c790a69666b9be8a74bb2eee44fab
Cr-Commit-Position: refs/heads/master@{#11045}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Changes in response to reviewer comments #
Total comments: 1
Patch Set 3 : Added missing input_sample_rate_hz() to FakeAudioProcessing #
Messages
Total messages: 30 (14 generated)
Description was changed from ========== Bugfix that fixes the error where the audio processing module is called using the wrong sample rate for the render signal. The CL is basically a partial revert of the related changes done on output_mixer.cc in the CL 1234463003 The CL also reverts the removal of the input_sample_rate_hz() method that was removed as part of the CL 1379123002 (as it was at that point no longer used). It should be noted that this CL turns off the effect of the IntelligibilityEnhancer when the AudioFrame AudioProcesing APIs are used. While it may be possible to solve that by adding upsampling after the API call, that approach was discarded due to that: -That would add extra processing in the echo path, leading to possible AEC performance reduction. -That would add extra complexity for the mobile case. -That would only patch the intelligibility enhancer operation as the proper way to do such an operation is within APM. -The intelligibility enhancer is not active by default anywhere. BUG=webrtc:5237 ========== to ========== Bugfix that fixes the error where the audio processing module is called using the wrong sample rate for the render signal. The CL is basically a partial revert of the related changes done on output_mixer.cc in the CL 1234463003 The CL also reverts the removal of the input_sample_rate_hz() method that was removed as part of the CL 1379123002 (as it was at that point no longer used). It should be noted that this CL turns off the effect of the IntelligibilityEnhancer when the AudioFrame AudioProcesing APIs are used. While it may be possible to solve that by adding upsampling after the API call, that approach was discarded due to that: -That would add extra processing in the echo path, leading to possible AEC performance reduction. -That would add extra complexity for the mobile case. -That would only patch the intelligibility enhancer operation as the proper way to do such an operation is within APM. -The intelligibility enhancer is not active by default anywhere. BUG=webrtc:5237 ==========
Description was changed from ========== Bugfix that fixes the error where the audio processing module is called using the wrong sample rate for the render signal. The CL is basically a partial revert of the related changes done on output_mixer.cc in the CL 1234463003 The CL also reverts the removal of the input_sample_rate_hz() method that was removed as part of the CL 1379123002 (as it was at that point no longer used). It should be noted that this CL turns off the effect of the IntelligibilityEnhancer when the AudioFrame AudioProcesing APIs are used. While it may be possible to solve that by adding upsampling after the API call, that approach was discarded due to that: -That would add extra processing in the echo path, leading to possible AEC performance reduction. -That would add extra complexity for the mobile case. -That would only patch the intelligibility enhancer operation as the proper way to do such an operation is within APM. -The intelligibility enhancer is not active by default anywhere. BUG=webrtc:5237 ========== to ========== Bugfix that fixes the error where the audio processing module is called using the wrong sample rate for the render signal. The CL is basically a partial revert of the related changes done on output_mixer.cc in the CL 1234463003. The CL also reverts the removal of the input_sample_rate_hz() method that was removed as part of the CL 1379123002 (as it was at that point no longer used). It should be noted that this CL turns off the effect of the IntelligibilityEnhancer when the AudioFrame AudioProcessing APIs are used. While it may be possible to solve that by adding upsampling after the API call, that approach was discarded due to that: -That would add extra processing in the echo path, leading to possible AEC performance reduction. -That would add extra complexity for the mobile case. -That would only patch the intelligibility enhancer operation as the proper way to do such an operation is within APM. -The intelligibility enhancer is not active by default anywhere. BUG=webrtc:5237 ==========
peah@webrtc.org changed reviewers: + aluebs@webrtc.org, henrik.lundin@webrtc.org, turaj@webrtc.org
On 2015/12/15 15:34:37, peah-webrtc wrote: Comment on the CL description: Please add links to the code reviews for the offending CLs (not just CL numbers).
LG, but I'd prefer if aluebs would take a look. https://codereview.webrtc.org/1525173002/diff/1/webrtc/voice_engine/output_mi... File webrtc/voice_engine/output_mixer.cc (right): https://codereview.webrtc.org/1525173002/diff/1/webrtc/voice_engine/output_mi... webrtc/voice_engine/output_mixer.cc:563: assert(false); DCHECK
lgtm https://codereview.webrtc.org/1525173002/diff/1/webrtc/voice_engine/output_mi... File webrtc/voice_engine/output_mixer.cc (right): https://codereview.webrtc.org/1525173002/diff/1/webrtc/voice_engine/output_mi... webrtc/voice_engine/output_mixer.cc:552: void OutputMixer::APMProcessReverseStream() { APMAnalyzeReverseStream?
solenberg@webrtc.org changed reviewers: + solenberg@webrtc.org
https://codereview.webrtc.org/1525173002/diff/1/webrtc/voice_engine/output_mi... File webrtc/voice_engine/output_mixer.cc (right): https://codereview.webrtc.org/1525173002/diff/1/webrtc/voice_engine/output_mi... webrtc/voice_engine/output_mixer.cc:552: void OutputMixer::APMProcessReverseStream() { On 2015/12/15 19:16:08, aluebs-webrtc wrote: > APMAnalyzeReverseStream? If the name is a problem I'd suggest making this code inline in the one place where it is being called. https://codereview.webrtc.org/1525173002/diff/1/webrtc/voice_engine/output_mi... webrtc/voice_engine/output_mixer.cc:558: RemixAndResample(_audioFrame, &audioproc_resampler_, &frame); What are the expected performance effects of this?
https://codereview.webrtc.org/1525173002/diff/1/webrtc/voice_engine/output_mi... File webrtc/voice_engine/output_mixer.cc (right): https://codereview.webrtc.org/1525173002/diff/1/webrtc/voice_engine/output_mi... webrtc/voice_engine/output_mixer.cc:552: void OutputMixer::APMProcessReverseStream() { On 2015/12/15 19:37:14, the sun wrote: > On 2015/12/15 19:16:08, aluebs-webrtc wrote: > > APMAnalyzeReverseStream? > > If the name is a problem I'd suggest making this code inline in the one place > where it is being called. Done. https://codereview.webrtc.org/1525173002/diff/1/webrtc/voice_engine/output_mi... webrtc/voice_engine/output_mixer.cc:552: void OutputMixer::APMProcessReverseStream() { On 2015/12/15 19:16:08, aluebs-webrtc wrote: > APMAnalyzeReverseStream? I decided to keep it since I expected that this CL would be followed up with another fix that allowed the ProcessReverseStream call to be reintroduced. I will change according to the inline suggestion instead though. Please let me know if you are ok with that! https://codereview.webrtc.org/1525173002/diff/1/webrtc/voice_engine/output_mi... webrtc/voice_engine/output_mixer.cc:558: RemixAndResample(_audioFrame, &audioproc_resampler_, &frame); On 2015/12/15 19:37:14, the sun wrote: > What are the expected performance effects of this? In what sense do you mean? There is a computational cost for the resampling and how big that is I cannot tell. But note that before Aug 14, this resampling was always done so it should not be anything that the platforms where it is used cannot handle. The resampling is needed to at all be able to run AECM but on a longer term basis we should perhaps consider to do it in the band-splitting mechanism instead. https://codereview.webrtc.org/1525173002/diff/1/webrtc/voice_engine/output_mi... webrtc/voice_engine/output_mixer.cc:563: assert(false); On 2015/12/15 16:07:05, hlundin-webrtc wrote: > DCHECK Done.
Description was changed from ========== Bugfix that fixes the error where the audio processing module is called using the wrong sample rate for the render signal. The CL is basically a partial revert of the related changes done on output_mixer.cc in the CL 1234463003. The CL also reverts the removal of the input_sample_rate_hz() method that was removed as part of the CL 1379123002 (as it was at that point no longer used). It should be noted that this CL turns off the effect of the IntelligibilityEnhancer when the AudioFrame AudioProcessing APIs are used. While it may be possible to solve that by adding upsampling after the API call, that approach was discarded due to that: -That would add extra processing in the echo path, leading to possible AEC performance reduction. -That would add extra complexity for the mobile case. -That would only patch the intelligibility enhancer operation as the proper way to do such an operation is within APM. -The intelligibility enhancer is not active by default anywhere. BUG=webrtc:5237 ========== to ========== Bugfix that fixes the error where the audio processing module is called using the wrong sample rate for the render signal. The CL is basically a partial revert of the related changes done on output_mixer.cc in the CL https://codereview.webrtc.org/1234463003. The CL also reverts the removal of the input_sample_rate_hz() method that was removed as part of the CL https://codereview.webrtc.org/1379123002 (as it was at that point no longer used). It should be noted that this CL turns off the effect of the IntelligibilityEnhancer when the AudioFrame AudioProcessing APIs are used. While it may be possible to solve that by adding upsampling after the API call, that approach was discarded due to that: -That would add extra processing in the echo path, leading to possible AEC performance reduction. -That would add extra complexity for the mobile case. -That would only patch the intelligibility enhancer operation as the proper way to do such an operation is within APM. -The intelligibility enhancer is not active by default anywhere. BUG=webrtc:5237 ==========
Description was changed from ========== Bugfix that fixes the error where the audio processing module is called using the wrong sample rate for the render signal. The CL is basically a partial revert of the related changes done on output_mixer.cc in the CL https://codereview.webrtc.org/1234463003. The CL also reverts the removal of the input_sample_rate_hz() method that was removed as part of the CL https://codereview.webrtc.org/1379123002 (as it was at that point no longer used). It should be noted that this CL turns off the effect of the IntelligibilityEnhancer when the AudioFrame AudioProcessing APIs are used. While it may be possible to solve that by adding upsampling after the API call, that approach was discarded due to that: -That would add extra processing in the echo path, leading to possible AEC performance reduction. -That would add extra complexity for the mobile case. -That would only patch the intelligibility enhancer operation as the proper way to do such an operation is within APM. -The intelligibility enhancer is not active by default anywhere. BUG=webrtc:5237 ========== to ========== Bugfix that fixes the error where the audio processing module is called using the wrong sample rate for the render signal. The CL is basically a partial revert of the related changes done on output_mixer.cc in the CL https://codereview.webrtc.org/1234463003. The CL also reverts the removal of the input_sample_rate_hz() method that was removed as part of the CL https://codereview.webrtc.org/1379123002 (as it was at that point no longer used). It should be noted that this CL turns off the effect of the IntelligibilityEnhancer when the AudioFrame AudioProcessing APIs are used. While it may be possible to solve that by adding upsampling after the API call, that approach was discarded due to that: -That would add extra processing in the echo path, leading to possible AEC performance reduction. -That would add extra complexity for the mobile case. -That would only patch the intelligibility enhancer operation as the proper way to do such an operation is within APM. -The intelligibility enhancer is not active by default anywhere. BUG=webrtc:5237 ==========
https://codereview.webrtc.org/1525173002/diff/1/webrtc/voice_engine/output_mi... File webrtc/voice_engine/output_mixer.cc (right): https://codereview.webrtc.org/1525173002/diff/1/webrtc/voice_engine/output_mi... webrtc/voice_engine/output_mixer.cc:552: void OutputMixer::APMProcessReverseStream() { On 2015/12/15 21:29:45, peah-webrtc wrote: > On 2015/12/15 19:16:08, aluebs-webrtc wrote: > > APMAnalyzeReverseStream? > > I decided to keep it since I expected that this CL would be followed up with > another fix that allowed the ProcessReverseStream call to be reintroduced. I > will change according to the inline suggestion instead though. Please let me > know if you are ok with that! Sounds good to me.
lgtm https://codereview.chromium.org/1525173002/diff/20001/webrtc/modules/audio_pr... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.chromium.org/1525173002/diff/20001/webrtc/modules/audio_pr... webrtc/modules/audio_processing/include/audio_processing.h:284: virtual int input_sample_rate_hz() const = 0; Note: as "luck" would have it, this method was never removed from MockAudioProcessing so there's no need to add it.
lgtm
The CQ bit was checked by peah@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1525173002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1525173002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/7955)
The CQ bit was checked by peah@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1525173002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1525173002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from aluebs@webrtc.org, solenberg@webrtc.org, henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/1525173002/#ps40001 (title: "Added missing input_sample_rate_hz() to FakeAudioProcessing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1525173002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1525173002/40001
Message was sent while issue was closed.
Description was changed from ========== Bugfix that fixes the error where the audio processing module is called using the wrong sample rate for the render signal. The CL is basically a partial revert of the related changes done on output_mixer.cc in the CL https://codereview.webrtc.org/1234463003. The CL also reverts the removal of the input_sample_rate_hz() method that was removed as part of the CL https://codereview.webrtc.org/1379123002 (as it was at that point no longer used). It should be noted that this CL turns off the effect of the IntelligibilityEnhancer when the AudioFrame AudioProcessing APIs are used. While it may be possible to solve that by adding upsampling after the API call, that approach was discarded due to that: -That would add extra processing in the echo path, leading to possible AEC performance reduction. -That would add extra complexity for the mobile case. -That would only patch the intelligibility enhancer operation as the proper way to do such an operation is within APM. -The intelligibility enhancer is not active by default anywhere. BUG=webrtc:5237 ========== to ========== Bugfix that fixes the error where the audio processing module is called using the wrong sample rate for the render signal. The CL is basically a partial revert of the related changes done on output_mixer.cc in the CL https://codereview.webrtc.org/1234463003. The CL also reverts the removal of the input_sample_rate_hz() method that was removed as part of the CL https://codereview.webrtc.org/1379123002 (as it was at that point no longer used). It should be noted that this CL turns off the effect of the IntelligibilityEnhancer when the AudioFrame AudioProcessing APIs are used. While it may be possible to solve that by adding upsampling after the API call, that approach was discarded due to that: -That would add extra processing in the echo path, leading to possible AEC performance reduction. -That would add extra complexity for the mobile case. -That would only patch the intelligibility enhancer operation as the proper way to do such an operation is within APM. -The intelligibility enhancer is not active by default anywhere. BUG=webrtc:5237 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Bugfix that fixes the error where the audio processing module is called using the wrong sample rate for the render signal. The CL is basically a partial revert of the related changes done on output_mixer.cc in the CL https://codereview.webrtc.org/1234463003. The CL also reverts the removal of the input_sample_rate_hz() method that was removed as part of the CL https://codereview.webrtc.org/1379123002 (as it was at that point no longer used). It should be noted that this CL turns off the effect of the IntelligibilityEnhancer when the AudioFrame AudioProcessing APIs are used. While it may be possible to solve that by adding upsampling after the API call, that approach was discarded due to that: -That would add extra processing in the echo path, leading to possible AEC performance reduction. -That would add extra complexity for the mobile case. -That would only patch the intelligibility enhancer operation as the proper way to do such an operation is within APM. -The intelligibility enhancer is not active by default anywhere. BUG=webrtc:5237 ========== to ========== Bugfix that fixes the error where the audio processing module is called using the wrong sample rate for the render signal. The CL is basically a partial revert of the related changes done on output_mixer.cc in the CL https://codereview.webrtc.org/1234463003. The CL also reverts the removal of the input_sample_rate_hz() method that was removed as part of the CL https://codereview.webrtc.org/1379123002 (as it was at that point no longer used). It should be noted that this CL turns off the effect of the IntelligibilityEnhancer when the AudioFrame AudioProcessing APIs are used. While it may be possible to solve that by adding upsampling after the API call, that approach was discarded due to that: -That would add extra processing in the echo path, leading to possible AEC performance reduction. -That would add extra complexity for the mobile case. -That would only patch the intelligibility enhancer operation as the proper way to do such an operation is within APM. -The intelligibility enhancer is not active by default anywhere. BUG=webrtc:5237 Committed: https://crrev.com/66085beef83c790a69666b9be8a74bb2eee44fab Cr-Commit-Position: refs/heads/master@{#11045} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/66085beef83c790a69666b9be8a74bb2eee44fab Cr-Commit-Position: refs/heads/master@{#11045} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
