Avoid reporting 0 as input level when AudioProcessing zero out the audio data

Avoid reporting 0 as input level when AudioProcessing zero out the audio data.

When the microphone volume is low, the pre-processed audio data contains energy, while the post-processed audio data has only 0, we report 0 as energy before this patch, which is wrong since the audio flow contains energy.

This patch fix the problem by checking the energy of pre-processed data before reporting 0 as energy.

BUG=424149
TEST=1, use a USB headset.
2, set the microphone volume low, like below 50%.
3, join a hangout call.
4, open chrome://webrtc-internals, and look at the energy of input stream, constantly it is 0 there.

Committed: https://crrev.com/ba5b0d78589ce6b41bade5f71f5f9657c2beef3d
Cr-Commit-Position: refs/heads/master@{#300274}

[no longer working on chromium, 2014-10-16 12:59:18]

xians@chromium.org changed reviewers:
+ tommi@chromium.org

[no longer working on chromium, 2014-10-16 12:59:33]

Tommi, please review, thanks.

[no longer working on chromium, 2014-10-17 08:37:58]

Tommi, please take a look.

[tommi (sloooow) - chröme, 2014-10-17 11:59:08]

https://codereview.chromium.org/661693003/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_level_calculator.h (right):

https://codereview.chromium.org/661693003/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_audio_level_calculator.h:25: int
number_of_frames, bool has_energy);
can you add documentation for the new parameter?  It's got a very special
purpose so I think it's important to make it clear.

I also think that the name of the parameter is misleading.  In most call sites,
you're passing false now, which will be interpreted as "the input audio data has
no energy" and that's not correct.  I think we should go with something like
force_nonzero_energy or perhaps always_assume_energy.

Btw, just looking at the prototype for this function now, I would wonder why
this flag isn't simply calculated inside the implementation of Calculate since
the audio_data is being passed to the function.  So why the parameter is needed
separate of the data, also needs to be documented.

[no longer working on chromium, 2014-10-17 14:57:43]

Hi Tommi, how about force_report_nonzero_energy ?
On Oct 17, 2014 1:59 PM, <tommi@chromium.org> wrote:

>
> https://codereview.chromium.org/661693003/diff/40001/
> content/renderer/media/media_stream_audio_level_calculator.h
> File content/renderer/media/media_stream_audio_level_calculator.h
> (right):
>
> https://codereview.chromium.org/661693003/diff/40001/
> content/renderer/media/media_stream_audio_level_calculator.h#newcode25
> content/renderer/media/media_stream_audio_level_calculator.h:25: int
> number_of_frames, bool has_energy);
> can you add documentation for the new parameter?  It's got a very
> special purpose so I think it's important to make it clear.
>
> I also think that the name of the parameter is misleading.  In most call
> sites, you're passing false now, which will be interpreted as "the input
> audio data has no energy" and that's not correct.  I think we should go
> with something like force_nonzero_energy or perhaps
> always_assume_energy.
>
> Btw, just looking at the prototype for this function now, I would wonder
> why this flag isn't simply calculated inside the implementation of
> Calculate since the audio_data is being passed to the function.  So why
> the parameter is needed separate of the data, also needs to be
> documented.
>
> https://codereview.chromium.org/661693003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tommi (sloooow) - chröme, 2014-10-17 15:47:57]

Sure. As long as it's also documented well, that should make things clear.

On Fri Oct 17 2014 at 4:57:42 PM Shijing Xian <xians@chromium.org> wrote:

> Hi Tommi, how about force_report_nonzero_energy ?
> On Oct 17, 2014 1:59 PM, <tommi@chromium.org> wrote:
>
>>
>> https://codereview.chromium.org/661693003/diff/40001/
>> content/renderer/media/media_stream_audio_level_calculator.h
>> File content/renderer/media/media_stream_audio_level_calculator.h
>> (right):
>>
>> https://codereview.chromium.org/661693003/diff/40001/
>> content/renderer/media/media_stream_audio_level_calculator.h#newcode25
>> content/renderer/media/media_stream_audio_level_calculator.h:25: int
>> number_of_frames, bool has_energy);
>> can you add documentation for the new parameter?  It's got a very
>> special purpose so I think it's important to make it clear.
>>
>> I also think that the name of the parameter is misleading.  In most call
>> sites, you're passing false now, which will be interpreted as "the input
>> audio data has no energy" and that's not correct.  I think we should go
>> with something like force_nonzero_energy or perhaps
>> always_assume_energy.
>>
>> Btw, just looking at the prototype for this function now, I would wonder
>> why this flag isn't simply calculated inside the implementation of
>> Calculate since the audio_data is being passed to the function.  So why
>> the parameter is needed separate of the data, also needs to be
>> documented.
>>
>> https://codereview.chromium.org/661693003/
>>
>  To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[no longer working on chromium, 2014-10-20 08:08:42]

Tommi, could you please look at the name of the variable and comment?

Thanks,
SX

https://codereview.chromium.org/661693003/diff/40001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_level_calculator.h (right):

https://codereview.chromium.org/661693003/diff/40001/content/renderer/media/m...
content/renderer/media/media_stream_audio_level_calculator.h:25: int
number_of_frames, bool has_energy);
On 2014/10/17 11:59:08, tommi wrote:
> can you add documentation for the new parameter?  It's got a very special
> purpose so I think it's important to make it clear.
> 
> I also think that the name of the parameter is misleading.  In most call
sites,
> you're passing false now, which will be interpreted as "the input audio data
has
> no energy" and that's not correct.  I think we should go with something like
> force_nonzero_energy or perhaps always_assume_energy.
> 
> Btw, just looking at the prototype for this function now, I would wonder why
> this flag isn't simply calculated inside the implementation of Calculate since
> the audio_data is being passed to the function.  So why the parameter is
needed
> separate of the data, also needs to be documented.

All done, please take a look.
Thanks.

[tommi (sloooow) - chröme, 2014-10-20 15:40:17]

lgtm % one nit

https://codereview.chromium.org/661693003/diff/60001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_level_calculator.h (right):

https://codereview.chromium.org/661693003/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_audio_level_calculator.h:25: // report
nonzero energy. Since |audio_data| is post processed data, and the
// report nonzero energy even if the energy of the processed audio is zero.
<etc>

[no longer working on chromium, 2014-10-20 15:44:37]

Patchset #5 (id:80001) has been deleted

[no longer working on chromium, 2014-10-20 15:45:46]

https://codereview.chromium.org/661693003/diff/60001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_level_calculator.h (right):

https://codereview.chromium.org/661693003/diff/60001/content/renderer/media/m...
content/renderer/media/media_stream_audio_level_calculator.h:25: // report
nonzero energy. Since |audio_data| is post processed data, and the
On 2014/10/20 15:40:17, tommi wrote:
> // report nonzero energy even if the energy of the processed audio is zero.
> <etc>

Done.

[no longer working on chromium, 2014-10-20 15:45:56]

thanks, landing now.

[no longer working on chromium, 2014-10-20 15:46:00]

The CQ bit was checked by xians@chromium.org

[commit-bot: I haz the power, 2014-10-20 15:47:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/661693003/100001

[commit-bot: I haz the power, 2014-10-20 16:35:03]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2014-10-20 16:35:51]

Patchset 5 (id:??) landed as
https://crrev.com/ba5b0d78589ce6b41bade5f71f5f9657c2beef3d
Cr-Commit-Position: refs/heads/master@{#300274}