Fix the stereo support in IntelligibilityEnhancer

Fix the stereo support in IntelligibilityEnhancer

Committed: https://crrev.com/0a007597803efe16b1a427256f2039eea3f53e4c
Cr-Commit-Position: refs/heads/master@{#11795}

[aluebs-webrtc, 2016-02-24 02:23:05]

aluebs@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org, turaj@webrtc.org

[hlundin-webrtc, 2016-02-24 10:17:03]

Nice. I have a few comments.

https://codereview.webrtc.org/1729753003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc
(right):

https://codereview.webrtc.org/1729753003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:131:
&filtered_clear_pow_[0]);
I suggest you use .data() instead of &...[0]. See
http://en.cppreference.com/w/cpp/container/vector/data. This is the preferred
way in C++11.

Several places below, too.

https://codereview.webrtc.org/1729753003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.h
(right):

https://codereview.webrtc.org/1729753003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/intelligibility/intelligibility_utils.h:57:
float* target() { return &target_[0]; }
.data()

https://codereview.webrtc.org/1729753003/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/intelligibility/test/intelligibility_proc.cc
(right):

https://codereview.webrtc.org/1729753003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/intelligibility/test/intelligibility_proc.cc:47:
const size_t noise_samples = noise_file.sample_rate() / 100;
What if the sample rates differ? Is that allowed and handled?

https://codereview.webrtc.org/1729753003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/intelligibility/test/intelligibility_proc.cc:57:
while (in_file.ReadSamples(in.size(), &in[0]) == in.size() &&
in.data()
several places below

https://codereview.webrtc.org/1729753003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/intelligibility/test/intelligibility_proc.cc:58:
noise_file.ReadSamples(noise.size(), &noise[0]) == noise.size()) {
noise.data()
several places below

[turaj, 2016-02-24 16:00:17]

lgtm

https://codereview.webrtc.org/1729753003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc
(right):

https://codereview.webrtc.org/1729753003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:126:
clear_power_estimator_.Step(in_block[0]);
I suppose this change and changes in lines 146-148 are highlights of this CL,
the reset is refactoring, right?

https://codereview.webrtc.org/1729753003/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/intelligibility/test/intelligibility_proc.cc
(right):

https://codereview.webrtc.org/1729753003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/intelligibility/test/intelligibility_proc.cc:60:
FloatS16ToFloat(&noise[0], noise.size(), &noise[0]);
NOT RELATED TO THIS CL--Is is guaranteed that in practice near end and far end
signals to APM are in the same range? I guess no other module requires this.

[aluebs-webrtc, 2016-02-25 00:18:37]

https://codereview.webrtc.org/1729753003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc
(right):

https://codereview.webrtc.org/1729753003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:126:
clear_power_estimator_.Step(in_block[0]);
On 2016/02/24 16:00:17, turaj wrote:
> I suppose this change and changes in lines 146-148 are highlights of this CL,
> the reset is refactoring, right?

Yes, as you point out, the main change is removing the callback class and make
the IE directly be the callback, only estimate the power once per chunk and
apply the same gain to all channels. I probably should have done 2 separate CLs
now that I look at it. Sorry about that.

https://codereview.webrtc.org/1729753003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:131:
&filtered_clear_pow_[0]);
On 2016/02/24 10:17:03, hlundin-webrtc wrote:
> I suggest you use .data() instead of &...[0]. See
> http://en.cppreference.com/w/cpp/container/vector/data. This is the preferred
> way in C++11.
> 
> Several places below, too.

I am aware of that, but I thought it was still not allowed in Chromium/WebRTC.
Now I checked and it is: https://chromium-cpp.appspot.com/. Changed to use that
method in all places.

https://codereview.webrtc.org/1729753003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.h
(right):

https://codereview.webrtc.org/1729753003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/intelligibility/intelligibility_utils.h:57:
float* target() { return &target_[0]; }
On 2016/02/24 10:17:03, hlundin-webrtc wrote:
> .data()

Done.

https://codereview.webrtc.org/1729753003/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/intelligibility/test/intelligibility_proc.cc
(right):

https://codereview.webrtc.org/1729753003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/intelligibility/test/intelligibility_proc.cc:47:
const size_t noise_samples = noise_file.sample_rate() / 100;
On 2016/02/24 10:17:03, hlundin-webrtc wrote:
> What if the sample rates differ? Is that allowed and handled?

Yes it is, since the sample rate of the noise only matters to the NS and the one
of the input only matters to the IE.

https://codereview.webrtc.org/1729753003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/intelligibility/test/intelligibility_proc.cc:57:
while (in_file.ReadSamples(in.size(), &in[0]) == in.size() &&
On 2016/02/24 10:17:03, hlundin-webrtc wrote:
> in.data()
> several places below

Done.

https://codereview.webrtc.org/1729753003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/intelligibility/test/intelligibility_proc.cc:58:
noise_file.ReadSamples(noise.size(), &noise[0]) == noise.size()) {
On 2016/02/24 10:17:03, hlundin-webrtc wrote:
> noise.data()
> several places below

Done.

https://codereview.webrtc.org/1729753003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/intelligibility/test/intelligibility_proc.cc:60:
FloatS16ToFloat(&noise[0], noise.size(), &noise[0]);
On 2016/02/24 16:00:17, turaj wrote:
> NOT RELATED TO THIS CL--Is is guaranteed that in practice near end and far end
> signals to APM are in the same range? I guess no other module requires this. 

That is a good question. Testing with audioproc they are and I want to hope this
holds for all platforms. It should, since in an aecdump the render and capture
signals are in the same range, right?

[aluebs-webrtc, 2016-02-26 19:12:35]

Henrik, could you please take another look? Thank you very much.

[hlundin-webrtc, 2016-02-26 21:58:57]

lgtm

https://codereview.webrtc.org/1729753003/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/intelligibility/test/intelligibility_proc.cc
(right):

https://codereview.webrtc.org/1729753003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/intelligibility/test/intelligibility_proc.cc:47:
const size_t noise_samples = noise_file.sample_rate() / 100;
On 2016/02/25 00:18:37, aluebs-webrtc wrote:
> On 2016/02/24 10:17:03, hlundin-webrtc wrote:
> > What if the sample rates differ? Is that allowed and handled?
> 
> Yes it is, since the sample rate of the noise only matters to the NS and the
one
> of the input only matters to the IE.

Acknowledged.

[aluebs-webrtc, 2016-02-26 23:58:46]

The CQ bit was checked by aluebs@webrtc.org

[aluebs-webrtc, 2016-02-26 23:58:46]

The patchset sent to the CQ was uploaded after l-g-t-m from turaj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1729753003/#ps20001 (title:
"Use data access of vector")

[commit-bot: I haz the power, 2016-02-26 23:58:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1729753003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1729753003/20001

[commit-bot: I haz the power, 2016-02-27 00:00:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-27 00:00:38]

Exceeded global retry quota

[aluebs-webrtc, 2016-02-27 00:20:37]

The CQ bit was checked by aluebs@webrtc.org

[aluebs-webrtc, 2016-02-27 00:20:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org, turaj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1729753003/#ps40001 (title:
"Rebasing")

[commit-bot: I haz the power, 2016-02-27 00:20:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1729753003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1729753003/40001

[commit-bot: I haz the power, 2016-02-27 01:17:45]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-27 01:17:47]

Description was changed from

==========
Fix the stereo support in IntelligibilityEnhancer
==========

to

==========
Fix the stereo support in IntelligibilityEnhancer

Committed: https://crrev.com/0a007597803efe16b1a427256f2039eea3f53e4c
Cr-Commit-Position: refs/heads/master@{#11795}
==========

[commit-bot: I haz the power, 2016-02-27 01:17:48]

Patchset 3 (id:??) landed as
https://crrev.com/0a007597803efe16b1a427256f2039eea3f53e4c
Cr-Commit-Position: refs/heads/master@{#11795}