Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2562593003:
Add an optional artificial nearend signal for aecdump call recreation (Closed)
|
Created:
4 years ago by peah-webrtc Modified:
4 years ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1 Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionWhen recreating a call based on an aecdump recording the nearend used
is the one stored in the aecdump.
During AEC development, it is handy to be able to simulate different
doubletalk scenarios. This CL adds the ability to add an
artificial nearend on top of that present in the aecdump, which
allows for the developer to artificially customize the scenario
being tested
BUG=webrtc:6018
Committed: https://crrev.com/df80fd125989fae30eff04e8413d63178d98c3e5
Cr-Commit-Position: refs/heads/master@{#15502}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Corrected index #Patch Set 3 : Removed unused variables #
Total comments: 12
Patch Set 4 : Changes in response to reviewer comments #
Total comments: 9
Patch Set 5 : Changes in response to reviewer comments #
Total comments: 2
Patch Set 6 : Changed DCHECK to CHECK #
Messages
Total messages: 25 (7 generated)
Description was changed from ========== Added the ability to add an artificial nearend signal when recreating a call from an aecdump recording BUG= ========== to ========== When recreating a call based on an aecdump recording the nearend used is the one stored in the aecdump. During AEC development, it is handy to be able to simulate different doubletalk scenarios. This CL adds the ability to add an artificial nearend on top of that present in the aecdump, which allows for the developer to artificially customize the scenario being tested BUG=webrtc:6018 ==========
peah@webrtc.org changed reviewers: + aleloi@webrtc.org, henrik.lundin@webrtc.org, ivoc@webrtc.org
PTAL Thanks!
https://codereview.webrtc.org/2562593003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/2562593003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:111: 32767 * artificial_nearend_buf_->channels()[0][k]); Shouldn't this be fwd_frame_.data[k]? Also, why is it adding instead of assigning? https://codereview.webrtc.org/2562593003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.h (right): https://codereview.webrtc.org/2562593003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/aec_dump_based_simulator.h:57: std::unique_ptr<WavReader> artificial_nearend_file_; I think this can be removed, it does not seem to be used anywhere. https://codereview.webrtc.org/2562593003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/aec_dump_based_simulator.h:58: std::unique_ptr<ChannelBufferWavReader> artificial_nearend_buffer_reader_; This can be removed as well right? The same member is part of AudioProcessingSimulator, from which this class derives.
https://codereview.webrtc.org/2562593003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/2562593003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:111: 32767 * artificial_nearend_buf_->channels()[0][k]); On 2016/12/08 10:55:19, ivoc wrote: > Shouldn't this be fwd_frame_.data[k]? Also, why is it adding instead of > assigning? Great find (I probably have not used this yet with the int16 interface.) Regarding the adding, the idea is to add a nearend signal to the existing microphone signal (which includes the echo which we want to maintain). Assigning would replace the microphone signal. Done. https://codereview.webrtc.org/2562593003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.h (right): https://codereview.webrtc.org/2562593003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/aec_dump_based_simulator.h:57: std::unique_ptr<WavReader> artificial_nearend_file_; On 2016/12/08 10:55:19, ivoc wrote: > I think this can be removed, it does not seem to be used anywhere. Good find! Done. https://codereview.webrtc.org/2562593003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/aec_dump_based_simulator.h:58: std::unique_ptr<ChannelBufferWavReader> artificial_nearend_buffer_reader_; On 2016/12/08 10:55:19, ivoc wrote: > This can be removed as well right? The same member is part of > AudioProcessingSimulator, from which this class derives. Good find! Done.
One last question from my side. https://codereview.webrtc.org/2562593003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/2562593003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:111: 32767 * artificial_nearend_buf_->channels()[0][k]); On 2016/12/08 12:02:26, peah-webrtc wrote: > On 2016/12/08 10:55:19, ivoc wrote: > > Shouldn't this be fwd_frame_.data[k]? Also, why is it adding instead of > > assigning? > > Great find (I probably have not used this yet with the int16 interface.) > > Regarding the adding, the idea is to add a nearend signal to the existing > microphone signal (which includes the echo which we want to maintain). Assigning > would replace the microphone signal. > > Done. I see, that makes sense. I think this code can cause over/underflows as well, should that be addressed, or is it not a problem in practice?
https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:105: bool samples_left_to_process = const https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:110: fwd_frame_.data_[k] += static_cast<int16_t>( What happens if the frame is stereo? Then frame audio is interleaved and artificial nearend data is only added to the first half of the samples. https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:111: 32767 * artificial_nearend_buf_->channels()[0][k]); Can we use a checked_cast + saturated_cast here? https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.h (right): https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.h:153: std::unique_ptr<ChannelBuffer<float>> artificial_nearend_buf_; The nearend buffer is only used in the aec-dump-based simulator. To limit the scope in which they are visible, I think the buffer and reader should be private in the derived class. https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audioproc_float.cc (right): https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:419: processor.reset(new WavBasedSimulator(settings)); IIUC, artificial nearend functionality only works when there is an AEC dump. There should probably be a few more error messages for that case.
https://codereview.webrtc.org/2562593003/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/2562593003/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:111: 32767 * artificial_nearend_buf_->channels()[0][k]); On 2016/12/08 12:13:24, ivoc wrote: > On 2016/12/08 12:02:26, peah-webrtc wrote: > > On 2016/12/08 10:55:19, ivoc wrote: > > > Shouldn't this be fwd_frame_.data[k]? Also, why is it adding instead of > > > assigning? > > > > Great find (I probably have not used this yet with the int16 interface.) > > > > Regarding the adding, the idea is to add a nearend signal to the existing > > microphone signal (which includes the echo which we want to maintain). > Assigning > > would replace the microphone signal. > > > > Done. > > I see, that makes sense. I think this code can cause over/underflows as well, > should that be addressed, or is it not a problem in practice? You are fully correct! I added explicit controlled saturation. Done. https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:105: bool samples_left_to_process = On 2016/12/08 13:00:55, aleloi wrote: > const I changed the code to avoid the need for this variable. Done. https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:110: fwd_frame_.data_[k] += static_cast<int16_t>( On 2016/12/08 13:00:55, aleloi wrote: > What happens if the frame is stereo? Then frame audio is interleaved and > artificial nearend data is only added to the first half of the samples. That is a great point! I overcame that problem by now enforcing mono files. I have a check for that in aec_dump_based_simulator.cc : 221. Further on we should, however, probably also add support for stereo as well. https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:110: fwd_frame_.data_[k] += static_cast<int16_t>( On 2016/12/08 13:00:55, aleloi wrote: > What happens if the frame is stereo? Then frame audio is interleaved and > artificial nearend data is only added to the first half of the samples. There is another hidden assumption here as well, and that is the sample rate. It is assumed to be the same as the input. Verifying this in a proper way that makes sense is, however, quite tricky as the sample rate of the input may change during the call. Therefore I just assumed that the correct sample rate has been supplied. https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:111: 32767 * artificial_nearend_buf_->channels()[0][k]); On 2016/12/08 13:00:55, aleloi wrote: > Can we use a checked_cast + saturated_cast here? True. Good point! I added explicit controlled saturation instead as saturation is allowed (if done in a controlled manner. https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.h (right): https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.h:153: std::unique_ptr<ChannelBuffer<float>> artificial_nearend_buf_; On 2016/12/08 13:00:55, aleloi wrote: > The nearend buffer is only used in the aec-dump-based simulator. To limit the > scope in which they are visible, I think the buffer and reader should be private > in the derived class. Great point! I move this and artificial_nearend_buffer_reader_ into the aec_dump_based simulator class instead. Done. https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audioproc_float.cc (right): https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:419: processor.reset(new WavBasedSimulator(settings)); On 2016/12/08 13:00:55, aleloi wrote: > IIUC, artificial nearend functionality only works when there is an AEC dump. > There should probably be a few more error messages for that case. There is an error message on line 283 that triggers if an artificial nearend wav file is used when a wav file is specified for either the render or capture side, but you are probably correct that there could be more. Do you have any special case that you think should be covered? A .wav file -based simulation requires either a wav file to be specified for the render or capture input so I think that the error message above is sufficient for that.
On 2016/12/09 07:19:25, peah-webrtc wrote: > https://codereview.webrtc.org/2562593003/diff/1/webrtc/modules/audio_processi... > File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): > > https://codereview.webrtc.org/2562593003/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:111: 32767 * > artificial_nearend_buf_->channels()[0][k]); > On 2016/12/08 12:13:24, ivoc wrote: > > On 2016/12/08 12:02:26, peah-webrtc wrote: > > > On 2016/12/08 10:55:19, ivoc wrote: > > > > Shouldn't this be fwd_frame_.data[k]? Also, why is it adding instead of > > > > assigning? > > > > > > Great find (I probably have not used this yet with the int16 interface.) > > > > > > Regarding the adding, the idea is to add a nearend signal to the existing > > > microphone signal (which includes the echo which we want to maintain). > > Assigning > > > would replace the microphone signal. > > > > > > Done. > > > > I see, that makes sense. I think this code can cause over/underflows as well, > > should that be addressed, or is it not a problem in practice? > > You are fully correct! I added explicit controlled saturation. > > Done. > > https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): > > https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:105: bool > samples_left_to_process = > On 2016/12/08 13:00:55, aleloi wrote: > > const > > I changed the code to avoid the need for this variable. > > Done. > > https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:110: > fwd_frame_.data_[k] += static_cast<int16_t>( > On 2016/12/08 13:00:55, aleloi wrote: > > What happens if the frame is stereo? Then frame audio is interleaved and > > artificial nearend data is only added to the first half of the samples. > > That is a great point! I overcame that problem by now enforcing mono files. I > have a check for that in aec_dump_based_simulator.cc : 221. > Further on we should, however, probably also add support for stereo as well. > > https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:110: > fwd_frame_.data_[k] += static_cast<int16_t>( > On 2016/12/08 13:00:55, aleloi wrote: > > What happens if the frame is stereo? Then frame audio is interleaved and > > artificial nearend data is only added to the first half of the samples. > > There is another hidden assumption here as well, and that is the sample rate. It > is assumed to be the same as the input. Verifying this in a proper way that > makes sense is, however, quite tricky as the sample rate of the input may change > during the call. Therefore I just assumed that the correct sample rate has been > supplied. > > https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:111: 32767 * > artificial_nearend_buf_->channels()[0][k]); > On 2016/12/08 13:00:55, aleloi wrote: > > Can we use a checked_cast + saturated_cast here? > > True. Good point! > I added explicit controlled saturation instead as saturation is allowed (if done > in a controlled manner. > > https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/test/audio_processing_simulator.h (right): > > https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/test/audio_processing_simulator.h:153: > std::unique_ptr<ChannelBuffer<float>> artificial_nearend_buf_; > On 2016/12/08 13:00:55, aleloi wrote: > > The nearend buffer is only used in the aec-dump-based simulator. To limit the > > scope in which they are visible, I think the buffer and reader should be > private > > in the derived class. > > Great point! I move this and artificial_nearend_buffer_reader_ into the > aec_dump_based simulator class instead. > > Done. > > https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/test/audioproc_float.cc (right): > > https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/test/audioproc_float.cc:419: processor.reset(new > WavBasedSimulator(settings)); > On 2016/12/08 13:00:55, aleloi wrote: > > IIUC, artificial nearend functionality only works when there is an AEC dump. > > There should probably be a few more error messages for that case. > > There is an error message on line 283 that triggers if an artificial nearend wav > file is used when a wav file is specified for either the render or capture side, > but you are probably correct that there could be more. > > Do you have any special case that you think should be covered? A .wav file > -based simulation requires either a wav file to be specified for the render or > capture input so I think that the error message above is sufficient for that. The CL title is too long. Make it fit one line.
LG. A few comments. https://codereview.webrtc.org/2562593003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/2562593003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:105: if (artificial_nearend_buffer_reader_->Read( If the file ends before the rest of the simulation, we'll just silently not add any more artificial nerend. Is this your intention? I think it is more honest to the one running the test to kick and scream about it (essentially RTC_CHECK the call to Read). https://codereview.webrtc.org/2562593003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:113: static_cast<int16_t>(std::min(32767, std::max(-32768, tmp))); rtc::saturated_cast<int16_t>(tmp) will do the same thing. https://codereview.webrtc.org/2562593003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:225: if (artificial_nearend_file->num_channels() != 1) { Any point continuing here? Or simply RTC_DCHECK_EQ(artificial_nearend_file->num_channels(), 1) << "Oh no..."; https://codereview.webrtc.org/2562593003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audioproc_float.cc (right): https://codereview.webrtc.org/2562593003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:402: (!valid_wav_name(*settings.artificial_nearend_filename)), Skip the outer parentheses.
On 2016/12/09 07:29:54, hlundin-webrtc wrote: > On 2016/12/09 07:19:25, peah-webrtc wrote: > > > https://codereview.webrtc.org/2562593003/diff/1/webrtc/modules/audio_processi... > > File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): > > > > > https://codereview.webrtc.org/2562593003/diff/1/webrtc/modules/audio_processi... > > webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:111: 32767 * > > artificial_nearend_buf_->channels()[0][k]); > > On 2016/12/08 12:13:24, ivoc wrote: > > > On 2016/12/08 12:02:26, peah-webrtc wrote: > > > > On 2016/12/08 10:55:19, ivoc wrote: > > > > > Shouldn't this be fwd_frame_.data[k]? Also, why is it adding instead of > > > > > assigning? > > > > > > > > Great find (I probably have not used this yet with the int16 interface.) > > > > > > > > Regarding the adding, the idea is to add a nearend signal to the existing > > > > microphone signal (which includes the echo which we want to maintain). > > > Assigning > > > > would replace the microphone signal. > > > > > > > > Done. > > > > > > I see, that makes sense. I think this code can cause over/underflows as > well, > > > should that be addressed, or is it not a problem in practice? > > > > You are fully correct! I added explicit controlled saturation. > > > > Done. > > > > > https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... > > File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): > > > > > https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... > > webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:105: bool > > samples_left_to_process = > > On 2016/12/08 13:00:55, aleloi wrote: > > > const > > > > I changed the code to avoid the need for this variable. > > > > Done. > > > > > https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... > > webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:110: > > fwd_frame_.data_[k] += static_cast<int16_t>( > > On 2016/12/08 13:00:55, aleloi wrote: > > > What happens if the frame is stereo? Then frame audio is interleaved and > > > artificial nearend data is only added to the first half of the samples. > > > > That is a great point! I overcame that problem by now enforcing mono files. I > > have a check for that in aec_dump_based_simulator.cc : 221. > > Further on we should, however, probably also add support for stereo as well. > > > > > https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... > > webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:110: > > fwd_frame_.data_[k] += static_cast<int16_t>( > > On 2016/12/08 13:00:55, aleloi wrote: > > > What happens if the frame is stereo? Then frame audio is interleaved and > > > artificial nearend data is only added to the first half of the samples. > > > > There is another hidden assumption here as well, and that is the sample rate. > It > > is assumed to be the same as the input. Verifying this in a proper way that > > makes sense is, however, quite tricky as the sample rate of the input may > change > > during the call. Therefore I just assumed that the correct sample rate has > been > > supplied. > > > > > https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... > > webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:111: 32767 * > > artificial_nearend_buf_->channels()[0][k]); > > On 2016/12/08 13:00:55, aleloi wrote: > > > Can we use a checked_cast + saturated_cast here? > > > > True. Good point! > > I added explicit controlled saturation instead as saturation is allowed (if > done > > in a controlled manner. > > > > > https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... > > File webrtc/modules/audio_processing/test/audio_processing_simulator.h > (right): > > > > > https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... > > webrtc/modules/audio_processing/test/audio_processing_simulator.h:153: > > std::unique_ptr<ChannelBuffer<float>> artificial_nearend_buf_; > > On 2016/12/08 13:00:55, aleloi wrote: > > > The nearend buffer is only used in the aec-dump-based simulator. To limit > the > > > scope in which they are visible, I think the buffer and reader should be > > private > > > in the derived class. > > > > Great point! I move this and artificial_nearend_buffer_reader_ into the > > aec_dump_based simulator class instead. > > > > Done. > > > > > https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... > > File webrtc/modules/audio_processing/test/audioproc_float.cc (right): > > > > > https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... > > webrtc/modules/audio_processing/test/audioproc_float.cc:419: > processor.reset(new > > WavBasedSimulator(settings)); > > On 2016/12/08 13:00:55, aleloi wrote: > > > IIUC, artificial nearend functionality only works when there is an AEC dump. > > > There should probably be a few more error messages for that case. > > > > There is an error message on line 283 that triggers if an artificial nearend > wav > > file is used when a wav file is specified for either the render or capture > side, > > but you are probably correct that there could be more. > > > > Do you have any special case that you think should be covered? A .wav file > > -based simulation requires either a wav file to be specified for the render or > > capture input so I think that the error message above is sufficient for that. > > The CL title is too long. Make it fit one line. Good point! Done.
https://codereview.webrtc.org/2562593003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/2562593003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:105: if (artificial_nearend_buffer_reader_->Read( On 2016/12/09 07:48:26, hlundin-webrtc wrote: > If the file ends before the rest of the simulation, we'll just silently not add > any more artificial nerend. Is this your intention? I think it is more honest to > the one running the test to kick and scream about it (essentially RTC_CHECK the > call to Read). The intention was actually to do this silently but it can definitely be discussed whether that is the best way. My view is that this is a simulation/development tool and I don't think that it should limit the user in any unnecessary way. If we CHECK on this, that will force the user to adjust the length of the wav file just to comply with this restriction. I'll add a printout to warn about this, because I agree that it could be that it is not intentional for the .wav file to be that short. WDYT? https://codereview.webrtc.org/2562593003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:113: static_cast<int16_t>(std::min(32767, std::max(-32768, tmp))); On 2016/12/09 07:48:26, hlundin-webrtc wrote: > rtc::saturated_cast<int16_t>(tmp) will do the same thing. Great! That I did not know. Changed the code to use that. Done. https://codereview.webrtc.org/2562593003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:225: if (artificial_nearend_file->num_channels() != 1) { On 2016/12/09 07:48:26, hlundin-webrtc wrote: > Any point continuing here? Or simply > RTC_DCHECK_EQ(artificial_nearend_file->num_channels(), 1) << "Oh no..."; Good point! I added the DCHECK. Done. https://codereview.webrtc.org/2562593003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audioproc_float.cc (right): https://codereview.webrtc.org/2562593003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:402: (!valid_wav_name(*settings.artificial_nearend_filename)), On 2016/12/09 07:48:26, hlundin-webrtc wrote: > Skip the outer parentheses. Done.
lgtm with one comment. https://codereview.webrtc.org/2562593003/diff/60001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/2562593003/diff/60001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:105: if (artificial_nearend_buffer_reader_->Read( On 2016/12/09 08:07:51, peah-webrtc wrote: > On 2016/12/09 07:48:26, hlundin-webrtc wrote: > > If the file ends before the rest of the simulation, we'll just silently not > add > > any more artificial nerend. Is this your intention? I think it is more honest > to > > the one running the test to kick and scream about it (essentially RTC_CHECK > the > > call to Read). > > The intention was actually to do this silently but it can definitely be > discussed whether that is the best way. My view is that this is a > simulation/development tool and I don't think that it should limit the user in > any unnecessary way. If we CHECK on this, that will force the user to adjust the > length of the wav file just to comply with this restriction. > > I'll add a printout to warn about this, because I agree that it could be that it > is not intentional for the .wav file to be that short. > > WDYT? Good. https://codereview.webrtc.org/2562593003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/2562593003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:232: RTC_DCHECK_EQ(1, artificial_nearend_file->num_channels()); Sorry, I mean to write CHECK, not DCHECK. I think you can simply RTC_CHECK_EQ this instead of the if-else clause.
LGTM.
LGTM! https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audioproc_float.cc (right): https://codereview.webrtc.org/2562593003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audioproc_float.cc:419: processor.reset(new WavBasedSimulator(settings)); On 2016/12/09 07:19:25, peah-webrtc wrote: > On 2016/12/08 13:00:55, aleloi wrote: > > IIUC, artificial nearend functionality only works when there is an AEC dump. > > There should probably be a few more error messages for that case. > > There is an error message on line 283 that triggers if an artificial nearend wav > file is used when a wav file is specified for either the render or capture side, > but you are probably correct that there could be more. > > Do you have any special case that you think should be covered? A .wav file > -based simulation requires either a wav file to be specified for the render or > capture input so I think that the error message above is sufficient for that. Ah, I see now. When there is an artificial nearend, but no aec dump, the check on 283 will be hit.
Thanks for the review! https://codereview.webrtc.org/2562593003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/2562593003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:232: RTC_DCHECK_EQ(1, artificial_nearend_file->num_channels()); On 2016/12/09 09:24:26, hlundin-webrtc wrote: > Sorry, I mean to write CHECK, not DCHECK. I think you can simply RTC_CHECK_EQ > this instead of the if-else clause. Done.
Thanks for the review! https://codereview.webrtc.org/2562593003/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/2562593003/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:232: RTC_DCHECK_EQ(1, artificial_nearend_file->num_channels()); On 2016/12/09 09:24:26, hlundin-webrtc wrote: > Sorry, I mean to write CHECK, not DCHECK. I think you can simply RTC_CHECK_EQ > this instead of the if-else clause. Done.
The CQ bit was checked by peah@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org, henrik.lundin@webrtc.org, aleloi@webrtc.org Link to the patchset: https://codereview.webrtc.org/2562593003/#ps100001 (title: "Changed DCHECK to CHECK")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481278454786330,
"parent_rev": "1586164856a6733bb4cf1b7b6475dac653f5d3c0", "commit_rev":
"b0781adba79d14a64881e88447eedeadce7e7e30"}
Message was sent while issue was closed.
Description was changed from ========== When recreating a call based on an aecdump recording the nearend used is the one stored in the aecdump. During AEC development, it is handy to be able to simulate different doubletalk scenarios. This CL adds the ability to add an artificial nearend on top of that present in the aecdump, which allows for the developer to artificially customize the scenario being tested BUG=webrtc:6018 ========== to ========== When recreating a call based on an aecdump recording the nearend used is the one stored in the aecdump. During AEC development, it is handy to be able to simulate different doubletalk scenarios. This CL adds the ability to add an artificial nearend on top of that present in the aecdump, which allows for the developer to artificially customize the scenario being tested BUG=webrtc:6018 Review-Url: https://codereview.webrtc.org/2562593003 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== When recreating a call based on an aecdump recording the nearend used is the one stored in the aecdump. During AEC development, it is handy to be able to simulate different doubletalk scenarios. This CL adds the ability to add an artificial nearend on top of that present in the aecdump, which allows for the developer to artificially customize the scenario being tested BUG=webrtc:6018 Review-Url: https://codereview.webrtc.org/2562593003 ========== to ========== When recreating a call based on an aecdump recording the nearend used is the one stored in the aecdump. During AEC development, it is handy to be able to simulate different doubletalk scenarios. This CL adds the ability to add an artificial nearend on top of that present in the aecdump, which allows for the developer to artificially customize the scenario being tested BUG=webrtc:6018 Committed: https://crrev.com/df80fd125989fae30eff04e8413d63178d98c3e5 Cr-Commit-Position: refs/heads/master@{#15502} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/df80fd125989fae30eff04e8413d63178d98c3e5 Cr-Commit-Position: refs/heads/master@{#15502} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
