Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1766383002:
Convert IntelligibilityEnhancer to multi-threaded mode (Closed)
|
Created:
4 years, 9 months ago by aluebs-webrtc Modified:
4 years, 9 months ago CC:
webrtc-reviews_webrtc.org, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, bjornv1 Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionConvert IntelligibilityEnhancer to multi-threaded mode
BUG=581029
R=henrik.lundin@webrtc.org, peah@webrtc.org, turaj@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/57ae82929a73b0572953283632fb7dd9907ea94a
Patch Set 1 #
Total comments: 29
Patch Set 2 : Remove num_noise_bins from the public API #Patch Set 3 : Another way to suppress warning #
Total comments: 9
Patch Set 4 : Dont need a pointer to the queue #Patch Set 5 : Formatting #
Total comments: 4
Messages
Total messages: 34 (11 generated)
aluebs@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, peah@webrtc.org, turaj@webrtc.org
https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:1237: public_submodules_->noise_suppression->num_noise_bins())); num_noise_bins() seemed could be declared static, do you think it makes sense to have it static? https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:76: filtered_noise_pow_(num_noise_bins, 0.f), I suppose I'm missing a point, |filtered_noise_pow_| and |filtered_clear_pow_| are the ones used to compute IE gains, right? Then why are they of different sizes? https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:105: noise_estimation_buffer_.resize(num_noise_bins); Any particular reason that |noise_estimation_buffer_| is not initialized in initializer list?
https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/include/audio_processing.h:933: virtual size_t num_noise_bins() const = 0; I don't think we should put methods in here unless absolutely necessary. This API is part of the public APM API, which means that anything that is put here may be used outside of APM. We are trying to remove the submodule public API as it is very hard to maintain. This method could instead be put into NoiseSuppressionImpl as it is only used within APM, right? In that way it will not be publicly visible outside of APM. https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/include/mock_audio_processing.h (right): https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/include/mock_audio_processing.h:145: MOCK_CONST_METHOD0(num_noise_bins, size_t()); Please remove this (see comment in AudioProcessing.h). https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:110: RTC_CHECK_EQ(noise.size(), num_noise_bins_); Do we really want a CHECK here? I think a DCHECK should be preferred. https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:111: (void)noise_estimation_queue_->Insert(&noise); Please add a comment on why buffer overflow is acceptable. https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h (right): https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h:82: Please rename as it is not frames that are buffered, but rather noise spectra, right? https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h:82: The size of this buffer seems a bit high. The reason for the large sizes in the buffers for the AECs is that data must not be dropped there. But in this case that requirement is not that superimportant, right? (which is reflected on the fact that the code that inserts data into the buffer (correctly) does not take buffer overflows into account). I would have expected a 10-50 ms of data to be sufficient for the buffer as what is passed is estimates of the stationary part of the capture noise spectrum which should not change much (as it is stationary) which means that dropped data due to thread stalls should not at all be critical. Wdyt? https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc (right): https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:204: const size_t kNumNoiseBins = 129; This is a bit of a strange number of bins, I would have expected a power of 2. https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/noise_suppression_impl.h (right): https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/noise_suppression_impl.h:42: size_t num_noise_bins() const override; Remove the override (see comment in audio_processing.h)
Nice. See comments inline. https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/include/audio_processing.h:933: virtual size_t num_noise_bins() const = 0; On 2016/03/08 07:02:38, peah-webrtc wrote: > I don't think we should put methods in here unless absolutely necessary. This > API is part of the public APM API, which means that anything that is put here > may be used outside of APM. We are trying to remove the submodule public API as > it is very hard to maintain. > > This method could instead be put into NoiseSuppressionImpl as it is only used > within APM, right? In that way it will not be publicly visible outside of APM. Acknowledged. https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:110: RTC_CHECK_EQ(noise.size(), num_noise_bins_); On 2016/03/08 07:02:38, peah-webrtc wrote: > Do we really want a CHECK here? I think a DCHECK should be preferred. Acknowledged. https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/noise_suppression_impl.cc (right): https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/noise_suppression_impl.cc:205: size_t num_freq = 0u; Exactly one of WEBRTC_NS_FLOAT and _FIXED must be defined, right? If so, simplify this function: { rtc::CritScope cs(crit_); #if defined(WEBRTC_NS_FLOAT) return WebRtcNs_num_freq(); #elif defined(WEBRTC_NS_FIXED) return WebRtcNsx_num_freq(); #else static_assert(false, "You are doing it wrong.") #endif } (If you don't want to verify that exactly one of the defines are set, skip the static assert and make the #elif a simple #else.)
https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/noise_suppression_impl.cc (right): https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/noise_suppression_impl.cc:205: size_t num_freq = 0u; On 2016/03/08 10:28:48, hlundin-webrtc wrote: > Exactly one of WEBRTC_NS_FLOAT and _FIXED must be defined, right? If so, > simplify this function: > > { > rtc::CritScope cs(crit_); > #if defined(WEBRTC_NS_FLOAT) > return WebRtcNs_num_freq(); > #elif defined(WEBRTC_NS_FIXED) > return WebRtcNsx_num_freq(); > #else > static_assert(false, "You are doing it wrong.") > #endif > } > > (If you don't want to verify that exactly one of the defines are set, skip the > static assert and make the #elif a simple #else.) This still doesn't fail if both are defined. Also, it's customary to use #error and not static_assert(false) in situations like this, both for historical reasons and because it's less complicated and works in more places.
https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_impl.cc:1237: public_submodules_->noise_suppression->num_noise_bins())); On 2016/03/07 20:34:35, turaj wrote: > num_noise_bins() seemed could be declared static, do you think it makes sense to > have it static? Great point. Done. https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/include/audio_processing.h:933: virtual size_t num_noise_bins() const = 0; On 2016/03/08 07:02:38, peah-webrtc wrote: > I don't think we should put methods in here unless absolutely necessary. This > API is part of the public APM API, which means that anything that is put here > may be used outside of APM. We are trying to remove the submodule public API as > it is very hard to maintain. > > This method could instead be put into NoiseSuppressionImpl as it is only used > within APM, right? In that way it will not be publicly visible outside of APM. Good point. Done. https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/include/mock_audio_processing.h (right): https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/include/mock_audio_processing.h:145: MOCK_CONST_METHOD0(num_noise_bins, size_t()); On 2016/03/08 07:02:38, peah-webrtc wrote: > Please remove this (see comment in AudioProcessing.h). Done. https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:76: filtered_noise_pow_(num_noise_bins, 0.f), On 2016/03/07 20:34:35, turaj wrote: > I suppose I'm missing a point, |filtered_noise_pow_| and |filtered_clear_pow_| > are the ones used to compute IE gains, right? Then why are they of different > sizes? They can (and have) different sizes, since one is the psd of the speech calculated internally and the other is the size of the noise estimates calculated by the noise suppressor and passed in. This mismatch is fixed when mapping them to critical bands. https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:105: noise_estimation_buffer_.resize(num_noise_bins); On 2016/03/07 20:34:35, turaj wrote: > Any particular reason that |noise_estimation_buffer_| is not initialized in > initializer list? No really. Changed. https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:110: RTC_CHECK_EQ(noise.size(), num_noise_bins_); On 2016/03/08 07:02:38, peah-webrtc wrote: > Do we really want a CHECK here? I think a DCHECK should be preferred. Agreed. Done. https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:111: (void)noise_estimation_queue_->Insert(&noise); On 2016/03/08 07:02:38, peah-webrtc wrote: > Please add a comment on why buffer overflow is acceptable. Done. https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h (right): https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h:82: On 2016/03/08 07:02:38, peah-webrtc wrote: > The size of this buffer seems a bit high. The reason for the large sizes in the > buffers for the AECs is that data must not be dropped there. > But in this case that requirement is not that superimportant, right? (which is > reflected on the fact that the code that inserts data into the buffer > (correctly) does not take buffer overflows into account). > > I would have expected a 10-50 ms of data to be sufficient for the buffer as what > is passed is estimates of the stationary part of the capture noise spectrum > which should not change much (as it is stationary) which means that dropped data > due to thread stalls should not at all be critical. > > Wdyt? That makes a lot of sense. I just copied and pasted without much thought on the actual value. https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h:82: On 2016/03/08 07:02:38, peah-webrtc wrote: > Please rename as it is not frames that are buffered, but rather noise spectra, > right? Done. https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc (right): https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:204: const size_t kNumNoiseBins = 129; On 2016/03/08 07:02:39, peah-webrtc wrote: > This is a bit of a strange number of bins, I would have expected a power of 2. This is only a unittest and shouldn't affect at all, but I chose that number since it is the number of noise bins in the NS. It isn't a strange number at all, it is the number of frequency bins (N/2+1) for a 256 block length. https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/noise_suppression_impl.cc (right): https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/noise_suppression_impl.cc:205: size_t num_freq = 0u; On 2016/03/08 10:40:22, kwiberg-webrtc wrote: > On 2016/03/08 10:28:48, hlundin-webrtc wrote: > > Exactly one of WEBRTC_NS_FLOAT and _FIXED must be defined, right? If so, > > simplify this function: > > > > { > > rtc::CritScope cs(crit_); > > #if defined(WEBRTC_NS_FLOAT) > > return WebRtcNs_num_freq(); > > #elif defined(WEBRTC_NS_FIXED) > > return WebRtcNsx_num_freq(); > > #else > > static_assert(false, "You are doing it wrong.") > > #endif > > } > > > > (If you don't want to verify that exactly one of the defines are set, skip the > > static assert and make the #elif a simple #else.) > > This still doesn't fail if both are defined. > > Also, it's customary to use #error and not static_assert(false) in situations > like this, both for historical reasons and because it's less complicated and > works in more places. In general I think the functions with only one return statement are less bug prone, but I can understand the argument for simplicity, so I accepted your suggestion. I am not adding the verification, because, as kwiberg pointed out, it doesn't fail if both are defined and because the whole file assumes one of them to be defined (and that is how they are defined in audio_processing.gypi). But if you think I should add this additional verification, I am happy to do so. https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/noise_suppression_impl.h (right): https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/noise_suppression_impl.h:42: size_t num_noise_bins() const override; On 2016/03/08 07:02:39, peah-webrtc wrote: > Remove the override (see comment in audio_processing.h) Done.
lgtm https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/noise_suppression_impl.cc (right): https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/noise_suppression_impl.cc:205: size_t num_freq = 0u; On 2016/03/08 10:53:00, aluebs-webrtc wrote: > On 2016/03/08 10:40:22, kwiberg-webrtc wrote: > > On 2016/03/08 10:28:48, hlundin-webrtc wrote: > > > Exactly one of WEBRTC_NS_FLOAT and _FIXED must be defined, right? If so, > > > simplify this function: > > > > > > { > > > rtc::CritScope cs(crit_); > > > #if defined(WEBRTC_NS_FLOAT) > > > return WebRtcNs_num_freq(); > > > #elif defined(WEBRTC_NS_FIXED) > > > return WebRtcNsx_num_freq(); > > > #else > > > static_assert(false, "You are doing it wrong.") > > > #endif > > > } > > > > > > (If you don't want to verify that exactly one of the defines are set, skip > the > > > static assert and make the #elif a simple #else.) > > > > This still doesn't fail if both are defined. > > > > Also, it's customary to use #error and not static_assert(false) in situations > > like this, both for historical reasons and because it's less complicated and > > works in more places. > > In general I think the functions with only one return statement are less bug > prone, but I can understand the argument for simplicity, so I accepted your > suggestion. I am not adding the verification, because, as kwiberg pointed out, > it doesn't fail if both are defined and because the whole file assumes one of > them to be defined (and that is how they are defined in audio_processing.gypi). > But if you think I should add this additional verification, I am happy to do so. This is good. Don't add any verification. This is not the place for it. (And I think you will get implicit verification that at least one is defined the way you wrote it now.)
On 2016/03/08 13:35:20, hlundin-webrtc wrote: > lgtm > > https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... > File webrtc/modules/audio_processing/noise_suppression_impl.cc (right): > > https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... > webrtc/modules/audio_processing/noise_suppression_impl.cc:205: size_t num_freq = > 0u; > On 2016/03/08 10:53:00, aluebs-webrtc wrote: > > On 2016/03/08 10:40:22, kwiberg-webrtc wrote: > > > On 2016/03/08 10:28:48, hlundin-webrtc wrote: > > > > Exactly one of WEBRTC_NS_FLOAT and _FIXED must be defined, right? If so, > > > > simplify this function: > > > > > > > > { > > > > rtc::CritScope cs(crit_); > > > > #if defined(WEBRTC_NS_FLOAT) > > > > return WebRtcNs_num_freq(); > > > > #elif defined(WEBRTC_NS_FIXED) > > > > return WebRtcNsx_num_freq(); > > > > #else > > > > static_assert(false, "You are doing it wrong.") > > > > #endif > > > > } > > > > > > > > (If you don't want to verify that exactly one of the defines are set, skip > > the > > > > static assert and make the #elif a simple #else.) > > > > > > This still doesn't fail if both are defined. > > > > > > Also, it's customary to use #error and not static_assert(false) in > situations > > > like this, both for historical reasons and because it's less complicated and > > > works in more places. > > > > In general I think the functions with only one return statement are less bug > > prone, but I can understand the argument for simplicity, so I accepted your > > suggestion. I am not adding the verification, because, as kwiberg pointed out, > > it doesn't fail if both are defined and because the whole file assumes one of > > them to be defined (and that is how they are defined in > audio_processing.gypi). > > But if you think I should add this additional verification, I am happy to do > so. > > This is good. Don't add any verification. This is not the place for it. (And I > think you will get implicit verification that at least one is defined the way > you wrote it now.) Great! I think the message queue passing is correct now, and from an APM perspective it looks like everything is fine threadwise. However, I would advice that further code refactoring is done inside IE in order to ensure thread safety. For the other subcomponents this was enforced as much as possible through 1) locks that anyway needed to be passed to the subcomponents due to their public APIs. 2) only having one single function which stores the render data so that it is passed to the capture side. The IE is different in that it have no locks (which is a good thing) and that it actually does substantial processing on both the render and capture sides. Therefore I think some measure must be taken to enforce thread safety before the IE is properly released. An example of how to do this would be to split the IE into two separate classes without any shared memory, one that only does render processing and one that only does capture processing. These could then be communicating via the SwapQueue. Another example is to use threadcheckers. Since the IE is still being developed and is not yet released I give an lgtm conditioned on that for the final release version some additional measure must be taken to ensure/enforce thread-safety inside the IE. lgtm lgtm for the message queue passing
On 2016/03/08 14:01:29, peah-webrtc wrote: > On 2016/03/08 13:35:20, hlundin-webrtc wrote: > > lgtm > > > > > https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... > > File webrtc/modules/audio_processing/noise_suppression_impl.cc (right): > > > > > https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... > > webrtc/modules/audio_processing/noise_suppression_impl.cc:205: size_t num_freq > = > > 0u; > > On 2016/03/08 10:53:00, aluebs-webrtc wrote: > > > On 2016/03/08 10:40:22, kwiberg-webrtc wrote: > > > > On 2016/03/08 10:28:48, hlundin-webrtc wrote: > > > > > Exactly one of WEBRTC_NS_FLOAT and _FIXED must be defined, right? If so, > > > > > simplify this function: > > > > > > > > > > { > > > > > rtc::CritScope cs(crit_); > > > > > #if defined(WEBRTC_NS_FLOAT) > > > > > return WebRtcNs_num_freq(); > > > > > #elif defined(WEBRTC_NS_FIXED) > > > > > return WebRtcNsx_num_freq(); > > > > > #else > > > > > static_assert(false, "You are doing it wrong.") > > > > > #endif > > > > > } > > > > > > > > > > (If you don't want to verify that exactly one of the defines are set, > skip > > > the > > > > > static assert and make the #elif a simple #else.) > > > > > > > > This still doesn't fail if both are defined. > > > > > > > > Also, it's customary to use #error and not static_assert(false) in > > situations > > > > like this, both for historical reasons and because it's less complicated > and > > > > works in more places. > > > > > > In general I think the functions with only one return statement are less bug > > > prone, but I can understand the argument for simplicity, so I accepted your > > > suggestion. I am not adding the verification, because, as kwiberg pointed > out, > > > it doesn't fail if both are defined and because the whole file assumes one > of > > > them to be defined (and that is how they are defined in > > audio_processing.gypi). > > > But if you think I should add this additional verification, I am happy to do > > so. > > > > This is good. Don't add any verification. This is not the place for it. (And I > > think you will get implicit verification that at least one is defined the way > > you wrote it now.) > > Great! > I think the message queue passing is correct now, and from an APM perspective it > looks like everything is fine threadwise. > However, I would advice that further code refactoring is done inside IE in order > to ensure thread safety. For the other subcomponents this was enforced as much > as possible through > 1) locks that anyway needed to be passed to the subcomponents due to their > public APIs. > 2) only having one single function which stores the render data so that it is > passed to the capture side. > > The IE is different in that it have no locks (which is a good thing) and that it > actually does substantial processing on both the render and capture sides. > Therefore I think some measure must be taken to enforce thread safety before the > IE is properly released. An example of how to do this would be to split the IE > into two separate classes without any shared memory, one that only does render > processing and one that only does capture processing. These could then be > communicating via the SwapQueue. Another example is to use threadcheckers. > > Since the IE is still being developed and is not yet released I give an lgtm > conditioned on that for the final release version some additional measure must > be taken to ensure/enforce thread-safety inside the IE. > > lgtm > > > > > lgtm for the message queue passing turaj, any additional comments? peah, now the IE does no processing on the capture side, only pushing the noise estimate on the queue. But even before than that it was only receiving the noise estimate and updating the psd in a exponential way. We still want to make sure that in the future nobody will add shared memory between render and capture sides by mistake, but I think that splitting into 2 classes would just increase the code complexity too much. In any case, we can look into it in another CL.
lgtm https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1766383002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:76: filtered_noise_pow_(num_noise_bins, 0.f), On 2016/03/08 10:53:00, aluebs-webrtc wrote: > On 2016/03/07 20:34:35, turaj wrote: > > I suppose I'm missing a point, |filtered_noise_pow_| and |filtered_clear_pow_| > > are the ones used to compute IE gains, right? Then why are they of different > > sizes? > > They can (and have) different sizes, since one is the psd of the speech > calculated internally and the other is the size of the noise estimates > calculated by the noise suppressor and passed in. This mismatch is fixed when > mapping them to critical bands. Thanks for the explanation.
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/1766383002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1766383002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_x64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...) android_compile_x86_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...) linux_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/13056)
solenberg@webrtc.org changed reviewers: + solenberg@webrtc.org
Drive-by comments https://codereview.webrtc.org/1766383002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1766383002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:102: noise_estimation_queue_.reset( Can you make to without the scoped_ptr, given this is instantiated in ctor? If you decide you must have the extra indirection, use a unique_ptr instead. https://codereview.webrtc.org/1766383002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:113: if(noise_estimation_queue_->Insert(&noise)) {}; Something missing here? https://codereview.webrtc.org/1766383002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h (right): https://codereview.webrtc.org/1766383002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h:22: #include "webrtc/modules/audio_processing/processing_component.h" Not needed
https://codereview.webrtc.org/1766383002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1766383002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:102: noise_estimation_queue_.reset( On 2016/03/09 09:02:55, the sun wrote: > Can you make to without the scoped_ptr, given this is instantiated in ctor? > > If you decide you must have the extra indirection, use a unique_ptr instead. Good point. Done. https://codereview.webrtc.org/1766383002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:113: if(noise_estimation_queue_->Insert(&noise)) {}; On 2016/03/09 09:02:55, the sun wrote: > Something missing here? No, that is a way of making all compilers happy and not complain about the warn_unused_result warning (casting to void didn't do the trick for android) and was tried to be explained in the comment above. https://codereview.webrtc.org/1766383002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h (right): https://codereview.webrtc.org/1766383002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h:22: #include "webrtc/modules/audio_processing/processing_component.h" On 2016/03/09 09:02:55, the sun wrote: > Not needed Yes it is, for the RenderQueueItemVerifier. I am aware there is a CL of peah removing the ProcessingComponent, but this verifier gets its own header. This is going to need an update when that lands.
solenberg@webrtc.org changed reviewers: - solenberg@webrtc.org
https://codereview.webrtc.org/1766383002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1766383002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:113: if(noise_estimation_queue_->Insert(&noise)) {}; On 2016/03/09 12:18:50, aluebs-webrtc wrote: > On 2016/03/09 09:02:55, the sun wrote: > > Something missing here? > > No, that is a way of making all compilers happy and not complain about the > warn_unused_result warning (casting to void didn't do the trick for android) and > was tried to be explained in the comment above. Ah, I need to learn how to read. :) nit: space between if and ( https://codereview.webrtc.org/1766383002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h (right): https://codereview.webrtc.org/1766383002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h:22: #include "webrtc/modules/audio_processing/processing_component.h" On 2016/03/09 12:18:50, aluebs-webrtc wrote: > On 2016/03/09 09:02:55, the sun wrote: > > Not needed > > Yes it is, for the RenderQueueItemVerifier. I am aware there is a CL of peah > removing the ProcessingComponent, but this verifier gets its own header. This is > going to need an update when that lands. Ah, right.
The CQ bit was checked by aluebs@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, peah@webrtc.org, turaj@webrtc.org Link to the patchset: https://codereview.webrtc.org/1766383002/#ps80001 (title: "Formatting")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1766383002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1766383002/80001
https://codereview.webrtc.org/1766383002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1766383002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:113: if(noise_estimation_queue_->Insert(&noise)) {}; On 2016/03/09 12:28:53, the sun wrote: > On 2016/03/09 12:18:50, aluebs-webrtc wrote: > > On 2016/03/09 09:02:55, the sun wrote: > > > Something missing here? > > > > No, that is a way of making all compilers happy and not complain about the > > warn_unused_result warning (casting to void didn't do the trick for android) > and > > was tried to be explained in the comment above. > > Ah, I need to learn how to read. :) > > nit: space between if and ( Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/9583)
Message was sent while issue was closed.
Description was changed from ========== Convert IntelligibilityEnhancer to multi-threaded mode BUG=581029 ========== to ========== Convert IntelligibilityEnhancer to multi-threaded mode BUG=581029 R=henrik.lundin@webrtc.org, peah@webrtc.org, turaj@webrtc.org Committed: https://crrev.com/57ae82929a73b0572953283632fb7dd9907ea94a Cr-Commit-Position: refs/heads/master@{#11929} ==========
Message was sent while issue was closed.
Description was changed from ========== Convert IntelligibilityEnhancer to multi-threaded mode BUG=581029 ========== to ========== Convert IntelligibilityEnhancer to multi-threaded mode BUG=581029 R=henrik.lundin@webrtc.org, peah@webrtc.org, turaj@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/57ae82929a73b057295328363... ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/57ae82929a73b0572953283632fb7dd9907ea94a Cr-Commit-Position: refs/heads/master@{#11929}
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 57ae82929a73b0572953283632fb7dd9907ea94a (presubmit successful).
Message was sent while issue was closed.
tommi@chromium.org changed reviewers: + tommi@chromium.org
Message was sent while issue was closed.
drive by https://codereview.webrtc.org/1766383002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1766383002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:111: }; nit: remove the if() and then you can leave in the semicolon ;) https://codereview.webrtc.org/1766383002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/noise_suppression_impl.h (right): https://codereview.webrtc.org/1766383002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/noise_suppression_impl.h:42: static size_t num_noise_bins(); This should be GetNumNoiseBins(). Also, please separate it from the non-static methods.
Message was sent while issue was closed.
https://codereview.webrtc.org/1766383002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc (right): https://codereview.webrtc.org/1766383002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:111: }; On 2016/03/10 21:59:12, tommi-chromium wrote: > nit: remove the if() and then you can leave in the semicolon ;) That is a way of making all compilers happy and not complain about the warn_unused_result warning (casting to void didn't do the trick for android) and was tried to be explained in the comment above. https://codereview.webrtc.org/1766383002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/noise_suppression_impl.h (right): https://codereview.webrtc.org/1766383002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/noise_suppression_impl.h:42: static size_t num_noise_bins(); On 2016/03/10 21:59:12, tommi-chromium wrote: > This should be GetNumNoiseBins(). Also, please separate it from the non-static > methods. I thought getters were "cheap" and could be named with lowercase_and_underscore names. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
