Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1776363002:
Use ProcessReverseStream in VoiceEngines OutputMixer (Closed)
|
Created:
4 years, 9 months ago by aluebs-webrtc Modified:
4 years, 9 months ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, henrika_webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@reverse2 Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionUse ProcessReverseStream in VoiceEngines OutputMixer
Committed: https://crrev.com/da116c4c3739204ff24a3287156bc8250cff8d5b
Cr-Commit-Position: refs/heads/master@{#12044}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Take the lock in the right place #
Messages
Total messages: 24 (8 generated)
aluebs@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, peah@webrtc.org, solenberg@webrtc.org, turaj@webrtc.org
https://codereview.webrtc.org/1776363002/diff/1/webrtc/voice_engine/output_mi... File webrtc/voice_engine/output_mixer.cc (right): https://codereview.webrtc.org/1776363002/diff/1/webrtc/voice_engine/output_mi... webrtc/voice_engine/output_mixer.cc:519: if (_audioProcessingModulePtr->ProcessReverseStream(&_audioFrame) != 0) { Do we know the implications of this? This changes the APM input from being mono to being stereo. How is this handled inside APM? Do we downmix to mono before doing AEC, AGC and AECM processing? If not, afaics this change will actually double the complexity for these subcomponents as the APM applies separate instances of these for each channel. On top of that, this may trigger a bug related to how the processing is applied on the capture channels. Lets discuss the implications of this change.
On 2016/03/09 22:07:54, peah-webrtc wrote: > https://codereview.webrtc.org/1776363002/diff/1/webrtc/voice_engine/output_mi... > File webrtc/voice_engine/output_mixer.cc (right): > > https://codereview.webrtc.org/1776363002/diff/1/webrtc/voice_engine/output_mi... > webrtc/voice_engine/output_mixer.cc:519: if > (_audioProcessingModulePtr->ProcessReverseStream(&_audioFrame) != 0) { > Do we know the implications of this? This changes the APM input from being mono > to being stereo. How is this handled inside APM? Do we downmix to mono before > doing AEC, AGC and AECM processing? If not, afaics this change will actually > double the complexity for these subcomponents as the APM applies separate > instances of these for each channel. On top of that, this may trigger a bug > related to how the processing is applied on the capture channels. > > Lets discuss the implications of this change. lgtm provided that the change is sufficiently tested.
https://codereview.webrtc.org/1776363002/diff/1/webrtc/voice_engine/output_mi... File webrtc/voice_engine/output_mixer.cc (right): https://codereview.webrtc.org/1776363002/diff/1/webrtc/voice_engine/output_mi... webrtc/voice_engine/output_mixer.cc:519: if (_audioProcessingModulePtr->ProcessReverseStream(&_audioFrame) != 0) { On 2016/03/09 22:07:54, peah-webrtc wrote: > Do we know the implications of this? This changes the APM input from being mono > to being stereo. How is this handled inside APM? Do we downmix to mono before > doing AEC, AGC and AECM processing? If not, afaics this change will actually > double the complexity for these subcomponents as the APM applies separate > instances of these for each channel. On top of that, this may trigger a bug > related to how the processing is applied on the capture channels. > > Lets discuss the implications of this change. After offline discussions: I think this change is fine as there is a functionality in APM that also internally downmixes the signal to 1 channels. This changes must, however, be properly tested as the internal downmixing has not been active (due to the code being removed in this CL) before this CL.
lgtm
not lgtm It's hard to follow the code paths in APM before and after this API switch (in OutputMixer) so I have a hard time convincing myself of the effects we can expect. I think you should clean up the tangle of Analyze... (x2), AnalyzeLocked, Process... (x2), ProcessLocked, in a CL preparing for this work. https://codereview.webrtc.org/1776363002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1776363002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:779: if (is_rev_processed()) { Remove this function: - The name uses an ambiguous abbreviation. - The plain "constants_.intelligibility_enabled" is used in more places than this function, for the same purpose. https://codereview.webrtc.org/1776363002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:782: } else if (render_check_rev_conversion_needed()) { Remove this function: - It does the exact same thing as rev_conversion_needed() - It is used in just one place. Rename rev_conversion_needed() while you're at it. https://codereview.webrtc.org/1776363002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:832: return ProcessReverseStreamLocked(); Wow! So AnalyzeReverseStreamLocked() can have side effects? It isn't purely for analyzing? https://codereview.webrtc.org/1776363002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/1776363002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/include/audio_processing.h:369: // DEPRECATED: Use |ProcessReverseStream| instead. If this is deprecated, can we make it a thin wrapper around ProcessReverseStream, which DCHECKs that the IE is disabled? Also - move the commentary to ProcessReverseStream.
solenberg, as discussed offline, the multiple interfaces are because we support a AudioFrame and a float* interface and now that are introducing the IE both cases have an Analyze and a Process in the reverse case, but I agree it can be confusing. In all cases the Process*StreamLocked is the method that does ALL the processing and is called by the AudioFrame and float* interfaces after setting all the configurations and data from the respective input. The only different between AnalizeReverseStream (the one called before this CL) and the ProcessReverseStream (the one called after it) is that if the frame is processed it copies the processed data onto it. I will be dropping AnalyzeReverseStream(AudioFrame) method in a follow-up CL, but please let me know if I can clarify something or you think I should modify something in particular. https://codereview.webrtc.org/1776363002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1776363002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:779: if (is_rev_processed()) { On 2016/03/10 12:49:22, the sun wrote: > Remove this function: > - The name uses an ambiguous abbreviation. > - The plain "constants_.intelligibility_enabled" is used in more places than > this function, for the same purpose. I still would argue that having a method would make it easier to refactor when we add other components in the render stream that modify the signal. Also, I can't find any place where the plain constant is used for the purpose to check if the signal has been processed. We could argue about the naming, but if you still think it should be removed I am happy to do so in another CL. https://codereview.webrtc.org/1776363002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:782: } else if (render_check_rev_conversion_needed()) { On 2016/03/10 12:49:22, the sun wrote: > Remove this function: > - It does the exact same thing as rev_conversion_needed() > - It is used in just one place. > > Rename rev_conversion_needed() while you're at it. Here you have a great point, but I don't think I should do the change in the CL, since it has nothing to do with it. But happy to do so in a follow-up CL. https://codereview.webrtc.org/1776363002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:832: return ProcessReverseStreamLocked(); On 2016/03/10 12:49:22, the sun wrote: > Wow! So AnalyzeReverseStreamLocked() can have side effects? It isn't purely for > analyzing? As discussed offline, it doesn't, because it doesn't copy the result into the AudioFrame as ProcessReverseStream does. But I agree it can be confusing. https://codereview.webrtc.org/1776363002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/1776363002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/include/audio_processing.h:369: // DEPRECATED: Use |ProcessReverseStream| instead. On 2016/03/10 12:49:22, the sun wrote: > If this is deprecated, can we make it a thin wrapper around > ProcessReverseStream, which DCHECKs that the IE is disabled? Also - move the > commentary to ProcessReverseStream. Actually, now that we are updating the VoE, we can remove this completely. I will do this in a follow up CL.
lgtm Thanks for the offline explanation aluebs@. I understand how the code works now. https://codereview.webrtc.org/1776363002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1776363002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:838: rtc::CritScope cs(&crit_render_); You should take this lock before calling AnalyzeReverseStream(), otherwise you don't know that the data you're copying out hasn't been touched by someone else. That is, IF you need to take a lock instead of using a thread checker in the "reverse" methods.
https://codereview.webrtc.org/1776363002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1776363002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:838: rtc::CritScope cs(&crit_render_); On 2016/03/11 09:49:19, the sun wrote: > You should take this lock before calling AnalyzeReverseStream(), otherwise you > don't know that the data you're copying out hasn't been touched by someone else. > > That is, IF you need to take a lock instead of using a thread checker in the > "reverse" methods. But AnalyzeReverseStream is taking this lock as well, so that would be a problem. I see the issue here, but that will be fixed in my next CL where I deprecate AnalyzeReverseStream and consolidate this in one method.
On 2016/03/11 13:31:03, aluebs-webrtc wrote: > https://codereview.webrtc.org/1776363002/diff/1/webrtc/modules/audio_processi... > File webrtc/modules/audio_processing/audio_processing_impl.cc (right): > > https://codereview.webrtc.org/1776363002/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/audio_processing_impl.cc:838: rtc::CritScope > cs(&crit_render_); > On 2016/03/11 09:49:19, the sun wrote: > > You should take this lock before calling AnalyzeReverseStream(), otherwise you > > don't know that the data you're copying out hasn't been touched by someone > else. > > > > That is, IF you need to take a lock instead of using a thread checker in the > > "reverse" methods. > > But AnalyzeReverseStream is taking this lock as well, so that would be a > problem. I see the issue here, but that will be fixed in my next CL where I > deprecate AnalyzeReverseStream and consolidate this in one method. Locks are re-entrant and AFAIS we only take crit_render_ (so order between locks won't be changed).
On 2016/03/11 13:40:15, the sun wrote: > On 2016/03/11 13:31:03, aluebs-webrtc wrote: > > > https://codereview.webrtc.org/1776363002/diff/1/webrtc/modules/audio_processi... > > File webrtc/modules/audio_processing/audio_processing_impl.cc (right): > > > > > https://codereview.webrtc.org/1776363002/diff/1/webrtc/modules/audio_processi... > > webrtc/modules/audio_processing/audio_processing_impl.cc:838: rtc::CritScope > > cs(&crit_render_); > > On 2016/03/11 09:49:19, the sun wrote: > > > You should take this lock before calling AnalyzeReverseStream(), otherwise > you > > > don't know that the data you're copying out hasn't been touched by someone > > else. > > > > > > That is, IF you need to take a lock instead of using a thread checker in the > > > "reverse" methods. > > > > But AnalyzeReverseStream is taking this lock as well, so that would be a > > problem. I see the issue here, but that will be fixed in my next CL where I > > deprecate AnalyzeReverseStream and consolidate this in one method. > > Locks are re-entrant and AFAIS we only take crit_render_ (so order between locks > won't be changed). Good point. Changed.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by aluebs@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, henrik.lundin@webrtc.org, peah@webrtc.org Link to the patchset: https://codereview.webrtc.org/1776363002/#ps20001 (title: "Take the lock in the right place")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1776363002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1776363002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/9933)
The CQ bit was checked by aluebs@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1776363002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1776363002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Use ProcessReverseStream in VoiceEngines OutputMixer ========== to ========== Use ProcessReverseStream in VoiceEngines OutputMixer Committed: https://crrev.com/da116c4c3739204ff24a3287156bc8250cff8d5b Cr-Commit-Position: refs/heads/master@{#12044} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/da116c4c3739204ff24a3287156bc8250cff8d5b Cr-Commit-Position: refs/heads/master@{#12044} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
