Add a noise indicator to the speech bubble volume indicator.

Add a noise indicator to the speech bubble volume indicator.

The noise indicator is drawn as a light blue area at the beginning and if there
was clipping that is denoted with a red area at the end of the meter. The noise
level comes from the endpointer -> SpeechRecognizer -> SpeechInputBubbleController -> SpeechInputBubble
hence a bunch of volume setting methods are updated with the new parameter.

I have also added a new utility method to SpeechInputManager to invoke the platform
provided microphone settings UI, this will be used in the next CL which contains
windows, mac and linux specific UI changes.

BUG=69886
TEST=manual, invoke speech input and check the bubble volume indicator to see background noise and clipping.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=76395

[Satish, 2011-03-01 16:44:17]

http://codereview.chromium.org/6597071/diff/1013/content/browser/speech/speec...
File content/browser/speech/speech_recognizer_unittest.cc (right):

http://codereview.chromium.org/6597071/diff/1013/content/browser/speech/speec...
content/browser/speech/speech_recognizer_unittest.cc:90: audio_packet_[i] =
static_cast<uint8>(base::RandInt(0, 50));
Please ignore this part, I should be hardcoding the values here with some
calculations instead of using base::RandInt(), because there is no way to seed
it with the same initial value and on each test run this generates different
values (hence the test fails).

[hans, 2011-03-01 17:25:25]

lgtm barring green try bots

http://codereview.chromium.org/6597071/diff/1013/chrome/browser/speech/speech...
File chrome/browser/speech/speech_input_bubble.h (right):

http://codereview.chromium.org/6597071/diff/1013/chrome/browser/speech/speech...
chrome/browser/speech/speech_input_bubble.h:157: void
DrawVolumeOverlay(SkCanvas& canvas, SkBitmap* bitmap, float volume);
nit: spontaneously, i would have made |bitmap| a const ref if you're not going
to change it? but i suppose it doesn't really matter

http://codereview.chromium.org/6597071/diff/1013/chrome/browser/speech/speech...
File chrome/browser/speech/speech_input_manager.cc (right):

http://codereview.chromium.org/6597071/diff/1013/chrome/browser/speech/speech...
chrome/browser/speech/speech_input_manager.cc:16: #include
"chrome/browser/browser_thread.h"
nit: alpha include order

http://codereview.chromium.org/6597071/diff/1013/content/browser/speech/speec...
File content/browser/speech/speech_recognizer_unittest.cc (right):

http://codereview.chromium.org/6597071/diff/1013/content/browser/speech/speec...
content/browser/speech/speech_recognizer_unittest.cc:302:
EXPECT_FLOAT_EQ(0.36962864f, noise_volume_);
maybe make these magic numbers into constant variables?

[bulach, 2011-03-01 17:39:04]

LGTM

a few comments and suggestions below, and of course, make the bots
deterministically happy! :)

http://codereview.chromium.org/6597071/diff/1013/chrome/browser/speech/speech...
File chrome/browser/speech/speech_input_bubble.cc (right):

http://codereview.chromium.org/6597071/diff/1013/chrome/browser/speech/speech...
chrome/browser/speech/speech_input_bubble.cc:132: SkScalar clip_right = (((1.0f
- volume) * (width * 13)) - width) / 12.0f;
it'd be great to have these magical 13 and 12.0 as named constants..

http://codereview.chromium.org/6597071/diff/1013/chrome/browser/speech/speech...
chrome/browser/speech/speech_input_bubble.cc:135:
buffer_canvas.drawBitmap(*bitmap, 0, 0);
I guess bitmap can be const&

http://codereview.chromium.org/6597071/diff/1013/chrome/browser/speech/speech...
chrome/browser/speech/speech_input_bubble.cc:141:
canvas.drawBitmap(*buffer_image_.get(), 0, 0);
and canvas a *

http://codereview.chromium.org/6597071/diff/1013/chrome/browser/speech/speech...
File chrome/browser/speech/speech_input_bubble.h (right):

http://codereview.chromium.org/6597071/diff/1013/chrome/browser/speech/speech...
chrome/browser/speech/speech_input_bubble.h:101: // Updates the current captured
audio volume displayed on screen.
perhaps "Updates and draws the current..."

http://codereview.chromium.org/6597071/diff/1013/chrome/browser/speech/speech...
chrome/browser/speech/speech_input_bubble.h:157: void
DrawVolumeOverlay(SkCanvas& canvas, SkBitmap* bitmap, float volume);
either SkCanvas* or const SkCanvas&

http://codereview.chromium.org/6597071/diff/1013/chrome/browser/speech/speech...
File chrome/browser/speech/speech_input_manager.cc (right):

http://codereview.chromium.org/6597071/diff/1013/chrome/browser/speech/speech...
chrome/browser/speech/speech_input_manager.cc:198:
DCHECK(AudioManager::GetAudioManager()->CanShowAudioInputSettings());
how expensive is this call?
if it's cheap, you may want to avoid round tripping the threads and checking it
before PostTask..

http://codereview.chromium.org/6597071/diff/1013/content/browser/speech/endpo...
File content/browser/speech/endpointer/endpointer.h (right):

http://codereview.chromium.org/6597071/diff/1013/content/browser/speech/endpo...
content/browser/speech/endpointer/endpointer.h:100: float NoiseLevelDB() const {
return energy_endpointer_.GetNoiseLevelDB(); }
s/DB/dB/ :)

http://codereview.chromium.org/6597071/diff/1013/content/browser/speech/endpo...
File content/browser/speech/endpointer/energy_endpointer.cc (right):

http://codereview.chromium.org/6597071/diff/1013/content/browser/speech/endpo...
content/browser/speech/endpointer/energy_endpointer.cc:331: if (rms > 1.0e-100)
again, it may be nicer to define these magic consts with names..

http://codereview.chromium.org/6597071/diff/1013/content/browser/speech/endpo...
content/browser/speech/endpointer/energy_endpointer.cc:340: return levelDB;
ditto, dB and named constants..

http://codereview.chromium.org/6597071/diff/1013/content/browser/speech/speec...
File content/browser/speech/speech_recognizer.cc (right):

http://codereview.chromium.org/6597071/diff/1013/content/browser/speech/speec...
content/browser/speech/speech_recognizer.cc:38: ++clipping_samples;
you may want to short circuit rather than traverse the whole thing all the
time..
const int kThreshold = num_samples / 20;
for (...) {
  if (samples[i] ... ) {
    if (++clipping_samples > kThreshold)
      return true;
  }
}
return false;

http://codereview.chromium.org/6597071/diff/1013/content/browser/speech/speec...
File content/browser/speech/speech_recognizer_unittest.cc (right):

http://codereview.chromium.org/6597071/diff/1013/content/browser/speech/speec...
content/browser/speech/speech_recognizer_unittest.cc:90: audio_packet_[i] =
static_cast<uint8>(base::RandInt(0, 50));
On 2011/03/01 16:44:17, Satish wrote:
> Please ignore this part, I should be hardcoding the values here with some
> calculations instead of using base::RandInt(), because there is no way to seed
> it with the same initial value and on each test run this generates different
> values (hence the test fails).

yay for deterministic tests! :)

[Satish, 2011-03-01 18:12:06]

Thanks for the quick reviews. I have addressed all comments in the latest patch
and will wait for try runs to go green before submitting.

[jam, 2011-03-01 19:49:11]

http://codereview.chromium.org/6597071/diff/15/chrome/browser/speech/speech_i...
File chrome/browser/speech/speech_input_manager.cc (right):

http://codereview.chromium.org/6597071/diff/15/chrome/browser/speech/speech_i...
chrome/browser/speech/speech_input_manager.cc:189: // processes, do that in the
PROCESS_LAUNCHER thread to not block the calling
how long can AudioManager::ShowAudioInputSettings take?  if it blocks while the
user interacts with some UI, then we don't want it blocking the PROCESS_LAUNCHER
thread in the meantime, since that would block any new renderer/plugin processes
from starting.

the process launcher thread is added to not block the ui on starting new
processes.  it shouldn't really have additional tasks put on it.

[Satish, 2011-03-01 20:31:21]

http://codereview.chromium.org/6597071/diff/15/chrome/browser/speech/speech_i...
File chrome/browser/speech/speech_input_manager.cc (right):

http://codereview.chromium.org/6597071/diff/15/chrome/browser/speech/speech_i...
chrome/browser/speech/speech_input_manager.cc:189: // processes, do that in the
PROCESS_LAUNCHER thread to not block the calling
On 2011/03/01 19:49:11, John Abd-El-Malek wrote:
> how long can AudioManager::ShowAudioInputSettings take?  if it blocks while
the
> user interacts with some UI, then we don't want it blocking the
PROCESS_LAUNCHER
> thread in the meantime, since that would block any new renderer/plugin
processes
> from starting.
> 
> the process launcher thread is added to not block the ui on starting new
> processes.  it shouldn't really have additional tasks put on it.

ShowAudioInputSettings does a check to figure out what command line to use for
invoking the microphone settings panel on each platform (gets winver on windows,
checks current window manager on linux) and then launches that command line. So
seems like PROCESS_LAUNCHER is a good fit for this case?

[jam, 2011-03-01 20:43:50]

On Tue, Mar 1, 2011 at 12:31 PM, <satish@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6597071/diff/15/chrome/browser/speech/speech_i...
> File chrome/browser/speech/speech_input_manager.cc (right):
>
>
>
http://codereview.chromium.org/6597071/diff/15/chrome/browser/speech/speech_i...
> chrome/browser/speech/speech_input_manager.cc:189: // processes, do that
> in the PROCESS_LAUNCHER thread to not block the calling
> On 2011/03/01 19:49:11, John Abd-El-Malek wrote:
>
>> how long can AudioManager::ShowAudioInputSettings take?  if it blocks
>>
> while the
>
>> user interacts with some UI, then we don't want it blocking the
>>
> PROCESS_LAUNCHER
>
>> thread in the meantime, since that would block any new renderer/plugin
>>
> processes
>
>> from starting.
>>
>
>  the process launcher thread is added to not block the ui on starting
>>
> new
>
>> processes.  it shouldn't really have additional tasks put on it.
>>
>
> ShowAudioInputSettings does a check to figure out what command line to
> use for invoking the microphone settings panel on each platform (gets
> winver on windows, checks current window manager on linux) and then
> launches that command line. So seems like PROCESS_LAUNCHER is a good fit
> for this case?


Does it return right away?  What if the binary is large, dlls aren't loaded
etc, does it wait on all that before returning?  If so, better to use FILE
thread instead, and to keep PROCSS_LAUNCHER responsive in case the user
opens another tab.  PROCESS_LAUNCHER is meant to launch only chrome
processes.  For launching files (i.e. download shelf), we use the FILE
thread.


>
> http://codereview.chromium.org/6597071/
>

[Satish, 2011-03-01 21:01:35]

> Does it return right away?  What if the binary is large, dlls aren't loaded
> etc, does it wait on all that before returning?  If so, better to use FILE
> thread instead, and to keep PROCSS_LAUNCHER responsive in case the user
> opens another tab.  PROCESS_LAUNCHER is meant to launch only chrome
> processes.  For launching files (i.e. download shelf), we use the FILE
> thread.

I use base::LaunchApp() to start the external process. On windows I see that
uses CreateProcess which supposedly returns before the process initialization
happens. However if PROCESS_LAUNCHER is for launching chrome processes only,
I'll change the code to use the FILE thread instead. Thanks for the suggestion.

[jam, 2011-03-01 21:38:13]

On Tue, Mar 1, 2011 at 1:01 PM, <satish@chromium.org> wrote:

> Does it return right away?  What if the binary is large, dlls aren't loaded
>> etc, does it wait on all that before returning?  If so, better to use FILE
>> thread instead, and to keep PROCSS_LAUNCHER responsive in case the user
>> opens another tab.  PROCESS_LAUNCHER is meant to launch only chrome
>> processes.  For launching files (i.e. download shelf), we use the FILE
>> thread.
>>
>
> I use base::LaunchApp() to start the external process. On windows I see
> that
> uses CreateProcess which supposedly returns before the process
> initialization
> happens. However if PROCESS_LAUNCHER is for launching chrome processes
> only,
> I'll change the code to use the FILE thread instead. Thanks for the
> suggestion.


Thanks.  I'll update browser_thread.h to make this point clear.

On Windows, the average time to load a Chrome process was ~50ms, and on
Linux it sometimes went up to nearly 1s the first time.


>
>
> http://codereview.chromium.org/6597071/
>